[Deploy Blocker] Changed max length to 11 for alignment issues #5243

akshayasalvi · 2021-09-14T04:11:09Z

@deetergp @marcaaron or anybody can review and merge this on priority?

Details

Fixed Issues

$ #5241
$ #5115

Tests

Tested by switching multiple 3 digit currency symbols

QA Steps

Go to any IOU transaction
Select any 3 digit currency
Enter a very long number
It should restrict to 8 digits without decimal (and 10 digits with decimal) and alignment shouldn't break.
Click the Next button
Choose a person to send the IOU request to
Enter a reason, confirm
Make sure the IOU is created and that no error message is returned

Tested On

Screenshots

Web

Screen.Recording.2021-09-15.at.12.00.58.AM.mov

Mobile Web

Updated with 10 digits
https://user-images.githubusercontent.com/57435789/133314295-be399ca0-63ad-4089-939f-d45d6e9f1cfb.mov

Desktop

iOS

Android

francoisl · 2021-09-14T18:18:18Z

src/CONST.js

@@ -362,7 +362,7 @@ const CONST = {
            PAYPAL_ME: 'PayPal.me',
            VENMO: 'Venmo',
        },
-        AMOUNT_MAX_LENGTH: 14,
+        AMOUNT_MAX_LENGTH: 11,


I commented on the deploy blocker, but I think this needs to be 10, our backend shouldn't allow 11 digits (assuming we use the name amount for the parameter when calling the API)

Ok sorry, small correction. The max is 10 digits when given amount is in cents. So for example you can use enter:

$12345678.91 because 12345678.91 * 100 = 1234567891 and is 10 digits, but you cannot enter the amount 1234567890, because 1234567890 * 100 = 123456789000 is 12 digits.

Okay I've updated the PR to 10 digits now.

francoisl · 2021-09-14T18:26:01Z

The tests should include creating the IOU though please, otherwise there's no point in fixing the UI if the backend won't allow the amount you enter. This is the error you receive if you try to complete the flow with the amount in the tests:

akshayasalvi · 2021-09-14T18:37:09Z

@francoisl I've updated the PR with the 10 digit check.

But I am not sure how to add tests (I've never done that earlier). Can you help with this and I'll add those.

francoisl · 2021-09-14T23:19:37Z

src/CONST.js

@@ -362,7 +362,7 @@ const CONST = {
            PAYPAL_ME: 'PayPal.me',
            VENMO: 'Venmo',
        },
-        AMOUNT_MAX_LENGTH: 14,
+        AMOUNT_MAX_LENGTH: 10,


So as I explained here, we always use amounts in cents in the Expensify backend, so if for example you type $1.23, then we use the amount 123. If you type $20, then we send 2000.
The amount in cents is what needs to be 10 digits max, so we need to multiply the amount to check in validateAmount() by 100 first.

Does that make sense?

Okay, I somehow misunderstood. I've updated the PR with the new cent logic.

Limits are 8 digits without decimal and 10 digit with decimal.

akshayasalvi · 2021-09-15T17:59:13Z

The tests should include creating the IOU though please, otherwise there's no point in fixing the UI if the backend won't allow the amount you enter. This is the error you receive if you try to complete the flow with the amount in the tests:

Are you asking me to add a test case to an existing one? or are you asking me to write a fresh new unit test case? Can you please confirm?

francoisl · 2021-09-15T18:19:21Z

Are you asking me to add a test case to an existing one?

No - just in the QA steps section, update the instructions to say to complete the entire flow, so:

Enter an amount
Click the Next button
Choose a person to send the IOU request to
Enter a reason, confirm
Make sure the IOU is created and that no error message is returned

But I tested on my end and it's working.

akshayasalvi · 2021-09-15T18:22:52Z

Update the QA steps. Thanks a lot, @francoisl for your help.

github-actions · 2021-09-15T18:35:24Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

[Deploy Blocker] Changed max length to 11 for alignment issues (cherry picked from commit 09dd0bf)

isagoico · 2021-09-15T19:48:15Z

@francoisl @akshayasalvi We're able to use 11 digits with decimals instead of 10

francoisl · 2021-09-15T19:55:39Z

Ah, good catch. It only works if the last digit is 0 though, and for example $1.000 is strictly equal to $1.00. All that matters is that the amount in cents (so, multiplied by 100) is no longer than 10 digits. Overall, this shouldn't cause any issue with creating the IOU, so I'd say let's ignore it.

isagoico · 2021-09-15T20:00:08Z

Great then! We can check this off 🎉

akshayasalvi · 2021-09-15T20:01:40Z

I had explicitly tested .000 and ignored because it wasn't breaking any backend or UI flow.

botify · 2021-09-15T20:05:22Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-22. 🎊

OSBotify · 2021-09-15T20:06:15Z

🚀 Cherry-picked to staging by @francoisl in version: 1.0.98-1 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2021-09-15T20:25:09Z

🚀 Deployed to production by @francoisl in version: 1.0.98-1 🚀

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

OSBotify · 2021-09-15T21:15:44Z

🚀 Deployed to staging by @francoisl in version: 1.0.98-2 🚀

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

OSBotify · 2021-09-20T21:00:52Z

🚀 Deployed to production by @Jag96 in version: 1.0.99-4 🚀

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

Changed max length to 11 for alignment issues

f8612a7

akshayasalvi requested a review from a team as a code owner September 14, 2021 04:11

MelvinBot requested review from marcaaron and removed request for a team September 14, 2021 04:11

francoisl reviewed Sep 14, 2021

View reviewed changes

Capped the amount to 10 digits

3ec2a58

marcaaron removed their request for review September 14, 2021 20:27

marcaaron assigned akshayasalvi Sep 14, 2021

francoisl requested changes Sep 14, 2021

View reviewed changes

Updated logic for length check with cents handling

1c4e396

francoisl approved these changes Sep 15, 2021

View reviewed changes

francoisl added the CP Staging label Sep 15, 2021

ctkochan22 approved these changes Sep 15, 2021

View reviewed changes

francoisl merged commit 09dd0bf into Expensify:main Sep 15, 2021

github-actions bot pushed a commit that referenced this pull request Sep 15, 2021

Merge pull request #5243 from akshayasalvi/iou-amt-maxlength

171c408

[Deploy Blocker] Changed max length to 11 for alignment issues (cherry picked from commit 09dd0bf)

This was referenced Sep 15, 2021

🍒 Cherry pick PR #5243 to staging 🍒 #5273

Merged

Deploy Checklist: New Expensify 2021-09-13 #5224

Closed

OSBotify mentioned this pull request Sep 15, 2021

Deploy Checklist: New Expensify 2021-09-15 #5279

Closed

27 tasks

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

jjcoffee mentioned this pull request Sep 15, 2023

[C+ checklist needs completion] [$500] Web - User cannot edit the large amount after creating the distance #27255

Closed

6 tasks

bernhardoj mentioned this pull request May 14, 2024

[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] mWeb - IOU - Creating a split expense with IQD currency & 10 digit amount unexpected error shown #42084

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Deploy Blocker] Changed max length to 11 for alignment issues #5243

