Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(fxa-settings): Disable double-clicking on buttons in account recovery key flow #15429

Merged
merged 1 commit into from
Jun 27, 2023
Merged

fix(fxa-settings): Disable double-clicking on buttons in account recovery key flow #15429

merged 1 commit into from
Jun 27, 2023

Conversation

vpomerleau
Copy link
Contributor

Because

  • We want to prevent users from clicking on submit buttons multiple times while account updates are in progress, and also provide users with feedback that the form has been submitted.
  • Clicking on the confirm password button multiple times would result in an error banner "Account recovery key already exists"

This pull request

  • Disable submit button when account is in loading state.

Issue that this pull request solves

Closes: #FXA-7551

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Screen.Recording.2023-06-08.at.10.37.34.AM.mov

Other information (Optional)

Any other information that is important to this pull request.

@vpomerleau vpomerleau requested a review from a team as a code owner June 8, 2023 17:39
LZoog
LZoog approved these changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to want a similar check when deleting a recovery key. I just tried in production clicking "remove" as many times as I could, and I got like 6 emails telling me I'd deleted my key. 😅 Apparently that's been an existing issue and your fix here does resolve the issue described so it's fine to do in a follow up if you'd prefer but if you've got time it'd be nice to sneak it in here since it's all related.

@vpomerleau
Copy link
Contributor Author

We are going to want a similar check when deleting a recovery key.

Yessssss good catch! I just pushed up a fix for this modal too.

@LZoog
Copy link
Contributor

LZoog commented Jun 13, 2023

Yessssss good catch! I just pushed up a fix for this modal too.

Did you try this locally? I just did and I'm able to click it at least twice 😩 I logged account.loading and don't see when it goes true. Our withLoadingStatus is a little confusing to me, we're pretty inconsistent with using it and I know I've tried using it in a couple places where I thought I'd be able to use it but couldn't, and I don't see that we've documented anywhere.

@vpomerleau
Copy link
Contributor Author

vpomerleau commented Jun 15, 2023 •
edited
Loading

Did you try this locally? I just did and I'm able to click it at least twice 😩

I did try it locally, and the requests only went through once. Not sure if there was a delay on the button being visually disabled though. Did you see requests go though more than once, or received multiple emails after clicking?

Re-tested with a slowed connection and did indeed see requests go through more than once. I've pushed a revised loading state that disables the button as long as the submit function as a whole is in progress.

@vpomerleau vpomerleau force-pushed the FXA-7551 branch 3 times, most recently from 8ac4476 to 5d85e6b Compare June 16, 2023 23:28
LZoog
LZoog approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@LZoog LZoog left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+wc😎 Fixes the issue in the ticket. (It's fine with me if you want to scoot the modal issue out elsewhere if you aren't sure about my question.)

packages/fxa-settings/src/components/Settings/FlowRecoveryKeyHint/index.tsx Outdated
@@ -84,6 +85,7 @@ export const FlowRecoveryKeyHint = ({
};

const onSubmit = async ({ hint }: FormData) => {
await setIsLoading(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need this await.

packages/fxa-settings/src/components/Settings/FlowRecoveryKeyHint/index.tsx Outdated
@@ -194,6 +198,7 @@ export const FlowRecoveryKeyHint = ({
<button
className="cta-primary cta-xl w-full mt-6 mb-4"
type="submit"
disabled={account.loading}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this check now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching! This should be the isLoading state.

packages/fxa-settings/src/components/Settings/Modal/index.tsx Outdated
@@ -119,7 +120,11 @@ export const Modal = ({
<button
className={classNames('mx-2 flex-1', confirmBtnClassName)}
data-testid="modal-confirm"
onClick={(event) => onConfirm()}
onClick={(event) => {
setIsLoading(true);
Copy link
Contributor

@LZoog LZoog Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does every instance of us using a modal across the codebase close after this function completes? Asking since setIsLoading(false) isn't called here so I'd want to double check that doesn't cause any problems 🤔 This modal is used in SubPlat too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add the loading state as an optional prop and control the loading state from the onConfirm function that's passed in from UnitRowRecoveryKey.

@vpomerleau
fix(fxa-settings): Disable double-clicking on buttons in account reco…
9eac595 
…very key flow

Because:

* We want to prevent users from clicking on submit buttons multiple times while account updates are in progress, and also provide users with feedback that the form has been submitted.
* Clicking on the confirm password button multiple times would result in an error banner "Account recovery key already exists"
* Clicking multiple times on the "remove" button from the "delete account recovery key" modal would send multiple key deleted emails

This commit:

* Disable submit button while password confirmation and key creation are in progress
* Disable modal confirmation button while submit function is in progress

Closes #FXA-7551
LZoog
LZoog approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with newest push! Thanks for sticking with it.

@vpomerleau vpomerleau merged commit 47375b4 into main Jun 27, 2023
@vpomerleau vpomerleau deleted the FXA-7551 branch June 27, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LZoog LZoog LZoog approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vpomerleau @LZoog