[$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: @hoangzinh

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

@hoangzinh @bfitzexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@hoangzinh, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[hoangzinh]

Thanks @FitseTLT. I will review it soon.

[melvin-bot]

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

[hoangzinh]

@FitseTLT is it possible to show errors for users to know if updating distance is failed?

[hoangzinh]

Like this:

[FitseTLT]

@hoangzinh Updated

2024-11-26.00-00-56.mp4

[hoangzinh]

@FitseTLT can you share screenshot of transaction?.errorFields? Why it's routes (not route) in this case? And is there any regression bugs if we change it to routes? I just double-checked and found that we still reference to route recently, for example: #51944

[FitseTLT]

@FitseTLT can you share screenshot of transaction?.errorFields? Why it's routes (not route) in this case? And is there any regression bugs if we change it to routes? I just double-checked and found that we still reference to route recently, for example: #51944

Yes Thanks @hoangzinh you are right looks like the backend sets errorFields.route on bad waypoints entered.
Ok now we have two options

1. if you agree on the error showing below the map as my latest results we can add errorFields.waypoints case in addition to errorFields.route here
   

   App/src/components/ReportActionItem/MoneyRequestView.tsx

   Line 388 in 8c4661d

   ...(transaction?.errorFields?.route ?? transaction?.errors),

   
   and also we will clear it waypoint error here
   

   App/src/libs/actions/Transaction.ts

   Line 461 in 8c4661d

   Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {errors: null, errorFields: {route: null}});

2. or we can omit showing error message and just revert the values only on failureData
   WDYT

[hoangzinh]

Do you mind to share recordings of option 1 & 2? @FitseTLT

[FitseTLT]

Do you mind to share recordings of option 1 & 2? @FitseTLT

@hoangzinh Option 1 will be exactly as here and option 2 will only revert the value without showing error.

[hoangzinh]

Oh I missed this recording. Thanks @FitseTLT. Can you attach it to your proposal here? Also, can you hide your previous proposal? Thank you.

[FitseTLT]

@hoangzinh Done

[hoangzinh]

@FitseTLT proposal looks good to me

Link to proposal #52248 (comment)

🎀👀🎀 C+ reviewed

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