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-02-01] $1000 - IOU - Decimal separator not updated when set to Spanish on some screens #6427

Closed
kavimuru opened this issue Nov 23, 2021 · 62 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Nov 23, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open app and login
  2. Go to profile settings and set language to Español
  3. Request money from any contact in any currency

Expected Result:

All money amounts are localized to Spanish, comma as decimal separator. Eg: C$ 23,85

Actual Result:

Decimal separators are not localized in some screens. They are shown as English, in dots. Eg: C$ 23.85

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.1.16-1
Reproducible in staging?: Yes
Reproducible in production?: yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:

Issue reported by: Applause
Slack conversation:

View all open jobs on GitHub

@MelvinBot
Copy link

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

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Nov 23, 2021
@MelvinBot
Copy link

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

@bfitzexpensify
Copy link
Contributor

Posted to Upwork - job

@MelvinBot
Copy link

Triggered auto assignment to @parasharrajat (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 25, 2021
@MelvinBot
Copy link

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

@anthony-hull
Copy link
Contributor

Proposal

I will format the value of this.state.amount

{this.state.amount}

in WithLocalise

props.numberFormat(
        this.state.amount
        {options},
    );

@parasharrajat
Copy link
Member

@anthony-hull Thanks for the proposal. How would you solve the first screen issue?

I feel that it would inconsistent for the Request Money screen if the user types . from the keypad and , is added on the screen. What do you think @Luke9389 ?

@railway17
Copy link
Contributor

railway17 commented Nov 28, 2021

I am submitting this proposal because I've got an email that was allowed to be hired for another jobs by this pull request approval

1. Replication

I was able to reproduce this issue in Web and Android versions and absolutely sure that this issue will also occur on other platforms.
I am attaching my replication screenshots to let you understand easier (but screenshots are displaying some HTTP requests).
web-issue
android-issue

2. Problem

Mainly, we have 2 problems that are related to this issue.

  • PROBLEM 1 is the WRITING and DISPLAYING issue in the IOUAmountPage (It is used to enter the Split or Request values).
    For now, the current code only allows to enter normal number format by following methods:
    validateAmount(amount) {
        const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,3})?$/, 'i');
        return amount === '' || (decimalNumberRegex.test(amount) && (parseFloat((amount * 
                   100).toFixed(3)).toString().length <= CONST.IOU.AMOUNT_MAX_LENGTH));
    }

    stripCommaFromAmount(amount) {
        return amount.replace(/,/g, '');
    }

We can easily update this module in order to display es format numbers too.
But is suggested to keep the normal number format when it is saved. So I am going to fix it for only displaying value.

  • PROBLEM 2 is GETTING and DISPLAYING in the ReportTransaction, IOUPreview and IOUQuote components.
    Because these values are displayed by Onyx and persistent storage without any conversion but this data is only saved in normal number format. So number format is not changed even though we change the locale.
    While researching, I noticed also another problem regarding this issue.

Requested {currency}{number} from {name} string is also taken from storage, but there is not any conversion module or Localization keys for these words.

I think it is not recommended way to save the values with formatted and is better to reformat the values before displaying in the frontend (after getting the values from storage).

3. Solution.

  • PROBLEM 1: can be fixed like below:
//define state like this.
-            amount: props.selectedAmount,
+            amount: props.selectedAmount ? props.numberFormat(props.selectedAmount) : '',
---
//update validate rule
     validateAmount(amount) {
-        const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,3})?$/, 'i');
-        return amount === '' || (decimalNumberRegex.test(amount) && (parseFloat((amount * 100).toFixed(3)).toString().length <= CONST.IOU.AMOUNT_MAX_LENGTH));
+        let decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,3})?$/, 'i');
+        let enFormattedAmount = amount;
+        if (this.props.preferredLocale == 'es') {
+            enFormattedAmount = amount.replace(/\./g, '').replace(/,/g, '.');
+        }
+        return enFormattedAmount === '' || (decimalNumberRegex.test(enFormattedAmount) && (parseFloat((enFormattedAmount * 100).toFixed(3)).toString().length <= CONST.IOU.AMOUNT_MAX_LENGTH));
     }

     stripCommaFromAmount(amount) {
-        return amount.replace(/,/g, '');
+        const stripped = this.props.preferredLocale == 'es' ? amount.replace(/\./g, '') : amount.replace(/,/g, '');
+        return stripped
     }

