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

[HOLD for payment 2022-06-15] $500 If your have your language set to Spanish, the currency is not localized when you select it #7915

Closed
mvtglobally opened this issue Feb 25, 2022 · 41 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

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


Action Performed:

  1. Set app language to spanish
  2. Navigate to IOU
  3. Check currency display

Expected Result:

Currency should be localized

Actual Result:

Currency is not localized.
It should be. ie: if I select USD, the symbol shown should be US$ and not just $.

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.40-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen Shot 2022-02-25 at 2 10 20 AM
image - 2022-02-25T021128 622

Expensify/Expensify Issue URL:
Issue reported by: @iwiznia
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1645483634039679

View all open jobs on GitHub

@MelvinBot
Copy link

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

@robertjchen
Copy link
Contributor

Great catch, opening this up for a contributor to tackle! 👍

@robertjchen robertjchen added the External Added to denote the issue can be worked on by a contributor label Feb 28, 2022
@MelvinBot
Copy link

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

@laurenreidexpensify laurenreidexpensify changed the title If your have your language set to Spanish, when selecting the currency is not localized If your have your language set to Spanish, the currency is not localized when you select it Feb 28, 2022
@laurenreidexpensify
Copy link
Contributor

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 28, 2022
@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

@iwiznia Do you think we have to update this page as well?
image

@iwiznia
Copy link
Contributor

iwiznia commented Feb 28, 2022

Good question, I think yes we probably should.

@laurenreidexpensify laurenreidexpensify changed the title If your have your language set to Spanish, the currency is not localized when you select it $500 If your have your language set to Spanish, the currency is not localized when you select it Mar 8, 2022
@tgolen
Copy link
Contributor

tgolen commented Mar 8, 2022

I guess this is still waiting for proposals.

@MelvinBot MelvinBot removed the Overdue label Mar 8, 2022
@cylim
Copy link

cylim commented Mar 9, 2022

@tgolen This is better to solve in the API Backend. I can't be sure whether the data is in the database or hardcoded data in Backend.

The reasons why it should be backend,

  1. The data is from API;
  2. If $ needed to add US in front of it, (KRW, KPW), (JPY, CNY) should add country code too.

API.js, line 1080
Screen Shot 2022-03-09 at 17 12 13

IOUCurrencySelection, try (KRW, KPW), (JPY, CNY)
Screen Shot 2022-03-09 at 17 13 53

Although I can propose a workaround in App to force adding the US in front of $, but it is hardcoded. I would recommend to make changes in database or API server.

@tgolen
Copy link
Contributor

tgolen commented Mar 9, 2022

Thanks @cylim! Ideally, it should work like this:

  1. All amounts are stored as an int
  2. There should be another field to store the currency as a string (eg. USD)
  3. Whenever we display an amount, we should look up how to display USD currency (this comes from the response of Mobile_GetConstants(), and then format the amount properly (the right symbols, the right decimal places, etc.)

Just for reference, the currency info from Mobile_GetConstants() is returning this:
image

@iwiznia It appears to me that we are formatting the currency correctly. The OP says you are expecting to see US$ but I'm not sure why that is what you are expecting. Is that an international symbol for USD? If it is, then I think we should be updating it in the source code as @cylim suggests (here - private repo). However, the fact that we've never updated that, and it would impact a ton of our systems, I'm not sure that this is a desirable change.

@parasharrajat
Copy link
Member

parasharrajat commented Mar 9, 2022

User-selected Spanish locale and for them, $ should be shown as US$. This is part of localization and thus it can be handled on UI.

-- Update grammer

@cylim
Copy link

cylim commented Mar 9, 2022

@tgolen
Yes, I agree with your comment. I think the user is expecting the country code in front or the symbol, because other currencies have it.

e.g. 
A$ - Australia Dollar
S$ - Singapore Dollar

These are not standard symbols too. Another suggestion is not to use the $,£,¥ symbols, directly using USD, SGD or AUD in the IOU screen. (frontend fix-able)

My proposal is,
IOU screen: directly using USD, SGD or AUD.
IOU Currency Selection Screen: (currency - name)

USD - United States Dollar 
SGD - Singapore Dollar

And if we want to translate it, we can translate the name part, it will be a huge list for each language.

@parasharrajat
I think you had misunderstood the questions and you didn't read my comment properly.
The Spanish locale is shown as $ and the user wants it to show as US$. Check out the screenshot from the issue.

@iwiznia
Copy link
Contributor

iwiznia commented Mar 9, 2022

@iwiznia It appears to me that we are formatting the currency correctly. The OP says you are expecting to see US$ but I'm not sure why that is what you are expecting. Is that an international symbol for USD?

Not sure if international, but yes, some countries were their currency is not USD use US$ to indicate amounts in USD.

If it is, then I think we should be updating it in the source code as @cylim suggests (here - private repo). However, the fact that we've never updated that, and it would impact a ton of our systems, I'm not sure that this is a desirable change.

No, I don't think we should update that because all the other systems using it, will break. The main problem with that file is that it is not internationalization aware, it assumes everyone is in the USA. All symbols there are how the USA represents the currencies, not how every country does it. Our new localization methods in newDot are internationalization aware and can take a currency code (the keys in the array you linked) and transform it to an amount using a specific locale. So for example USD 1000.23 in the USA would be shown as $1,000.23 while in Spain would be shown as 1.000,23 US$ (which is how spanish people would write the amount).

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 21, 2022
@MelvinBot
Copy link

📣 @mdneyazahmad You have been assigned to this job by @laurenreidexpensify!
Please apply to this job in Upwork 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 📖

@laurenreidexpensify
Copy link
Contributor

@mdneyazahmad
Copy link
Contributor

@laurenreidexpensify applied

@laurenreidexpensify
Copy link
Contributor

Upwork was being a bit weird about allowing me to update this to $500, so ignore what it says there but @mdneyazahmad @parasharrajat I will make sure appropriate payment is issued after deploy.

@mdneyazahmad
Copy link
Contributor

Update: PR is ready will create soon.

@laurenreidexpensify laurenreidexpensify added the Reviewing Has a PR in review label Mar 28, 2022
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Apr 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 20, 2022

This issue has not been updated in over 15 days. @tgolen, @parasharrajat, @laurenreidexpensify, @mdneyazahmad eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Apr 20, 2022
@laurenreidexpensify
Copy link
Contributor

Look like we're still making steady progress as per the PR

@laurenreidexpensify laurenreidexpensify added Weekly KSv2 and removed Monthly KSv2 labels May 3, 2022
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels May 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 26, 2022

This issue has not been updated in over 15 days. @tgolen, @parasharrajat, @laurenreidexpensify, @mdneyazahmad eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@laurenreidexpensify
Copy link
Contributor

@parasharrajat @mdneyazahmad what's the latest here on the PR ? Who are we waiting on to take the next action: contributor, contributor plus or Expensify engineer?

@mdneyazahmad
Copy link
Contributor

@laurenreidexpensify waiting review from @tgolen

@mountiny
Copy link
Contributor

mountiny commented Jun 6, 2022

Merged, let's hope everything will go smooth 🎉 Great work @mdneyazahmad

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Jun 8, 2022
@melvin-bot melvin-bot bot changed the title $500 If your have your language set to Spanish, the currency is not localized when you select it [HOLD for payment 2022-06-15] $500 If your have your language set to Spanish, the currency is not localized when you select it Jun 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2022

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

If no regressions arise, payment will be issued on 2022-06-15. 🎊

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 14, 2022
@laurenreidexpensify
Copy link
Contributor

Everyone has been paid. Hurrah 4 months later and we're finally done :) Well done everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests