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-09-15] [$500] Currency symbol not being properly shown for all currencies - reported by @Puneet-here #10287

Closed
kavimuru opened this issue Aug 6, 2022 · 30 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

Comments

@kavimuru
Copy link

kavimuru commented Aug 6, 2022

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. Navigate to a 1:1 conversation
  2. Press the + button in the compose bar and click Request Money
  3. Click on the currency to enter the currency list
  4. Type the first letter of currency code, for example U

Expected Result:

The list of currencies is reduced to those that contain the entered letter, for example U.

Actual Result:

The list of currencies contains not only codes with letter U, but also codes without U.

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.1.88-1

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:
Bug5676929_currency-list-H
Bug5676929_currency-list-U

Bug5676929_currency-list-U.mp4

Expensify/Expensify Issue URL:

Issue reported by:
@Puneet-here

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Aug 6, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2022

@puneetlath Whoops! This issue is 2 days overdue. Let's get this updated quick!

@puneetlath
Copy link
Contributor

The bug here isn't that these options are showing up when the letter h is typed (as in the first image). The bug is that we are showing AED - AED when we should be showing AED - Dhs like we do on old dot.

image

So the search feature is properly searching the two fields. But we are not correctly showing them.

@melvin-bot melvin-bot bot removed the Overdue label Aug 10, 2022
@puneetlath puneetlath changed the title IOU- Searching by currency codes returns codes that do not contain the entered letter Currency symbol not being properly shown for all currencies Aug 10, 2022
@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Aug 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2022

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

@puneetlath
Copy link
Contributor

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2022

Current assignee @puneetlath is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Currency symbol not being properly shown for all currencies [$250] Currency symbol not being properly shown for all currencies Aug 10, 2022
@Puneet-here
Copy link
Contributor

Hi, I had reported this here.

@tyagi6165
Copy link

We can resolve this by maintaining a JSON collection with county names and country currency symbols and flags. On the selecting a country, we can hold the country data and show symbols where ever needed

@puneetlath puneetlath changed the title [$250] Currency symbol not being properly shown for all currencies [$250] Currency symbol not being properly shown for all currencies - reported by @Puneet-here Aug 11, 2022
@puneetlath
Copy link
Contributor

@tyagi6165 that information is already being stored. That is why we are able to search on it.

@Harshdeepjoshi
Copy link
Contributor

The bug here isn't that these options are showing up when the letter h is typed (as in the first image). The bug is that we are showing AED - AED when we should be showing AED - Dhs like we do on old dot.

image

So the search feature is properly searching the two fields. But we are not correctly showing them.

According to this comment, what is the expected behavior, Do we need to fix the search, or do we need to fix the symbol?

@puneetlath
Copy link
Contributor

@Harshdeepjoshi we need to fix the symbol. The search is working properly. But the display should be this (image taken from old expensify):
image

Instead of this (image taken from new expensify):
image

@zereraz
Copy link
Contributor

zereraz commented Aug 15, 2022

Proposal

To show the currency symbols beside the currency code.

text: `${currencyCode} - ${CurrencySymbolUtils.getLocalizedCurrencySymbol(this.props.preferredLocale, currencyCode)}`,

Changes to

text: `${currencyCode} - ${currencyInfo.symbol}`,

This is the output I got here

image

Also if we call getLocalizedCurrencySymbol,

return _.find(parts, part => part.type === 'currency').value;

is returning Currency Code and not the Symbol.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Aug 16, 2022

Also if we call getLocalizedCurrencySymbol,

return _.find(parts, part => part.type === 'currency').value;

is returning Currency Code and not the Symbol.

Then we should fix the method which is not working as expected

@puneetlath
Copy link
Contributor

Agreed. @zereraz feel free to re-propose a solution that fixes the getLocalizedCurrencySymbol function instead of removing it from IOUCurrencySelection.js.

@0xmiros
Copy link
Contributor

0xmiros commented Aug 16, 2022

Proposal

