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

Use suitable decimals while calculating tax #43948

Merged
merged 18 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion src/components/MoneyRequestAmountInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ const getNewSelection = (oldSelection: Selection, prevLength: number, newLength:
return {start: cursorPosition, end: cursorPosition};
};

const defaultOnFormatAmount = (amount: number) => CurrencyUtils.convertToFrontendAmountAsString(amount);
const defaultOnFormatAmount = (amount: number, currency?: string) => CurrencyUtils.convertToFrontendAmountAsString(amount, currency ?? CONST.CURRENCY.USD);

function MoneyRequestAmountInput(
{
Expand Down
4 changes: 2 additions & 2 deletions src/components/MoneyRequestConfirmationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -329,10 +329,10 @@ function MoneyRequestConfirmationList({
taxCode = transaction?.taxCode ?? TransactionUtils.getDefaultTaxCode(policy, transaction) ?? '';
}
const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, taxCode) ?? '';
const taxAmount = TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount);
const taxAmount = TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount, currency);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ShridharGoel

Why did we use currency and not transcation.currency? currency seems to be the currency associated to the mileage rate or the policy: (mileageRate as MileageRate)?.currency ?? policyCurrency;. What if the transaction is not a distance transaction and it has a different currency from the policy default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aldo-expensify This makes sense, we can update it to use transaction.currency instead. Should I open a PR for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I'll update it here: #43519

const taxAmountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(taxAmount.toString()));
IOU.setMoneyRequestTaxAmount(transaction?.transactionID ?? '', taxAmountInSmallestCurrencyUnits);
}, [policy, shouldShowTax, previousTransactionAmount, previousTransactionCurrency, transaction, isDistanceRequest, customUnitRateID]);
}, [policy, shouldShowTax, previousTransactionAmount, previousTransactionCurrency, transaction, isDistanceRequest, customUnitRateID, currency]);

// If completing a split expense fails, set didConfirm to false to allow the user to edit the fields again
if (isEditingSplitBill && didConfirm) {
Expand Down
16 changes: 9 additions & 7 deletions src/libs/CurrencyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,20 +87,22 @@ function convertToBackendAmount(amountAsFloat: number): number {
*
* @note we do not support any currencies with more than two decimal places.
*/
function convertToFrontendAmountAsInteger(amountAsInt: number): number {
return Math.trunc(amountAsInt) / 100.0;
function convertToFrontendAmountAsInteger(amountAsInt: number, currency: string = CONST.CURRENCY.USD): number {
const decimals = getCurrencyDecimals(currency);
return Number((Math.trunc(amountAsInt) / 100.0).toFixed(decimals));
}

/**
* Takes an amount in "cents" as an integer and converts it to a string amount used in the frontend.
*
* @note we do not support any currencies with more than two decimal places.
*/
function convertToFrontendAmountAsString(amountAsInt: number | null | undefined): string {
function convertToFrontendAmountAsString(amountAsInt: number | null | undefined, currency: string = CONST.CURRENCY.USD): string {
if (amountAsInt === null || amountAsInt === undefined) {
return '';
}
return convertToFrontendAmountAsInteger(amountAsInt).toFixed(2);
const decimals = getCurrencyDecimals(currency);
return convertToFrontendAmountAsInteger(amountAsInt, currency).toFixed(decimals);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to use toFixed(decimals) here? we just used toFixed(decimals) in convertToFrontendAmountAsInteger

Copy link
Contributor

Choose a reason for hiding this comment

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

@ShridharGoel How about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to convert it to a String (we can also use just the toString() method).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use toString method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be needed because in 2 decimals, Number changes 25.00 to 25. We again need to change it back to 25.00 when calling convertToFrontendAmountAsString.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, make sense

}

/**
Expand All @@ -111,7 +113,7 @@ function convertToFrontendAmountAsString(amountAsInt: number | null | undefined)
* @param currency - IOU currency
*/
function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURRENCY.USD): string {
const convertedAmount = convertToFrontendAmountAsInteger(amountInCents);
const convertedAmount = convertToFrontendAmountAsInteger(amountInCents, currency);
/**
* Fallback currency to USD if it empty string or undefined
*/
Expand All @@ -137,7 +139,7 @@ function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURR
* @param currency - IOU currency
*/
function convertToShortDisplayString(amountInCents = 0, currency: string = CONST.CURRENCY.USD): string {
const convertedAmount = convertToFrontendAmountAsInteger(amountInCents);
const convertedAmount = convertToFrontendAmountAsInteger(amountInCents, currency);

return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, {
style: 'currency',
Expand Down Expand Up @@ -168,7 +170,7 @@ function convertAmountToDisplayString(amount = 0, currency: string = CONST.CURRE
* Acts the same as `convertAmountToDisplayString` but the result string does not contain currency
*/
function convertToDisplayStringWithoutCurrency(amountInCents: number, currency: string = CONST.CURRENCY.USD) {
const convertedAmount = convertToFrontendAmountAsInteger(amountInCents);
const convertedAmount = convertToFrontendAmountAsInteger(amountInCents, currency);
return NumberFormatUtils.formatToParts(BaseLocaleListener.getPreferredLocale(), convertedAmount, {
style: 'currency',
currency,
Expand Down
7 changes: 5 additions & 2 deletions src/libs/TransactionUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type {Comment, Receipt, TransactionChanges, TransactionPendingFieldsKey,
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type {IOURequestType} from './actions/IOU';
import {isCorporateCard, isExpensifyCard} from './CardUtils';
import {getCurrencyDecimals} from './CurrencyUtils';
import DateUtils from './DateUtils';
import * as Localize from './Localize';
import * as NumberUtils from './NumberUtils';
Expand Down Expand Up @@ -714,9 +715,11 @@ function hasWarningTypeViolation(transactionID: string, transactionViolations: O
/**
* Calculates tax amount from the given expense amount and tax percentage
*/
function calculateTaxAmount(percentage: string, amount: number) {
function calculateTaxAmount(percentage: string, amount: number, currency: string) {
const divisor = Number(percentage.slice(0, -1)) / 100 + 1;
return Math.round(amount - amount / divisor) / 100;
const taxAmount = (amount - amount / divisor) / 100;
const decimals = getCurrencyDecimals(currency);
return parseFloat(taxAmount.toFixed(decimals));
}

/**
Expand Down
25 changes: 14 additions & 11 deletions src/pages/iou/MoneyRequestAmountForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ type MoneyRequestAmountFormProps = {
};

const isAmountInvalid = (amount: string) => !amount.length || parseFloat(amount) < 0.01;
const isTaxAmountInvalid = (currentAmount: string, taxAmount: number, isTaxAmountForm: boolean) =>
isTaxAmountForm && Number.parseFloat(currentAmount) > CurrencyUtils.convertToFrontendAmountAsInteger(Math.abs(taxAmount));
const isTaxAmountInvalid = (currentAmount: string, taxAmount: number, isTaxAmountForm: boolean, currency: string) =>
isTaxAmountForm && Number.parseFloat(currentAmount) > CurrencyUtils.convertToFrontendAmountAsInteger(Math.abs(taxAmount), currency);

const AMOUNT_VIEW_ID = 'amountView';
const NUM_PAD_CONTAINER_VIEW_ID = 'numPadContainerView';
Expand Down Expand Up @@ -148,14 +148,17 @@ function MoneyRequestAmountForm(
});
}, [isFocused, wasFocused]);

const initializeAmount = useCallback((newAmount: number) => {
const frontendAmount = newAmount ? CurrencyUtils.convertToFrontendAmountAsString(newAmount) : '';
moneyRequestAmountInput.current?.changeAmount(frontendAmount);
moneyRequestAmountInput.current?.changeSelection({
start: frontendAmount.length,
end: frontendAmount.length,
});
}, []);
const initializeAmount = useCallback(
(newAmount: number) => {
const frontendAmount = newAmount ? CurrencyUtils.convertToFrontendAmountAsString(newAmount, currency) : '';
moneyRequestAmountInput.current?.changeAmount(frontendAmount);
moneyRequestAmountInput.current?.changeSelection({
start: frontendAmount.length,
end: frontendAmount.length,
});
},
[currency],
);

useEffect(() => {
if (!currency || typeof amount !== 'number') {
Expand Down Expand Up @@ -218,7 +221,7 @@ function MoneyRequestAmountForm(
return;
}

if (isTaxAmountInvalid(currentAmount, taxAmount, isTaxAmountForm)) {
if (isTaxAmountInvalid(currentAmount, taxAmount, isTaxAmountForm, currency)) {
setFormError(translate('iou.error.invalidTaxAmount', {amount: formattedTaxAmount}));
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/iou/request/step/IOURequestStepAmount.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ function IOURequestStepAmount({
const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, currentTransaction, currency) ?? '';
const taxCode = (currency !== transactionCurrency ? defaultTaxCode : transactionTaxCode) ?? defaultTaxCode;
const taxPercentage = TransactionUtils.getTaxValue(policy, currentTransaction, taxCode) ?? '';
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, newAmount));
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, newAmount, currency ?? CONST.CURRENCY.USD));

if (isSplitBill) {
IOU.setDraftSplitTransaction(transactionID, {amount: newAmount, currency, taxCode, taxAmount});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ function IOURequestStepDistanceRate({
const taxRateExternalID = policyCustomUnitRate?.attributes?.taxRateExternalID ?? '-1';
const taxableAmount = DistanceRequestUtils.getTaxableAmount(policy, customUnitRateID, TransactionUtils.getDistance(transaction));
const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, taxRateExternalID) ?? '';
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount));
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount, rates[customUnitRateID].currency ?? CONST.CURRENCY.USD));
IOU.setMoneyRequestTaxAmount(transactionID, taxAmount);
IOU.setMoneyRequestTaxRate(transactionID, taxRateExternalID);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ function getTaxAmount(transaction: OnyxEntry<Transaction>, policy: OnyxEntry<Pol
const moneyRequestTaxPercentage = (transactionTaxCode ? getTaxValue(transactionTaxCode) : defaultTaxValue) ?? '';
const editingTaxPercentage = (transactionTaxCode ? getTaxValue(transactionTaxCode) : moneyRequestTaxPercentage) ?? '';
const taxPercentage = isEditing ? editingTaxPercentage : moneyRequestTaxPercentage;
return CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, transactionTaxAmount));
return CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, transactionTaxAmount, currency ?? CONST.CURRENCY.USD));
}

function IOURequestStepTaxAmountPage({
Expand Down
3 changes: 2 additions & 1 deletion src/pages/iou/request/step/IOURequestStepTaxRatePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as CurrencyUtils from '@libs/CurrencyUtils';
import Navigation from '@libs/Navigation/Navigation';
import type {TaxRatesOption} from '@libs/OptionsListUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
import {getCurrency} from '@libs/TransactionUtils';
import * as IOU from '@userActions/IOU';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand Down Expand Up @@ -38,7 +39,7 @@ function getTaxAmount(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transact
const getTaxValue = (taxCode: string) => TransactionUtils.getTaxValue(policy, transaction, taxCode);
const taxPercentage = getTaxValue(selectedTaxCode);
if (taxPercentage) {
return TransactionUtils.calculateTaxAmount(taxPercentage, amount);
return TransactionUtils.calculateTaxAmount(taxPercentage, amount, getCurrency(transaction));
}
}

Expand Down
45 changes: 29 additions & 16 deletions tests/unit/CurrencyUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,27 +107,40 @@ describe('CurrencyUtils', () => {

describe('convertToFrontendAmountAsInteger', () => {
test.each([
[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 as an integer', (amount, expectedResult) => {
expect(CurrencyUtils.convertToFrontendAmountAsInteger(amount)).toBe(expectedResult);
[2500, 25, 'USD'],
[2550, 25.5, 'USD'],
[25, 0.25, 'USD'],
[2500, 25, 'USD'],
[2500.5, 25, 'USD'], // The backend should never send a decimal .5 value
[2500, 25, 'VND'],
[2550, 26, 'VND'],
[25, 0, 'VND'],
[2586, 26, 'VND'],
[2500.5, 25, 'VND'], // The backend should never send a decimal .5 value
])('Correctly converts %s to amount in units handled in frontend as an integer', (amount, expectedResult, currency) => {
expect(CurrencyUtils.convertToFrontendAmountAsInteger(amount, currency)).toBe(expectedResult);
});
});

describe('convertToFrontendAmountAsString', () => {
test.each([
[2500, '25.00'],
[2550, '25.50'],
[25, '0.25'],
[2500.5, '25.00'],
[null, ''],
[undefined, ''],
[0, '0.00'],
])('Correctly converts %s to amount in units handled in frontend as a string', (input, expectedResult) => {
expect(CurrencyUtils.convertToFrontendAmountAsString(input)).toBe(expectedResult);
[2500, '25.00', 'USD'],
[2550, '25.50', 'USD'],
[25, '0.25', 'USD'],
[2500.5, '25.00', 'USD'],
[null, '', 'USD'],
[undefined, '', 'USD'],
[0, '0.00', 'USD'],
[2500, '25', 'VND'],
[2550, '26', 'VND'],
[25, '0', 'VND'],
[2500.5, '25', 'VND'],
[null, '', 'VND'],
[undefined, '', 'VND'],
[0, '0', 'VND'],
[2586, '26', 'VND'],
])('Correctly converts %s to amount in units handled in frontend as a string', (input, expectedResult, currency) => {
expect(CurrencyUtils.convertToFrontendAmountAsString(input, currency ?? CONST.CURRENCY.USD)).toBe(expectedResult);
});
});

Expand Down
Loading