Updated Amount validation conditions to not allow indefinite zeroes at the start. by sig5 · Pull Request #7104 · Expensify/App

sig5 · 2022-01-10T18:18:43Z

Details

The IOU Amount page didn't impose limits on the length of the string if zeroes are appended to the string. Hence the string can be indefinitely long if zeroes are used at the start.

Fixed Issues

$ #7068

Tests

Open IOU Amount Page(Send/Receive).
Enter amounts starting with zeroes.
Test edge cases such as 0's at end and fractional values.

Verify that no errors appear in the JS console

QA Steps

Try entering a string of consecutive zeroes.
Test if fractional values appear as expected.

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Screencast.from.10-01-22.11-39-07.PM.IST.mov

Mobile Web

WhatsApp.Video.2022-01-10.at.11.41.38.PM.mp4

Desktop

iOS

Android

WhatsApp.Video.2022-01-11.at.12.52.15.AM.mp4

github-actions · 2022-01-10T18:19:05Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

sig5 · 2022-01-10T18:23:53Z

I have read the CLA Document and I hereby sign the CLA

parasharrajat · 2022-01-10T18:30:57Z

@sig5 Please add videos for the rest of the platforms.

sig5 · 2022-01-10T18:56:01Z

@parasharrajat I don't have access to a Macbook at the moment. But since this is just a regex change, this should work just fine. Can we do without the Mac/iOS videos?

parasharrajat · 2022-01-10T18:57:48Z

That's fine.

sig5 · 2022-01-10T19:25:39Z

@parasharrajat Thanks :D ! Added for Android.

parasharrajat

~~LGTM.~~. Found an issue below.

parasharrajat

Found an issue
Enter this value => 2000222

Now place the cursor after 2 and try to delete the preceding 2. We can't delete it.

sig5 · 2022-01-10T20:58:44Z

Ah, I missed that! As a user, he/she might want to change 5001 to 1001, So we should allow trailing zeroes at the beginning.
Now what we can do is:
I can revert the regex change and modify the validation condition.
The issue comes due to the fact that the last length checking condition parses the string as a number and compares the length.

App/src/pages/iou/steps/IOUAmountPage.js

Lines 141 to 144 in 6803d00

    
           validateAmount(amount) { 
        
               const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,2})?$/, 'i'); 
        
               return amount === '' || (decimalNumberRegex.test(amount) && (parseFloat((amount * 100).toFixed(2)).toString().length <= CONST.IOU.AMOUNT_MAX_LENGTH)); 
        
           }

We can instead parse the string directly after removing the Commas.
this.stripCommaFromAmount(amount).length <= CONST.IOU.AMOUNT_MAX_LENGTH;

And this should take care of all the values.

cc: @parasharrajat

parasharrajat · 2022-01-10T21:17:06Z

Sound like a plan...

sig5 · 2022-01-10T22:18:03Z

I have modified the conditions to work with all strings. I have also modified the value of Constant AMOUNT_MAX_LENGTH because only the length of part before decimal matters, and the value after the decimal is limited to two places.

parasharrajat · 2022-01-11T07:40:16Z

+        const lengthBeforeDecimal = this.stripCommaFromAmount(amount.split('.')[0]).length;
+        return amount === '' || (decimalNumberRegex.test(amount) && decimalNumberRegex.test(amount) && lengthBeforeDecimal <= (CONST.IOU.AMOUNT_MAX_LENGTH);


Why do need to check the decimal part only? It seems to be that we just need to strip , and . from the number and check the length.

Because the regex enforces the number of values after the decimal to <= 2. If the condition is imposed on the whole string instead, the user can have 500000 and 50000.0 in his valid range but not 50000.24 if IOU.AMOUNT_MAX_LENGTH =6. Applying check on the only part before decimal will ensure a uniform range for the user.
In essence: The sufficient condition to ensure the range of values stays under the max limit is to ensure the length of the decimal part.

Also commas were handled because they were expected to be handled in the existing code. The appropriate use case must be : We might be inserting commas if we are copy-pasting the amount!

Yeah, commas are added. Missed that upper line. On Backend, we check the whole number via converting the floats to integer (amount * 100) is used.
If we try the logic from the previous code.

5000.24 => 500024

5000.0 => 500000

I will try to stick to the old code. and we can just prevent zeros.

Let me know if I am missing something.

The logic that was previously provided works well for a processed string (as we get in backend ) since we can be happy without caring about zeroes in the beginning. In this case, we will need to add an additional condition that essentially checks if (length of stripped portion as a parsedFloat + length of trailing zeroes ) doesn't exceed the limit.

Previous Code:
00500.23 => disregards the initial two zeroes, applies the older condition, and checks from the amount string if the number of zeroes in the beginning, invalidates the amount string.

Newer Logic
00500.23 => takes care of the length of the string directly. The two conditions: (1) fractional part upto 2 decimal places[ Regex takes care] (2) Length of string before decimal should be bounded by Amount_Length, are handled appropriately this way.

In my humble opinion, this condition will ensure a lesser overhead, cleaner, and more generally applicable too! Do let me know your thoughts on it :D .

disregards the initial two zeroes, applies the older condition, and checks from the amount string if the number of zeroes in the beginning, invalidates the amount string.

Hmm. Still, I am not sure of what you explained above but I got the idea.

Since we essentially cast strings as integers, we lose the zeroes in the beginning of the number that causes the referenced issue ( Since parseFloat("00000005")=5 ).
I am suggesting that if we directly use the string for validation, we won't be losing the zeroes in the beginning.

parasharrajat · 2022-01-11T08:27:41Z

            REQUEST: 'request',
        },
