Conversation
dschom
force-pushed
the
FXA-12390
branch
3 times, most recently
from
0cc4527 to
b1560c2
Compare
MagentaManifold
left a comment
MagentaManifold
left a comment
There was a problem hiding this comment.
I manually tested and it works. Thanks Dan!
vpomerleau
left a comment
vpomerleau
left a comment
There was a problem hiding this comment.
Thanks for the improvements. Tested locally and working well to prevent resend on reload. Just a few non-blocking comments.
| ); | ||
|
|
||
| await act(async () => { | ||
| await new Promise((r) => setTimeout(r, 101)); |
There was a problem hiding this comment.
Could we consider using Jest's useFakeTimers instead of setTimeout? It's a drop in the bucket, but likely better practice for runtime on CI.
There was a problem hiding this comment.
I think that's a good recommendation. I actually tried using useFakeTimers but it didn't work... but it wasn't apparent why. I can try again, but will land that in a polish PR.
| * The main purpose of this cache is to track when we email users | ||
| * MFA OTP codes. This info can then be used to debounce sends. | ||
| */ | ||
| export class MfaOtpRequestCache { |
There was a problem hiding this comment.
Observation: the cache for tokens and requests doesn't get cleared on account signout. Likely not a big deal since it can't be reused if the session token is no longer valid?
There was a problem hiding this comment.
Excellent point. I'll clear these caches on sign out. I'll do this in a separate PR though.
|
|
||
| const handleResendCode = async () => { | ||
| // Stop users from hammering the resend button... | ||
| if (debounce(debounceIntervalMs)) { |
There was a problem hiding this comment.
The interval is currently pretty small, so it's not too much of a concern rn, but if we were to increase the wait time for resends we should provide clear feedback in the UI (disable the button or return an info/error message). This would be less confusing for the user.
There was a problem hiding this comment.
I was also considering this. I was doing a little research on this and it seemed like if the interval was small it's alright to just debounce quietly.
There was a problem hiding this comment.
Yeah, agree that with the current very short timeout it's not much of a problem.
| }); | ||
| }); | ||
|
|
||
| it('debounces OPT resend requests', async () => { |
There was a problem hiding this comment.
typo
| it('debounces OPT resend requests', async () => { | |
| it('debounces OTP resend requests', async () => { |
Because: - Due to mobile issue the previous debounce technique might still have issues This Commit: - Caches record of when MFA OTP code requests occur - Uses record to determine whether or not to debounce - Also handles case where some one hammers the 'resend' button
3 participants
Because
This pull request
Issue that this pull request solves
Closes: FXA-12390
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)
Any other information that is important to this pull request.