fix: to update isEmailVerified while updating email #10707
Conversation
iamhks
changed the title
| // Part 2: Update the last element of identities array | ||
| if (identitiesArray && identitiesArray.length > 0) { | ||
| const lastIndex = identitiesArray.length - 1 | ||
| const lastElement = identitiesArray[lastIndex] as JsonObject; | ||
| lastElement!.isEmailVerified = false | ||
| } else { | ||
| throw new Error('Empty Identities array!') | ||
| } | ||
|
|
||
| // Part 3: Update the identities in the database | ||
| await pg | ||
| .updateTable('User') | ||
| .set({ | ||
| identities: identitiesArray, | ||
| }) | ||
| .where('email', '=', oldEmail) | ||
| .execute() |
There was a problem hiding this comment.
-1 instead of having a separate update call, we should update the identities array in the updateTable('User') call further below. We should also find the identity matchin AuthIdentityLocal and update that one explicitly.
There was a problem hiding this comment.
Sure, updated the PR. Please have a second look.
| throw new Error('No LOCAL identity found!') | ||
| } | ||
| } else { | ||
| throw new Error('Empty Identities array!') |
There was a problem hiding this comment.
we can't throw here because SSO signups won't have an identity #9202.
| const identitiesArray = user.identities | ||
|
|
||
| // Update the identities array for AuthIdentityLocal, isEmailVerified=false | ||
| if (identitiesArray && identitiesArray.length > 0) { |
There was a problem hiding this comment.
TBH this mutation should be rewritten if we start dipping into identities. There's an assumption that the identity stays the same, only the email changes, which we can only verify by talking to the user. This mutation should work for SSO or google identities, too, which this new code will break.
I'd suggest we hold off for now as this work isn't eating our lunch, but breaking current functionality could impact our customer service folks. In a better future we'll just enforce email verification upon signup so we can reduce the number of spam account & do away with the flag all together
There was a problem hiding this comment.
Alternatively, we could also remove just the else part of the code, throwing no error.
This would not break the existing functionality and keep working for LOCAL identities.
As and when SSO identities are implemented, this mutation can also be updated.
There was a problem hiding this comment.
totally down for that! as long as API doesn't break current functionality I'm happy
There was a problem hiding this comment.
Great! I have update the PR accordingly, tested the same; works for both Google Auth and Local.
| if (!user) { | ||
| throw new Error(`User with ${oldEmail} not found`) | ||
| } | ||
|
|
||
| // RESOLUTION | ||
| //Use identities array for the user above |
There was a problem hiding this comment.
-1 Please remove. The comment states the obvious and just adds clutter
| if (!user) { | ||
| throw new Error(`User with ${oldEmail} not found`) | ||
| } | ||
|
|
||
| // RESOLUTION | ||
| //Use identities array for the user above | ||
| const identitiesArray = user.identities |
There was a problem hiding this comment.
+1 In our code base we usually destructure fields. Also the Array suffix is not necessary as it's typed and identities is already plural. So we would move l35 up here and it would look like
const {id: userId, identities} = user|
|
||
| // Update the identities array for AuthIdentityLocal, isEmailVerified=false | ||
| if (identitiesArray && identitiesArray.length > 0) { | ||
| const localIdentity = (identitiesArray as JsonObject[]).find((identity: JsonObject) => identity.type === 'LOCAL') |
There was a problem hiding this comment.
+1 typing in the find is not necessary as you've cast already the array itself.
| //Use identities array for the user above | ||
| const identitiesArray = user.identities | ||
|
|
||
| // Update the identities array for AuthIdentityLocal, isEmailVerified=false |
There was a problem hiding this comment.
+1 There is no point in repeating the logic in a comment. It's ok to have a block comment for structuring, or even better to state the intent of the change, so something like
// unset isEmailVerified
or
// changing the email means it's no longer verified
would be better, but if in doubt, omit the comment
Description
Fixes #8990
Made changes to fetch the identities array and update isEmailVerified for the old email
Demo
Testing scenarios
Scenario A: In case of multiple values in identities array
Scenario B: Confusion w.r.t "changed email mentioned in bug description"
Final checklist