Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.85k forks source link

[HOLD for payment 2024-08-07] [$250] Login – Please enter an email or phone number is not is not displayed in Spanish #44849

Closed lanitochka17 closed 1 month ago

lanitochka17 commented 3 months ago

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


Version Number: 9.0.4.0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): gocemate+a523@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Scroll down the login page
  3. Change the language to Spanish

Expected Result:

"Please enter an email or phone number" error message should be displayed in Spanish

Actual Result:

"Please enter an email or phone number" error message is not displayed in Spanish

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/e28ac091-2a72-4559-b93a-1e5a59783d92

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015ecfe6dbe2a9cae6
  • Upwork Job ID: 1810338171865148259
  • Last Price Increase: 2024-07-15
  • Automatic offers:
    • alitoshmatov | Reviewer | 103143197
    • ShridharGoel | Contributor | 103143199
Issue OwnerCurrent Issue Owner: @muttmuure
ShridharGoel commented 3 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Login – Please enter an email or phone number is not is not displayed in Spanish

What is the root cause of that problem?

translate is used here:

                setFormError(translate('common.pleaseEnterEmailOrPhoneNumber'));

Same issue is at multiple other places also.

What changes do you think we should make in order to solve the problem?

We need to remove translate:

                setFormError('common.pleaseEnterEmailOrPhoneNumber');

And set it in the errorText:

errorText={formError ? translate(formError) : undefined}

Update at other string usages also (some in the same file and some in other files).

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 3 months ago

@muttmuure FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

Krishna2323 commented 3 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Login – Please enter an email or phone number is not is not displayed in Spanish

What is the root cause of that problem?

translate is use when setting the form error. https://github.com/Expensify/App/blob/003d2feca6a70f7e25783240c965d89a3f46ccf8/src/pages/signin/LoginForm/BaseLoginForm.tsx#L76-L87

What changes do you think we should make in order to solve the problem?

Remove translate when updating formError and use it when passing the formError as errorText prop.

We also need to update the type for formError state: const [formError, setFormError] = useState<TranslationPaths | undefined>();

We should also look up for other components which uses translate when setting the error text state.

What alternative solutions did you explore? (Optional)

ShridharGoel commented 3 months ago

Proposal

Updated

Krishna2323 commented 3 months ago

Proposal updated

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~015ecfe6dbe2a9cae6

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External)

alitoshmatov commented 3 months ago

I think I have seen similar issue in the past, and if I am not mistaken we ignored this kind of issues for now. I will try to look for it

alitoshmatov commented 3 months ago

@ShridharGoel, @Krishna2323 Thank you both for your proposals. This approach is not stable solution since there errors could be any type, for example if backend sends an error we are not able to translate it and it breaks the app

ShridharGoel commented 3 months ago

Backend messages use serverErrorText which is used in FormAlertWithSubmitButton.

Zakpak0 commented 3 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

At the Expensify Website, on the login page, the error text displayed under the input labeled "Phone or email" is not being updated.

If the error text is already on display, when the language is switched from English to Spanish, or vice versa, that error text is not updated to the new language.

What is the root cause of that problem?

In BaseLoginForm.tsx this error is only being translated when the validate function is called.

What changes do you think we should make in order to solve the problem?

The components consuming the error need to have a variable which contains the most up to date state. We can utilize the useMemo hook to fix our problem simply. It's callback should return the translated text and the translate function, returned from the useLocalize, can be set in our memo's dependency array. This will give us a value we can pass to our error component which will always be in the correct language.

I took these and additional, more advanced steps that can be viewed here on my test branch