----
//format saving values to normal number format
+     getENFormattedAmount() {
+       if(this.props.preferredLocale == 'es') {
+            return this.state.amount.replace(/\./g, '').replace(/,/g, '.');
+        }
+       return this.state.amount;
+    }

---
         <Button
             success
             style={[styles.w100, styles.mt5]}
-            onPress={() => this.props.onStepComplete(this.state.amount)}
+           onPress={() => this.props.onStepComplete(this.getENFormattedAmount())}
             pressOnEnter
-            isDisabled={!this.state.amount.length || parseFloat(this.state.amount) < 0.01}
+            isDisabled={!this.state.amount.length || parseFloat(this.getENFormattedAmount()) < 0.01}
             text={this.props.translate('common.next')}
         />
  • PROBLEM 2: Let me describe how to fix with the ReportTransaction component as an example.
    Because we can use the same conversion method in the 3 parts, I am defining the conversion method in the libs/reportUtils.js. And will display the converted values by using this conversion method.
//ReportTransaction.js
+ import compose from '../libs/compose';
+ import withLocalize from './withLocalize';
+ import * as ReportUtils from './../libs/reportUtils'
---
         <Text style={[styles.chatItemMessage]}>
-              {this.props.action.message[0].text}
+             {ReportUtils.changeNumberFormatFromMessage(this.props.action.message[0].text, this.props.numberFormat)}
          </Text>

---
-export default withOnyx({
-    transactionsBeingRejected: {
-        key: ONYXKEYS.TRANSACTIONS_BEING_REJECTED,
-    },
-})(ReportTransaction);
+export default compose(
+        withLocalize,
+        withOnyx({
+        transactionsBeingRejected: {
+            key: ONYXKEYS.TRANSACTIONS_BEING_REJECTED,
+        },
+    }),
+)(ReportTransaction);

// libs/reportUtils.js

+  function changeNumberFormatFromMessage(messageText, changeNumberFormat) {
+     const numRegex = new RegExp(/[\d,]+[\.\d+]*/g);
+     const matches = messageText.match(numRegex)
+   if (matches) {
+        const matchStrNum = matches[0].replace(/,/g, '');
+       const formattedNumber = changeNumberFormat(matchStrNum);
+        messageText = messageText.replace(numRegex, formattedNumber);
+    }
+    return messageText
+ }

@railway17
Copy link
Contributor

railway17 commented Nov 28, 2021

Well..
@bfitzexpensify

I tried to submit my proposal in upwork but got a strange issue.
It is not loaded while loading send proposal page.
Sometimes it is loaded but Terms form is different from another jobs and can't submit.

image

Maybe, is it related open jobs that I worked for Magnifying icon issue?
Even though I've got the pull request approval and email, still can't send new proposal?

Please help me how can I do

@parasharrajat
Copy link
Member

Looking at the proposals....

@roryabraham
Copy link
Contributor

@railway17 I really liked your thorough summary of the problems, but wasn't a big fan of your proposal for a few reasons:

  1. There was no written summary of your proposed solutions. As a reviewer, it's a lot of work to evaluate proposals by reviewing code snippets. The goal of these proposals is to discuss and agree upon a solution before diving into any code review.
  2. You also didn't address the UX problem that @parasharrajat pointed out in this comment.
  3. It could have been more concise.

That said, here are some high-level goals we should strive for in a solution for this issue:

  1. We should strive to solve this issue on all screens, such that there is no place in our app where we display non-localized floating-point numbers.
  2. We should not store any numeric values within formatted strings in Onyx, because that would prevent us from being able to localize those numbers easily. i.e: don't store Requested $10.50 from Rajat. Instead, just store the number 10.50, and localize it before displaying it.
  3. Numbers should always be stored in US format, regardless of the format they were entered in.
  4. We should leverage generic functions to localize floating point numbers on the front-end.
  5. The BigNumberPad and IOUAmountPage should be localized as well, such that if the locale is es then a user cannot manually enter a ., just as they cannot manually enter a , in the components' current forms.

@Luke9389
Copy link
Contributor

I think we're still looking for a proposal that can encapsulate the points that @roryabraham has mentioned above. Maybe we need a localNumber component or something like that to use.

@railway17
Copy link
Contributor

3. concise
Thank you for your explain, @roryabraham
Let me rewrite the solution with the summary.
As I mentioned, we need to resolve 2 kinds of problems at this issue.

  • Resolve the WRITING and DISPLAYING issue in the IOUAmountPage and BigNumberPad.
    It can be resolved by updating the validateAmount and stripCommaFromAmount methods in IOUAmountPage.js

  • Resolve the GETTING and DISPLAYING issue in the ReportTransaction, IOUPreview, and IOUQuote pages.
    As I mentioned, we need to display numbers before displaying them on these pages because values are saved in US format in the Onyx.
    But for now, those are displaying without any conversion like below:

...
                  {Str.htmlDecode(fragment.text)}
...
                  props.iouReport.cachedTotal ? props.iouReport.cachedTotal.replace(/[()]/g, '') : '';
...
                  {this.props.action.message[0].text}
...

  • For the @parasharrajat 's comment, let me describe my idea.
    I am not sure how Spaineses are typing numbers.
    The problem might occur when they enter long numbers such as 123. 234,56, 34. 252. 323,98... in IOUAmountPage.
    I think we should allow type , they want to enter float value.
    I don't think it is not recommended way to click the , key if they want to enter . or click . if they want to enter ,.

  • For the Requested {number} from {name} string issue, it wasn't reported in this issue.
    Actually, I also think we don't need to save the strings in the Onyx but those are saved now. So, I reported for it in my above proposal. I think Expected Result should be updated or New Issue should be created for this problem.

Thank you.

@parasharrajat
Copy link
Member

I agree with @roryabraham points.
Although the proposal touches most of the code but does not explain it clearly. I also think,

  1. we don't make use of locales in code directly,
     if (this.props.preferredLocale == 'es') {
+            enFormattedAmount = amount.replace(/\./g, '').replace(/,/g, '.');
+        }

we don't do this. The app is not specifically configured for Spanish. There could be many languages. The solution should work with any language that we have or add in the future. for this purpose, we have withLocalize HOC.

The problem might occur when they enter long numbers such as 123. 234,56, 34. 252. 323,98... in IOUAmountPage.
I think we should allow type , they want to enter float value. I don't think it is not recommended way to click the , key if they want to enter . or click . if they want to enter ,.

I think, we are not concerned with the number of segments ATM. Only the decimal part matters. it's totally fine that if you enter 123456.34 and it is shown as 123,456.34 in other parts of the app. But if the decimal is presented as , instead of . it becomes an issue. For E.g. Spanish user typing 123.456 then it's not ~~ 123 instead he would parse it as 123 thousand 4 hundred fifty-six but in real our app would take that as 1 hundred twenty-three point 46. This is a big transactional issue.

@parasharrajat
Copy link
Member

We should not store any numeric values within formatted strings in Onyx, because that would prevent us from being able to localize those numbers easily. i.e: don't store Requested $10.50 from Rajat. Instead, just store the number 10.50, and localize it before displaying it.
For the Requested {number} from {name} string issue, it wasn't reported in this issue.

I think we had another issue to localize IOU strings. cc: @iwiznia

@iwiznia
Copy link
Contributor

iwiznia commented Nov 30, 2021

I agree that onyx should not save localized data.
Not sure what other issue you are referring to, I think you might be thinking about the messages that are generated in the backend? (which we are not localizing now)

@railway17
Copy link
Contributor

I agree that onyx should not save localized data. Not sure what other issue you are referring to, I think you might be thinking about the messages that are generated in the backend? (which we are not localizing now)

Yes, I think these messages are generated from the backend.

