[$250] Expense - Leading zeros are not removed when returning to amount input page

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.80-9
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4592856
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Submit expense > Manual.
3. Enter many zeros, then any digit > Next.
4. Return to amount input page via back button.

Expected Result:

The leading zeros will be removed.

Actual Result:

The leading zeros are not removed.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6504348_1717705798824.20240607_041959.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d1b0565d440e64fb
• Upwork Job ID: 1799101354337638448
• Last Price Increase: 2024-06-07
• Automatic offers:
• hungvu193 | Reviewer | 102719101
• abzokhattab | Contributor | 102719103

Issue Owner

Current Issue Owner: @hungvu193

[melvin-bot]

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

Triggered auto assignment to @techievivek (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[kavimuru]

@techievivek FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[kavimuru]

We think this bug might be related to #wave-collect - Release 1

[neonbhai]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Expense - Leading zeros are not removed when returning to amount input page

What is the root cause of that problem?

This happens as in the Submit Expense flow we access the IOU amount page from here:

App/src/pages/iou/request/IOURequestStartPage.tsx

Line 166 in 1426c4e

<IOURequestStepAmount route={route} />

This does not have the shouldKeepUserInput prop passed to it.

Since the prop is false by default:

App/src/pages/iou/request/step/IOURequestStepAmount.tsx

Line 74 in 1426c4e

shouldKeepUserInput = false,

The last param here is false, leading to the amount not being formatted.

App/src/pages/iou/request/step/IOURequestStepAmount.tsx

Line 171 in 1426c4e

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

What changes do you think we should make in order to solve the problem?

Since <IOURequestStepAmount is only accessed from the IOURequestStartPage, we can simply have the prop true by default.

App/src/pages/iou/request/step/IOURequestStepAmount.tsx

Line 74 in 1426c4e

shouldKeepUserInput = false,

shouldKeepUserInput = false, 

Alternatively

• We can pass shouldKeepUserInput here

• We should also consider renaming the prop to shouldFormatUserInput to better reflect what its doing

[trjExpensify]

Have we identified the PR on staging that caused this bug?

[luacmartins]

I think this PR caused the regression

[techievivek]

Not a backend blocker.

[sakluger]

I don't think this needs to be a deploy blocker, since it doesn't block the user from doing anything, it's just a display bug in a single flow.

cc @trjExpensify in case you disagree.

[techievivek]

Yeah, it doesn't break anything related to making request so we can remove the blocker label.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d1b0565d440e64fb

[melvin-bot]

📣 @Rakshh! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[Har103]

Just saw this on UW
And I might give a stupid solution.

Why are we not checking user input at the time of input (like on change event) and removing the leading zeros or any non numaric character at that time.
A small sterilization function should not cause any performance issues.

[melvin-bot]

📣 @Har103! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[jainilparikh]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When the user enters leading 0's in the input, these extra 0's are not removed.

What is the root cause of that problem?

Issue causing PR (https://github.com/Expensify/App/pull/40062/commits)

The PR introduces a new flag, called shouldKeepUserInput (in the component IOURequestStepAmount) which determines if the user input needs to be sanitised or not.

The above mentioned flag is NOT provided a value when the IOURequestStepAmount is rendered inside the IOURequestStartPage page here:

https://github.com/Expensify/App/blob/main/src/pages/iou/request/IOURequestStartPage.tsx#L157

What changes do you think we should make in order to solve the problem?

Set the flag to False

 <TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
                                        {() => (
                                            <IOURequestStepAmount
                                                shouldKeepUserInput = {false}
                                                route={route}
                                            />
                                        )}
</TopTab.Screen>

Validated that the above solution works locally.

[hungvu193]

@abzokhattab Can you take a look at this issue, it seems to have stem from #40062

Thanks for proposals everyone. But I think we should wait for this comment first.

[hungvu193]

@abzokhattab Can you take a look at this issue, it seems to have stem from #40062

little bump @abzokhattab on this comment

[abzokhattab]

Thank you @hungvu193 sure i will work on it

[hungvu193]

Hold for #43243 (comment)

[melvin-bot]

📣 @hungvu193 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @abzokhattab 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[techievivek]

Assigning it to @abzokhattab based on #43243 (comment)

[hungvu193]

@techievivek I think the original C+ will handle this one right?

[sakluger]

Good call @hungvu193. I'll reassign @eVoloshchak as the C+.

[sakluger]

Hey @abzokhattab - do you have a proposal or PR to fix this issue yet?

[abzokhattab]

isn't this already the expected behavior according to #35797 (comment) and #34894 (comment)?

cc @dangrous @eVoloshchak

[dangrous]

So yeah this is currently expected behavior - to maintain user input in this flow. I could see us making an exception for leading zeros, but I don't think this should count as a regression.

Basically our reasoning was that it's confusing if what you put in changed when you go back to it, and we do all the parsing afterwards. Again, this can be open for discussion, but shouldn't be a regression.

We could also go the other route that Square Cash (and maybe others) take, and just disallow leading zeros from the beginning.

[sakluger]

Thanks for clarifying @dangrous. I agree that we don't need to count this as a regression, but we should change the behavior to disallow leading zeros.

[sakluger]

You know what - since this is the expected behavior at the moment, I'm going to close the issue for now. We can reopen later or open a new issue if we want to change the behavior.