[HOLD for payment 2024-08-02] [$250] Distance - Amount does not update after selecting a different workspace with different rate

lanitochka17 commented 1 month ago

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: 9.0.7-4 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4723512 Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Workspace A has distance rate A
Workspace B has distance rate B
1. Go to staging.new.expensify.com
2. Go to FAB > Submit expense > Distance
3. Enter waypoints
4. Select Workspace A
5. On confirmation page, click Rate and select Distance rate A
6. Note that the amount is amount A
7. Click back button on confirmation page
8. Select Workspace B

Expected Result:

After selecting Workspace B, the distance amount in the confirmation page will adjust accordingly to Amount B based on Distance rate B

Actual Result:

After selecting Workspace B, the distance amount in the confirmation page is still Amount A when the selected rate is Distance rate B

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/9fc4093a-d135-411c-a56b-9aa4fbdaed87

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010e501d4313825dcb
Upwork Job ID: 1814314096244747204
Last Price Increase: 2024-07-19

Issue Owner

Current Issue Owner: @CortneyOfstad

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @CortneyOfstad (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.

lanitochka17 commented 1 month ago

@CortneyOfstad 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

lanitochka17 commented 1 month ago

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

Krishna2323 commented 1 month ago

Proposal

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

Distance - Amount does not update after selecting a different workspace with different rate

What is the root cause of that problem?

shouldCalculateDistanceAmount remains false when the mileage rate is changed. https://github.com/Expensify/App/blob/1465a9294f5ff357fb30f55c1e90df791c73cd29/src/components/MoneyRequestConfirmationList.tsx#L268C11-L268C40

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

Update the condition to also check for change in mileage rate.

    const prevMileageRate = usePrevious(mileageRate);
    const shouldCalculateDistanceAmount = isDistanceRequest && (iouAmount === 0 || prevRate !== rate || prevDistance !== distance || mileageRate !== prevMileageRate);

What alternative solutions did you explore? (Optional)

Result

https://github.com/user-attachments/assets/1085ee39-0fdd-4562-809d-5578f1f71c60

CortneyOfstad commented 1 month ago

Was able to recreate, so going to get eyes on this!

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~010e501d4313825dcb

melvin-bot[bot] commented 1 month ago

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

CortneyOfstad commented 1 month ago

@Ollyws we have a proposal here when you have a chance — thank you!

tienifr commented 1 month ago

Proposal

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

After selecting Workspace B, the distance amount in the confirmation page is still Amount A when the selected rate is Distance rate B

What is the root cause of that problem?

In the confirmation page, we have a logic to check whether we should update the transaction amount here.
It is used to reduce the redundant call of function IOU.setMoneyRequestAmount here entire its life cycle, for example when we are in the confirmation page, then navigate to any other screens (the confirmation page is still on the screen stack), then navigate back to the confirmation page.
But in case of this bug, after step 8 in OP's reproduction steps, the confirmation page is mounted. Now the condition here is always false because the rate is never changed on this page but it is changed when we select the another policy in step 8(I only tell about the rate in the scope of this bug because it is the factor leads to the bug).
Because shouldCalculateDistanceAmount is always false, so logic to update amount here is never called.

What changes do you think we should make in order to solve the problem?
We should make sure logic: https://github.com/Expensify/App/blob/0d2bd2a4c8e77d03658a66142bcf6069900b471b/src/components/MoneyRequestConfirmationList.tsx#L334 is always called at least one time in confirmation page 's life cycle.

We can use the below:

const isFirstUpdatedDistanceAmount = useRef(false);
useEffect(() => {
    if (isFirstUpdatedDistanceAmount.current) {
        return;
    }
    if (!isDistanceRequest) {
        return;
    }
    const amount = DistanceRequestUtils.getDistanceRequestAmount(distance, unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, rate ?? 0);
    IOU.setMoneyRequestAmount(transactionID, amount, currency ?? '');
    isFirstUpdatedDistanceAmount.current = true;
}, [distance, unit, rate, transactionID, isDistanceRequest]);

What alternative solutions did you explore? (Optional)

We can call this in here, right after we change the participant, and only call it if requestType === CONST.IOU.REQUEST_TYPE.DISTANCE

Ollyws commented 1 month ago

I think as @Krishna2323 was the first to propose a working solution going with their proposal is more fair even if @tienifr's is somewhat more in-depth, unless there's any way in which it can actually cause a regression?

tienifr commented 1 month ago

@Ollyws I think this solution does not work. The additional condition mileageRate !== prevMileageRate is just a comparison between two object, which always return true. For example:
```
const obj1 = {name: 'Tom' }
const obj2 = {name: 'Tom' }
console.log(obj1 !== obj2) // true
```
In our case, mileageRate !== prevMileageRate return true after step 8, so we consider that the solution will fix the issue, but the fact that it is not. Still, there is the case mileageRate !== prevMileageRate return false.
If we apply that solution, you can try the below steps, and you still can reproduce the bug:

Precondition:

Workspace A has distance rate A Workspace B has distance rate B

Go to staging.new.expensify.com
Go to FAB > Submit expense > Distance
Enter waypoints
Select Workspace A
On confirmation page, click Rate and select Distance rate A
Note that the amount is amount A
Click back button on confirmation page. Then refresh the page. (Important step)
Select Workspace B

https://github.com/user-attachments/assets/0eda5f6c-27b5-4b61-b79c-f229102246d3

Ollyws commented 1 month ago

I think this solution does not work. The additional condition mileageRate !== prevMileageRate is just a comparison between two object, which always return true.

This doesn't seem to be the case for me for, when we change the milage rate milageRate !== prevMilageRate will be true:

https://github.com/user-attachments/assets/b1a2b1fb-9436-4a12-8bc7-382279bebc56

tienifr commented 1 month ago

@Ollyws Have you tried my reproduce steps above?

Ollyws commented 1 month ago

Yeah that it reproducible. Let's go with @tienifr's proposal then. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/46092

If no regressions arise, payment will be issued on 2024-08-02. :confetti_ball:

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

@Ollyws requires payment through NewDot Manual Requests
@tienifr requires payment through NewDot Manual Requests

melvin-bot[bot] commented 1 month ago

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:

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

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @jliexpensify (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.

CortneyOfstad commented 3 weeks ago

@Ollyws can you please update the checklist when you have a moment so there is no delay in payment? Thanks!

@jliexpensify I am heading OoO so reassign so there is not a delay in payment. Thank you!

jliexpensify commented 3 weeks ago

Payment Summary

C: @tienifr $250 (ND Payment)
C+: @Ollyws $250 (ND Payment)

Waiting on checklist

jliexpensify commented 3 weeks ago

Bump @Ollyws for checklist

Ollyws commented 3 weeks ago

BugZero Checklist:

[x] The PR that introduced the bug has been identified. Link to the PR:

I don't think we can pin this on any particular PR, it seems that this has always been missing since inception.

[x] 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

[x] 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

[x] Determine if we should create a regression test for this bug.

Yes

[x] 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. Create a distance expense to workspace A
2. In confirmation page, note the amount
3. Press Back
4. Select Workspace B
5. In confirmation page, verify the amount is updated according to distance rate (different from workspace A)

Do we agree 👍 or 👎

github-actions[bot] commented 3 weeks ago

true

Ollyws commented 3 weeks ago

Requested in ND.

jliexpensify commented 3 weeks ago

Here's the payment summary for J.Mills!

JmillsExpensify commented 3 weeks ago

$250 approved for @Ollyws

Expensify / App