@iwiznia
Copy link
Contributor

iwiznia commented Nov 30, 2021

Any texts coming from the server can remain unlocalized, since we yet have to add support for localization in the server, for now only the frontend has the framework to localize things.

@iwiznia
Copy link
Contributor

iwiznia commented Jan 4, 2022

Are we planning to localize the IOU strings shown in the screenshots as part of this issue?

If you are referring to the comment you see in the chat after doing the request, which is generated in the server, then no.

I don't like @songlang1994's proposal of putting the decimal and thousand separator in the translation files, mostly because they are already defined inside Intl.NumberFormat. The proposal from @mdneyazahmad is a bit better, but kind of odd too, I would recommend using formatToParts and passing a number like 1.10, then grab the part that have type set to decimal.

Regarding the big number pad UI, I agree it should display only the decimal separator of the current locale (grabbed using the method described in the paragraph above).

Regarding the physical keyboard, nothing needs to be done there, if you type a number, it shows up, if you type your locale's decimal separator it will add it and if you type anything else, then nothing should happen.

As for how we return the value to a component using the BigNumberPad, it should always be as a number, not as a string formatted number.

@songlang1994
Copy link
Contributor

songlang1994 commented Jan 5, 2022

@iwiznia I've post a new implement above which use Intl.NumberFormat to grab the locale digits. See LocaleDiditUtils in this comment. #6427 (comment)

It's just a demo so I didn't optimize the performance yet.

@iwiznia
Copy link
Contributor

iwiznia commented Jan 5, 2022

Oh, I think I missed that one, sorry. Did not even think about other numbers like arabic.
I think that proposal is good, but will let @parasharrajat and @Luke9389 have the final say, since they are assigned.

@parasharrajat
Copy link
Member

Thanks for the input @iwiznia.

If you are referring to the comment you see in the chat after doing the request, which is generated in the server, then no.

Based on this, We won't update the message that is coming from the backend in https://user-images.githubusercontent.com/43996225/143068980-41848fd5-bc85-4b2b-9a6b-1315610211ff.jpg.

But some parts of it can be localized such as IOU Preview and Header of Split Page. @songlang1994 your proposal is good for BigNumberpad but could you please add more details for these changes as well.

Thanks for your patience and for helping us understand your proposal.

@Luke9389
Copy link
Contributor

Luke9389 commented Jan 5, 2022

First of all, I'd like to thank @songlang1994 for helping us clear this up in this comment.

@parasharrajat has a good point. So far, we haven't addressed how we will localize numbers that are coming from the back-end. I'm assuming @songlang1994 would want to use LocaleDigitUtils for this.

@songlang1994, would you be willing to show a proposal for how that would work?
Thanks for your patience. I think we're almost there.

@songlang1994
Copy link
Contributor

The messages from API are actually TEXT rather than numbers. So I think the localization process should be taken care of by the server.

BTW, IOUPreview re-format the amount TEXT by replacing brackets which may not be a nice way. It might be better to contact the back-end team and ask them to handle it.

// File: src/components/ReportActionItem/IOUPreview.js

// The original `cachedTotal` from API was something like "(HK$63.69)".
// `IOUPreview` convert it to "HK$63.69".
const cachedTotal = props.iouReport.cachedTotal ? props.iouReport.cachedTotal.replace(/[()]/g, '') : '';

iou-components

text-from-server

@parasharrajat
Copy link
Member

yeah, IOUQuote is populated from the backend and we can skip that for now. But we can still change the IOUPreview amount and currency. Look at the IOUBadge for this.

And there is one more screen that needs to be localized the header of
image

It would be great if you can submit a finalized proposal in one place so that others are aware of it.

But Overall, I like @songlang1994 's proposal.

cc: @Luke9389

🎀 👀 🎀 C+ reviewed

@Luke9389
Copy link
Contributor

Luke9389 commented Jan 6, 2022

We're hoping to do localization entirely on the front end. @songlang1994, would you be willing to post the exact response the server is giving? Is it literally returning "Requested HK$12.88 from Test"? If so, maybe we should change the return to be an object, like

