feat(dashboard): Improve validation for missing permissions when creating new app #14395
Conversation
github-actions
bot
added
area/dashboard
framitdavid
changed the title
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to the frontend application's user permission management system. The changes focus on implementing a new mechanism to check and control user permissions when creating applications within an organization. The implementation spans multiple files, introducing a new query hook, updating component logic, adding test cases, and including a new error message for Norwegian language support. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/dashboard/components/ServiceOwnerSelector/ServiceOwnerSelector.tsx (1)
43-43: Consider adding null check for onChange callback.While the implementation is correct, it would be more robust to add a null check before invoking the callback.
- onChange={(event) => onChange(event.target.value)} + onChange={(event) => onChange?.(event.target.value)}frontend/dashboard/components/ServiceOwnerSelector/ServiceOwnerSelector.test.tsx (1)
82-92: Consider adding edge cases to onChange tests.While the basic onChange functionality is well tested, consider adding tests for:
- Handling undefined onChange prop
- Multiple selection changes
- Selection of the same value
Example additional test:
it('should handle undefined onChange prop gracefully', async () => { const user = userEvent.setup(); renderServiceOwnerSelector({ onChange: undefined }); const select = screen.getByLabelText(textMock('general.service_owner')); await user.selectOptions(select, 'organizationUsername'); // Should not throw any errors });frontend/dashboard/hooks/queries/useUserOrgPermissionsQuery.ts (1)
10-20: Consider adding error handling for empty org parameter.The implementation looks good, but it might benefit from additional validation.
export const useUserOrgPermissionQuery = ( org: string, options?: { enabled: boolean }, ): UseQueryResult<UserOrgPermission> => { + if (!org?.trim()) { + throw new Error('Organization parameter is required'); + } const { getUserOrgPermissions } = useServicesContext(); return useQuery({ queryKey: [QueryKey.UserOrgPermissions, org], queryFn: () => getUserOrgPermissions(org), ...options, }); };frontend/dashboard/components/NewApplicationForm/NewApplicationForm.tsx (1)
103-108: Consider adding loading state handling.While the error handling is good, the UI might benefit from showing a loading state while permissions are being fetched.
const createRepoAccessError: string = - !userOrgPermission?.canCreateOrgRepo && !isFetching + isFetching + ? '' + : !userOrgPermission?.canCreateOrgRepo ? t('dashboard.missing_service_owner_rights_error_message') : ''; const hasCreateRepoAccessError: boolean = Boolean(createRepoAccessError); +const isSubmitDisabled: boolean = hasCreateRepoAccessError || isFetching;frontend/packages/shared/src/api/queries.ts (1)
141-141: Consider adding TypeScript return type.The new
getUserOrgPermissionsfunction would benefit from an explicit return type definition for better type safety and developer experience.-export const getUserOrgPermissions = (org: string) => get(userOrgPermissionsPath(org)); +export const getUserOrgPermissions = (org: string): Promise<{ hasPermission: boolean }> => get(userOrgPermissionsPath(org));frontend/packages/shared/src/mocks/queriesMock.ts (1)
145-145: Enhance mock data to match expected API response.The mock currently returns an empty object. Consider using more realistic mock data that matches the expected API response structure for better testing.
- getUserOrgPermissions: jest.fn().mockImplementation(() => Promise.resolve({})), + getUserOrgPermissions: jest.fn().mockImplementation(() => Promise.resolve({ hasPermission: true })),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/dashboard/components/NewApplicationForm/NewApplicationForm.test.tsx(5 hunks)frontend/dashboard/components/NewApplicationForm/NewApplicationForm.tsx(5 hunks)frontend/dashboard/components/ServiceOwnerSelector/ServiceOwnerSelector.test.tsx(3 hunks)frontend/dashboard/components/ServiceOwnerSelector/ServiceOwnerSelector.tsx(3 hunks)frontend/dashboard/hooks/queries/useUserOrgPermissionsQuery.ts(1 hunks)frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/api/paths.js(1 hunks)frontend/packages/shared/src/api/queries.ts(2 hunks)frontend/packages/shared/src/mocks/queriesMock.ts(1 hunks)frontend/packages/shared/src/types/QueryKey.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Testing
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
🔇 Additional comments (11)
frontend/dashboard/components/ServiceOwnerSelector/ServiceOwnerSelector.tsx (1)
13-13: LGTM! Well-typed optional callback.The onChange prop is properly typed as an optional callback, following TypeScript best practices.
frontend/dashboard/components/ServiceOwnerSelector/ServiceOwnerSelector.test.tsx (1)
Line range hint
9-24: LGTM! Improved type safety with explicit type annotation.The addition of the ServiceOwnerSelectorProps type annotation to defaultProps enhances type safety and makes the interface requirements explicit.
frontend/dashboard/hooks/queries/useUserOrgPermissionsQuery.ts (1)
6-8: LGTM! Well-defined type for user organization permissions.The
UserOrgPermissiontype is clear, focused, and properly encapsulates the permission check.frontend/packages/shared/src/types/QueryKey.ts (1)
53-53: LGTM! Well-placed enum value.The new
UserOrgPermissionsenum value is properly placed and follows the existing pattern.frontend/dashboard/components/NewApplicationForm/NewApplicationForm.tsx (2)
55-58: LGTM! Well-implemented permission query.The permission query is properly set up with the current organization and enabled flag.
116-116: LGTM! Proper UI feedback implementation.The error message display and button disabling are well implemented, providing clear feedback to users.
Also applies to: 118-118, 130-130
frontend/dashboard/components/NewApplicationForm/NewApplicationForm.test.tsx (3)
58-62: LGTM! Well-structured mock setup.The mock setup is clean and properly initialized with a default positive case.
110-138: LGTM! Comprehensive test coverage.The test cases thoroughly cover both permission scenarios with clear assertions.
141-152: LGTM! Well-implemented test helper.The
renderNewApplicationFormhelper function improves test maintainability and reduces duplication.frontend/packages/shared/src/api/paths.js (1)
20-22: LGTM! Clean implementation of the new endpoint path.The new
userOrgPermissionsPathfunction follows the established patterns and RESTful conventions. The section comment helps with code organization.frontend/language/src/nb.json (1)
192-192: LGTM! Clear and well-structured error message.The new error message is clear, follows naming conventions, and is appropriately placed in alphabetical order within the file.
framitdavid
added
the
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14395 +/- ##
=======================================
Coverage 95.69% 95.70%
=======================================
Files 1902 1903 +1
Lines 24714 24730 +16
Branches 2829 2831 +2
=======================================
+ Hits 23651 23667 +16
Misses 802 802
Partials 261 261 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/dashboard/components/MakeCopyModal/MakeCopyModal.test.tsx (1)
19-21: Consider adding a test scenario wherecanCreateOrgRepois false.
Currently, the mock always returnstrue. Testing negative or edge cases (e.g. user lacking permissions) ensures more robust coverage and confidence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/dashboard/components/MakeCopyModal/MakeCopyModal.test.tsx(1 hunks)frontend/language/src/nb.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/language/src/nb.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (2)
frontend/dashboard/components/MakeCopyModal/MakeCopyModal.test.tsx (2)
15-16: Import statement looks correct.
The import ofuseUserOrgPermissionQueryaligns with the usage in this test suite.
17-17: Mocking the hook is a good approach for test isolation.
Good practice. This allows testing behavior without relying on the real query implementation.
Description
This PR introduces an error message that informs users when they do not have the necessary permissions to create a repository in a org.
Spiller.inn.2025-01-09.224118.mp4
Related Issue(s)
Verification
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization
Bug Fixes