function getLocalizedCurrencySymbol(preferredLocale, currencyCode) {

This function uses Intl.NumberFormat:
return new Intl.NumberFormat(locale, options).formatToParts(number);

Currency symbol from Intl.NumberFormat cannot be exactly same as what we're trying to get and it also depends on preferred locale.
AED
Currency symbol of AED we're trying to get is Dhs, neither AED nor د.إ.
Therefore, we cannot use default javascript Intl.NumberFormat to get correct currency symbol.

Solution
We need to connect Onyx in CurrencySymbolUtils.js and get currencyList from ONYKEYS.CURRENCY_LIST key.
Here's updated code proposal of getLocalizedCurrencySymbol function:
getLocalizedCurrencySymbol
By doing this, fixes both screens (currency symbol issue in this GH and also in Request money screen)

If we don't want to connect Onyx, we can send currencyList as a param of getLocalizedCurrencySymbol function, instead of preferredLocale

Before fix:
before fix 1
before fix 2

After fix:
after fix 1
after fix 2

@melvin-bot melvin-bot bot removed the Overdue label Aug 22, 2022
@puneetlath puneetlath changed the title [$250] Currency symbol not being properly shown for all currencies - reported by @Puneet-here [$500] Currency symbol not being properly shown for all currencies - reported by @Puneet-here Aug 23, 2022
@puneetlath
Copy link
Contributor

First of all, I'm doubling the job price to $500 since the job has been open for over a week.

Secondly, thanks everyone for all the proposals/info on this issue. This has been quite enlightening!

I think we should go with @zereraz new proposal of using ${CurrencySymbolUtils.getLocalizedCurrencySymbol(this.props.preferredLocale, currencyCode)} for both the display text and the search text.

That feels like the most accurate since it takes into account their preferred locale and searches for and displays the right symbol based on the preferred local.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Help Wanted Apply this label when an issue is open to proposals by contributors Daily KSv2 labels Aug 23, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2022

📣 @zereraz You have been assigned to this job by @puneetlath!
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 📖

@zereraz
Copy link
Contributor

zereraz commented Aug 25, 2022

I've applied on Upwork. I can provide the PR within 2 days.

@puneetlath
Copy link
Contributor

Not overdue. PR is on staging.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Overdue Weekly KSv2 labels Sep 7, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.97-7 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-09-15. 🎊

@melvin-bot melvin-bot bot changed the title [$500] Currency symbol not being properly shown for all currencies - reported by @Puneet-here [HOLD for payment 2022-09-15] [$500] Currency symbol not being properly shown for all currencies - reported by @Puneet-here Sep 8, 2022
@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Sep 14, 2022
@Santhosh-Sellavel
Copy link
Collaborator

Not overdue, PR is on production.

@melvin-bot melvin-bot bot removed the Overdue label Sep 14, 2022
@puneetlath
Copy link
Contributor

@zereraz paid.

@Santhosh-Sellavel @Puneet-here sent you hiring offers.

@zereraz
Copy link
Contributor

zereraz commented Sep 15, 2022

@puneetlath @Santhosh-Sellavel Thanks !

@thesahindia
Copy link
Member

thesahindia commented Sep 15, 2022

I was just searching if there are any existing issue before posting one about the currency and I found this which is already fixed but I couldn't understand what we really did here. The issue was about currency symbol as mentioned here, which is still there.
Here's a side by side comparison with currency selector of general settings and currency selector page -
Screenshot 2022-09-15 at 8 28 18 PM

@puneetlath
Copy link
Contributor

@thesahindia ah, looks like we need to update that component also.

Everyone has been paid!

@thesahindia
Copy link
Member

Not sure what you mean by that. Are you saying that we will need to update general settings? We want to show the symbols right?

@puneetlath
Copy link
Contributor

Yes, I'm saying we should update General Settings.

In the IOU currency selector we are using ${currencyCode} - ${CurrencySymbolUtils.getLocalizedCurrencySymbol(this.props.preferredLocale, currencyCode)}

Whereas in the General Settings we are using ${currencyCode} - ${this.props.currencyList[currencyCode].symbol}

We should make those consistent.

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
Projects
None yet
Development

No branches or pull requests

9 participants