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

Preserve transactions amount in create IOU #40062

Merged
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions src/components/transactionPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,7 @@ export default PropTypes.shape({

/** Server side errors keyed by microtime */
errorFields: PropTypes.objectOf(PropTypes.objectOf(translatableTextPropTypes)),

/** Whether the original input should be shown */
shouldShowOriginalAmount: PropTypes.bool,
});
20 changes: 17 additions & 3 deletions src/libs/CurrencyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +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);
abzokhattab marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* 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 @@ -98,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 @@ -139,7 +152,8 @@ export {
getCurrencySymbol,
isCurrencySymbolLTR,
convertToBackendAmount,
convertToFrontendAmount,
convertToFrontendAmountAsInteger,
convertToFrontendAmountAsString,
convertToDisplayString,
convertAmountToDisplayString,
isValidCurrencyCode,
Expand Down
4 changes: 2 additions & 2 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,8 @@ 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) {
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {amount, currency});
function setMoneyRequestAmount_temporaryForRefactor(transactionID: string, amount: number, currency: string, shouldShowOriginalAmount = false) {
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.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,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
7 changes: 6 additions & 1 deletion src/pages/iou/request/step/IOURequestStepAmount.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ type IOURequestStepAmountProps = IOURequestStepAmountOnyxProps &
WithWritableReportOrNotFoundProps<typeof SCREENS.MONEY_REQUEST.STEP_AMOUNT | typeof SCREENS.MONEY_REQUEST.CREATE> & {
/** The transaction object being modified in Onyx */
transaction: OnyxEntry<Transaction>;

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

function IOURequestStepAmount({
Expand All @@ -49,6 +52,7 @@ function IOURequestStepAmount({
transaction,
splitDraftTransaction,
draftTransaction,
shouldKeepUserInput = false,
}: IOURequestStepAmountProps) {
const {translate} = useLocalize();
const textInput = useRef<BaseTextInputRef | null>(null);
Expand Down Expand Up @@ -107,7 +111,7 @@ function IOURequestStepAmount({
const amountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(amount));

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
IOU.setMoneyRequestAmount_temporaryForRefactor(transactionID, amountInSmallestCurrencyUnits, currency || CONST.CURRENCY.USD);
IOU.setMoneyRequestAmount_temporaryForRefactor(transactionID, amountInSmallestCurrencyUnits, currency || CONST.CURRENCY.USD, shouldKeepUserInput);

if (backTo) {
Navigation.goBack(backTo);
Expand Down Expand Up @@ -165,6 +169,7 @@ function IOURequestStepAmount({
currency={currency}
amount={Math.abs(transactionAmount)}
ref={(e) => (textInput.current = e)}
shouldKeepUserInput={transaction?.shouldShowOriginalAmount}
onCurrencyButtonPress={navigateToCurrencySelectionPage}
onSubmitButtonPress={saveAmountAndCurrency}
selectedTab={iouRequestType}
Expand Down
21 changes: 10 additions & 11 deletions src/pages/iou/steps/MoneyRequestAmountForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ type MoneyRequestAmountFormProps = {

/** The current tab we have navigated to in the request modal. String that corresponds to the request type. */
selectedTab?: SelectedTabRequest;

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

type Selection = {
Expand All @@ -66,7 +69,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(Math.abs(taxAmount));
isTaxAmountForm && Number.parseFloat(currentAmount) > CurrencyUtils.convertToFrontendAmountAsInteger(Math.abs(taxAmount));

const AMOUNT_VIEW_ID = 'amountView';
const NUM_PAD_CONTAINER_VIEW_ID = 'numPadContainerView';
Expand All @@ -82,6 +85,7 @@ function MoneyRequestAmountForm(
onCurrencyButtonPress,
onSubmitButtonPress,
selectedTab = CONST.TAB_REQUEST.MANUAL,
shouldKeepUserInput = false,
}: MoneyRequestAmountFormProps,
forwardedRef: ForwardedRef<BaseTextInputRef>,
) {
Expand All @@ -93,7 +97,7 @@ function MoneyRequestAmountForm(
const isTaxAmountForm = Navigation.getActiveRoute().includes('taxAmount');

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

Choose a reason for hiding this comment

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

Won't this just return '' if we let the null amount go through?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes but if the amount is zero the convertToFrontendAmountAsString will return 0.00 which is not wanted.. so i kept that check to return "" in this specific case


const [currentAmount, setCurrentAmount] = useState(selectedAmountAsString);
const [formError, setFormError] = useState<MaybePhraseKey>('');
Expand Down Expand Up @@ -135,7 +139,7 @@ function MoneyRequestAmountForm(
};

const initializeAmount = useCallback((newAmount: number) => {
const frontendAmount = newAmount ? CurrencyUtils.convertToFrontendAmount(newAmount).toString() : '';
const frontendAmount = newAmount ? CurrencyUtils.convertToFrontendAmountAsString(newAmount) : '';
setCurrentAmount(frontendAmount);
setSelection({
start: frontendAmount.length,
Expand All @@ -144,13 +148,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 @@ -264,13 +268,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});
}, [currentAmount, taxAmount, isTaxAmountForm, onSubmitButtonPress, currency, 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 @@ -217,6 +217,9 @@ type Transaction = OnyxCommon.OnyxValueWithOfflineFeedback<
/** Indicates transaction loading */
isLoading?: boolean;

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

/** The actionable report action ID associated with the transaction */
actionableWhisperReportActionID?: string;

Expand Down
20 changes: 17 additions & 3 deletions tests/unit/CurrencyUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,29 @@ 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);
});
});

Expand Down
Loading