Fix null-safe email validation for legacy users#404
Fix null-safe email validation for legacy users#404
Conversation
overheadhunter
left a comment
overheadhunter
left a comment
There was a problem hiding this comment.
fix optional types (same for initialEmail)
frontend/src/common/backend.ts
Outdated
| name: string; | ||
| pictureUrl?: string; | ||
| email: string; | ||
| email?: string | null; |
There was a problem hiding this comment.
email?: string already implies string | undefined. Prefer undefined over null
frontend/src/common/formvalidator.ts
Outdated
| * Validates email format | ||
| */ | ||
| static isValidEmail(email: string): boolean { | ||
| static isValidEmail(email: string | null | undefined): boolean { |
There was a problem hiding this comment.
same as above: avoid null, no need for | undefined when using ?
| static isValidEmail(email: string | null | undefined): boolean { | |
| static isValidEmail(email?: string): boolean { |
WalkthroughUser email in the backend DTO (UserDto.user.email) was changed to optional. FormValidator.validateUser now accepts an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🧹 Nitpick comments (2)
frontend/src/common/backend.ts (1)
90-90:CreateUserDto.emailbecomes optional, breaking the type-level creation contract.Making
UserDto.emailoptional propagates intoCreateUserDtoviaPick, making email optional there too. The PR explicitly states email must remain required for new users, butbackend.users.createUser({ ...withoutEmail })now compiles without error. The guarantee is only upheld by UI-layer validation, not the type system.Consider overriding
CreateUserDto:♻️ Proposed fix
-export type CreateUserDto = Pick<UserDto, 'name' | 'email' | 'firstName' | 'lastName' | 'pictureUrl' | 'realmRoles'> & { - password: string; -}; +export type CreateUserDto = Pick<UserDto, 'name' | 'firstName' | 'lastName' | 'pictureUrl' | 'realmRoles'> & { + email: string; + password: string; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/common/backend.ts` at line 90, UserDto made email optional accidentally makes CreateUserDto (which is derived via Pick<UserDto, ...>) allow missing email; change CreateUserDto to explicitly require email again by overriding the inherited optionality—locate the CreateUserDto type and replace the pure Pick<UserDto, ...> usage with a construction that forces email required (for example by intersecting or re-mapping the type to include { email: string } or using Required<Pick<...,'email'>> merged with the other picked fields) so that CreateUserDto.email is statically required while preserving the rest of the picked UserDto fields.frontend/src/components/authority/UserList.vue (1)
83-83:user.email ?? undefinedis a no-op forstring | undefined.Since
UserDto.emailis now typed asstring | undefined, the?? undefinedguard has no effect — nullish coalescing only replacesnullorundefinedwith the RHS, so for a value already typed asstring | undefined, this is always equivalent touser.email. The expression can be simplified to justuser.email.♻️ Suggested simplification
- <span class="text-xs text-gray-500 truncate" :title="user.firstName || user.lastName ? `${user.firstName ?? ''} ${user.lastName ?? ''}`.trim() : user.email ?? undefined"> + <span class="text-xs text-gray-500 truncate" :title="user.firstName || user.lastName ? `${user.firstName ?? ''} ${user.lastName ?? ''}`.trim() : user.email">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/authority/UserList.vue` at line 83, The template expression unnecessarily uses "user.email ?? undefined" which is a no-op given UserDto.email is already string | undefined; in the span (the title and displayed text) remove the "?? undefined" and just use "user.email" and keep the rest of the ternary logic that builds `${user.firstName ?? ''} ${user.lastName ?? ''}`.trim() so the span's title and content use user.firstName/user.lastName fallback to user.email without the redundant nullish coalescing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/authority/UserEditCreate.vue`:
- Line 398: The current assignment data.email = data.email?.trim() ?? '' forces
undefined legacy emails to become an empty string and thus gets serialized into
UpdateUserDto; change the assignment so that after trimming an empty string
collapses to undefined (use || logic) — e.g., in the UserEditCreate component
replace the nullish-coalescing fallback with a fallback that maps empty-string
to undefined so data.email is undefined for missing emails and will be omitted
from the JSON payload.
---
Nitpick comments:
In `@frontend/src/common/backend.ts`:
- Line 90: UserDto made email optional accidentally makes CreateUserDto (which
is derived via Pick<UserDto, ...>) allow missing email; change CreateUserDto to
explicitly require email again by overriding the inherited optionality—locate
the CreateUserDto type and replace the pure Pick<UserDto, ...> usage with a
construction that forces email required (for example by intersecting or
re-mapping the type to include { email: string } or using
Required<Pick<...,'email'>> merged with the other picked fields) so that
CreateUserDto.email is statically required while preserving the rest of the
picked UserDto fields.
In `@frontend/src/components/authority/UserList.vue`:
- Line 83: The template expression unnecessarily uses "user.email ?? undefined"
which is a no-op given UserDto.email is already string | undefined; in the span
(the title and displayed text) remove the "?? undefined" and just use
"user.email" and keep the rest of the ternary logic that builds
`${user.firstName ?? ''} ${user.lastName ?? ''}`.trim() so the span's title and
content use user.firstName/user.lastName fallback to user.email without the
redundant nullish coalescing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/authority/UserEditCreate.vue`:
- Around line 94-96: The inline validation shows "invalid email" for inputs with
surrounding whitespace because the guard uses data.email?.trim() but
isValidEmail receives the untrimmed data.email; update the conditional to call
isValidEmail on the trimmed value (use isValidEmail(data.email.trim())) so the
validator receives a trimmed string — this is safe because the truthy guard
(data.email?.trim()) guarantees data.email is a non-null string; change the call
where the template uses isValidEmail to pass the trimmed email in the
UserEditCreate.vue template.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/authority/UserEditCreate.vue`:
- Line 398: The current trimming expression in UserEditCreate.vue (data.email =
data.email?.trim() || undefined) is correct and should be left as-is: it
preserves undefined, converts whitespace-only strings to undefined, and trims
valid emails before sending to the backend; do not change this line in the data
handling for the UserEditCreate component.
| /** | ||
| * Checks if email is required (always for CREATE, only if previously set for EDIT) | ||
| */ | ||
| static isEmailRequired(isEditMode: boolean, initialEmail?: string): boolean { | ||
| if (!isEditMode) return true; | ||
| return !!initialEmail?.trim(); | ||
| } | ||
|
|
There was a problem hiding this comment.
legibility for humans is more important than compact code (js will be preprocessed during build anyway)
| /** | |
| * Checks if email is required (always for CREATE, only if previously set for EDIT) | |
| */ | |
| static isEmailRequired(isEditMode: boolean, initialEmail?: string): boolean { | |
| if (!isEditMode) return true; | |
| return !!initialEmail?.trim(); | |
| } | |
| /** | |
| * Checks if email is required | |
| */ | |
| static isEmailRequired(isEditMode: boolean, initialEmail?: string): boolean { | |
| return isEditMode | |
| ? initialEmail?.trim() !== '' // required if previously set | |
| : true; // always required in CREATE mode | |
| } | |
| data.lastName = data.lastName?.trim(); | ||
| data.name = data.name.trim(); | ||
| data.email = data.email.trim(); | ||
| data.email = data.email?.trim() || undefined; |
There was a problem hiding this comment.
this is already optional
| data.email = data.email?.trim() || undefined; | |
| data.email = data.email?.trim(); |
2 participants
Email validation failed when editing users migrated from legacy systems with empty email. This fix makes email handling null-safe and keeps email required for new users and those with existing email, while making it optional for legacy users without one.