[WAITING ON FEDI - CHECKLIST] Web - Distance - Amount does not change if waypoint is changed via distance option

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.23-0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Click on FAB > Request money > Distance
2. Set start point and finish point > Click next > Select a workspace
3. Observe the amount
4. Click back two times
5. Change the finish way point > Click next > Select a workspace
6. Observe the amount has changed
7. Click back two times
8. Change the finish point to the original finish point selected on step 2
9. Click next > Select a workspace
10. Observe the amount
11. While on confirmation page click on distance
12. Change the finish point > Click on next
13. Observe the amount

Expected Result:

The amount should change since the finish point has been changed

Actual Result:

The amount does not change when user changes the finish point via the distance option from the confirmation page (without going back two times)

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

Add any screenshot/video evidence

Bug6337433_1704875155064.PR-Fail-33959.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01eb4b0c3cc7b8e197
• Upwork Job ID: 1745052607647092736
• Last Price Increase: 2024-01-10
• Automatic offers:
• fedirjh | Reviewer | 28103764
• dukenv0307 | Contributor | 28103765

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[DanutGavrus]

Proposal

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

Amount does not change if waypoint is changed via distance option.

What is the root cause of that problem?

In theMoneyTemporaryForRefactorRequestConfirmationList component we have a hook around shouldCalculateDistanceAmount which is not set to true when we change the distance from this component.

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

We should set shouldCalculateDistanceAmount to true when changing any waypoint from here.

What alternative solutions did you explore?

[FIRST EDIT - Alternative solution added]

From what I see, getDistanceRequestAmount method inside DistanceRequestUtils makes use of simple math operations. I think we could remove our custom shouldCalculateDistanceAmount from MoneyTemporaryForRefactorRequestConfirmationList, and:

A. let the component calculate it each time.
B. make use of 'useMemo' so it re-calculates only when one or more values from distance, unit, or rate changes.

[FitseTLT]

Proposal

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

Distance - Amount does not change if waypoint is changed via distance option

What is the root cause of that problem?

The root cause is that shouldCalculateDistanceAmount is not set to true on changing transaction route distance here

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Line 257 in e9f27ab

const shouldCalculateDistanceAmount = isDistanceRequest && iouAmount === 0;

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

We should Change it to

const prevDistance = usePrevious(distance);
    const shouldCalculateDistanceAmount = isDistanceRequest && (iouAmount === 0 || distance !== prevDistance);

What alternative solutions did you explore? (Optional)

We can also use the same technique for rate and unit as we are using them to calculate amount

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Line 290 in e9f27ab

shouldCalculateDistanceAmount ? DistanceRequestUtils.getDistanceRequestAmount(distance, unit, rate) : iouAmount,

or
We can also use transaction.amount here

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Line 290 in e9f27ab

shouldCalculateDistanceAmount ? DistanceRequestUtils.getDistanceRequestAmount(distance, unit, rate) : iouAmount,

[dukenv0307]

Proposal

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

The amount does not change when user changes the finish point via the distance option from the confirmation page (without going back two times)

What is the root cause of that problem?

We're not clearing the distance in Onyx when saving the waypoint.

When we save the new waypoint, we're already setting the amount to 0 here. It should have triggered this line to calculate and update the new correct amount to Onyx. But it didn't work because the distance is still stale (not updated via back-end yet), so the amount to be saved is still the old amount, we can verify this by adding logs, it will show that the transition of the amount is old amount > 0 > new amount.

So the problem here is that the distance is stale when saving new waypoint.

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

Clear the distance in Onyx when saving the waypoint here

distance: null,

It doesn't make sense to keep the distance because the waypoint is already changed and the distance will be recalculated. We're already doing the same when removing the waypoint here

We also already clearing the old route here when saving waypoint due to the same reason.

// Clear the existing route so that we don't show an old route

What alternative solutions did you explore? (Optional)

We can check other operations related to amount as well, and make sure we also remove the distance in that case.

[jliexpensify]

Bump @fedirjh for reviews.

I've also let Dylan know here: https://expensify.slack.com/archives/C05DWUDHVK7/p1705021251503859

[fedirjh]

Thanks everyone for the proposal. I believe @dukenv0307 has correctly identified the root cause of the bug. shouldCalculateDistanceAmount evaluates to true when we select a new waypoint (which is expected), but the distance is not updated (stale). This distance is crucial in calculating the new amount. Resetting the distance seems to fix the issue.

Let's proceed with this proposal from @dukenv0307

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @Beamanator, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[DanutGavrus]

@fedirjh
What do you think about removing our custom logic for shouldCalculateDistanceAmount? Wouldn't it be better in order to prevent bugs like this? Here we'll clear the distance, but we may later encounter similar issues regarding the unit, or the rate. I'm just asking. If you consider this being a worse solution, no problem. Thanks!

[fedirjh]

What do you think about removing our custom logic for shouldCalculateDistanceAmount? Wouldn't it be better in order to prevent bugs like this?

I am not really sure how removing that logic will fix this bug? As I stated, the bug arises from stale data not from the custom logic, So even if we remove the custom logic, we will still use the stale data (in this case the distance), and the bug will still exist.

[DanutGavrus]

With this diffs the bug does not reproduce anymore:

So even if we remove the custom logic, we will still use the stale data (in this case the distance), and the bug will still exist.

I logged the distance, it comes updated in the component. Only problem is that shouldCalculateDistanceAmount remains false. I've also tried to add a distanceRequestAmount const at the top which makes use of useMemo and it still works.

[fedirjh]

Only problem is that shouldCalculateDistanceAmount remains false

Can you please elaborate more on this? why doesn't the hook work in this case? The amount is reset to 0 when we save a new waypoint, So technically the hook should work. From the testing, it seems that the hook works fine, but it uses the old distance value.

App/src/libs/actions/Transaction.ts

Line 67 in 1f7a97b

amount: CONST.IOU.DEFAULT_AMOUNT,

[DanutGavrus]

Can you please elaborate more on this?

I logged shouldCalculateDistanceAmount and distance right in the useEffect at the top.

true 1601790.625
false 1601790.625

true 1601790.625
false 1601790.625
false 2850321.75
false 2850321.75

First 2 logs are from the first time we enter the component in this flow. Next 4 ones are after changing the distance from the component. Indeed, we use the old distance value when shouldCalculateDistanceAmount is true. When the new value gets set to the distance, shouldCalculateDistanceAmount is already false.

I think you two are right, clearing the distance will return the new value from the start and will fix this bug. I was just thinking if logic may be simplified by removing the custom shouldCalculateDistanceAmount and making use of:

const distanceRequestAmount = useMemo(() => DistanceRequestUtils.getDistanceRequestAmount(distance, unit, rate), [distance, unit, rate]);

Thus, preventing cases like this one and also taking unit and rate into account. Maybe we may do both things, or just that fix. In any case, I'm sure you know better which is the right solution here.

[dukenv0307]

Thus, preventing cases like this one and also taking unit and rate into account.

@DanutGavrus I think it will only hide the bugs. If the data is stale, that fix will make it looks like it's working fine. But in reality the data is still stale and will cause bugs and unintended side effects in other places.

So IMO we should fix it at the root.

[melvin-bot]

📣 @fedirjh 🎉 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]

📣 @dukenv0307 🎉 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 📖

[melvin-bot]

@Beamanator, @jliexpensify, @fedirjh, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[jliexpensify]

Not overdue! PR being worked on

[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.4.33-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• clear distance when save waypoint #34779

If no regressions arise, payment will be issued on 2024-02-07. 🎊

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

• @fedirjh requires payment automatic offer (Reviewer)
• @dukenv0307 requires payment automatic offer (Contributor)

[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:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] 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:
• [@fedirjh] 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:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] 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.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@Beamanator, @jliexpensify, @fedirjh, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Beamanator]

Seems like still waiting on payment - bump @jliexpensify

[jliexpensify]

Oh hey sorry, missed this one! @fedirjh bump on the checklist please

[jliexpensify]

Paid and job closed

[fedirjh]

BugZero Checklist:

• Link to the PR: Refactor the money request creation flow #28618
• Link to comment: Refactor the money request creation flow #28618 (comment)
• Link to discussion: N/A should be caught during the PR testing phase, but it looks like the PR is huge.
• Determine if we should create a regression test for this bug: Yes

---

Regression Test Proposal

1. Click on FAB -> Request money -> Distance
2. Set start point and finish point -> Click next -> Select a workspace
3. Observe the amount
4. While on confirmation page click on distance
5. Change the finish point -> Click on next
6. Verify that: The amount changed since the finish point has been changed

• Do we agree 👍 or 👎