[Deploy Blocker] Changed max length to 11 for alignment issues #5243

akshayasalvi commented Sep 14, 2021 •

edited

Loading

francoisl Sep 14, 2021

francoisl Sep 14, 2021

akshayasalvi Sep 14, 2021

francoisl commented Sep 14, 2021

akshayasalvi commented Sep 14, 2021

francoisl Sep 14, 2021

akshayasalvi Sep 15, 2021

akshayasalvi Sep 15, 2021

akshayasalvi commented Sep 15, 2021

francoisl commented Sep 15, 2021 •

edited

Loading

akshayasalvi commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

isagoico commented Sep 15, 2021 •

edited

Loading

francoisl commented Sep 15, 2021

isagoico commented Sep 15, 2021

akshayasalvi commented Sep 15, 2021

botify commented Sep 15, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 20, 2021

[Deploy Blocker] Changed max length to 11 for alignment issues #5243

[Deploy Blocker] Changed max length to 11 for alignment issues #5243

Conversation

akshayasalvi commented Sep 14, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

francoisl Sep 14, 2021

Choose a reason for hiding this comment

francoisl Sep 14, 2021

Choose a reason for hiding this comment

akshayasalvi Sep 14, 2021

Choose a reason for hiding this comment

francoisl commented Sep 14, 2021

akshayasalvi commented Sep 14, 2021

francoisl Sep 14, 2021

Choose a reason for hiding this comment

akshayasalvi Sep 15, 2021

Choose a reason for hiding this comment

akshayasalvi Sep 15, 2021

Choose a reason for hiding this comment

akshayasalvi commented Sep 15, 2021

francoisl commented Sep 15, 2021 • edited Loading

akshayasalvi commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

isagoico commented Sep 15, 2021 • edited Loading

francoisl commented Sep 15, 2021

isagoico commented Sep 15, 2021

akshayasalvi commented Sep 15, 2021

botify commented Sep 15, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 20, 2021

akshayasalvi commented Sep 14, 2021 •

edited

Loading

francoisl commented Sep 15, 2021 •

edited

Loading

isagoico commented Sep 15, 2021 •

edited

Loading