Enhance create user and manage user forms#1339
Enhance create user and manage user forms#1339joshunrau merged 17 commits intoDouglasNeuroInformatics:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds email and phoneNumber support to user management across the API and web applications. Changes include API DTOs and services to accept and persist these fields, web forms updated with new input fields and validation rules, a shared phone number regex utility, and corresponding translation strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/api/src/users/dto/create-user.dto.ts (1)
57-59: Duplicate@ApiProperty()decorator onsex.Line 58 re-applies
@ApiProperty()with no args, which overrides / conflicts with the descriptive one on line 57. Not introduced by this PR but adjacent to your changes — worth removing while you're here.Proposed fix
`@ApiProperty`({ description: 'Sex at Birth' }) - `@ApiProperty`() sex?: Sex;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/users/dto/create-user.dto.ts` around lines 57 - 59, The sex property in CreateUserDto has a duplicate decorator; remove the redundant `@ApiProperty`() so only the descriptive `@ApiProperty`({ description: 'Sex at Birth' }) remains on the sex?: Sex; field to avoid overriding/conflicting decorators.apps/web/src/routes/_app/admin/users/index.tsx (2)
92-100: Password/confirmPassword mismatch flagged even when both are empty-but-differ edge cases — consider gating onpasswordpresence.As written, the check fires whenever
confirmPassword !== password. If a user fills in onlyconfirmPassword(withoutpassword), they'll still get a mismatch — arguably correct, but consider also skipping when both areundefined/empty (they already match via!==→ false, so fine). The real gap: if the user setspasswordbut leavesconfirmPasswordblank, the mismatch error attaches toconfirmPasswordonly — good. Just confirm UX by gating explicitly onctx.value.passwordto avoid showing the error when neither field is touched after an accidental interaction:- if (ctx.value.confirmPassword !== ctx.value.password) { + if ((ctx.value.password || ctx.value.confirmPassword) && ctx.value.confirmPassword !== ctx.value.password) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/admin/users/index.tsx` around lines 92 - 100, The custom check on the Zod schema (.check callback) currently flags a mismatch whenever ctx.value.confirmPassword !== ctx.value.password; update the logic to only validate equality when a password is present (e.g., gate on ctx.value.password truthiness) so the mismatch error (pushed to path ['confirmPassword']) is not raised when both fields are untouched/empty — locate the .check block that inspects ctx.value.confirmPassword and ctx.value.password and add the conditional guard around the equality check.
321-332: Collapse duplicatedinitialValuesbranches.The two branches differ only in whether
additionalPermissionsis included. You can build a single object and addadditionalPermissionsconditionally.Proposed refactor
- initialValues: selectedUser?.additionalPermissions.length - ? { - additionalPermissions: selectedUser.additionalPermissions, - email: selectedUser.email ?? undefined, - groupIds: new Set(selectedUser.groupIds), - phoneNumber: selectedUser.phoneNumber ?? undefined - } - : { - email: selectedUser.email ?? undefined, - groupIds: new Set(selectedUser.groupIds), - phoneNumber: selectedUser.phoneNumber ?? undefined - } + initialValues: { + email: selectedUser.email ?? undefined, + groupIds: new Set(selectedUser.groupIds), + phoneNumber: selectedUser.phoneNumber ?? undefined, + ...(selectedUser.additionalPermissions.length + ? { additionalPermissions: selectedUser.additionalPermissions } + : {}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/admin/users/index.tsx` around lines 321 - 332, Collapse the duplicated initialValues branches by constructing a single base object for initialValues (include email: selectedUser.email ?? undefined, groupIds: new Set(selectedUser.groupIds), phoneNumber: selectedUser.phoneNumber ?? undefined) and then conditionally add additionalPermissions when selectedUser?.additionalPermissions.length is truthy (e.g., if (selectedUser?.additionalPermissions?.length) base.initialValues.additionalPermissions = selectedUser.additionalPermissions). Update the initialValues assignment to use this single object; reference the initialValues assignment, selectedUser, and additionalPermissions to locate the code.apps/web/src/utils/validation.ts (1)
1-1: Drop the redundantnew RegExp(...)wrapper.Wrapping a regex literal in
new RegExp(...)is a no-op — just export the literal directly.Proposed simplification
-export const PHONE_REGEX = new RegExp(/^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); +export const PHONE_REGEX = /^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/;Side note: the pattern accepts fairly loose inputs (e.g.,
1is 3 groups of\d{1,N}all optional-separated, but first group is required; strings as short as 2 digits pass). If you want stricter E.164-ish validation, consider a library likelibphonenumber-js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/utils/validation.ts` at line 1, The PHONE_REGEX export wraps a regex literal in new RegExp(...) which is redundant; replace the wrapped expression by exporting the regex literal directly (replace export const PHONE_REGEX = new RegExp(/^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); with a direct literal assignment to PHONE_REGEX) and, if desired, consider tightening the pattern or using libphonenumber-js for stricter E.164-style validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/users/dto/create-user.dto.ts`:
- Around line 35-36: The $CreateUserData schema currently validates email with
z.email() but leaves phoneNumber as z.string(); update the phoneNumber validator
in the $CreateUserData schema (and the other instance noted) to enforce the same
PHONE_REGEX used by the UI (e.g. z.string().regex(PHONE_REGEX, "Invalid phone
number") or a z.string().refine(...) using PHONE_REGEX) and import/reuse the
PHONE_REGEX constant where it’s defined so server-side validation matches the UI
pattern for phone numbers (refer to the phoneNumber field and $CreateUserData
symbol).
In `@apps/web/src/translations/common.json`:
- Around line 50-53: Fix the French translation typos and capitalization in
common.json: change the "email" object's fr value from "Addresse courriel" to
"Adresse courriel" (or "Courriel" if you prefer the shorter label) and update
the French label whose current value is "numéro de téléphone" to use
sentence-case capitalization "Numéro de téléphone" so it matches other labels
like "Mot de passe" and "Nom d'utilisateur".
---
Nitpick comments:
In `@apps/api/src/users/dto/create-user.dto.ts`:
- Around line 57-59: The sex property in CreateUserDto has a duplicate
decorator; remove the redundant `@ApiProperty`() so only the descriptive
`@ApiProperty`({ description: 'Sex at Birth' }) remains on the sex?: Sex; field to
avoid overriding/conflicting decorators.
In `@apps/web/src/routes/_app/admin/users/index.tsx`:
- Around line 92-100: The custom check on the Zod schema (.check callback)
currently flags a mismatch whenever ctx.value.confirmPassword !==
ctx.value.password; update the logic to only validate equality when a password
is present (e.g., gate on ctx.value.password truthiness) so the mismatch error
(pushed to path ['confirmPassword']) is not raised when both fields are
untouched/empty — locate the .check block that inspects
ctx.value.confirmPassword and ctx.value.password and add the conditional guard
around the equality check.
- Around line 321-332: Collapse the duplicated initialValues branches by
constructing a single base object for initialValues (include email:
selectedUser.email ?? undefined, groupIds: new Set(selectedUser.groupIds),
phoneNumber: selectedUser.phoneNumber ?? undefined) and then conditionally add
additionalPermissions when selectedUser?.additionalPermissions.length is truthy
(e.g., if (selectedUser?.additionalPermissions?.length)
base.initialValues.additionalPermissions = selectedUser.additionalPermissions).
Update the initialValues assignment to use this single object; reference the
initialValues assignment, selectedUser, and additionalPermissions to locate the
code.
In `@apps/web/src/utils/validation.ts`:
- Line 1: The PHONE_REGEX export wraps a regex literal in new RegExp(...) which
is redundant; replace the wrapped expression by exporting the regex literal
directly (replace export const PHONE_REGEX = new
RegExp(/^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); with a direct literal
assignment to PHONE_REGEX) and, if desired, consider tightening the pattern or
using libphonenumber-js for stricter E.164-style validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89f02106-872f-4a6e-9c8a-52d95d392b08
📒 Files selected for processing (7)
apps/api/src/users/dto/create-user.dto.tsapps/api/src/users/users.service.tsapps/web/src/routes/_app/admin/users/create.tsxapps/web/src/routes/_app/admin/users/index.tsxapps/web/src/routes/_app/user.tsxapps/web/src/translations/common.jsonapps/web/src/utils/validation.ts
2 participants
The pr adds the following:
Added contact information section to create user form:
phone number
email
Added phone number and email to manage user form.
Added confirm password field and check into manage user form
Added initial values for phone, email and groups into the manage user form
refactored phone number regex to a variable exported from utils directory.
works on issue #1294
code reviewed with claude code
model: Claude Sonnet 4.6
Summary by CodeRabbit
New Features
Documentation