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

Fix currency handling - always handle backend amounts as if they are in cents #24175

Merged
merged 16 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 20 additions & 25 deletions src/libs/CurrencyUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Onyx.connect({
*/
function getCurrencyDecimals(currency = CONST.CURRENCY.USD) {
const decimals = lodashGet(currencyList, [currency, 'decimals']);
return _.isUndefined(decimals) ? 2 : Math.min(decimals, 2);
return _.isUndefined(decimals) ? 2 : decimals;
}

/**
Expand Down Expand Up @@ -73,54 +73,49 @@ function isCurrencySymbolLTR(currencyCode) {
}

/**
* Takes an amount as a floating point number and converts it to an integer amount.
* For example, given [25, USD], return 2500.
* Given [25.50, USD] return 2550.
* Given [2500, JPY], return 2500.
* Takes an amount as a floating point number and converts it to an integer equivalent to the amount in "cents".
* This is because the backend always stores amounts in "cents". The backend works in integer cents to avoid precision errors
* when doing math operations.
*
* @note we do not currently support any currencies with more than two decimal places. Sorry Tunisia :(
* @note we do not currently support any currencies with more than two decimal places. Decimal past the second place will be rounded. Sorry Tunisia :(
*
* @param {String} currency
* @param {Number} amountAsFloat
* @returns {Number}
*/
function convertToSmallestUnit(currency, amountAsFloat) {
const currencyUnit = getCurrencyUnit(currency);
// We round off the number to resolve floating-point precision issues.
return Math.round(amountAsFloat * currencyUnit);
function convertToBackendAmount(amountAsFloat) {
return Math.round(amountAsFloat * 100);
}

/**
* Takes an amount as an integer and converts it to a floating point amount.
* For example, give [25, USD], return 0.25
* Given [2550, USD], return 25.50
* Given [2500, JPY], return 2500
* Takes an amount in "cents" as an integer and converts it to a floating point amount used in the frontend.
*
* @note we do not support any currencies with more than two decimal places.
*
* @param {String} currency
* @param {Number} amountAsInt
* @returns {Number}
*/
function convertToWholeUnit(currency, amountAsInt) {
const currencyUnit = getCurrencyUnit(currency);
return Math.trunc(amountAsInt) / currencyUnit;
function convertToFrontendAmount(amountAsInt) {
return Math.trunc(amountAsInt) / 100.0;
}

/**
* Given an amount in the smallest units of a currency, convert it to a string for display in the UI.
* Given an amount in the "cents", convert it to a string for display in the UI.
* The backend always handle things in "cents" (subunit equal to 1/100)
*
* @param {Number} amountInSmallestUnit – should be an integer. Anything after a decimal place will be dropped.
* @param {Number} amountInCents – should be an integer. Anything after a decimal place will be dropped.
* @param {String} currency
* @returns {String}
*/
function convertToDisplayString(amountInSmallestUnit, currency = CONST.CURRENCY.USD) {
const currencyUnit = getCurrencyUnit(currency);
const convertedAmount = Math.trunc(amountInSmallestUnit) / currencyUnit;
function convertToDisplayString(amountInCents, currency = CONST.CURRENCY.USD) {
const convertedAmount = convertToFrontendAmount(amountInCents);
return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, {
style: 'currency',
currency,

// We are forcindg the number of decimals because we overridden the default number of decimals in the backend for RSD
aldo-expensify marked this conversation as resolved.
Show resolved Hide resolved
// See: https://github.com/Expensify/PHP-Libs/pull/834
minimumFractionDigits: currency === 'RSD' ? getCurrencyDecimals(currency) : undefined,
Copy link
Contributor Author

@aldo-expensify aldo-expensify Aug 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library used in the backend and the library used in the frontend (Intl) format the RSD currency with 0 decimals. A client needed decimals, so we updated the backend to display RSD with two decimals.
Now, we have to force the frontend to also do the same so we don't get bug reports because of inconsistent behaviour.

I ONLY enabled this for RSD because otherwise the formatting will be inconsistent between backend and frontend for currencies with more than 2 decimals.

});
}

export {getCurrencyDecimals, getCurrencyUnit, getLocalizedCurrencySymbol, isCurrencySymbolLTR, convertToSmallestUnit, convertToWholeUnit, convertToDisplayString};
export {getCurrencyDecimals, getCurrencyUnit, getLocalizedCurrencySymbol, isCurrencySymbolLTR, convertToBackendAmount, convertToFrontendAmount, convertToDisplayString};
15 changes: 1 addition & 14 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import * as Expensicons from '../components/Icon/Expensicons';
import Navigation from './Navigation/Navigation';
import ROUTES from '../ROUTES';
import * as NumberUtils from './NumberUtils';
import * as NumberFormatUtils from './NumberFormatUtils';
import * as ReportActionsUtils from './ReportActionsUtils';
import Permissions from './Permissions';
import DateUtils from './DateUtils';
Expand Down Expand Up @@ -39,17 +38,6 @@ Onyx.connect({
},
});

let preferredLocale = CONST.LOCALES.DEFAULT;
Onyx.connect({
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
callback: (val) => {
if (!val) {
return;
}
preferredLocale = val;
},
});

let allPersonalDetails;
let currentUserPersonalDetails;
Onyx.connect({
Expand Down Expand Up @@ -1724,8 +1712,7 @@ function buildOptimisticExpenseReport(chatReportID, policyID, payeeAccountID, to
* @returns {Array}
*/
function getIOUReportActionMessage(type, total, comment, currency, paymentType = '', isSettlingUp = false) {
const currencyUnit = CurrencyUtils.getCurrencyUnit(currency);
const amount = NumberFormatUtils.format(preferredLocale, Math.abs(total) / currencyUnit, {style: 'currency', currency});
const amount = CurrencyUtils.convertToDisplayString(total, currency);
let paymentMethodMessage;
switch (paymentType) {
case CONST.IOU.PAYMENT_TYPE.ELSEWHERE:
Expand Down
4 changes: 2 additions & 2 deletions src/pages/iou/steps/MoneyRequestAmountForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu

const textInput = useRef(null);

const selectedAmountAsString = amount ? CurrencyUtils.convertToWholeUnit(currency, amount).toString() : '';
const selectedAmountAsString = amount ? CurrencyUtils.convertToFrontendAmount(amount).toString() : '';

const [currentAmount, setCurrentAmount] = useState(selectedAmountAsString);
const [shouldUpdateSelection, setShouldUpdateSelection] = useState(true);
Expand Down Expand Up @@ -102,7 +102,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu
if (!currencyCode || !amountInCurrencyUnits) {
return;
}
const amountAsStringForState = CurrencyUtils.convertToWholeUnit(currencyCode, amountInCurrencyUnits).toString();
const amountAsStringForState = CurrencyUtils.convertToFrontendAmount(amountInCurrencyUnits).toString();
setCurrentAmount(amountAsStringForState);
setSelection({
start: amountAsStringForState.length,
Expand Down
2 changes: 1 addition & 1 deletion src/pages/iou/steps/NewRequestAmountPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ function NewRequestAmountPage({route, iou, report, errors}) {
};

const navigateToNextPage = (currentAmount) => {
const amountInSmallestCurrencyUnits = CurrencyUtils.convertToSmallestUnit(currency, Number.parseFloat(currentAmount));
const amountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(currentAmount));
IOU.setMoneyRequestAmount(amountInSmallestCurrencyUnits);
IOU.setMoneyRequestCurrency(currency);

Expand Down
60 changes: 32 additions & 28 deletions tests/unit/CurrencyUtilsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,36 +85,36 @@ describe('CurrencyUtils', () => {
});
});

describe('convertToSmallestUnit', () => {
describe('convertToBackendAmount', () => {
test.each([
[CONST.CURRENCY.USD, 25, 2500],
[CONST.CURRENCY.USD, 25.25, 2525],
[CONST.CURRENCY.USD, 25.5, 2550],
[CONST.CURRENCY.USD, 2500, 250000],
[CONST.CURRENCY.USD, 80.6, 8060],
[CONST.CURRENCY.USD, 80.9, 8090],
[CONST.CURRENCY.USD, 80.99, 8099],
['JPY', 25, 25],
['JPY', 25.25, 25],
['JPY', 25.5, 26],
['JPY', 2500, 2500],
['JPY', 80.6, 81],
['JPY', 80.9, 81],
['JPY', 80.99, 81],
])('Correctly converts %s to amount in smallest units', (currency, amount, expectedResult) => {
expect(CurrencyUtils.convertToSmallestUnit(currency, amount)).toBe(expectedResult);
[25, 2500],
[25.25, 2525],
[25.5, 2550],
[2500, 250000],
[80.6, 8060],
[80.9, 8090],
[80.99, 8099],
[25, 2500],
[25.25, 2525],
[25.5, 2550],
[2500, 250000],
[80.6, 8060],
[80.9, 8090],
[80.99, 8099],
])('Correctly converts %s to amount in cents (subunit) handled in backend', (amount, expectedResult) => {
expect(CurrencyUtils.convertToBackendAmount(amount)).toBe(expectedResult);
});
});

describe('convertToWholeUnit', () => {
describe('convertToFrontendAmount', () => {
test.each([
[CONST.CURRENCY.USD, 2500, 25],
[CONST.CURRENCY.USD, 2550, 25.5],
['JPY', 25, 25],
['JPY', 2500, 2500],
['JPY', 25.5, 25],
])('Correctly converts %s to amount in whole units', (currency, amount, expectedResult) => {
expect(CurrencyUtils.convertToWholeUnit(currency, amount)).toBe(expectedResult);
[2500, 25],
[2550, 25.5],
[25, 0.25],
[2500, 25],
[2500.5, 25], // The backend should never send a decimal .5 value
])('Correctly converts %s to amount in units handled in frontend', (amount, expectedResult) => {
expect(CurrencyUtils.convertToFrontendAmount(amount)).toBe(expectedResult);
});
});

Expand All @@ -124,9 +124,13 @@ describe('CurrencyUtils', () => {
[CONST.CURRENCY.USD, 2500, '$25.00'],
[CONST.CURRENCY.USD, 150, '$1.50'],
[CONST.CURRENCY.USD, 250000, '$2,500.00'],
['JPY', 25, '¥25'],
['JPY', 2500, '¥2,500'],
['JPY', 25.5, '¥25'],
['JPY', 2500, '¥25'],
['JPY', 250000, '¥2,500'],
['JPY', 2500.5, '¥25'],
['RSD', 100, 'RSD\xa01.00'],
['RSD', 145, 'RSD\xa01.45'],
['BHD', 12345, 'BHD\xa0123.450'],
['BHD', 1, 'BHD\xa00.010'],
])('Correctly displays %s', (currency, amount, expectedResult) => {
expect(CurrencyUtils.convertToDisplayString(amount, currency)).toBe(expectedResult);
});
Expand Down
1 change: 0 additions & 1 deletion tests/unit/currencyList.json
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,6 @@
"RSD": {
"symbol": "РСД",
"name": "Serbian Dinar",
"decimals": 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ISO4217": "941"
},
"RUB": {
Expand Down