feat: complete user permissions management system#24
Conversation
- Convert getCommitteeNames function to committee-names.pipe for better performance - Add permissions matrix component explaining different permission levels and scopes - Implement edit user functionality with proper form initialization and validation - Add comprehensive remove user functionality that preserves user records - Fix backend API to handle permission updates with proper 404 handling - Move committee names pipe to shared pipes folder for reusability - Update permission service methods to match backend API contracts - Ensure proper error handling for all CRUD operations on user permissions Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis update introduces a comprehensive refactor and expansion of the user permissions system in the project. It replaces the previous role-based model with explicit project and committee-level permissions, adds new Angular components for permission management, updates backend services and API routes to support granular permission operations, and enhances UI components for improved user experience and configurability. Changes
Sequence Diagram(s)User Permission Management FlowsequenceDiagram
participant User
participant SettingsDashboardComponent
participant UserPermissionsTableComponent
participant UserFormComponent
participant PermissionsService
participant UserService
participant BackendAPI
participant SupabaseService
User->>SettingsDashboardComponent: Click "Add User"
SettingsDashboardComponent->>UserFormComponent: Open dialog
UserFormComponent->>UserService: createUserWithPermissions(request)
UserService->>BackendAPI: POST /api/projects/:projectId/permissions
BackendAPI->>SupabaseService: createUserWithPermissions(request)
SupabaseService-->>BackendAPI: User created and permissions assigned
BackendAPI-->>UserService: Success response
UserService-->>UserFormComponent: User created
UserFormComponent-->>SettingsDashboardComponent: Close dialog, trigger refresh
SettingsDashboardComponent->>PermissionsService: getProjectPermissions(projectId)
PermissionsService->>BackendAPI: GET /api/projects/:projectId/permissions
BackendAPI->>SupabaseService: getProjectPermissions(projectId)
SupabaseService-->>BackendAPI: UserPermissionSummary[]
BackendAPI-->>PermissionsService: UserPermissionSummary[]
PermissionsService-->>SettingsDashboardComponent: UserPermissionSummary[]
SettingsDashboardComponent-->>UserPermissionsTableComponent: Update users input
User Permission Removal FlowsequenceDiagram
participant User
participant UserPermissionsTableComponent
participant PermissionsService
participant BackendAPI
participant SupabaseService
User->>UserPermissionsTableComponent: Select "Remove" for user
UserPermissionsTableComponent->>User: Show confirmation dialog
User->>UserPermissionsTableComponent: Confirm removal
UserPermissionsTableComponent->>PermissionsService: removeUserFromProject(projectId, userId)
PermissionsService->>BackendAPI: DELETE /api/projects/:projectId/permissions/:userId
BackendAPI->>SupabaseService: removeUserFromProject(userId, projectId)
SupabaseService-->>BackendAPI: Success (204 No Content)
BackendAPI-->>PermissionsService: Success
PermissionsService-->>UserPermissionsTableComponent: Success
UserPermissionsTableComponent-->>SettingsDashboardComponent: Emit refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
requested review from
andrest50,
audigregorie and
mauriciozanettisalomao
Signed-off-by: Asitha de Silva <asithade@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
apps/lfx-pcc/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html (1)
114-136: Eliminate duplicate empty state renderingThe empty state message is duplicated in both the
emptymessagetemplate and the else block. Consider extracting this into a reusable component or template reference.Create a template reference to avoid duplication:
+<ng-template #emptyStateTemplate> + <div class="text-center py-8 text-gray-500"> + <i class="fa-light fa-users text-4xl text-gray-400 mb-4"></i> + <h3 class="text-lg font-medium text-gray-900 mb-2">No User Permissions</h3> + <p class="text-gray-600">No users have been assigned permissions to this project yet.</p> + </div> +</ng-template> <ng-template #emptymessage> <tr> <td colspan="6" class="text-center py-8"> - <div class="text-center py-8 text-gray-500"> - <i class="fa-light fa-users text-4xl text-gray-400 mb-4"></i> - <h3 class="text-lg font-medium text-gray-900 mb-2">No User Permissions</h3> - <p class="text-gray-600">No users have been assigned permissions to this project yet.</p> - </div> + <ng-container *ngTemplateOutlet="emptyStateTemplate"></ng-container> </td> </tr> </ng-template> } @else { <div class="flex justify-center items-center h-full"> - <div class="text-center py-8"> - <i class="fa-light fa-users text-4xl text-gray-400 mb-4"></i> - <h3 class="text-lg font-medium text-gray-900 mb-2">No User Permissions</h3> - <p class="text-gray-600">No users have been assigned permissions to this project yet.</p> - </div> + <ng-container *ngTemplateOutlet="emptyStateTemplate"></ng-container> </div> }
🧹 Nitpick comments (8)
apps/lfx-pcc/src/app/app.component.scss (1)
33-38: Selector duplication – consider flattening to keep CSS concise
p-select-overlayandp-multiselect-overlayshare identical nested rules. You could merge them to reduce specificity and maintenance overhead without changing behaviour.-.p-select-overlay, -.p-multiselect-overlay { - .p-select-list-container, - .p-multiselect-list-container { - .p-select-option, - .p-multiselect-option { - @apply text-sm; - } - } -} +.p-select-overlay .p-select-list-container .p-select-option, +.p-multiselect-overlay .p-multiselect-list-container .p-multiselect-option { + @apply text-sm; +}apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.html (1)
5-16: Repeated function invocations in high-frequency template
users(),loading()andrefreshUsers()are signal/function calls executed on each CD pass. Cache their values into readonly signals or variables and bind to those, or convert to async pipes, to avoid performance degradation when the dashboard displays large tables.apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html (1)
14-33: LGTM! Well-structured permission matrix display with minor styling suggestion.The template effectively displays the permission matrix using modern Angular control flow. The structure is semantic and user-friendly.
Consider these minor improvements for better maintainability:
- <span class="inline-flex items-center px-2 py-1 rounded-full text-xs font-medium" [class]="item.badge.bgColor + ' ' + item.badge.color"> + <span class="inline-flex items-center px-2 py-1 rounded-full text-xs font-medium" [ngClass]="[item.badge.bgColor, item.badge.color]"> - <i class="fa-light fa-check text-green-500 mr-1 mt-0.5" style="font-size: 10px"></i> + <i class="fa-light fa-check text-green-500 mr-1 mt-0.5 text-[10px]"></i>This uses
[ngClass]for cleaner class binding and Tailwind utilities instead of inline styles.apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts (1)
67-133: Consider using a logging service instead of console.errorThe form submission logic is well-implemented, but consider replacing
console.errorwith a proper logging service for production environments.Replace console.error with a logging service:
error: (error) => { - console.error('Error saving user:', error); + // Consider injecting and using a logging service + // this.loggingService.error('Error saving user:', error); this.messageService.add({ severity: 'error', summary: 'Error', detail: `Failed to ${this.isEditing() ? 'update' : 'create'} user. Please try again.`, }); this.submitting.set(false); },apps/lfx-pcc/src/server/routes/permissions.ts (4)
33-34: Use consistent property access notationFor consistency with line 18, use dot notation instead of bracket notation when accessing
req.params.projectId.- req.log.error({ error, projectId: req.params['projectId'] }, 'Error fetching project permissions'); + req.log.error({ error, projectId: req.params.projectId }, 'Error fetching project permissions');
78-79: Use consistent property access notation- req.log.error({ error, projectId: req.params['projectId'] }, 'Error creating user with permissions'); + req.log.error({ error, projectId: req.params.projectId }, 'Error creating user with permissions');
127-128: Use consistent property access notation- req.log.error({ error, projectId: req.params['projectId'], userId: req.params['userId'] }, 'Error updating user permissions'); + req.log.error({ error, projectId: req.params.projectId, userId: req.params.userId }, 'Error updating user permissions');
153-154: Use consistent property access notation- req.log.error({ error, projectId: req.params['projectId'], userId: req.params['userId'] }, 'Error removing user permissions'); + req.log.error({ error, projectId: req.params.projectId, userId: req.params.userId }, 'Error removing user permissions');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/lfx-pcc/src/app/app.component.scss(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.ts(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html(4 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts(1 hunks)apps/lfx-pcc/src/app/shared/components/button/button.component.html(1 hunks)apps/lfx-pcc/src/app/shared/components/multi-select/multi-select.component.html(1 hunks)apps/lfx-pcc/src/app/shared/components/multi-select/multi-select.component.ts(1 hunks)apps/lfx-pcc/src/app/shared/pipes/committee-names.pipe.ts(1 hunks)apps/lfx-pcc/src/app/shared/services/permissions.service.ts(2 hunks)apps/lfx-pcc/src/app/shared/services/user.service.ts(1 hunks)apps/lfx-pcc/src/server/routes/permissions.ts(2 hunks)apps/lfx-pcc/src/server/services/supabase.service.ts(4 hunks)apps/lfx-pcc/src/styles.scss(1 hunks)packages/shared/src/interfaces/permissions.interface.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/lfx-pcc/src/app/shared/pipes/committee-names.pipe.ts (1)
packages/shared/src/interfaces/committee.interface.ts (1)
Committee(4-27)
apps/lfx-pcc/src/app/shared/services/user.service.ts (3)
apps/lfx-pcc/src/app/shared/services/permissions.service.ts (1)
Injectable(9-34)packages/shared/src/interfaces/auth.ts (1)
User(4-21)packages/shared/src/interfaces/permissions.interface.ts (1)
CreateUserPermissionRequest(50-59)
apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.ts (3)
apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
Component(20-77)apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts (1)
Component(20-204)packages/shared/src/interfaces/permissions.interface.ts (1)
PermissionMatrixItem(69-78)
apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (3)
apps/lfx-pcc/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
Component(22-166)apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts (1)
Component(20-204)packages/shared/src/interfaces/permissions.interface.ts (1)
UserPermissionSummary(37-48)
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts (4)
apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
Component(20-77)apps/lfx-pcc/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
Component(22-166)apps/lfx-pcc/src/app/shared/components/multi-select/multi-select.component.ts (1)
Component(9-29)packages/shared/src/interfaces/permissions.interface.ts (3)
PermissionScope(8-8)PermissionLevel(7-7)CreateUserPermissionRequest(50-59)
apps/lfx-pcc/src/server/routes/permissions.ts (1)
packages/shared/src/interfaces/permissions.interface.ts (2)
CreateUserPermissionRequest(50-59)UpdateUserPermissionRequest(61-67)
apps/lfx-pcc/src/app/shared/services/permissions.service.ts (1)
packages/shared/src/interfaces/permissions.interface.ts (2)
UserPermissionSummary(37-48)UpdateUserPermissionRequest(61-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Quality Checks
🔇 Additional comments (23)
apps/lfx-pcc/src/styles.scss (1)
42-43: Verify variables are actually consumed
--p-multiselect-sm-font-sizeand--p-multiselect-md-font-sizeare declared but I don’t see corresponding usages in the changed SCSS. Make sure PrimeNG is configured to read these variables; otherwise they become dead code.apps/lfx-pcc/src/app/shared/services/user.service.ts (1)
4-7: LGTM! Clean dependency injection and imports.The HttpClient injection using Angular's
inject()function and the import ofCreateUserPermissionRequestinterface are properly implemented and align with the new permissions system.apps/lfx-pcc/src/app/shared/components/multi-select/multi-select.component.ts (1)
27-28: LGTM! Good enhancement for component configurability.The addition of
sizeandstyleClassinput properties enhances the component's flexibility. The union type constraint forsizeand sensible default values are well-implemented.apps/lfx-pcc/src/app/shared/pipes/committee-names.pipe.ts (2)
5-5: LGTM! Appropriate import for the new data structure.The import of the
Committeeinterface aligns with the updated permission model where committee information is now nested within permission objects.
12-17: LGTM! Well-implemented refactor to handle new permission structure.The changes correctly adapt the pipe to work with the new permission data structure:
- The guard clause provides good defensive programming
- The parameter type accurately reflects the new nested committee structure
- The mapping logic correctly extracts committee names from
cp.committee.nameThis aligns well with the refactored permission interfaces mentioned in the AI summary.
apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.ts (2)
4-13: LGTM! Clean component structure and imports.The component setup follows Angular best practices with proper standalone configuration and appropriate imports for
CardComponentandPermissionMatrixIteminterface.
15-66: PermissionMatrix display labels are independent ofPermissionLevelenumThe
permissionMatrixinPermissionsMatrixComponentuses user-facing labels (“View”/“Manage”) purely for display. Itslevelfield is typed as a plain string, so it isn’t bound to the'read' | 'write'values of thePermissionLeveltype used elsewhere (e.g., in the user-form). No change is needed unless you intend to drive this matrix dynamically from the enum values themselves.Likely an incorrect or invalid review comment.
apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html (1)
3-11: LGTM! Clear informational structure.The card header and key information section effectively orient users about the permission system differences between project and committee scopes.
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.html (3)
1-59: Well-structured form with proper validationThe basic information section is well-implemented with:
- Clear field labeling and required field indicators
- Proper reactive form bindings
- Appropriate validation feedback using Angular's new control flow syntax
61-94: Permission settings properly implementedGood use of:
- Dynamic radio button generation with track functions for performance
- Clear labeling for permission scope and level options
- Proper form control bindings
95-131: Excellent conditional rendering and user feedbackThe committee selection and permission summary section effectively:
- Shows committee selection only when relevant
- Provides clear permission summary for user understanding
- Uses
appendTo="body"for the multi-select to avoid potential z-index issuesapps/lfx-pcc/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html (2)
1-37: Clean table structure with loading stateThe table setup is well-implemented with:
- Clear loading indicator
- Proper pagination configuration
- Well-organized column headers
38-112: Comprehensive permission display with good UXExcellent implementation of:
- Visual distinction using colored badges for permission scopes
- Clear access level indicators (Manage/View)
- Contextual details based on permission type
- Individual loading states for user actions
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts (1)
1-45: Well-structured component with modern Angular patternsExcellent use of:
- Standalone component architecture
- Signals for reactive state management
- Computed properties for derived values
- Proper dependency injection
apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
57-76: Clean dialog management implementationThe dialog handling is well-implemented with:
- Proper configuration for modal behavior
- Appropriate use of RxJS operators
- Clear separation of concerns
apps/lfx-pcc/src/app/shared/services/permissions.service.ts (1)
15-34: Service implementation follows Angular best practicesThe permissions service is well-structured with:
- Proper use of HttpClient and Observables
- RESTful API endpoint conventions
- Clear method signatures with appropriate types
apps/lfx-pcc/src/server/services/supabase.service.ts (5)
809-845: LGTM!The implementation correctly removes both project and committee permissions with appropriate error handling for 404 responses.
848-913: LGTM!Well-implemented user creation with proper email existence check, URL encoding, and permission assignment based on scope.
915-938: LGTM!Clean implementation of permission updates with proper removal of existing permissions before adding new ones.
982-994: LGTM!Clean helper method for project permission creation.
996-1008: LGTM!Clean helper method for committee permission creation.
packages/shared/src/interfaces/permissions.interface.ts (1)
7-78: LGTM!Well-structured interfaces with clear type definitions and appropriate optional fields. The permission model is comprehensive and consistent.
apps/lfx-pcc/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
28-166: LGTM!Well-implemented Angular component with:
- Proper use of signals for state management
- Memory leak prevention with
take(1)operator- Comprehensive error handling with user-friendly messages
- Clean separation of concerns using input/output patterns
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.html
Show resolved
Hide resolved
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts
Show resolved
Hide resolved
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts
Show resolved
Hide resolved
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts
Show resolved
Hide resolved
.../lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts
Show resolved
Hide resolved
❌ E2E Tests FailedBrowser: chromium Some E2E tests failed. Check the test report for details. Test Configuration
|
Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts (3)
125-125: Replace console.error with proper logging serviceConsole.error should be replaced with proper logging using a logging service for consistency with the rest of the application.
141-151: Add dynamic validation for committee_idsWhen permission scope is 'committee', the committee_ids field should be required. The current implementation lacks this dynamic validation that was previously suggested.
153-168: Add null safety for project IDThe project ID is cast to string without checking if the project exists, which could cause runtime errors. This issue was previously identified but not addressed.
🧹 Nitpick comments (2)
apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
33-33: Improve BehaviorSubject initializationThe
BehaviorSubjectis initialized withundefinedwhich is unconventional. Consider usingvoid 0for better practice.- public refresh: BehaviorSubject<void> = new BehaviorSubject<void>(undefined); + public refresh: BehaviorSubject<void> = new BehaviorSubject<void>(void 0);apps/lfx-pcc/src/server/services/supabase.service.ts (1)
472-494: Consider optimizing the permission aggregation logicThe current implementation correctly handles merging project and committee permissions. However, the logic could be more concise by always initializing the user entry if not present.
projectPermissions.forEach((perm: { user_id: string; users: User; project_id: string; permission_level: PermissionLevel }) => { userPermissionsMap.set(perm.user_id, { user: perm.users, projectPermission: { level: perm.permission_level, scope: 'project' }, committeePermissions: [], }); }); committeePermissions.forEach((perm: { user_id: string; users: User; committee_id: string; permission_level: PermissionLevel; committees: Committee }) => { - const user = userPermissionsMap.get(perm.user_id); - if (user) { - userPermissionsMap.set(perm.user_id, { - user: perm.users, - projectPermission: user.projectPermission, - committeePermissions: [...user.committeePermissions, { committee: perm.committees, level: perm.permission_level, scope: 'committee' }], - }); - } else { - userPermissionsMap.set(perm.user_id, { - user: perm.users, - committeePermissions: [{ committee: perm.committees, level: perm.permission_level, scope: 'committee' }], - }); - } + const existingUser = userPermissionsMap.get(perm.user_id); + const committeePermission = { committee: perm.committees, level: perm.permission_level, scope: 'committee' as const }; + + userPermissionsMap.set(perm.user_id, { + user: perm.users, + projectPermission: existingUser?.projectPermission, + committeePermissions: existingUser ? [...existingUser.committeePermissions, committeePermission] : [committeePermission], + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.ts(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts(1 hunks)apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts(1 hunks)apps/lfx-pcc/src/app/shared/services/permissions.service.ts(2 hunks)apps/lfx-pcc/src/app/shared/services/user.service.ts(1 hunks)apps/lfx-pcc/src/server/services/supabase.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.html
- apps/lfx-pcc/src/app/shared/services/user.service.ts
- apps/lfx-pcc/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/lfx-pcc/src/app/shared/services/permissions.service.ts (1)
packages/shared/src/interfaces/permissions.interface.ts (2)
UserPermissionSummary(37-48)UpdateUserPermissionRequest(61-67)
apps/lfx-pcc/src/server/services/supabase.service.ts (3)
packages/shared/src/interfaces/permissions.interface.ts (6)
UserPermissionSummary(37-48)PermissionLevel(7-7)CreateUserPermissionRequest(50-59)UpdateUserPermissionRequest(61-67)ProjectPermission(10-17)CommitteePermission(19-27)packages/shared/src/interfaces/auth.ts (1)
User(4-21)packages/shared/src/interfaces/committee.interface.ts (1)
Committee(4-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Quality Checks
🔇 Additional comments (3)
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts (1)
185-185: Good: Proper typing used for committee permissionsThe code correctly uses typed access to
committeePermissionswithout type assertions, addressing the previous concern about type safety.apps/lfx-pcc/src/app/shared/services/permissions.service.ts (1)
1-29: Good: Service properly refactoredThe service has been properly refactored with the removal of the unused
addUserPermissionsmethod that was previously flagged. The remaining methods use appropriate types and follow a clean API design.apps/lfx-pcc/src/server/services/supabase.service.ts (1)
799-835: Good: Proper error handling for permission removalThe implementation correctly handles 404 responses when removing permissions, which is appropriate since the permissions might not exist. This defensive approach prevents unnecessary errors.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a complete user permissions management system with CRUD operations, replacing the previous role-based system with a simplified permission structure based on scope (project vs committee) and access level (read vs write).
- Replaces complex role-based permissions with a simplified scope/level system
- Adds comprehensive user management functionality including create, edit, and delete operations
- Implements a permission matrix component to document different permission levels
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/interfaces/permissions.interface.ts |
Refactors permission interfaces from role-based to scope/level system |
apps/lfx-pcc/src/server/services/supabase.service.ts |
Updates backend service to support new permission system with CRUD operations |
apps/lfx-pcc/src/server/routes/permissions.ts |
Adds REST API endpoints for user permission management |
apps/lfx-pcc/src/app/shared/services/*.ts |
Updates frontend services to work with new permission system |
apps/lfx-pcc/src/app/modules/project/settings/components/* |
Implements user form, permissions table, and matrix documentation components |
apps/lfx-pcc/src/app/modules/project/settings/settings-dashboard/* |
Updates dashboard to integrate new user management functionality |
| Various styling and component files | Adds styling support and enhances existing components for the new UI |
apps/lfx-pcc/src/app/modules/project/settings/components/user-form/user-form.component.ts
Show resolved
Hide resolved
...dules/project/settings/components/user-permissions-table/user-permissions-table.component.ts
Show resolved
Hide resolved
Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.ts(5 hunks)apps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts(4 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.html(2 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.ts(5 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.html(0 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.ts(4 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/member-form/member-form.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-form/meeting-form.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/participant-form/participant-form.component.html(0 hunks)apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts(6 hunks)
💤 Files with no reviewable changes (2)
- apps/lfx-pcc/src/app/modules/project/meetings/components/participant-form/participant-form.component.html
- apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.html
✅ Files skipped from review due to trivial changes (1)
- apps/lfx-pcc/src/app/modules/project/committees/components/member-form/member-form.component.html
🧰 Additional context used
📓 Path-based instructions (4)
apps/**/src/**/*.html
📄 CodeRabbit Inference Engine (CLAUDE.md)
apps/**/src/**/*.html: Always add data-testid attributes when creating new components for reliable test targeting
Use data-testid naming convention - '[section]-[component]-[element]' for hierarchical structure
Files:
apps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.htmlapps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-form/meeting-form.component.html
{apps,packages}/**/*.{ts,tsx,js,jsx,css,scss}
📄 CodeRabbit Inference Engine (CLAUDE.md)
License headers are required on all source files
Files:
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use TypeScript interfaces instead of union types for better maintainability
Files:
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
🧠 Learnings (4)
📚 Learning: 2025-08-06T00:40:54.228Z
Learnt from: asithade
PR: linuxfoundation/lfx-v2-pcc-ui#24
File: apps/lfx-pcc/src/app/shared/components/multi-select/multi-select.component.html:16-18
Timestamp: 2025-08-06T00:40:54.228Z
Learning: In Angular applications, when reviewing template code that uses parentheses syntax like `size()` or `styleClass()`, these may be Angular signals rather than regular methods. Angular signals are memoized and only re-compute when their dependencies change, unlike regular methods which execute on every change detection cycle. Therefore, the performance concerns about avoiding method calls in templates don't apply to signals.
Applied to files:
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
📚 Learning: 2025-08-07T16:35:57.349Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-07T16:35:57.349Z
Learning: All PrimeNG components are wrapped in LFX components for UI library independence
Applied to files:
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
📚 Learning: 2025-08-07T16:35:57.349Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-07T16:35:57.349Z
Learning: Always reference PrimeNG's component interface when trying to define types
Applied to files:
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
📚 Learning: 2025-08-07T16:35:57.348Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-07T16:35:57.348Z
Learning: Applies to packages/shared/src/{interfaces,constants,enums}/**/*.{ts,tsx} : All shared types, interfaces, and constants are centralized in lfx-pcc/shared package
Applied to files:
apps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
🧬 Code Graph Analysis (2)
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.ts (1)
packages/shared/src/interfaces/member.interface.ts (1)
CommitteeMember(4-22)
apps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts (2)
packages/shared/src/interfaces/committee.interface.ts (1)
Committee(4-27)packages/shared/src/interfaces/member.interface.ts (1)
CommitteeMember(4-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Quality Checks
🔇 Additional comments (25)
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-form/meeting-form.component.html (1)
313-313: LGTM! Improved form submission handling.Removing the explicit
(onClick)handler on the submit button is a good practice. The form submission is now handled consistently through the(ngSubmit)event on the form element, which ensures proper form validation and follows standard HTML form semantics.apps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.html (3)
4-4: LGTM! Consistent event handler naming.Updating the form submission handler from
handleSubmit()toonSubmit()improves consistency with the standardized "on" prefix naming convention used across other form components in the project.
118-118: LGTM! Consistent event handler naming.Updating the cancel button handler from
handleCancel()toonCancel()aligns with the standardized naming convention.
124-124: LGTM! Improved form submission handling.The submit button now relies on the form's
(ngSubmit)event rather than a redundant click handler, which is a best practice for form handling and ensures proper validation flow.apps/lfx-pcc/src/app/modules/project/committees/components/committee-form/committee-form.component.ts (5)
54-54: LGTM! Consistent event handler naming.Renaming
handleSubmit()toonSubmit()follows the standardized "on" prefix convention used across other form components in the project, improving code consistency.
69-69: LGTM! Internal method calls updated correctly.All internal calls to the renamed success and error handlers have been properly updated to use the new method names, maintaining functionality while following the new naming convention.
Also applies to: 73-73, 81-81, 85-85
95-95: LGTM! Consistent event handler naming.Renaming
handleCancel()toonCancel()aligns with the standardized naming convention and matches the template binding update.
130-130: LGTM! Consistent method naming.Renaming
handleSuccess()toonSuccess()follows the established naming pattern for event handlers and callbacks.
152-152: LGTM! Consistent method naming.Renaming
handleError()toonError()completes the standardization of method naming throughout the component.apps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.ts (6)
1-2: License header compliance verified.The required license header is properly included as specified in the coding guidelines.
22-23: Imports properly updated for reactive pattern.The addition of
BehaviorSubjectandswitchMapimports supports the new reactive refresh mechanism correctly.
72-74: Property declarations follow good practices.The
refreshproperty is correctly typed asBehaviorSubject<void>and the reordering ofsearchTermanddialogRefmaintains consistency. The public access modifier onrefreshis appropriate for the component's needs.
84-84: BehaviorSubject initialization is correct.The
refreshsubject is properly initialized withundefinedas the initial value, which is appropriate for triggering refresh actions.
194-194: Simplified refresh mechanism improves maintainability.The refactored
refreshCommittees()method now correctly uses the reactive pattern by emitting on therefreshsubject instead of complex router navigation. This is cleaner and more efficient.
228-238: Reactive data loading pattern implemented correctly.The
initializeCommittees()method properly implements the reactive pattern:
- Uses
refresh.pipe()to trigger data fetchingtap()sets loading state correctlyswitchMap()ensures previous requests are cancelled- Handles the case when no project is available by returning empty array
- Loading state management is consistent
This pattern aligns well with the broader architectural improvements mentioned in the AI summary.
apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts (8)
1-2: License header compliance verified.The required license header is properly included as specified in the coding guidelines.
5-5: Import cleanup noted.The
Injectorimport is retained even though the AI summary mentionsrunInInjectionContextwas removed. This suggestsInjectormight still be needed elsewhere in the component.
22-23: Reactive pattern imports added correctly.The addition of
BehaviorSubjectandswitchMapimports supports the new reactive refresh mechanism. These align with the same pattern used in the CommitteeDashboardComponent.
75-75: Refresh subject property properly declared.The
refreshproperty is correctly typed asBehaviorSubject<void>and marked as public, consistent with the pattern established in other dashboard components.
83-83: BehaviorSubject initialization is consistent.The
refreshsubject is properly initialized withundefined, matching the pattern used in the CommitteeDashboardComponent.
161-161: Simplified refresh mechanism implemented correctly.The
refreshMeetings()method now uses the reactive pattern by emitting on therefreshsubject. This is much cleaner than the previousrunInInjectionContextapproach mentioned in the AI summary.
197-208: Reactive meetings data loading implemented correctly.The
initializeMeetings()method properly implements the reactive pattern:
- Uses
refresh.pipe(switchMap(...))to trigger data fetchingtap()correctly setsmeetingsLoadingto false after fetch- Handles null project case by returning empty array
- Maintains proper signal initialization with empty array default
The pattern is consistent with the CommitteeDashboardComponent implementation.
210-221: Reactive past meetings data loading implemented correctly.The
initializePastMeetings()method follows the same correct pattern:
- Uses
refresh.pipe(switchMap(...))for reactive data fetchingtap()setspastMeetingsLoadingto false appropriately- Consistent error handling and initialization approach
- Matches the pattern established in the meetings initialization
Both meeting data initialization methods now use the same reactive architecture for consistency.
apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.ts (1)
5-106: LGTM! Good refactoring to improve separation of concernsThe component has been successfully refactored to receive members and loading state as inputs rather than managing them internally. This improves:
- Separation of concerns by moving data fetching to the parent component
- Reusability by making the component more flexible
- Testability by reducing dependencies
The removal of the unused
finalizeoperator import is also appropriate.apps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts (1)
63-93: LGTM! Clean reactive refresh patternThe refresh mechanism using
BehaviorSubjectcombined withcombineLatestis a good reactive pattern that allows data to be refreshed on demand without route navigation.
apps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.html
Show resolved
Hide resolved
apps/lfx-pcc/src/app/modules/project/committees/committee-view/committee-view.component.ts
Show resolved
Hide resolved
Signed-off-by: Asitha de Silva <asithade@gmail.com>
Summary
Features
Technical Implementation
🤖 Generated with Claude Code