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

3195 fix messages on password screen #3769

Merged

Conversation

dklymenk
Copy link
Contributor

@dklymenk dklymenk commented Jun 26, 2021

Details

Makes the client facing offline error message more user friendly as well as adds translations to all potential errors on password form.

Fixed Issues

$ #3195

Tests / QA Steps

Offline error

  1. Log out
  2. Enter email to proceed to password form
  3. Disable internet on the device or in browser devtools
  4. Try to submit any password
  5. Verify that the error message says: Incorrect login or password. Please try again.

Incorrect login or password

  1. Log out
  2. Enter email to proceed to password form
  3. Submit wrong password
  4. Verify that the error message says: Looks like you're offline. Please check your connection and try again.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

DeepinScreenshot_select-area_20210626210058

Mobile Web

Screen Shot 2021-06-26 at 21 56 44

Desktop

DeepinScreenshot_select-area_20210626214822

iOS

Screen Shot 2021-06-26 at 21 53 56

Android

DeepinScreenshot_select-area_20210626213753

@dklymenk dklymenk marked this pull request as ready for review June 26, 2021 19:34
@dklymenk dklymenk requested a review from a team as a code owner June 26, 2021 19:34
@MelvinBot MelvinBot requested review from sketchydroide and removed request for a team June 26, 2021 19:34
@dklymenk
Copy link
Contributor Author

There is this one concern I'd like to raise.

If you put less than 4 characters in password text input and submit the form, you will get an error 402 from server which translates into a You have 2FA enabled on this account. Please sign in using your email or phone number. error message on FE. I think I should ask someone to validate that these error codes are still up to date: https://github.com/Expensify/Expensify.cash/blob/main/src/libs/API.js#L220-L241

@sketchydroide
Copy link
Contributor

@dklymenk thanks for raising that, I think that might be an issue with our backend, and I'll investigate. Code wise your changes look good and I'm happy to aprove/merge them, And I think we can create a GH either for the internal team if it's in the backend, or for contributors if it's not, but it feels that there is at least some backend work.

Copy link
Contributor

@sketchydroide sketchydroide left a comment

Choose a reason for hiding this comment

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

LGTM

@sketchydroide
Copy link
Contributor

Ok after some investigation, this seems to make sense to have the error like that for case @dklymenk mentioned, the sense is that if the user has entered a password and a email and the account has 2FA then it needs the 2FA code as well before it tries to authenticate.

@dklymenk
Copy link
Contributor Author

dklymenk commented Jun 30, 2021

The issue I've described is for account with no 2FA. I get the 2FA error message with that account when I send less than 4 characters. Please, give it a try, if you have an account with no 2FA.

@sketchydroide
Copy link
Contributor

Ohh, that is definetly interesting, still should not affect this PR.

@sketchydroide sketchydroide merged commit 7c34010 into Expensify:main Jul 5, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Jul 5, 2021

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

@dklymenk dklymenk deleted the 3195-fix-messages-on-password-screen branch July 5, 2021 14:21
@roryabraham
Copy link
Contributor

The deploy messages are unfortunately broken right now. This was deployed to staging yesterday.

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