[$50] Workspace chat - Report header shows "0.00 for Pending... @1.00 / mi" when changing distance & rate offline

[izarutskaya]

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.35-4 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/4966766 Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Precondition:

• Workspace has at least two distance rates.

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Submit a distance expense.
4. Go to transaction thread.
5. Go offline.
6. Click Distance.
7. Edit the distance by swapping the waypoints.
8. Save it.
9. Click Rate.
10. Select a different rate.

Expected Result:

Report header will display "Pending" after changing distance and rate offline (production behavior).

Actual Result:

Report header displays "0.00 for Pending... @1.00 / mi" after changing distance and rate offline.

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

https://github.com/user-attachments/assets/c5aff1a5-907a-4142-95cf-eaba49917a8d

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021835712136231108969
• Upwork Job ID: 1835712136231108969
• Last Price Increase: 2024-10-31
• Automatic offers:
• paultsimura | Contributor | 104755241

[melvin-bot[bot]]

Triggered auto assignment to @alexpensify (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[bot]]

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

[github-actions[bot]]

:wave: 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.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[mountiny]

This is very minor and edge case flow with no real burden to the user, demoting

[melvin-bot[bot]]

Upwork job price has been updated to $50

[mountiny]

$50 for finding the offending PR

[CyberAndrii]

Offending PR seems to be https://github.com/Expensify/App/issues/47136

[s77rt]

cc @paultsimura Can you check this one please

[paultsimura]

Managed to reproduce. I'll take a look in the morning. cc: @neil-marcellini

[paultsimura]

@neil-marcellini this is one of those edge cases that I wanted to fix holistically:

1. We should split the calculateAmountForUpdatedWaypointOrRate into 2 separate functions as the logic here is gradually growing apart with every new specific test case:
   • calculateModifiedFieldsForUpdatedWaypoints
   • calculateModifiedFieldsForUpdatedDistanceRate
2. Have a proper way to determine the current unit & rate – possibly hold for https://github.com/Expensify/App/issues/43588 to know what way we are going: updating the BE model or parsing the merchant.

[alexpensify]

@s77rt do you agree with this plan? Thanks!

[s77rt]

@neil-marcellini is working on https://github.com/Expensify/App/issues/43588 and this one is not a priority, holding makes sense to me

[alexpensify]

Thanks for the update. I've moved this to hold, and we can revisit it after #43588 is in production.

[alexpensify]

Weekly Update: On hold for https://github.com/Expensify/App/issues/43588

[alexpensify]

@s77rt - the update is live in NewExpensify, but one is pending for Old. Should we continue to wait or move forward here? Thanks!

[s77rt]

