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

Sign in - Offline message on password page is not friendly #3195

Closed
isagoico opened this issue May 27, 2021 · 27 comments
Closed

Sign in - Offline message on password page is not friendly #3195

isagoico opened this issue May 27, 2021 · 27 comments
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

isagoico commented May 27, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

Let's use the following message:
Looks like you're offline. Please check your connection and try again.

Actual Result:

API is offline is displayed which might be confusing for regular users.

Action Performed:

  1. Launch the app
  2. In Log in screen put email witch with has 2FA enabled
  3. Proceed in password screen
  4. Turn off internet connection and try to enter a password.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.56-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@isagoico isagoico added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels May 27, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label May 27, 2021
@Christinadobrzyn
Copy link
Contributor

Don't see any other GH e.cash issues related to this sending to eng to get a second opinion!

@MelvinBot
Copy link

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Christinadobrzyn Christinadobrzyn removed their assignment May 28, 2021
@Julesssss
Copy link
Contributor

This is an easy one to pass over to external contributors, but first, we need to choose better messaging. Perhaps something like:

Please check your connection and try again.

@Expensify/design any thoughts?

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@Julesssss
Copy link
Contributor

Hey @isagoico, would you mind reminding me what makes this issue be retested? I think it doesn't need to be because this isn't a regression but will be a new change. Please let me know if you disagree :)

@isagoico
Copy link
Author

isagoico commented Jun 2, 2021

For our weekly KI we retest all open issues that were raised by our team on regression runs, that's why it was retested. I agree with you that we can exclude this one from the restests, let me know if there are any change of plans

@Julesssss
Copy link
Contributor

Ah, that makes sense. Thanks :)

@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label Jun 21, 2021
@MelvinBot
Copy link

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 21, 2021
@Julesssss Julesssss removed their assignment Jun 21, 2021
@dklymenk
Copy link
Contributor

Hello, I initially assumed this one is as simple as replacing a string in CONST.js, but since the app has support for multiple languages, the user facing errors should be translated as well.

I also noticed that there already is a similar string in the app that is used on the first login step form (email):

    session: {
        offlineMessage: 'Looks like you\'re not connected to internet. Can you check your connection and try again?',
    },

It is very similar to the one mentioned in the OP : "Looks like you're offline. Please check your connection and try again.".

In my opinion there is no reason these errors should be different. Like why would offline message on 1st step form be different from 2nd step form one?

Anyway, after that I wanted to replace every reference to CONST.ERROR.API_OFFLINE in the codebase with translate('', 'session.offlineMessage'). This would have worked, but that causes a require cycle since src/libs/API.js would import src/libs/translate.js that imports src/libs/Log.js that imports src/libs/API.js. Anyway, it's not a good time. So I propose the following:

I will replace this line with the following:

Onyx.merge(ONYXKEYS.ACCOUNT, {error: translate(error.message), loading: false});

I will then make sure that all the potential errors that might be caught there have translatable strings as their message:

  1. https://github.com/Expensify/Expensify.cash/blob/main/src/libs/API.js#L222-L239 I will move all these to translations en.js, create new translation keys for them and add es translations too. Then replace the error strings in API.js file with translation keys.
  2. For API is offline I can do similar thing - just replace the value for ERROR.API_OFFLINE in CONST.js with a translation key session.offlineMessage.

Overall, this solution not only replaces the text for the particular error message the issue is about, but also allows to translate it and all the other potentials errors one can get on that form, while avoiding any app breaking imports that cause require cycles.

@MelvinBot
Copy link

Triggered auto assignment to @roryabraham (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@mallenexpensify
Copy link
Contributor

Posted. https://www.upwork.com/jobs/~017ce3cd40c288a83f
@roryabraham can you review @dklymenk 's proposal above?

@roryabraham
Copy link
Contributor

@dklymenk Looks good to me 👍

Feel free to submit a PR as soon as you've been hired on Upwork

@mallenexpensify
Copy link
Contributor

Hired @dklymenk in Upwork

@dklymenk
Copy link
Contributor

Great. I will need a few more things from you:

  1. What about the offline message? Do I use the one that is already in the app (used one the 1st step login form, where you enter the email)? Do I use 2 separate ones as suggested by the OP? Or do I use new message, but on both 1st and 2nd step forms?
  2. I will need Spanish translations for the following strings:
    • "Incorrect login or password. Please try again."
    • "You have 2FA enabled on this account. Please sign in using your email or phone number."
    • "Invalid login or password. Please try again or reset your password."
    • " We were unable to change your password. This is likely due to an expired password reset link in an old password reset email. We have emailed you a new link so you can try again. Check your Inbox and your Spam folder; it should arrive in just a few minutes."
    • "You do not have access to this application. Please add your GitHub username for access."
    • "Your account has been locked after too many unsuccessful attempts. Please try again after 1 hour."
    • "Something went wrong. Please try again later."
    • and if the answer to my first question is to use the string from OP at least once, I will also need a translation for "Looks like you're offline. Please check your connection and try again."

@parasharrajat
Copy link
Member

parasharrajat commented Jun 24, 2021

I will need Spanish translations for the following strings:

Google translate.

@roryabraham
Copy link
Contributor

roryabraham commented Jun 24, 2021

"Incorrect login or password. Please try again."

Usuario o clave incorrectos. Por favor inténtalo de nuevo

You have 2FA enabled on this account. Please sign in using your email or phone number.

Tienes autenticación de 2 factores activada en esta cuenta. Por favor conéctate usando su email o número de teléfono

Invalid login or password. Please try again or reset your password.

Usuario o clave incorrectos. Por favor inténtalo de nuevo o resetea tu clave

We were unable to change your password. This is likely due to an expired password reset link in an old password reset email. We have emailed you a new link so you can try again. Check your Inbox and your Spam folder; it should arrive in just a few minutes.

No pudimos cambiar tu clave. Probablemente porque el enlace para resetear la clave ha expirado. Te hemos enviado un nuevo enlace. Chequea tu bandeja de entrada y tu carpeta de Spam

You do not have access to this application. Please add your GitHub username for access.

No tienes acceso a esta aplicación. Por favor agrega tu usuario de GitHub para acceder

Your account has been locked after too many unsuccessful attempts. Please try again after 1 hour.

Tu cuenta ha sido bloqueada tras varios intentos fallidos. Por favor inténtalo otra vez dentro de 1 hora

Something went wrong. Please try again later.

Ha ocurrido un error. Por favor inténtalo mas tarde

Looks like you're offline. Please check your connection and try again.

Parece que estás desconectado. Por favor chequea tu conexión e inténtalo otra vez

dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jun 25, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jun 26, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jun 26, 2021
@dklymenk
Copy link
Contributor

Hello, I submitted a PR #3769 for this issue.

FYI since I haven't gotten a response my question of whether or not replace the original offline message in code with new one or to use two different ones, I have replaced the original message with a new one.

@roryabraham
Copy link
Contributor

Thanks @dklymenk, that seems like a good idea to me 👍🏼

@mallenexpensify
Copy link
Contributor

@roryabraham to confirm... seems like a good idea to me 👍🏼 means I should hire @dklymenk in Upwork?

@roryabraham
Copy link
Contributor

Hmmm @mallenexpensify I thought he was already hired? #3195 (comment)

@mallenexpensify
Copy link
Contributor

D'oh, my bad. I think I forgot to assign the issue earlier so didn't see @dklymenk when I glanced earlier. Assigned now

@sketchydroide
Copy link
Contributor

I've look into the code in the PR, it looks good, @dklymenk raised a concern that I'll investigate into, but I don't think it should affect this GH and PR, it will most likely be resolved internaly by us.

@mallenexpensify mallenexpensify removed their assignment Jul 2, 2021
@mallenexpensify mallenexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Jul 2, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@mallenexpensify mallenexpensify self-assigned this Jul 2, 2021
@mallenexpensify
Copy link
Contributor

@trjExpensify added you via External label for buddy check next week while I'm OOO, thanks

@roryabraham
Copy link
Contributor

@mallenexpensify This has been done for a while now. Can we confirm that @dklymenk has been paid on Upwork then close this out?

@mallenexpensify
Copy link
Contributor

Paid in Upwork, it was weird cuz it showed @dklymenk twice for being hired, but neither showed being paid yet. Sorry for the delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants