[HOLD for payment 2024-08-14] [$250] Distance - Distance still shows Pending when preview and report header already show distance

[lanitochka17]

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: 1.4.82-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: https://expensify.testrail.io/index.php?/tests/view/4620914
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
4. Go to expense report while the receipt is being generated
5. Note that the distance is already computed in the expense preview
6. Go to transaction thread while the receipt is being generated
7. Note that the distance in transaction thread header is also computed
8. Note that the Distance row still shows "Pending" while it is computed in Step 6 and 7

Expected Result:

The Distance row in transaction thread should display the distance when the distance is computed

Actual Result:

The Distance row in transaction thread displays "Pending" when the expense preview and transaction thread header already display the distance

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

Bug6510345_1718175159906.bandicam_2024-06-12_14-45-41-618.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01531942e35c6e84c9
• Upwork Job ID: 1801172514335645153
• Last Price Increase: 2024-07-18

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

@trjExpensify 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]

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

[bernhardoj]

Proposal

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

Distance value shows Pending... even though the distance is shown/calculated in the confirmation list and in the preview.

What is the root cause of that problem?

If there is a pending action of waypoints, it will show Pending...

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 283 in 8563b9f

title={getPendingFieldAction('waypoints') ? translate('iou.fieldPending') : distanceToDisplay}

When we create a new request, an ADD pending action is set, that's why the Pending... is shown.

App/src/libs/actions/IOU.ts

Line 1963 in 8563b9f

isDistanceRequest ? {waypoints: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD} : undefined,

It's to address this comment where the distance value still shows the old value after update, so Pending... is shown while waiting for the updated distance value.

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

In main, when we change the waypoints, the distance value is already updated from GetRoute request, so I don't think we need to show Pending... anymore by always showing the distanceToDisplay here.

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 283 in 8563b9f

title={getPendingFieldAction('waypoints') ? translate('iou.fieldPending') : distanceToDisplay}

title={distanceToDisplay}

(or update the condition to specifically check for update pending action)

However, I see another issue. The distance shown is in meters even though the unit is in miles.

Screen.Recording.2024-06-13.at.00.20.01.mov

It's because, GetRoute API returns the distance in meters, but we try to convert it from miles to meters here,

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 190 in 8563b9f

const distance = DistanceRequestUtils.convertToDistanceInMeters(TransactionUtils.getDistance(transaction), unit);

The reason we try to convert it from miles to meters is because after the transaction is created (RequestMoney)/waypoints is updated (UpdateMoneyRequestDistance), the distance value is in miles instead of meters which is different from GetRoute response. I think this needs to be discussed first how to fix it. Maybe we can update GetRoute to returns the distance value based on the workspace unit (miles/km).

Based on the discussion, we want to get the distance from transaction.routes.route0.distance if it exist (which is in meters), otherwise, we get it from transaction.comment.customUnit.quantity (which is in saved unit). The first thing to do is to clear the routes object from the transaction when the create/update distance request is completed.

App/src/libs/actions/IOU.ts

Lines 689 to 696 in 7c37886

	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`,
	value: {
	pendingAction: null,
	pendingFields: clearedPendingFields,
	},
	},

App/src/libs/actions/IOU.ts

Lines 2707 to 2715 in 7c37886

	successData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
	value: {
	pendingFields: clearedPendingFields,
	isLoading: false,
	errorFields: null,
	},
	});

Next, we create a new TransactionUtils function called getDistanceInMeters which contains the logic above.

function getDistanceInMeters(transaction, unit) {
    if (transaction.routes.route0.distance) return transaction.routes.route0.distance;
    if (transaction.comment.customUnit.quantity && unit) return DistanceRequestUtils.convertToDistanceInMeters(transaction.comment.customUnit.quantity, unit);
    return 0;
}

Then, replace all TransactionUtils.getDistance usage with TransactionUtils.getDistanceInMeters.

For MoneyRequestView, we want to replace the whole line with TransactionUtils.getDistanceInMeters.

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 210 in 7c37886

const distance = DistanceRequestUtils.convertToDistanceInMeters(TransactionUtils.getDistance(transaction), unit);

[trjExpensify]

Hm, I think this was a vip-vsb initiative originally. @paultsimura and @neil-marcellini I recall have the subject matter knowledge on this one.

[melvin-bot]

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

[melvin-bot]

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

[parasharrajat]

However, I see another issue. The distance shown is in meters even though the unit is in miles.

Nice catch.

Maybe we can update GetRoute to return the distance value based on the workspace unit (miles/km).

I like this suggestion. However, it might create more issues if we are supposed to use a static unit in calculations. But I differ this to internal team who has better idea on this.

---

Other than this, @bernhardoj Did you check for the offline behaviour?

[melvin-bot]

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

[parasharrajat]

@madmax330 What do you think about the suggestion and problem mentioned in #43569 (comment). (More details on the proposal #43569 (comment))

[bernhardoj]

Other than this, @bernhardoj Did you check for the offline behaviour?

If we apply my suggestion and do the steps while offline, then we will see the distance as

{distanceInMeters} miles ❌ 

[madmax330]

Hmm I'm not sure what the best way forward is here.

cc @neil-marcellini since you worked quite a bit on distance requests IIRC can you take a look?

[melvin-bot]

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

[melvin-bot]

@madmax330, @trjExpensify, @parasharrajat Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[parasharrajat]

Need some help from @neil-marcellini

[melvin-bot]

@madmax330 @trjExpensify @parasharrajat 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]

@madmax330, @trjExpensify, @parasharrajat Eep! 4 days overdue now. Issues have feelings too...

[trjExpensify]

I've asked Neil if he can take a look here.

[neil-marcellini]

Thanks for the DM Tom! Wow, this is kind of a headache. Thanks for figuring out what's going in your proposal @bernhardoj!

I agree that we should show the distance optimistically for the request since it was already fetched with GetRoute. Regarding the distance being in meters instead of the proper unit, it's not currently a problem since we show Pending... instead.

However, I probably made a mistake with updating the GetRoute api command to return distance and set in in Onyx under transaction.comment.customUnit.quantity. I didn't realize/remember that the distance is stored at that path in the user selected unit upon creation. It's a bad idea to have different units stored under the same path, even if we could determine the unit based on whether it's optimistic or not. Too messy.

We can't really convert into the proper unit for the GetRoute response because we don't know what policy the expense will be linked to yet. Therefore I think we'll probably have to go back to only getting the distance from transaction.routes.route0.distance. Luckily that data is still present in the response. I'll create a low priority cleanup issue to stop returning it under the customUnit eventually.

@bernhardoj would you please update your proposal to make the app always get the distance from that path for optimistic distance requests? Once the request is created the distance can come from customUnit.quantity. It would be nice to have some util functions for this with nice names to make it clear how it works, or maybe just modify TransactionUtils.getDistance to handle that logic. Whatever you feel is more reasonable.

@madmax330 I'm going to take this over, but let me know if you would rather handle it.

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

• Fix distance still shows Pending when preview and report header already show distance #46165

If no regressions arise, payment will be issued on 2024-08-14. 🎊

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

• @parasharrajat requires payment through NewDot Manual Requests
• @bernhardoj requires payment through NewDot Manual Requests

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

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

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @parasharrajat owed $250 via NewDot
• Reviewer: @bernhardoj owed $250 via NewDot

BugZero Checklist (@trjExpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1801172514335645153/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[trjExpensify]

Checklist time!

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: [CP Staging] Feat/36985 create new rate field followups #40711
• [@parasharrajat] 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: https://github.com/Expensify/App/pull/40711/files#r1718487624
• [@parasharrajat] 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: NA
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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 Steps

Prerequisite:

• Have a workspace with multiple distance rates
• P2P distance enabled
• Enable distance rate tax tracking
• Set Tax Rate and Tax reclaimable on for each rate

1. Open a workspace expense chat
2. Press the + button > Submit expense > Switch to distance tab
3. Enter any waypoints and next
4. In the confirmation page, try changing the rate value
5. Verify the amount and tax is updated
6. Submit the request
7. Open the request transaction thread
8. Verify the Distance field shows the distance value instead of Pending...
9. Press the Distance field and update the waypoints
10. Verify the distance value is updated

Do you agree 👍 or 👎 ?

[trjExpensify]

Thanks! @neil-marcellini will regression tests for the P2P Distance project be added centrally when the project is wrapped?

[trjExpensify]

Payment summary as follows:

• $250 to @parasharrajat for the C+ review
• $250 to @bernhardoj for the fix

Feel free to go ahead and submit on ND!

[bernhardoj]

Requested in ND.

[melvin-bot]

@trjExpensify, @parasharrajat, @neil-marcellini, @bernhardoj Huh... This is 4 days overdue. Who can take care of this?

[JmillsExpensify]

$250 approved for @bernhardoj

[melvin-bot]

@trjExpensify, @parasharrajat, @neil-marcellini, @bernhardoj 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[trjExpensify]

I'm back, reached out to @neil-marcellini on this Q before closing this out.

[neil-marcellini]

Thanks! @neil-marcellini will regression tests for the P2P Distance project be added centrally when the project is wrapped?

Yes

[trjExpensify]

Great, closing!

[parasharrajat]

Payment requested as per #43569 (comment)

[JmillsExpensify]

$250 approved for @parasharrajat