feat(settings): Add code resend for secondary email add #19657
Conversation
nshirley
left a comment
nshirley
left a comment
There was a problem hiding this comment.
Sorry it took a bit longer to finish review! I wanted to do testing on the branch
Code looks good (as far as I can tell with React code 😅) and I was able to test happy paths without problem:
- resend code button works
- debounce works and prevents spamming resend
- and submitting with the new code works!
One thing I noticed, and maybe it's a result of local but whenever I requested a new code, it is the same as the original code that was sent. No matter how many times I requested a new code it was always the same. Probably not a blocker but wanted to flag
And, also likely not a blocker but wanted to flag. If the MFA window expires while on the confirm code page, and you click "resend code" with an expired JWT, it just kicks back to the settings page. Interestingly, I didn't see any network or console errors
I'll add r+ for now, I don't think those would be blocking but happy to re-review quickly once those are fixed if you want!
| await settings.secondaryEmail.addButton.click(); | ||
| await secondaryEmail.fillOutEmail(newEmail); | ||
| // Click resend to ensure we test re-send path | ||
| await secondaryEmail.clickResendConfirmationCode(); |
There was a problem hiding this comment.
It might be worth getting the first code in the inbox before clicking resend.
My thinking is, the test could move fast enough that it gets to the point of getVerifySecondaryCode() and there's already an existing code in the inbox, and that gets used. But if you await/fetch the first code, it'll clear the inbox so you can then request a new code and know it's the secondary code
| : String(sessionToken.uid); | ||
|
|
||
| // If there is no reservation or it belongs to another user, throw. | ||
| if (!parsedRecord || parsedRecord.uid !== uidStr) { |
There was a problem hiding this comment.
Good thinking making sure it belongs to the user!
vpomerleau
force-pushed
the
FXA-12649
branch
from
84e041c to
d1c70cc
Compare
Because: * We want to allow a resend option directly on the verification page * The resend option on settings will no longer be available on the settings page (unconfirmed emails no longer stored in db and there not displayed in settings) * We want the resend to be authenticated with the email scoped JWT token This commit: * Add button to resend confirmation code in PageSecondaryEmailVerify * Adds success/error banner for code resend * Include cool-off to prevent successive clicks, including disabled state * Add unit tests, l10n, storybook states * Add mfa-authed secondary email code resend endpoint that relies on redis, not unconfirmed email in db, and returns an error if no valid reservation is found * Add route unit tests for the new resend endpoint * Add functional test for resend Closes #FXA-12649
vpomerleau
force-pushed
the
FXA-12649
branch
from
d1c70cc to
791abab
Compare
|
Thanks for your review @nshirley! I think I've sorted out the issue with the expired jwt token (the mfa guard was getting dismissed automatically). The other finding you mentioned about the same code being resent is intentional - this is to ensure that if the emails are delayed the user doesn't have to figure out which emailed code is correct (which could lead to frustration and rate limiting). Also great catch about the test - I've updated it! |
nshirley
left a comment
nshirley
left a comment
There was a problem hiding this comment.
Tested the edge case that you fixed and it's working meow! lgtm!
Because
This pull request
Issue that this pull request solves
Closes: FXA-12649
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
This is a follow-up to moving the secondary email add to Redis reservations.