feat(permissions): implement user permissions management system #99
Conversation
Implement comprehensive user permissions management for projects with role-based access control. - Add user management API endpoints (add/update/remove users) - Create user permissions UI components (table, form, matrix) - Implement route guards and conditional UI for settings access - Simplify from project/committee scope to project-wide roles - Update E2E tests for permission-based settings visibility JIRA: LFXV2-548, LFXV2-549, LFXV2-550, LFXV2-551, LFXV2-552, LFXV2-553, LFXV2-554 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
WalkthroughIntroduces writer-gated Settings access in UI and routes, adds a writer route guard, updates E2E tests, replaces the permissions model with a role-based (view/manage) scheme across UI, client service, and server. Removes legacy permissions router, adds new project-scoped permissions endpoints, and adds shared interfaces for roles and project settings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant R as Angular Router
participant G as writerGuard
participant US as UserService
participant PS as ProjectService
participant S as Server (GET /projects/:slug)
U->>R: Navigate to /project/:slug/settings
R->>G: canActivate(route)
G->>US: authenticated()
alt not authenticated
G-->>R: false (deny)
else authenticated
G->>PS: getProject(slug, noCache)
PS->>S: Fetch project
S-->>PS: Project{ writer: boolean }
PS-->>G: Project
alt project not found
G->>R: redirect "/"
G-->>R: false
else writer === false
G->>R: redirect "/project/:slug"
G-->>R: false
else writer === true
G-->>R: true (allow)
end
end
sequenceDiagram
autonumber
actor A as Admin (Writer)
participant UI as Settings UI
participant CPS as Client PermissionsService
participant API as Server /projects/:uid/permissions
participant SV as ProjectService (server)
participant V2 as V2 Backend + ETag
Note over UI,API: List permissions
A->>UI: Open Permissions
UI->>CPS: getProjectPermissions(uid)
CPS->>API: GET /:uid/permissions
API->>SV: getProjectSettings(uid)
SV->>V2: GET settings (with ETag)
V2-->>SV: Settings + ETag
SV-->>API: Settings
API-->>CPS: Settings
CPS-->>UI: ProjectPermissionUser[]
rect rgba(200,255,200,0.2)
Note over UI,API: Add or Update role
A->>UI: Add/Update user role
UI->>CPS: addUserToProject/updateUserRole
CPS->>API: POST or PUT
API->>SV: updateProjectPermissions(op, username, role)
SV->>V2: GET settings + ETag
V2-->>SV: Settings + ETag
SV->>SV: Apply op (writers/auditors)
SV->>V2: PUT settings with If-Match ETag
V2-->>SV: 200 OK
SV-->>API: Updated settings
API-->>CPS: 201/200
CPS-->>UI: success
end
rect rgba(255,220,220,0.2)
Note over UI,API: Remove user
A->>UI: Remove user
UI->>CPS: removeUserFromProject
CPS->>API: DELETE /:uid/permissions/:username
API->>SV: updateProjectPermissions(remove, username)
SV->>V2: GET settings + ETag
V2-->>SV: Settings + ETag
SV->>SV: Remove from writers/auditors
SV->>V2: PUT settings with If-Match
V2-->>SV: 204/200
SV-->>API: ok
API-->>CPS: 204 No Content
CPS-->>UI: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
github-actions
bot
requested review from
andrest50,
audigregorie,
dealako and
mauriciozanettisalomao
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive user permissions management system for projects with simplified role-based access control, transitioning from a complex project/committee scope model to a streamlined project-wide View/Manage role system.
- Simplifies permission model from project/committee scopes to unified project-wide roles (View/Manage)
- Adds complete user management API endpoints and UI components for adding, updating, and removing users
- Implements route guards and conditional UI visibility for Settings access based on writer permissions
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/project.interface.ts | Adds ProjectSettings interface for project permission configuration |
| packages/shared/src/interfaces/permissions.interface.ts | Defines simplified permission interfaces for project-wide roles |
| apps/lfx-one/src/server/services/project.service.ts | Implements user permission management methods with ETag support |
| apps/lfx-one/src/server/server.ts | Removes legacy permissions router in favor of integrated project routes |
| apps/lfx-one/src/server/routes/projects.route.ts | Adds new permission management endpoints to project routes |
| apps/lfx-one/src/server/routes/permissions.route.ts | Removes legacy permissions route implementation |
| apps/lfx-one/src/server/controllers/project.controller.ts | Implements new permission management controller methods |
| apps/lfx-one/src/app/shared/services/permissions.service.ts | Updates service to use simplified permission model |
| apps/lfx-one/src/app/shared/guards/writer.guard.ts | Implements route guard for writer access protection |
| Frontend components | Updates user permissions UI components to simplified role-based model |
| apps/lfx-one/src/app/modules/project/project.routes.ts | Adds writer guard to settings route |
| apps/lfx-one/src/app/layouts/project-layout/project-layout.component.* | Conditionally shows Settings menu based on writer access |
| apps/lfx-one/e2e/project-dashboard.spec.ts | Updates E2E tests for conditional Settings visibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...les/project/settings/components/user-permissions-table/user-permissions-table.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html (1)
3-3: Add data-testid to root container (guideline).Per coding guidelines for apps/lfx-one/src/**/*.html, add a stable data-testid on the root element for E2E targeting.
Apply this diff:
-<lfx-card header="Permission Guide"> +<lfx-card header="Permission Guide" data-testid="settings-permissions-matrix-card">apps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
29-53: Users signal won’t react to project changes; possible empty/stale list.Constructor currently chooses between
this.refresh.pipe(...)andof([])once at initialization; if the project loads later or changes,usersstays empty/stale. Make it react to both refresh and project changes — apply the diff below.File: apps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts
- public users: Signal<ProjectPermissionUser[]>; + public users: Signal<ProjectPermissionUser[]>; @@ - this.users = toSignal( - this.project()?.uid - ? this.refresh.pipe( - tap(() => this.loading.set(true)), - switchMap(() => this.permissionsService.getProjectPermissions(this.project()?.uid as string).pipe(tap(() => this.loading.set(false)))) - ) - : of([]), - { - initialValue: [], - } - ); + this.users = toSignal( + combineLatest([this.refresh.pipe(startWith(undefined)), toObservable(this.project)]).pipe( + switchMap(([_, project]) => { + const uid = project?.uid; + if (!uid) { + return of<ProjectPermissionUser[]>([]); + } + this.loading.set(true); + return this.permissionsService.getProjectPermissions(uid).pipe(tap(() => this.loading.set(false))); + }) + ), + { initialValue: [] } + );Add these imports outside this hunk:
import { toSignal, toObservable } from '@angular/core/rxjs-interop'; import { BehaviorSubject, of, switchMap, take, tap, combineLatest, startWith } from 'rxjs';
🧹 Nitpick comments (26)
apps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html (2)
12-13: Instrument matrix rows/elements with data-testid attributes.Add stable testids to rows, badges, descriptions, and capability items to align with the “Always add data-testid” rule.
Apply this diff:
- @for (item of permissionMatrix; track item.scope + item.level) { - <div class="border border-gray-200 rounded-lg p-3"> + @for (item of permissionMatrix; track item.scope + '-' + item.level) { + <div class="border border-gray-200 rounded-lg p-3" + [attr.data-testid]="'settings-permissions-matrix-row-' + item.scope + '-' + item.level"> <div class="flex items-center justify-between mb-2"> - <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" + [class]="item.badge.bgColor + ' ' + item.badge.color" + [attr.data-testid]="'settings-permissions-matrix-badge-' + item.scope + '-' + item.level"> {{ item.scope }} {{ item.level }} </span> </div> - <p class="text-xs text-gray-600 mb-2">{{ item.description }}</p> - <ul class="text-xs text-gray-500 space-y-1"> - @for (capability of item.capabilities; track capability) { - <li class="flex items-start"> - <i class="fa-light fa-check text-green-500 mr-1 mt-0.5" style="font-size: 10px"></i> - {{ capability }} - </li> + <p class="text-xs text-gray-600 mb-2" + [attr.data-testid]="'settings-permissions-matrix-desc-' + item.scope + '-' + item.level"> + {{ item.description }} + </p> + <ul class="text-xs text-gray-500 space-y-1" + [attr.data-testid]="'settings-permissions-matrix-capabilities-' + item.scope + '-' + item.level"> + @for (capability of item.capabilities; track capability; let i = $index) { + <li class="flex items-start" + [attr.data-testid]="'settings-permissions-matrix-capability-' + i"> + <i class="fa-light fa-check text-green-500 mr-1 mt-0.5 text-[10px]" aria-hidden="true"></i> + <span>{{ capability }}</span> + </li> } </ul>Also applies to: 15-16, 19-21, 23-25
23-23: Accessibility: mark decorative icon as aria-hidden and avoid inline style.The check icon is decorative; hide it from AT and prefer utility class over inline style.
Apply this diff:
- <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]" aria-hidden="true"></i>packages/shared/src/interfaces/project.interface.ts (1)
120-137: ProjectSettings looks good; clarify role mapping and date formats.
- Please document that writers == 'manage' role and auditors == 'view' role to align with the new ProjectPermissionUser model.
- Explicitly note ISO 8601 in date fields to avoid ambiguity.
Suggested additions outside this hunk:
// Prefer a clear alias for dates used across shared interfaces export type ISODateString = string;And update fields:
- announcement_date: string; + announcement_date: ISODateString; - created_at: string; + created_at: ISODateString; - updated_at: string; + updated_at: ISODateString;apps/lfx-one/src/app/modules/project/project.routes.ts (1)
31-31: Consider canMatch for earlier, cheaper gating.Using canMatch prevents route activation and component loading earlier in the pipeline, reducing work for unauthorized users. You can keep canActivate as defense in depth.
Example (outside this hunk):
import { writerMatchGuard } from '../../shared/guards/writer.guard'; { path: 'settings', canMatch: [writerMatchGuard], canActivate: [writerGuard], loadComponent: () => import('./settings/settings-dashboard/settings-dashboard.component').then(m => m.SettingsDashboardComponent), data: { preload: false }, }apps/lfx-one/src/server/routes/projects.route.ts (1)
23-30: API path naming diverges from objectives (/permissions vs /users).PR objectives mention .../projects/:projectId/users; routes here use .../permissions. Align path naming or update API docs/clients to avoid confusion.
apps/lfx-one/src/app/shared/guards/writer.guard.ts (2)
30-32: Handle missing slug with a redirect UrlTree.Avoid silent navigation cancel.
- if (!projectSlug) { - return false; - } + if (!projectSlug) { + return router.createUrlTree(['/']); + }
35-51: Return an UrlTree from the guard instead of calling router.navigate()Returning an UrlTree lets the router handle redirection (no imperative navigation side effects). Verified that ProjectService.getProject(slug: string, current: boolean = true) exists at apps/lfx-one/src/app/shared/services/project.service.ts:26 and supports passing false.
return projectService.getProject(projectSlug, false).pipe( - map((project) => { - if (!project) { - // Project not found, redirect to home - router.navigate(['/']); - return false; - } - - if (!project.writer) { - // User doesn't have writer access, redirect to project dashboard - router.navigate(['/project', projectSlug]); - return false; - } - - return true; - }) + map((project) => { + if (!project) { + return router.createUrlTree(['/']); + } + if (!project.writer) { + return router.createUrlTree(['/project', projectSlug]); + } + return true; + }) );apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html (2)
61-66: Prefer property binding for routerLinkUse [routerLink]="['/project', projectSlug(), 'settings']" instead of string interpolation to avoid accidental malformed URLs and enable Angular’s route generation.
Apply this diff:
- <a - routerLink="/project/{{ projectSlug() }}/settings" + <a + [routerLink]="['/project', projectSlug(), 'settings']" class="pill"- <a - routerLink="/project/{{ projectSlug() }}/settings" + <a + [routerLink]="['/project', projectSlug(), 'settings']" class="pill"Also applies to: 75-81
64-66: Test id naming consistency (optional)Consider data-testid naming per guideline [section]-[component]-[element], e.g., project-layout-settings-mobile and project-layout-settings-desktop for future clarity. Current ids are fine given updated E2E.
Also applies to: 78-80
packages/shared/src/interfaces/permissions.interface.ts (1)
145-151: Unify role typing and avoid repeating string unionsReplace repeated 'view' | 'manage' with a single exported enum or type to prevent drift across client/server and ease validation reuse.
Apply this diff to changed lines:
-export interface ProjectPermissionUser { +export interface ProjectPermissionUser { /** Username identifier */ username: string; /** Permission role - 'view' for auditors, 'manage' for writers */ - role: 'view' | 'manage'; + role: ProjectRole; } /** * Request payload for adding user to project * @description Data required to add a user to project writers or auditors */ export interface AddUserToProjectRequest { /** Username to add */ username: string; /** Role to assign - 'view' for auditors, 'manage' for writers */ - role: 'view' | 'manage'; + role: ProjectRole; } /** * Request payload for updating user role in project * @description Data required to change a user's role in project */ export interface UpdateUserRoleRequest { /** New role to assign - 'view' for auditors, 'manage' for writers */ - role: 'view' | 'manage'; + role: ProjectRole; }Add this outside the changed block:
// Prefer enum to union for maintainability and shared reuse export enum ProjectRole { View = 'view', Manage = 'manage', }Also applies to: 156-161, 167-171
apps/lfx-one/src/server/services/project.service.ts (2)
162-171: Normalize usernames to avoid duplicates and case sensitivity issuesTrim and lowercase the username before compare/add to ensure consistent storage and filtering.
Apply this diff:
- // Initialize arrays if they don't exist + // Initialize arrays if they don't exist + username = username.trim(); + const normalizedUsername = username.toLowerCase(); if (!updatedSettings.writers) updatedSettings.writers = []; if (!updatedSettings.auditors) updatedSettings.auditors = []; // Remove user from both arrays first (for all operations) - updatedSettings.writers = updatedSettings.writers.filter((u) => u !== username); - updatedSettings.auditors = updatedSettings.auditors.filter((u) => u !== username); + updatedSettings.writers = updatedSettings.writers.filter((u) => u.toLowerCase() !== normalizedUsername); + updatedSettings.auditors = updatedSettings.auditors.filter((u) => u.toLowerCase() !== normalizedUsername);And here:
- if (role === 'manage') { - updatedSettings.writers = [...new Set([...updatedSettings.writers, username])]; + if (role === 'manage') { + updatedSettings.writers = [...new Set([...updatedSettings.writers, normalizedUsername])]; } else { - updatedSettings.auditors = [...new Set([...updatedSettings.auditors, username])]; + updatedSettings.auditors = [...new Set([...updatedSettings.auditors, normalizedUsername])]; }
145-151: Use shared role typePrefer using ProjectRole from shared interfaces instead of a local string union to prevent drift.
- role?: 'view' | 'manage' + role?: ProjectRoleAnd add:
import { ProjectRole } from '@lfx-one/shared/interfaces';apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.html (1)
9-20: Form UX/a11y polish for usernameAdd autocomplete="username" to improve UX; associate error with input via aria-describedby.
Apply this diff:
- <lfx-input-text + <lfx-input-text size="small" [form]="form()" control="username" id="username" placeholder="Enter username" styleClass="w-full" - data-testid="settings-user-form-username"></lfx-input-text> + data-testid="settings-user-form-username" + autocomplete="username" + aria-describedby="username-error"></lfx-input-text> @@ - <p class="mt-1 text-xs text-red-600">Username is required</p> + <p id="username-error" class="mt-1 text-xs text-red-600">Username is required</p>apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html (1)
54-62: Add accessible label to actions buttonScreen readers will announce only the icon. Add aria-label like "User actions for {{ user.username }}".
Apply this diff:
- <lfx-button + <lfx-button [icon]="isRemoving() === user.username ? 'fa-light fa-circle-notch fa-spin' : 'fa-light fa-ellipsis-vertical'" [text]="true" [rounded]="true" size="small" severity="secondary" [disabled]="isRemoving() === user.username" (onClick)="toggleUserActionMenu($event, user, userActionMenu)" - data-testid="user-actions-menu"> + data-testid="user-actions-menu" + aria-label="User actions for {{ user.username }}"> </lfx-button>apps/lfx-one/e2e/project-dashboard.spec.ts (1)
442-457: Add a negative route-guard test (optional)Consider attempting navigation to /settings as a read-only user and assert redirect/denial to validate server/guard behavior end-to-end.
Also applies to: 486-489
apps/lfx-one/src/app/shared/services/permissions.service.ts (2)
31-59: Deduplicate usernames when merging writers/auditorsIf upstream data ever contains overlap, UI would show duplicates. Simple Set-based dedupe will harden this.
Apply this diff:
public getProjectPermissions(project: string): Observable<ProjectPermissionUser[]> { return this.http.get<ProjectSettings>(`/api/projects/${project}/permissions`).pipe( map((settings: ProjectSettings) => { - const users: ProjectPermissionUser[] = []; + const seen = new Set<string>(); + const users: ProjectPermissionUser[] = []; @@ - if (settings.auditors) { + if (settings.auditors) { users.push( ...settings.auditors.map((username) => ({ username, role: 'view' as const, })) ); } @@ - if (settings.writers) { + if (settings.writers) { users.push( ...settings.writers.map((username) => ({ username, role: 'manage' as const, })) ); } - return users.sort((a, b) => a.username.localeCompare(b.username)); + const deduped = users.filter((u) => (seen.has(u.username) ? false : (seen.add(u.username), true))); + return deduped.sort((a, b) => a.username.localeCompare(b.username)); }) ); }
15-28: Centralize API base and adopt shared role type (optional)Extract base path
/api/projects/${project}/permissionsinto a private helper to reduce duplication; prefer ProjectRole for request types when adopting enum.apps/lfx-one/src/server/controllers/project.controller.ts (2)
234-265: Trim/normalize username inputsNormalize req.params.username and body.username (trim/lowercase) before passing to service to avoid duplicate records with different casing.
Apply this diff:
- const userData: AddUserToProjectRequest = req.body; + const userData: AddUserToProjectRequest = req.body; + userData.username = userData.username?.trim(); @@ - const result = await this.projectService.updateProjectPermissions(req, uid, 'add', userData.username, userData.role); + const result = await this.projectService.updateProjectPermissions(req, uid, 'add', userData.username.toLowerCase(), userData.role);- const roleData: UpdateUserRoleRequest = req.body; + const roleData: UpdateUserRoleRequest = req.body; @@ - const result = await this.projectService.updateProjectPermissions(req, uid, 'update', username, roleData.role); + const result = await this.projectService.updateProjectPermissions(req, uid, 'update', username.trim().toLowerCase(), roleData.role);- await this.projectService.updateProjectPermissions(req, uid, 'remove', username); + await this.projectService.updateProjectPermissions(req, uid, 'remove', username.trim().toLowerCase());Also applies to: 319-350, 405-413
266-274: Audit logging: include actorAugment success logs with the acting user identifier (e.g., req.user.sub or equivalent) for traceability.
Also applies to: 351-358, 407-413
apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (2)
94-101: Avoid non‑null assertion on project() and handle ETag conflicts (409/412).Capture projectId once, show a user‑facing error if missing, and add conflict/precondition handling.
Apply this diff:
- private removeUser(user: ProjectPermissionUser): void { - if (!this.project()) return; - - this.isRemoving.set(user.username); - - this.permissionsService - .removeUserFromProject(this.project()!.uid, user.username) + private removeUser(user: ProjectPermissionUser): void { + const projectId = this.project()?.uid; + if (!projectId) { + this.messageService.add({ + severity: 'error', + summary: 'Error', + detail: 'Project is not loaded. Please refresh and try again.', + life: 4000, + }); + return; + } + + this.isRemoving.set(user.username); + + this.permissionsService + .removeUserFromProject(projectId, user.username) .pipe(take(1)) .subscribe({ next: () => { this.isRemoving.set(null); @@ - // Provide more specific error messages based on error status + // Provide more specific error messages based on error status if (error?.status === 404) { errorMessage = 'User not found. They may have already been removed.'; } else if (error?.status === 403) { errorMessage = 'You do not have permission to remove this user.'; + } else if (error?.status === 409 || error?.status === 412) { + errorMessage = 'Changes conflict with a newer project version. Please refresh and try again.'; } else if (error?.status === 500) { errorMessage = 'Server error occurred. Please try again later.'; } else if (error?.status === 0) { errorMessage = 'Network error. Please check your connection and try again.'; }Also applies to: 119-131
24-24: Consider OnPush change detection for performance.This table is input‑driven and uses signals; OnPush will reduce unnecessary change detection.
Add:
-import { Component, inject, input, output, signal, WritableSignal } from '@angular/core'; +import { Component, ChangeDetectionStrategy, inject, input, output, signal, WritableSignal } from '@angular/core';and in the @component metadata:
@Component({ selector: 'lfx-user-permissions-table', standalone: true, imports: [CommonModule, TableComponent, TooltipModule, CardComponent, ConfirmDialogModule, ButtonComponent, MenuComponent], templateUrl: './user-permissions-table.component.html', + changeDetection: ChangeDetectionStrategy.OnPush, })apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.ts (5)
9-9: Unify import path for shared interfaces.Other components import from '@lfx-one/shared/interfaces'. Mixing barrel vs. subpath can cause duplicate type copies in some bundlers.
Apply this diff:
-import { AddUserToProjectRequest, ProjectPermissionUser, UpdateUserRoleRequest } from '@lfx-one/shared'; +import { AddUserToProjectRequest, ProjectPermissionUser, UpdateUserRoleRequest } from '@lfx-one/shared/interfaces';
41-44: Preserve literal types for role options.Mark as const to keep 'view' | 'manage' literals and prevent accidental mutation.
Apply this diff:
- public permissionLevelOptions = [ - { label: 'View', value: 'view' }, - { label: 'Manage', value: 'manage' }, - ]; + public permissionLevelOptions = [ + { label: 'View', value: 'view' as const }, + { label: 'Manage', value: 'manage' as const }, + ] as const;
74-85: Harden submit flow: trim username, capture projectId once, reset submitting via finalize, and surface specific errors.Improves UX and resiliency; avoids stale signal reads and ambiguous failures.
Apply these diffs:
- Import finalize:
-import { take } from 'rxjs'; +import { take, finalize } from 'rxjs';
- Submit logic:
- this.submitting.set(true); - const formValue = this.form().value; - - const operation = this.isEditing() - ? this.permissionsService.updateUserRole(project.uid, this.user()!.username, { - role: formValue.role, - } as UpdateUserRoleRequest) - : this.permissionsService.addUserToProject(project.uid, { - username: formValue.username, - role: formValue.role, - } as AddUserToProjectRequest); - - operation.pipe(take(1)).subscribe({ + this.submitting.set(true); + const { username, role } = this.form().getRawValue(); + const projectId = project.uid; + + const operation$ = this.isEditing() + ? this.permissionsService.updateUserRole(projectId, this.user()!.username, { role } as UpdateUserRoleRequest) + : this.permissionsService.addUserToProject(projectId, { username: (username as string).trim(), role } as AddUserToProjectRequest); + + operation$.pipe(take(1), finalize(() => this.submitting.set(false))).subscribe({ next: () => { this.messageService.add({ severity: 'success', summary: 'Success', detail: `User ${this.isEditing() ? 'updated' : 'added'} successfully`, }); this.dialogRef.close(true); }, - error: (error: any) => { - console.error('Error saving user:', error); - this.messageService.add({ - severity: 'error', - summary: 'Error', - detail: `Failed to ${this.isEditing() ? 'update' : 'add'} user. Please try again.`, - }); - this.submitting.set(false); - }, + error: (error: any) => { + console.error('Error saving user:', error); + const action = this.isEditing() ? 'update' : 'add'; + let detail = `Failed to ${action} user. Please try again.`; + if (error?.status === 400) detail = 'Invalid input. Check the username and role.'; + else if (error?.status === 403) detail = 'You do not have permission to perform this action.'; + else if (error?.status === 404) detail = 'Project or user not found.'; + else if (error?.status === 409 || error?.status === 412) detail = 'Changes conflict with a newer project version. Please refresh.'; + else if (error?.status === 0) detail = 'Network error. Check your connection and retry.'; + this.messageService.add({ severity: 'error', summary: 'Error', detail }); + }, });Also applies to: 86-105, 15-15
114-116: Add basic username constraints.Prevents obvious bad inputs client‑side.
Apply this diff:
- username: new FormControl('', [Validators.required]), + username: new FormControl('', [ + Validators.required, + Validators.minLength(3), + Validators.maxLength(64), + Validators.pattern(/^[a-zA-Z0-9._-]+$/), + ]),
4-4: Consider OnPush change detection.The form is dialog‑scoped, signal‑driven, and benefits from fewer checks.
Add:
-import { Component, computed, inject, signal } from '@angular/core'; +import { Component, ChangeDetectionStrategy, computed, inject, signal } from '@angular/core';and:
@Component({ selector: 'lfx-user-form', standalone: true, imports: [ReactiveFormsModule, InputTextComponent, ButtonComponent, RadioButtonComponent, TooltipModule], templateUrl: './user-form.component.html', + changeDetection: ChangeDetectionStrategy.OnPush, })Also applies to: 20-21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
apps/lfx-one/e2e/project-dashboard.spec.ts(3 hunks)apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html(1 hunks)apps/lfx-one/src/app/layouts/project-layout/project-layout.component.ts(1 hunks)apps/lfx-one/src/app/modules/project/project.routes.ts(2 hunks)apps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html(1 hunks)apps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.ts(0 hunks)apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.html(3 hunks)apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.ts(3 hunks)apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html(3 hunks)apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts(5 hunks)apps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts(2 hunks)apps/lfx-one/src/app/shared/guards/writer.guard.ts(1 hunks)apps/lfx-one/src/app/shared/services/permissions.service.ts(1 hunks)apps/lfx-one/src/server/controllers/project.controller.ts(2 hunks)apps/lfx-one/src/server/routes/permissions.route.ts(0 hunks)apps/lfx-one/src/server/routes/projects.route.ts(1 hunks)apps/lfx-one/src/server/server.ts(0 hunks)apps/lfx-one/src/server/services/project.service.ts(3 hunks)packages/shared/src/interfaces/permissions.interface.ts(1 hunks)packages/shared/src/interfaces/project.interface.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/lfx-one/src/server/server.ts
- apps/lfx-one/src/server/routes/permissions.route.ts
- apps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/app/modules/project/project.routes.tspackages/shared/src/interfaces/permissions.interface.tsapps/lfx-one/src/app/layouts/project-layout/project-layout.component.tsapps/lfx-one/src/server/routes/projects.route.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.tsapps/lfx-one/src/app/shared/services/permissions.service.tsapps/lfx-one/src/app/shared/guards/writer.guard.tsapps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.tsapps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.tsapps/lfx-one/e2e/project-dashboard.spec.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/modules/project/project.routes.tspackages/shared/src/interfaces/permissions.interface.tsapps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.htmlapps/lfx-one/src/app/layouts/project-layout/project-layout.component.tsapps/lfx-one/src/server/routes/projects.route.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.tsapps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.htmlapps/lfx-one/src/app/shared/services/permissions.service.tsapps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.htmlapps/lfx-one/src/app/shared/guards/writer.guard.tsapps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.tsapps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.tsapps/lfx-one/src/app/layouts/project-layout/project-layout.component.htmlapps/lfx-one/e2e/project-dashboard.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/app/modules/project/project.routes.tspackages/shared/src/interfaces/permissions.interface.tsapps/lfx-one/src/app/layouts/project-layout/project-layout.component.tsapps/lfx-one/src/server/routes/projects.route.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.tsapps/lfx-one/src/app/shared/services/permissions.service.tsapps/lfx-one/src/app/shared/guards/writer.guard.tsapps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.tsapps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.tsapps/lfx-one/e2e/project-dashboard.spec.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all TypeScript interfaces in the shared package at packages/shared/src/interfaces
Files:
packages/shared/src/interfaces/permissions.interface.tspackages/shared/src/interfaces/project.interface.ts
apps/lfx-one/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.html: Always add data-testid attributes when creating new Angular components for reliable test targeting
Use data-testid naming convention [section]-[component]-[element]
Files:
apps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.htmlapps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.htmlapps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.htmlapps/lfx-one/src/app/layouts/project-layout/project-layout.component.html
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use .spec.ts for content-based tests
Files:
apps/lfx-one/e2e/project-dashboard.spec.ts
🧬 Code graph analysis (8)
apps/lfx-one/src/app/modules/project/project.routes.ts (1)
apps/lfx-one/src/app/shared/guards/writer.guard.ts (1)
writerGuard(17-52)
apps/lfx-one/src/server/controllers/project.controller.ts (1)
packages/shared/src/interfaces/permissions.interface.ts (2)
AddUserToProjectRequest(156-161)UpdateUserRoleRequest(167-170)
apps/lfx-one/src/server/services/project.service.ts (1)
packages/shared/src/interfaces/project.interface.ts (1)
ProjectSettings(124-137)
apps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
packages/shared/src/interfaces/permissions.interface.ts (1)
ProjectPermissionUser(145-150)
apps/lfx-one/src/app/shared/services/permissions.service.ts (2)
packages/shared/src/interfaces/permissions.interface.ts (3)
AddUserToProjectRequest(156-161)UpdateUserRoleRequest(167-170)ProjectPermissionUser(145-150)packages/shared/src/interfaces/project.interface.ts (1)
ProjectSettings(124-137)
apps/lfx-one/src/app/shared/guards/writer.guard.ts (1)
apps/lfx-one/src/server/services/project.service.ts (1)
ProjectService(16-206)
apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.ts (4)
apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
Component(21-159)apps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
Component(19-76)apps/lfx-one/src/server/services/project.service.ts (1)
ProjectService(16-206)packages/shared/src/interfaces/permissions.interface.ts (3)
ProjectPermissionUser(145-150)UpdateUserRoleRequest(167-170)AddUserToProjectRequest(156-161)
apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
packages/shared/src/interfaces/permissions.interface.ts (1)
ProjectPermissionUser(145-150)
⏰ 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: E2E Tests / Playwright E2E Tests
🔇 Additional comments (11)
apps/lfx-one/src/app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html (2)
1-2: License header present and correct.SPDX and copyright lines look good.
12-12: Confirm badge class sources are trusted (defense-in-depth).[item.badge.bgColor] and [item.badge.color] feed into [class] as strings. Ensure these values are from a trusted, static map (not user input) to avoid unintended class injection.
Also applies to: 15-15
apps/lfx-one/src/app/modules/project/project.routes.ts (1)
6-7: Import path is correct.Guard import path resolves correctly from modules/project to shared/guards.
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.ts (1)
47-47: LGTM: explicit boolean check avoids truthy pitfalls.Strict compare against true is clear and safe for conditional UI.
apps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
8-8: Type swap to ProjectPermissionUser is correct.Matches the simplified role model (username + role).
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html (1)
59-72: Gating Settings behind hasWriterAccess(): good alignment with route guardConditional render for mobile Settings looks correct and matches tests.
apps/lfx-one/src/server/services/project.service.ts (1)
185-205: ETag conflict handling: confirm behaviorIf updateWithETag encounters a 412/409, ensure we translate to a 409 Conflict (or retry with backoff). Verify ETagService behavior and add a targeted error message for clients to refresh.
apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.html (1)
38-46: Ensure role is required at form levelConfirm the FormGroup includes Validators.required for control 'role' and that a validation message is shown when not selected.
Also applies to: 80-84
apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html (1)
36-48: LGTM: simplified columns align with new modelUsername/Role presentation and conditional styling for Manage/View look good.
apps/lfx-one/e2e/project-dashboard.spec.ts (1)
85-99: Good coverage for Settings visibility across viewportsThe manage-role visibility checks for desktop and mobile are solid.
apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
10-10: Model switch to ProjectPermissionUser is consistent.Types, method signatures, and state updates are aligned with the new interface.
Also applies to: 35-35, 39-39, 46-46, 69-69, 75-75, 78-78
...app/modules/project/settings/components/permissions-matrix/permissions-matrix.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.ts
Show resolved
Hide resolved
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-99.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
🧹 Deployment RemovedThe deployment for PR #99 has been removed. |
Summary
Implement comprehensive user permissions management for projects with role-based access control.
Technical Changes
Test Plan
JIRA: LFXV2-548, LFXV2-549, LFXV2-550, LFXV2-551, LFXV2-552, LFXV2-553, LFXV2-554