[$250] Distance - Inconsistency in showing unit in preview and transaction thread after updating unit

[lanitochka17]

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: 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/4621218
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• Distance rates is enabled

1. Go to staging.new.expensify.com
2. Go to workspace chat
3. Submit two distance expenses (if there is no other unsettled expense)
4. Go to workspace settings
5. Go to Distance rates > Settings > Unit
6. Select a different unit
7. Go to transaction thread
8. Note that the new unit is applied in Distance and Rate rows in transaction thread
9. Note that the transaction thread header shows the previous unit
10. Go to expense report

Expected Result:

There should be consistency in showing updated distance unit in transaction thread and preview in expense report

Actual Result:

In Step 8, thenew unit is applied in Distance and Rate rows in transaction thread
In Step 9, transaction thread header shows the previous unit
In Step 10, the expense preview also shows the previous unit

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

Bug6510280_1718170765275.bandicam_2024-06-12_13-26-43-493.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014c60d0efed5e27db
• Upwork Job ID: 1801098418852168279
• Last Price Increase: 2024-06-13

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

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

@jliexpensify 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

[jliexpensify]

@lanitochka17 is this only happening on Chrome?

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-08-12 08:14:20 UTC.

Proposal

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

In Step 8, thenew unit is applied in Distance and Rate rows in transaction thread
In Step 9, transaction thread header shows the previous unit
In Step 10, the expense preview also shows the previous unit

What is the root cause of that problem?

1. When we enable P2P distance, we display the distance text with the current unit of the WS.

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 192 in fb323f6

const distanceToDisplay = DistanceRequestUtils.getDistanceForDisplay(hasRoute, distance, unit, rate, translate);

2. Meanwhile, the description text in money request preview is the merchant of the transaction and the unit is the unit at the time we create or edit this distance not the current unit of the WS

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 88 in fb323f6

const requestMerchant = truncate(merchant, {length: CONST.REQUEST_PREVIEW.MAX_LENGTH});

That is the inconsistency.

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

Both distance display in transaction thread report and money request preview are wrong. The one has wrong distance value with current unit, the other has wrong unit and distance value isn't converted. For two cases, we should display the distance with the current unit of the WS and the convert distance value with this unit.

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 192 in fb323f6

const distanceToDisplay = DistanceRequestUtils.getDistanceForDisplay(hasRoute, distance, unit, rate, translate);

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 88 in fb323f6

const requestMerchant = truncate(merchant, {length: CONST.REQUEST_PREVIEW.MAX_LENGTH});

To do that we can create a util to get the display distance of the distance request. In this function, we will get the unit of transaction via the merchant or modifiedMerchant of the transaction, get the current unit of the WS via policy, get the distance value via transaction. And then we will convert the distance value to the current unit of the WS and then return the display distance text with the converted distance value and the current unit of the WS

What alternative solutions did you explore? (Optional)

For the distance from WS, we should get the unit via merchant

1. Create a util to get the transaction unit via merchant

function getTransactionUnit(transaction: OnyxEntry<Transaction>): Unit | undefined {
    // For distance request, the merchant has the format `distance unit @ rate / unit` so we can get the unit as the last text when splitting the merchant
    return !isDistanceRequest(transaction) || isCustomUnitRateIDForP2P(transaction) ? undefined : (getTransactionDetails(transaction)?.merchant.split(' ')?.at(-1) as Unit);
}

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 243 in 0980fcd

const {unit} = mileageRate;

2. Then we can use this unit here if it exists

const transactionUnit = TransactionUtils.getTransactionUnit(transaction);
const {unit: policyUnit} = mileageRate;
const unit = transactionUnit ?? policyUnit;

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 243 in 0980fcd

const {unit} = mileageRate;

Or BE should store the unit in transaction.comment.customUnit which is the unit when we save the transaction and returns it in Onyx data. Then we can get the unit of the distance transaction via transaction.comment.customUnit.unit

[lanitochka17]

@jliexpensify Reproducible in all environment.

[s77rt]

I was not able to reproduce this. The money request info stayed the same (with old unit) which I think is the correct behaviour otherwise the amount will have to change. @nkdengineer Is this still reproducible for you?

[nkdengineer]

@s77rt I can reproduce this issue. Are you enable the P2PDistance beta, it's required to reproduce this bug.

[s77rt]

@nkdengineer I was able to reproduce after enabling the beta 👍 I think we should hold this on #36987 cc @jliexpensify

[jliexpensify]

Thanks, have adjusted the issue!

[neil-marcellini]

The decision here is that we don't really care about fixing this for existing distance transactions, as there could be millions of those to update and given that the bug exists on OldDot and hasn't been seeming to bother people it's not really worth fixing historically.

I will work on PRs to start saving and using the transaction distance unit, and if it's unavailable we'll fallback to the current method 🤷‍♂️. I hope to get started sometime this week.

[neil-marcellini]

Cool I got it saving for creating requests. I'll add tests for editing next, and then also any places where it's read in Web-E. For example I should test the exact steps of this issue, verifying the merchant is unchanged when the policy unit changes.

[jliexpensify]

Just a heads up I'm OOO from 3rd to 14th, but I don't think anything is needed from me during this period. Feel free to reassign to another B0 if needed though!

[neil-marcellini]

I got the bug fixed and working well with and without the beta. I still need to do some fixes for when the rate is updated, and I should check that this works for tracked distance expenses and splits too.

[neil-marcellini]

I just realized that I also need to fix this for distance expenses created from Expensify classic. The current PRs only relate to New Expensify.

I think it's fine to make the update for the old and new platforms separately because the distanceUnit is currently not stored at all, so it's fine if new is storing it and old isn't for a little while. That will keep the PRs focused. I'm noting this so I don't forget to update for classic.

[neil-marcellini]

I made a few more changes on the App PR from review feedback and I'm ready for another C+ review.

[neil-marcellini]

@jliexpensify this is on prod now, next step is payment.

[jliexpensify]

PAYMENT SUMMARY

• C+: @s77rt $250 (payment owed via New.Dot)

[s77rt]

Yes 👍

[jliexpensify]

Any checklist needed @s77rt?

[s77rt]

@jliexpensify I don't think that's needed here given that this bug comes from a new feature that's still in beta. As for the regression test(s) they will be added at once #23291 https://github.com/Expensify/Expensify/issues/437283

[jliexpensify]

Awesome, thanks! I'll close this out but here is the linked Payment Summary for your payment @s77rt

[JmillsExpensify]

$250 approved for @s77rt