[HOLD for payment 2023-03-01] [$4000] Few currency symbols (specially symbols after WST) are not correctly displayed on mWeb chrome on android

[kavimuru]

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. Open the app
2. Open any report
3. Click on plus and open Request Money/Send Money
4. Scroll to bottom and see the currencies below WST on mWeb chrome

Expected Result:

Symbol for XAF should be FCFA, XCD symbol should be EC$, XOF symbol should be CFA and XPF symbol should be CFPF

Actual Result:

Symbols are not correctly displayed, they are same as name of currency for XAF, XCD, XOF, XPF on mWeb chrome android

Workaround:

unknown

Platforms:

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

• Android / Chrome

Version Number: 1.2.54-2
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

mWeb.chrome.currency.issue.mp4

az_recorder_20230113_173715.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673630821293879

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c6b2d46695b607ed
• Upwork Job ID: 1617777751000506368
• Last Price Increase: 2023-02-08

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[slafortune]

Bug0 Triage Checklist

Note: see this SO for more information.

• The "bug" is actually a bug
• The bug is not a duplicate report of an existing GH.
  • If it is, close the GH and add any novel details to the original GH instead
• The bug is reproducible, following the reproduction steps.
  • If the GH doesn’t have steps to reliably reproduce the bug and you figure it out, then please add them.
  • If you’re unable to reproduce the bug, add the Needs reproduction label.
  • Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.
• The GH template is filled out as fully as possible
  • The GH body and title are clear (ie. could an external contributor understand it and work on it?)
• The GH was created by an Expensify employee or a QA tester
• If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification
• If there's a link to Slack, check the discussion to see if we decided not to fix it
• Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label

[slafortune]

Is this something that could be fixed by an external contributor @techievivek ?

[techievivek]

Looking into now.

[techievivek]

Weird. It just seems to be happening on Chrome(Android). This can be worked externally.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01c6b2d46695b607ed

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Prince-Mendiratta]

Proposal

RCA

The root cause behind this issue is the different implementation of Intl.NumberFormat on Chrome browsers on Android, introduced in #7915 . JavaScript Internationalization API (Intl) behavior may differ between Android Chrome and Chrome on desktop due to differences in the underlying platform and browser implementation. Due to these differences, the output obtained from Intl.NumberFormat is different. To be more specific, this is the output comparison for NumberFormatUtils.formatToParts on the different platforms -
Android Chrome

Linux Chrome

As can be seen, the value of the currency is different on both platforms. Digging deeper, I was unable to find the source code for the NumberFormat function in v8 but trying to get insight from polyfills, the implementation of NumberFormat has this part:

https://github.com/andyearnshaw/Intl.js/blob/0958dc1ad8153f1056653ea22b8208f0df289a4e/src/11.numberformat.js#L709-L712

By default, the currencyDisplay is symbol. Thus, I assume that the locale data is different on both platforms since Intl is unable to get the symbol for the currency XAF and other as mentioned above.

Solution

Solution 1

Instead of using Intl polyfills in just native android apps, as done in #2208, we can use the polyfills for all platforms. This would ensure that the behaviour is consistent across platforms. This is the best solution since after internationalization, the amount of potential differences between cross platforms might increase.

Code

diff --git a/src/setup/index.js b/src/setup/index.js
index 7cf509e1e3..6f4d454d7d 100644
--- a/src/setup/index.js
+++ b/src/setup/index.js
@@ -4,6 +4,14 @@ import ONYXKEYS from '../ONYXKEYS';
 import CONST from '../CONST';
 import platformSetup from './platformSetup';
 import * as Metrics from '../libs/Metrics';
