Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Proof of concept for content library on organisation level #14864

Merged
merged 63 commits into from
Mar 6, 2025

Conversation

TomasEng
Copy link
Contributor

@TomasEng TomasEng commented Mar 4, 2025

Description

This pull request contains a proof-of-concept for the organisation library. Currently it works only for the TTD organisation and it is not possible to reference the resources in apps.

Main changes:

  • The setup script creates a repository for the library.
  • CRUD endpoints for code lists and text resources in the content library repository.
  • New content library page with code lists on dashboard level. It does not include text resource references from the code lists.
  • Routing between the app dashboard and the content library on the dashboard (we should consider some renaming here to distinguish dashboard from app-dashboard).
  • A new feature flag named orgLibrary that hides all this by default.

image

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Summary by CodeRabbit

  • New Features
    • Enabled organization-level management of content, allowing users to create, update, delete, and upload code lists and text resources.
    • Advanced dashboard routing to support context-specific views between the application dashboard and organization library.
  • Enhancements
    • Improved error messaging and notifications during content operations.
    • Updated and expanded Norwegian interface translations to enhance user guidance and clarity.

TomasEng and others added 30 commits January 28, 2025 12:36
…14605)

Co-authored-by: andreastanderen <[email protected]>
Co-authored-by: Erling Hauan <[email protected]>
Co-authored-by: Erling Hauan <[email protected]>
# Conflicts:
#	frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx
#	frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/utils/codeListPageUtils.test.ts
#	frontend/packages/shared/src/api/paths.js
@TomasEng TomasEng marked this pull request as ready for review March 5, 2025 10:55
@TomasEng TomasEng linked an issue Mar 5, 2025 that may be closed by this pull request
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🔭 Outside diff range comments (1)
backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListTrailingComma.json (1)

1-4: ⚠️ Potential issue

Fix invalid JSON syntax with trailing comma

The JSON file contains a trailing comma after the last array element, which is invalid according to the JSON specification. This could cause parsing errors in strict JSON parsers.

Fix the array by removing the trailing comma:

[
-    { "label": "someLabel", "value":  "someValue"},
+    { "label": "someLabel", "value":  "someValue"}
]
🧰 Tools
🪛 Biome (1.9.4)

[error] 2-3: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)

🧹 Nitpick comments (55)
frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.ts (1)

89-89: Simplify regular expression pattern

The implementation is clean and concise, but the global flag (g) in the regular expression is unnecessary since we're only matching at the beginning of the string (there can be at most one match).

- static removeLeadingSlash = (str: string): string => str.replace(/^\//g, '');
+ static removeLeadingSlash = (str: string): string => str.replace(/^\//, '');
frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts (1)

1-7: Convert static-only class to standalone functions

Following best practices and static analysis suggestions, consider refactoring this class with only static members to a module with exported functions. This approach is more aligned with functional programming principles and results in simpler, more maintainable code.

- export class FileUtils {
-   static convertToFormData = (file: File): FormData => {
-     const formData = new FormData();
-     formData.append('file', file);
-     return formData;
-   };
- }

+ /**
+  * Converts a File object to a FormData object with the file appended under the key 'file'.
+  * @param file The file to convert to FormData.
+  * @returns A FormData object containing the file.
+  */
+ export const convertToFormData = (file: File): FormData => {
+   const formData = new FormData();
+   formData.append('file', file);
+   return formData;
+ };
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-7: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)

frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (1)

1-17: Well-structured test for the FileUtils module.

The test is focused, concise, and clearly verifies the functionality of the convertToFormData method. It follows good testing practices by setting up test inputs, executing the function under test, and making appropriate assertions.

Consider enhancing this test suite with additional test cases for edge conditions (e.g., empty files, large files) in the future to ensure robust handling of all possible scenarios.

frontend/dashboard/hooks/useSubRoute/useSubroute.ts (1)

1-1: Minor: Inconsistent file naming and export.

There's a small inconsistency between the file name (useSubroute.ts) and the exported function name (useSubroute). Consider aligning these for better maintainability.

Also applies to: 10-10

frontend/dashboard/pages/OrgContentLibrary/utils.ts (1)

1-10: Clean utility implementation with good type safety.

The isOrg function is well-designed and follows best practices:

  • Clear naming that indicates what it returns when true
  • Proper type definition for the parameter
  • Efficient implementation using array includes method
  • Good separation of concerns by handling just one specific check

Consider making the function more maintainable for future changes by using a type guard pattern:

-export function isOrg(contextType: string): boolean {
+export function isOrg(contextType: string): contextType is string {
  const notOrgContexts: string[] = [
    SelectedContextType.Self,
    SelectedContextType.All,
    SelectedContextType.None,
  ];
  return !notOrgContexts.includes(contextType);
}

This would help TypeScript understand that contextType is a string that's not one of the SelectedContextType values, improving type safety in consuming code.

frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.ts (1)

9-22: Consider adding error handling to the mutation.

The hook is well-structured and follows React Query best practices. However, it doesn't include any error handling logic.

Consider adding an onError handler to provide a more robust implementation:

 return useMutation({
   mutationFn,
   onSuccess: (newData: CodeListsResponse) => {
     queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData);
   },
+  onError: (error) => {
+    // Handle error - e.g., logging, notifications, etc.
+    console.error('Failed to create code list:', error);
+  },
 });
frontend/dashboard/dashboardTestUtils.tsx (1)

33-37: Consider making initialEntries optional with a default value.

While the MemoryRouter component can handle undefined initialEntries, it would be more robust to provide a default value.

Consider providing a default value for initialEntries:

- <MemoryRouter initialEntries={initialEntries}>
+ <MemoryRouter initialEntries={initialEntries || ['/']}>

This ensures the router always has at least one entry, even if initialEntries is undefined.

frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.test.ts (2)

7-7: Consider using more descriptive mock data.

While the languageMock is functional, consider using a more descriptive name or adding a comment explaining that 'nb' represents Norwegian Bokmål for better readability.

-const languageMock: string = 'nb';
+// 'nb' represents Norwegian Bokmål language code
+const languageMock: string = 'nb';

12-22: Good basic test coverage.

The test verifies that the mutation calls the API with the correct parameters. Consider adding a test case for error handling scenarios to make the test suite more comprehensive.

You could add an additional test case that simulates an API error to verify the mutation handles errors correctly:

it('Handles API errors correctly', async () => {
  const errorMock = new Error('API Error');
  const createTextResourcesForOrgMock = jest.fn(() => Promise.reject(errorMock));
  
  const { result } = renderHookWithProviders(() =>
    useCreateTextResourcesForOrgMutation(org, languageMock),
    {
      queries: { createTextResourcesForOrg: createTextResourcesForOrgMock },
    }
  );

  result.current.mutate();
  await waitFor(() => expect(result.current.isError).toBe(true));
  
  expect(result.current.error).toBe(errorMock);
});
frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.ts (1)

1-18: Well-implemented mutation hook with proper cache management.

The hook follows React Query best practices:

  • Clear separation of concerns
  • Properly updates the query cache on success
  • Type-safe implementation

Consider adding error handling to improve robustness:

 return useMutation({
   mutationFn,
   onSuccess: (newData: CodeListsResponse) => {
     queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData);
   },
+  onError: (error) => {
+    // Log error or show notification
+    console.error('Failed to delete code list:', error);
+  },
 });
frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.test.ts (1)

12-28: Comprehensive test coverage for the mutation hook.

The test adequately verifies:

  • Correct mutation execution
  • Proper payload passing
  • Success state handling

Consider adding tests for error scenarios:

it('Handles errors correctly', async () => {
  queriesMock.updateTextResourcesForOrg.mockRejectedValueOnce(new Error('Update failed'));
  
  const { result } = renderHookWithProviders(() =>
    useUpdateTextResourcesForOrgMutation(org, languageMock),
  );

  result.current.mutate(textResourcesMock);
  await waitFor(() => expect(result.current.isError).toBe(true));
  
  expect(result.current.error).toBeDefined();
});
frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.test.ts (1)

45-59: Consider adding error case testing.

While the current test effectively verifies the cache update on success, adding tests for error handling would make the test suite more comprehensive.

  it('Replaces cache with api response', async () => {
    const queryClient = createQueryClientMock();
    queryClient.setQueryData([QueryKey.OrgCodeLists, org], [existingCodeList]);
    const createCodeListForOrg = jest.fn(() => Promise.resolve([existingCodeList, newCodeList]));
    const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org), {
      queryClient,
      queries: { createCodeListForOrg },
    });

    await result.current.mutateAsync(newCodeList);

    const expectedUpdatedData: CodeListsResponse = [existingCodeList, newCodeList];
    const updatedData = queryClient.getQueryData([QueryKey.OrgCodeLists, org]);
    expect(updatedData).toEqual(expectedUpdatedData);
  });
+
+  it('Handles error when API call fails', async () => {
+    const error = new Error('API error');
+    const createCodeListForOrg = jest.fn(() => Promise.reject(error));
+    const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org), {
+      queries: { createCodeListForOrg },
+    });
+
+    await expect(result.current.mutateAsync(newCodeList)).rejects.toEqual(error);
+    expect(createCodeListForOrg).toHaveBeenCalledTimes(1);
+  });
frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.test.ts (1)

41-54: Consider adding error handling test.

While the test effectively verifies the successful path, adding a test for error handling would make the test suite more comprehensive.

  it('Replaces cache with api response', async () => {
    const queryClient = createQueryClientMock();
    const uploadCodeListForOrg = jest.fn(() => Promise.resolve(codeListResponse));
    const { result } = renderHookWithProviders(() => useUploadOrgCodeListMutation(org), {
      queryClient,
      queries: { uploadCodeListForOrg },
    });

    await result.current.mutateAsync(file);

    const expectedUpdatedData: CodeListsResponse = codeListResponse;
    const updatedData = queryClient.getQueryData([QueryKey.OrgCodeLists, org]);
    expect(updatedData).toEqual(expectedUpdatedData);
  });
+
+  it('Handles error when API call fails', async () => {
+    const error = new Error('Upload failed');
+    const uploadCodeListForOrg = jest.fn(() => Promise.reject(error));
+    const { result } = renderHookWithProviders(() => useUploadOrgCodeListMutation(org), {
+      queries: { uploadCodeListForOrg },
+    });
+
+    await expect(result.current.mutateAsync(file)).rejects.toEqual(error);
+    expect(uploadCodeListForOrg).toHaveBeenCalledTimes(1);
+  });
frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.ts (1)

6-19: Consider adding error handling.

The mutation doesn't include any error handling. It's recommended to add onError callback or error handling within the mutationFn to provide better feedback to users when operations fail.

export const useCreateTextResourcesForOrgMutation = (org: string, language: string) => {
  const q = useQueryClient();
  const { createTextResourcesForOrg } = useServicesContext();
  return useMutation({
    mutationFn: async () => {
      const textResourcesWithLanguage: ITextResourcesWithLanguage[] =
        await createTextResourcesForOrg(org, language);

      return textResourcesWithLanguage;
    },
    onSuccess: (textResourcesWithLanguage) =>
      q.setQueryData([QueryKey.TextResourcesForOrg, org], textResourcesWithLanguage),
+    onError: (error: Error) => {
+      console.error('Failed to create text resources:', error);
+      // Optionally add toast notification or other error handling here
+    },
  });
};
frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.ts (1)

