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

Corrected Currency Display: Enforce Two Decimal Places in Amounts #35797

Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2169530
Corrected Currency Display: Enforce Two Decimal Places in Amounts
abzokhattab Feb 5, 2024
72d282f
fix the case where there is no decimal value passed
abzokhattab Feb 6, 2024
8e80ffb
cleaning
abzokhattab Feb 6, 2024
8315492
fixing tests
abzokhattab Feb 6, 2024
bf046e2
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Feb 18, 2024
65f90db
keeping the user input when clicking next
abzokhattab Feb 18, 2024
3fa6bb9
Fixing tests
abzokhattab Feb 18, 2024
45013c3
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Feb 22, 2024
6c40f80
Removing the amount from the useEffect dependencies
abzokhattab Feb 22, 2024
c9614cc
minor change
abzokhattab Feb 22, 2024
bd68682
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Feb 29, 2024
c7363b6
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Mar 10, 2024
4441216
initialize amount only if the form is an edit form
abzokhattab Mar 10, 2024
10e0185
fixing the display of the entered IOU amount
abzokhattab Mar 16, 2024
1288e8f
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Mar 16, 2024
731ef4d
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Mar 16, 2024
0a08a01
cleanup
abzokhattab Mar 19, 2024
3ed17ed
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Mar 19, 2024
f0a071e
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Mar 28, 2024
0000b2b
Adding edge cases tests
abzokhattab Mar 28, 2024
89d4d77
minor edit
abzokhattab Mar 28, 2024
ad1507b
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Apr 1, 2024
d4b3764
Merge remote-tracking branch 'origin/main' into preserving-the-transa…
abzokhattab Apr 2, 2024
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
3 changes: 3 additions & 0 deletions src/components/transactionPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ export default PropTypes.shape({
/** The original transaction amount */
amount: PropTypes.number,

/** Whether the original input should be shown */
shouldShowOriginalAmount: PropTypes.bool,

/** The edited transaction amount */
modifiedAmount: PropTypes.number,

Expand Down
19 changes: 16 additions & 3 deletions src/libs/CurrencyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,22 @@ function convertToBackendAmount(amountAsFloat: number): number {
*
* @note we do not support any currencies with more than two decimal places.
*/
function convertToFrontendAmount(amountAsInt: number): number {
function convertToFrontendAmountAsInteger(amountAsInt: number): number {
return Math.trunc(amountAsInt) / 100.0;
}

/**
* 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 {
if (amountAsInt === null || amountAsInt === undefined) {
return '';
}
return convertToFrontendAmountAsInteger(amountAsInt).toFixed(2);
}

/**
* 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)
Expand All @@ -99,7 +111,7 @@ function convertToFrontendAmount(amountAsInt: number): number {
* @param currency - IOU currency
*/
function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURRENCY.USD): string {
const convertedAmount = convertToFrontendAmount(amountInCents);
const convertedAmount = convertToFrontendAmountAsInteger(amountInCents);
return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, {
style: 'currency',
currency,
Expand Down Expand Up @@ -140,7 +152,8 @@ export {
getCurrencySymbol,
isCurrencySymbolLTR,
convertToBackendAmount,
convertToFrontendAmount,
convertToFrontendAmountAsInteger,
convertToFrontendAmountAsString,
convertToDisplayString,
convertAmountToDisplayString,
isValidCurrencyCode,
Expand Down
6 changes: 3 additions & 3 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,12 @@ function startMoneyRequest(iouType: ValueOf<typeof CONST.IOU.TYPE>, reportID: st
}

// eslint-disable-next-line @typescript-eslint/naming-convention
function setMoneyRequestAmount_temporaryForRefactor(transactionID: string, amount: number, currency: string, removeOriginalCurrency = false) {
function setMoneyRequestAmount_temporaryForRefactor(transactionID: string, amount: number, currency: string, removeOriginalCurrency = false, shouldShowOriginalAmount = false) {
if (removeOriginalCurrency) {
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {amount, currency, originalCurrency: null});
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {amount, currency, originalCurrency: null, shouldShowOriginalAmount});
return;
}
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {amount, currency});
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {amount, currency, shouldShowOriginalAmount});
}

// eslint-disable-next-line @typescript-eslint/naming-convention
Expand Down
9 changes: 8 additions & 1 deletion src/pages/iou/request/IOURequestStartPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,14 @@ function IOURequestStartPage({
onTabSelected={resetIOUTypeIfChanged}
tabBar={TabSelector}
>
<TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>{() => <IOURequestStepAmount route={route} />}</TopTab.Screen>
<TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
{() => (
<IOURequestStepAmount
shouldKeepUserInput
route={route}
/>
)}
</TopTab.Screen>
<TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>{() => <IOURequestStepScan route={route} />}</TopTab.Screen>
{shouldDisplayDistanceRequest && <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>{() => <IOURequestStepDistance route={route} />}</TopTab.Screen>}
</OnyxTabNavigator>
Expand Down
8 changes: 7 additions & 1 deletion src/pages/iou/request/step/IOURequestStepAmount.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ const propTypes = {
/** The transaction object being modified in Onyx */
transaction: transactionPropTypes,

/** Whether the user input should be kept or not */
shouldKeepUserInput: PropTypes.bool,

/** The policy of the report */
policy: PropTypes.shape({
/**
Expand All @@ -57,6 +60,7 @@ const defaultProps = {
report: {},
transaction: {},
policy: {},
shouldKeepUserInput: false,
};

const getTaxAmount = (transaction, defaultTaxValue, amount) => {
Expand All @@ -72,6 +76,7 @@ function IOURequestStepAmount({
transaction,
transaction: {currency},
policy,
shouldKeepUserInput,
}) {
const {translate} = useLocalize();
const textInput = useRef(null);
Expand Down Expand Up @@ -133,7 +138,7 @@ function IOURequestStepAmount({
IOU.setMoneyRequestTaxAmount(transaction.transactionID, taxAmountInSmallestCurrencyUnits);
}

IOU.setMoneyRequestAmount_temporaryForRefactor(transactionID, amountInSmallestCurrencyUnits, currency || CONST.CURRENCY.USD, true);
IOU.setMoneyRequestAmount_temporaryForRefactor(transactionID, amountInSmallestCurrencyUnits, currency || CONST.CURRENCY.USD, true, shouldKeepUserInput);

if (backTo) {
Navigation.goBack(backTo);
Expand Down Expand Up @@ -167,6 +172,7 @@ function IOURequestStepAmount({
currency={currency}
amount={transaction.amount}
ref={(e) => (textInput.current = e)}
shouldKeepUserInput={transaction.shouldShowOriginalAmount}
onCurrencyButtonPress={navigateToCurrencySelectionPage}
onSubmitButtonPress={navigateToNextPage}
selectedTab={iouRequestType}
Expand Down
27 changes: 12 additions & 15 deletions src/pages/iou/steps/MoneyRequestAmountForm.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, {useCallback, useEffect, useRef, useState} from 'react';
import type {ForwardedRef} from 'react';
import {View} from 'react-native';
import React, {useCallback, useEffect, useRef, useState} from 'react';
import type {NativeSyntheticEvent, TextInputSelectionChangeEventData} from 'react-native';
import {View} from 'react-native';
import type {ValueOf} from 'type-fest';
import BigNumberPad from '@components/BigNumberPad';
import Button from '@components/Button';
Expand All @@ -28,6 +28,9 @@ type MoneyRequestAmountFormProps = {
/** Calculated tax amount based on selected tax rate */
taxAmount?: number;

/** Whether the user input should be kept or not */
shouldKeepUserInput?: boolean;

/** Currency chosen by user or saved in Onyx */
currency?: string;

Expand Down Expand Up @@ -59,7 +62,7 @@ const getNewSelection = (oldSelection: Selection, prevLength: number, newLength:

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

const AMOUNT_VIEW_ID = 'amountView';
const NUM_PAD_CONTAINER_VIEW_ID = 'numPadContainerView';
Expand All @@ -74,6 +77,7 @@ function MoneyRequestAmountForm(
onCurrencyButtonPress,
onSubmitButtonPress,
selectedTab = CONST.TAB_REQUEST.MANUAL,
shouldKeepUserInput = false,
}: MoneyRequestAmountFormProps,
forwardedRef: ForwardedRef<BaseTextInputRef>,
) {
Expand All @@ -83,10 +87,8 @@ function MoneyRequestAmountForm(

const textInput = useRef<BaseTextInputRef | null>(null);
const isTaxAmountForm = Navigation.getActiveRoute().includes('taxAmount');

const decimals = CurrencyUtils.getCurrencyDecimals(currency);
const selectedAmountAsString = amount ? CurrencyUtils.convertToFrontendAmount(amount).toString() : '';

const selectedAmountAsString = CurrencyUtils.convertToFrontendAmountAsString(amount);
const [currentAmount, setCurrentAmount] = useState(selectedAmountAsString);
const [formError, setFormError] = useState<MaybePhraseKey>('');
const [shouldUpdateSelection, setShouldUpdateSelection] = useState(true);
Expand Down Expand Up @@ -119,7 +121,7 @@ function MoneyRequestAmountForm(
};

const initializeAmount = useCallback((newAmount: number) => {
const frontendAmount = newAmount ? CurrencyUtils.convertToFrontendAmount(newAmount).toString() : '';
const frontendAmount = CurrencyUtils.convertToFrontendAmountAsString(newAmount);
setCurrentAmount(frontendAmount);
setSelection({
start: frontendAmount.length,
Expand All @@ -128,13 +130,13 @@ function MoneyRequestAmountForm(
}, []);

useEffect(() => {
if (!currency || typeof amount !== 'number') {
if (!currency || typeof amount !== 'number' || shouldKeepUserInput) {
return;
}
initializeAmount(amount);
// we want to re-initialize the state only when the selected tab or amount changes
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [selectedTab, amount]);
}, [selectedTab, amount, shouldKeepUserInput]);

/**
* Sets the selection and the amount accordingly to the value passed to the input
Expand Down Expand Up @@ -235,13 +237,8 @@ function MoneyRequestAmountForm(
return;
}

// Update display amount string post-edit to ensure consistency with backend amount
// Reference: https://github.com/Expensify/App/issues/30505
const backendAmount = CurrencyUtils.convertToBackendAmount(Number.parseFloat(currentAmount));
initializeAmount(backendAmount);

onSubmitButtonPress({amount: currentAmount, currency});
}, [onSubmitButtonPress, currentAmount, taxAmount, currency, isTaxAmountForm, formattedTaxAmount, initializeAmount]);
}, [currentAmount, taxAmount, isTaxAmountForm, onSubmitButtonPress, currency, formattedTaxAmount]);

/**
* Input handler to check for a forward-delete key (or keyboard shortcut) press.
Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/Transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ type Transaction = OnyxCommon.OnyxValueWithOfflineFeedback<
/** Whether the request is billable */
billable?: boolean;

/** Whether the user input should be kept */
shouldShowOriginalAmount?: boolean;

/** The category name */
category?: string;

Expand Down
19 changes: 16 additions & 3 deletions tests/unit/CurrencyUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,31 @@ describe('CurrencyUtils', () => {
});
});

describe('convertToFrontendAmount', () => {
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', (amount, expectedResult) => {
expect(CurrencyUtils.convertToFrontendAmount(amount)).toBe(expectedResult);
])('Correctly converts %s to amount in units handled in frontend as an integer', (amount, expectedResult) => {
expect(CurrencyUtils.convertToFrontendAmountAsInteger(amount)).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);
});
});
describe('convertToDisplayString', () => {
test.each([
[CONST.CURRENCY.USD, 25, '$0.25'],
Expand Down
Loading