{
    type: request,
    amount: 12.58,
    actor: Test
}

That way the front end can localize it more easily.

@Luke9389
Copy link
Contributor

Luke9389 commented Jan 6, 2022

Yep, I'm in the same boat as @parasharrajat. I'm ready to give the green light to @songlang1994 once a final proposal with all front end screens is submitted. It's OK to skip IOUQuote for now. It's a bit out of scope for this issue.

@songlang1994
Copy link
Contributor

Is it literally returning "Requested HK$12.88 from Test"?

@Luke9389 Yes. The data from server related to IOUQuote is

"message": [
    {
        "type": "COMMENT",
        "html": "Requested HK$12.58 from Test",
        "text": "Requested HK$12.58 from Test",
        "isEdited": false
    }
],
Click to expand the full response
// POST /api?command=Report_GetHistory

// [FormData]
//
// reportID: 85812791
// reportActionsOffset: -1
// reportActionsLimit: 50
// shouldRetry: true
// email: songlang1994@outlook.com
// authToken: 01BC5DB123416766C2665414B243244FA25B6CC8666E3C5449D3E3B5C83C03C5F4434C55E7B7047F90C951D410624C855E1FBB6D8A682A9E47B3297182318EF3DDDA6E744F669B0BC28C92DC4778548272BBE38C19B7C63EDC046F1A096B9FFB0FC7E0000943BE6117129D0233FDB6CDC15CB21998F0FE4E9DE7F5AB021E353EB97F55F99ECA36D83CA47E3F77A8F517A742D7837F47EE0E1403DF5A37B2DDB188DCF2930D3A36E94D5A5AC4EBC9D4781FDD382075DE91293901CCAE276198AA08CC22ACB0E7D5564EFA5B1AABB2C31C6FFCF5104BF029C8D0A49D3F8FC1F28E9CFD58A239416CE0045A9D5BC3148C1E9DFBF4DC4FB9A21B7464A332E369E2ECDD1DE117B5AA0CC85D618C6D9E48D5E19FC7F3D8C5585A46A3CC8F603634227DB9CFC3B57BFA9FD95FB97B7AC200D6BE08932CDEA7FBF5B293674FE1D8CE4AD514B61B9E50BB328399EFA525D576BB95
// referer: ecash
// api_setCookie: false