(a) I extracted the logic into a hook for reusability. **src/hooks/useTranslation.ts** ```typescript export default function useTranslation() { const [phrase, setPhrase] = useState() const { translate } = useLocalize() const translation = useMemo(() => { try { if (!phrase) return undefined return translate(phrase) }catch (e) { return undefined } }, [translate]) return {translation, setPhrase} } ```
(b) Additionally, I created a more complex version of that hook that can return multiple variables which contain translations. This one is a bit different, it will accept an initialState as an argument. Which should be a map of key (name of variable you want returned) and value (initial value of that key which should be one of the TranslationPaths). It will then return to you a map of the aforementioned key and the translated value of it's "TranslationPath". **src/hooks/useTranslation.ts** ```typescript export function useMultipleTranslations< TState extends MultipleTranslationsState = MultipleTranslationsState, TAction extends MultipleTranslationsAction = MultipleTranslationsAction >(initalState: TState) { const { translate } = useLocalize(); const reducer = (state: TState, action: TAction): TState => { return { ...state, [action.type]: action.payload } }; const translateState = (state: TState) => { return Object.entries(state).reduce((translations, [key, phrase]) => { try { if (!phrase) return { ...translations, [key]: undefined }; return { ...translations, [key]: translate(phrase) } } catch (e) { return { ...translations, [key]: undefined } } }, state) }; const [state, dispatch] = useReducer(reducer, translateState(initalState)); const translations = useMemo(() => { return translateState(state); }, [translate, state]); return { translations, dispatch }; } ```

Both of these implementations handle incorrect values being passed to the translate function. But I would need more guidance on what the proper thing is to do in these cases.

What alternative solutions did you explore?

N/A

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 3 months ago

@muttmuure, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

alitoshmatov commented 3 months ago

Okay I think we can go with @ShridharGoel 's proposal https://github.com/Expensify/App/issues/44849#issuecomment-2209284457

@Krishna2323 Your proposal is almost the same, you might have small improvements but it is still not significant enough to considered different.

C+ reviewed 🎀 👀 🎀

melvin-bot[bot] commented 3 months ago

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

Krishna2323 commented 3 months ago

@alitoshmatov, can you pls check the time stamps, @ShridharGoel updated his proposal after mine, initially his proposal only suggested to remove the translate which would not work.

This was the only proposal and was marked as outdated at the time when I posted my proposal.

Monosnap  $250  Login – Please enter an email or phone number is not is not displayed in Spanish · Issue #44849 · Expensify:App 2024-07-16 19-32-48
alitoshmatov commented 3 months ago

@Krishna2323 I understand what you mean here, but I still think it was pretty obvious from his proposal that we should need to add translate function in errorText attribute. I checked his latest edit which was on 9:05 right before your proposal.

I think I am making a right judgement here, this was an obvious change that we wouldn't miss when applying solution

alitoshmatov commented 3 months ago

If you disagree with my judgement, I think we can ask for @luacmartins 's opinion

Krishna2323 commented 3 months ago

@alitoshmatov, I mean if that wasn't important then ShridharGoel wouldn't have marked his proposal as outdated and have posted a new one after finding the correct one, it seems obvious now because we know the solution, After 3 minutes of my proposal ShridharGoel added his second proposal which should have been considered as the first on because at the time of my proposal his only and initial proposal was marked as outdated. That was not working and took some time for me also to find the correct solution.

@luacmartins, can you pls help reviewing this.

Krishna2323 commented 3 months ago

@luacmartins, just for your info here is the time line of events.

  1. ShridharGoel posted his initial proposal to remove the the use of translate which he marked as outdated after few minutes.
  2. I posted my proposal which suggests to remove the the use of translate and to use it when we pass the message. At this time ShridharGoel's proposal was marked as outdated and wasn't working.
  3. After 3 minutes when my proposal was posted ShridharGoel posted his second proposal which was same as mine and not only he posted a new proposal, at the same time he also made change to the outdated proposal
  4. After few hours he turned his second proposal into Proposal Updated comment and marked his outdated proposal as regular comment.

@alitoshmatov, pls look the these events.

ShridharGoel commented 3 months ago

Edit: It was actually an issue that I wasn't able to convert from outdated to normal because of which I had to post a new proposal: https://github.com/Expensify/App/issues/44849#issuecomment-2231381531

Also, the timestamp difference is 1 minute which makes it obvious that the change is not copied.

Old comment (can't cross it out since markdown doesn't work in email replies): FYI, I had added a new proposal because I wasn't getting the option to edit it, to add the line mentioning that translate needs to be added while passing the prop. Later, I started getting the option to edit, so I had edited the original one and edited the new one to say "Proposal Updated".

Krishna2323 commented 3 months ago

