fix(resource-adm): Fix misleading "Sist endret" field in resources table #14621
Conversation
…table for resources never checked into gitea
📝 WalkthroughWalkthroughThe changes encompass modifications in both backend and frontend components. In the backend, default value assignments in the Gitea API wrapper have been removed. On the frontend, Norwegian language strings have been updated, resource table components and tests have been enhanced to handle special date values using a new constant, and query and utility functions have been adjusted by renaming and extending sorting functionality. Exports for the new constant have also been added. Changes
Suggested labels
Suggested reviewers
✨ 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
solution/studio/designer
mgunnerud
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14621 +/- ##
=======================================
Coverage 95.73% 95.74%
=======================================
Files 1908 1908
Lines 24878 24888 +10
Branches 2846 2849 +3
=======================================
+ Hits 23818 23828 +10
Misses 799 799
Partials 261 261 ☔ View full report in Codecov by Sentry. |
…r-resource-not-checked-in-to-gitea
mgunnerud
added
skip-releasenotes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…not-checked-in-to-gitea' of https://github.com/Altinn/altinn-studio into bug/14620-sist-endret-field-is-misleading-for-resource-not-checked-in-to-gitea
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/resourceadm/hooks/queries/useGetResourceListQuery.ts (1)
8-15: Update JSDoc comments to reflect expanded functionality.The JSDoc comments should be updated to mention that the query not only maps and sorts the list but also sets the last changed date for resources not checked into Gitea.
/** * Query to get the list of resources. It maps the date to correct display format - * and sorts the list before it is being returned. + * and sorts the list before it is being returned. For resources not checked into + * Gitea, it sets a special last changed date to prioritize them in the sorted list. * * @param org the organisation of the user * * @returns UseQueryResult with a list of resources of Resource */frontend/resourceadm/utils/mapperUtils/mapperUtils.ts (1)
6-12: Update JSDoc comments to reflect expanded functionality.The JSDoc comments should be updated to mention that the function also sets a special last changed date for resources not checked into Gitea.
/** - * Sorts a resource list by the date so the newest is at the top + * Sets a special last changed date for resources not checked into Gitea and + * sorts the resource list by date so the newest is at the top. * * @param resourceList the list to sort * * @returns the sorted list */frontend/resourceadm/utils/mapperUtils/mapperUtils.test.ts (2)
34-66: Add test case for mixed date scenarios.The current test case verifies sorting but could be more explicit about the sorting criteria. Consider adding assertions to verify that:
- Resources with
LOCAL_RESOURCE_CHANGED_TIMEappear first- Resources with actual dates are sorted in descending order
- Resources with null dates appear last
it('should sort the list by date', () => { const resource1Id = 'resource-1'; const resource2Id = 'resource-2'; const resource3Id = 'resource-3'; + const resource4Id = 'resource-4'; const loadedResourceList = [ { title: { nb: resource1Id, en: '', nn: '' }, createdBy: '', lastChanged: null, identifier: resource1Id, environments: ['tt02'], }, { title: { nb: resource2Id, en: '', nn: '' }, createdBy: '', lastChanged: null, identifier: resource2Id, environments: ['gitea'], }, { title: { nb: resource3Id, en: '', nn: '' }, createdBy: 'ulrik user', lastChanged: new Date('2023-08-29'), identifier: resource3Id, environments: ['gitea', 'tt02'], }, + { + title: { nb: resource4Id, en: '', nn: '' }, + createdBy: 'ulrik user', + lastChanged: new Date('2023-08-30'), + identifier: resource4Id, + environments: ['tt02'], + }, ]; const resultResourceList = setLastChangedAndSortResourceListByDate(loadedResourceList); + // Verify that Gitea resources with LOCAL_RESOURCE_CHANGED_TIME appear first expect(resultResourceList[0].identifier).toBe(resource2Id); + // Verify that resources with actual dates are sorted in descending order + expect(resultResourceList[1].identifier).toBe(resource4Id); expect(resultResourceList[2].identifier).toBe(resource3Id); + // Verify that resources with null dates appear last expect(resultResourceList[3].identifier).toBe(resource1Id); });
82-94: Add test case for environment order consistency.The test verifies environment sorting but doesn't explain the rationale behind the order. Consider adding a comment explaining the expected order and its significance.
it('should sort environments', () => { + // Environments should be sorted in order of deployment progression: + // 1. Production (prod) - live environment + // 2. Test (tt02) - primary testing environment + // 3. Acceptance Test (at22, at24) - secondary testing environments + // 4. Development (gitea) - local development environment const loadedResourceList = [ { title: { nb: 'resource-1', en: '', nn: '' },frontend/resourceadm/components/ResourceTable/ResourceTable.test.tsx (2)
111-116: Add more test cases for date display logic.The current test only verifies that future dates are not displayed. Consider adding test cases for:
- Null dates
- Invalid date strings
- Edge cases around the LOCAL_RESOURCE_CHANGED_TIME
it('displays last changed date blank if resource has last changed date in the future', () => { render(<ResourceTable {...defaultProps} list={[mockResourceListItem4]} />); const lastChangedCell = screen.queryByText('31.12.9999'); expect(lastChangedCell).not.toBeInTheDocument(); }); + + it('displays blank for null or invalid dates', () => { + const mockItems = [ + { ...mockResourceListItem1, lastChanged: null }, + { ...mockResourceListItem1, lastChanged: 'invalid-date' }, + ]; + render(<ResourceTable {...defaultProps} list={mockItems} />); + + const rows = screen.getAllByRole('row'); + expect(rows[1].textContent).not.toMatch(/\d{2}\.\d{2}\.\d{4}/); + expect(rows[2].textContent).not.toMatch(/\d{2}\.\d{2}\.\d{4}/); + }); + + it('displays blank for dates near LOCAL_RESOURCE_CHANGED_TIME', () => { + const almostFutureDate = new Date(LOCAL_RESOURCE_CHANGED_TIME); + almostFutureDate.setDate(almostFutureDate.getDate() - 1); + const mockItem = { ...mockResourceListItem1, lastChanged: almostFutureDate }; + render(<ResourceTable {...defaultProps} list={[mockItem]} />); + + const dateCell = screen.queryByText(/30\.12\.9999/); + expect(dateCell).toBeInTheDocument(); + });
34-41: Consider using a test data factory.The mock data setup is becoming complex with multiple test items. Consider extracting it to a test data factory for better maintainability.
+const createMockResourceListItem = ( + overrides: Partial<ResourceListItem> = {}, + index = 1 +): ResourceListItem => ({ + title: { nb: `tittel ${index}`, en: '', nn: '' }, + createdBy: 'John Doe', + lastChanged: new Date(`2023-08-${27 + index}`), + identifier: `resource-${index}`, + environments: ['gitea'], + ...overrides, +}); + -const resource4Title = 'tittel 4'; -const mockResourceListItem4: ResourceListItem = { - title: { nb: resource4Title, en: '', nn: '' }, - createdBy: '', - lastChanged: LOCAL_RESOURCE_CHANGED_TIME, - identifier: 'resource-4', - environments: ['gitea'], -}; +const mockResourceListItem4 = createMockResourceListItem({ + createdBy: '', + lastChanged: LOCAL_RESOURCE_CHANGED_TIME, + identifier: 'resource-4', +}, 4);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/src/Designer/Services/Implementation/GiteaAPIWrapper/GiteaAPIWrapper.cs(0 hunks)frontend/language/src/nb.json(2 hunks)frontend/resourceadm/components/ResourceTable/ResourceTable.test.tsx(3 hunks)frontend/resourceadm/components/ResourceTable/ResourceTable.tsx(2 hunks)frontend/resourceadm/hooks/queries/useGetResourceListQuery.ts(2 hunks)frontend/resourceadm/utils/mapperUtils/index.ts(1 hunks)frontend/resourceadm/utils/mapperUtils/mapperUtils.test.ts(2 hunks)frontend/resourceadm/utils/mapperUtils/mapperUtils.ts(2 hunks)frontend/resourceadm/utils/resourceListUtils/index.ts(1 hunks)frontend/resourceadm/utils/resourceListUtils/resourceListUtils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/Designer/Services/Implementation/GiteaAPIWrapper/GiteaAPIWrapper.cs
✅ Files skipped from review due to trivial changes (2)
- frontend/resourceadm/utils/resourceListUtils/resourceListUtils.ts
- frontend/language/src/nb.json
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (3)
frontend/resourceadm/utils/resourceListUtils/index.ts (1)
1-1: LGTM!The export statement correctly exposes the new
LOCAL_RESOURCE_CHANGED_TIMEconstant that will be used for handling resources not checked into Gitea.frontend/resourceadm/utils/mapperUtils/index.ts (1)
1-5: LGTM!The function name change from
sortResourceListByDatetosetLastChangedAndSortResourceListByDateaccurately reflects its expanded functionality.frontend/resourceadm/utils/mapperUtils/mapperUtils.ts (1)
13-29: LGTM!The implementation correctly:
- Sets a special last changed date for resources not checked into Gitea
- Maintains the existing sorting functionality
frontend/resourceadm/components/ResourceTable/ResourceTable.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/resourceadm/utils/mapperUtils/mapperUtils.test.ts (3)
2-6: Consider splitting the functionality for better separation of concerns.The function name
setLastChangedAndSortResourceListByDatesuggests it's handling two distinct responsibilities: setting the last changed date and sorting. Consider splitting these into separate functions for better maintainability and testing.Example refactor:
- import { setLastChangedAndSortResourceListByDate } from './mapperUtils'; + import { setLastChanged, sortResourceListByDate } from './mapperUtils';
34-107: Enhance test coverage with additional scenarios.The test suite is well-structured but could benefit from additional test cases:
- Empty resource list handling
- Resources with future dates
- Invalid date formats
- Edge cases for environment combinations
Example additional test:
it('should handle empty resource list', () => { const emptyList = []; const result = setLastChangedAndSortResourceListByDate(emptyList); expect(result).toEqual([]); }); it('should handle future dates', () => { const futureDate = new Date(Date.now() + 86400000); // tomorrow const resourceList = [{ title: { nb: 'resource-1', en: '', nn: '' }, createdBy: '', lastChanged: futureDate, identifier: 'resource-1', environments: ['gitea'], }]; const result = setLastChangedAndSortResourceListByDate(resourceList); expect(result[0].lastChanged).toEqual(futureDate); });Also consider:
- Making test descriptions more specific (e.g., "should sort resources by lastChanged in descending order with gitea resources first")
- Extracting test data setup into helper functions or constants
40-69: Extract test data setup into reusable constants.Consider extracting the test data setup into reusable constants to improve maintainability and reduce duplication.
Example refactor:
const TEST_RESOURCES = { NO_DATE: { title: { nb: 'resource-1', en: '', nn: '' }, createdBy: '', lastChanged: null, identifier: 'resource-1', environments: ['tt02'], }, GITEA_RESOURCE: { title: { nb: 'resource-2', en: '', nn: '' }, createdBy: '', lastChanged: null, identifier: 'resource-2', environments: ['gitea'], }, // ... more test resources }; // In test: const loadedResourceList = [ TEST_RESOURCES.NO_DATE, TEST_RESOURCES.GITEA_RESOURCE, // ... ];frontend/resourceadm/utils/mapperUtils/mapperUtils.ts (1)
14-30: LGTM with a minor suggestion!The implementation correctly handles setting special last changed date for resources not checked into Gitea while preserving the sorting logic. The function maintains immutability by using map to create new objects.
Consider extracting the condition into a helper function for better readability:
+const shouldSetSpecialLastChangedDate = (resource: ResourceListItem): boolean => + resource.lastChanged === null && resource.environments.includes('gitea'); + export const setLastChangedAndSortResourceListByDate = ( resourceList: ResourceListItem[], ): ResourceListItem[] => { const listWithSortedEnvs = resourceList.map((resource) => { return { ...resource, lastChanged: - resource.lastChanged === null && resource.environments.includes('gitea') + shouldSetSpecialLastChangedDate(resource) ? LOCAL_RESOURCE_CHANGED_TIME : resource.lastChanged, environments: resource.environments.sort((a, b) => EnvOrder.indexOf(a) - EnvOrder.indexOf(b)), }; }); return listWithSortedEnvs.sort((a, b) => { return new Date(b.lastChanged).getTime() - new Date(a.lastChanged).getTime(); }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/language/src/nb.json(1 hunks)frontend/resourceadm/components/ResourceTable/ResourceTable.test.tsx(3 hunks)frontend/resourceadm/components/ResourceTable/ResourceTable.tsx(2 hunks)frontend/resourceadm/hooks/queries/useGetResourceListQuery.ts(2 hunks)frontend/resourceadm/utils/mapperUtils/mapperUtils.test.ts(2 hunks)frontend/resourceadm/utils/mapperUtils/mapperUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/resourceadm/components/ResourceTable/ResourceTable.tsx
- frontend/resourceadm/hooks/queries/useGetResourceListQuery.ts
- frontend/language/src/nb.json
🔇 Additional comments (6)
frontend/resourceadm/utils/mapperUtils/mapperUtils.ts (2)
3-3: LGTM!The import statement is correctly defined and follows the project's conventions.
6-13: LGTM!The documentation accurately describes the new functionality and follows JSDoc conventions.
frontend/resourceadm/components/ResourceTable/ResourceTable.test.tsx (4)
8-8: LGTM!The import statement is correctly defined and follows the project's conventions.
29-41: LGTM!The mock data changes provide good test coverage for resources with empty creator and special last changed date.
46-46: LGTM!The mock list is correctly updated to include the new test resource.
111-116: LGTM!The test case properly verifies that resources with future dates have blank last changed field, aligning with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/resourceadm/utils/mapperUtils/mapperUtils.ts (2)
7-11: Add JSDoc documentation for the utility function.The function logic is clear and aligns well with the PR objectives. However, it would benefit from JSDoc documentation explaining its purpose and parameters.
Add documentation like this:
+/** + * Determines the last changed date for a resource. + * Returns a special date for resources not yet checked into Gitea, + * otherwise returns the actual last changed date. + * + * @param resource - The resource item to process + * @returns The determined last changed date + */ const setLastChangedDate = (resource: ResourceListItem): Date => {
13-34: Consider splitting the function to follow Single Responsibility Principle.The implementation correctly handles the requirements and is well documented. However, the function now has two responsibilities: setting dates and sorting.
Consider splitting it into two functions for better maintainability:
-export const setLastChangedAndSortResourceListByDate = ( +const enrichResourcesWithDates = ( + resourceList: ResourceListItem[], +): ResourceListItem[] => { + return resourceList.map((resource) => ({ + ...resource, + lastChanged: setLastChangedDate(resource), + environments: resource.environments.sort( + (a, b) => EnvOrder.indexOf(a) - EnvOrder.indexOf(b) + ), + })); +}; + +export const setLastChangedAndSortResourceListByDate = ( resourceList: ResourceListItem[], ): ResourceListItem[] => { - const listWithSortedEnvs = resourceList.map((resource) => { - return { - ...resource, - lastChanged: setLastChangedDate(resource), - environments: resource.environments.sort((a, b) => EnvOrder.indexOf(a) - EnvOrder.indexOf(b)), - }; - }); - return listWithSortedEnvs.sort((a, b) => { + const enrichedList = enrichResourcesWithDates(resourceList); + return enrichedList.sort((a, b) => { return new Date(b.lastChanged).getTime() - new Date(a.lastChanged).getTime(); }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/resourceadm/utils/mapperUtils/mapperUtils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: Run integration tests against actual gitea and db
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
🔇 Additional comments (1)
frontend/resourceadm/utils/mapperUtils/mapperUtils.ts (1)
3-3: LGTM!The import of LOCAL_RESOURCE_CHANGED_TIME is correctly added and properly used in the new functionality.
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Style / UI Updates
Bug Fixes
New Features
Refactor