-
Notifications
You must be signed in to change notification settings - Fork 0
feat(permissions): implement email-first workflow with email-to-sub l… #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ookup Update project permissions user form to use email-first workflow: - Ask for email first, then show confirmation if user not found - Support manual entry with full user details (name, username, avatar) - Replace EMAIL_TO_USERNAME with EMAIL_TO_SUB NATS subject - Convert writers/auditors from string arrays to UserInfo objects - Clean empty avatar fields before API submission to fix validation - Add user initials fallback when avatar not available - Display full user info (name, email, username, avatar) in table Changes include: - Add EMAIL_TO_SUB to NatsSubjects enum - Update resolveEmailToUsername to use email_to_sub endpoint - Add getUserInfo method for fetching complete user profile - Modify updateProjectPermissions to handle UserInfo objects - Update user form with conditional field display logic - Add confirmation dialog for user not found scenario - Enhance permissions table with avatar, name, and email columns - Clean up empty avatar strings to prevent API validation errors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jordan Evans <[email protected]>
WalkthroughIntroduces email-first user addition with optional manual entry on not-found; updates UI to display avatar/name/email; expands permissions data from usernames to full user objects; adjusts server APIs to accept and propagate manual user info; adds new NATS subjects and user info retrieval; updates shared interfaces and enums accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin (UI)
participant UF as UserFormComponent
participant API as ProjectController
participant SVC as ProjectService
participant NATS as NATS
participant Store as ProjectSettings
Admin->>UF: Submit Add User (email + role)
alt Manual fields not shown
UF->>API: POST addUser(email, role)
alt manualUserInfo provided (edit/manual path)
API->>SVC: updateProjectPermissions(add/update, username, role, manualUserInfo)
SVC->>Store: Upsert writers/auditors with manualUserInfo
Store-->>UF: Success
else lookup needed
SVC->>NATS: EMAIL_TO_SUB / USERNAME_TO_USER_INFO
alt Found user info
SVC->>Store: Upsert with UserInfo (name,email,username,avatar?)
Store-->>UF: Success
else Not found
SVC-->>API: 404 NOT_FOUND
API-->>UF: 404
UF->>Admin: Confirm switch to manual entry
alt Admin accepts
UF->>UF: Enable manual fields + validators
else Admin cancels
UF-->>Admin: Abort
end
end
end
else Manual fields shown
Admin->>UF: Submit Manual Data (name, email, username, avatar?, role)
UF->>API: POST addUser(...manualUserInfo)
API->>SVC: updateProjectPermissions(add, username, role, manualUserInfo)
SVC->>Store: Upsert with manualUserInfo
Store-->>UF: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Implements an email‑first workflow for managing project permissions and enriches permission user records with full profile information (name, email, username, avatar). Key changes replace simple string username arrays with structured UserInfo objects and introduce NATS subjects/endpoints to resolve email → sub and fetch full user info.
- Introduce UserInfo model and migrate writers/auditors from string[] to UserInfo[]
- Add new NATS subjects (EMAIL_TO_SUB, USERNAME_TO_USER_INFO) and related service methods (email resolution + user info lookup)
- Update UI (forms + table) to support email-first lookup, manual fallback, and display avatars/name/email
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/project.interface.ts | Adds UserInfo interface and changes writers/auditors to structured arrays |
| packages/shared/src/interfaces/permissions.interface.ts | Extends permission-related request/response shapes with full user profile fields |
| packages/shared/src/enums/nats.enum.ts | Adds new NATS subjects for email→sub and username→user info lookups |
| apps/lfx-one/src/server/services/project.service.ts | Adapts permission update logic to UserInfo objects; adds email→sub resolution and user info retrieval |
| apps/lfx-one/src/server/controllers/project.controller.ts | Passes optional manual user info to service on add |
| apps/lfx-one/src/app/shared/services/permissions.service.ts | Normalizes server ProjectSettings into ProjectPermissionUser objects with full profile data |
| apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/* | Enhances table to show avatar, name, email and adds initials fallback |
| apps/lfx-one/src/app/modules/project/settings/components/user-form/* | Implements email-first add flow, manual entry confirmation, and extended form fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
94-103: Improve edge case handling in getUserInitials.The method has a few edge cases that could be handled better:
Empty/whitespace-only names: Line 95 checks
!namebut doesn't catch whitespace-only strings. Aftertrim()on line 97, a whitespace-only name becomes an empty string, leading tocharAt(0)returning an empty string instead of '?'.Multiple consecutive spaces: Line 97's
split(' ')can create empty string elements if there are multiple spaces between name parts. This could affect the "last part" logic on line 102.Apply this diff to handle edge cases:
protected getUserInitials(name: string): string { - if (!name) return '?'; + if (!name || !name.trim()) return '?'; - const parts = name.trim().split(' '); + const parts = name.trim().split(/\s+/).filter(p => p); if (parts.length === 1) { return parts[0].charAt(0).toUpperCase(); } return (parts[0].charAt(0) + parts[parts.length - 1].charAt(0)).toUpperCase(); }apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html (1)
26-36: Consider adding data-testid attributes to header columns.While the implementation is correct, consider adding
data-testidattributes to the header column cells for more reliable test targeting, following the[section]-[component]-[element]convention mentioned in the coding guidelines.Example:
<th class="w-16" data-testid="permissions-table-header-avatar"> <span class="text-sm font-sans text-gray-500">Avatar</span> </th> <th data-testid="permissions-table-header-name"> <span class="text-sm font-sans text-gray-500">Name</span> </th>As per coding guidelines.
apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.html (1)
30-76: Consider UX guidance for manual username entry.When
showManualFields()is true, both the email field (line 13, control="email") and the username field (line 54, control="username") are required. Users might be confused about what to enter for username, especially if they've already entered an email.Consider adding:
- Helper text explaining the username field (e.g., "Enter a unique username for this user")
- Auto-populating the username field with a suggestion (e.g., the local part of the email)
- Making it clear that username and email serve different purposes
📜 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 (10)
apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.html(2 hunks)apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.ts(4 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(1 hunks)apps/lfx-one/src/app/shared/services/permissions.service.ts(2 hunks)apps/lfx-one/src/server/controllers/project.controller.ts(1 hunks)apps/lfx-one/src/server/services/project.service.ts(7 hunks)packages/shared/src/enums/nats.enum.ts(1 hunks)packages/shared/src/interfaces/permissions.interface.ts(1 hunks)packages/shared/src/interfaces/project.interface.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/project.interface.tspackages/shared/src/interfaces/permissions.interface.ts
**/*.{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:
packages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/app/shared/services/permissions.service.tspackages/shared/src/enums/nats.enum.tspackages/shared/src/interfaces/permissions.interface.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/server/controllers/project.controller.tsapps/lfx-one/src/server/services/project.service.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
packages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/app/shared/services/permissions.service.tspackages/shared/src/enums/nats.enum.tspackages/shared/src/interfaces/permissions.interface.tsapps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.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.tsapps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/services/project.service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
packages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/app/shared/services/permissions.service.tspackages/shared/src/enums/nats.enum.tspackages/shared/src/interfaces/permissions.interface.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/server/controllers/project.controller.tsapps/lfx-one/src/server/services/project.service.ts
packages/shared/src/enums/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all enums in the shared package at packages/shared/src/enums
Files:
packages/shared/src/enums/nats.enum.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/user-form/user-form.component.htmlapps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html
🧬 Code graph analysis (2)
apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.ts (3)
apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.ts (1)
Component(21-170)apps/lfx-one/src/app/modules/project/settings/settings-dashboard/settings-dashboard.component.ts (1)
Component(19-76)packages/shared/src/interfaces/permissions.interface.ts (2)
UpdateUserRoleRequest(180-183)AddUserToProjectRequest(163-174)
apps/lfx-one/src/server/services/project.service.ts (1)
packages/shared/src/constants/api.constants.ts (1)
NATS_CONFIG(28-43)
⏰ 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 (13)
apps/lfx-one/src/app/shared/services/permissions.service.ts (2)
39-46: LGTM! Auditors mapping correctly extracts UserInfo fields.The mapping properly extracts name, email, username, and avatar from each UserInfo object in the auditors array, maintaining the 'view' role assignment.
52-59: LGTM! Writers mapping correctly extracts UserInfo fields.The mapping properly extracts name, email, username, and avatar from each UserInfo object in the writers array, maintaining the 'manage' role assignment.
packages/shared/src/enums/nats.enum.ts (1)
11-12: LGTM! New NATS subjects added correctly.The new enum members follow the existing naming convention and support the enhanced user info retrieval capabilities introduced in this PR.
apps/lfx-one/src/server/controllers/project.controller.ts (2)
302-302: LGTM! Updated call to include manual user info.The call to
updateProjectPermissionscorrectly passes the optionalmanualUserInfoparameter, enabling the propagation of manually entered user data.
288-300: Manual fallbacks never applied due to front-end validators
The form initializer (createUserFormGroup) already marksValidators.required, and whenshowManualFieldsis enabled it adds required validators tonameandusername. ThususerData.name,userData.email, andusernamewill always be populated on manual entries, so the empty-string defaults aren’t invoked.Likely an incorrect or invalid review comment.
packages/shared/src/interfaces/project.interface.ts (2)
120-133: LGTM! UserInfo interface well-defined.The new
UserInfointerface is well-documented and provides a clear structure for storing complete user profile data with optional avatar support.
144-147: All consumers updated for UserInfo[] change
Server and client services now filter byusernameand map object properties onwriters/auditors; no remaining string-array references found.apps/lfx-one/src/app/modules/project/settings/components/user-permissions-table/user-permissions-table.component.html (2)
41-67: LGTM! Avatar and user info columns implemented correctly.The avatar rendering with initials fallback and the display of name, username, and email are well-implemented. The conditional rendering properly handles cases where avatar may be missing.
95-95: LGTM! Empty state colspan correctly updated.The colspan value of 6 correctly matches the updated table structure with Avatar, Name, Username, Email, Role, and Actions columns.
apps/lfx-one/src/app/modules/project/settings/components/user-form/user-form.component.html (2)
7-28: LGTM! Email field properly configured with validation.The email field is well-implemented with appropriate type, validation messages, and helper text. The use of
type="email"provides built-in browser validation.
142-143: LGTM! Confirmation dialog added for user-not-found flow.The confirmation dialog component supports the new workflow for handling users not found in the directory.
packages/shared/src/interfaces/permissions.interface.ts (2)
146-153: LGTM! ProjectPermissionUser expanded with user profile fields.The interface correctly adds name, email, and optional avatar fields while maintaining backward compatibility with username and role. The required nature of name and email ensures complete user data for display.
168-173: LGTM! AddUserToProjectRequest supports manual entry workflow.The optional fields (name, email, avatar) appropriately support the manual user entry flow when a user is not found in the directory. The comment clearly explains the use case.
| // 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); | ||
| // Compare by username property since writers/auditors are now UserInfo objects | ||
| updatedSettings.writers = updatedSettings.writers.filter((u) => u.username !== username); | ||
| updatedSettings.auditors = updatedSettings.auditors.filter((u) => u.username !== username); | ||
|
|
||
| // Add user to appropriate array based on operation and role | ||
| // For 'add' or 'update', we need to add the user back with full UserInfo | ||
| if (operation === 'add' || operation === 'update') { | ||
| if (!role) { | ||
| throw new Error('Role is required for add/update operations'); | ||
| } | ||
|
|
||
| // Use manual user info if provided, otherwise fetch from NATS | ||
| let userInfo: { name: string; email: string; username: string; avatar?: string }; | ||
| if (manualUserInfo) { | ||
| req.log.info( | ||
| { | ||
| username, | ||
| operation: `${operation}_user_project_permissions`, | ||
| }, | ||
| 'Using manually provided user info' | ||
| ); | ||
| userInfo = { | ||
| name: manualUserInfo.name, | ||
| email: manualUserInfo.email, | ||
| username: manualUserInfo.username, | ||
| }; | ||
| // Only include avatar if it's provided and not empty | ||
| if (manualUserInfo.avatar) { | ||
| userInfo.avatar = manualUserInfo.avatar; | ||
| } | ||
| } else { | ||
| // Fetch user info from user service via NATS | ||
| userInfo = await this.getUserInfo(req, username); | ||
| } | ||
|
|
||
| if (role === 'manage') { | ||
| updatedSettings.writers = [...new Set([...updatedSettings.writers, username])]; | ||
| updatedSettings.writers = [...updatedSettings.writers, userInfo]; | ||
| } else { | ||
| updatedSettings.auditors = [...new Set([...updatedSettings.auditors, username])]; | ||
| updatedSettings.auditors = [...updatedSettings.auditors, userInfo]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix removal to use the canonical username before re-adding.
When the UI submits in email-first mode it still supplies the email in the username argument. Here we filter writers/auditors by that raw value before resolving it via getUserInfo. Since the stored entries use the real sub, the filter misses existing rows and the subsequent append adds a duplicate. Resolve the canonical username (from manualUserInfo.username or getUserInfo) first, then perform the removal with that value.
- // Remove user from both arrays first (for all operations)
- // Compare by username property since writers/auditors are now UserInfo objects
- updatedSettings.writers = updatedSettings.writers.filter((u) => u.username !== username);
- updatedSettings.auditors = updatedSettings.auditors.filter((u) => u.username !== username);
-
- // For 'add' or 'update', we need to add the user back with full UserInfo
- if (operation === 'add' || operation === 'update') {
+ let canonicalUsername = username;
+
+ // For 'add' or 'update', resolve the canonical username and user info
+ if (operation === 'add' || operation === 'update') {
if (!role) {
throw new Error('Role is required for add/update operations');
}
- // Use manual user info if provided, otherwise fetch from NATS
- let userInfo: { name: string; email: string; username: string; avatar?: string };
- if (manualUserInfo) {
+ // Use manual user info if provided, otherwise fetch from NATS
+ let userInfo: { name: string; email: string; username: string; avatar?: string };
+ if (manualUserInfo) {
req.log.info(
{
username,
operation: `${operation}_user_project_permissions`,
},
'Using manually provided user info'
);
userInfo = {
name: manualUserInfo.name,
email: manualUserInfo.email,
username: manualUserInfo.username,
};
+ canonicalUsername = manualUserInfo.username;
// Only include avatar if it's provided and not empty
if (manualUserInfo.avatar) {
userInfo.avatar = manualUserInfo.avatar;
}
} else {
// Fetch user info from user service via NATS
userInfo = await this.getUserInfo(req, username);
+ canonicalUsername = userInfo.username;
}
+ // Remove user from both arrays first (for all operations)
+ updatedSettings.writers = updatedSettings.writers.filter((u) => u.username !== canonicalUsername);
+ updatedSettings.auditors = updatedSettings.auditors.filter((u) => u.username !== canonicalUsername);
if (role === 'manage') {
updatedSettings.writers = [...updatedSettings.writers, userInfo];
} else {
updatedSettings.auditors = [...updatedSettings.auditors, userInfo];
}
+ } else {
+ updatedSettings.writers = updatedSettings.writers.filter((u) => u.username !== canonicalUsername);
+ updatedSettings.auditors = updatedSettings.auditors.filter((u) => u.username !== canonicalUsername);
}Committable suggestion skipped: line range outside the PR's diff.
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
dealako
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give it a spin!
…ookup
Update project permissions user form to use email-first workflow:
Changes include:
🤖 Generated with Claude Code