FYI, I had added a new proposal because I wasn't getting the option to edit it, to add the line mentioning that translate needs to be added while passing the prop. Later, I started getting the option to edit, so I had edited the original one and edited the new one to say "Proposal Updated".

The edit option was not showing, but as soon as my proposal was posted the edit option became visible and you updated you proposal after 2 mins, right? I'm haven't faced something like this in gh comments and why you had marked your proposal outdated?

Also, the timestamp difference is 1 minute which makes it obvious that the change is not copied.

Can you pls share screenshots, its showing something else for me.

My initial proposal came at 9:43 PM GMT+5:30:

initial_proposal_time

You posted your second proposal at 9:46 PM GMT+5:30, 3 mins after mine proposal was posted:

Monosnap  $250  Login – Please enter an email or phone number is not is not displayed in Spanish · Issue #44849 · Expensify:App 2024-07-16 21-01-50

You updated your initial proposal at 9:45 PM GMT+5:30, 2 mins after mine proposal was posted:

Monosnap  $250  Login – Please enter an email or phone number is not is not displayed in Spanish · Issue #44849 · Expensify:App 2024-07-16 21-03-46

ShridharGoel commented 3 months ago

Screenshots showing 1 min time diff:

9:45 PM:

Screenshot 2024-07-16 at 9 22 49 PM

9:44 PM

Screenshot 2024-07-16 at 9 23 06 PM
ShridharGoel commented 3 months ago

I'm okay with skipping this task since I already have other tasks to work on, but I don't like the false "copied" statement mentioned by you. In a similar way, I can say that you copied my initial proposal, and just added an extra line and submitted it taking advantage of the fact that I was facing some issue with outdated/edited options - would you be okay in seeing such a statement?

Krishna2323 commented 3 months ago

In a similar way, I can say that you copied my initial proposal, and just added an extra line and submitted it taking advantage of the fact that I was facing some issue with outdated/edited options - would you be okay in seeing such a statement?

Your initial proposal wasn't working so you marked that as outdated. At the time I posted my proposal, there was no existing proposal. You said the edit option wasn't showing, but as soon as my proposal came the edit option started working for you and you updated your proposal. This doesn't makes sense to me.

You could have posted a new proposal instantly if the edit option wasn't working but you only posted a new proposal after my proposal came. Wasn't 8 mins enough for adding a new proposal with just 1 line changed?

You said the edit option wasn't showing so you have added a new proposal, right? Then how edit was made to your previous proposal 1 min before your second proposal came? That means it was working before you posted your second proposal.

Monosnap  $250  Login – Please enter an email or phone number is not is not displayed in Spanish · Issue #44849 · Expensify:App 2024-07-16 21-03-46

Monosnap  $250  Login – Please enter an email or phone number is not is not displayed in Spanish · Issue #44849 · Expensify:App 2024-07-16 21-01-50

luacmartins commented 3 months ago

Thank you all for your proposals and commitment to solving this issue. Please keep in mind that we are all working towards the same goal. It's normal to come up with similar proposals independently, especially in obvious bugs like this one or to build on each others proposals to come up with the best solutions. We're all part of the same team here.

I still think it was pretty obvious from his proposal that we should need to add translate function in errorText attribute.

I agree with @alitoshmatov here, it's a pretty simple bug so I think we can go ahead with @ShridharGoel's proposal.

melvin-bot[bot] commented 3 months ago

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 3 months ago

📣 @ShridharGoel 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

Krishna2323 commented 3 months ago

@luacmartins, there was no proposal at the time I posted my proposal. I think thats unfair. If it was obvious then ShridharGoel wouldn't have marked his proposal as outdated. It only seems obvious now because we know the solution.

ShridharGoel commented 3 months ago

You said the edit option wasn't showing so you have added a new proposal, right? Then how edit was made to your previous proposal 1 min before your second proposal came?

Okay I think I remembered it wrongly earlier. What happened is that I had marked my proposal as outdated, then edited it, then tried converting it to normal but that wasn't working - so I had to post a new proposal. And later, I got the option to change it back to normal which I did.

Krishna2323 commented 3 months ago

Okay I think I remembered it wrongly earlier. What happened is that I had marked my proposal as outdated, then edited it, then tried converting it to normal but that wasn't working - so I had to post a new proposal. And later, I got the option to change it back to normal which I did.

