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.

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

Stop double-submit of set password form #9445

Merged
merged 2 commits into from
Jun 16, 2022
Merged

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Jun 14, 2022

Details

When pressing enter, the form was being submit twice. Once for the text input and one for the submit button. The multiple submits was causing validateAndSubmitForm() to be ran multiple times, and the second time it ran, it would have this.props.userSignUp.authToken from the first request. This caused changePasswordAndSignIn() to be triggered from an invalid state and put the UI into a position where it couldn't fetch all the reports. Since the reports couldn't fully load, it was stuck at a loading spinner.

Fixed Issues

$ #9058

Tests

For local tests, you'll need to be able to get the validate code for a new account.

  1. Launch NewDot and make sure you're signed out
  2. Sign in with a new email address to create a new account
  3. From the CLI run ../script/sql.sh
  4. Run the query SELECT FROM notifications ORDER BY created DESC LIMIT 0,1;
  5. Look for the accountID and validateCode
  6. Go back to NewDot and modify the URL to go to http://localhost:8080/setpassword/<accountID>/<validateCode> (use the values from the query)
  7. Enter a valid password and press the ENTER key
  8. Verify that you are taken into the app and everything loads
  9. Repeat the entire process with another new user, this time, instead of pressing the ENTER key on step 7, click the submit button
  • Verify that no errors appear in the JS console

QA Steps

  1. Go to URL https://staging.new.expensify.com/
  2. In the email field, enter a email address for a new account and press continue
  3. Navigate to the email inbox of the account and open the validation email
  4. Copy the link and change it to staging
  5. Set a password that follows the password requirements
  6. Submit the form by pressing the ENTER key
  7. Verify the app loads everything properly
  8. Repeat all the steps but for step 6, click the submit button on the form
  • Verify that no errors appear in the JS console

Screenshots

This but only effects web because that is the only way to validate an account.

Web

2022-06-14_09-53-11.mp4
2022-06-14_09-37-19.mp4

Mobile Web

Desktop

iOS

Android

@tgolen tgolen self-assigned this Jun 14, 2022
@tgolen tgolen marked this pull request as ready for review June 14, 2022 13:54
@tgolen tgolen requested a review from a team as a code owner June 14, 2022 13:54
@melvin-bot melvin-bot bot requested review from aldo-expensify and removed request for a team June 14, 2022 13:54
@@ -126,7 +126,6 @@ class SetPasswordPage extends Component {
password={this.state.password}
updatePassword={password => this.setState({password})}
updateIsFormValid={isValid => this.setState({isFormValid: isValid})}
onSubmitEditing={this.validateAndSubmitForm}
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up looks like this is a required prop and will be called here

onSubmitEditing: PropTypes.func.isRequired,

onSubmitEditing={() => this.props.onSubmitEditing()}

https://reactnative.dev/docs/textinput#onsubmitediting (had to look this up wasn't quite sure what it was for)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good to know. I'll put a no-op instead then.

@tgolen
Copy link
Contributor Author

tgolen commented Jun 14, 2022

Updated

@aldo-expensify aldo-expensify merged commit d2fb1c1 into main Jun 16, 2022
@aldo-expensify aldo-expensify deleted the tgolen-fix-setpassword branch June 16, 2022 15:26
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@aldo-expensify
Copy link
Contributor

@tgolen I just noticed that this PR included changes on the package-lock.json, do we want to reverse?

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @aldo-expensify in version: 1.1.78-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@tgolen
Copy link
Contributor Author

tgolen commented Jun 17, 2022

@aldo-expensify ah, phooey. That wasn't intended to get through. I'm sure it's fine (for the sake of production and for NPM packages), but I'm not sure what havok this will cause to other devs. Once we are all on M1s it will probably self-resolve. I think it's OK to lave as-is for now though.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @sketchydroide in version: 1.1.78-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants