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

[kbecciv]

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

---

Action Performed:

1. Set a big distance rate on a workspace (e.g.: 90000000)
2. Request money -> distance
3. Fill start and finish points
4. Select that workspace
5. Click on the requested money
6. Click on Amount

Expected Result:

The user should be able to edit the Amount

Actual Result:

User cannot edit the large amount after creating the distance

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.68.12
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

big-rate.webm

Recording.4411.mp4

Expensify/Expensify Issue URL:
Issue reported by: @hichamcc
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694207504466369

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0155a08bb4447cde17

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to @johncschuster (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)

[akinwale]

Proposal

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

User cannot edit large amounts after creating the distance.

What is the root cause of that problem?

There is a max length limit enforced on the input field in MoneyRequestAmountForm which prevents the ability to edit amounts greater than that limit. The current max length is 10 as defined in the app constants.

App/src/CONST.ts

Line 1059 in 2163c54

AMOUNT_MAX_LENGTH: 10,

This is used in the MoneyRequestUtils#validateAmount method.

App/src/libs/MoneyRequestUtils.js

Line 61 in 2163c54

return amount === '' || (decimalNumberRegex.test(amount) && calculateAmountLength(amount) <= CONST.IOU.AMOUNT_MAX_LENGTH);

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

Solution 1
Increase the max length limit for the input field.

Solution 2
Set a maximum limit on the amount that a distance request can be. This maximum limit cannot be more than the allowed limit in the MoneyRequestAmountForm. If the amount is greater than this limit, do not allow the user to proceed. Either display a client-side validation error, or validate on the backend and return an error.

In this case, the maximum amount limit would be 99,999,999.99, taking into account the fact that the amounts could have 2 decimal places.

Alternatively, just use the MoneyRequestUtils#validateAmount method (or just calculateAmountLength - we have to add this to the exports in MoneyRequestUtils) to check the amount and display a validation error in the distance request flow.

Solution 3
Set a maximum limit on the distance rate. This will still need to be combined with Solution 2.

What alternative solutions did you explore? (Optional)

None.

[b4s36t4]

Proposal

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

Web - User cannot edit the large amount after creating the distance

What is the root cause of that problem?

We have a hard limit on the IOU request Amount which we're checking before updating.

Here in this case even we're deleting the amount we're still checking limit which is causing the issue not to update the amount.

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

Line 126 in f57e5e2

if (!MoneyRequestUtils.validateAmount(newAmountWithoutSpaces)) {

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

1. We can restrict the distance rate so that amount can be reduced a bit (although we had set limit it can be reduced some-more), but that would still raise issues.

2. Let user delete the amount even if the max-length (10) reached, using the length of currentAmount and newAmount we're getting from input.

If we do this user can't edit the amount till he reaches the max-length condition, say he want to replace 0 with 1 he can't. the above solution just
allow user to delete amount.

3. Or if we've reached max limit, i.e any distance request/scan receipt which we depends on scan or distance units set by workspace we don't let user edit the amount because they are pre-defined.

This requires user to edit the workspace units which might require us to update all the existing requests to use the update units and update amount
for them as well

What alternative solutions did you explore? (Optional)

NA

[jjcoffee]

@greg-schroeder Are you able to find out what sort of BE validation of the amount length exists? Looks like AMOUNT_MAX_LENGTH was added quite a long time ago, so unsure if this comment still reflects the current state of the BE.

Leaning towards @akinwale's solution 2 here, as it makes sense to me that we'd not want to allow users to create such large distance requests in the first place, unless there's a good reason for it that I'm missing!

[b4s36t4]

@jjcoffee I think restricting max amount for distance request require multiple changes. Also we have max distance limit & as well as limit on distance rate for workspace too. So I feel it's max distance is not thw way to go.

[jjcoffee]

@b4s36t4 The main issue as I see it is that we can now make bigger money requests by making a distance request vs. manual request, which is inconsistent and leads to us not being able to edit the amount. So either there's a reason for allowing that (which I can't think of!), or we haven't implemented the right validation to stop this from happening (i.e. prevent a distance request from being created if it exceeds the max amount). Feel free to make a case if you disagree though!

[greg-schroeder]

@jjcoffee I am not positive - can you take that question to Slack to discuss with the greater team?

[b4s36t4]

I think yes there's a difference between the two features but as far as I understand the distance feature we can not restrict the user request based on the distance. At the same time we calculate amount based on the distance rate configured in the workspace which only a admin can do.

Either way a user can request money by splitting distance but that would make the feature irrelevant.

I feel at this instance we can only give the editing power to only admin if it's a distance request and it exceeded max length. Also it's not easy to restrict the distance request amount as I said it depends on two factors which might change at any instance which requires us reiterate the issue again.

[greg-schroeder]

Leaning towards @akinwale's solution 2 here, as it makes sense to me that we'd not want to allow users to create such large distance requests in the first place, unless there's a good reason for it that I'm missing!

Leaning more towards this myself the more I think about it

[jjcoffee]

@greg-schroeder I've asked on Slack to feel out if there are any differences of opinion.

[melvin-bot]

@jjcoffee, @greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jjcoffee]

Bumped on Slack. If there isn't much more feedback over the weekend, I think we'll just settle on @akinwale's solution no. 2.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.76-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: prevent requesting for amounts larger than the allowed maximum #28266
• [CP Staging] fix: pass decimals parameter to validateAmount in MoneyRequestConfirmationList #28685

If no regressions arise, payment will be issued on 2023-10-10. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @akinwale requires payment
• @jjcoffee requires payment
• @hichamcc requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
• [@jjcoffee] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@jjcoffee] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@jjcoffee] Determine if we should create a regression test for this bug.
• [@jjcoffee] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.77-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] fix: pass decimals parameter to validateAmount in MoneyRequestConfirmationList #28685

If no regressions arise, payment will be issued on 2023-10-12. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @akinwale requires payment
• @jjcoffee requires payment
• @hichamcc requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@akinwale / @jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
• [@akinwale / @jjcoffee] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@akinwale / @jjcoffee] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@akinwale / @jjcoffee] Determine if we should create a regression test for this bug.
• [@akinwale / @jjcoffee] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[greg-schroeder]

processing

[greg-schroeder]

@akinwale offer sent ($250)
@jjcoffee offer sent ($250)
@hichamcc offer sent ($50)

[akinwale]

@akinwale offer sent ($250) @jjcoffee offer sent ($250) @hichamcc offer sent ($50)

The regression that happened here couldn't have been caught during testing because it was due to a separate PR that was merged at the same time. Would the penalty still apply in this case?

cc @aldo-expensify

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A - behaviour was always there
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug. Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Set a big distance rate on a workspace (e.g.: 90000000)
2. Open FAB -> Request money -> Distance
3. Set any start and finish points and continue
4. Verify that the "Invalid amount" error shows

Do we agree 👍 or 👎

[jjcoffee]

@greg-schroeder Checklist complete!

Also agree with @akinwale - the regression penalty isn't meant to be applied if the review checklist was completed properly (per the C+ process doc at least!). The other PR could also be argued to have caused the regression since it was merged later than our one 😄

[melvin-bot]

@akinwale, @jjcoffee, @greg-schroeder, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[greg-schroeder]

Okay I will review this either today or tomorrow

[greg-schroeder]

All right I agree @jjcoffee and @akinwale. I'll send over a new offer to account for the difference.

[greg-schroeder]

Filed regression test

[jjcoffee]

@greg-schroeder Thanks - offer accepted!