-
Notifications
You must be signed in to change notification settings - Fork 183
FIX: Inconsistency in User Verification Status between Email and Phone Number & Password Field Not Reset On Email Update #2115
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
base: main
Are you sure you want to change the base?
Conversation
227da36
to
5f461ee
Compare
@stnguyen90 @ItzNotABug PR review request! |
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
This PR corrects user verification alert messages for email and phone statuses and ensures the password field is cleared after updating the email.
- Fixes inverted logic and message formatting for email and phone verification alerts
- Removes “The account” fallback in notifications and applies consistent phrasing
- Resets
emailPassword
tonull
after a successful email update
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
src/routes/(console)/project-[region]-[project]/auth/user-[user]/updateStatus.svelte | Updated notification templates for email and phone verification |
src/routes/(console)/account/updateEmail.svelte | Set emailPassword to null after successfully updating email |
addNotification({ | ||
message: `${$user.name || $user.email || $user.phone || 'The account'} has been ${ | ||
!$user.emailVerification ? 'unverified' : 'verified' | ||
message: `The email${$user.name && ` for ${$user.name}`} ${ |
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.
Using $user.name &&
inside a template literal can yield undefined
if name
is falsy. Consider using a ternary: ${$user.name ?
for ${$user.name} : ''}
to avoid inserting “undefined.”
message: `The email${$user.name && ` for ${$user.name}`} ${ | |
message: `The email${$user.name ? ` for ${$user.name}` : ''} ${ |
Copilot uses AI. Check for mistakes.
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.
It's an empty string by default, so not needed
addNotification({ | ||
message: `${$user.name || $user.email || $user.phone || 'The account'} has been ${ | ||
$user.phoneVerification ? 'unverified' : 'verified' | ||
message: `The phone ${$user[$user.name ? 'name' : 'email'] && `for ${$user[$user.name ? 'name' : 'email']}`}${ |
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.
The dynamic property guard here may insert undefined
if both name
and email
are missing. Use a nested ternary to explicitly handle each case, e.g., ${$user.name ?
for ${$user.name}: $user.email ?
for ${$user.email} : ''}
.
message: `The phone ${$user[$user.name ? 'name' : 'email'] && `for ${$user[$user.name ? 'name' : 'email']}`}${ | |
message: `The phone${$user.name ? ` for ${$user.name}` : $user.email ? ` for ${$user.email}` : ''}${ |
Copilot uses AI. Check for mistakes.
message: `${$user.name || $user.email || $user.phone || 'The account'} has been ${ | ||
!$user.emailVerification ? 'unverified' : 'verified' | ||
message: `The email${$user.name && ` for ${$user.name}`} ${ | ||
!$user.emailVerification ? 'is no longer verified' : ' has been verified' |
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.
[nitpick] The leading space on the 'has been verified' branch combined with the preceding template space can result in double spaces. Move spacing outside the conditional or remove the extra leading space for consistency.
!$user.emailVerification ? 'is no longer verified' : ' has been verified' | |
!$user.emailVerification ? 'is no longer verified' : 'has been verified' |
Copilot uses AI. Check for mistakes.
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.
Ooops, small leading space there. I think this is not intentional, right? @Joyal-George-KJ
message: `The phone ${$user[$user.name ? 'name' : 'email'] && `for ${$user[$user.name ? 'name' : 'email']}`}${ | ||
!$user.phoneVerification ? ' is no longer verified' : ' has been verified' |
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.
[nitpick] Similarly here, both branches include a leading space—consider unifying whitespace handling outside the conditional to prevent inconsistent spacing in the final message.
message: `The phone ${$user[$user.name ? 'name' : 'email'] && `for ${$user[$user.name ? 'name' : 'email']}`}${ | |
!$user.phoneVerification ? ' is no longer verified' : ' has been verified' | |
message: `The phone ${$user[$user.name ? 'name' : 'email'] && `for ${$user[$user.name ? 'name' : 'email']}`} ${ | |
!$user.phoneVerification ? 'is no longer verified' : 'has been verified' |
Copilot uses AI. Check for mistakes.
message: `The email${$user.name && ` for ${$user.name}`} ${ | ||
!$user.emailVerification ? 'is no longer verified' : ' has been verified' |
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.
this can be simplified for readability.
addNotification({ | ||
message: `${$user.name || $user.email || $user.phone || 'The account'} has been ${ | ||
$user.phoneVerification ? 'unverified' : 'verified' | ||
message: `The phone ${$user[$user.name ? 'name' : 'email'] && `for ${$user[$user.name ? 'name' : 'email']}`}${ | ||
!$user.phoneVerification ? ' is no longer verified' : ' has been verified' |
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.
same here.
The preview deployment is ready. 🟢 Open Preview | Open Build Logs Last updated at: 2025-07-14 17:17:18 CET |
The preview deployment is ready. 🟢 Open Preview | Open Build Logs Last updated at: 2025-07-14 17:17:17 CET |
@ItzNotABug and @DH-555: The changes has been made.
|
What does this PR do?
Password Field Not Reset On Email Update
Issue 🐛 Bug Report: Password Field Not Reset On Email Update #976Changes:
Issue
Inconsistency in User Verification Status between Email and Phone Number
🐛 Bug Report: Inconsistency in User Verification Status between Email and Phone Number #1392:!
to user verification check condition$user.phoneVerification ? 'unverified' : 'verified'
changed to!$user.phoneVerification ? 'unverified' : 'verified'
.The Account
from both phone and email verification because the condition is not valid. Condition says that if account has no username, email and phone user useThe account
instead of usingname
,email
orphone
. Which means its an anonymous account that doesn't need verification.XYZ's email has been verified.
XYZ's email has been unverified.
XYZ's phone has been verified.
XYZ's phone has been unverified.
Issue
Password Field Not Reset On Email Update
🐛 Bug Report: Password Field Not Reset On Email Update #976:emailPassword to null
after email updated successfully.Test Plan
Related PRs and Issues
Addressing Issue #976 : There was this PR #1136 but it had conflicts and wasn't resolved till now (1 month has been passed)
Have you read the Contributing Guidelines on issues?