[HOLD for payment 2024-12-25] [$250] Distance-Distance field is stuck on pending... and no error shown when updating of waypoints fails

[IuliiaHerets]

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: 9.0.59-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Submit a distance expense with two waypoints.
4. Go to transaction thread.
5. Go offline.
6. Click Distance.
7. Edit the distance by swapping the position and save it.

Expected Result:

The distance field will change to "Pending" when editing the distance by swapping in offline mode.

Actual Result:

The distance field does not show "Pending" when editing the distance by swapping in offline mode.
It shows the same amount but it is grayed out.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6658691_1731061670854.20241108_182248.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021854958931528313891
• Upwork Job ID: 1854958931528313891
• Last Price Increase: 2024-11-22
• Automatic offers:
• FitseTLT | Contributor | 105096400

Issue Owner

Current Issue Owner: @bfitzexpensify

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (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]

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

[melvin-bot]

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

[ikevin127]

Proposal

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

The distance field does not show "Pending" when editing the distance by swapping in offline mode.

What is the root cause of that problem?

The getDistanceForDisplay function is responsible for returning either the actual distance or Pending... depending on the arguments passed, the key one in our case for showing Pending... being hasRoute.

This is where the function is called and the hasRoute is passed. This is what the hasRoute function checks:

App/src/libs/TransactionUtils/index.ts

Lines 694 to 696 in 533aed6

	function hasRoute(transaction: OnyxEntry<Transaction>, isDistanceRequestType?: boolean): boolean {
	return !!transaction?.routes?.route0?.geometry?.coordinates || (!!isDistanceRequestType && !!transaction?.comment?.customUnit?.quantity);
	}

In our case the first condition always passes because in updateWaypoints we set geometry.coordinates to null, resulting in !!transaction?.routes?.route0?.geometry?.coordinates passing because coordinates object exists with null value.

The reason why the second condition passes as well is because the !!transaction?.comment?.customUnit?.quantity check returns true as well since we do have quantity calculated already from the current transaction route and when we update it while online we don't reset the parent object as we should.

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

In updateWaypoints set customUnit object to null since its always populated by BE upon returning online.

function updateWaypoints(transactionID: string, waypoints: WaypointCollection, isDraft = false): Promise<void | void[]> {
    return Onyx.merge(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
        comment: {
            waypoints,
            customUnit: null, // <- Add this line
        },
...

This change will also account for the following case of distance expense confirmation while offline:

Distance Cofirmation Change - Offline

Screen.Recording.2024-11-08.at.14.28.09.mov

Solution result

MacOS: Chrome

Screen.Recording.2024-11-08.at.14.17.42.mov

[hoangzinh]

Thanks for your proposals @FitseTLT @ikevin127.

@ikevin127 provided a more thorough root cause than @FitseTLT, I couldn't understand the root cause without your proposal. However, I lean towards to @FitseTLT solution, I think we only need to optimistic set

           customUnit: {
                quantity: null,
            },

is enough to fix our issue.

[ikevin127]

@hoangzinh Thanks for the feedback, however, regarding your decision, quoting from the proposal template (code):

ATTN: Contributor+

You are the first line of defense in making sure every proposal has a clear and easily understood problem with a "root cause". Do not approve any proposals that lack a satisfying explanation to the first two prompts. It is CRITICALLY important that we understand the root cause at a minimum even if the solution doesn't directly address it. When we avoid this step, we can end up solving the wrong problems entirely or just writing hacks and workarounds.
...
3. Choose the first proposal that has a reasonable answer to all the required questions.

I thought we cannot hire a contributor who's proposal's RCA is not clear enough, in this case I'd expect the proposal that provided a clear RCA and working solution to be assigned.

My bad, I didn't know that there are exceptions to those rules. I'll make sure to avoid issues reviewed by you in the future since I usually post proposals with those rules in mind 👍

[FitseTLT]

I think this issue has pretty clear RCA that's why I put a concise proposal.
It's pretty clear updateWaypoints is called on drag

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

Line 519 in 533aed6

onDragEnd={updateWaypoints}

and the obsolete customUnit data should be cleared and that is what we do for saveWaypoints which I mentioned in my proposal (that's why the problem doesn't occur if we change or add a waypoint from IOURequestStepWaypoint)
I would have made the review's job easier if I put more detail, I'm sorry about that 🙏 , but I thought it was pretty straightforward.

[hoangzinh]

My bad, I didn't know that there are exceptions to those rules

No worry @ikevin127. If it's a guideline, we should follow it and avoid exceptions as much as possible. Tbh, I haven't made any decisions here yet. And because I haven't been satisfied with any proposals, I just shared my thoughts.

[hoangzinh]

quoting from the proposal template (code):

It is CRITICALLY important that we understand the root cause at a minimum

Hmm. Tbh, it's hard to define minimum. @FitseTLT's root cause is correct but not "minimum" to me. @FitseTLT, can you add more details to your RCA? Thank you.

[FitseTLT]

@hoangzinh Updated

[hoangzinh]

@ikevin127 As I mentioned here, your RCA looks great but your solution doesn't look good to me. I think if we go down with that approach, we should update the minimum data that can solve our issue. Update more data than we need, it will potentially clear data in somewhere.
If you still think we should set customUnit: null, can you elaborate on it? Or if you have a better approach, feel free to update your proposal. Thank you

[FitseTLT]

There is more data in customUnit than we need to clear

App/src/types/onyx/Transaction.ts

Lines 89 to 94 in 533aed6

	type TransactionCustomUnit = {
	/** ID of the custom unit */
	customUnitID?: string;
	/** ID of the custom unit rate */
	customUnitRateID?: string;

We can't be clearing customUnitRateID, for instance.

[ikevin127]

@hoangzinh It doesn't matter anymore since I just noticed that the same code suggested in the first proposal's solution was just merged in PR:

• fix: 0.00 for Pending... @1.00 / mi #52197

return Onyx.merge(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
    comment: {
        waypoints,
+       customUnit: {
+           quantity: null,
+       },
    },

which means our issue is not reproducible anymore on dev and soon won't be reproducible on staging either once the PR will be deployed.

If you really want a complete solution for this issue, you'd have to look deeper into what happens in the edge case where editing distance call fails.

Here's a comparison of the transaction related optimisticData without any fix applied to updateWaypoints vs any of the solutions suggested so far as proposals on this issue:

No Fix	Latest Fix (Proposal 1 Solution)

Notice the customUnit object differences ⬆️

The issue is that even with this fix which seemingly fixes our issue according to the Expected result:

The distance field will change to "Pending" when editing the distance by swapping in offline mode.

In case the UpdateMoneyRequestDistance call fails for some reason, we're going to be stuck with Distance field enabled and showing Pending... upon returning back online, without any error message. I simulated network error in order to test this, here's a comparison on the differences between editing the Date field with failing request vs Distance field:

Editing Date - Failed Request	Editing Date - Failed Request (with current fix)
Screen.Recording.2024-11-11.at.15.26.47.mov	Screen.Recording.2024-11-11.at.15.23.29.mov

Note: We did not have anything in place before the fix anyway, but at least before the fix the Distance would show the previous data in case the API call failed.

[hoangzinh]

It's a great finding @ikevin127.

cc @bfitzexpensify TLDR; the original issue is fixed in this PR. But @ikevin127 found another edge case here that makes "Pending" status still show even the API call is failed. What do you think if we should fix it in this issue?

[bfitzexpensify]

Let's fix it!

[hoangzinh]

@ikevin127 do you mind updating your proposal again? including the problem you want to solve, RC + solution. Thank you.

[melvin-bot]

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

[ikevin127]

@hoangzinh @bfitzexpensify This one is complicated, I looked into it for a few hours but I wasn't able to come up with a fix. Maybe we can raise the bounty to get more eyes on the issue and even update the description for clarity.

[hoangzinh]

Thanks @ikevin127. @bfitzexpensify, what do you think? If we still want to solve this edge-case issue, I guess we need to update the issue's title + description, then wait for a few days to get more eyes on it.

[melvin-bot]

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

[FitseTLT]

@bfitzexpensify please change the title of the issue to Distance-Distance field is stuck on pending... and no error shown when updating of waypoints fails

[pecanoro]

Sounds good! Assigning @FitseTLT

[melvin-bot]

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

[mvtglobally]

Issue not reproducible during KI retests. (First week)

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

• Fix - Distance-Distance field is stuck on pending... and no error shown when updating of waypoints fails #53329

If no regressions arise, payment will be issued on 2024-12-25. 🎊

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

• @hoangzinh requires payment through NewDot Manual Requests
• @FitseTLT requires payment automatic offer (Contributor)

[melvin-bot]

@hoangzinh @bfitzexpensify @hoangzinh The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[hoangzinh]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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: There is no offending PR. It's a new scenario that we haven't covered before.

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Precondition:

• Enable "Distance rates" on a workspace
• Create at least two distance rates on that workspace

Test:

1. Go to workspace chat above.
2. Submit a distance expense with distance rate A.
3. Go to workspace settings > Distance rates.
4. Delete distance rate A (from Step 3).
5. Go to workspace chat.
6. Go to transaction thread of the distance expense.
7. Click Distance.
8. Edit the distance and save it.
9. Dismiss the error "The selected rate has been deleted"
10. Verify that the receipt and distance field will be reverted to the previous value.

Do we agree 👍 or 👎

[bfitzexpensify]

Payment summary:

@FitseTLT paid $250 for contributor work via Upwork ✅
@hoangzinh due $250 via NewDot manual request

[hoangzinh]

Requested payment in ND

[garrettmknight]

$250 approved for @hoangzinh