+import '@formatjs/intl-getcanonicallocales/polyfill';
+import '@formatjs/intl-locale/polyfill';
+import '@formatjs/intl-pluralrules/polyfill';
+import '@formatjs/intl-numberformat/polyfill';
+
+// Load en & es Locale data
+import '@formatjs/intl-numberformat/locale-data/en';
+import '@formatjs/intl-numberformat/locale-data/es';
 
 export default function () {
     /*
diff --git a/src/setup/platformSetup/index.native.js b/src/setup/platformSetup/index.native.js
index 89cdb9892e..2ffbdcb036 100644
--- a/src/setup/platformSetup/index.native.js
+++ b/src/setup/platformSetup/index.native.js
@@ -4,16 +4,6 @@ import PushNotification from '../../libs/Notification/PushNotification';
 import * as Report from '../../libs/actions/Report';
 import Performance from '../../libs/Performance';
 
-// we only need polyfills for Mobile.
-import '@formatjs/intl-getcanonicallocales/polyfill';
-import '@formatjs/intl-locale/polyfill';
-import '@formatjs/intl-pluralrules/polyfill';
-import '@formatjs/intl-numberformat/polyfill';
-
-// Load en & es Locale data
-import '@formatjs/intl-numberformat/locale-data/en';
-import '@formatjs/intl-numberformat/locale-data/es';
-
 export default function () {
     // We do not want to send crash reports if we are on a locally built release version of the app.
     // Crashlytics is disabled by default for debug builds, but not local release builds so we are using

Results

2023-01-24.15-10-23.mp4

Solution 2

The only implementation of Intl currently is in this case only, i.e Intl.NumberFormat. We could get rid of Intl and rework NumberFormatUtils.formatToParts and NumberFormatUtils.format to make use of a custom function that maps currency to symbol based on locale. We could also integrate a third party library, like Open Exchange Rates API in the custom function instead of manually creating and maintaining a data source.

If possible, I would like to take a look at the source code of Mobile_GetConstants() as mentioned in this comment

Solution 3

We can simply let this remain as is and not do anything. In the current issue only, the currencies being impacted are relatively less used (i think).

[techievivek]

Great work @Prince-Mendiratta, loved the breakdown that you posted above.

CC @mananjadhav do you mind reviewing the above proposal? IMO I would say solution 1 would be the best here, but curious to hear your thoughts and understand why we never used polyfill for all the platforms. Is it because we only see a need for it now?

[mananjadhav]

@techievivek I think we should post this on the slack? Quick read I agree with Solution 1, but I am not sure why we didn't use polyfills. Also IIRC (I could be wrong), we had polyfills earlier? and we removed them later? Hence better we check on Slack?

[techievivek]

Discussing here: https://expensify.slack.com/archives/C01GTK53T8Q/p1675152607405109

[MelvinBot]

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:

• [@mananjadhav / @techievivek] The PR that introduced the bug has been identified. Link to the PR:
• [@mananjadhav / @techievivek] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@mananjadhav / @techievivek] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@Prince-Mendiratta] Propose regression test steps to ensure the same bug will not reach production again.
• [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Prince-Mendiratta]

Regression test proposal

To be repeated for all platforms:

1. Login to newdot
2. Click on the + icon.
3. Click on send/receive money.
4. Choose the currency as XAF from the currency list.
5. Ensure that the currency displayed is FCFA.

Repeat the above test for the following currencies and ensure that all are displayed correctly:

• XCD - EC$
• XOF - CFA
• XPF - CFPF

[mananjadhav]

@Prince-Mendiratta Minor suggestions/edits.

1. Login to NewDot
2. Click the FAB (+) icon
3. Click on Send Money or Receive Money
4. On the IOU screen, click the currency symbol to open the currency list
5. Choose the currency as XAF from the currency list.
6. Ensure that IOU screen shows up, and the currency displayed is FCFA
7. Successfully complete the IOU transaction with any account
8. Ensure that the correct currency symbol (FCFA) shows up in the chat IOU record.
9. Repeat the steps 3-8 for the currencies with the corresponding expected output: (XCD - EC$, XOF - CFA, XPF - CFPF)

@techievivek @Prince-Mendiratta Do we agree 👍 or 👎

Also I don't think we have a PR to be linked as it is a missing Intl locale data specific to a platform.

[Prince-Mendiratta]

@mananjadhav I support the edits, just to state a relevant detail - The only time the locale data is accessed is when it is selected from the currency list. The currency symbol displayed in the IOU transaction screen is just the string passed around.

[mananjadhav]

The currency symbol displayed in the IOU transaction screen is just the string passed around.

Agreed. Just for the sake of completing the whole workflow.

[mananjadhav]

@techievivek Quick bump on this one.

[mananjadhav]

@techievivek did you get a chance to look at the test steps?

@slafortune this is ready for payout today and wanted to confirm that the PR was merged within 3 business days (excluding weekends), so hoping it is eligible for the timeline bonus.

[techievivek]

@mananjadhav Yeah, it looks good to me. How do we go about adding it? Any idea?

[mananjadhav]

I am not sure. I did see a post on C+ channel about Testrail, but I have never done it here.

Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Based on the checklist, I think @slafortune can help here.

[slafortune]

Hired @mananjadhav (and paid) , @Prince-Mendiratta , and dhanashree-sawant (who I can't tag and don't know why!)

Will look at the regression steps on Monday!

[techievivek]

Not overdue, @slafortune will add the regression steps today.

[Prince-Mendiratta]

Hiya @slafortune, any updates on this one?

[techievivek]

Gentle bump @slafortune to get the regression test added to the testRail.

[slafortune]

Sorry for my delay everyone - done!

[Prince-Mendiratta]

@slafortune Hi, I haven't received the payment yet, can you please recheck once? Just reminding, I believe this issue is eligible for timeline bonus too!

Thanks!

[dhanashree-sawant]

Hi @slafortune , same for me too, recieved the contract but milestone still pending to be approved.

[slafortune]

Hmm- I did receive an error but it appeared to go through - let me check!

[dhanashree-sawant]

Hi @slafortune, can you check once you are back to work?

[slafortune]

Alright - not sure what I messed up to start with, but looks like we should be good now! Shout if that isn't the case @dhanashree-sawant @Prince-Mendiratta

[dhanashree-sawant]

Hi @slafortune , all good, thanks for payment.