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: Incorrect bill splits for some currencies #16936

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f8482ea
fix: incorrect bill splits for some currencies
tienifr Apr 4, 2023
36e3c8b
fix eslint
tienifr Apr 4, 2023
8bc9e5e
export function getCurrencyUnits
tienifr Apr 5, 2023
1d5fb24
refactor: remove duplicated logic
tienifr Apr 5, 2023
81f909b
add JSDoc and unit test for getCurrencyDecimals function
tienifr Apr 5, 2023
714c49d
add JSDoc and unit test for getCurrencyUnits function
tienifr Apr 5, 2023
7f3b1ee
Merge branch 'main' of https://github.com/tienifr/App into fix/15878-…
tienifr Apr 13, 2023
075d642
get currency decimals from onyx
tienifr Apr 13, 2023
b4cdbeb
default decimal to 2 for missing currencies
tienifr Apr 14, 2023
1c21bce
refactor for readibility
tienifr Apr 14, 2023
e547012
Merge branch 'main' of https://github.com/tienifr/App into fix/15878-…
tienifr Apr 14, 2023
c48fd57
Merge branch 'main' of https://github.com/tienifr/App into fix/15878-…
tienifr Apr 14, 2023
0486241
update mock currencyList.json and init Onyx for IOUUtilsTest
tienifr Apr 14, 2023
4308726
added unit test for calculateAmount
tienifr Apr 14, 2023
5b5845d
rename getCurrencyUnits to singular
tienifr Apr 15, 2023
d2b28c0
default currency param to USD
tienifr Apr 15, 2023
0f8cf46
keep original indents currencyList.json
tienifr Apr 15, 2023
1a931f1
add edge case unit test for calculateAmount
tienifr Apr 15, 2023
1d73317
add unit test for calculateAmount with currency having two decimals
tienifr Apr 17, 2023
a7359ea
remove shouldConvertTwo2DigitsFormat
tienifr Apr 18, 2023
8d19eab
update unit test for calculateAmount
tienifr Apr 18, 2023
36e33e8
fix json indent
tienifr Apr 18, 2023
1837007
add line break json
tienifr Apr 18, 2023
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
7 changes: 5 additions & 2 deletions src/components/MoneyRequestConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,11 @@ class MoneyRequestConfirmationList extends Component {
* @returns {Array}
*/
getParticipantsWithAmount(participants) {
const iouAmount = IOUUtils.calculateAmount(participants, this.props.iouAmount, this.props.iou.selectedCurrencyCode);

return OptionsListUtils.getIOUConfirmationOptionsFromParticipants(
participants,
this.props.numberFormat(IOUUtils.calculateAmount(participants, this.props.iouAmount) / 100, {
this.props.numberFormat(iouAmount / 100, {
style: 'currency',
currency: this.props.iou.selectedCurrencyCode,
}),
Expand Down Expand Up @@ -172,9 +174,10 @@ class MoneyRequestConfirmationList extends Component {
const formattedUnselectedParticipants = this.getParticipantsWithoutAmount(unselectedParticipants);
const formattedParticipants = _.union(formattedSelectedParticipants, formattedUnselectedParticipants);

const myIOUAmount = IOUUtils.calculateAmount(selectedParticipants, this.props.iouAmount, this.props.iou.selectedCurrencyCode, true);
const formattedMyPersonalDetails = OptionsListUtils.getIOUConfirmationOptionsFromMyPersonalDetail(
this.props.currentUserPersonalDetails,
this.props.numberFormat(IOUUtils.calculateAmount(selectedParticipants, this.props.iouAmount, true) / 100, {
this.props.numberFormat(myIOUAmount / 100, {
style: 'currency',
currency: this.props.iou.selectedCurrencyCode,
}),
Expand Down
64 changes: 55 additions & 9 deletions src/libs/IOUUtils.js
Original file line number Diff line number Diff line change
@@ -1,31 +1,75 @@
import _ from 'underscore';
import Onyx from 'react-native-onyx';
import lodashGet from 'lodash/get';
import CONST from '../CONST';
import ONYXKEYS from '../ONYXKEYS';

let currencyList = {};
Onyx.connect({
key: ONYXKEYS.CURRENCY_LIST,
callback: (val) => {
if (_.isEmpty(val)) {
return;
}

currencyList = val;
},
});

/**
* Returns the number of digits after the decimal separator for a specific currency.
* For currencies that have decimal places > 2, floor to 2 instead:
* https://github.com/Expensify/App/issues/15878#issuecomment-1496291464
*
* @param {String} currency - IOU currency
* @returns {Number}
*/
function getCurrencyDecimals(currency = CONST.CURRENCY.USD) {
const decimals = lodashGet(currencyList, [currency, 'decimals']);
return _.isUndefined(decimals) ? 2 : Math.min(decimals, 2);
}

/**
* Returns the currency's minor unit quantity
* e.g. Cent in USD
*
* @param {String} currency - IOU currency
* @returns {Number}
*/
function getCurrencyUnit(currency = CONST.CURRENCY.USD) {
return 10 ** getCurrencyDecimals(currency);
}

/**
* Calculates the amount per user given a list of participants
* @param {Array} participants - List of logins for the participants in the chat. It should not include the current user's login.
* @param {Number} total - IOU total amount
* @param {String} currency - IOU currency
* @param {Boolean} isDefaultUser - Whether we are calculating the amount for the current user
* @returns {Number}
*/
function calculateAmount(participants, total, isDefaultUser = false) {
function calculateAmount(participants, total, currency, isDefaultUser = false) {
// Convert to cents before working with iouAmount to avoid
// javascript subtraction with decimal problem -- when dealing with decimals,
// because they are encoded as IEEE 754 floating point numbers, some of the decimal
// numbers cannot be represented with perfect accuracy.
// Cents is temporary and there must be support for other currencies in the future
const iouAmount = Math.round(parseFloat(total * 100));
// Currencies that do not have minor units (i.e. no decimal place) are also supported.
// https://github.com/Expensify/App/issues/15878
const currencyUnit = getCurrencyUnit(currency);
const iouAmount = Math.round(parseFloat(total * currencyUnit));

const totalParticipants = participants.length + 1;
const amountPerPerson = Math.round(iouAmount / totalParticipants);

if (!isDefaultUser) {
return amountPerPerson;
}
let finalAmount = amountPerPerson;

const sumAmount = amountPerPerson * totalParticipants;
const difference = iouAmount - sumAmount;
if (isDefaultUser) {
const sumAmount = amountPerPerson * totalParticipants;
const difference = iouAmount - sumAmount;
finalAmount = iouAmount !== sumAmount ? (amountPerPerson + difference) : amountPerPerson;
}

return iouAmount !== sumAmount ? (amountPerPerson + difference) : amountPerPerson;
return (finalAmount * 100) / currencyUnit;
}

/**
Expand Down Expand Up @@ -139,4 +183,6 @@ export {
updateIOUOwnerAndTotal,
getIOUReportActions,
isIOUReportPendingCurrencyConversion,
getCurrencyUnit,
getCurrencyDecimals,
};
4 changes: 2 additions & 2 deletions src/libs/actions/IOU.js
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,8 @@ function createSplitsAndOnyxData(participants, currentUserLogin, amount, comment
];

// Loop through participants creating individual chats, iouReports and reportActionIDs as needed
const splitAmount = IOUUtils.calculateAmount(participants, amount);
const splits = [{email: currentUserEmail, amount: IOUUtils.calculateAmount(participants, amount, true)}];
const splitAmount = IOUUtils.calculateAmount(participants, amount, currency, false);
const splits = [{email: currentUserEmail, amount: IOUUtils.calculateAmount(participants, amount, currency, true)}];

const hasMultipleParticipants = participants.length > 1;
_.each(participants, (participant) => {
Expand Down
106 changes: 87 additions & 19 deletions tests/unit/IOUUtilsTest.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import Onyx from 'react-native-onyx';
import CONST from '../../src/CONST';
import * as IOUUtils from '../../src/libs/IOUUtils';
import * as ReportUtils from '../../src/libs/ReportUtils';
import ONYXKEYS from '../../src/ONYXKEYS';
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
import currencyList from './currencyList.json';

let iouReport;
let reportActions;
Expand Down Expand Up @@ -38,27 +42,37 @@ function cancelMoneyRequest(moneyRequestAction, {isOnline} = {}) {
);
}

beforeEach(() => {
reportActions = [];
const chatReportID = ReportUtils.generateReportID();
const amount = 1000;
const currency = 'USD';

iouReport = ReportUtils.buildOptimisticIOUReport(
ownerEmail,
managerEmail,
amount,
chatReportID,
currency,
CONST.LOCALES.EN,
);

// The starting point of all tests is the IOUReport containing a single non-pending transaction in USD
// All requests in the tests are assumed to be offline, unless isOnline is specified
createIOUReportAction('create', amount, currency, {IOUTransactionID: '', isOnline: true});
});
function initCurrencyList() {
Onyx.init({
keys: ONYXKEYS,
initialKeyStates: {
[ONYXKEYS.CURRENCY_LIST]: currencyList,
},
});
return waitForPromisesToResolve();
}

describe('isIOUReportPendingCurrencyConversion', () => {
beforeEach(() => {
reportActions = [];
const chatReportID = ReportUtils.generateReportID();
const amount = 1000;
const currency = 'USD';

iouReport = ReportUtils.buildOptimisticIOUReport(
ownerEmail,
managerEmail,
amount,
chatReportID,
currency,
CONST.LOCALES.EN,
);

// The starting point of all tests is the IOUReport containing a single non-pending transaction in USD
// All requests in the tests are assumed to be offline, unless isOnline is specified
createIOUReportAction('create', amount, currency, {IOUTransactionID: '', isOnline: true});
});

test('Requesting money offline in a different currency will show the pending conversion message', () => {
// Request money offline in AED
createIOUReportAction('create', 100, 'AED');
Expand Down Expand Up @@ -132,3 +146,57 @@ describe('isIOUReportPendingCurrencyConversion', () => {
});
});

describe('getCurrencyDecimals', () => {
beforeAll(() => initCurrencyList());
test('Currency decimals smaller than or equal 2', () => {
expect(IOUUtils.getCurrencyDecimals('JPY')).toBe(0);
expect(IOUUtils.getCurrencyDecimals('USD')).toBe(2);
});

test('Currency decimals larger than 2 should return 2', () => {
// Actual: 3
expect(IOUUtils.getCurrencyDecimals('LYD')).toBe(2);

// Actual: 4
expect(IOUUtils.getCurrencyDecimals('UYW')).toBe(2);
});
});

describe('getCurrencyUnit', () => {
beforeAll(() => initCurrencyList());
test('Currency with decimals smaller than or equal 2', () => {
expect(IOUUtils.getCurrencyUnit('JPY')).toBe(1);
expect(IOUUtils.getCurrencyUnit('USD')).toBe(100);
});

test('Currency with decimals larger than 2 should be floor to 2', () => {
expect(IOUUtils.getCurrencyUnit('LYD')).toBe(100);
});
});

describe('calculateAmount', () => {
beforeAll(() => initCurrencyList());
test('103 JPY split among 3 participants including the default user should be [35, 34, 34]', () => {
const participants = ['tonystark@expensify.com', 'reedrichards@expensify.com'];
expect(IOUUtils.calculateAmount(participants, 103, 'JPY', true)).toBe(3500);
expect(IOUUtils.calculateAmount(participants, 103, 'JPY')).toBe(3400);
});

test('10 AFN split among 4 participants including the default user should be [1, 3, 3, 3]', () => {
const participants = ['tonystark@expensify.com', 'reedrichards@expensify.com', 'suestorm@expensify.com'];
expect(IOUUtils.calculateAmount(participants, 10, 'AFN', true)).toBe(100);
expect(IOUUtils.calculateAmount(participants, 10, 'AFN')).toBe(300);
});

test('10 BHD split among 3 participants including the default user should be [334, 333, 333]', () => {
const participants = ['tonystark@expensify.com', 'reedrichards@expensify.com'];
expect(IOUUtils.calculateAmount(participants, 10, 'BHD', true)).toBe(334);
expect(IOUUtils.calculateAmount(participants, 10, 'BHD')).toBe(333);
});

test('0.02 USD split among 4 participants including the default user should be [-1, 1, 1, 1]', () => {
const participants = ['tonystark@expensify.com', 'reedrichards@expensify.com', 'suestorm@expensify.com'];
expect(IOUUtils.calculateAmount(participants, 0.02, 'USD', true)).toBe(-1);
expect(IOUUtils.calculateAmount(participants, 0.02, 'USD')).toBe(1);
});
tienifr marked this conversation as resolved.
Show resolved Hide resolved
});
Loading