fix: update org codelist controller #14667
Conversation
…po const from Controller to service.
|
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 functionality for managing organization-level code lists and options. A new controller and corresponding service have been added to handle CRUD operations for code lists. Existing controllers, repositories, and factory methods are updated to support dynamic folder paths for retrieving options. In addition, interface additions, dependency registrations, and extensive updates to backend tests and frontend components (including hooks, UI routing, and styles) ensure that code list operations are integrated consistently across the application and its shared packages. 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
Konrad-Simso
added
skip-releasenotes
Konrad-Simso
changed the title
![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: 7
🧹 Nitpick comments (45)
backend/src/Designer/Services/Implementation/OptionsService.cs (1)
23-23: Consider moving the path to configuration.While the constant is well-named and uses cross-platform compatible path separators, hardcoding the path might make it less flexible for different environments or future changes.
Consider moving this to configuration (e.g., appsettings.json) for better maintainability:
- private const string OptionsFolderPath = "App/options/";Add to your configuration class:
public class AppSettings { public string OptionsFolderPath { get; set; } }And inject it via constructor:
private readonly string _optionsFolderPath; public OptionsService(IOptions<AppSettings> settings, ...) { _optionsFolderPath = settings.Value.OptionsFolderPath; }frontend/dashboard/app/App.tsx (3)
25-39: Consider handling additional default or idle states.
React-query can also yield an'idle'status or additional states. A default case in the switch statement could handle unexpected statuses and log or display an error.switch (queryStatus) { case 'pending': return <PendingPage />; case 'error': return <ErrorMessage userStatus={userStatus} />; case 'success': return <AppWithData user={user} organizations={organizations} />; + default: + console.warn('Unhandled queryStatus:', queryStatus); + return <ErrorMessage userStatus={userStatus} />; }
74-90: AppWithData routing structure looks good.
The nested routes and page layout are cleanly organized. Consider adding a 404 or fallback route for unknown paths.
92-103: SubrouteGuard missing a default case.
If additional subroutes are introduced later, returning a placeholder or fallback component could enhance user experience.switch (subrouteWithLeadingSlash) { case APP_DASHBOARD_BASENAME: return <Dashboard {...props} />; case ORG_LIBRARY_BASENAME: return <OrgContentLibrary />; + default: + return <StudioPageError title="Not Found" message="Unknown subroute" />; }frontend/dashboard/hooks/guards/useContextRedirectionGuard.ts (1)
41-52: handleContextRedirection approach is concise.
Conditionally redirecting to self or a different context is well-structured. Consider adding logs or telemetry for debugging.backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (1)
95-109: Consider consistency in directory creation for UpdateCodeList.
While update typically expects an existing file, if the folder is missing, this operation may fail. Consider settingcreateDirectory: trueif you want to allow newly added code lists to be updated in all scenarios.await WriteTextByRelativePathAsync( codeListFilePath, codeListString, - false, + true, cancellationToken );backend/src/Designer/Controllers/Organisation/OrgCodeListController.cs (2)
53-56: Consider using 404 status code instead of 204
When no code lists are found, responding withNoContent()might confuse clients expecting a 404 for “not found.” Evaluate whether returning a 404 would more accurately reflect the resource’s absence.- return NoContent(); + return NotFound();
127-151: Return consistent status codes on non-existent code list deletes
If a code list does not exist, the method returns HTTP 200 along with a message stating the file has been deleted. This may mislead clients. Consider returning 404 or a specific status to indicate that it did not exist.if (CodeListExists) { ... } else { - return Ok($"The code-list file {codeListId}.json has been deleted."); + return NotFound($"No code-list found for {codeListId}.json"); }backend/src/Designer/Services/Implementation/Organisation/OrgCodeListService.cs (1)
99-99: Handle edge cases for filename parsing
Replacing".json"inpayload.FileNameassumes the extension exists exactly once. You might want to validate or handle cases where the file extension is missing or the filename contains multiple occurrences of".json".- string codeListId = payload.FileName.Replace(".json", ""); + string fileNameWithoutExtension = Path.GetFileNameWithoutExtension(payload.FileName); + string codeListId = fileNameWithoutExtension ?? "default_code_list";frontend/packages/shared/src/types/CodeListData.ts (1)
3-7: Add JSDoc comments to document the type.Consider adding JSDoc comments to document the purpose of each field, especially
hasErrorto clarify when it's set to true.+/** + * Represents a code list with its data and status. + */ export type CodeListData = { + /** The title/name of the code list */ title: string; + /** Optional array of code list items */ data?: CodeListItem[]; + /** Indicates if there was an error processing the code list */ hasError?: boolean; };frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts (1)
1-7: Convert class to standalone function.Since this utility contains only a single static method, it would be cleaner to implement it as a standalone function rather than a class.
-export class FileUtils { - static convertToFormData = (file: File): FormData => { - const formData = new FormData(); - formData.append('file', file); - return formData; - }; -} +/** + * Converts a File object to FormData. + */ +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/pages/OrgContentLibrary/utils.ts (1)
3-10: Improve type safety and performance.Consider these improvements:
- Use the enum type directly instead of string
- Use a Set for more efficient lookups
- Consider positive logic for better readability
-export function isOrg(contextType: string): boolean { - const notOrgContexts: string[] = [ +export function isOrg(contextType: SelectedContextType): boolean { + const notOrgContexts = new Set<SelectedContextType>([ SelectedContextType.Self, SelectedContextType.All, SelectedContextType.None, - ]; - return !notOrgContexts.includes(contextType); + ]); + return !notOrgContexts.has(contextType); }Alternative with positive logic:
export function isOrg(contextType: SelectedContextType): boolean { return contextType !== SelectedContextType.Self && contextType !== SelectedContextType.All && contextType !== SelectedContextType.None; }frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (2)
13-14: Use appropriate matcher for File object comparison.The
toBematcher performs strict equality comparison which might not be appropriate for File objects. Consider usingtoEqualor a custom matcher that compares relevant File properties.- expect(retrievedFile).toBe(file); + expect(retrievedFile).toEqual(file);
3-17: Enhance test coverage with additional test cases.Consider adding tests for:
- Error cases (e.g., null/undefined file)
- Different file types (e.g., images, PDFs)
- Large files
- Files with special characters in names
Would you like me to generate additional test cases to improve coverage?
frontend/dashboard/pages/PageLayout/DashboardHeader/dashboardHeaderMenuItems.ts (2)
8-12: Add JSDoc comments to document the interface.Consider adding JSDoc comments to document the purpose and usage of the
HeaderMenuIteminterface and its properties.+/** + * Represents a menu item in the dashboard header navigation. + */ export interface HeaderMenuItem { + /** Unique identifier for the menu item */ key: HeaderMenuItemKey; + /** Route path for the menu item */ link: string; + /** Translation key for the menu item's display text */ name: string; }
14-25: Consider making the menu items array readonly.Since this is a constant array of menu items, consider making it immutable to prevent accidental modifications.
-export const dashboardHeaderMenuItems: HeaderMenuItem[] = [ +export const dashboardHeaderMenuItems: ReadonlyArray<HeaderMenuItem> = [frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.ts (1)
12-17: Add error handling to the mutation.Consider adding error handling to manage failed deletions gracefully.
Apply this diff to add error handling:
return useMutation({ mutationFn, onSuccess: (newData: CodeListsResponse) => { queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); }, + onError: (error: Error) => { + // Handle error (e.g., show error notification) + console.error('Failed to delete code list:', error); + }, });frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.test.ts (1)
7-12: LGTM! Consider adding more test cases.The test correctly verifies the basic functionality of calling
getCodeListsForOrgwith the right parameter.Consider adding test cases for:
- Error handling
- Loading state
- Empty response
- Response with data
frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.ts (1)
6-19: Consider adding error handling and optimistic updates.The mutation hook is well-structured but could benefit from additional features.
Consider adding:
- Error handling with
onErrorcallback- Optimistic updates using
onMutate- Cache rollback on error using
onErrorExample implementation:
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, _, context) => { + // Rollback to previous state on error + if (context?.previousData) { + q.setQueryData([QueryKey.TextResourcesForOrg, org], context.previousData); + } + }, + onMutate: async () => { + // Cancel outgoing fetches + await q.cancelQueries([QueryKey.TextResourcesForOrg, org]); + // Save previous data + const previousData = q.getQueryData([QueryKey.TextResourcesForOrg, org]); + return { previousData }; + }, }); };frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.ts (1)
6-19: Consider adding error handling and optimistic updates.The mutation hook is well-structured but could benefit from additional features. Similar improvements as suggested for
useCreateTextResourcesForOrgMutationwould be beneficial here.Consider adding:
- Error handling with
onErrorcallback- Optimistic updates using
onMutate- Cache rollback on error using
onErrorExample implementation:
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, _, context) => { + // Rollback to previous state on error + if (context?.previousData) { + q.setQueryData([QueryKey.TextResourcesForOrg, org], context.previousData); + } + }, + onMutate: async (payload) => { + // Cancel outgoing fetches + await q.cancelQueries([QueryKey.TextResourcesForOrg, org]); + // Save previous data + const previousData = q.getQueryData([QueryKey.TextResourcesForOrg, org]); + // Optimistically update the cache + q.setQueryData([QueryKey.TextResourcesForOrg, org], (old: ITextResourcesWithLanguage[]) => { + // Update logic here + return old; + }); + return { previousData }; + }, }); };frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.ts (1)
9-22: Consider adding error handling and optimistic updates.The mutation hook is well-structured with good separation of concerns, but could benefit from additional features.
Consider adding:
- Error handling with
onErrorcallback- Optimistic updates using
onMutate- Cache rollback on error using
onErrorExample implementation:
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); }, + onError: (error, _, context) => { + // Rollback to previous state on error + if (context?.previousData) { + queryClient.setQueryData([QueryKey.OrgCodeLists, org], context.previousData); + } + }, + onMutate: async ({ title, data }) => { + // Cancel outgoing fetches + await queryClient.cancelQueries([QueryKey.OrgCodeLists, org]); + // Save previous data + const previousData = queryClient.getQueryData([QueryKey.OrgCodeLists, org]); + // Optimistically update the cache + queryClient.setQueryData([QueryKey.OrgCodeLists, org], (old: CodeListsResponse) => { + // Update logic here + return old; + }); + return { previousData }; + }, }); };frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.test.ts (1)
9-17: Consider adding error and edge case tests.The test suite currently only covers the happy path. Consider adding tests for:
- Error states
- Empty or invalid language
- Network failures
- Loading states
frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.test.ts (1)
12-22: Enhance test coverage with additional test cases.Consider adding tests for:
- Failed mutations
- Invalid payloads
- Network errors
- Loading states
frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.test.ts (1)
12-28: Add beforeEach cleanup and error cases.While the happy path is well tested, consider:
- Adding
beforeEach(jest.clearAllMocks)for consistent test state- Including tests for:
- Failed mutations
- Invalid text resources
- Network errors
- Loading states
frontend/packages/shared/src/constants.js (1)
3-3: Consider grouping related route constants.The new route constants (
APP_DASHBOARD_BASENAMEandORG_LIBRARY_BASENAME) are related to navigation. Consider grouping all route-related constants together for better organization.Example grouping:
+ // Route basenames export const APP_DEVELOPMENT_BASENAME = '/editor'; export const APP_DASHBOARD_BASENAME = '/app-dashboard'; export const DASHBOARD_BASENAME = '/dashboard'; export const ORG_LIBRARY_BASENAME = '/org-library'; export const PREVIEW_BASENAME = '/preview';Also applies to: 7-7
frontend/packages/shared/src/mocks/codeListsResponse.ts (1)
15-31: Consider adding more edge cases to codeList1.While the mock data is well-structured, consider adding test cases for:
- Empty description/helpText
- Special characters in values
- Very long text values
backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs (1)
13-13: Fix typo in parameter documentation.There's a typo in the word "identified" in the parameter documentation.
- /// <param name="org">The organization owning the repository identfied by it's short name as defined in Gitea.</param> + /// <param name="org">The organization owning the repository identified by its short name as defined in Gitea.</param>frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.test.ts (1)
43-57: Consider adding error case testing.While the happy path is well tested, consider adding test cases for:
- API failure scenarios
- Network errors
- Invalid input validation
frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.test.ts (1)
45-59: Consider adding validation test cases.While the basic creation flow is well tested, consider adding test cases for:
- Duplicate title handling
- Invalid code list data validation
- Empty title/data scenarios
frontend/packages/shared/src/mocks/textResourcesMock.ts (1)
1-87: Add documentation and consider i18n best practices.The mock data structure is well-organized, but could benefit from:
- Documentation explaining the purpose and usage of these mocks
- Using i18n keys instead of hardcoded Norwegian text to align with internationalization practices
Consider adding JSDoc comments and refactoring to use i18n keys:
+/** + * Mock text resources for testing purposes. + * Used in conjunction with text resource API mutations and components. + */ import type { ITextResource, ITextResourcesWithLanguage } from 'app-shared/types/global'; export const label1TextResource: ITextResource = { id: 'label1', - value: 'Ledetekst 1', + value: 'shared.text_resources.label1', };frontend/packages/shared/src/types/QueryKey.ts (1)
54-54: Maintain alphabetical ordering in enum.The
TextResourcesForOrgmember should be placed alphabetically betweenTextResourcesandWidgetsto maintain consistency.- TextResourcesForOrg = 'TextResourcesForOrg', DataType = 'DataType', // Resourceadm + TextResourcesForOrg = 'TextResourcesForOrg', ResourceList = 'ResourceList',frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (1)
82-86: Enhance error handling specificity.The error handling could be more specific by handling different types of errors separately.
onError: (error: AxiosError<ApiError>) => { if (isErrorUnknown(error)) { toast.error(t('dashboard.org_library.code_list_upload_generic_error')); + } else if (error.response?.status === 413) { + toast.error(t('dashboard.org_library.code_list_upload_size_error')); + } else if (error.response?.status === 400) { + toast.error(t('dashboard.org_library.code_list_upload_format_error')); } },frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.ts (1)
89-89: Optimize regex and add documentation.The implementation can be improved in two ways:
- Simplify regex as multiple leading slashes can be handled with a single replacement
- Add JSDoc documentation for consistency with other methods
+ /** + * Removes leading slash from a 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(/^\//, '');backend/src/Designer/Services/Interfaces/Organisation/IOrgCodeListService.cs (1)
60-60: Consider return type consistency for DeleteCodeList.The
DeleteCodeListmethod returnsList<OptionListData>which seems unnecessary for a deletion operation. Consider returning a boolean or void instead.- public Task<List<OptionListData>> DeleteCodeList(string org, string developer, string codeListId, CancellationToken cancellationToken = default); + public Task<bool> DeleteCodeList(string org, string developer, string codeListId, CancellationToken cancellationToken = default);frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (1)
75-78: Consider simplifying path extraction logic.The
extractSecondLastRouterParamfunction could be simplified using array destructuring.-function extractSecondLastRouterParam(pathname: string): string { - const pathnameArray = pathname.split('/'); - return pathnameArray[pathnameArray.length - 2]; -} +function extractSecondLastRouterParam(pathname: string): string { + const [..., secondLast = ''] = pathname.split('/').slice(0, -1); + return secondLast; +}backend/src/Designer/Controllers/Preview/InstancesController.cs (2)
189-189: Consider implementing dynamic options list support.The TODO comment indicates missing functionality for dynamic options list based on language and source.
Would you like me to help implement the dynamic options list functionality or create an issue to track this task?
195-199: Review error handling approach.Currently, when an option list is not found, an empty list is returned to prevent frontend issues. Consider:
- Logging the error for monitoring
- Adding a specific response status code or header to indicate the fallback behavior
catch (NotFoundException) { + // Log the error for monitoring + _logger.LogWarning("Options list {optionListId} not found, returning empty list", optionListId); // Return empty list since app-frontend don't handle a null result - return Ok(new List<string>()); + return StatusCode(StatusCodes.Status204NoContent, new List<string>()); }frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.test.tsx (1)
158-168: Consider adding assertion for upload function parameters.While the test verifies the function call and error message hiding, it would be beneficial to also assert the structure of the FormData being passed to the upload function.
expect(queriesMock.uploadCodeListForOrg).toHaveBeenCalledWith(org, expect.any(FormData)); +const formDataArg = queriesMock.uploadCodeListForOrg.mock.calls[0][1]; +expect(formDataArg.get('file')).toBeTruthy(); +expect(formDataArg.get('file').name).toBe(`${codeListNameMock}.json`);backend/src/Designer/Controllers/Preview/V3/OldInstancesController.cs (1)
30-30: Consider adding XML documentation for the constant.Adding XML documentation for the
OptionsFolderPathconstant would improve code maintainability.+/// <summary> +/// The folder path where options are stored in the app repository. +/// </summary> private const string OptionsFolderPath = "App/options/";frontend/dashboard/pages/CreateService/CreateService.test.tsx (1)
315-326: Consider consolidating similar test cases.The test cases for different context types (all, org) follow the same pattern. Consider using a parameterized test to reduce code duplication.
-it('should set cancel link to /all when selected context is all', async () => { - const selectedContext = SelectedContextType.All; - 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}`); -}); - -it('should set cancel link to /org when selected context is org', async () => { - const selectedContext = 'ttd'; - 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}`); -}); +it.each([ + [SelectedContextType.All, 'all'], + ['ttd', 'org'] +])('should set cancel link to /%s when selected context is %s', async (selectedContext, contextType) => { + 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}`); +});Also applies to: 328-339
frontend/packages/shared/src/api/queries.ts (2)
184-185: Consider adding return type annotations for better type safety.While TypeScript can infer the return type, explicit return type annotations improve code readability and maintainability.
-export const getCodeListsForOrg = async (org: string): Promise<CodeListsResponse> => Promise.resolve(codeListsResponse); +export const getCodeListsForOrg = async (org: string): Promise<CodeListsResponse> => { + return Promise.resolve(codeListsResponse); +};
182-185: Consider adding error handling for the mock implementation.Even with mock data, it's good practice to simulate potential error scenarios that might occur in the real API.
// Organisation library -export const getCodeListsForOrg = async (org: string): Promise<CodeListsResponse> => Promise.resolve(codeListsResponse); +export const getCodeListsForOrg = async (org: string): Promise<CodeListsResponse> => { + if (!org) { + throw new Error('Organization ID is required'); + } + return Promise.resolve(codeListsResponse); +};backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (2)
845-845: Consider using the provided optionsFolderPath parameter.The
CreateOrOverwriteOptionsListmethod is still using the hardcodedOptionsFolderPathconstant. Consider updating it to use the dynamicoptionsFolderPathparameter for consistency with the other methods.-string optionsFilePath = Path.Combine(OptionsFolderPath, $"{optionsListId}.json"); +string optionsFilePath = Path.Combine(optionsFolderPath, $"{optionsListId}.json");
858-858: Consider using the provided optionsFolderPath parameter.The
DeleteOptionsListandUpdateOptionsListIdmethods are still using the hardcodedOptionsFolderPathconstant. Consider updating them to use the dynamicoptionsFolderPathparameter for consistency with the other methods.-string optionsFilePath = Path.Combine(OptionsFolderPath, $"{optionsListId}.json"); +string optionsFilePath = Path.Combine(optionsFolderPath, $"{optionsListId}.json"); -string currentFilePath = Path.Combine(OptionsFolderPath, oldOptionsListFileName); -string newFilePath = Path.Combine(OptionsFolderPath, newOptionsListFileName); +string currentFilePath = Path.Combine(optionsFolderPath, oldOptionsListFileName); +string newFilePath = Path.Combine(optionsFolderPath, newOptionsListFileName);Also applies to: 874-875
frontend/language/src/nb.json (1)
235-238: Consider adding periods consistently to alert messages.While the translations are clear and well-written, consider adding periods at the end of all alert messages for consistency, as some other alert messages in the file use periods. For example:
- "dashboard.org_library.alert_no_org_selected": "For å vise biblioteket, må du velge en organisasjon i menyen øverst til høyre", + "dashboard.org_library.alert_no_org_selected": "For å vise biblioteket, må du velge en organisasjon i menyen øverst til høyre.", - "dashboard.org_library.alert_no_org_selected_no_access": "Hvis du ikke har tilgang til noen organisasjoner, må du be om tilgang fra en tjenesteeier", + "dashboard.org_library.alert_no_org_selected_no_access": "Hvis du ikke har tilgang til noen organisasjoner, må du be om tilgang fra en tjenesteeier."
🛑 Comments failed to post (7)
backend/src/Designer/Services/Implementation/Organisation/OrgCodeListService.cs (1)
38-38:
⚠️ Potential issueFix invalid list initialization syntax
UsingList<OptionListData> codeLists = [];is not valid in C#. Change it to use object initialization or a constructor call.- List<OptionListData> codeLists = []; + List<OptionListData> codeLists = new List<OptionListData>();📝 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.List<OptionListData> codeLists = new List<OptionListData>();frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.ts (1)
12-13:
⚠️ Potential issueInclude parameters in the query key for proper caching.
The query key should include both
organdlanguageparameters to ensure proper caching and data invalidation.Apply this diff to fix the query key:
- queryKey: [QueryKey.TextResourcesForOrg], + queryKey: [QueryKey.TextResourcesForOrg, 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/mutations/useUploadOrgCodeListMutation.test.ts (1)
31-55: 🛠️ Refactor suggestion
Add error case tests.
The test suite should include cases for:
- Upload failure
- Invalid file format
- Network errors
Example test case:
it('Handles upload failure correctly', async () => { const queryClient = createQueryClientMock(); const uploadError = new Error('Upload failed'); const uploadCodeListForOrg = jest.fn(() => Promise.reject(uploadError)); const { result } = renderHookWithProviders(() => useUploadOrgCodeListMutation(org), { queryClient, queries: { uploadCodeListForOrg }, }); await expect(result.current.mutateAsync(file)).rejects.toThrow(uploadError); expect(queryClient.getQueryData([QueryKey.OrgCodeLists, org])).toBeUndefined(); });frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts (1)
46-49:
⚠️ Potential issueFix incorrect hook usage in cache update test.
The test is using
useCreateOrgCodeListMutationinstead ofuseDeleteOrgCodeListMutation. This is testing the wrong mutation.Apply this diff to fix the test:
- 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/dashboard/pages/CreateService/CreateService.tsx (1)
91-91: 💡 Verification agent
🧩 Analysis chain
Add validation for route parameters.
The href construction assumes both
subrouteandselectedContextare defined. Consider adding validation or fallback values to prevent potential navigation issues.- href: `/${subroute}/${selectedContext}`, + href: `/${subroute || ''}/${selectedContext || ''}`.replace(/\/+/g, '/'),Let's verify the usage of these parameters:
🏁 Script executed:
#!/bin/bash # Search for other usages of subroute and selectedContext in href construction rg -l 'subroute.*selectedContext.*href|selectedContext.*subroute.*href'Length of output: 73
Below is an additional verification step to further inspect the file for occurrences of both the parameters and any fallback validations. Please run the following shell script so we can see if there’s already any handling for fallback values in the
CreateService.tsxfile:Once we verify that there isn’t already any fallback or validation logic around these parameters, we can confidently note that the original suggestion to add validation (or provide fallback values) remains necessary.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for occurrences of 'subroute' in CreateService.tsx:" rg -n "subroute" frontend/dashboard/pages/CreateService/CreateService.tsx echo "Searching for occurrences of 'selectedContext' in CreateService.tsx:" rg -n "selectedContext" frontend/dashboard/pages/CreateService/CreateService.tsx echo "Searching for href construction in CreateService.tsx (check for any fallback validations):" rg -n "href:" frontend/dashboard/pages/CreateService/CreateService.tsx echo "Searching for fallback pattern usage (|| operator) in CreateService.tsx:" rg -n "||" frontend/dashboard/pages/CreateService/CreateService.tsxLength of output: 4635
Add validation for route parameters to prevent navigation issues.
The current href construction on line 91 assumes that both
subrouteandselectedContextare always defined, which could lead to broken URL paths if either is undefined. Consider adding fallback values or validation logic to ensure the URL remains well-formed. For example, you could modify the code as follows:- href: `/${subroute}/${selectedContext}`, + href: `/${subroute || ''}/${selectedContext || ''}`.replace(/\/+/g, '/'),This change will help to avoid issues with extra or missing slashes when one of the parameters is undefined.
📝 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.href: `/${subroute || ''}/${selectedContext || ''}`.replace(/\/+/g, '/'),frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (1)
35-37: 🛠️ Refactor suggestion
Add error handling to handleUpdate function.
The
handleUpdatefunction should include error handling to provide feedback when the update fails.const handleUpdate = ({ title, codeList }: CodeListWithMetadata): void => { - updateOptionList({ title, data: codeList }); + updateOptionList({ title, data: codeList }, { + onError: (error: AxiosError<ApiError>) => { + if (isErrorUnknown(error)) { + toast.error(t('dashboard.org_library.code_list_update_generic_error')); + } + }, + onSuccess: () => { + toast.success(t('dashboard.org_library.code_list_update_success')); + }, + }); };📝 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 handleUpdate = ({ title, codeList }: CodeListWithMetadata): void => { - updateOptionList({ title, data: codeList }); + updateOptionList({ title, data: codeList }, { + onError: (error: AxiosError<ApiError>) => { + if (isErrorUnknown(error)) { + toast.error(t('dashboard.org_library.code_list_update_generic_error')); + } + }, + onSuccess: () => { + toast.success(t('dashboard.org_library.code_list_update_success')); + }, + }); };backend/src/Designer/Services/Interfaces/Organisation/IOrgCodeListService.cs (1)
32-41: 🛠️ Refactor suggestion
Fix incorrect documentation for UpdateCodeList.
The documentation states "Adds a new option to the code list" but the method actually updates the entire code list.
/// <summary> - /// Adds a new option to the code list. + /// Updates an existing code list with new contents. /// If the file already exists, it will be overwritten. /// </summary>📝 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./// <summary> /// Updates an existing code list with new contents. /// If the file already exists, it will be overwritten. /// </summary> /// <param name="org">Organisation</param> /// <param name="developer">Username of developer</param> /// <param name="codeListId">Name of the new code list</param> /// <param name="codeList">The code list contents</param> /// <param name="cancellationToken">A <see cref="CancellationToken"/> that observes if operation is cancelled.</param> public Task<List<OptionListData>> UpdateCodeList(string org, string developer, string codeListId, List<Option> codeList, CancellationToken cancellationToken = default);
backend/src/Designer/Services/Interfaces/Organisation/IOrgCodeListService.cs
Outdated
Show resolved
Hide resolved
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Description
Update
OrgCodeListControllerandOrgCodeListServiceto find the correct repository.Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Enhancements