@alexpensify We still need to wait for the frontend changes (https://github.com/Expensify/App/pull/50001)

[alexpensify]

Weekly Update: Updated on hold for https://github.com/Expensify/App/pull/50001

[alexpensify]

@s77rt the changes in https://github.com/Expensify/App/pull/50001 went to production yesterday. Can we move forward now?

[s77rt]

@alexpensify Let's remove the hold. We now have two bugs:

1. If we simply swap the waypoints the distance does not change to "Pending" but it should because the roadmap can still change

https://github.com/user-attachments/assets/86a458b0-e77b-4283-8c34-f453a15bb2ab

2. The report header still changes and shows "0.00 for Pending" if we add a new waypoint

https://github.com/user-attachments/assets/08d8bf26-6d2d-4c44-b61f-68c9a309b73c

[s77rt]

@paultsimura I don't think we need to split calculateAmountForUpdatedWaypointOrRate into two. The bug is that hasModifiedRateWithPendingWaypoints returns false because the updatedTransaction overwrites the existing pendingFields.

Simply adding ...updatedTransaction.pendingFields won't work because after https://github.com/Expensify/App/pull/50001 we added these two lines https://github.com/Expensify/App/blob/be80f3d54bd1f36dd244da680de8140bd31d7de2/src/libs/actions/IOU.ts#L2735 and https://github.com/Expensify/App/blob/be80f3d54bd1f36dd244da680de8140bd31d7de2/src/libs/actions/IOU.ts#L2950

and we don't want to clear pending fields that are set in ACTION1 by ACTION2.

cc @neil-marcellini (for context)

[s77rt]

I think we need to revert the above two lines and handle the problem that we were trying to resolve in a different way, the problem was that if you change the rate to a rate that uses a different unit we also need to change the waypoints (the distance) https://github.com/Expensify/App/blob/f0ec762fc6514b4e2627cbad519e6f40a1e6fd97/src/libs/TransactionUtils/index.ts#L286-L292

Now we basically need to move the above logic and add waypoints to transactionChanges

[paultsimura]

I don't think we need to split calculateAmountForUpdatedWaypointOrRate into two... and we don't want to clear pending fields that are set in ACTION1 by ACTION2.

I can't entirely agree with this approach, let's take a look at the following scenario:

1. We are performing 2 different operations while being offline:
   • Change the distance by moving a waypoint
   • Change the rate
2. For this, we'll want to build a MODIFIEDEXPENSE action for each atomic change: first, the distance was changed, then – the rate.
3. If we don't clear the pending fields between the operations, we'll build "changed distance" and "changed distance + rate" actions.

There might be some other side effects, but this is the first one that comes to my mind, and splitting calculateAmountForUpdatedWaypointOrRate into 2 seems to work fine with it. I'll play with the code and add a proposal a bit later.

[paultsimura]

Edited by proposal-police: This proposal was edited at 2024-10-31 08:56:33 UTC.

Proposal

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

There are 2 bugs as described by @s77rt here.

1. If we simply swap the waypoints the distance does not change to "Pending" but it should because the roadmap can still change

2. The report header still changes and shows "0.00 for Pending" if we go offline, add a new waypoint, and change the rate

What is the root cause of that problem?

For bug 1, we do not reset the customUnit.quantity here:

https://github.com/Expensify/App/blob/70fd756bd82e8fb108b86be64472adbcd53b6848/src/libs/actions/Transaction.ts#L283-L287

Therefore, the hasRoute returns true:

https://github.com/Expensify/App/blob/ce0161406526fe7b333cbca7c08f93a74f5204e5/src/libs/TransactionUtils/index.ts#L658-L660

And the MoneyRequestView doesn't show Pending... because of that:

https://github.com/Expensify/App/blob/f0ec762fc6514b4e2627cbad519e6f40a1e6fd97/src/components/ReportActionItem/MoneyRequestView.tsx#L201-L205

For bug 2, we are trying to handle both distance & rate changes in 1 function – TransactionUtils.calculateAmountForUpdatedWaypointOrRate – which is too generic and doesn't handle the consecutive change correctly.

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

For bug 1, we should reset the quantity:

        comment: {
            waypoints,
            customUnit: {
                quantity: null,
            },
        },

We already do it when saving or removing a waypoint:

https://github.com/Expensify/App/blob/70fd756bd82e8fb108b86be64472adbcd53b6848/src/libs/actions/Transaction.ts#L90-L92

For bug 2, move the TransactionUtils.calculateAmountForUpdatedWaypointOrRate into 2 different places inside the TransactionUtils.getUpdatedTransaction:

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/libs/TransactionUtils/index.ts#L278-L281

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/libs/TransactionUtils/index.ts#L283-L304

And remove these calls:

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/libs/actions/IOU.ts#L2539-L2542

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/libs/actions/IOU.ts#L2877-L2880

Also, refine other places where we can move distance-related transaction modifications from outside the TransactionUtils.getUpdatedTransaction inside it, e.g.:

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/libs/actions/IOU.ts#L2544-L2545

This way, we have more refined control over what data is changed when changing either distance or rate, also have more centralised logic for modifying the transaction, and it allows keeping the updatedTransaction constant inside functions like getUpdateMoneyRequestParams.

Updated approach with pendingWaypoints:

Add ...updatedTransaction.pendingFields here:

https://github.com/Expensify/App/blob/9503d95b4b3b44234b52a435089478c5a453f8bb/src/libs/TransactionUtils/index.ts#L341-L342

Remove setting the pending field here:

https://github.com/Expensify/App/blob/9503d95b4b3b44234b52a435089478c5a453f8bb/src/libs/TransactionUtils/index.ts#L302

Instead, add waypoints to transactionChanges in IOU.updateMoneyRequestDistanceRate by adding this chunk of code:

    const transaction = allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
    if (transaction) {
        const existingDistanceUnit = transaction?.comment?.customUnit?.distanceUnit;
        lodashSet(transaction, 'comment.customUnit.customUnitRateID', transactionChanges.customUnitRateID);
        lodashSet(transaction, 'comment.customUnit.defaultP2PRate', null);

        const newDistanceUnit = DistanceRequestUtils.getUpdatedDistanceUnit({transaction, policy});
        if (existingDistanceUnit && newDistanceUnit !== existingDistanceUnit) {
            transactionChanges.waypoints = TransactionUtils.getWaypoints(transaction);
        }
    }

https://github.com/Expensify/App/blob/13ba38b05941524d43f5263b69429d7ed6fc7813/src/libs/actions/IOU.ts#L3234-L3262

Revert the clearedPendingFields to pre- https://github.com/Expensify/App/pull/50001 state in getUpdateTrackExpenseParams and getUpdateMoneyRequestParams.

What alternative solutions did you explore? (Optional)

[s77rt]

@paultsimura Thanks for the proposal.

Regarding the first bug, the RCA and the solution look good to me. Regarding the second bug, I don't think handling both fields in one function is the problem but I see what you are referring to, here is what I think we should do:

1. Move the calculateAmountForUpdatedWaypointOrRate logic into getUpdatedTransaction so the updatedTransaction becomes a constant. In the getUpdatedTransaction there is the if-blocks where you can have the logic done depending on what field we are actually changing
2. Move https://github.com/Expensify/App/blob/26e1d2d279cb78d62c31d0c189c7dff5b94e7889/src/libs/actions/IOU.ts#L2544-L2545 and https://github.com/Expensify/App/blob/26e1d2d279cb78d62c31d0c189c7dff5b94e7889/src/libs/actions/IOU.ts#L2666-L2668 to getUpdatedTransaction. This will ensure a single source of truth
3. Fix getUpdatedTransaction because it's still returning wrong pendingFields (even though this is not necessary to fix our bug here it's still something we should do)