6-19: Well-implemented mutation hook with proper error handling.

The hook follows React Query best practices:

  1. Properly uses useQueryClient to access the query cache
  2. Correctly structures the mutation function with async/await
  3. Handles the response and updates the query cache on success
  4. Returns the mutation object for use in components

There is one improvement opportunity regarding error handling:

Consider adding an onError handler to provide better error feedback or logging:

export const useUpdateTextResourcesForOrgMutation = (org: string, language: string) => {
  const q = useQueryClient();
  const { updateTextResourcesForOrg } = useServicesContext();
  return useMutation({
    mutationFn: async (payload: ITextResource[]) => {
      const textResourcesWithLanguage: ITextResourcesWithLanguage[] =
        await updateTextResourcesForOrg(org, language, payload);

      return textResourcesWithLanguage;
    },
    onSuccess: (textResourcesWithLanguage) =>
      q.setQueryData([QueryKey.TextResourcesForOrg, org], textResourcesWithLanguage),
+   onError: (error) => {
+     // Log or handle error appropriately
+     console.error('Failed to update text resources:', error);
+   },
  });
};
backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs (1)

31-41: Document precondition for UpdateTextsForKeys method

The method signature and documentation look good, but there's an important precondition missing: this method requires that the text resource already exists. This constraint should be documented to ensure proper usage.

/// <summary>
-/// Updates values for specified keys in the text resouce.
+/// Updates values for specified keys in the text resource.
+/// The text resource must already exist before calling this method.
/// </summary>
/// <param name="org">Organisation</param>
/// <param name="developer">Username of developer</param>
/// <param name="keysTexts">KeysTexts</param>
/// <param name="languageCode">LanguageCode</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> that observes if operation is cancelled.</param>
/// <returns></returns>

I'm suggesting this based on a learning from PR #14583 which indicates that the implementation of this method assumes the resource already exists.

frontend/dashboard/pages/CreateService/CreateService.test.tsx (1)

329-339: Consider testing with an actual org identifier.

While this test correctly verifies routing for the "org" context using "ttd", you might want to add a test with a different organization name to verify the functionality works for any valid org identifier.

 it('should set cancel link to /org when selected context is org', async () => {
-  const selectedContext = 'ttd';
+  // Test with a specific organization identifier
+  const organizationId = 'ttd';
+  const selectedContext = organizationId;
   const subroute = Subroute.AppDashboard;
   (useParams as jest.Mock).mockReturnValue({ selectedContext, subroute });

   renderWithMockServices();
   const cancelLink: HTMLElement = screen.getByRole('link', {
     name: textMock('general.cancel'),
   });

   expect(cancelLink.getAttribute('href')).toBe(`/${subroute}/${selectedContext}`);
 });
frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.test.tsx (1)

220-222: Consider testing navigation with different subroutes.

While the current test confirms navigation works for AppDashboard, it would be beneficial to also test navigation with the OrgLibrary subroute to ensure all navigation paths work as expected.

 it('should navigate to the "Self" context when the "Self" menu item is clicked', async () => {
   const user = userEvent.setup();
   (useParams as jest.Mock).mockReturnValue({
     selectedContext: SelectedContextType.All,
     subroute: Subroute.AppDashboard,
   });

   renderDashboardHeader();

   const avatarButton = screen.getByRole('button', { name: userMock.full_name });
   await user.click(avatarButton);

   const selfItem = screen.getByRole('menuitemradio', { name: userMock.full_name });
   await user.click(selfItem);

   expect(mockNavigate).toHaveBeenCalledWith(
     `${Subroute.AppDashboard}/${SelectedContextType.Self}`,
   );
   expect(mockNavigate).toHaveBeenCalledTimes(1);
 });

+it('should navigate to the "Self" context with OrgLibrary subroute when in library view', async () => {
+  const user = userEvent.setup();
+  (useParams as jest.Mock).mockReturnValue({
+    selectedContext: SelectedContextType.All,
+    subroute: Subroute.OrgLibrary,
+  });
+
+  renderDashboardHeader();
+
+  const avatarButton = screen.getByRole('button', { name: userMock.full_name });
+  await user.click(avatarButton);
+
+  const selfItem = screen.getByRole('menuitemradio', { name: userMock.full_name });
+  await user.click(selfItem);
+
+  expect(mockNavigate).toHaveBeenCalledWith(
+    `${Subroute.OrgLibrary}/${SelectedContextType.Self}`,
+  );
+  expect(mockNavigate).toHaveBeenCalledTimes(1);
+});
backend/src/Designer/Controllers/Preview/InstancesController.cs (1)

190-194: Consider supporting organization-level content library.

Given the PR objective to create a content library at the organization level, you might want to make the OptionsFolderPath dynamic based on whether the options are at the app level or organization level.

 string developer = AuthenticationHelper.GetDeveloperUserName(HttpContext);
 AltinnAppGitRepository altinnAppGitRepository = altinnGitRepositoryFactory.GetAltinnAppGitRepository(org, app, developer);
-string options = await altinnAppGitRepository.GetOptionsList(optionListId, OptionsFolderPath, cancellationToken);
+
+// Check if this options list should use organization level content library
+bool useOrgLibrary = false; // Logic to determine if org library should be used
+string options;
+
+if (useOrgLibrary)
+{
+    var orgRepository = altinnGitRepositoryFactory.GetAltinnOrgGitRepository(org, "content-library", developer);
+    options = await orgRepository.GetOptionsList(optionListId, "options/", cancellationToken);
+}
+else
+{
+    options = await altinnAppGitRepository.GetOptionsList(optionListId, OptionsFolderPath, cancellationToken);
+}
 return Ok(options);
backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs (1)

26-32: Consider specifying repository naming conventions in documentation.

Given that this method is specifically for accessing organization-level content libraries, it would be helpful to document any naming conventions or expected format for the repository parameter, especially since it appears to have a special meaning in this context.

 /// <summary>
 /// Creates an instance of <see cref="AltinnOrgGitRepository"/>
 /// </summary>
 /// <param name="org">The organization owning the repository identified by its short name as defined in Gitea.</param>
-/// <param name="repository">The name of the repository as specified in Gitea.</param>
+/// <param name="repository">The name of the repository as specified in Gitea. For organization content libraries, this is typically "content-library".</param>
 /// <param name="developer">The user name of the developer working on the repository.</param>
 AltinnOrgGitRepository GetAltinnOrgGitRepository(string org, string repository, string developer);
backend/tests/Designer.Tests/Controllers/OrgCodeListController/DeleteCodeListTests.cs (2)

24-45: Add verification that the code list exists before deletion

The test validates that the code list is deleted but doesn't verify that it existed before the delete operation. This validation would strengthen the test by ensuring the deletion is actually removing an existing item.

Consider adding a verification step before the delete operation:

// Arrange
const string codeListId = "codeListNumber";
string targetOrg = TestDataHelper.GenerateTestOrgName();
string targetRepository = TestDataHelper.GetOrgContentRepoName(targetOrg);
await CopyOrgRepositoryForTest(Developer, Org, Repo, targetOrg, targetRepository);

+// Verify code list exists before deletion
+string getApiUrl = $"designer/api/{targetOrg}/code-lists";
+using HttpRequestMessage getRequest = new(HttpMethod.Get, getApiUrl);
+using HttpResponseMessage getResponse = await HttpClient.SendAsync(getRequest);
+string getResponseBody = await getResponse.Content.ReadAsStringAsync();
+List<OptionListData> initialList = JsonSerializer.Deserialize<List<OptionListData>>(getResponseBody);
+Assert.Contains(initialList, e => e.Title == codeListId);

string apiUrl = ApiUrl(targetOrg, codeListId);
using HttpRequestMessage httpRequestMessage = new(HttpMethod.Delete, apiUrl);

14-19: Add test for authentication/authorization scenarios

The current tests verify successful deletion and handling of non-existent resources, but don't cover authentication or authorization failures.

Consider adding tests for scenarios like unauthorized access:

[Fact]
public async Task Delete_Returns_401Unauthorized_When_User_Not_Authenticated()
{
    // Arrange
    const string codeListId = "codeListNumber";
    string targetOrg = TestDataHelper.GenerateTestOrgName();
    
    // Create client without authentication
    var clientWithoutAuth = Factory.CreateClient();
    
    string apiUrl = ApiUrl(targetOrg, codeListId);
    using HttpRequestMessage httpRequestMessage = new(HttpMethod.Delete, apiUrl);

    // Act
    using HttpResponseMessage response = await clientWithoutAuth.SendAsync(httpRequestMessage);
    
    // Assert
    Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
}
backend/tests/Designer.Tests/Controllers/OrgTextController/CreateResourceTests.cs (1)

37-38: Add assertion for response content

The test verifies the status code and file existence but does not check the response content from the API. This would provide more complete validation of the API behavior.

Add an assertion to check the response content:

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+string responseContent = await response.Content.ReadAsStringAsync();
+Assert.NotEmpty(responseContent);
+Assert.Contains("success", responseContent.ToLower());
Assert.True(TestDataHelper.FileExistsInRepo(targetOrg, targetRepository, developer, RelativePath(lang)));
Assert.True(JsonUtils.DeepEquals(payload, TestDataHelper.GetFileFromRepo(targetOrg, targetRepository, developer, RelativePath(lang))));
frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (2)

78-81: Improve URL path extraction with more robust implementation

The current implementation of extractSecondLastRouterParam might be fragile if URL patterns change. Consider using a more robust approach that handles edge cases like URLs with or without trailing slashes.

-function extractSecondLastRouterParam(pathname: string): string {
-  const pathnameArray = pathname.split('/');
-  return pathnameArray[pathnameArray.length - 2];
-}
+function extractSecondLastRouterParam(pathname: string): string {
+  // Remove leading and trailing slashes
+  const normalizedPath = pathname.replace(/^\/+|\/+$/g, '');
+  
+  // Split into path segments
+  const pathnameArray = normalizedPath.split('/');
+  
+  // If we have at least 2 segments, return the second-to-last
+  if (pathnameArray.length >= 2) {
+    return pathnameArray[pathnameArray.length - 2];
+  }
+  
+  // Otherwise return the last segment or empty string
+  return pathnameArray.length > 0 ? pathnameArray[pathnameArray.length - 1] : '';
+}

67-68: Simplify the className condition with a more readable approach

The current className assignment is somewhat complex. Consider using a more declarative approach to improve readability.

-<span
-  className={cn({
-    [classes.active]: StringUtils.removeLeadingSlash(menuItem.link) === currentRoutePath,
-  })}
->
+<span
+  className={
+    StringUtils.removeLeadingSlash(menuItem.link) === currentRoutePath
+      ? classes.active
+      : ''
+  }
+>
frontend/dashboard/app/App.test.tsx (2)

58-63: Add more specific assertions for dashboard page content

The test verifies that links are present but doesn't fully validate the structure or content of the dashboard page. More specific assertions would make the test more robust.

