From b77293ad143e6e71870660f730a3a497f9a3d169 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 4 Aug 2023 16:12:00 -0700 Subject: [PATCH 01/13] Fix currency handling --- src/libs/CurrencyUtils.js | 35 +++++------- src/pages/iou/steps/MoneyRequestAmountForm.js | 4 +- src/pages/iou/steps/NewRequestAmountPage.js | 2 +- tests/unit/CurrencyUtilsTest.js | 56 +++++++++---------- 4 files changed, 44 insertions(+), 53 deletions(-) diff --git a/src/libs/CurrencyUtils.js b/src/libs/CurrencyUtils.js index f8d56f7e60e9..7d6b7d3181a7 100644 --- a/src/libs/CurrencyUtils.js +++ b/src/libs/CurrencyUtils.js @@ -73,54 +73,45 @@ 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 {String} currency * @returns {String} */ function convertToDisplayString(amountInSmallestUnit, currency = CONST.CURRENCY.USD) { - const currencyUnit = getCurrencyUnit(currency); - const convertedAmount = Math.trunc(amountInSmallestUnit) / currencyUnit; + const convertedAmount = convertToFrontendAmount(amountInSmallestUnit); return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, { style: 'currency', currency, }); } -export {getCurrencyDecimals, getCurrencyUnit, getLocalizedCurrencySymbol, isCurrencySymbolLTR, convertToSmallestUnit, convertToWholeUnit, convertToDisplayString}; +export {getCurrencyDecimals, getCurrencyUnit, getLocalizedCurrencySymbol, isCurrencySymbolLTR, convertToBackendAmount, convertToFrontendAmount, convertToDisplayString}; diff --git a/src/pages/iou/steps/MoneyRequestAmountForm.js b/src/pages/iou/steps/MoneyRequestAmountForm.js index 7178ed0e0158..8ad7357c8a45 100644 --- a/src/pages/iou/steps/MoneyRequestAmountForm.js +++ b/src/pages/iou/steps/MoneyRequestAmountForm.js @@ -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(currency, amount).toString() : ''; const [currentAmount, setCurrentAmount] = useState(selectedAmountAsString); const [shouldUpdateSelection, setShouldUpdateSelection] = useState(true); @@ -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(currencyCode, amountInCurrencyUnits).toString(); setCurrentAmount(amountAsStringForState); setSelection({ start: amountAsStringForState.length, diff --git a/src/pages/iou/steps/NewRequestAmountPage.js b/src/pages/iou/steps/NewRequestAmountPage.js index 90cc4c20b837..165c99614092 100644 --- a/src/pages/iou/steps/NewRequestAmountPage.js +++ b/src/pages/iou/steps/NewRequestAmountPage.js @@ -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(currency, Number.parseFloat(currentAmount)); IOU.setMoneyRequestAmount(amountInSmallestCurrencyUnits); IOU.setMoneyRequestCurrency(currency); diff --git a/tests/unit/CurrencyUtilsTest.js b/tests/unit/CurrencyUtilsTest.js index 937bf95044cf..3f265899f3b1 100644 --- a/tests/unit/CurrencyUtilsTest.js +++ b/tests/unit/CurrencyUtilsTest.js @@ -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); }); }); @@ -124,9 +124,9 @@ 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'], ])('Correctly displays %s', (currency, amount, expectedResult) => { expect(CurrencyUtils.convertToDisplayString(amount, currency)).toBe(expectedResult); }); From cf1f0d0e9858c1c83f9343ab041cd8b7cccc7e5a Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 4 Aug 2023 16:29:24 -0700 Subject: [PATCH 02/13] Stop passing currency --- src/pages/iou/steps/MoneyRequestAmountForm.js | 4 ++-- src/pages/iou/steps/NewRequestAmountPage.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pages/iou/steps/MoneyRequestAmountForm.js b/src/pages/iou/steps/MoneyRequestAmountForm.js index 8ad7357c8a45..f6e4fcf8036a 100644 --- a/src/pages/iou/steps/MoneyRequestAmountForm.js +++ b/src/pages/iou/steps/MoneyRequestAmountForm.js @@ -62,7 +62,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu const textInput = useRef(null); - const selectedAmountAsString = amount ? CurrencyUtils.convertToFrontendAmount(currency, amount).toString() : ''; + const selectedAmountAsString = amount ? CurrencyUtils.convertToFrontendAmount(amount).toString() : ''; const [currentAmount, setCurrentAmount] = useState(selectedAmountAsString); const [shouldUpdateSelection, setShouldUpdateSelection] = useState(true); @@ -102,7 +102,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu if (!currencyCode || !amountInCurrencyUnits) { return; } - const amountAsStringForState = CurrencyUtils.convertToFrontendAmount(currencyCode, amountInCurrencyUnits).toString(); + const amountAsStringForState = CurrencyUtils.convertToFrontendAmount(amountInCurrencyUnits).toString(); setCurrentAmount(amountAsStringForState); setSelection({ start: amountAsStringForState.length, diff --git a/src/pages/iou/steps/NewRequestAmountPage.js b/src/pages/iou/steps/NewRequestAmountPage.js index 165c99614092..f280788caf7a 100644 --- a/src/pages/iou/steps/NewRequestAmountPage.js +++ b/src/pages/iou/steps/NewRequestAmountPage.js @@ -147,7 +147,7 @@ function NewRequestAmountPage({route, iou, report, errors}) { }; const navigateToNextPage = (currentAmount) => { - const amountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(currency, Number.parseFloat(currentAmount)); + const amountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(currentAmount)); IOU.setMoneyRequestAmount(amountInSmallestCurrencyUnits); IOU.setMoneyRequestCurrency(currency); From 19210743ebfa3247d50e95da5a9b0a71835f5299 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 4 Aug 2023 16:39:37 -0700 Subject: [PATCH 03/13] Fix offline behaviour --- src/libs/CurrencyUtils.js | 6 +++--- src/libs/ReportUtils.js | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libs/CurrencyUtils.js b/src/libs/CurrencyUtils.js index 7d6b7d3181a7..dd4b19ff4a6a 100644 --- a/src/libs/CurrencyUtils.js +++ b/src/libs/CurrencyUtils.js @@ -102,12 +102,12 @@ function convertToFrontendAmount(amountAsInt) { * 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 convertedAmount = convertToFrontendAmount(amountInSmallestUnit); +function convertToDisplayString(amountInCents, currency = CONST.CURRENCY.USD) { + const convertedAmount = convertToFrontendAmount(amountInCents); return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, { style: 'currency', currency, diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js index 327611fa0191..c18fb1f19727 100644 --- a/src/libs/ReportUtils.js +++ b/src/libs/ReportUtils.js @@ -1724,8 +1724,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: From 538524b0d674edfcf23000f2acf8badf5a439050 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 4 Aug 2023 16:43:38 -0700 Subject: [PATCH 04/13] Remove unused stuff --- src/libs/ReportUtils.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js index c18fb1f19727..7dd2e0a01e2c 100644 --- a/src/libs/ReportUtils.js +++ b/src/libs/ReportUtils.js @@ -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'; @@ -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({ From 5a9d343735a8eab433888d1886e32af38780777b Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 4 Aug 2023 17:52:06 -0700 Subject: [PATCH 05/13] RSD has 2 decimals now --- src/libs/CurrencyUtils.js | 4 ++++ tests/unit/currencyList.json | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libs/CurrencyUtils.js b/src/libs/CurrencyUtils.js index dd4b19ff4a6a..5a7d6ffb7475 100644 --- a/src/libs/CurrencyUtils.js +++ b/src/libs/CurrencyUtils.js @@ -111,6 +111,10 @@ function convertToDisplayString(amountInCents, currency = CONST.CURRENCY.USD) { 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 + // See: https://github.com/Expensify/PHP-Libs/pull/834 + minimumFractionDigits: getCurrencyDecimals(currency), }); } diff --git a/tests/unit/currencyList.json b/tests/unit/currencyList.json index 1242509c0813..d3659d9b1370 100644 --- a/tests/unit/currencyList.json +++ b/tests/unit/currencyList.json @@ -622,7 +622,6 @@ "RSD": { "symbol": "РСД", "name": "Serbian Dinar", - "decimals": 0, "ISO4217": "941" }, "RUB": { From f72fe23dda4e8e450ad58f5f364219d9163d884d Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 4 Aug 2023 18:07:44 -0700 Subject: [PATCH 06/13] More tests / make RSD consistent with backend --- src/libs/CurrencyUtils.js | 4 ++-- tests/unit/CurrencyUtilsTest.js | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libs/CurrencyUtils.js b/src/libs/CurrencyUtils.js index 5a7d6ffb7475..c7ba50ed14e4 100644 --- a/src/libs/CurrencyUtils.js +++ b/src/libs/CurrencyUtils.js @@ -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; } /** @@ -114,7 +114,7 @@ function convertToDisplayString(amountInCents, currency = CONST.CURRENCY.USD) { // We are forcindg the number of decimals because we overridden the default number of decimals in the backend for RSD // See: https://github.com/Expensify/PHP-Libs/pull/834 - minimumFractionDigits: getCurrencyDecimals(currency), + minimumFractionDigits: currency === 'RSD' ? getCurrencyDecimals(currency) : undefined, }); } diff --git a/tests/unit/CurrencyUtilsTest.js b/tests/unit/CurrencyUtilsTest.js index 3f265899f3b1..67a29930e333 100644 --- a/tests/unit/CurrencyUtilsTest.js +++ b/tests/unit/CurrencyUtilsTest.js @@ -127,6 +127,10 @@ describe('CurrencyUtils', () => { ['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); }); From dfbc548d8a0767b0b3e3dc5a991b3a82f9685dde Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 4 Aug 2023 18:16:14 -0700 Subject: [PATCH 07/13] Fix paypal payment url --- src/libs/actions/IOU.js | 3 +-- tests/unit/CurrencyUtilsTest.js | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/IOU.js b/src/libs/actions/IOU.js index 45fcd35eb839..cc8e9b290384 100644 --- a/src/libs/actions/IOU.js +++ b/src/libs/actions/IOU.js @@ -1006,8 +1006,7 @@ function deleteMoneyRequest(transactionID, reportAction, isSingleTransactionView * @returns {String} */ function buildPayPalPaymentUrl(amount, submitterPayPalMeAddress, currency) { - const currencyUnit = CurrencyUtils.getCurrencyUnit(currency); - return `https://paypal.me/${submitterPayPalMeAddress}/${Math.abs(amount) / currencyUnit}${currency}`; + return `https://paypal.me/${submitterPayPalMeAddress}/${Math.abs(amount) / 100}${currency}`; } /** diff --git a/tests/unit/CurrencyUtilsTest.js b/tests/unit/CurrencyUtilsTest.js index 67a29930e333..5058c56bfa00 100644 --- a/tests/unit/CurrencyUtilsTest.js +++ b/tests/unit/CurrencyUtilsTest.js @@ -63,6 +63,7 @@ describe('CurrencyUtils', () => { test('Currency decimals smaller than or equal 2', () => { expect(CurrencyUtils.getCurrencyDecimals('JPY')).toBe(0); expect(CurrencyUtils.getCurrencyDecimals('USD')).toBe(2); + expect(CurrencyUtils.getCurrencyDecimals('RSD')).toBe(2); }); test('Currency decimals larger than 2 should return 2', () => { From a3b3ff752e35aa8880fd8968c296dbba70c357b4 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 11 Aug 2023 18:02:49 -0700 Subject: [PATCH 08/13] Split based on currency --- .../MoneyRequestConfirmationList.js | 4 ++-- src/components/ReportActionItem/IOUPreview.js | 2 +- src/libs/IOUUtils.js | 16 +++++++++------ src/libs/actions/IOU.js | 4 ++-- tests/unit/IOUUtilsTest.js | 20 ++++++++++++------- 5 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/components/MoneyRequestConfirmationList.js b/src/components/MoneyRequestConfirmationList.js index e421dae4e37e..51530233234c 100755 --- a/src/components/MoneyRequestConfirmationList.js +++ b/src/components/MoneyRequestConfirmationList.js @@ -133,7 +133,7 @@ function MoneyRequestConfirmationList(props) { */ const getParticipantsWithAmount = useCallback( (participantsList) => { - const iouAmount = IOUUtils.calculateAmount(participantsList.length, props.iouAmount); + const iouAmount = IOUUtils.calculateAmount(participantsList.length, props.iouAmount, props.iouCurrencyCode); return OptionsListUtils.getIOUConfirmationOptionsFromParticipants(participantsList, CurrencyUtils.convertToDisplayString(iouAmount, props.iouCurrencyCode)); }, [props.iouAmount, props.iouCurrencyCode], @@ -176,7 +176,7 @@ function MoneyRequestConfirmationList(props) { })); } - const myIOUAmount = IOUUtils.calculateAmount(selectedParticipants.length, props.iouAmount, true); + const myIOUAmount = IOUUtils.calculateAmount(selectedParticipants.length, props.iouAmount, props.iouCurrencyCode, true); const formattedPayeeOption = OptionsListUtils.getIOUConfirmationOptionsFromPayeePersonalDetail( payeePersonalDetails, CurrencyUtils.convertToDisplayString(myIOUAmount, props.iouCurrencyCode), diff --git a/src/components/ReportActionItem/IOUPreview.js b/src/components/ReportActionItem/IOUPreview.js index 85a0b22ac327..0aa2aa16524e 100644 --- a/src/components/ReportActionItem/IOUPreview.js +++ b/src/components/ReportActionItem/IOUPreview.js @@ -237,7 +237,7 @@ function IOUPreview(props) { {props.isBillSplit && !_.isEmpty(participantAccountIDs) && ( {props.translate('iou.amountEach', { - amount: CurrencyUtils.convertToDisplayString(IOUUtils.calculateAmount(participantAccountIDs.length - 1, requestAmount), requestCurrency), + amount: CurrencyUtils.convertToDisplayString(IOUUtils.calculateAmount(participantAccountIDs.length - 1, requestAmount, requestCurrency), requestCurrency), })} )} diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js index 241baf26c998..dd96feb0f405 100644 --- a/src/libs/IOUUtils.js +++ b/src/libs/IOUUtils.js @@ -1,25 +1,29 @@ import _ from 'underscore'; import CONST from '../CONST'; import * as ReportActionsUtils from './ReportActionsUtils'; +import * as CurrencyUtils from './CurrencyUtils'; /** * Calculates the amount per user given a list of participants * * @param {Number} numberOfParticipants - Number of participants in the chat. It should not include the current user. - * @param {Number} total - IOU total amount in the smallest units of the currency + * @param {Number} total - IOU total amount in backend format (cents, no matter the currency) + * @param {String} currency - This is used to know how many decimal places are valid to use when splitting the total * @param {Boolean} isDefaultUser - Whether we are calculating the amount for the current user * @returns {Number} */ -function calculateAmount(numberOfParticipants, total, isDefaultUser = false) { +function calculateAmount(numberOfParticipants, total, currency, isDefaultUser = false) { + const currencyUnit = CurrencyUtils.getCurrencyUnit(currency); + const totalInCurrencyUnit = total / 100 * currencyUnit; const totalParticipants = numberOfParticipants + 1; - const amountPerPerson = Math.round(total / totalParticipants); + const amountPerPerson = Math.round(totalInCurrencyUnit / totalParticipants); let finalAmount = amountPerPerson; if (isDefaultUser) { const sumAmount = amountPerPerson * totalParticipants; - const difference = total - sumAmount; - finalAmount = total !== sumAmount ? amountPerPerson + difference : amountPerPerson; + const difference = totalInCurrencyUnit - sumAmount; + finalAmount = totalInCurrencyUnit !== sumAmount ? amountPerPerson + difference : amountPerPerson; } - return finalAmount; + return finalAmount * 100 / currencyUnit; } /** diff --git a/src/libs/actions/IOU.js b/src/libs/actions/IOU.js index cc8e9b290384..f25e2338bf5c 100644 --- a/src/libs/actions/IOU.js +++ b/src/libs/actions/IOU.js @@ -589,8 +589,8 @@ function createSplitsAndOnyxData(participants, currentUserLogin, currentUserAcco } // Loop through participants creating individual chats, iouReports and reportActionIDs as needed - const splitAmount = IOUUtils.calculateAmount(participants.length, amount, false); - const splits = [{email: currentUserEmail, accountID: currentUserAccountID, amount: IOUUtils.calculateAmount(participants.length, amount, true)}]; + const splitAmount = IOUUtils.calculateAmount(participants.length, amount, currency, false); + const splits = [{email: currentUserEmail, accountID: currentUserAccountID, amount: IOUUtils.calculateAmount(participants.length, amount, currency, true)}]; const hasMultipleParticipants = participants.length > 1; _.each(participants, (participant) => { diff --git a/tests/unit/IOUUtilsTest.js b/tests/unit/IOUUtilsTest.js index 1fa7974e762d..9dfa3f5fe058 100644 --- a/tests/unit/IOUUtilsTest.js +++ b/tests/unit/IOUUtilsTest.js @@ -138,20 +138,26 @@ describe('IOUUtils', () => { test('103 JPY split among 3 participants including the default user should be [35, 34, 34]', () => { const participantsAccountIDs = [100, 101]; - expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 103, true)).toBe(35); - expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 103)).toBe(34); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 10300, 'JPY', true)).toBe(3500); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 10300, 'JPY')).toBe(3400); + }); + + test('103 USD split among 3 participants including the default user should be [34.34, 34.33, 34.33]', () => { + const participantsAccountIDs = [100, 101]; + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 10300, 'USD', true)).toBe(3434); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 10300, 'USD')).toBe(3433); }); test('10 AFN split among 4 participants including the default user should be [1, 3, 3, 3]', () => { const participantsAccountIDs = [100, 101, 102]; - expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 10, true)).toBe(1); - expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 10)).toBe(3); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 1000, 'AFN', true)).toBe(100); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 1000, 'AFN')).toBe(300); }); - test('0.02 USD split among 4 participants including the default user should be [-1, 1, 1, 1]', () => { + test('0.02 USD split among 4 participants including the default user should be [-0.01, 0.01, 0.01, 0.01]', () => { const participantsAccountIDs = [100, 101, 102]; - expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 2, true)).toBe(-1); - expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 2)).toBe(1); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 2, 'USD', true)).toBe(-1); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 2, 'USD')).toBe(1); }); }); }); From 96e6657a6b4049ce6f0ec3966d9f58d1c8a7a456 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 11 Aug 2023 18:04:37 -0700 Subject: [PATCH 09/13] Style --- src/libs/IOUUtils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js index dd96feb0f405..ed7ef0cf215d 100644 --- a/src/libs/IOUUtils.js +++ b/src/libs/IOUUtils.js @@ -14,7 +14,7 @@ import * as CurrencyUtils from './CurrencyUtils'; */ function calculateAmount(numberOfParticipants, total, currency, isDefaultUser = false) { const currencyUnit = CurrencyUtils.getCurrencyUnit(currency); - const totalInCurrencyUnit = total / 100 * currencyUnit; + const totalInCurrencyUnit = (total / 100) * currencyUnit; const totalParticipants = numberOfParticipants + 1; const amountPerPerson = Math.round(totalInCurrencyUnit / totalParticipants); let finalAmount = amountPerPerson; @@ -23,7 +23,7 @@ function calculateAmount(numberOfParticipants, total, currency, isDefaultUser = const difference = totalInCurrencyUnit - sumAmount; finalAmount = totalInCurrencyUnit !== sumAmount ? amountPerPerson + difference : amountPerPerson; } - return finalAmount * 100 / currencyUnit; + return (finalAmount * 100) / currencyUnit; } /** From 45dfc062ca49c1073129c49e3bc5dfe47aefdba9 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Fri, 11 Aug 2023 18:16:49 -0700 Subject: [PATCH 10/13] Cap decimal places on split and add tests --- src/libs/IOUUtils.js | 4 +++- tests/unit/IOUUtilsTest.js | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js index ed7ef0cf215d..508da2f9e290 100644 --- a/src/libs/IOUUtils.js +++ b/src/libs/IOUUtils.js @@ -13,7 +13,9 @@ import * as CurrencyUtils from './CurrencyUtils'; * @returns {Number} */ function calculateAmount(numberOfParticipants, total, currency, isDefaultUser = false) { - const currencyUnit = CurrencyUtils.getCurrencyUnit(currency); + // Since the backend can maximum store 2 decimal places, any currency with more than 2 decimals + // has to be capped to 2 decimal places + const currencyUnit = Math.min(100, CurrencyUtils.getCurrencyUnit(currency)); const totalInCurrencyUnit = (total / 100) * currencyUnit; const totalParticipants = numberOfParticipants + 1; const amountPerPerson = Math.round(totalInCurrencyUnit / totalParticipants); diff --git a/tests/unit/IOUUtilsTest.js b/tests/unit/IOUUtilsTest.js index 9dfa3f5fe058..f21156a5a8b6 100644 --- a/tests/unit/IOUUtilsTest.js +++ b/tests/unit/IOUUtilsTest.js @@ -159,6 +159,20 @@ describe('IOUUtils', () => { expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 2, 'USD', true)).toBe(-1); expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 2, 'USD')).toBe(1); }); + + test('1 RSD split among 3 participants including the default user should be [0.34, 0.33, 0.33]', () => { + // RSD is a special case that we forced to have 2 decimals + const participantsAccountIDs = [100, 101]; + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 100, 'RSD', true)).toBe(34); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 100, 'RSD')).toBe(33); + }); + + test('1 BHD split among 3 participants including the default user should be [0.34, 0.33, 0.33]', () => { + // BHD has 3 decimal places, but it still produces parts with only 2 decimal places because of a backend limitation + const participantsAccountIDs = [100, 101]; + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 100, 'BHD', true)).toBe(34); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 100, 'BHD')).toBe(33); + }); }); }); From 8c7770d4726594eacd2b67614042c8485eb02760 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Tue, 15 Aug 2023 11:32:56 -0700 Subject: [PATCH 11/13] Avoid precision errors and add test --- src/libs/IOUUtils.js | 10 +++++----- tests/unit/IOUUtilsTest.js | 12 ++++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js index 508da2f9e290..5d2b4a7d8578 100644 --- a/src/libs/IOUUtils.js +++ b/src/libs/IOUUtils.js @@ -16,16 +16,16 @@ function calculateAmount(numberOfParticipants, total, currency, isDefaultUser = // Since the backend can maximum store 2 decimal places, any currency with more than 2 decimals // has to be capped to 2 decimal places const currencyUnit = Math.min(100, CurrencyUtils.getCurrencyUnit(currency)); - const totalInCurrencyUnit = (total / 100) * currencyUnit; + const totalInCurrencySubunit = Math.round((total / 100) * currencyUnit); const totalParticipants = numberOfParticipants + 1; - const amountPerPerson = Math.round(totalInCurrencyUnit / totalParticipants); + const amountPerPerson = Math.round(totalInCurrencySubunit / totalParticipants); let finalAmount = amountPerPerson; if (isDefaultUser) { const sumAmount = amountPerPerson * totalParticipants; - const difference = totalInCurrencyUnit - sumAmount; - finalAmount = totalInCurrencyUnit !== sumAmount ? amountPerPerson + difference : amountPerPerson; + const difference = totalInCurrencySubunit - sumAmount; + finalAmount = totalInCurrencySubunit !== sumAmount ? amountPerPerson + difference : amountPerPerson; } - return (finalAmount * 100) / currencyUnit; + return Math.round(finalAmount * 100 / currencyUnit); } /** diff --git a/tests/unit/IOUUtilsTest.js b/tests/unit/IOUUtilsTest.js index f21156a5a8b6..0d40ee87424c 100644 --- a/tests/unit/IOUUtilsTest.js +++ b/tests/unit/IOUUtilsTest.js @@ -154,6 +154,18 @@ describe('IOUUtils', () => { expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 1000, 'AFN')).toBe(300); }); + test('10.12 USD split among 4 participants including the default user should be [2.53, 2.53, 2.53, 2.53]', () => { + const participantsAccountIDs = [100, 101, 102]; + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 1012, 'USD', true)).toBe(253); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 1012, 'USD')).toBe(253); + }); + + test('10.12 USD split among 3 participants including the default user should be [3.38, 3.37, 3.37]', () => { + const participantsAccountIDs = [100, 102]; + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 1012, 'USD', true)).toBe(338); + expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 1012, 'USD')).toBe(337); + }); + test('0.02 USD split among 4 participants including the default user should be [-0.01, 0.01, 0.01, 0.01]', () => { const participantsAccountIDs = [100, 101, 102]; expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 2, 'USD', true)).toBe(-1); From 3d6f57a699a5df9b636e2aafe2d9fde845a1f6e0 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Tue, 15 Aug 2023 11:51:53 -0700 Subject: [PATCH 12/13] Style --- src/libs/IOUUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/IOUUtils.js b/src/libs/IOUUtils.js index 5d2b4a7d8578..91e3b09a587c 100644 --- a/src/libs/IOUUtils.js +++ b/src/libs/IOUUtils.js @@ -25,7 +25,7 @@ function calculateAmount(numberOfParticipants, total, currency, isDefaultUser = const difference = totalInCurrencySubunit - sumAmount; finalAmount = totalInCurrencySubunit !== sumAmount ? amountPerPerson + difference : amountPerPerson; } - return Math.round(finalAmount * 100 / currencyUnit); + return Math.round((finalAmount * 100) / currencyUnit); } /** From 1d465e233d3bcc8a49a73698f0f345ca70e54866 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Garay <87341702+aldo-expensify@users.noreply.github.com> Date: Wed, 16 Aug 2023 10:35:27 -0700 Subject: [PATCH 13/13] Fix comment Co-authored-by: Maria D'Costa --- src/libs/CurrencyUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/CurrencyUtils.js b/src/libs/CurrencyUtils.js index c7ba50ed14e4..271086ede36e 100644 --- a/src/libs/CurrencyUtils.js +++ b/src/libs/CurrencyUtils.js @@ -112,7 +112,7 @@ function convertToDisplayString(amountInCents, currency = CONST.CURRENCY.USD) { style: 'currency', currency, - // We are forcindg the number of decimals because we overridden the default number of decimals in the backend for RSD + // We are forcing the number of decimals because we override the default number of decimals in the backend for RSD // See: https://github.com/Expensify/PHP-Libs/pull/834 minimumFractionDigits: currency === 'RSD' ? getCurrencyDecimals(currency) : undefined, });