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

Fixed Confirm Password Validation in "Change Password" screen #4409

Merged

Conversation

mananjadhav
Copy link
Collaborator

@mananjadhav mananjadhav commented Aug 4, 2021

@AndrewGable can you please review this?

Details

  • Added onBlur functions to compare the newPassword and confirmPassword fields.
  • shouldShowPasswordConfirmError flag to hide/show the validation message error.

Fixed Issues

$ #4367

Tests

  1. Tested error scenario where both the passwords don't match
  2. Tested scenario where both the passwords match
  3. Tested updating newPassword after updating comparePassword

QA Steps

  1. Enter different passwords for the newPassword field field and confirmPassword field.
  2. Enter the confirm password first and then a new password to confirm the error is shown
  3. Error should automatically go and the button should be enabled in case the passwords match
  4. If the new password doesn't match the password rule, show the password hint
  5. Disable button if old password is same as new password
  6. Copy and paste the passwords instead of typing
  7. Type an invalid old password, match both new and confirm passwords. Submitting should throw server error, then change either new or confirm password. Server error should hide and password mismatch error should be shown.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

exp-change-pass-val-web.mp4

Mobile Web

exp-change-pass-val-mweb.mp4

Desktop

exp-change-pass-val-desktop.mp4

iOS

exp-changes-pass-val-ios.mp4

Android

exp-changes-pass-android.mp4

@mananjadhav mananjadhav requested a review from a team as a code owner August 4, 2021 16:44
@MelvinBot MelvinBot requested review from alex-mechler and removed request for a team August 4, 2021 16:44
@mananjadhav
Copy link
Collaborator Author

@AndrewGable PR Raised. I'd like to clarify one case.

If I put an incorrect password and submit we throw a validation error from the backend. After this step, if I have a mismatch between newPassword and confirmPassword it shows the server validation message as well as the password mismatch message. Do you want me to hide the server validation when mismatch error is shown?

Attached is the video for this test.

two-validation-messages.mp4

src/pages/settings/PasswordPage.js Outdated Show resolved Hide resolved
@mananjadhav
Copy link
Collaborator Author

@alex-mechler Any feedback on #4409 (comment)

@alex-mechler
Copy link
Contributor

@alex-mechler Any feedback on #4409 (comment)

I don't have any strong feedback on it, but I think we should only show 1 error message at a time. I think the server error should be hidden when any of the fields change personally.

@mananjadhav
Copy link
Collaborator Author

@alex-mechler Any feedback on #4409 (comment)

I don't have any strong feedback on it, but I think we should only show 1 error message at a time. I think the server error should be hidden when any of the fields change personally.
Yeah agree we should only show 1 message, but do we want to consider that as a part of this change?

Currently, the server error is coming from props. We will have to move it to state and ensure that the state is updated when props are updated.

Simplest I can do is:

{!this.state.shouldShowPasswordConfirmError && !isEmpty(this.props.account.error) && (
     <Text style={styles.formError}>
          {this.props.account.error}
     </Text>
)}

Let me know which path to take?

  1. No Fix
  2. Move props.account to state
  3. Add the above code

cc: @AndrewGable

@alex-mechler
Copy link
Contributor

I think I would pick 3, but I would love to hear @AndrewGable's thoughts on this too, since it has been a little while since I have been heavily in this code

@mananjadhav
Copy link
Collaborator Author

@AndrewGable Poke

@AndrewGable
Copy link
Contributor

I agree 3 looks good!

@mananjadhav
Copy link
Collaborator Author

@AndrewGable @alex-mechler Changes completed! Please review.

@parasharrajat
Copy link
Member

parasharrajat commented Aug 6, 2021

@mananjadhav It's always helpful to clean up the PR title. You can keep this if you like it. 😄

@mananjadhav mananjadhav changed the title fix(change-password): Confirm password validation in Change Password Fixed Confirm Password Validation in "Change Password" screen Aug 6, 2021
@mananjadhav
Copy link
Collaborator Author

Updated. Thanks for the tip.

@alex-mechler
Copy link
Contributor

Can you add some QA steps so that QA can test this once its on staging?

@mananjadhav
Copy link
Collaborator Author

@alex-mechler Updated

@mananjadhav
Copy link
Collaborator Author

@alex-mechler Anything else is required for this PR to get merged?

@alex-mechler
Copy link
Contributor

I was OOO yesterday. I'll give it another review in a few minutes 😄

src/pages/settings/PasswordPage.js Outdated Show resolved Hide resolved
@mananjadhav
Copy link
Collaborator Author

PR Updated

@alex-mechler alex-mechler merged commit f3228de into Expensify:main Aug 10, 2021
@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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @alex-mechler in version: 1.0.83-2 🚀

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

@isagoico
Copy link

This issue is failing this PR on desktop #4665

@botify
Copy link

botify commented Aug 17, 2021

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-08-24. 🎊

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

platform result
🤖 android 🤖 failure ❌
🖥 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.

7 participants