it('should display dashboard page when data are loaded', async () => {
  renderApp();
  await waitForElementToBeRemoved(querySpinner());
  expect(screen.getByRole('link', { name: textMock('dashboard.header_item_dashboard') }));
  expect(screen.getByRole('link', { name: textMock('dashboard.header_item_library') }));
+ 
+ // Verify dashboard structure
+ expect(screen.getByRole('heading', { name: textMock('dashboard.favourites') })).toBeInTheDocument();
+ expect(screen.getByRole('navigation')).toBeInTheDocument();
});

80-90: Add waitFor to handle asynchronous navigation

The test doesn't account for the asynchronous nature of navigation. Using waitFor would make the test more reliable.

it('should display the apps overview when the user is on the library page and clicks on the apps link', async () => {
  const user = userEvent.setup();
  const client = createQueryClientWithUserAndOrg();
  const initialEntries = [`${APP_DASHBOARD_BASENAME}/${org.username}`];
  renderApp({ client, initialEntries });
  await user.click(screen.getByRole('link', { name: textMock('dashboard.header_item_library') }));
  await user.click(
    screen.getByRole('link', { name: textMock('dashboard.header_item_dashboard') }),
  );
- expect(getFavouriteAppListHeading()).toBeInTheDocument();
+ await waitFor(() => {
+   expect(getFavouriteAppListHeading()).toBeInTheDocument();
+ });
});
backend/tests/Designer.Tests/Controllers/OrgCodeListController/UploadCodeListTests.cs (1)

53-53: Fix the spelling in the test method name
The word "Retuns" seems to be a typo. It should read "Returns" for consistency and clarity.

Apply this diff to correct the method name:

-[Fact]
-public async Task Post_Retuns_200OK_When_Uploading_New_CodeList_With_Empty_Strings()
+[Fact]
+public async Task Post_Returns_200OK_When_Uploading_New_CodeList_With_Empty_Strings()
backend/tests/Designer.Tests/Controllers/OrgCodeListController/CreateCodeListTests.cs (1)

60-66: Consider verifying all properties or adding comprehensive assertions
The loop checks basic label, value, description, and helpText. For a more robust test, consider verifying any additional properties or verifying the object fully.

frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (2)

51-84: Well-structured component with clear prop types.

The component has well-defined prop types and cleanly handles the setup of the content library implementation. The callbacks for updating, deleting, and uploading code lists are properly set up.

However, there's an empty callback for onUpdateCodeListId, which doesn't appear to be implemented yet.

Consider either implementing the onUpdateCodeListId functionality or adding a comment explaining why it's currently a no-op:

-          onUpdateCodeListId: () => {},
+          onUpdateCodeListId: () => {
+            // Not implemented in this phase - will be added in future iterations
+          },

100-120: Custom hook with missing test coverage.

The useUploadCodeList hook provides good error handling and success notifications. However, static analysis indicates line 102 is not covered by tests.

Consider adding a test case that triggers the hideDefaultError callback to ensure complete test coverage:

it('hides default error when error is unknown', async () => {
  const errorResponse = { response: {} }; // Unknown error without specific code
  const uploadCodeListForOrg = jest
    .fn()
    .mockImplementation(() => Promise.reject(errorResponse));
  
  renderOrgContentLibraryWithCodeLists({
    initialEntries: [mockOrgPath],
    queries: { uploadCodeListForOrg },
  });
  
  await goToLibraryPage(user, 'code_lists');
  const uploadCodeListButton = screen.getByRole('button', { name: uploadCodeListButtonTextMock });
  await user.click(uploadCodeListButton);
  
  // Verify hideDefaultError was called correctly
  expect(uploadCodeListForOrg).toHaveBeenCalledTimes(1);
  // Further assertions as needed
});
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 102-102: frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx#L102
Added line #L102 was not covered by tests

frontend/dashboard/app/App.tsx (3)

12-20: Consider adding a fallback handling for route constants or erroneous imports.
The new imports of constants, hooks, and utilities appear valid. However, ensure that any new or future subroute constants have corresponding handling in your routing logic. This helps prevent silent failures if a subroute is referenced but not handled.


31-39: Add a default case to the switch for unexpected query statuses.
While 'pending', 'error', and 'success' are the main statuses, considering a fallback error or a default case can handle unexpected situations gracefully.

  switch (queryStatus) {
    case 'pending':
      return <PendingPage />;
    case 'error':
      return <ErrorMessage userStatus={userStatus} />;
    case 'success':
      return <AppWithData user={user} organizations={organizations} />;
+   default:
+     // log or display a generic error or fallback here
  }

92-103: Consider a fallback return for unknown subroutes.
The SubrouteGuard handles two known subroutes but does nothing if the user hits an unknown subroute. A fallback case (e.g., redirect or error component) would enhance user experience.

  switch (subrouteWithLeadingSlash) {
    case APP_DASHBOARD_BASENAME:
      return <Dashboard {...props} />;
    case ORG_LIBRARY_BASENAME:
      return <OrgContentLibrary />;
+   default:
+     // Return an error, placeholder, or redirect for unknown subroutes
  }
backend/tests/Designer.Tests/Services/OrgTextsServiceTests.cs (1)

60-81: Ensure consistent language on method naming.
The method name SaveText_ShouldCreateTextFile is concise and clear. Consider continuing with the same naming pattern for all test methods, ensuring each method clearly states the scenario and the expected behavior. So far, the naming convention looks consistent.

backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnOrgGitRepositoryTests.cs (1)

57-86: Verify directory creation logic for code lists in actual implementation.
From prior learnings, you may use WriteTextByRelativePathAsync(createDirectory: true) or an equivalent approach to avoid manual directory creation. Consider adding an explicit test to confirm that missing directories are created automatically during code list operations.

backend/src/Designer/Controllers/Organisation/OrgTextController.cs (2)

44-45: Consider adding request handling logs.

When retrieving the developer username and performing operations, consider adding logs or structured telemetry to help troubleshoot potential issues. This can be especially useful if multiple developers are working in parallel.


112-115: Use HTTP 404 for missing resources.

The code currently returns a 400 Bad Request when the text resource is not found. Aligning with REST best practices, returning a 404 Not Found might be more appropriate for this scenario.

- return BadRequest($"The text resource, resource.{languageCode}.json, could not be updated.");
+ return NotFound($"The text resource, resource.{languageCode}.json, could not be updated.");
frontend/dashboard/hooks/guards/useContextRedirectionGuard.ts (2)

46-51: Consider using useLocation for URL query parameters.

Currently, the code references the global location object to append location.search. Consider using the react-router hook useLocation for consistent SSR behavior and easier testing.


88-103: Consolidate logic for context switching.

The isTargetContextDifferent check and subsequent condition partially duplicate the logic found in handleContextRedirection. Consider unifying or reducing duplication to simplify maintenance and ensure consistent behavior.

backend/tests/Designer.Tests/Controllers/OrgTextController/UpdateResourceTests.cs (2)

24-48: Increase test scenario coverage.

The MemberData array for the UpdateResource_Returns200OK_WithValidInput test currently includes a single scenario. Adding more test cases, such as multiple languages or organizations, an empty dictionary, or invalid keys, would help increase coverage.


50-77: Extend variable handling tests.

While the test confirms that existing variables remain intact, consider adding scenarios in which the dictionary modifies or removes variables. Testing these edge cases increases confidence in the solution's robustness.

backend/src/Designer/Controllers/Organisation/OrgCodeListController.cs (5)

39-62: Consider returning a different HTTP status for missing code lists.

Currently, a NotFoundException triggers a NoContent() response, which can be confusing for clients expecting a 404 status or an empty list with a 200 status. A more conventional approach would be to return either NotFound() or an empty list with a 200 status if no code lists are found, ensuring consistency with RESTful API practices.

Here's an example diff:

catch (NotFoundException)
{
-    return NoContent();
+    return NotFound("No code lists found for the specified organization.");
}

64-85: Use 201 (Created) for a newly created resource.

This endpoint creates (or overwrites) a code list. Returning 201 would be more explicit about a resource creation event, especially for a newly created file. Consider returning 201 where appropriate, and 200 only if the resource was updated but not created anew.


86-107: Clarify or unify the documentation for Create/Update endpoints.

Both the “CreateCodeList” and “UpdateCodeList” endpoints have very similar descriptions about overwriting existing files. It would be helpful to distinguish them clearly or consolidate the docs if both effectively perform the same action.


108-131: Align “Upload” method name with the doc description.

The method name “UploadCodeList” and summary stating "Creates a new code list" can be confusing. Consider renaming the method or updating the documentation to reflect a consistent action (e.g., "Upload and overwrite existing code list" or “CreateOrUpdateCodeListFromFile”).


132-156: Ensure consistent HTTP status codes for missing resources.

When a code list does not exist, this endpoint returns NotFound(), whereas the GetCodeLists endpoint returns NoContent(). Consistency in how the API responds to missing resources helps create a more predictable and aligned user experience.

frontend/packages/shared/src/api/mutations.ts (2)

183-188: Consider adding error handling for code list mutations.

These methods currently rely on simple server responses. If partial failures or specific errors need to be handled (e.g., invalid payload), you might want to add more fine-grained error handling or logs to improve maintainability and clarity.


190-192: Replace mocks with real API calls once endpoints are ready.

There is a TODO indicating these mocks will be replaced with real calls. Let me know if you would like assistance opening an issue to ensure these changes are tracked and prioritized.

backend/src/Designer/Services/Interfaces/Organisation/IOrgCodeListService.cs (2)

10-19: Revise the summary or method signature for GetCodeLists.

The summary references a code list by a specific ID, but the method’s signature and return type suggest multiple code lists and no specific ID parameter. Updating the documentation to reflect that it retrieves all code lists would clarify usage.


43-51: Match the method name with the summary for UploadCodeList.

The summary says "Creates a new code list" but the method name is "UploadCodeList," which can be ambiguous. Consider clarifying that it “Uploads/overwrites an existing code list file” to accurately describe the operation.

backend/tests/Designer.Tests/Services/OrgCodeListServiceTests.cs (1)

85-85: Typographical fix for clarity.

There's a small typo in the constant: "newCoodeList" → "newCodeList". Fixing it will avoid confusion.

- const string codeListId = "newCoodeList";
+ const string codeListId = "newCodeList";
backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (1)

138-154: Correct the parameter and comment typo for “code list.”

Lines 138 and 154 refer to “cost list” instead of “code list.” Updating the XML documentation will improve clarity and consistency.

- /// <param name="codeListId">The name of the cost list to update.</param>
+ /// <param name="codeListId">The name of the code list to update.</param>
- /// <param name="codeListId">The name of the cost list to be deleted.</param>
+ /// <param name="codeListId">The name of the code list to be deleted.</param>
backend/src/Designer/Services/Implementation/Organisation/OrgCodeListService.cs (2)

99-99: Edge case in file name handling.

Using .Replace(".json", "") can lead to unexpected results if the filename has multiple .json segments or does not end in .json. Consider using a more robust approach, such as Path.GetFileNameWithoutExtension(), to reliably extract the codeListId.

- string codeListId = payload.FileName.Replace(".json", "");
+ string codeListId = Path.GetFileNameWithoutExtension(payload.FileName);

76-78: Potential performance overhead from returning all code lists after each operation.

All Create/Update/Delete methods return the entire set of code lists instead of just the updated or newly created code list. While this might be intentional from a design perspective, it also adds overhead. Consider returning only the relevant code list to optimize performance.

Also applies to: 89-91, 112-114

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a6fc8b3 and b186c40.

📒 Files selected for processing (107)
  • backend/src/Designer/Controllers/Organisation/OrgCodeListController.cs (1 hunks)
  • backend/src/Designer/Controllers/Organisation/OrgTextController.cs (1 hunks)
  • backend/src/Designer/Controllers/Preview/InstancesController.cs (2 hunks)
  • backend/src/Designer/Controllers/Preview/V3/OldInstancesController.cs (2 hunks)
  • backend/src/Designer/Controllers/PreviewController.cs (2 hunks)
  • backend/src/Designer/Factories/AltinnGitRepositoryFactory.cs (1 hunks)
  • backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (4 hunks)
  • backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (1 hunks)
  • backend/src/Designer/Infrastructure/ServiceRegistration.cs (3 hunks)
  • backend/src/Designer/Services/Implementation/OptionsService.cs (3 hunks)
  • backend/src/Designer/Services/Implementation/Organisation/OrgCodeListService.cs (1 hunks)
  • backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs (1 hunks)
  • backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs (1 hunks)
  • backend/src/Designer/Services/Interfaces/Organisation/IOrgCodeListService.cs (1 hunks)
  • backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/ApiTests/DesignerEndpointsTestsBase.cs (3 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/CreateCodeListTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/DeleteCodeListTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/GetCodeListsTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/UpdateCodeListTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/UploadCodeListTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgTextController/CreateResourceTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgTextController/GetResourcesTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgTextController/UpdateResourceTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnAppGitRepositoryTests.cs (4 hunks)
  • backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnOrgGitRepositoryTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Services/OrgCodeListServiceTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Services/OrgTextsServiceTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Services/TextsServiceTest.cs (2 hunks)
  • backend/tests/Designer.Tests/Utils/TestDataHelper.cs (2 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content-empty/Codelists/.gitignore (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content-empty/Texts/.gitignore (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListBoolean.json (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListMissingLabel.json (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListMissingValue.json (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListNumber.json (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListString.json (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListTrailingComma.json (1 hunks)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Texts/resource.nb.json (1 hunks)
  • development/setup.js (1 hunks)
  • frontend/dashboard/app/App.test.tsx (2 hunks)
  • frontend/dashboard/app/App.tsx (1 hunks)
  • frontend/dashboard/components/NewApplicationForm/NewApplicationForm.tsx (1 hunks)
  • frontend/dashboard/context/HeaderContext/HeaderContext.tsx (0 hunks)
  • frontend/dashboard/context/HeaderContext/index.ts (1 hunks)
  • frontend/dashboard/dashboardTestUtils.tsx (3 hunks)
  • frontend/dashboard/enums/HeaderMenuItemKey.ts (1 hunks)
  • frontend/dashboard/enums/SelectedContextType.ts (1 hunks)
  • frontend/dashboard/enums/Subroute.ts (1 hunks)
  • frontend/dashboard/hooks/guards/useContextRedirectionGuard.ts (2 hunks)
  • frontend/dashboard/hooks/usePageHeaderTitle/usePageHeaderTitle.test.tsx (1 hunks)
  • frontend/dashboard/hooks/usePageHeaderTitle/usePageHeaderTitle.tsx (1 hunks)
  • frontend/dashboard/hooks/useProfileMenuTriggerButtonText/useProfileMenuTriggerButtonText.test.tsx (1 hunks)
  • frontend/dashboard/hooks/useProfileMenuTriggerButtonText/useProfileMenuTriggerButtonText.tsx (1 hunks)
  • frontend/dashboard/hooks/useSelectedContext/useSelectedContext.ts (1 hunks)
  • frontend/dashboard/hooks/useSubRoute/index.ts (1 hunks)
  • frontend/dashboard/hooks/useSubRoute/useSubroute.ts (1 hunks)
  • frontend/dashboard/pages/CreateService/CreateService.test.tsx (3 hunks)
  • frontend/dashboard/pages/CreateService/CreateService.tsx (3 hunks)
  • frontend/dashboard/pages/Dashboard/Dashboard.test.tsx (1 hunks)
  • frontend/dashboard/pages/Dashboard/Dashboard.tsx (3 hunks)
  • frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.test.tsx (1 hunks)
  • frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (1 hunks)
  • frontend/dashboard/pages/OrgContentLibrary/index.ts (1 hunks)
  • frontend/dashboard/pages/OrgContentLibrary/utils.test.ts (1 hunks)
  • frontend/dashboard/pages/OrgContentLibrary/utils.ts (1 hunks)
  • frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.module.css (1 hunks)
  • frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.test.tsx (6 hunks)
  • frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (6 hunks)
  • frontend/dashboard/pages/PageLayout/PageLayout.test.tsx (4 hunks)
  • frontend/dashboard/testing/mocks.tsx (4 hunks)
  • frontend/dashboard/types/HeaderMenuItem.ts (1 hunks)
  • frontend/dashboard/utils/filterUtils/filterUtils.test.ts (1 hunks)
  • frontend/dashboard/utils/filterUtils/filterUtils.ts (1 hunks)
  • frontend/dashboard/utils/headerUtils/headerUtils.ts (1 hunks)
  • frontend/dashboard/utils/headerUtils/index.ts (1 hunks)
  • frontend/dashboard/utils/repoUtils/repoUtils.test.ts (1 hunks)
  • frontend/dashboard/utils/repoUtils/repoUtils.ts (1 hunks)
  • frontend/dashboard/utils/userUtils/userUtils.test.ts (1 hunks)
  • frontend/dashboard/utils/userUtils/userUtils.ts (1 hunks)
  • frontend/language/src/nb.json (2 hunks)
  • frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx (1 hunks)
  • frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/utils/utils.test.ts (1 hunks)
  • frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/utils/utils.ts (1 hunks)
  • frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (1 hunks)
  • frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts (1 hunks)
  • frontend/libs/studio-pure-functions/src/FileUtils/index.ts (1 hunks)
  • frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.test.ts (1 hunks)
  • frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.ts (1 hunks)
  • frontend/libs/studio-pure-functions/src/index.ts (1 hunks)
  • frontend/packages/shared/src/api/mutations.ts (4 hunks)
  • frontend/packages/shared/src/api/paths.js (1 hunks)
  • frontend/packages/shared/src/api/queries.ts (3 hunks)
  • frontend/packages/shared/src/constants.js (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useAddOptionListMutation.test.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useAddOptionListMutation.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.test.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.test.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.test.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.test.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.ts (1 hunks)
  • frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.test.ts (1 hunks)
⛔ Files not processed due to max files limit (26)
  • frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.ts
  • frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.test.ts
  • frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.ts
  • frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.test.ts
  • frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.ts
  • frontend/packages/shared/src/mocks/codeListsResponse.ts
  • frontend/packages/shared/src/mocks/queriesMock.ts
  • frontend/packages/shared/src/mocks/textResourcesMock.ts
  • frontend/packages/shared/src/types/CodeList.ts
  • frontend/packages/shared/src/types/CodeListData.ts
  • frontend/packages/shared/src/types/QueryKey.ts
  • frontend/packages/shared/src/types/api/CodeListsResponse.ts
  • frontend/packages/shared/src/utils/featureToggleUtils.ts
  • frontend/testing/playwright/components/DashboardHeader.ts
  • frontend/testing/playwright/enum/AppNames.ts
  • frontend/testing/playwright/enum/TestNames.ts
  • frontend/testing/playwright/helpers/RouterRoute.ts
  • frontend/testing/playwright/pages/DashboardPage.ts
  • frontend/testing/playwright/pages/OrgLibraryPage/CodeLists.ts
  • frontend/testing/playwright/pages/OrgLibraryPage/OrgLibraryPage.ts
  • frontend/testing/playwright/pages/OrgLibraryPage/index.ts
  • frontend/testing/playwright/pages/OrgLibraryPage/testCodelist.json
  • frontend/testing/playwright/playwright.config.ts
  • frontend/testing/playwright/tests/dashboard/dashboard-navigation.spec.ts
  • frontend/testing/playwright/tests/org-library/codelists.spec.ts
  • frontend/testing/playwright/tests/org-library/org-library.spec.ts
💤 Files with no reviewable changes (1)
  • frontend/dashboard/context/HeaderContext/HeaderContext.tsx
✅ Files skipped from review due to trivial changes (26)
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content-empty/Texts/.gitignore
  • frontend/dashboard/pages/OrgContentLibrary/index.ts
  • frontend/dashboard/hooks/useSubRoute/index.ts
  • frontend/libs/studio-pure-functions/src/FileUtils/index.ts
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content-empty/Codelists/.gitignore
  • frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.module.css
  • frontend/dashboard/utils/repoUtils/repoUtils.ts
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListMissingValue.json
  • frontend/dashboard/hooks/usePageHeaderTitle/usePageHeaderTitle.test.tsx
  • frontend/dashboard/utils/headerUtils/index.ts
  • frontend/dashboard/enums/HeaderMenuItemKey.ts
  • frontend/dashboard/hooks/useSelectedContext/useSelectedContext.ts
  • frontend/dashboard/pages/Dashboard/Dashboard.test.tsx
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListNumber.json
  • frontend/dashboard/components/NewApplicationForm/NewApplicationForm.tsx
  • frontend/dashboard/utils/headerUtils/headerUtils.ts
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListBoolean.json
  • frontend/dashboard/enums/Subroute.ts
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListMissingLabel.json
  • frontend/dashboard/utils/userUtils/userUtils.ts
  • frontend/dashboard/utils/filterUtils/filterUtils.ts
  • frontend/dashboard/hooks/usePageHeaderTitle/usePageHeaderTitle.tsx
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListString.json
  • backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Texts/resource.nb.json
  • frontend/dashboard/utils/filterUtils/filterUtils.test.ts
  • frontend/dashboard/utils/userUtils/userUtils.test.ts
🧰 Additional context used
🧠 Learnings (6)
backend/src/Designer/Factories/AltinnGitRepositoryFactory.cs (2)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs:64-70
Timestamp: 2025-02-06T11:16:47.768Z
Learning: In Altinn Studio, when writing files using `WriteTextByRelativePathAsync`, set `createDirectory: true` to automatically create the directory structure if it doesn't exist, instead of manually creating directories.
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs:64-70
Timestamp: 2025-02-06T11:16:47.768Z
Learning: The `WriteTextByRelativePathAsync` method in Altinn Studio has a `createDirectory` parameter that can be set to true to automatically create directories when writing files.
backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs (1)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs:65-94
Timestamp: 2025-02-06T11:20:54.269Z
Learning: In the Altinn Studio Designer, the UpdateTextsForKeys method in OrgTextsService can only be called for text resources that have already been created. The resource's existence is a precondition for this method, and handling non-existent resources is unnecessary except for potential race conditions.
backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs (2)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs:65-94
Timestamp: 2025-02-06T11:20:54.269Z
Learning: In the Altinn Studio Designer, the UpdateTextsForKeys method in OrgTextsService can only be called for text resources that have already been created. The resource's existence is a precondition for this method, and handling non-existent resources is unnecessary except for potential race conditions.
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs:29-36
Timestamp: 2025-02-06T11:19:22.211Z
Learning: In ASP.NET Core applications, parameter validation for null/empty strings is automatically handled at the controller level through model binding, making additional validation in service methods unnecessary.
backend/src/Designer/Controllers/Organisation/OrgTextController.cs (1)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs:65-94
Timestamp: 2025-02-06T11:20:54.269Z
Learning: In the Altinn Studio Designer, the UpdateTextsForKeys method in OrgTextsService can only be called for text resources that have already been created. The resource's existence is a precondition for this method, and handling non-existent resources is unnecessary except for potential race conditions.
backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnOrgGitRepositoryTests.cs (1)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs:64-70
Timestamp: 2025-02-06T11:16:47.768Z
Learning: In Altinn Studio, when writing files using `WriteTextByRelativePathAsync`, set `createDirectory: true` to automatically create the directory structure if it doesn't exist, instead of manually creating directories.
backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (2)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs:64-70
Timestamp: 2025-02-06T11:16:47.768Z
Learning: In Altinn Studio, when writing files using `WriteTextByRelativePathAsync`, set `createDirectory: true` to automatically create the directory structure if it doesn't exist, instead of manually creating directories.
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs:64-70
Timestamp: 2025-02-06T11:16:47.768Z
Learning: The `WriteTextByRelativePathAsync` method in Altinn Studio has a `createDirectory` parameter that can be set to true to automatically create directories when writing files.
🪛 GitHub Check: codecov/patch
frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx

[warning] 102-102: frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx#L102
Added line #L102 was not covered by tests

🪛 Biome (1.9.4)
frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts

[error] 1-7: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)

backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListTrailingComma.json

[error] 2-3: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)

🔇 Additional comments (148)
development/setup.js (1)

185-185: Fixed JSON syntax by removing trailing comma

The trailing comma after "value": "someValue" has been correctly removed. This ensures the JSON is strictly valid according to the JSON specification, preventing potential parsing errors when handling code list data.

frontend/libs/studio-pure-functions/src/index.ts (1)

5-5: Good addition of FileUtils export

The export statement is correctly added, making the FileUtils module's functionality accessible wherever this index is imported. This follows the established pattern in the file.

frontend/dashboard/enums/SelectedContextType.ts (1)

1-5: Well-structured enum for context types

The SelectedContextType enum is well-defined with clear, descriptive names and follows TypeScript conventions with explicit string values. This provides good type safety for context selection operations.

frontend/dashboard/types/HeaderMenuItem.ts (1)

1-7: Good type definition for header menu items

The HeaderMenuItem type is well-structured with appropriate property types that provide good type safety for menu items in the header. The import of HeaderMenuItemKey as a type ensures proper integration with the enum.

frontend/dashboard/context/HeaderContext/index.ts (1)

1-1: Clean export statement refactoring

The removal of SelectedContextType from this export statement is part of a good refactoring effort to improve code organization by moving it to a dedicated enum file. This change enhances modularity and makes imports more explicit throughout the codebase.

frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/utils/utils.test.ts (1)

54-57: Well-structured test for edge case handling

This test properly verifies the behavior of getCodeListSourcesById when both parameters are undefined, ensuring the function gracefully handles this edge case by returning an empty array. This is good defensive programming practice.

frontend/packages/shared/src/hooks/mutations/useAddOptionListMutation.test.ts (1)

5-5: Good refactoring to use the FileUtils utility.

The change to use FileUtils.convertToFormData instead of directly creating a FormData object is a good practice that centralizes the FormData creation logic, making it consistent across the codebase.

Also applies to: 9-9

frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx (1)

37-37:

❓ Verification inconclusive

Verify codeListsUsages is handled safely throughout the component.

You've made codeListsUsages optional in the type definition, which makes sense for the organization-level library implementation. However, ensure that all places where this prop is used (like line 105) handle the case when it's undefined.


🏁 Script executed:

#!/bin/bash
# Check for potential usage of codeListsUsages without null checks
rg -A 1 -B 1 "codeListsUsages" frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/

Length of output: 8719


Action: Confirm Safe Handling of codeListsUsages

The optional codeListsUsages prop is correctly updated in the type definition, and its usage within the utils.ts file appropriately uses optional chaining. However, our grep search indicates that in some parts of the component (for instance, in CodeListPage.tsx where it’s passed to child components like CodeLists), the prop is forwarded directly without a null-check. Please ensure that any component expecting a defined array either handles an undefined value appropriately or explicitly defaults it (e.g., using codeListsUsages ?? []) before further use.

  • Verify the value passed at line 105 in CodeListPage.tsx and any similar patterns.
  • Confirm that consumers of this prop (e.g., CodeLists in CodeLists.tsx) are safe with an optional value or receive a default array.
frontend/dashboard/hooks/useSubRoute/useSubroute.ts (1)

1-10: Well-designed hook for subroute management.

This hook cleanly extracts the subroute parameter with a sensible default. It follows React hooks best practices and supports the new routing functionality for the content library.

frontend/dashboard/utils/repoUtils/repoUtils.test.ts (1)

1-1: Consistent import refactoring implemented correctly.

The import path for SelectedContextType has been updated to use the new dedicated enum file, which aligns with good code organization practices by moving enums to their own files rather than embedding them in context definitions.

frontend/dashboard/hooks/useProfileMenuTriggerButtonText/useProfileMenuTriggerButtonText.tsx (1)

2-3: Imports properly separated for improved code organization.

The SelectedContextType import has been correctly moved to its own line and updated to reference the dedicated enum file. This follows the project-wide refactoring pattern of centralizing enum definitions and is a good practice for maintainability.

frontend/dashboard/pages/OrgContentLibrary/utils.test.ts (1)

1-17: Well-structured test suite with good coverage.

The test file provides comprehensive coverage for the new isOrg utility function, testing both positive and negative scenarios. I particularly like the use of it.each to efficiently test multiple enum values against the function.

frontend/dashboard/hooks/useProfileMenuTriggerButtonText/useProfileMenuTriggerButtonText.test.tsx (1)

3-4: Import path refactoring looks good.

The import for SelectedContextType has been moved from dashboard/context/HeaderContext to ../../enums/SelectedContextType, which aligns with good software engineering practices by keeping enums in a dedicated location. The condensed import of HeaderContext and HeaderContextType also improves code readability.

frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.ts (2)

1-6: Import statements are well structured.

The imports are properly organized, bringing in only what's needed for the hook functionality.


7-8: Good type definition for mutation arguments.

Creating a dedicated type for the mutation arguments using Pick ensures type safety while keeping the interface minimal.

frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/utils/utils.ts (2)

7-11: Good defensive programming with optional chaining.

The changes to make codeListsUsages parameter optional and adding the optional chaining operator improve the function's robustness against undefined values.


11-15:

✅ Verification successful

Ensure consistent return type handling.

While the function now handles undefined input correctly, you should verify that all code using this function can handle potentially empty array returns.


🏁 Script executed:

#!/bin/bash
# Search for usages of getCodeListSourcesById to ensure consumers handle empty arrays
rg -A 5 "getCodeListSourcesById" --glob "!*.test.*" --glob "*.{ts,tsx}"

Length of output: 7749


Consistent return type confirmed

The function now returns an empty array when no matching CodeListUsage is found, and our tests already verify that:

  • For valid inputs, the correct non-empty array is returned.
  • For empty or undefined inputs, an empty array is reliably returned.
  • Consumer code (e.g., in CodeLists.tsx) is using the function as expected.

No further changes are required unless additional consumers need re-verification for handling empty arrays.

frontend/dashboard/dashboardTestUtils.tsx (3)

4-4: Import for MemoryRouterProps added correctly.

Adding the import for MemoryRouterProps is appropriate for type-checking the router properties.


14-18: Good use of TypeScript's Pick utility type.

Using Pick<MemoryRouterProps, 'initialEntries'> is an elegant way to extend the props interface without duplicating type definitions.


20-25: Props destructuring updated correctly.

The component signature has been updated to include the new initialEntries prop.

backend/src/Designer/Factories/AltinnGitRepositoryFactory.cs (1)

66-74: Implementation follows established factory pattern perfectly.

The new GetAltinnOrgGitRepository method is well-implemented, consistently following the same pattern as the other repository factory methods. The XML documentation is clear and concise.

frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.test.ts (3)

11-27: Well-structured test data setup.

The test data objects are clearly defined with appropriate types and values that will be useful across different test scenarios.


32-41: Comprehensive mutation parameter verification.

Good test case that properly verifies the mutation calls the underlying API function with all the correct parameters.


43-57: Strong cache update verification.

This test effectively verifies the cache management aspect of the mutation by checking that the query client's data is properly updated with the API response after mutation completion.

backend/src/Designer/Infrastructure/ServiceRegistration.cs (2)

15-15: Properly organized imports.

The new imports are correctly placed with related imports, maintaining the organization of the file.

Also applies to: 19-19


72-72: Service registrations follow consistent pattern.

The new service registrations for IOrgCodeListService and IOrgTextsService follow the established pattern and are placed appropriately in the service registration sequence.

Also applies to: 83-83

frontend/packages/shared/src/constants.js (2)

3-3: Good addition of the APP_DASHBOARD_BASENAME constant.

This new constant provides a clear path for the app dashboard, which is beneficial for routing and navigation.


7-7:

❓ Verification inconclusive

Review the library basename change.

The change from STUDIO_LIBRARY_BASENAME to ORG_LIBRARY_BASENAME aligns with the PR objective of implementing an organization-level content library. Ensure that all references to the previous constant have been updated throughout the codebase.


🏁 Script executed:

#!/bin/bash
# Search for any remaining references to STUDIO_LIBRARY_BASENAME
echo "Checking for any remaining references to STUDIO_LIBRARY_BASENAME..."
rg "STUDIO_LIBRARY_BASENAME" --type js --type ts --type jsx --type tsx

Length of output: 243


Library Basename Update Verification

The constant update from STUDIO_LIBRARY_BASENAME to ORG_LIBRARY_BASENAME in frontend/packages/shared/src/constants.js appears correct per the PR’s aim of moving to an organization-level content library. However, the initial search using file type filters encountered an issue with the JSX matcher. Please re-run the above script—which uses a glob pattern to include .jsx files—to confirm that all references to STUDIO_LIBRARY_BASENAME have been removed from the codebase.

  • File to verify: frontend/packages/shared/src/constants.js (Line 7)
  • Action required: Manually verify that no residual references to STUDIO_LIBRARY_BASENAME exist across all JavaScript, TypeScript, JSX, and TSX files.

🏁 Script executed:

#!/bin/bash
echo "Searching for 'STUDIO_LIBRARY_BASENAME' in all JavaScript, TypeScript, JSX, and TSX files..."
rg "STUDIO_LIBRARY_BASENAME" --glob "*.{js,jsx,ts,tsx}"

Length of output: 378


Library Basename Update – Confirm with Manual Verification

  • The constant change in frontend/packages/shared/src/constants.js (line 7) updating STUDIO_LIBRARY_BASENAME to ORG_LIBRARY_BASENAME reflects the intended organization-level library update.
  • A subsequent search using glob patterns for STUDIO_LIBRARY_BASENAME returned no results, suggesting that all instances have been removed.
  • However, due to the initial command errors (e.g. improper invocation of `` and the JSX file type warning), please manually verify that no residual references to STUDIO_LIBRARY_BASENAME exist in any `.js`, `.jsx`, `.ts`, or `.tsx` files across the codebase.

Constant Update Verification – Manual Check Recommended

  • The constant change in frontend/packages/shared/src/constants.js (line 7) from STUDIO_LIBRARY_BASENAME to ORG_LIBRARY_BASENAME aligns with the PR objective.
  • An automated search using the updated glob pattern did not produce any additional matches for STUDIO_LIBRARY_BASENAME.
  • Action: Please perform a manual verification, particularly in JSX files, to ensure that all references to STUDIO_LIBRARY_BASENAME have been removed across the codebase.
frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.test.ts (1)

9-11: Good test setup with proper mocks.

The test constants are well-defined with clear types.

frontend/packages/shared/src/hooks/mutations/useAddOptionListMutation.ts (2)

5-5: Good addition of FileUtils import.

Utilizing a shared utility module improves code organization.


12-12:

✅ Verification successful

Excellent refactoring using FileUtils.

Replacing the custom implementation with a shared utility function:

  • Reduces code duplication
  • Improves maintainability
  • Ensures consistent behavior across the application

🏁 Script executed:

#!/bin/bash
# Check if FileUtils is used consistently in other similar mutation hooks
echo "Checking for other form data creation patterns that could use FileUtils..."
rg "new FormData\(\)" --type js --type ts

Length of output: 1688


Excellent refactoring with FileUtils – a step toward improved consistency!

The change in frontend/packages/shared/src/hooks/mutations/useAddOptionListMutation.ts to use

const formData = FileUtils.convertToFormData(file);

instead of directly instantiating FormData is well done. This refactoring:

  • Reduces code duplication
  • Enhances maintainability
  • Promotes consistent behavior across the application

Additional note: A quick search reveals other parts of the codebase still instantiate FormData directly (e.g., in frontend/app-development/hooks/mutations/useUploadDataModelMutation.ts and frontend/packages/shared/src/hooks/useUpdateBpmn.ts). It might be beneficial to review those areas to determine if refactoring them to use FileUtils would further improve consistency.

frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.test.ts (1)

141-163: Great test coverage for the new removeLeadingSlash method!

The tests are comprehensive and cover all essential scenarios:

  • Removing a leading slash
  • Handling strings without leading slashes
  • Preserving slashes in non-leading positions
  • Handling multiple leading slashes
  • Ensuring the original string is not mutated

This thorough test suite will help maintain the integrity of the method as the codebase evolves.

frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.ts (3)

1-8: Well-organized imports and type definition

The imports are logically grouped and the custom type alias UpdateOrgCodeListMutationArgs is clearly defined using the Pick utility type to reuse existing type definitions, which is a good practice.


9-15: Clean mutation function implementation

The hook accepts an organization identifier and correctly uses the services context to access the updateCodeListForOrg method. The mutation function is concisely defined with proper type safety.


16-22: Effective cache management

Good implementation of the onSuccess callback to update the query cache, ensuring UI consistency after mutations. This prevents unnecessary refetching of data.

backend/tests/Designer.Tests/Services/TextsServiceTest.cs (2)

174-174: Path parameter updated for options list retrieval

The modification to include the "App/options" path parameter aligns with the broader change to make options folder paths configurable across the application.


190-190: Path parameter added consistently across tests

The same parameter update has been consistently applied here, maintaining the integrity of the test suite with the updated method signature.

frontend/dashboard/pages/Dashboard/Dashboard.tsx (3)

20-23: Import paths updated for better modularity

The import for SelectedContextType has been correctly updated to use a dedicated enums module, and new imports for DASHBOARD_BASENAME and useSubroute have been added. This improves code organization and maintainability.


34-34: Added subroute hook for dynamic routing

The addition of the useSubroute hook allows for more flexible and dynamic URL construction, which is essential for the organization-level content library feature being implemented.


61-64: Improved URL construction using template literals

The href attribute now properly uses template literals to construct URLs, incorporating the dashboard basename, current subroute, and selected context. This makes the code more readable and maintainable compared to string concatenation.

frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.test.ts (3)

1-10: LGTM! The imports are well-structured and comprehensive.

The test file correctly imports all necessary dependencies, mocks, and types needed for testing the hook.


11-27: Well-defined test data that covers the necessary scenarios.

The test data is clearly organized with distinct variables for new and existing code lists, which helps make the tests more readable and maintainable.


29-43: LGTM! The test properly verifies API call parameters.

This test effectively verifies that the mutation correctly passes the organization and code list data to the API function.

frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.test.ts (3)

1-10: LGTM! Imports are well-structured and appropriate.

The test file correctly imports all necessary dependencies, including FileUtils for form data conversion.


11-30: Well-structured test data setup with proper file construction.

The test data setup is comprehensive, creating a proper File object with JSON data and converting it to FormData using the FileUtils helper. This accurately reflects how the hook would be used in production.


31-39: LGTM! Test effectively verifies the API call parameters.

This test ensures that the hook correctly calls the uploadCodeListForOrg function with the expected organization and form data parameters.

frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.ts (1)

1-5: LGTM! Imports are appropriate and concise.

The imports are well-structured, importing only the necessary functions and types from their respective modules.

frontend/packages/shared/src/api/paths.js (1)

90-94: LGTM! Well-structured path definitions for organization-level code lists.

The new path definitions follow the established pattern in the codebase, ensuring consistency. The comments indicating the HTTP methods (GET, POST, PUT, DELETE) for each endpoint are helpful for developers using these paths.

frontend/packages/shared/src/api/queries.ts (2)

104-105: Well-structured imports for the new features.

The imports for CodeListsResponse and textResourcesMock are appropriately organized with the rest of the type imports.


187-190:

✅ Verification successful

Good implementation with clear TODO comment for future development.

The implementation includes:

  1. A proper implementation of getCodeListsForOrg using the API path
  2. A mock implementation of getTextResourcesForOrg with a clear TODO comment and issue reference (Endpoints for text resources #14503)

The use of mock data is appropriate for this proof-of-concept stage while maintaining clear documentation about the plan to replace it with a real API call.

To verify the current usage of these functions, let's run:


🏁 Script executed:

#!/bin/bash
# Search for usages of the new API functions in the codebase
echo "Searching for usages of getCodeListsForOrg..."
rg -A 3 "getCodeListsForOrg" --type ts

echo "Searching for usages of getTextResourcesForOrg..."
rg -A 3 "getTextResourcesForOrg" --type ts

Length of output: 5180


Verified Implementation and Clear Documentation
Both getCodeListsForOrg and getTextResourcesForOrg were confirmed to be correctly integrated and consistently referenced across the codebase. Key points:

  • getCodeListsForOrg:
    • Properly implemented using the expected API path.
    • Verified usage in both dashboard tests and hook tests.

  • getTextResourcesForOrg:
    • Uses a mock implementation with a clear TODO comment linking to issue [Endpoints for text resources #14503].
    • Usage in tests confirms correct parameter passing.

Everything appears to be in order, and the clear documentation/TODO ensures future updates can be tracked seamlessly.

backend/src/Designer/Controllers/Preview/V3/OldInstancesController.cs (2)

30-31: Good standardization of folder path constant.

Defining OptionsFolderPath as a constant improves maintainability and consistency. This makes it easier to update the path in the future if needed.


188-188: Consistent parameter update for GetOptionsList method.

The method call has been updated to include the OptionsFolderPath constant as a parameter. This change aligns with the updated method signature in the repository class.

backend/src/Designer/Controllers/PreviewController.cs (2)

65-65: Consistent path constant definition.

The OptionsFolderPath constant is defined with the same value as in other controller classes, maintaining consistency across the codebase.


631-631: Consistent parameter update for GetOptionsList method call.

The method call has been updated to include the OptionsFolderPath constant as a parameter, consistent with changes in other controllers.

frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.ts (1)

1-5: Well-structured imports with proper type definitions.

The imports are organized in a clean manner:

  1. External library imports (@tanstack/react-query)
  2. Internal context imports
  3. Type imports with explicit type annotations
  4. Enum imports

This follows good practices for import organization.

frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts (3)

1-11: LGTM - Appropriate imports for React Query testing

The file properly imports all necessary testing utilities, mock data, and the hooks that will be tested or used within the test cases.


12-30: LGTM - Well-structured test data setup

Test data is clearly defined with appropriate values for different test scenarios, including the code list to be deleted and another code list that should remain after deletion.


32-40: LGTM - Properly validates function call parameters

The test correctly verifies that the mutation hook calls the underlying API function with the expected parameters.

backend/tests/Designer.Tests/Controllers/ApiTests/DesignerEndpointsTestsBase.cs (3)

33-33: LGTM - Added property for test organization path

The TestOrgPath property is properly documented and follows the same pattern as the existing TestRepoPath and RemoteTestRepoPath properties.


100-119: LGTM - Well-structured method for setting up organization test environment

The CopyOrgRepositoryForTest method is thoroughly documented and follows the existing pattern of other test environment setup methods. It properly validates that only one organization can be cloned at a time and sets up the necessary test environment.


136-139: LGTM - Proper cleanup of test organization directories

The Dispose method is appropriately updated to clean up the TestOrgPath resources, ensuring that test resources are properly released after test execution.

backend/tests/Designer.Tests/Controllers/OrgCodeListController/GetCodeListsTests.cs (4)

15-23: LGTM - Well-structured test class setup

The test class is properly set up with inheritance from DesignerEndpointsTestsBase and IClassFixture. Constants are clearly defined and will be reused across test methods.


24-62: LGTM - Comprehensive test for code list retrieval

This test thoroughly verifies the GetCodeLists endpoint by:

  1. Setting up various code lists with different data types and formats
  2. Verifying response status code
  3. Checking the total number of retrieved code lists
  4. Validating individual code lists have the correct title and error state

Good job covering both valid and error cases for code lists.


64-85: LGTM - Proper handling of empty result case

The test correctly verifies that the endpoint returns a 200 OK with an empty list when no code lists exist, ensuring robust error handling.


87-87: LGTM - Clean API URL construction

The helper method provides a clean way to construct the API endpoint URL, making the tests more maintainable.

backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs (2)

8-18: LGTM - Well-designed GetText method with proper documentation

The GetText method is clearly documented with XML comments and follows standard C# conventions. It properly includes cancellation support.


20-30: LGTM - Well-designed SaveText method with proper documentation

The SaveText method is well-documented and follows the established pattern for service methods, including proper cancellation support.

frontend/dashboard/pages/CreateService/CreateService.test.tsx (4)

12-13: Good refactoring of imports to use proper enums.

The change to import Subroute and SelectedContextType from their respective enum files improves code organization and type safety, making the code more maintainable.


75-77: LGTM: Proper test setup with enum values.

The test setup properly uses enum values instead of hardcoded strings, which is a good practice for maintainability and preventing typos.


303-313: Good update to the test for dynamic route construction.

The test has been correctly updated to use dynamic route construction based on the subroute and selectedContext parameters. This is consistent with the PR objectives of enhancing routing functionality.


316-326: Consistent approach to route testing across contexts.

The test for the "all" context follows the same pattern as the other context tests, maintaining consistency in the test suite.

frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.test.tsx (9)

5-7: Good refactoring of imports for enums.

Properly importing enums from their respective files improves code organization and maintainability.


14-15: Good addition of feature flag utilities.

The addition of imports for local storage and feature flags is necessary for the new tests that verify feature flag behavior.


21-21: Ensure mock params are consistent with real usage.

The useParams mock now returns default values for both subroute and selectedContext, which provides a better baseline for tests. Ensure these default values are realistic and commonly used in the application.


48-48: Simplified cleanup function is more concise.

Directly passing jest.clearAllMocks to afterEach is cleaner than wrapping it in an arrow function.


109-121: Well-structured test for feature flag controlled UI elements.

This test effectively verifies that the library and dashboard menu items are displayed when the OrgLibrary feature flag is enabled. Good practice to clean up by removing the feature flag after the test.


123-134: Comprehensive test for library menu link structure.

This test ensures that the library menu element has the correct link structure when the feature flag is enabled, which is important for proper navigation.


136-150: Good negative test cases for feature flag behavior.

Tests 136-150 properly check that menu elements are not rendered when the feature flag is disabled, which is crucial for ensuring the feature toggle works correctly.


167-180: Ensure navigation works correctly with new subroute parameter.

The test now properly includes the subroute when checking navigation, which aligns with the updated routing approach in the application.


199-201: Good test for navigation path construction.

This test verifies that the correct path is constructed when navigating, ensuring the subroute and context are properly combined.

backend/src/Designer/Controllers/Preview/InstancesController.cs (2)

33-34: Good use of a constant for the options folder path.

Defining the path as a constant improves code maintainability by centralizing the definition and making it easier to update if needed.


193-193:

✅ Verification successful

Updated method call to pass options folder path.

The method call has been updated to pass the defined constant for the options folder path, which provides more flexibility in retrieving options from different locations.


🏁 Script executed:

#!/bin/bash
# Check for other usage of GetOptionsList in the codebase to verify consistent updates

# Look for other calls to GetOptionsList
rg "GetOptionsList\(" --type cs

Length of output: 3351


Method Update Verified: Passing Options Folder Path

The update in backend/src/Designer/Controllers/Preview/InstancesController.cs—calling

string options = await altinnAppGitRepository.GetOptionsList(optionListId, OptionsFolderPath, cancellationToken);

—is consistent with similar changes in other controllers (e.g., in PreviewController.cs and Preview/V3/OldInstancesController.cs) and aligns with the intended flexibility improvements for option retrieval. No further adjustments are needed.

backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs (3)

12-14: Documentation improvement for clarity.

The updated comment now clearly specifies that the organization is identified by its short name as defined in Gitea, which improves the API documentation.


20-22: Consistent documentation improvement.

Similar to the previous comment improvement, this update maintains consistency in the documentation style across the interface.


26-32: Good addition of method for organization repository.

The addition of GetAltinnOrgGitRepository method is well-documented and follows the same pattern as existing methods. This supports the new organization-level content library feature.

frontend/dashboard/pages/CreateService/CreateService.tsx (3)

14-14: Good addition of the useSubroute hook

The new import of useSubroute enhances the component's routing capabilities by abstracting URL parameter extraction logic into a reusable hook.


40-42: Improved variable organization and dependency management

Good refactoring to organize the related variables together. The addition of subroute and reordering of selectedContext makes the code more logical and maintainable.


91-91: More flexible and maintainable routing implementation

The href construction now uses dynamic parameters (subroute and selectedContext) instead of hardcoded routes. This change:

  1. Makes the component more flexible to routing changes
  2. Removes dependency on the removed DASHBOARD_ROOT_ROUTE constant
  3. Aligns with the organization-level content library feature purpose

This is a good improvement that makes the navigation system more adaptable to different contexts.

backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnAppGitRepositoryTests.cs (5)

267-267: Good: Added options folder path constant for test clarity

Adding the optionsFolderPath variable improves test readability and maintains consistency with the implementation changes.


270-270: Updated method signature to include the options folder path parameter

Properly updating the test to pass the new optionsFolderPath parameter to match changes in the implementation. This ensures proper testing of the updated method signature.


286-289: Consistent parameter updates across test methods

Good work consistently updating all related test methods with the new optionsFolderPath parameter. This ensures all test cases properly validate the updated implementation.


298-301: Complete test coverage for the updated method signature

Properly updated the test for GetOptionsListIds to use the new parameter. The test verifies that the method correctly returns option list IDs from the specified path.


314-316: Test edge case handling for empty options list

Good test coverage of the edge case where no option lists exist. The test now correctly passes the optionsFolderPath parameter while verifying empty return.

frontend/dashboard/testing/mocks.tsx (6)

6-6: Good addition of MemoryRouterProps type import

Adding this import enables proper typing for the router props, which improves type safety in the tests.


15-18: Enhanced WrapperArgs type with routing capabilities

Good improvement to extend the WrapperArgs type with initialEntries from MemoryRouterProps. This provides test components with the ability to set initial routes.


21-24: Updated wrapper function with initialEntries support

The wrapper function now correctly accepts and passes through the initialEntries parameter to the MemoryRouter. This enables testing of components that depend on route parameters.


31-31: Well-structured interface extension

Extending ProviderData with Partial<WrapperArgs> is a clean way to incorporate the new properties without duplicating type definitions.


36-45: Updated renderWithProviders to support route testing

The function now properly accepts and passes the initialEntries parameter, allowing tests to simulate different routing contexts.


50-66: Consistently updated renderHookWithProviders with routing support

Good job ensuring that both rendering functions (renderWithProviders and renderHookWithProviders) have consistent parameter handling for initialEntries. This provides a unified testing experience.

backend/src/Designer/Services/Implementation/OptionsService.cs (3)

23-23: Good addition of OptionsFolderPath constant

Adding this constant improves code maintainability by centralizing the path definition and making it easily updatable across the codebase. The trailing slash in the path ensures consistent path construction.


41-41: Properly updated GetOptionsListIds method call

The method call now passes the OptionsFolderPath constant to properly specify where to look for options lists.


58-58: Updated GetOptionsList method call with folder path parameter

Correctly passing the OptionsFolderPath constant to the repository method ensures consistent options retrieval.

frontend/dashboard/pages/PageLayout/PageLayout.test.tsx (5)

10-12: New imports for dashboard routing enhancements.

These new imports support the updated navigation path construction for the application routes.


77-80: Navigation path format updated for consistency.

The navigation path is now constructed using a more structured approach combining the app dashboard subroute with the context type, making routes consistent across the application.


86-89: Navigation path standardized for self context redirection.

Similar to the previous change, this standardizes how redirect paths are constructed when no context is defined.


100-103: Consistent path formatting for context navigation.

This change ensures that navigation to any context (self, all, or organization) follows the same path structure pattern.


112-115: Navigation format standardized for fallback to self context.

This completes the standardization of navigation paths across all test cases for the page layout component.

frontend/language/src/nb.json (2)

229-230: Added Norwegian translations for dashboard navigation items.

These translations provide localized labels for the main dashboard and library navigation items in the header.


246-249: Added Norwegian translations for organization library UI messages.

These translations provide clear Norwegian text for organization library-related alerts and messages, supporting the new content library functionality. The messages guide users on organization selection requirements and provide feedback for upload operations and error states.

frontend/dashboard/app/App.test.tsx (1)

95-101: Good pattern for render function! 👍

The renderApp function is a great pattern for making the tests more maintainable and easier to read. It encapsulates the rendering logic with optional arguments, making the test cases cleaner.

backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (4)

89-91: Documentation correction noted.

The documentation has been corrected to reference the actual class name AltinnAppGitRepository instead of AltinnGitRepository, which properly aligns the documentation with the class it's describing.


609-609: Method updated to use correct path variable.

The method now uses GetOptionsListIds(OptionsFolderPath) to fetch option list IDs using the constant path, making the code more consistent with the updated method signature.


998-1000: Method signature improved for greater flexibility.

The GetOptionsListIds method has been enhanced to accept a parameter optionsFolderPath instead of using a hardcoded path, which improves flexibility and reusability when working with different folder structures.


1015-1026: Method parameter added for better flexibility.

The GetOptionsList method has been updated to accept an optionsFolderPath parameter and uses it instead of the hardcoded constant. This change allows the method to work with different folder locations, supporting the organization-level content library functionality.

frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.test.tsx (8)

1-18: LGTM! Proper imports and test setup.

The test file correctly imports the necessary testing utilities, mocks, and the component under test. It establishes a good foundation for comprehensive testing.


19-26: Well-defined mocks for test data.

The mock data is clearly defined with descriptive names, which improves readability and maintainability of the tests.


27-55: Effective component mocking strategy.

The mock implementation of the CodeListPage component cleanly simulates the functionality needed for testing with buttons that trigger the appropriate callbacks. This approach isolates the tests from external dependencies.


56-75: Comprehensive basic functionality tests.

The test suite effectively verifies loading states and error handling by testing the rendering of the spinner during data fetching and proper error message display when queries fail.


77-92: Good use of parameterized tests.

The use of it.each for testing different context types is efficient and ensures all context scenarios are covered without code duplication. This is a clean testing approach.


94-113: UI rendering tests are thorough.

These tests verify that the library title and landing page elements render correctly, ensuring the component displays the expected content to users.


115-197: Comprehensive interaction testing.

The test suite thoroughly tests all user interactions including code list update, upload, and delete operations, as well as toast notifications for success and error cases.


200-228: Well-structured test helper functions.

The helper functions for rendering components and setting up test data are well-designed, making the tests more readable and maintainable. The createQueryClientWithOptionsDataList function is particularly useful for setting up the query client state.

backend/tests/Designer.Tests/Controllers/OrgTextController/GetResourcesTests.cs (4)

12-17: Good test class structure.

The test class properly inherits from the base test class and implements the necessary interfaces for ASP.NET Core integration testing.


18-38: Well-structured positive test case.

This test thoroughly validates the successful path of retrieving resources, with proper setup, execution, and assertions. The test data is provided via InlineData attribute, promoting test maintainability.


40-58: Comprehensive error case testing.

The test for the 404 Not Found scenario properly verifies both the status code and error message content, ensuring the API provides meaningful information when a resource isn't found.


60-62: Helpful utility methods.

The utility methods for constructing API URLs and file paths improve code readability and maintainability by centralizing these string operations.

frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (4)

1-23: Well-organized imports.

The imports are logically grouped and only include what's necessary for the component, following best practices.


24-32: Clean main component with conditional rendering.

The main OrgContentLibrary component uses a clean conditional rendering approach based on the selected context, separating the different rendering paths into dedicated components.


34-49: Effective handling of query states.

The component properly handles all query states (loading, error, success) with appropriate UI components, providing a good user experience regardless of the data fetching state.


86-98: Clear error state handling.

The ContextWithoutLibraryAccess component provides a clear message to users when there is no proper context selected, enhancing usability.

frontend/dashboard/app/App.tsx (1)

25-29: Great approach merging query states.
Using mergeQueryStatuses for both userStatus and organizationsStatus simplifies the logic and reduces branching code. This is a concise pattern.

backend/tests/Designer.Tests/Services/OrgTextsServiceTests.cs (2)

17-44: Test coverage is excellent for retrieval scenarios.
Your series of GetText_ShouldReturnAllTexts and GetText_ShouldReturnNotFoundException_WhenFileDoesNotExist comprehensively validates the success and failure paths of text retrieval. Great job providing coverage of both expected data and error conditions.


178-229: Overall test design is well-structured, but consider verifying concurrency or parallel usage scenarios.
Although these tests cover many edge cases, verifying concurrency or simultaneous updates (if relevant) could strengthen coverage further and detect potential race conditions in text resource operations.

backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnOrgGitRepositoryTests.cs (2)

22-37: Robust text resource fetch test with deep JSON validation.
Comparing serialized JSON contents with JsonUtils.DeepEquals is effective and ensures an accurate reflection of the repository’s content. This is a solid approach for regression testing.


157-173: Add an assertion or post-deletion fetch check for deletion tests.
After deleting a code list, you correctly assert that the file no longer exists. Consider also ensuring that the repository does not return the deleted code list when attempting to fetch it, further validating the operation.

 // After codeListRepository.DeleteCodeList(codeListId);
+await Assert.ThrowsAsync<NotFoundException>(() => codeListRepository.GetCodeList(codeListId));
 string repositoryDir = ...
backend/src/Designer/Controllers/Organisation/OrgTextController.cs (1)

1-19: Well-structured controller introduction.

This file provides a clear and documented controller for text resources at the organisation level. The usage of [Authorize], structured routes, and XML documentation is a well-organized approach.

frontend/packages/shared/src/api/mutations.ts (3)

52-53: Newly added paths look good.


63-63: Type imports are consistent.


81-82: Correct usage of new types.

backend/tests/Designer.Tests/Utils/TestDataHelper.cs (6)

141-141: Unify test repo and org name length validation for consistency.

The updated validation message matches the new range (12 to 28). However, consider aligning it with the range used in the GenerateTestOrgName method (12 to 27) or explain why they differ.


420-428: Constructor slicing approach is acceptable.

The GenerateTestOrgName method uses string slicing with a bound of 27 to ensure no out-of-range exception. The logic is sound for ensuring minimal and maximal lengths. Implementation looks good.


430-433: Method name and return value are clear.

GetOrgContentRepoName is concise and indicates its purpose clearly. No issues found.


435-445: Confirm directory creation logic.

CopyOrgForTest creates the target org directory before copying files. This ensures the structure is in place. Great use of CreateEmptyDirectory followed by CopyDirectory.


447-455: Path construction is straightforward.

GetOrgDirectory and GetOrgRepositoryDirectory consistently rely on the base test data path. The code is self-explanatory and easy to maintain.


457-461: Deletion approach is fine.

DeleteOrgDirectory calls DeleteDirectory after constructing the path. The narrowing of scope to a single org is convenient and consistent with the rest of the utility methods.

backend/tests/Designer.Tests/Services/OrgCodeListServiceTests.cs (2)

160-162: Dispose the memory stream to address resource leakage.

Although you manually close the MemoryStream, consider disposing it in a using block or calling Dispose() for thorough resource cleanup.


1-269: Overall test coverage is robust.

All relevant operations (Get, Create, Update, Upload, Delete, Existence check) are thoroughly tested, including failure scenarios. This comprehensive coverage should help maintain reliability.

backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (1)

14-173: Good use of createDirectory in WriteTextByRelativePathAsync.

You are correctly setting createDirectory: true for creating or overwriting code lists, aligning with best practices for auto-creating needed directories. The exception handling is also clear and thorough.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (1)

79-82: Add edge case handling to the route parameter extraction function

The current implementation might return undefined or empty string for paths that have fewer than 2 segments or paths that end with a trailing slash.

function extractSecondLastRouterParam(pathname: string): string {
  const pathnameArray = pathname.split('/').filter(Boolean);
- return pathnameArray[pathnameArray.length - 2];
+ return pathnameArray.length >= 2 ? pathnameArray[pathnameArray.length - 2] : '';
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b186c40 and 30c5653.

📒 Files selected for processing (3)
  • frontend/dashboard/app/App.test.tsx (2 hunks)
  • frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (6 hunks)
  • frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Analyze
  • GitHub Check: Build environment and run e2e test
  • GitHub Check: Run dotnet build and test (windows-latest)
  • GitHub Check: Run dotnet build and test (macos-latest)
  • GitHub Check: Run dotnet build and test (ubuntu-latest)
  • GitHub Check: Testing
  • GitHub Check: CodeQL
🔇 Additional comments (15)
frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (5)

35-40: Nicely implemented conditional navigation based on feature flag

The addition of conditionally rendered navigation menu items in the header is clean and follows best practices. Good use of the feature flag to control this feature's visibility.


49-51: Clean type definition for menu item props

Good job defining a clear type for the navigation menu props, promoting type safety.


53-77: Well-structured navigation component with proper route handling

The TopNavigationMenuItem component is well-implemented with proper use of React Router's NavLink and conditional class application. The component has clean separation of concerns and good reusability.


97-97: Use useLocation hook instead of accessing location directly

The component is directly accessing the global location object rather than using React Router's useLocation hook.

const DashboardHeaderMenu = () => {
  const { t } = useTranslation();
  const showButtonText = !useMediaQuery(MEDIA_QUERY_MAX_WIDTH);
  const selectedContext = useSelectedContext();
  const subroute = useSubroute();
+ const location = useLocation();
  const { mutate: logout } = useLogoutMutation();
  const { user, selectableOrgs } = useContext(HeaderContext);
  const navigate = useNavigate();

  // ...

  const handleSetSelectedContext = (context: string | SelectedContextType) => {
-   navigate(`${subroute}/${context}${location.search}`);
+   navigate(`${subroute}/${context}${location.search}`);
  };

136-136: Improved triggerButtonText assignment clarity

Good refactor to use a ternary operator for explicitly setting undefined when showButtonText is false, rather than relying on a logical AND pattern.

frontend/dashboard/app/App.test.tsx (10)

17-17: Acknowledged: Temporary solution for React Router mocking.

While it's best practice to avoid global mocks, the comment correctly identifies this as a known issue that will be addressed later (per the referenced issue #14597). As you mentioned in your response to the previous review, this can't be changed until that issue is resolved.


19-22: Good implementation of feature flag mocking.

This is a cleaner approach than manipulating localStorage directly, addressing the previous review suggestion. The mock ensures that only the OrgLibrary feature is enabled for testing purposes.


24-29: Well-structured test data.

Defining the organization test data as a constant improves readability and maintainability.


32-32: Improved test setup.

Using beforeEach(jest.clearAllMocks) is a cleaner way to reset mocks between tests than the previous approach.


59-64: Good test coverage for the dashboard page.

This test verifies that the dashboard loads correctly and displays both dashboard and library navigation links after the spinner disappears.


72-79: Well-implemented navigation test.

This test effectively verifies navigation from the dashboard to the library using proper user interaction simulation.


81-91: Comprehensive navigation testing.

This test completes the navigation flow by testing the return from library to dashboard, ensuring bidirectional navigation works correctly.


94-102: Excellent utility function for test rendering.

The renderApp function with optional arguments significantly improves test clarity and reduces duplication. This is a good refactoring that makes the tests more maintainable.


104-110: Good extraction of utility query functions.

These helper functions improve readability and maintainability by encapsulating common element queries in semantically named functions.


112-117: Well-structured query client setup.

This utility function cleanly sets up the query client with the necessary test data, making the tests more readable and reducing duplicate code.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
backend/tests/Designer.Tests/Controllers/OrgCodeListController/DeleteCodeListTests.cs (2)

43-43: Consider making the assertion more robust

The assertion Assert.Equal(5, responseList.Count); is brittle as it depends on the exact number of code lists in the test data. If the test data changes, this test will fail even if the deletion functionality works correctly.

Consider one of these alternatives:

- Assert.Equal(5, responseList.Count);
+ // Option 1: Compare against initial count
+ var initialCount = 6; // Get this dynamically if possible
+ Assert.Equal(initialCount - 1, responseList.Count);

+ // Option 2: Just verify the deleted item is not in the list (already done in the next line)

66-66: Consider using a separate error object structure

The test is validating the error message directly from the response body. In production APIs, it's generally better to return structured error objects rather than plain strings.

- Assert.Equal($"The code list file {codeListId}.json does not exist.", responseDocument.RootElement.ToString());
+ // Consider updating the API to return a structured error and test like this:
+ Assert.Equal($"The code list file {codeListId}.json does not exist.", responseDocument.RootElement.GetProperty("message").GetString());
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 30c5653 and e1f620e.

📒 Files selected for processing (3)
  • backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (3 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/DeleteCodeListTests.cs (1 hunks)
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/UpdateCodeListTests.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs
  • backend/tests/Designer.Tests/Controllers/OrgCodeListController/UpdateCodeListTests.cs
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: Build environment and run e2e test
  • GitHub Check: Run integration tests against actual gitea and dbs
  • GitHub Check: Analyze
  • GitHub Check: Run dotnet build and test (macos-latest)
  • GitHub Check: Run dotnet build and test (windows-latest)
  • GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (2)
backend/tests/Designer.Tests/Controllers/OrgCodeListController/DeleteCodeListTests.cs (2)

1-70: Well-structured tests with clear Arrange-Act-Assert pattern

The implementation follows good testing practices with a clear separation of test phases and appropriate assertions for both success and failure cases.


48-49: Test method name correctly matches the expected behavior

The method name properly indicates that a 404 response is expected when attempting to delete a non-existent code list, which aligns with the assertions.

@TomasEng TomasEng added skip-manual-testing PRs that do not need to be tested manually skip-documentation Issues where updating documentation is not relevant labels Mar 6, 2025
@TomasEng
Copy link
Contributor Author

TomasEng commented Mar 6, 2025

Added skip-manual-testing to get second approval. This has been tested in the dev environment.

@TomasEng TomasEng merged commit ec4cbe7 into main Mar 6, 2025
29 checks passed
@TomasEng TomasEng deleted the org-library-mvp branch March 6, 2025 07:15
@TomasEng TomasEng restored the org-library-mvp branch March 6, 2025 07:15
@TomasEng
Copy link
Contributor Author

TomasEng commented Mar 6, 2025

Since "squash and merge" was the only option for merging into the main branch, I'm keeping the branch for future reference.

nkylstad pushed a commit that referenced this pull request Mar 12, 2025
)

Co-authored-by: Erling Hauan <[email protected]>
Co-authored-by: Konrad-Simso <[email protected]>
Co-authored-by: andreastanderen <[email protected]>
Co-authored-by: William Thorenfeldt <[email protected]>
Co-authored-by: andreastanderen <[email protected]>
Co-authored-by: Erling Hauan <[email protected]>
Co-authored-by: wrt95 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/app-preview Area: Related to test and preview of apps that are developed in Altinn Studio. area/content-library Area: Related to library for shared resources area/dashboard Area: Related to the dashboard application area/text-editor Area: Related to creating, translating and editing texts. backend frontend quality/testing Tests that are missing, needs to be created or could be improved. skip-documentation Issues where updating documentation is not relevant skip-manual-testing PRs that do not need to be tested manually skip-releasenotes Issues that do not make sense to list in our release notes solution/studio/designer Issues related to the Altinn Studio Designer solution. team/studio-domain1
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Merge org library branch to main
5 participants