{
  "history": [
    {
      "person": [
        {
          "type": "TEXT",
          "style": "strong",
          "text": "[email protected]"
        }
      ],
      "actorEmail": "[email protected]",
      "actorAccountID": 11012599,
      "message": [
        {
          "type": "COMMENT",
          "html": "Requested HK$12.99 from Test",
          "text": "Requested HK$12.99 from Test",
          "isEdited": false
        }
      ],
      "originalMessage": {
        "IOUReportID": 86311814,
        "IOUTransactionID": "58630061686784005",
        "amount": 1299,
        "comment": "",
        "currency": "HKD",
        "type": "create"
      },
      "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
      "created": "Dec 31 2021 12:05am PST",
      "timestamp": 1640937928,
      "automatic": false,
      "sequenceNumber": 5,
      "actionName": "IOU",
      "shouldShow": true,
      "clientID": "",
      "reportActionID": "917927660"
    },
    {
      "person": [
        {
          "type": "TEXT",
          "style": "strong",
          "text": "[email protected]"
        }
      ],
      "actorEmail": "[email protected]",
      "actorAccountID": 11012599,
      "message": [
        {
          "type": "COMMENT",
          "html": "Requested HK$12.66 from Test",
          "text": "Requested HK$12.66 from Test",
          "isEdited": false
        }
      ],
      "originalMessage": {
        "IOUReportID": 86311814,
        "IOUTransactionID": "58630061686784004",
        "amount": 1266,
        "comment": "",
        "currency": "HKD",
        "type": "create"
      },
      "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
      "created": "Dec 31 2021 12:04am PST",
      "timestamp": 1640937899,
      "automatic": false,
      "sequenceNumber": 4,
      "actionName": "IOU",
      "shouldShow": true,
      "clientID": "",
      "reportActionID": "917927544"
    },
    {
      "person": [
        {
          "type": "TEXT",
          "style": "strong",
          "text": "[email protected]"
        }
      ],
      "actorEmail": "[email protected]",
      "actorAccountID": 11012599,
      "message": [
        {
          "type": "COMMENT",
          "html": "Requested HK$12.88 from Test",
          "text": "Requested HK$12.88 from Test",
          "isEdited": false
        }
      ],
      "originalMessage": {
        "IOUReportID": 86311814,
        "IOUTransactionID": "58630061686784003",
        "amount": 1288,
        "comment": "",
        "currency": "HKD",
        "type": "create"
      },
      "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
      "created": "Dec 31 2021 12:03am PST",
      "timestamp": 1640937829,
      "automatic": false,
      "sequenceNumber": 3,
      "actionName": "IOU",
      "shouldShow": true,
      "clientID": "",
      "reportActionID": "917927269"
    },
    {
      "person": [
        {
          "type": "TEXT",
          "style": "strong",
          "text": "[email protected]"
        }
      ],
      "actorEmail": "[email protected]",
      "actorAccountID": 11012599,
      "message": [
        {
          "type": "COMMENT",
          "html": "Requested HK$12.58 from Test",
          "text": "Requested HK$12.58 from Test",
          "isEdited": false
        }
      ],
      "originalMessage": {
        "IOUReportID": 86311814,
        "IOUTransactionID": "58630061686784002",
        "amount": 1258,
        "comment": "",
        "currency": "HKD",
        "type": "create"
      },
      "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
      "created": "Dec 30 2021 11:38pm PST",
      "timestamp": 1640936298,
      "automatic": false,
      "sequenceNumber": 2,
      "actionName": "IOU",
      "shouldShow": true,
      "clientID": "",
      "reportActionID": "917920802"
    },
    {
      "person": [
        {
          "type": "TEXT",
          "style": "strong",
          "text": "[email protected]"
        }
      ],
      "actorEmail": "[email protected]",
      "actorAccountID": 11012599,
      "message": [
        {
          "type": "COMMENT",
          "html": "Requested HK$12.58 from Test",
          "text": "Requested HK$12.58 from Test",
          "isEdited": false
        }
      ],
      "originalMessage": {
        "IOUReportID": 86311814,
        "IOUTransactionID": "58630061686784001",
        "amount": 1258,
        "comment": "",
        "currency": "HKD",
        "type": "create"
      },
      "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
      "created": "Dec 30 2021 11:32pm PST",
      "timestamp": 1640935967,
      "automatic": false,
      "sequenceNumber": 1,
      "actionName": "IOU",
      "shouldShow": true,
      "clientID": "",
      "reportActionID": "917920460"
    },
    {
      "actionName": "CREATED",
      "created": "Dec 20 2021 10:51pm PST",
      "timestamp": 1640069488,
      "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
      "message": [
        {
          "type": "TEXT",
          "style": "strong",
          "text": "__fake__"
        },
        {
          "type": "TEXT",
          "style": "normal",
          "text": " created this report"
        }
      ],
      "person": [
        {
          "type": "TEXT",
          "style": "strong",
          "text": "__fake__"
        }
      ],
      "automatic": false,
      "sequenceNumber": 0,
      "shouldShow": true
    }
  ],
  "jsonCode": 200,
  "requestID": "6c93e6470c2e3b4c-SJC"
}

@songlang1994
Copy link
Contributor

songlang1994 commented Jan 7, 2022

@parasharrajat IOUBadge and the header you mentioned above has been localized already. So the last place that we need to localize is IOUPreview. This is a finalized proposal base on this proposal (v2). In this proposal I will improve:

  1. Localize amount in IOUPreview.
  2. Add cache in LocaleDigitUtils to optimize the performace.
  3. Transform the entire text in onChangeText callback instead of handing the last input character. This is a better approach which can handling every input event such as historyUndo (User pressed Ctrl+Z), insertFromPaste (User pressed Ctrl+V) and so on. It will allow users to paste from clipboard.

Proposal (v3)

  1. Rename src/libs/numberFormat/index.js to src/libs/NumberFormatUtils/index.js in order to export formatToParts function that we need to use in the following LocaleDigitUtils.js. Of course, other files that reference numberFormat will also be updated.
// File: src/libs/NumberFormatUtils/index.js, src/libs/NumberFormatUtils/index.native.js
function format(locale, number, options) {
    return new Intl.NumberFormat(locale, options).format(number);
}

function formatToParts(locale, number, options) {
    return new Intl.NumberFormat(locale, options).formatToParts(number);
}

export {
    format,
    formatToParts,
};
  1. Create a new LocaleDigitUtils.js. It will manage the relation between localized format digits and standard format digits.
// File: src/libs/LocaleDigitUtils.js
import invariant from 'invariant';
import _ from 'underscore';

import * as NumberFormatUtils from './NumberFormatUtils';

const CACHE = {};

const STANDARD_DIGITS = Object.freeze(['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '.', '-', ',']);

const INDEX_DECIMAL = 10;
const INDEX_MINUS_SIGN = 11;
const INDEX_GROUP = 12;

function getLocaleDigits(locale) {
    if (!CACHE[locale]) {
        const localeDigits = _.clone(STANDARD_DIGITS);
        for (let i = 0; i <= 9; i++) {
            localeDigits[i] = NumberFormatUtils.format(locale, i);
        }
        _.forEach(NumberFormatUtils.formatToParts(locale, 1000000.5), (part) => {
            switch (part.type) {
                case 'decimal':
                    localeDigits[INDEX_DECIMAL] = part.value;
                    break;
                case 'minusSign':
                    localeDigits[INDEX_MINUS_SIGN] = part.value;
                    break;
                case 'group':
                    localeDigits[INDEX_GROUP] = part.value;
                    break;
                default:
                    break;
            }
        });
        CACHE[locale] = Object.freeze(localeDigits);
    }

    return CACHE[locale];
}

function isValidLocaleDigit(locale, digit) {
    return getLocaleDigits(locale).includes(digit);
}

function toLocaleDigit(locale, digit) {
    const index = _.indexOf(STANDARD_DIGITS, digit);
    invariant(index > -1, `"${digit}" must be in ${JSON.stringify(STANDARD_DIGITS)}`);
    return getLocaleDigits(locale)[index];
}

function fromLocaleDigit(locale, localeDigit) {
    const index = _.indexOf(getLocaleDigits(locale), localeDigit);
    invariant(index > -1, `"${localeDigit}" must be in ${JSON.stringify(getLocaleDigits(locale))}`);
    return STANDARD_DIGITS[index];
}

export {
    isValidLocaleDigit,
    toLocaleDigit,
    fromLocaleDigit,
};
  1. Translate every digits including the decimal point in BigNumberPad.js.
  // File: src/components/BigNumberPad.js
  <Button
      key={column}
      style={[styles.flex1, marginLeft]}
-     text={column}
+     text={column === '<' ? column : this.props.toLocaleDigit(column)}
      onLongPress={() => this.handleLongPress(column)}
  1. Convert the text of input to standard number format in onChangeText callback before setState of amount. Then localize amount before pass into TextInputAutoWidth.
// File: src/pages/iou/steps/IOUAmountPage.js
class IOUAmountPage {
    // ...

    get formattedInputingAmount() {
        if (!this.state.amount) {
            return '';
        }

        const endsWithDecimalPoint = this.state.amount.endsWith('.');
        if (endsWithDecimalPoint) {
            const localized = this.props.numberFormat(this.state.amount, {useGrouping: false, minimumFractionDigits: 1});
            return localized.slice(0, -1);
        }

        const fraction = this.state.amount.split('.')[1];
        const fractionDigits = fraction ? fraction.length : 0;
        return this.props.numberFormat(this.state.amount, {useGrouping: false, minimumFractionDigits: fractionDigits});
    }

    updateAmount(text) {
        this.setState((prevState) => {
            const amount = this.tryConvertToStandardDigits(text);
            return amount !== undefined && this.validateAmount(amount)
                ? {amount: this.stripCommaFromAmount(amount)}
                : prevState;
        });
    }

    tryConvertToStandardDigits(localeDigits) {
        try {
            return _.chain([...localeDigits])
                .map(char => this.props.fromLocaleDigit(char))
                .join('')
                .value();
        } catch {
            return undefined;
        }
    }

    // ...
}
  1. Localize amount in IOUPreview.
// File: src/components/ReportActionItem/IOUPreview.js
- const cachedTotal = props.iouReport.cachedTotal ? props.iouReport.cachedTotal.replace(/[()]/g, '') : '';
+ const cachedTotal = props.iouReport.total && props.iouReport.currency
+     ? props.numberFormat(
+         props.iouReport.total / 100, // Divide by 100 because total is in cents
+         {style: 'currency', currency: props.iouReport.currency},
+     ) : '';

@aldo-expensify
Copy link
Contributor

Is Intl.NumberFormat supported enough? https://caniuse.com/?search=NumberFormat

@parasharrajat
Copy link
Member

Is Intl.NumberFormat supported enough? caniuse.com/?search=NumberFormat

Yup.

Overall, the proposal is fine. There are a few modifications and simplifications needed but I will point them in review. Waiting for @Luke9389's decision here...

Changes could be:

Rename src/libs/numberFormat/index.js to src/libs/NumberFormatUtils/index.js in order to export formatToParts function that we need to use in the following LocaleDigitUtils.js. Of course, other files that reference numberFormat will also be updated.

We don't need to rename this. numberFormat lib is inspired from Intl.numberFormat and thus we would like to keep it that way.

const CACHE = {};

_.memoize instead of local cache.

const fraction = this.state.amount.split('.')[1];
const fractionDigits = fraction ? fraction.length : 0;
return this.props.numberFormat(this.state.amount, {useGrouping: false, minimumFractionDigits: fractionDigits});
}

we know that amount will always have at most 2 fraction digits so we can hardcode it.

Missing parts

  1. If I am not mistaken, Localization of Request/Split page header. But this is the easy part.

@Luke9389
Copy link
Contributor

I think I'm actually a fan of the name NumberFormatUtils over numberFormat. I think having the "utils" in there is nice. Is there a reason for your preference aside from referencing Intl.numberFormat @parasharrajat?
Also naming them separately removes any potential ambiguity.

@songlang1994 thank you very much for your patience in developing this proposal. You've got the 🟢 green light 🟢 to start a PR!

@parasharrajat
Copy link
Member

Is there a reason for your preference aside from referencing Intl.numberFormat @parasharrajat?

No. It does not matter much. Happy with anything.

@songlang1994
Copy link
Contributor

Thank you @Luke9389. I will send a PR within 2 days. Should I apply for the Upwork job now? The job link mentioned in this comment is unavailable.

@bfitzexpensify
Copy link
Contributor

Hey @songlang1994, please apply here (I updated the link, but it got lost in the hidden comments in this issue). Thanks!

@songlang1994
Copy link
Contributor

Aha, thank you @bfitzexpensify. I forgot the hidden comments.

@bfitzexpensify
Copy link
Contributor

Deployed to staging. @songlang1994, once this is deployed to production, this issue will automatically update, and then we'll pay out the fee once there have been no regressions after 7 days.

@MelvinBot MelvinBot removed the Overdue label Jan 24, 2022
@songlang1994
Copy link
Contributor

songlang1994 commented Jan 25, 2022

@bfitzexpensify Okay. That sounds good for me. Thank you.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 25, 2022
@botify
Copy link

botify commented Jan 25, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.32-0 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-02-01. 🎊

@botify botify changed the title $1000 - IOU - Decimal separator not updated when set to Spanish on some screens [HOLD for payment 2022-02-01] $1000 - IOU - Decimal separator not updated when set to Spanish on some screens Jan 25, 2022
@bfitzexpensify
Copy link
Contributor

Paid out contracts. Thanks @songlang1994!

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 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests