fix: Update codelist endpoint return types #14622
Conversation
Konrad-Simso
added
solution/studio/designer
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a new controller and supporting services for managing code lists at the organization level. It adds the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/dashboard
|
Tried triggering automatic tests, it did not work. i'll run them manually from now on. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (2)
796-807: Enhance error handling and documentation.The method could benefit from:
- XML documentation for the
optionsFolderPathparameter- Input validation for the path parameter
- More descriptive error messages
/// <summary> /// Gets a list of file names from the Options folder representing the available options lists. /// </summary> + /// <param name="optionsFolderPath">The folder path where options lists are stored.</param> /// <returns>A list of option list names.</returns> + /// <exception cref="ArgumentException">Thrown when optionsFolderPath is null or empty.</exception> public string[] GetOptionsListIds(string optionsFolderPath) { + if (string.IsNullOrEmpty(optionsFolderPath)) + { + throw new ArgumentException("Options folder path cannot be null or empty.", nameof(optionsFolderPath)); + } + string optionsFolder = Path.Combine(optionsFolderPath); if (!DirectoryExistsByRelativePath(optionsFolder)) { return []; }
816-828: Enhance error handling and documentation.The method could benefit from:
- XML documentation for the
optionsFolderPathparameter- Input validation for both parameters
- More descriptive error messages
/// <summary> /// Gets a specific options list with the provided id. /// </summary> /// <param name="optionsListId">The name of the options list to fetch.</param> + /// <param name="optionsFolderPath">The folder path where options lists are stored.</param> /// <param name="cancellationToken">A <see cref="CancellationToken"/> that observes if operation is cancelled.</param> /// <returns>The options list as a string.</returns> + /// <exception cref="ArgumentException">Thrown when optionsListId or optionsFolderPath is null or empty.</exception> + /// <exception cref="NotFoundException">Thrown when the options list file is not found.</exception> public async Task<string> GetOptionsList(string optionsListId, string optionsFolderPath, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); + if (string.IsNullOrEmpty(optionsListId)) + { + throw new ArgumentException("Options list ID cannot be null or empty.", nameof(optionsListId)); + } + + if (string.IsNullOrEmpty(optionsFolderPath)) + { + throw new ArgumentException("Options folder path cannot be null or empty.", nameof(optionsFolderPath)); + } + string optionsFilePath = Path.Combine(optionsFolderPath, $"{optionsListId}.json"); if (!FileExistsByRelativePath(optionsFilePath)) { - throw new NotFoundException($"Options file {optionsListId}.json was not found."); + throw new NotFoundException($"Options file {optionsListId}.json was not found in folder {optionsFolderPath}."); }
🧹 Nitpick comments (50)
frontend/dashboard/app/App.tsx (2)
54-72: Error-handling logic is fine, but consider more detailed organization error checks
Currently, ifuserStatus === 'error', the user error is shown; otherwise, the organization error is shown. Consider also checking if organizations explicitly failed if you need more contextual error messages.
92-103: SubrouteGuard missing a default case
If the subroute doesn't match any known base, no fallback is provided. Consider adding a default to handle unknown subroutes with an error or redirect.frontend/dashboard/hooks/guards/useContextRedirectionGuard.ts (1)
46-50: Using navigateToSelf if user lacks access
Consider whether a 403 or alert might be more appropriate for restricted contexts, instead of silently redirecting to Self.backend/src/Designer/Services/Interfaces/Organisation/ICodeListService.cs (2)
21-30: GetCodeList needs clarity on missing code lists
If the code list is not found, decide whether to return an empty list or throw an exception. Consider doc comments to clarify.
42-52: CreateCodeList supports overwrite
Since creating existing code list overwrites data, an “upsert” naming could better reflect the behavior.backend/src/Designer/Controllers/Organisation/CodeListController.cs (2)
79-81: Clarify or de-duplicate summary comments.Lines 79–81 reuse the “Creates or overwrites” summary, which is nearly identical to lines 58–60 for the
CreateCodeListmethod. Consider differentiating the summaries for clarity or removing duplication.
135-147: Return a more appropriate status code for non-existent code lists.When the specified code list does not exist, the controller still returns a success (
200 OK) message stating it "has been deleted." This can be confusing or misleading. Consider returning a404 NotFoundor a more accurate message if the resource was not previously present.backend/src/Designer/Services/Implementation/Organisation/CodeListService.cs (3)
110-132: Reduce code duplication between create and update methods.The
CreateCodeListandUpdateCodeListmethods both callCreateOrOverwriteOptionsListand then retrieve all code lists. If the logic is identical, consider consolidating them into a single method or factoring out the shared logic to improve maintainability.
110-153: Optimize by returning only the updated/created code list.Each method that creates, updates, or uploads code lists calls
GetCodeListsafterward and returns the entire set. If the consumer only needs the newly updated or created list, returning the full set may be inefficient, especially if the number of code lists grows large. Consider returning only the relevant code list data to optimize performance.
104-108: Consider performance implications of validating each option.This row-by-row validation ensures correctness but might become costly for very large lists. If the volume of options is high, consider batched validation or partial checks to reduce overhead.
frontend/packages/shared/src/types/CodeListData.ts (1)
3-7: Add JSDoc documentation for better type clarity.Consider adding JSDoc comments to document the purpose and usage of the
CodeListDatatype and its properties.+/** + * Represents a code list with its data and error state. + */ export type CodeListData = { + /** The title of the code list */ title: string; + /** Optional array of code list items */ data?: CodeListItem[]; + /** Indicates if there was an error loading the code list */ hasError?: boolean; };frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts (1)
1-7: Refactor to a standalone function.Instead of using a class with only static members, consider refactoring to a standalone function for better simplicity and maintainability.
-export class FileUtils { - static convertToFormData = (file: File): FormData => { - const formData = new FormData(); - formData.append('file', file); - return formData; - }; -} +/** + * Converts a File object to FormData. + * @param file The file to convert + * @returns FormData 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/dashboard/hooks/useSubRoute/useSubroute.ts (1)
10-10: Consider moving the constant declaration above the hook.For better readability and to avoid using variables before they are declared, consider moving the
defaultSubrouteconstant above the hook implementation.-export function useSubroute(): string { - const { subroute = defaultSubroute } = useParams(); - return subroute; -} - -const defaultSubroute = StringUtils.removeLeadingSlash(APP_DASHBOARD_BASENAME); +const defaultSubroute = StringUtils.removeLeadingSlash(APP_DASHBOARD_BASENAME); + +export function useSubroute(): string { + const { subroute = defaultSubroute } = useParams(); + return subroute; +}frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (1)
3-17: Consider adding test cases for edge cases.The current test suite only covers the happy path. Consider adding test cases for:
- Empty files
- Files with special characters in the name
- Large files
- Files with different MIME types
frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.ts (1)
9-12: Consider adding error handling and retry options.For better error handling and resilience, consider adding:
- Retry options for transient failures
- Error boundaries for handling query errors
- Proper error typing
return useQuery<CodeListsResponse>({ queryKey: [QueryKey.OrgCodeLists, org], queryFn: () => getCodeListsForOrg(org), + retry: 3, + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), + onError: (error) => { + console.error('Failed to fetch code lists:', error); + }, });frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.ts (1)
6-15: Add parameter validation and query configuration.Consider adding parameter validation and configuring stale/cache time for optimal performance.
export const useTextResourcesForOrgQuery = ( org: string, language: string, ): UseQueryResult<ITextResourcesWithLanguage> => { + if (!org || !language) { + throw new Error('org and language parameters are required'); + } + const { getTextResourcesForOrg } = useServicesContext(); return useQuery<ITextResourcesWithLanguage>({ queryKey: [QueryKey.TextResourcesForOrg, org, language], queryFn: () => getTextResourcesForOrg(org, language), + staleTime: 5 * 60 * 1000, // 5 minutes + cacheTime: 30 * 60 * 1000, // 30 minutes }); };frontend/dashboard/context/HeaderContext/HeaderContext.tsx (1)
12-15: Add documentation and type safety.Consider adding JSDoc documentation and type assertions for the basename constants.
+/** + * Represents the available subroutes in the application. + * Used for navigation and routing purposes. + */ export enum Subroute { - AppDashboard = APP_DASHBOARD_BASENAME, - OrgLibrary = ORG_LIBRARY_BASENAME, + AppDashboard = APP_DASHBOARD_BASENAME as string, + OrgLibrary = ORG_LIBRARY_BASENAME as string, }frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.test.ts (1)
14-18: Add cleanup and improve test helper.The test helper should clean up after each test and handle errors gracefully.
+import { cleanup } from '@testing-library/react'; + +afterEach(() => { + cleanup(); + jest.clearAllMocks(); +}); + const render = async () => { const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); - await waitFor(() => expect(result.current.isSuccess).toBe(true)); + try { + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + } catch (error) { + console.error('Error waiting for hook to succeed:', error); + } return result; };frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.ts (1)
6-19: LGTM! Consider adding error handling.The implementation correctly manages the mutation lifecycle and cache updates. However, consider adding error handling to manage failed mutations gracefully.
Add error handling by including an
onErrorcallback:return useMutation({ mutationFn: async () => { const textResourcesWithLanguage: ITextResourcesWithLanguage[] = await createTextResourcesForOrg(org, language); return textResourcesWithLanguage; }, onSuccess: (textResourcesWithLanguage) => q.setQueryData([QueryKey.TextResourcesForOrg, org], textResourcesWithLanguage), + onError: (error) => { + // Handle error (e.g., show toast notification) + console.error('Failed to create text resources:', error); + }, });frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.ts (2)
11-14: Add file type validation.Consider validating the file type before uploading to prevent invalid file uploads.
Add file type validation:
const mutationFn = (file: File) => { + const allowedTypes = ['text/csv', 'application/json']; + if (!allowedTypes.includes(file.type)) { + throw new Error('Invalid file type. Only CSV and JSON files are allowed.'); + } const formData = FileUtils.convertToFormData(file); return uploadCodeListForOrg(org, formData); };
16-21: Add error handling for failed uploads.The implementation correctly manages the mutation lifecycle and cache updates. However, consider adding error handling to manage failed uploads gracefully.
Add error handling:
return useMutation({ mutationFn, onSuccess: (newData: CodeListsResponse) => { queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); }, + onError: (error) => { + // Handle error (e.g., show toast notification) + console.error('Failed to upload code list:', error); + }, });frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.ts (2)
13-13: Consider adding a confirmation check before deletion.Since deletion is a destructive operation, consider adding a confirmation check before proceeding.
Add confirmation check:
-const mutationFn = ({ title }: DeleteOrgCodeListMutationArgs) => deleteCodeListForOrg(org, title); +const mutationFn = ({ title }: DeleteOrgCodeListMutationArgs) => { + const confirmed = window.confirm(`Are you sure you want to delete the code list "${title}"?`); + if (!confirmed) { + throw new Error('Operation cancelled by user'); + } + return deleteCodeListForOrg(org, title); +};
15-20: Add error handling for failed deletions.The implementation correctly manages the mutation lifecycle and cache updates. However, consider adding error handling to manage failed deletions gracefully.
Add error handling:
return useMutation({ mutationFn, onSuccess: (newData: CodeListsResponse) => { queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); }, + onError: (error) => { + // Handle error (e.g., show toast notification) + console.error('Failed to delete code list:', error); + }, });frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.ts (2)
10-15: Add payload validation.Consider validating the payload before updating to prevent invalid text resources.
Add payload validation:
mutationFn: async (payload: ITextResource[]) => { + if (!Array.isArray(payload) || payload.length === 0) { + throw new Error('Payload must be a non-empty array of text resources'); + } + if (!payload.every(resource => resource.id && resource.value)) { + throw new Error('Each text resource must have an id and value'); + } const textResourcesWithLanguage: ITextResourcesWithLanguage[] = await updateTextResourcesForOrg(org, language, payload); return textResourcesWithLanguage; },
16-18: Add error handling for failed updates.The implementation correctly manages the mutation lifecycle and cache updates. However, consider adding error handling to manage failed updates gracefully.
Add error handling:
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) => { + // Handle error (e.g., show toast notification) + console.error('Failed to update text resources:', error); + }, });frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.ts (1)
16-21: Consider adding error handling.The mutation is missing error handling. Consider adding an
onErrorcallback to handle API errors gracefully.return useMutation({ mutationFn, onSuccess: (newData: CodeListsResponse) => { queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); }, + onError: (error) => { + // Handle error appropriately + console.error('Failed to create code list:', error); + }, });frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.ts (1)
7-22: Consider extracting shared mutation logic.This hook shares significant code with
useCreateOrgCodeListMutation. Consider extracting the common mutation configuration into a shared utility function.+const createCodeListMutation = ( + mutationFn: (args: Pick<CodeListData, 'title' | 'data'>) => Promise<CodeListsResponse>, + org: string, + queryClient: QueryClient +) => ({ + mutationFn, + onSuccess: (newData: CodeListsResponse) => { + queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); + }, + onError: (error) => { + console.error('Failed to update code list:', error); + }, +}); export const useUpdateOrgCodeListMutation = (org: string) => { const queryClient = useQueryClient(); const { updateCodeListForOrg } = useServicesContext(); const mutationFn = ({ title, data }: UpdateOrgCodeListMutationArgs) => updateCodeListForOrg(org, title, data); - return useMutation({ - mutationFn, - onSuccess: (newData: CodeListsResponse) => { - queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); - }, - }); + return useMutation(createCodeListMutation(mutationFn, org, queryClient)); };frontend/packages/shared/src/hooks/mutations/useAddOptionListMutation.ts (1)
18-21: Consider handling Promise rejections from cache invalidation.The
voidoperator is used to ignore Promises frominvalidateQueries. Consider handling potential rejections.- onSuccess: () => { - void queryClient.invalidateQueries({ queryKey: [QueryKey.OptionListIds, org, app] }); - void queryClient.invalidateQueries({ queryKey: [QueryKey.OptionLists, org, app] }); + onSuccess: async () => { + try { + await Promise.all([ + queryClient.invalidateQueries({ queryKey: [QueryKey.OptionListIds, org, app] }), + queryClient.invalidateQueries({ queryKey: [QueryKey.OptionLists, org, app] }) + ]); + } catch (error) { + console.error('Failed to invalidate queries:', error); + } },frontend/packages/shared/src/hooks/mutations/useAddOptionListMutation.test.ts (1)
11-22: Enhance test coverage.The test suite only covers the happy path. Consider adding tests for:
- Error cases
- Cache invalidation
- Different file types
describe('useAddOptionsMutation', () => { afterEach(jest.clearAllMocks); it('Calls useAddOptionsMutation with correct arguments and payload', async () => { const optionsResult = renderHookWithProviders(() => useAddOptionListMutation(org, app)).result; await optionsResult.current.mutateAsync(file); expect(queriesMock.uploadOptionList).toHaveBeenCalledTimes(1); expect(queriesMock.uploadOptionList).toHaveBeenCalledWith(org, app, formData); }); + + it('Invalidates queries on success', async () => { + const queryClient = createQueryClient(); + const optionsResult = renderHookWithProviders( + () => useAddOptionListMutation(org, app), + { queryClient } + ).result; + + await optionsResult.current.mutateAsync(file); + + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: [QueryKey.OptionListIds, org, app] + }); + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: [QueryKey.OptionLists, org, app] + }); + }); + + it('Handles API errors', async () => { + const error = new Error('API Error'); + queriesMock.uploadOptionList.mockRejectedValueOnce(error); + const optionsResult = renderHookWithProviders(() => useAddOptionListMutation(org, app)).result; + + await expect(optionsResult.current.mutateAsync(file)).rejects.toThrow(error); + }); });frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.test.ts (2)
9-17: Consider adding more test cases for error scenarios and loading states.The test suite currently only verifies the success path. Consider adding test cases for:
- Error handling when the query fails
- Loading state behavior
- Empty response handling
19-22: Consider extracting test data setup to a separate file.The
renderAndWaitForResulthelper function is well-structured, but consider moving test data likelanguageMockto a separate test data file for better reusability across test suites.frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.test.ts (1)
12-22: Enhance test coverage for mutation scenarios.The test suite should include additional test cases:
- Error handling when mutation fails
- Validation of mutation payload structure
- Loading state during mutation
- Edge cases with empty/invalid payload
frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.test.ts (1)
9-10: Consider extracting test data setup.Move mock data setup to a separate test data file for better reusability across test suites.
frontend/packages/shared/src/mocks/codeListsResponse.ts (1)
1-48: Consider adding tests for the mock data.While the mock data is well-structured, consider adding unit tests to verify that the mock data matches the expected types and formats.
frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.test.tsx (1)
7-35: Consider adding tests for user interactions and error states.The current tests cover basic rendering. Consider adding tests for:
- User interactions with the code list menu
- Error states and loading states
- Edge cases like empty code lists
frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.test.ts (1)
31-55: Consider adding tests for error cases and validation.The current tests cover the happy path. Consider adding tests for:
- Failed uploads
- Invalid file formats
- Network errors
- Cache invalidation on error
frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.test.ts (1)
29-58: Consider adding error case test.While the happy path tests are well implemented, consider adding a test case for error scenarios to ensure proper error handling in the mutation hook.
describe('useUpdateOrgCodeListMutation', () => { beforeEach(jest.clearAllMocks); // ... existing tests ... + it('Handles API errors correctly', async () => { + const error = new Error('API Error'); + const updateCodeListForOrg = jest.fn(() => Promise.reject(error)); + const { result } = renderHookWithProviders(() => useUpdateOrgCodeListMutation(org), { + queries: { updateCodeListForOrg }, + }); + + await expect(result.current.mutateAsync(updatedCodeList)).rejects.toThrow(error); + }); });frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.test.ts (1)
29-60: Consider adding validation test.While the basic functionality is well tested, consider adding a test case to verify that the mutation validates the input data before making the API call.
describe('useCreateOrgCodeListMutation', () => { beforeEach(jest.clearAllMocks); // ... existing tests ... + it('Validates input data before API call', async () => { + const invalidCodeList = { title: '', data: [] }; + const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org)); + + await expect(result.current.mutateAsync(invalidCodeList)).rejects.toThrow(); + expect(queriesMock.createCodeListForOrg).not.toHaveBeenCalled(); + }); });frontend/packages/shared/src/mocks/textResourcesMock.ts (1)
3-87: Consider using a factory function for mock data generation.The current implementation has a lot of repetitive code. Consider using a factory function to generate mock data more efficiently and make it easier to maintain.
import type { ITextResource, ITextResourcesWithLanguage } from 'app-shared/types/global'; +const createTextResource = (type: string, index: number): ITextResource => ({ + id: `${type}${index}`, + value: `${type === 'label' ? 'Ledetekst' : type === 'description' ? 'Beskrivelse' : 'Hjelpetekst'} ${index}`, +}); + +const createTextResources = (type: string, count: number): ITextResource[] => + Array.from({ length: count }, (_, i) => createTextResource(type, i + 1)); + +const textResources: ITextResource[] = [ + ...createTextResources('label', 5), + ...createTextResources('description', 5), + ...createTextResources('helpText', 5), +]; + export const textResourcesMock: ITextResourcesWithLanguage = { language: 'nb', resources: textResources, };frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.ts (1)
89-89: Improve the implementation ofremoveLeadingSlash.
- Remove the unnecessary global flag 'g' from the regex since we're only matching the start of the string
- Add JSDoc documentation to maintain consistency with other methods
+ /** + * Removes leading slash from the given string. + * @param str The string to process. + * @returns The string with leading slash removed. + */ - static removeLeadingSlash = (str: string): string => str.replace(/^\//g, ''); + static removeLeadingSlash = (str: string): string => str.replace(/^\//, '');frontend/dashboard/app/App.test.tsx (1)
83-89: Consider using test.each for repetitive test cases.The helper functions improve readability, but consider using test.each for similar test cases that only differ in their input data.
- const getFavouriteAppListHeading = (): HTMLElement => - screen.getByRole('heading', { name: textMock('dashboard.favourites') }); - - const getLibraryHeading = (): HTMLElement => - screen.getByRole('heading', { name: textMock('app_content_library.library_heading') }); + const getHeading = (key: 'dashboard.favourites' | 'app_content_library.library_heading'): HTMLElement => + screen.getByRole('heading', { name: textMock(key) });Also applies to: 91-96
frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (1)
43-45: Consider extending TopNavigationMenuProps interface.The props interface could be extended to include optional styling or behavior customization.
type TopNavigationMenuProps = { menuItem: HeaderMenuItem; + className?: string; + onClick?: (event: React.MouseEvent) => void; };frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.test.tsx (1)
44-44: Consider using a function for cleanup.While
jest.clearAllMocksworks, wrapping it in a function is more consistent with Jest's cleanup patterns.- afterEach(jest.clearAllMocks); + afterEach(() => { + jest.clearAllMocks(); + });backend/src/Designer/Services/Implementation/OptionsService.cs (1)
23-23: Consider making the folder path configurable.While using a constant is good, consider making this path configurable through dependency injection or configuration for better flexibility.
- private const string OptionsFolderPath = "App/options/"; + private readonly string _optionsFolderPath; + + public OptionsService( + IAltinnGitRepositoryFactory altinnGitRepositoryFactory, + IConfiguration configuration) + { + _altinnGitRepositoryFactory = altinnGitRepositoryFactory; + _optionsFolderPath = configuration.GetValue<string>("OptionsFolderPath") ?? "App/options/"; + }backend/src/Designer/Controllers/PreviewController.cs (1)
65-65: Remove the trailing slash from the folder path.The trailing slash in
OptionsFolderPathis unnecessary and might cause issues withPath.Combine. Consider removing it to maintain consistency with other folder path constants in the file.-private const string OptionsFolderPath = "App/options/"; +private const string OptionsFolderPath = "App/options";backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (2)
813-816: Add XML documentation for theoptionsFolderPathparameter.The XML documentation for the new
optionsFolderPathparameter is missing a description. Consider adding one to maintain consistency with the documentation style./// <summary> /// Gets a specific options list with the provided id. /// </summary> /// <param name="optionsListId">The name of the options list to fetch.</param> - /// <param name="optionsFolderPath"></param> + /// <param name="optionsFolderPath">The folder path where options lists are stored.</param> /// <param name="cancellationToken">A <see cref="CancellationToken"/> that observes if operation is cancelled.</param> /// <returns>The options list as a string.</returns>
813-813: Add XML documentation for the new parameter.The XML documentation for the
optionsFolderPathparameter is missing. Add a description to maintain consistency with the rest of the codebase./// <summary> /// Gets a specific options list with the provided id. /// </summary> /// <param name="optionsListId">The name of the options list to fetch.</param> - /// <param name="optionsFolderPath"></param> + /// <param name="optionsFolderPath">The folder path where options lists are stored.</param> /// <param name="cancellationToken">A <see cref="CancellationToken"/> that observes if operation is cancelled.</param> /// <returns>The options list as a string.</returns>backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnAppGitRepositoryTests.cs (3)
267-267: Consider extracting the options folder path as a constant.The string "App/options" is duplicated across multiple test methods. Consider extracting it as a private constant at the class level to improve maintainability and reduce duplication.
public class AltinnAppGitRepositoryTests { + private const string TestOptionsFolderPath = "App/options";Also applies to: 286-286, 298-298, 314-314
267-268: Extract hardcoded path to a constant.The hardcoded path "App/options" is used in multiple test methods. Consider extracting it to a private constant at the class level to improve maintainability and ensure consistency.
public class AltinnAppGitRepositoryTests { + private const string TestOptionsFolderPath = "App/options"; [Fact] public async Task GetOptions_WithAppThatHasOptions_ShouldReturnSpecificOptionsList() { string org = "ttd"; string repository = "app-with-options"; string developer = "testUser"; string optionsId = "test-options"; - string optionsFolderPath = "App/options"; + string optionsFolderPath = TestOptionsFolderPath;
292-306: Add test cases for edge cases and error scenarios.The current test only verifies the happy path. Consider adding test cases for:
- Empty options list
- Invalid folder path
- Folder path with special characters
- Folder path with trailing slashes
🛑 Comments failed to post (11)
backend/src/Designer/Controllers/Organisation/CodeListController.cs (1)
70-77:
⚠️ Potential issueFix mismatched return type.
Your service call at line 75 returns a
List<OptionListData>object, yet the method signature declaresTask<ActionResult<List<Option>>>. This is inconsistent and will result in runtime type mismatches.Consider updating the method signature to match the underlying data type:
- public async Task<ActionResult<List<Option>>> CreateCodeList(string org, [FromRoute] string optionsListId, [FromBody] List<Option> payload, CancellationToken cancellationToken = default) + public async Task<ActionResult<List<OptionListData>>> CreateCodeList(string org, [FromRoute] string optionsListId, [FromBody] List<Option> payload, CancellationToken cancellationToken = default)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public async Task<ActionResult<List<OptionListData>>> CreateCodeList(string org, [FromRoute] string optionsListId, [FromBody] List<Option> payload, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); string developer = AuthenticationHelper.GetDeveloperUserName(HttpContext); var newOptionsList = await _codeListService.CreateCodeList(org, Repo, developer, optionsListId, payload, cancellationToken); return Ok(newOptionsList); }frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (1)
13-14:
⚠️ Potential issueFix the assertion to properly compare File objects.
Using
toBefor File objects might not work as expected since it checks for reference equality. Consider using more specific assertions to verify the file properties.- expect(retrievedFile).toBe(file); + expect(retrievedFile instanceof File).toBe(true); + expect((retrievedFile as File).name).toBe(fileName); + expect((retrievedFile as File).type).toBe(fileType); + expect((retrievedFile as File).size).toBe(file.size);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const retrievedFile = formData.get('file'); - expect(retrievedFile).toBe(file); + expect(retrievedFile instanceof File).toBe(true); + expect((retrievedFile as File).name).toBe(fileName); + expect((retrievedFile as File).type).toBe(fileType); + expect((retrievedFile as File).size).toBe(file.size);frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (2)
20-20: 🛠️ Refactor suggestion
Add error boundary and loading state.
The component should handle loading and error states gracefully.
- return <div>{getContentResourceLibrary()}</div>; + return ( + <ErrorBoundary fallback={<ErrorMessage />}> + <div> + {isLoading ? ( + <Spinner /> + ) : ( + getContentResourceLibrary() + )} + </div> + </ErrorBoundary> + );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return ( <ErrorBoundary fallback={<ErrorMessage />}> <div> {isLoading ? ( <Spinner /> ) : ( getContentResourceLibrary() )} </div> </ErrorBoundary> );
10-14:
⚠️ Potential issueImplement callback functions and data fetching.
The empty callback functions and
codeListsDataarray don't fulfill the PR's objective of updating codelist endpoint return types. Consider:
- Implementing the callback functions using the appropriate API mutations.
- Using
useOrgCodeListsQueryhook to fetch and populatecodeListsData.- Adding error handling and loading states.
Here's a suggested implementation:
+import { useOrgCodeListsQuery } from '@studio/shared/hooks/queries/useOrgCodeListsQuery'; +import { useCreateCodeListForOrgMutation, useUpdateCodeListForOrgMutation, useDeleteCodeListForOrgMutation, useUploadCodeListForOrgMutation } from '@studio/shared/hooks/mutations'; + export function OrgContentLibrary(): ReactElement { + const { data: codeListsData = [], isLoading, error } = useOrgCodeListsQuery(org); + const { mutate: createCodeList } = useCreateCodeListForOrgMutation(); + const { mutate: updateCodeList } = useUpdateCodeListForOrgMutation(); + const { mutate: deleteCodeList } = useDeleteCodeListForOrgMutation(); + const { mutate: uploadCodeList } = useUploadCodeListForOrgMutation(); + const { getContentResourceLibrary } = new ResourceContentLibraryImpl({ pages: { codeList: { props: { - codeListsData: [], - onDeleteCodeList: () => {}, - onUpdateCodeListId: () => {}, - onUpdateCodeList: () => {}, - onUploadCodeList: () => {}, + codeListsData, + onDeleteCodeList: deleteCodeList, + onUpdateCodeListId: updateCodeList, + onUpdateCodeList: updateCodeList, + onUploadCodeList: uploadCodeList, + isLoading, + error, }, }, }, });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { useOrgCodeListsQuery } from '@studio/shared/hooks/queries/useOrgCodeListsQuery'; import { useCreateCodeListForOrgMutation, useUpdateCodeListForOrgMutation, useDeleteCodeListForOrgMutation, useUploadCodeListForOrgMutation } from '@studio/shared/hooks/mutations'; export function OrgContentLibrary(): ReactElement { const { data: codeListsData = [], isLoading, error } = useOrgCodeListsQuery(org); const { mutate: createCodeList } = useCreateCodeListForOrgMutation(); const { mutate: updateCodeList } = useUpdateCodeListForOrgMutation(); const { mutate: deleteCodeList } = useDeleteCodeListForOrgMutation(); const { mutate: uploadCodeList } = useUploadCodeListForOrgMutation(); const { getContentResourceLibrary } = new ResourceContentLibraryImpl({ pages: { codeList: { props: { codeListsData, onDeleteCodeList: deleteCodeList, onUpdateCodeListId: updateCodeList, onUpdateCodeList: updateCodeList, onUploadCodeList: uploadCodeList, isLoading, error, }, }, }, }); // ... rest of your component logic }frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.ts (1)
12-13:
⚠️ Potential issueInclude dynamic parameters in query key.
The query key should include the dynamic parameters
organdlanguageto ensure proper cache invalidation.return useQuery<ITextResourcesWithLanguage>({ - queryKey: [QueryKey.TextResourcesForOrg], + queryKey: [QueryKey.TextResourcesForOrg, org, language], queryFn: () => getTextResourcesForOrg(org, language),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.queryKey: [QueryKey.TextResourcesForOrg, org, language], queryFn: () => getTextResourcesForOrg(org, language),frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.test.ts (1)
7-12: 🛠️ Refactor suggestion
Add test cases for error and loading states.
The test suite should cover error scenarios and loading states.
describe('useOrgCodeListsQuery', () => { it('calls getCodeListsForOrg with the correct parameters', () => { render(); expect(queriesMock.getCodeListsForOrg).toHaveBeenCalledWith(org); }); + + it('handles loading state correctly', async () => { + const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); + expect(result.current.isLoading).toBe(true); + }); + + it('handles error state correctly', async () => { + queriesMock.getCodeListsForOrg.mockRejectedValueOnce(new Error('Failed to fetch')); + const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.error).toBeDefined(); + }); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.describe('useOrgCodeListsQuery', () => { it('calls getCodeListsForOrg with the correct parameters', () => { render(); expect(queriesMock.getCodeListsForOrg).toHaveBeenCalledWith(org); }); it('handles loading state correctly', async () => { const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); expect(result.current.isLoading).toBe(true); }); it('handles error state correctly', async () => { queriesMock.getCodeListsForOrg.mockRejectedValueOnce(new Error('Failed to fetch')); const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error).toBeDefined(); }); });frontend/dashboard/dashboardTestUtils.tsx (1)
32-34:
⚠️ Potential issueReconsider using BrowserRouter in tests.
Using
BrowserRouterin tests might lead to issues as it manipulates the actual browser history. Consider usingMemoryRouterinstead, which is designed for testing and provides a self-contained environment.- <BrowserRouter> + <MemoryRouter> <ServicesContextProvider {...queries}>{children}</ServicesContextProvider> - </BrowserRouter> + </MemoryRouter>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<MemoryRouter> <ServicesContextProvider {...queries}>{children}</ServicesContextProvider> </MemoryRouter>frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.test.ts (1)
12-28: 🛠️ Refactor suggestion
Add cleanup and enhance test coverage.
Consider the following improvements:
- Add
beforeEach(jest.clearAllMocks)for consistent test state- Add test cases for:
- Error handling
- Invalid text resources
- Empty text resources array
- Network failures
Example addition:
describe('useUpdateTextResourcesForOrgMutation', () => { + beforeEach(jest.clearAllMocks); + it('Calls updateTextResourcesForOrg with correct arguments and payload', async () => { // ... existing test }); + + it('Handles error cases appropriately', async () => { + // Add error case test + }); + + it('Validates text resources before update', async () => { + // Add validation test + }); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.describe('useUpdateTextResourcesForOrgMutation', () => { beforeEach(jest.clearAllMocks); it('Calls updateTextResourcesForOrg with correct arguments and payload', async () => { const { result } = renderHookWithProviders(() => useUpdateTextResourcesForOrgMutation(org, languageMock), ); result.current.mutate(textResourcesMock); await waitFor(() => expect(result.current.isSuccess).toBe(true)); expect(queriesMock.updateTextResourcesForOrg).toHaveBeenCalledTimes(1); expect(queriesMock.updateTextResourcesForOrg).toHaveBeenCalledWith( org, languageMock, textResourcesMock, ); }); it('Handles error cases appropriately', async () => { // Add error case test }); it('Validates text resources before update', async () => { // Add validation test }); });frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts (1)
45-49:
⚠️ Potential issueFix incorrect hook usage in cache update test.
The test is using
useCreateOrgCodeListMutationinstead ofuseDeleteOrgCodeListMutation. This is likely a copy-paste error and needs to be fixed.- const createCodeListForOrg = jest.fn(() => Promise.resolve([otherCodeList])); - const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org), { + const deleteCodeListForOrg = jest.fn(() => Promise.resolve([otherCodeList])); + const { result } = renderHookWithProviders(() => useDeleteOrgCodeListMutation(org), { queryClient, - queries: { createCodeListForOrg }, + queries: { deleteCodeListForOrg }, });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const deleteCodeListForOrg = jest.fn(() => Promise.resolve([otherCodeList])); const { result } = renderHookWithProviders(() => useDeleteOrgCodeListMutation(org), { queryClient, queries: { deleteCodeListForOrg }, });frontend/packages/shared/src/api/queries.ts (1)
183-185: 💡 Verification agent
🧩 Analysis chain
Mock implementation with clear TODO reference.
The implementation returns mock data with a clear reference to the future API endpoint (issue #14505).
Please track the TODO comment and ensure the implementation is updated when the API endpoint is ready:
🌐 Web query:
Is GitHub issue #14505 still open?💡 Result:
GitHub issue #14505 exists in multiple repositories with different statuses:
Podman (containers/podman)
Closed on September 28, 2022, after healthcheck restart capability was added[1][9].Tailscale
Open as of January 16, 2025 (feature request for CentOS Stream 10 support)[7].Bun (oven-sh/bun)
Open as of February 10, 2025 (opened ~5 hours ago)[9].Snipe-IT
Open since March 27, 2024 (feature request for shipping labels)[10].Other notable mentions:
- Notepad++: Closed after a fix in December 2023[4].
- FilamentPHP: Closed after reproduction steps were provided[3].
- OpenZFS: Open as a feature request since February 2023[5].
To determine if a specific #14505 is open, the repository name must be specified.
Citations:
- 1: Unhealthy pod(created via podman play kube) doesn't restart containers/podman#14505
- 2: Node-exporter failed to start kyma-project/kyma#14505
- 3: Bug-report project does not run out-of-the-box filamentphp/filament#14505
- 4: v.8.6 64bit: "Replace All in All Opened Documents" does double replacement on document and its clone notepad-plus-plus/notepad-plus-plus#14505
- 5: resilver_defer should have a threshold for triggering openzfs/zfs#14505
- 6: Login page should automatically focus on the email input field metabase/metabase#14505
- 7: 🔌 Plugin: Prometheus metrics + analytics plugin backstage/backstage#14505
- 8: FR: install.sh - add support for CentOS Stream 10 tailscale/tailscale#14505
- 9: Why this randomly happen? oven-sh/bun#14505
- 10: [Feature Request]:SHIPPING LABEL GENERATING snipe/snipe-it#14505
I'll do a refined web query targeting the Altinn Studio repository to determine if issue #14505 is still open.
🌐 Web query:
Is the GitHub issue https://github.com/Altinn/altinn-studio/issues/14505 still open?💡 Result:
The GitHub issue #14505 in the Altinn/altinn-studio repository is not listed in the current open issues from the provided search results[5]. The latest visible issues range from #14623 to #14582 (as of February 10, 2025), and there is no mention of #14505 in any of the active or closed issue lists provided[1][2][3][4][5][6][7][8][9].
Given GitHub’s sequential issue numbering and the absence of this specific issue ID in recent activity logs, it likely has been resolved or closed. For absolute certainty, you should verify directly on GitHub.
Citations:
- 1: https://github.com/Altinn/altinn-studio/releases
- 2: https://github.com/Altinn/altinn-platform/issues
- 3: https://github.com/Altinn/altinn-studio
- 4: https://github.com/Altinn/altinn-studio-docs/issues
- 5: https://github.com/Altinn/altinn-studio/issues
- 6: https://github.com/altinn
- 7: https://github.com/Altinn/altinn-resource-registry/issues
- 8: https://github.com/Altinn/altinn-studio-docs/activity
- 9: https://github.com/Altinn/altinn-studio-charts/issues
Action Required: Replace Mock API Implementation with Real Call
The web query indicates that GitHub issue #14505 in the Altinn/altinn-studio repository is no longer open. This suggests that the API endpoint intended to replace the mock may now be available. Accordingly, update the implementation in:
- File:
frontend/packages/shared/src/api/queries.ts(lines 183-185)to replace the
Promise.resolvemocks with the real API calls and remove or update the now-outdated TODO comments.backend/src/Designer/Controllers/PreviewController.cs (1)
65-65:
⚠️ Potential issueStandardize path format across codebase.
The path format "App/options/" includes a trailing slash, while in tests it's "App/options". This inconsistency could lead to path resolution issues. Consider standardizing the path format across the codebase.
- private const string OptionsFolderPath = "App/options/"; + private const string OptionsFolderPath = "App/options";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private const string OptionsFolderPath = "App/options";
Konrad-Simso
removed
area/dashboard
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## org-library-mvp #14622 +/- ##
================================================
Coverage 95.73% 95.73%
================================================
Files 1919 1919
Lines 24948 24948
Branches 2838 2838
================================================
Hits 23885 23885
Misses 802 802
Partials 261 261 ☔ View full report in Codecov by Sentry. |
|
Closing this PR as it's covered by #14650 |
Description
All endpoints now return the current version of the options lists.
This will make it easier to use in our frontend as the cache can be set by a new copy after every sucessful update or creation and not updated/altered.
Related Issue
Verification
Summary by CodeRabbit
New Features
Improvements