fix: 14718 add confirm when deleting row in codelist #14726
Conversation
📝 WalkthroughWalkthroughThis PR introduces new backend controllers and services for managing organization-level code lists and text resources, including full CRUD operations and file upload support. Repository factories and Git repository methods are updated to support dynamic options folder paths. New interfaces and service registrations are provided for organization-specific functionalities. On the frontend, routing and dashboard components are restructured to handle dynamic subroutes, and new tests have been added across UI components, libraries, API mutations, and Playwright end-to-end tests. Enhancements in utility functions and type definitions further streamline code list and text resource management across the application. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. frontend/libs/studio-components/src/components/StudioCodelistEditor/types/CodeListEditorTexts.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
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/ui-editor
|
Jeg vet ikke om vi burde legge til bekreftelsesbokser på dette nivået. Det kan bli irriterende for brukerne hvis de vil slette mange koder på én gang. Kanskje vi bør vente på tilbakemeldinger fra brukerne før vi innfører dette? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## org-library-mvp #14726 +/- ##
================================================
Coverage 95.78% 95.79%
================================================
Files 1931 1931
Lines 25150 25157 +7
Branches 2857 2858 +1
================================================
+ Hits 24091 24098 +7
Misses 799 799
Partials 260 260 ☔ View full report in Codecov by Sentry. |
![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 (1)
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx (1)
32-32:⚠️ Potential issueImplement confirmation dialog for delete action.
The PR objectives mention adding a confirmation prompt when deleting a row, but the implementation is missing. Consider wrapping the
onDeleteCodeListcallback with a confirmation dialog.Here's a suggested implementation:
- onDeleteCodeList: (codeListId: string) => void; + onDeleteCodeList: (codeListId: string) => { + if (window.confirm('Are you sure you want to delete this code list?')) { + // Call the actual delete function + } + };
🧹 Nitpick comments (44)
frontend/testing/playwright/pages/OrgLibraryPage/CodeLists.ts (1)
4-26: Add test coverage for delete row confirmation dialog.The PR's main objective is to add a confirmation dialog when deleting a row in the codelist. However, there's no test coverage for this functionality. Please add methods to:
- Verify the presence of the confirmation dialog when deleting a row
- Test both confirmation and cancellation scenarios
Would you like me to help you implement these test methods? Here's a suggested structure:
public async deleteRow(rowIndex: number): Promise<void> { // Click delete button for the specified row await this.page.getByRole('button', { name: 'delete' }).nth(rowIndex).click(); // Verify confirmation dialog const dialog = this.page.getByRole('dialog'); await expect(dialog).toBeVisible(); // Verify dialog content const confirmText = this.page.getByText(this.textMock('confirm_delete_row')); await expect(confirmText).toBeVisible(); } public async confirmDelete(): Promise<void> { await this.page.getByRole('button', { name: this.textMock('confirm') }).click(); // Add verification that row was deleted } public async cancelDelete(): Promise<void> { await this.page.getByRole('button', { name: this.textMock('cancel') }).click(); // Add verification that row still exists }frontend/testing/playwright/tests/org-library/org-library.spec.ts (1)
38-47: Add test coverage for row deletion confirmation.The current test only verifies navigation and empty state. Based on the PR objective to "add confirm when deleting row in codelist", we need additional test coverage for:
- Creating a row in the codelist
- Triggering row deletion
- Verifying the confirmation dialog appears
- Confirming/canceling the deletion
Would you like me to help generate the additional test cases?
frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.test.ts (1)
29-58: Consider adding test cases for error handling and edge cases.While the current tests cover the happy path well, consider adding tests for:
- Error handling when the mutation fails
- Edge cases with empty or invalid code list data
- Validation of the title and data fields
Example test cases to add:
it('Handles mutation errors correctly', async () => { const error = new Error('Update failed'); const updateCodeListForOrg = jest.fn(() => Promise.reject(error)); const { result } = renderHookWithProviders(() => useUpdateOrgCodeListMutation(org), { queries: { updateCodeListForOrg }, }); await expect(result.current.mutateAsync(updatedCodeList)).rejects.toThrow(error); }); it('Validates input data', async () => { const { result } = renderHookWithProviders(() => useUpdateOrgCodeListMutation(org)); // @ts-expect-error Testing runtime validation await expect(result.current.mutateAsync({ title: '', data: [] })) .rejects.toThrow(); });frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (1)
41-48: Consider adding a default case in the switch statementIf
codeListResponseStatusever falls outside'pending' | 'error' | 'success'(for example,'idle'), the UI might break. Adding a default case protects against unexpected states and improves resilience.backend/src/Designer/Services/Implementation/Organisation/OrgCodeListService.cs (3)
37-37: Pass the cancellation token toGetCodeListIdsTo maintain consistency, consider updating
GetCodeListIdsto accept and leverage the existingcancellationToken—especially if fetching code list IDs can be canceled.
39-62: Consider parallelizing code list retrieval for performanceIf there are many code lists, the sequential
foreachloop could slow things down. Using parallel or batched retrieval (e.g.Task.WhenAll) might improve performance.
74-78: Review returning all code lists on createReturning the entire set of code lists after creating a new one may be excessive for some use cases. If not strictly needed, returning only the newly created code list could be more efficient.
backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (1)
165-172: Potential concurrency concern with file deletionConcurrent code list edits or deletions might lead to races or file locks. Investigate locking or concurrency strategies if your application allows simultaneous edits.
frontend/dashboard/hooks/useSubRoute/useSubroute.ts (1)
5-8: Add JSDoc documentation for the hook.Consider adding JSDoc documentation to describe the hook's purpose, parameters, and return value. This would improve maintainability and developer experience.
Example documentation:
+/** + * Custom hook to get the current subroute from URL parameters. + * Falls back to the default dashboard basename if no subroute is present. + * @returns {string} The current subroute + */ export function useSubroute(): string { const { subroute = defaultSubroute } = useParams(); return subroute; }frontend/dashboard/pages/OrgContentLibrary/utils.test.ts (1)
4-17: Consider adding more edge cases to the test suite.The current test coverage is good, but consider adding tests for edge cases such as:
- Empty string
- Undefined/null (with proper TypeScript handling)
- Special characters or numbers in the context type
Example additional tests:
it('Returns true for empty string', () => { expect(isOrg('')).toBe(true); }); it('Handles special characters correctly', () => { expect(isOrg('org@123')).toBe(true); });frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (1)
13-14: Enhance test reliability by using more appropriate assertions.The current test uses
toBefor object comparison, which might not reliably compare File objects. Consider using more specific assertions to verify the file properties.- const retrievedFile = formData.get('file'); - expect(retrievedFile).toBe(file); + const retrievedFile = formData.get('file') as File; + expect(retrievedFile.name).toBe(fileName); + expect(retrievedFile.type).toBe(fileType); + expect(retrievedFile.size).toBe(file.size);frontend/testing/playwright/components/DashboardHeader.ts (1)
8-10: Remove unnecessary constructor.The constructor only calls super with the same parameters and can be safely removed.
- constructor(page: Page, environment?: Environment) { - super(page, environment); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.test.ts (1)
7-12: Add test cases for error and loading states.The test suite should include additional test cases to verify the behavior of the hook in different states:
- Loading state
- Error state
- Empty response
Example test cases to add:
it('handles loading state correctly', () => { const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); expect(result.current.isLoading).toBe(true); }); it('handles error state correctly', async () => { queriesMock.getCodeListsForOrg.mockRejectedValueOnce(new Error('Test error')); const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error).toBeDefined(); }); it('handles empty response correctly', async () => { queriesMock.getCodeListsForOrg.mockResolvedValueOnce({ codeLists: [] }); const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); await waitFor(() => expect(result.current.isSuccess).toBe(true)); expect(result.current.data?.codeLists).toHaveLength(0); });frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.test.ts (1)
1-23: LGTM! Well-structured test implementation.The test properly verifies the query behavior with correct parameters and mocks.
Consider adding these test cases to improve coverage:
- Test with different language values to ensure proper handling of language parameter
- Add error case tests to verify error handling behavior
frontend/packages/shared/src/mocks/codeListsResponse.ts (1)
15-31: Consider adding JSDoc comments for mock data.The mock data is well-structured and includes various test scenarios. Consider adding JSDoc comments to explain the purpose of each mock code list for better maintainability.
+/** Mock code list with full data including description and help text */ const codeList1: CodeListData = { // ... }; +/** Mock code list with minimal data (only label and value) */ const codeList2: CodeListData = { // ... }; +/** Mock code list representing an error state */ const codeListWithError: CodeListData = { // ... };Also applies to: 33-40, 42-45
backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs (1)
31-40: Document precondition for UpdateTextsForKeys method.Based on retrieved learnings, this method requires the text resource to exist before it can be updated.
/// <summary> -/// Updates values for specified keys in the text resouce. +/// Updates values for specified keys in the text resource. +/// Precondition: The text resource must exist before calling this method. /// </summary>frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.test.ts (1)
31-54: Add error case test scenario.The tests cover successful scenarios well, but consider adding test cases for error handling:
- File upload failure
- Invalid file format
- Network errors
it('Handles upload failure correctly', async () => { const error = new Error('Upload failed'); const uploadCodeListForOrg = jest.fn(() => Promise.reject(error)); const { result } = renderHookWithProviders(() => useUploadOrgCodeListMutation(org), { queryClient, queries: { uploadCodeListForOrg }, }); await expect(result.current.mutateAsync(file)).rejects.toThrow(error); });frontend/packages/shared/src/mocks/textResourcesMock.ts (1)
3-82: Consider refactoring to reduce code duplication.The text resources follow a clear pattern and could be generated programmatically to improve maintainability.
Consider this approach:
-export const label1TextResource: ITextResource = { - id: 'label1', - value: 'Ledetekst 1', -}; -// ... (similar declarations) +const createTextResources = ( + prefix: string, + valuePrefix: string, + count: number +): ITextResource[] => + Array.from({ length: count }, (_, i) => ({ + id: `${prefix}${i + 1}`, + value: `${valuePrefix} ${i + 1}`, + })); + +export const labelTextResources = createTextResources('label', 'Ledetekst', 5); +export const descriptionTextResources = createTextResources('description', 'Beskrivelse', 5); +export const helpTextTextResources = createTextResources('helpText', 'Hjelpetekst', 5); + +const textResources: ITextResource[] = [ + ...labelTextResources, + ...descriptionTextResources, + ...helpTextTextResources, +];frontend/dashboard/pages/CreateService/CreateService.tsx (2)
40-42: LGTM! Consider destructuring hook returns.The routing changes improve maintainability. Consider destructuring the hook returns for better readability.
-const selectedContext = useSelectedContext(); -const subroute = useSubroute(); +const { selectedContext } = useSelectedContext(); +const { subroute } = useSubroute();
91-91: Consider using template literals consistently.For better readability and consistency with modern JavaScript practices.
-href: `/${subroute}/${selectedContext}`, +href: `${subroute}/${selectedContext}`,frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.ts (1)
89-89: Consider simplifying the regex pattern.The global flag (
g) in the regex pattern/^\//gis unnecessary since we're only matching at the start of the string. Consider one of these simpler alternatives:- static removeLeadingSlash = (str: string): string => str.replace(/^\//g, ''); + static removeLeadingSlash = (str: string): string => str.replace(/^\//, '');Or even more readable:
- static removeLeadingSlash = (str: string): string => str.replace(/^\//g, ''); + static removeLeadingSlash = (str: string): string => str.startsWith('/') ? str.slice(1) : str;frontend/dashboard/hooks/guards/useContextRedirectionGuard.ts (2)
104-107: Consider moving type definition to a separate file.The
DashboardRoutetype could be moved to a shared types file since it represents a core routing concept that might be used by other components.
86-86: Consider destructuring location.search for clarity.The
location.searchusage could be more explicit by destructuring it from thewindow.locationobject:-): void => navigate(subroute + '/' + selectedContextType + location.search, { replace: true }); +): void => { + const { search } = window.location; + navigate(`${subroute}/${selectedContextType}${search}`, { replace: true }); +};backend/src/Designer/Services/Interfaces/Organisation/IOrgCodeListService.cs (2)
60-60: Consider returning void for DeleteCodeList.The
DeleteCodeListmethod returnsList<OptionListData>, but a delete operation typically doesn't need to return the remaining items. Consider changing the return type toTaskorTask<bool>to indicate success/failure.
21-24: Update documentation for CreateCodeList and UpdateCodeList.The XML documentation for both methods is identical, stating "Creates a new code list". Please update the documentation for
UpdateCodeListto accurately reflect its purpose of updating an existing code list.Also applies to: 32-35
backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs (2)
66-66: Remove misleading comment.Based on retrieved learnings, the comment "handle if file not already exist" is incorrect as
UpdateTextsForKeyscan only be called for text resources that already exist.
77-86: Simplify variable handling logic.The null check for variables can be simplified using the null-coalescing operator.
- if (textResourceContainsKey.Variables == null) - { - textResourceObject.Resources[indexTextResourceElementUpdateKey] = new TextResourceElement { Id = kvp.Key, Value = kvp.Value }; - } - else - { - List<TextResourceVariable> variables = textResourceContainsKey.Variables; - textResourceObject.Resources[indexTextResourceElementUpdateKey] = new TextResourceElement { Id = kvp.Key, Value = kvp.Value, Variables = variables }; - } + textResourceObject.Resources[indexTextResourceElementUpdateKey] = new TextResourceElement + { + Id = kvp.Key, + Value = kvp.Value, + Variables = textResourceContainsKey.Variables + };frontend/dashboard/app/App.test.tsx (3)
28-31: Consider adding cleanup after each test.While
jest.clearAllMocks()is called before each test, it's good practice to also clean up after each test to ensure a clean state.beforeEach(() => { jest.clearAllMocks(); typedLocalStorage.setItem('featureFlags', [FeatureFlag.OrgLibrary]); }); +afterEach(() => { + jest.clearAllMocks(); + typedLocalStorage.clear(); +});
58-63: Add timeout to waitForElementToBeRemoved.The
waitForElementToBeRemovedfunction should have a timeout to prevent infinite waiting in case of test failures.-await waitForElementToBeRemoved(querySpinner()); +await waitForElementToBeRemoved(querySpinner(), { timeout: 5000 });
93-101: Consider adding error handling for render failures.The
renderAppfunction should handle potential render failures gracefully.function renderApp(args: RenderAppArgs = {}): RenderResult { + try { return render(<App />, { wrapper: ({ children }) => ( <MockServicesContextWrapper {...args}>{children}</MockServicesContextWrapper> ), }); + } catch (error) { + console.error('Failed to render App:', error); + throw error; + } }backend/src/Designer/Controllers/Organisation/OrgTextController.cs (2)
17-20: Add API version attribute.Consider adding API version attribute to support versioning of your API endpoints.
[ApiController] [Authorize] +[ApiVersion("1.0")] [Route("designer/api/{org}/text")] public class OrgTextController : ControllerBase
40-56: Add ProducesResponseType attributes.Document the possible response types for better API documentation and client understanding.
[HttpGet] [Route("language/{languageCode}")] +[ProducesResponseType(typeof(TextResource), StatusCodes.Status200OK)] +[ProducesResponseType(StatusCodes.Status404NotFound)] +[ProducesResponseType(StatusCodes.Status401Unauthorized)] public async Task<ActionResult<TextResource>> GetResource(string org, string languageCode, CancellationToken cancellationToken = default)frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (1)
34-39: Consider extracting feature flag check to a hook.The feature flag check could be extracted to a custom hook for better reusability and testing.
+function useOrgLibraryFeature() { + return shouldDisplayFeature(FeatureFlag.OrgLibrary); +} <StudioPageHeader.Center> - {shouldDisplayFeature(FeatureFlag.OrgLibrary) && + {useOrgLibraryFeature() && dashboardHeaderMenuItems.map((menuItem: HeaderMenuItem) => ( <TopNavigationMenuItem key={menuItem.name} menuItem={menuItem} /> ))} </StudioPageHeader.Center>backend/src/Designer/Infrastructure/ServiceRegistration.cs (1)
39-86: Consider grouping related services.The service registrations could be organized better by grouping related services together.
public static IServiceCollection RegisterServiceImplementations(this IServiceCollection services, IConfiguration configuration) { + // Repository and source control services services.AddTransient<IRepository, RepositorySI>(); services.AddTransient<ISchemaModelService, SchemaModelService>(); services.AddTransient<IAltinnGitRepositoryFactory, AltinnGitRepositoryFactory>(); services.AddTransient<ISourceControl, SourceControlSI>(); services.Decorate<ISourceControl, SourceControlLoggingDecorator>(); + // Database context services.AddSingleton(configuration); services.AddDbContext<DesignerdbContext>(options => { PostgreSQLSettings postgresSettings = configuration.GetSection(nameof(PostgreSQLSettings)).Get<PostgreSQLSettings>(); options.UseNpgsql(postgresSettings.FormattedConnectionString()); }); + // Repository services services.AddScoped<IReleaseRepository, ReleaseRepository>(); services.AddScoped<IDeploymentRepository, DeploymentRepository>(); services.AddScoped<IAppScopesRepository, AppScopesRepository>(); + // User and authentication services services.AddScoped<IUserService, UserService>(); services.AddTransient<IAccessTokenGenerator, AccessTokenGenerator>(); services.AddTransient<ISigningCredentialsResolver, SigningCredentialsResolver>(); + // Organization services services.AddTransient<IOrgCodeListService, OrgCodeListService>(); services.AddTransient<IOrgTextsService, OrgTextsService>(); services.AddHttpClient<IOrgService, OrgService>(); // ... rest of the registrationsbackend/src/Designer/Controllers/Organisation/OrgCodeListController.cs (2)
81-83: Update XML documentation for UpdateCodeList method.The XML documentation for
UpdateCodeListis identical toCreateCodeList. Consider updating it to better reflect the update operation.Apply this diff to improve the documentation:
- /// Creates or overwrites an code-list. + /// Updates an existing code-list.
111-125: Add file size and type validation in UploadCodeList method.The method should validate the file size and type before processing to prevent potential issues with large or invalid files.
Apply this diff to add validation:
public async Task<ActionResult<List<OptionListData>>> UploadCodeList(string org, [FromForm] IFormFile file, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); string developer = AuthenticationHelper.GetDeveloperUserName(HttpContext); + if (file.Length > 5 * 1024 * 1024) // 5MB limit + { + return BadRequest("File size exceeds the limit of 5MB"); + } + + if (!file.ContentType.Equals("application/json", StringComparison.OrdinalIgnoreCase)) + { + return BadRequest("Only JSON files are supported"); + } + try {backend/src/Designer/Services/Implementation/OptionsService.cs (1)
23-23: Consider moving OptionsFolderPath to configuration.While using a constant improves maintainability, consider moving it to a configuration file for better flexibility across different environments.
- Add to
appsettings.json:{ "RepositorySettings": { "OptionsFolderPath": "App/options/" } }
- Inject through configuration:
- private const string OptionsFolderPath = "App/options/"; + private readonly string _optionsFolderPath; + + public OptionsService(IAltinnGitRepositoryFactory altinnGitRepositoryFactory, IConfiguration configuration) + { + _altinnGitRepositoryFactory = altinnGitRepositoryFactory; + _optionsFolderPath = configuration.GetValue<string>("RepositorySettings:OptionsFolderPath") ?? "App/options/"; + }frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx (1)
258-258: Fix typo in variable name.The variable name
delteConfirmationTextcontains a typo. It should bedeleteConfirmationText.- const delteConfirmationText: string = texts.deleteItemConfirmation(number); + const deleteConfirmationText: string = texts.deleteItemConfirmation(number);frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.test.tsx (2)
19-26: Consider using a test constants file.These mock values could be moved to a separate test constants file to improve maintainability and reusability across test files.
-const updateCodeListButtonTextMock: string = 'Update Code List'; -const uploadCodeListButtonTextMock: string = 'Upload Code List'; -const deleteCodeListButtonTextMock: string = 'Delete Code List'; -const codeListNameMock: string = 'codeListNameMock'; -const codeListMock: CodeList = [{ value: '', label: '' }]; -const codeListsDataMock: CodeListData[] = [{ title: codeListNameMock, data: codeListMock }]; -const mockOrgPath: string = '/testOrg'; +import { + updateCodeListButtonTextMock, + uploadCodeListButtonTextMock, + deleteCodeListButtonTextMock, + codeListNameMock, + codeListMock, + codeListsDataMock, + mockOrgPath +} from '../../testing/constants';
159-175: Enhance error handling test coverage.The error handling test only covers the generic error case. Consider adding tests for specific error codes and edge cases.
+it('renders error toast when onUploadCodeList is rejected with specific error code', async () => { + const user = userEvent.setup(); + const uploadCodeListForOrg = jest + .fn() + .mockImplementation(() => Promise.reject({ response: { status: 413, data: 'File too large' } })); + renderOrgContentLibraryWithCodeLists({ + initialEntries: [mockOrgPath], + queries: { uploadCodeListForOrg }, + }); + await goToLibraryPage(user, 'code_lists'); + const uploadCodeListButton = screen.getByRole('button', { name: uploadCodeListButtonTextMock }); + await user.click(uploadCodeListButton); + const errorToastMessage = screen.getByText( + textMock('dashboard.org_library.code_list_upload_size_error'), + ); + expect(errorToastMessage).toBeInTheDocument(); +});frontend/dashboard/pages/CreateService/CreateService.test.tsx (1)
12-13: Consider adding more routing test cases.The test suite could benefit from additional test cases that verify different subroute combinations.
+it('should handle different subroute combinations', async () => { + const user = userEvent.setup(); + const testCases = [ + { selectedContext: SelectedContextType.Self, subroute: Subroute.AppDashboard }, + { selectedContext: SelectedContextType.All, subroute: Subroute.ContentLibrary }, + { selectedContext: 'ttd', subroute: Subroute.ResourceLibrary } + ]; + + for (const { selectedContext, subroute } of testCases) { + (useParams as jest.Mock).mockReturnValue({ selectedContext, subroute }); + renderWithMockServices(); + const cancelLink = screen.getByRole('link', { name: textMock('general.cancel') }); + expect(cancelLink.getAttribute('href')).toBe(`/${subroute}/${selectedContext}`); + cleanup(); + } +});Also applies to: 74-76
frontend/packages/shared/src/api/queries.ts (1)
183-185: Track the TODO implementation for text resources API.The
getTextResourcesForOrgfunction currently uses a mock. Ensure to implement the real API call as mentioned in the TODO comment.Would you like me to create an issue to track the implementation of the real API call for
getTextResourcesForOrg?backend/tests/Designer.Tests/Services/TextsServiceTest.cs (1)
167-180: Consider adding test cases for different options paths.The test suite could benefit from additional test cases that verify the behavior with different options folder paths.
+[Theory] +[InlineData("App/options")] +[InlineData("App/custom/options")] +[InlineData("Organization/options")] +public async Task UpdateRelatedFiles_Options_ShouldUpdateOptionsListWithDifferentPaths(string optionsPath) +{ + (var targetRepository, var altinnGitRepositoryFactory, var textsService) = await SetupRepository(); + + List<TextIdMutation> keyMutations = new() { new() { OldId = "label1", NewId = "label1new" } }; + await textsService.UpdateRelatedFiles(org, targetRepository, developer, keyMutations); + AltinnAppGitRepository altinnAppGitRepository = altinnGitRepositoryFactory.GetAltinnAppGitRepository(org, targetRepository, developer); + string raw = await altinnAppGitRepository.GetOptionsList("test-options", optionsPath); + JsonNode optionsList = JsonNode.Parse(raw); + + Assert.NotNull(optionsList); + Assert.Equal("label1new", (optionsList as JsonArray)[0]["label"].ToString()); + Assert.Equal("label2", (optionsList as JsonArray)[1]["label"].ToString()); +}frontend/dashboard/app/App.tsx (1)
50-51: Refine error specificity if both queries fail.
Currently, if both user and organizations are in error state, the code only checksuserStatus === 'error'. This is a sensible default. However, if a scenario arises where both might fail, consider showing or logging both failures.Also applies to: 54-55, 57-57, 60-61, 64-64, 66-69, 72-72
🛑 Comments failed to post (11)
frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (1)
74-74:
⚠️ Potential issueAdd a confirmation prompt before deletion
The PR objective (#14718) mentions adding a confirm dialog to prevent accidental code list deletions. Consider wrapping the
deleteCodeListcall in a confirmation step to align with the PR's scope.frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts (1)
1-7: 🛠️ Refactor suggestion
Refactor class to a simple function.
The class contains only static members, which is a code smell. Consider refactoring it to a simple function for better maintainability and adherence to functional programming principles.
Apply this diff to refactor the class into a function:
-export class FileUtils { - static convertToFormData = (file: File): FormData => { - const formData = new FormData(); - formData.append('file', file); - return formData; - }; -} +export const convertToFormData = (file: File): FormData => { + const formData = new FormData(); + formData.append('file', file); + return formData; +};📝 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.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/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.ts (1)
12-12: 🛠️ Refactor suggestion
Include dynamic parameters in the query key.
The query key should include the dynamic parameters (
organdlanguage) to ensure proper caching and invalidation.Apply this diff to update 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],frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.ts (1)
6-18: 🛠️ Refactor suggestion
⚠️ Potential issueAdd confirmation dialog before deletion.
According to the PR objectives, a confirmation dialog should be implemented when deleting a row in the codelist. However, the current implementation directly deletes without confirmation.
Do you want me to generate a solution that implements the confirmation dialog using a modal or dialog component?
Add error handling and loading state.
Consider handling error states and providing loading feedback to improve user experience.
Apply this diff to add error handling:
return useMutation({ mutationFn, onSuccess: (newData: CodeListsResponse) => { queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); }, + onError: (error) => { + console.error('Failed to delete code list:', error); + // TODO: Add error notification or toast + }, });📝 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.export const useDeleteOrgCodeListMutation = (org: string) => { const queryClient = useQueryClient(); const { deleteCodeListForOrg } = useServicesContext(); const mutationFn = (title: string) => deleteCodeListForOrg(org, title); return useMutation({ mutationFn, onSuccess: (newData: CodeListsResponse) => { queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); }, onError: (error) => { console.error('Failed to delete code list:', error); // TODO: Add error notification or toast }, }); };frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts (1)
42-56:
⚠️ Potential issueFix incorrect hook usage in cache update test.
The test case for cache updates incorrectly uses
useCreateOrgCodeListMutationinstead ofuseDeleteOrgCodeListMutation. This mismatch could lead to incorrect test coverage.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.it('Replaces cache with api response', async () => { const queryClient = createQueryClientMock(); queryClient.setQueryData([QueryKey.OrgCodeLists, org], [codeListToDelete, otherCodeList]); const deleteCodeListForOrg = jest.fn(() => Promise.resolve([otherCodeList])); const { result } = renderHookWithProviders(() => useDeleteOrgCodeListMutation(org), { queryClient, queries: { deleteCodeListForOrg }, }); await result.current.mutateAsync({ title: codeListToDelete.title }); const expectedUpdatedData: CodeListsResponse = [otherCodeList]; const updatedData = queryClient.getQueryData([QueryKey.OrgCodeLists, org]); expect(updatedData).toEqual(expectedUpdatedData); });backend/src/Designer/Controllers/Organisation/OrgTextController.cs (2)
95-116:
⚠️ Potential issueHandle NotFoundException consistently.
Based on the retrieved learning about UpdateTextsForKeys requiring existing resources, the NotFoundException should return NotFound instead of BadRequest.
catch (NotFoundException) { - return BadRequest($"The text resource, resource.{languageCode}.json, could not be updated."); + return NotFound($"The text resource, resource.{languageCode}.json, could not be found."); }📝 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.[HttpPut] [Route("language/{languageCode}")] public async Task<ActionResult<TextResource>> UpdateResource(string org, [FromBody] Dictionary<string, string> keysTexts, string languageCode, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); string developer = AuthenticationHelper.GetDeveloperUserName(HttpContext); try { await _orgTextsService.UpdateTextsForKeys(org, developer, keysTexts, languageCode, cancellationToken); TextResource textResource = await _orgTextsService.GetText(org, developer, languageCode, cancellationToken); return Ok(textResource); } catch (ArgumentException exception) { return BadRequest(exception.Message); } catch (NotFoundException) { return NotFound($"The text resource, resource.{languageCode}.json, could not be found."); } }
66-83: 🛠️ Refactor suggestion
Add input validation.
The CreateResource method should validate the input parameters before processing.
[HttpPost] [Route("language/{languageCode}")] public async Task<ActionResult<TextResource>> CreateResource([FromBody] TextResource jsonData, string languageCode, string org, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); + if (string.IsNullOrEmpty(languageCode)) + { + return BadRequest("Language code is required"); + } + if (string.IsNullOrEmpty(org)) + { + return BadRequest("Organization identifier is required"); + } + if (jsonData == null) + { + return BadRequest("Text resource data is required"); + } string developer = AuthenticationHelper.GetDeveloperUserName(HttpContext);📝 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.[HttpPost] [Route("language/{languageCode}")] public async Task<ActionResult<TextResource>> CreateResource([FromBody] TextResource jsonData, string languageCode, string org, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); if (string.IsNullOrEmpty(languageCode)) { return BadRequest("Language code is required"); } if (string.IsNullOrEmpty(org)) { return BadRequest("Organization identifier is required"); } if (jsonData == null) { return BadRequest("Text resource data is required"); } string developer = AuthenticationHelper.GetDeveloperUserName(HttpContext); try { await _orgTextsService.SaveText(org, developer, jsonData, languageCode, cancellationToken); TextResource textResource = await _orgTextsService.GetText(org, developer, languageCode, cancellationToken); return Ok(textResource); } catch (ArgumentException e) { return BadRequest(e.Message); } }frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (1)
77-80: 🛠️ Refactor suggestion
Add input validation to extractSecondLastRouterParam.
The utility function should handle edge cases for pathname.
function extractSecondLastRouterParam(pathname: string): string { + if (!pathname) { + return ''; + } const pathnameArray = pathname.split('/'); + if (pathnameArray.length < 2) { + return ''; + } return pathnameArray[pathnameArray.length - 2]; }📝 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.function extractSecondLastRouterParam(pathname: string): string { if (!pathname) { return ''; } const pathnameArray = pathname.split('/'); if (pathnameArray.length < 2) { return ''; } return pathnameArray[pathnameArray.length - 2]; }backend/src/Designer/Controllers/Organisation/OrgCodeListController.cs (1)
137-149: 🛠️ Refactor suggestion
Standardize response type in DeleteCodeList method.
The method returns either a list of
OptionListDataor a string message, which is inconsistent. Consider standardizing the response type to always return the updated list of code lists.Apply this diff to standardize the response:
- return Ok($"The code-list file {codeListId}.json has been deleted."); + return Ok(codeLists);Committable suggestion skipped: line range outside the PR's diff.
frontend/dashboard/app/App.tsx (2)
92-94: 🛠️ Refactor suggestion
Add a default return path to avoid undefined rendering.
SubrouteGuardlacks a default case; an unknownsubrouteWithLeadingSlashwould returnundefined, violating theReact.ReactElementreturn type. Provide a fallback.function SubrouteGuard(props: AppWithDataProps): React.ReactElement { const subroute = useSubroute(); const subrouteWithLeadingSlash = '/' + subroute; switch (subrouteWithLeadingSlash) { case APP_DASHBOARD_BASENAME: return <Dashboard {...props} />; case ORG_LIBRARY_BASENAME: return <OrgContentLibrary />; + default: + // e.g., route back to the main dashboard or show an error message + return <Dashboard {...props} />; } }Also applies to: 96-102
31-39: 🛠️ Refactor suggestion
Consider a fallback case in the switch statement.
The switch covers'pending','error', and'success', but ifmergeQueryStatusescan yield other statuses (e.g.,'idle'), the function may inadvertently returnundefined.switch (queryStatus) { case 'pending': return <PendingPage />; case 'error': return <ErrorMessage userStatus={userStatus} />; case 'success': return <AppWithData user={user} organizations={organizations} />; + default: + // Provide a fallback or safe default + return <PendingPage />; }📝 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.switch (queryStatus) { case 'pending': return <PendingPage />; case 'error': return <ErrorMessage userStatus={userStatus} />; case 'success': return <AppWithData user={user} organizations={organizations} />; default: // Provide a fallback or safe default return <PendingPage />; } }
Description
Skjermopptak.2025-02-19.kl.14.50.56.mov
Related Issue(s)
Verification
Summary by CodeRabbit