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.

Sign up for GitHub

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

#2 - Fix user can submit 10-digit amount and gives an unexpected error #42815

Merged
merged 9 commits into from
Jun 7, 2024
Merged

#2 - Fix user can submit 10-digit amount and gives an unexpected error #42815

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
aa4be9e
always convert the amount to cent
bernhardoj May 30, 2024
08e85dc
don't allow NaN, Infinity, and number with scientific notation
bernhardoj May 30, 2024
6319c88
remove dot when calculating the length
bernhardoj May 30, 2024
261d307
refactor max amount digit logic
bernhardoj Jun 3, 2024
2c9e8cc
add unit test
bernhardoj Jun 3, 2024
cc87e52
remove unused function
bernhardoj Jun 3, 2024
63819bf
prettier
bernhardoj Jun 3, 2024
8d75abf
use the correct const
bernhardoj Jun 3, 2024
fa1031d
Merge branch 'main' into fix/42084-limi-amount-to-8-digit
bernhardoj Jun 7, 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
18 changes: 13 additions & 5 deletions src/libs/MoneyRequestUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ function stripCommaFromAmount(amount: string): string {
return amount.replace(/,/g, '');
}

/**
* Strip dot from the amount
*/
function stripDotFromAmount(amount: string): string {
return amount.replace(/\./g, '');
}

/**
* Strip spaces from the amount
*/
Expand Down Expand Up @@ -40,16 +47,17 @@ function addLeadingZero(amount: string): string {
/**
* Calculate the length of the amount with leading zeroes
*/
function calculateAmountLength(amount: string, decimals: number): number {
function calculateAmountLength(amount: string): number {
const leadingZeroes = amount.match(/^0+/);
const leadingZeroesLength = leadingZeroes?.[0]?.length ?? 0;
const absAmount = parseFloat((Number(stripCommaFromAmount(amount)) * 10 ** decimals).toFixed(2)).toString();
const absAmount = parseFloat((Number(stripCommaFromAmount(amount)) * 100).toFixed(2)).toString();

if (/\D/.test(absAmount)) {
// Don't allow the amount if the parsing to number results in NaN, Infinity, or with a scientific notation (e.g., 1e+26)
if (/NaN|Infinity|e\+/.test(absAmount)) {
return CONST.IOU.AMOUNT_MAX_LENGTH + 1;
}

return leadingZeroesLength + (absAmount === '0' ? 2 : absAmount.length);
return leadingZeroesLength + (absAmount === '0' ? 2 : stripDotFromAmount(absAmount).length);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, another way to fix this is by passing the removed-dot-amount to the old regex

const amountWithoutDecimalSeparator = stripDotFromAmount(absAmount);
if (/\D/.test(amountWithoutDecimalSeparator)) {
    return CONST.IOU.AMOUNT_MAX_LENGTH + 1;
}

Let me know if we want to use this instead.

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 I like the current version better, but can we also add unit tests to prevent future regressions please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before adding a test, what do you think about my suggestion here. Do you think we should do it or keep it as it is for now?

If we want to do it, then we don't need the test for this specific function.

}

/**
Expand All @@ -61,7 +69,7 @@ function validateAmount(amount: string, decimals: number, amountMaxLength: numbe
? `^\\d+(,\\d*)*$` // Don't allow decimal point if decimals === 0
: `^\\d+(,\\d*)*(\\.\\d{0,${decimals}})?$`; // Allow the decimal point and the desired number of digits after the point
const decimalNumberRegex = new RegExp(regexString, 'i');
return amount === '' || (decimalNumberRegex.test(amount) && calculateAmountLength(amount, decimals) <= amountMaxLength);
return amount === '' || (decimalNumberRegex.test(amount) && calculateAmountLength(amount) <= amountMaxLength);
}

/**
Expand Down
Loading