[$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
• [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/Expensify/App/assets/78819774/29318e87-4d14-4030-88ef-b34f03bc3f4c

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-04

Issue Owner

Current Issue Owner: @neil-marcellini

[melvin-bot[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... https://github.com/Expensify/App/blob/8563b9f77f21ded0dce09572ab6e1983c3f1fabd/src/components/ReportActionItem/MoneyRequestView.tsx#L283

When we create a new request, an ADD pending action is set, that's why the Pending... is shown. https://github.com/Expensify/App/blob/8563b9f77f21ded0dce09572ab6e1983c3f1fabd/src/libs/actions/IOU.ts#L1963

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. https://github.com/Expensify/App/blob/8563b9f77f21ded0dce09572ab6e1983c3f1fabd/src/components/ReportActionItem/MoneyRequestView.tsx#L283

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.

https://github.com/Expensify/App/assets/50919443/a95ee41b-f76a-4568-9c63-afc54c0e2401

It's because, GetRoute API returns the distance in meters, but we try to convert it from miles to meters here, https://github.com/Expensify/App/blob/8563b9f77f21ded0dce09572ab6e1983c3f1fabd/src/components/ReportActionItem/MoneyRequestView.tsx#L190

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

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

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

[melvin-bot[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[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 https://github.com/Expensify/App/issues/43569#issuecomment-2173622674. (More details on the proposal https://github.com/Expensify/App/issues/43569#issuecomment-2163457710)

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

[bernhardoj]

We can't really convert into the proper unit for the GetRoute

GetRoute API is called when editing the distance, so maybe it's possible, idk though.

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

Ok, so if I understand correctly, I would do it like this:

(in MoneyRequestView)
const draftDistance = TransactionUtils.getDraftDistance(transaction);
const distance = draftDistance ?? DistanceRequestUtils.convertToDistanceInMeters(TransactionUtils.getDistance(transaction), unit);

getDraftDistance gets the distance from transaction.routes.route0.distance. getDistance stays the same.

But I see that after the distance request is created, transaction.routes.route0.distance is still exists, so we will never use the transaction.comment.customUnit.quantity. So, can we just use transaction.routes.route0.distance all the time instead?

[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? 💸

[melvin-bot[bot]]

@trjExpensify, @parasharrajat, @neil-marcellini Eep! 4 days overdue now. Issues have feelings too...

[neil-marcellini]

Ok, so if I understand correctly, I would do it like this:

(in MoneyRequestView)
const draftDistance = TransactionUtils.getDraftDistance(transaction);
const distance = draftDistance ?? DistanceRequestUtils.convertToDistanceInMeters(TransactionUtils.getDistance(transaction), unit);

getDraftDistance gets the distance from transaction.routes.route0.distance. getDistance stays the same.

But I see that after the distance request is created, transaction.routes.route0.distance is still exists, so we will never use the transaction.comment.customUnit.quantity. So, can we just use transaction.routes.route0.distance all the time instead?

@bernhardoj thanks for the update. I'm thinking of something a bit different. You're right that transaction.routes.route0.distance doesn't get cleared. We can't use it all the time though, because it's not saved on the backend, so it will be cleared upon sign out. As a first step, let's merge routes: null on the transaction in the success data for createDistanceRequest and trackExpense for distance.

Next, let's try to make it really clear with the names of the functions/the JSDoc comments, what each value represents.

• getDraftDistanceInMeters(transaction) will get it from the route as you described
• getDistanceInSavedUnit(transaction) will get it from the customUnit.quantity
• getDistance(transaction, unit) will try to get it from the draft, then convert it into the provided unit. If that's not available it will get it in the saved unit.
• getDistanceInMeters(transaction, unit) will work like getDistance, but return the draft directly or convert the saved unit into meters.

Maybe we should only export the bottom two functions. What do you think about this? Please let me know if you spot any problems or have ideas how it could be more simple.

[bernhardoj]

We can't use it all the time though, because it's not saved on the backend, so it will be cleared upon sign out. As a first step, let's merge routes: null on the transaction in the success data for createDistanceRequest and trackExpense for distance.

I see. But if we merge routes: null, the coordinate is cleared too which caused the same issue as https://github.com/Expensify/App/issues/44156.

https://github.com/Expensify/App/assets/50919443/37a4cb36-1b17-4eb5-80a4-608a27c79784

I think we can solve that first. (I posted a proposal there)

Maybe we should only export the bottom two functions. What do you think about this? Please let me know if you spot any problems or have ideas how it could be more simple.

Looking through the current getDistance usages, all of them expect the distance in meters, so I think we only need getDistanceInMeters. Do you agree? And just to make sure I understand the description of getDistanceInMeters, does the pseudo-code below correct? (tested it and works fine so far)

fn getDistanceInMeters(transaction, unit):
    if route.distance:
        return route.distance
    if customUnit.quantity and unit:
        return toMeters(customUnit.quantity, unit)
    return 0 // fallback

[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? 💸