Let me know if that makes sense

[paultsimura]

One possible drawback of this is that we use calculateAmountForUpdatedWaypointOrRate also when building the optimistic modifiedexpense action. If we move this logic inside getUpdatedTransaction, it won't be reusable🤔

[s77rt]

@paultsimura We don't necessary have to remove that function, just use it in getUpdatedTransaction.

[s77rt]

Not overdue. Still looking for proposals

[s77rt]

Same ^

[alexpensify]

I think we should be able to get proposals now. I updated the title.

[paultsimura]

@s77rt I've updated my proposal using your suggestions. Tried a rough version of it locally – this approach also solves the issue 👍

[s77rt]

@paultsimura Overall looks good to me but can we fix getUpdatedTransaction as well? (to return the correct pendingFields)

[paultsimura]

@s77rt I'm not sure I understand the issue with the pending fields here. Could you please clarify a little?

Moreover, I'm kinda confused by this line:

https://github.com/Expensify/App/blob/e7532e972c28bad5fda2994435b5a37e9aed153b/src/libs/TransactionUtils/index.ts#L302

Doesn't it get always overwritten by this final chunk?

https://github.com/Expensify/App/blob/e7532e972c28bad5fda2994435b5a37e9aed153b/src/libs/TransactionUtils/index.ts#L341-L354

[s77rt]

Right, that's another reason that we need to add ...updatedTransaction.pendingFields,

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[s77rt]

Still working on the proposals

[s77rt]

Same ^

[paultsimura]

@s77rt changing the way we work with pending fields is quite a fundamental change that will need a lot of extra testing for the stacked offline operations. Therefore, if it's not critical for this issue, I would like to avoid it here. However, if you insist – feel free to post this GH in Slack to get more 👀

[s77rt]

@paultsimura We are not changing how we work with pending fields. Why do you think that's the case here?

[paultsimura]

You're right, I was mistaken here. I've updated the proposal – added the Updated approach with pendingWaypoints: section.

[s77rt]

@paultsimura Thank you! Looks good to me but can you check if the linter complains about mutating transactionChanges?

[paultsimura]

if the linter complains about mutating transactionChanges?

No, he's ok with it

[s77rt]

@paultsimura Got it! Let's handle this in the PR.

🎀 👀 🎀 C+ reviewed Link to proposal

[melvin-bot[bot]]

Current assignee @francoisl is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[paultsimura]

Note to self: there were some changes to the existing logic in https://github.com/Expensify/App/issues/51285, need to make sure it doesn't break after this GH.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[paultsimura]

The PR is ready for review: https://github.com/Expensify/App/pull/52197