You are just changing your statements after you are proved wrong.

ShridharGoel commented 3 months ago

@luacmartins I'm okay with the other contributor taking this one, since I don't want anyone to feel unfair. But I still stand by the statement that my solution was original and wasn't copied and it would be appreciated if the other contributor deletes that comment.

ShridharGoel commented 3 months ago

You are just changing your statements after you are proved wrong.

It's fine, you can think whatever you feel like. I'm here to contribute and collaborate and not argue.

Krishna2323 commented 3 months ago

Thanks @ShridharGoel, no one likes to get into arguments until they see their efforts go in vain.

ShridharGoel commented 3 months ago

until they see their efforts go in vain

FYI, I had also put in effort.

ShridharGoel commented 3 months ago

it clearly seems that my proposal was copied.

Hope you know that I can also claim that you copied the RCA and main part of the solution from my initial proposal.

Krishna2323 commented 3 months ago

Hope you know that I can also claim that you copied the RCA and main part of the solution from my initial proposal.

There was no proposal at the time I posted mine. I hope you want want to end the argument because you are here to contribute and collaborate and not argue.

ShridharGoel commented 3 months ago

There was no proposal at the time I posted mine.

Then how do you know that I had posted a proposal before yours which was marked outdated and then changed back to a normal comment? You must have seen the proposal before posting yours.

I hope you want want to end the argument

Yes, but the fact that you haven't yet deleted the unnecessary comments made me write the above.

Krishna2323 commented 3 months ago

Then how do you know that I had posted a proposal before yours which was marked outdated and then changed back to a normal comment? You must have seen the proposal before posting yours.

I knew that you had marked your proposal outdated and that's why I invested my time and effort to find the correct solution as your solution wasn't working. I don't think outdated comment should be considered as proposal.

Yes, but the fact that you haven't yet deleted the unnecessary comments made me write the above.

Sorry for that, I forgot updating it. Updated now.

@luacmartins, could you pls assign this issue to me as per this comment? I know some of my comments may seem a bit rude, but believe me, I'm just stating facts with proof. At the time I posted my proposal, there were no other proposals, so mine should be considered the first one.

alitoshmatov commented 3 months ago

@luacmartins, there was no proposal at the time I posted my proposal. I think thats unfair. If it was obvious then ShridharGoel wouldn't have marked his proposal as outdated. It only seems obvious now because we know the solution.

There was no way for me to check if proposal was marked outdated, that's why I just checked edit history and that's all. I don't think arguing this much was right thing to do here.

Krishna2323 commented 3 months ago

There was no way for me to check if proposal was marked outdated, that's why I just checked edit history and that's all. I don't think arguing this much was right thing to do here.

@alitoshmatov, I agree with this. If this had been the first time, I could have ignored it, but how many times can I ignore these types of incidents? In the last few weeks, there have been at least 4-5 incidents where I posted the first correct proposal, but my proposal wasn't selected just because a contributor posted a new proposal with the exact same RCA and solution as mine and just added a few minor changes. In this case my proposal should be considered as first correct and working proposal. It's very upsetting and demotivating to go through these types of incidents.

melvin-bot[bot] commented 3 months ago

@luacmartins @muttmuure @alitoshmatov this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Krishna2323 commented 3 months ago

@luacmartins, you can assign it to @ShridharGoel.

I just wanted to state the timeline of the events happened in this issue because marking a proposal as outdated won't be shown in the GitHub comment edit history, so I wanted to make C+ and you aware of that. I should have stated my points clearly before proposal selection, I agree it's my fault.

Sorry for any inconvenience I have caused.🙏

ShridharGoel commented 3 months ago

@Krishna2323 I think it is fine for you to take this. Else, we can split the work and the bounty if @luacmartins agrees.

melvin-bot[bot] commented 3 months ago

@luacmartins, @muttmuure, @alitoshmatov Huh... This is 4 days overdue. Who can take care of this?

Krishna2323 commented 3 months ago

@alitoshmatov, PR ready for review ^

melvin-bot[bot] commented 2 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.14-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-08-07. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 2 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed: