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.34k stars 2.77k forks source link

[HOLD for payment 2022-02-01] $1000 - IOU - Decimal separator not updated when set to Spanish on some screens #6427

Closed kavimuru closed 2 years ago

kavimuru commented 2 years 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!


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?

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

songlang1994 commented 2 years ago

@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)}
  2. 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;
        }
    }
    
    // ...
    }
  3. 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 commented 2 years ago

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

parasharrajat commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

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

songlang1994 commented 2 years ago

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 commented 2 years ago

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

songlang1994 commented 2 years ago

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

bfitzexpensify commented 2 years ago

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.

songlang1994 commented 2 years ago

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

botify commented 2 years ago

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

bfitzexpensify commented 2 years ago

Paid out contracts. Thanks @songlang1994!