-        AMOUNT_MAX_LENGTH: 10,
+        AMOUNT_MAX_LENGTH: 7,


Suggested change

AMOUNT_MAX_LENGTH: 7,

AMOUNT_WHOLE_MAX_LENGTH: 7,

Can you think of a better name to denote the part correctly? I can't think any..

We can probably do something like AMOUNT_INTEGER_PART_MAX_LEN

Hmm, did I miss the reasoning behind this change? We want to support large IOU amounts.

Can you revert the value to 10 please

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

parasharrajat

LGTM.

cc: @Julesssss

🎀 👀 🎀 C+ reviewed

Julesssss · 2022-01-11T14:24:33Z

Hi @sig5. please merge main to resolve the failing test.

sig5 · 2022-01-11T15:27:59Z

Done. Thanks!

Julesssss

Yep, that has fixed the lag on my size. Thanks.

It looks like there's a trailing space lint failure to fix, but other than that the PR looks good.

parasharrajat · 2022-01-12T15:01:05Z

Seems, PR is changed completely. I will do the review shortly...

parasharrajat

Good work. Loved the math.

parasharrajat · 2022-01-14T17:33:05Z

+     * @returns {Number}
+     */
+    amountLength(amount) {
+        const trailingZeroes = amount.match(/^0+/);


Should this be leadingZeroes ? if so, please replace all the trailing to leading.

Yeah my bad!

parasharrajat · 2022-01-14T17:43:34Z

+        We return sum of trailing zeroes length and absAmount length( which has value of the all digits after the decimal).
+        In case of zeroes, we add the trailing zeroes length to 2, which
+        represent places reserved for digits after decimal.
+        */
+        return trailingZeroesLength + (absAmount === '0' ? 2 : absAmount.length);


Suggested change

We return sum of trailing zeroes length and absAmount length( which has value of the all digits after the decimal).

In case of zeroes, we add the trailing zeroes length to 2, which

represent places reserved for digits after decimal.

*/

return trailingZeroesLength + (absAmount === '0' ? 2 : absAmount.length);

Return the sum of leading zeroes length and absolute amount length(including fraction digits).

When the absolute amount is 0, add 2 to the leading zeroes length to represent fraction digits.

*/

return trailingZeroesLength + absAmount === '0' ? 2 : absAmount.length;

I addressed the comments, but am doing away with parentheses change since === and ?: has lower precedence than +, it will break the code to put w/o it both at beginning and end.

sig5 · 2022-01-14T20:54:56Z

Thanks, @parasharrajat :D! I addressed the suggestions.

parasharrajat

@sig5 Could you please do a final checkout on performance? just to make sure lodashGet is not slow... 😄

sig5 · 2022-01-15T07:44:02Z

Hey @parasharrajat , the runtimes of both the methods seem pretty much equal on my system!

parasharrajat · 2022-01-15T19:14:01Z

https://measurethat.net/Benchmarks/Show/16725/0/lodashget-vs-property-df

Native code is 1.5 times faster than lodashGet. But I am fine as far as it does not affect the UX. I leave this to @Julesssss to check. For me, both are the same and I don't see a recognizable delay.

parasharrajat

LGTM.

cc: @Julesssss

Julesssss · 2022-01-17T14:44:37Z

Hi @sig5, with the latest changes I'm once again able to add multiple zeros prior to the decimal place.

Julesssss

Requesting further changes.

sig5 · 2022-01-17T15:26:22Z

Hi @Julesssss this is expected since we decided to only limit the number of zeroes rather than limiting the zeroes to one since it will pose issues while editing the number. (Eg: 50001->00001-> 10001).
The initial issue was the addition of unlimited zeroes in the number which has been resolved, So you cannot add more zeroes indefinitely but up to 8 0's are allowed as per the constant length criterion.
#7104 (review)

Ah, I missed that! As a user, he/she might want to change 5001 to 1001, So we should allow trailing zeroes at the beginning.

Julesssss · 2022-01-17T15:40:44Z

Ah, that makes sense. Thanks for pointing out the comment.

OSBotify · 2022-01-17T15:41:45Z

@sig5, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

OSBotify · 2022-01-17T15:42:13Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-01-18T22:45:25Z

🚀 Deployed to staging by @Julesssss in version: 1.1.30-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mvtglobally · 2022-01-20T00:35:28Z

@marcaaron @chiragsalian @mallenexpensify When we have a single zero upfront, it is not being stripped. Can you please confirm its expected?

sig5 · 2022-01-20T01:07:12Z

Hi @mvtglobally I feel like I should mention here that this was discussed here:#7104 (review),
We concluded that, in case the user just missed adding a number before the zeroes, he might want to go back and edit it with the zeroes .

Example:

User wants to type 60001
User types 50001 by mistake
User clears the 5, and 0001 gets stripped to 1
So it can pose issues while editing the number.

chiragsalian · 2022-01-20T01:21:24Z

Hi @mvtglobally, yes i believe this is fine. From my understanding the code here does not allow indefinite 0's. i.e., you can't enter more than 8 0's to destroy the UI. You can enter 00012345 and its totally fine. Its just that you cannot enter like 30 0's to break the UI (which currently happens on production). Can you retest and let us know if its a pass or if you have any more questions?

OSBotify · 2022-01-21T00:15:24Z

🚀 Deployed to production by @chiragsalian in version: 1.1.31-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Updated regex to not allow more than one trailing zero at start.

1e66809

sig5 requested a review from a team as a code owner January 10, 2022 18:18

MelvinBot requested review from Julesssss and parasharrajat and removed request for a team January 10, 2022 18:18

parasharrajat approved these changes Jan 10, 2022

View reviewed changes

parasharrajat suggested changes Jan 10, 2022

View reviewed changes

sig5 added 2 commits January 11, 2022 03:36

revert regex change

766886d

Updated validation condition on amount string

e9338b0

sig5 requested a review from parasharrajat January 11, 2022 07:35

parasharrajat suggested changes Jan 11, 2022

View reviewed changes

sig5 and others added 3 commits January 11, 2022 14:22

Updated Suggested changes

db13fb7

Update src/pages/iou/steps/IOUAmountPage.js

94d1be7

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

updated constant name

671cab5

parasharrajat previously approved these changes Jan 11, 2022

View reviewed changes

sig5 changed the title ~~Updated regex to not allow more than one trailing zero at start.~~ Updated Amount validation conditions to not allow more than one trailing zero at start. Jan 11, 2022

sig5 changed the title ~~Updated Amount validation conditions to not allow more than one trailing zero at start.~~ Updated Amount validation conditions to not allow indefinite zeroes at the start. Jan 11, 2022

Merge branch 'Expensify:main' into fix-issue#7068

63a9c34

sig5 and others added 4 commits January 12, 2022 17:19

fixed linting issue

3531879

added comments

ee716fb

Edit comments.

c1c87d0

Edit comments.

9461f82

Julesssss reviewed Jan 12, 2022

View reviewed changes

Fixed linting

1ba12aa

parasharrajat suggested changes Jan 14, 2022

View reviewed changes

updated comments and var_names

9a50a1d

parasharrajat reviewed Jan 15, 2022

View reviewed changes

parasharrajat approved these changes Jan 15, 2022

View reviewed changes

Julesssss approved these changes Jan 17, 2022

View reviewed changes

Julesssss requested changes Jan 17, 2022

View reviewed changes

Julesssss approved these changes Jan 17, 2022

View reviewed changes

Julesssss merged commit eaa09e7 into Expensify:main Jan 17, 2022

OSBotify mentioned this pull request Jan 18, 2022

Deploy Checklist: New Expensify 2022-01-18 #7297

Closed

56 tasks

botify mentioned this pull request Jan 20, 2022

[HOLD for payment 2022-01-27] IOU - Zeros can be used indefinitely #7068

Closed

		const lengthBeforeDecimal = this.stripCommaFromAmount(amount.split('.')[0]).length;
		return amount === '' \|\| (decimalNumberRegex.test(amount) && decimalNumberRegex.test(amount) && lengthBeforeDecimal <= (CONST.IOU.AMOUNT_MAX_LENGTH);

Uh oh!

Conversation

sig5 commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

github-actions Bot commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sig5 commented Jan 10, 2022

Uh oh!

parasharrajat commented Jan 10, 2022

Uh oh!

sig5 commented Jan 10, 2022

Uh oh!

parasharrajat commented Jan 10, 2022

Uh oh!

sig5 commented Jan 10, 2022

Uh oh!

parasharrajat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sig5 commented Jan 10, 2022

Uh oh!

parasharrajat commented Jan 10, 2022

Uh oh!

sig5 commented Jan 10, 2022

Uh oh!

parasharrajat Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sig5 Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sig5 Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

parasharrajat Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sig5 Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Julesssss commented Jan 11, 2022

Uh oh!

sig5 commented Jan 11, 2022

sig5 commented Jan 10, 2022 •

edited

Loading

github-actions Bot commented Jan 10, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Jan 11, 2022 •

edited

Loading

sig5 Jan 11, 2022 •

edited

Loading

sig5 Jan 11, 2022 •

edited

Loading

parasharrajat Jan 11, 2022 •

edited

Loading

sig5 Jan 11, 2022 •

edited

Loading

sig5 Jan 14, 2022 •

edited

Loading

sig5 commented Jan 17, 2022 •

edited

Loading