Use transaction distanceUnit to prevent retroactive changes by neil-marcellini · Pull Request #50001 · Expensify/App

neil-marcellini · 2024-10-01T15:07:01Z

Details

Fixed Issues

#43588
PROPOSAL: N/A

Tests

Precondition:

P2P distance beta is enabled
Distance rates are enabled

Original bug:

Go to workspace chat
Submit two distance expenses (if there is no other unsettled expense)
Go to workspace settings
Go to Distance rates > Settings > Unit
Select a different unit (km)
Go to transaction thread
Verify that the unit is unchanged in the Distance and Rate rows in transaction thread
Click on the rate row and verify the unit of the selected rate is unchanged
Verify that the transaction thread header shows the unit unchanged
Go to expense report
Verify the expense preview shows the unit unchanged
Update the distance, adding a waypoint
Verify the new unit is used on the transaction thread, header, expense preview, on the selected rate

1001Record241002081325.mov

Test without the beta:

Sign in with an account not on the p2pDistanceRequests beta, or manually disable it on dev by making this function return false
Have a report with 2+ distanced expenses
Change the policy distance unit to a different one (km) than the unit used for a distance expense (mi)
Verify the distance unit is unchanged on the expense preview, transaction thread header, and distance row
Update the distance
Verify the distance unit is changed to the current policy distance unit immediately on the expense preview, transaction thread header, and distance row

no-beta.mov

Test updating rate updates distanceUnit and converts distance

Submit a distance expense on a policy that has multiple rates
Go to the policy settings and note the distance unit
Update the distance unit
Go to the existing expense and verify the unit is unchanged
Update the rate
Verify the unit updates everywhere it's shown on the expense
Click the rate field and verify it shows the new unit

2024-10-09_15-13-02.mp4

Note: There's an existing bug on main where the map doesn't load and the modified expense action doesn't show as the most recent one. Here's proof it fails on main too

BugOnMain2024-10-09_15-23-28.mp4

Tracked expense

Go to OldDot settings, workspaces, individual, open it
Go to reports, verify the report currency is USD
Track a distance expense
Verify unit is mi and rate is $ and mi
Go to OldDot and change the personal policy report currency to EUR
Go to NewDot, refresh the page
Verify the distance unit is still mi
Note: The rate currency not matching the amount will be fixed separately here, we only care that the unit isn't updated retroactively

2024-10-04_08-21-16.mp4

Distance split

Go to OldDot settings, workspaces, individual, open it
Go to reports, verify the report currency is USD
Create a group chat
Split a distance expense with the group
Verify unit is mi and rate is $ and mi
Go to OldDot and change the personal policy report currency to EUR
Go to NewDot, refresh the page
Verify the distance unit is still mi

2024-10-04_08-24-57.mp4

Optimistic distance unit

P2P distance beta Enabled
Login with a new account
Create a workspace and enable distance rates feature
Change distance unit to km
Press FAB (+) > Submit request
Chose Distance tab, select start and stop waypoints then press Next
Select workspace then press Next
Go offline
Click on Submit
Open the expense report
Verify the unit is in km

2024-10-16_11-25-35.mp4

I also updates some of the allowed write counts where I found issues during testing. I haven't added any extra writes with these PRs.

Verify that no errors appear in the JS console

Offline tests

N/A optimistic data is tested above

QA Steps

Same a tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

See above. I only tested on Chrome since changes should be platform independent and C+ will test all platforms.

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

neil-marcellini · 2024-10-07T15:07:27Z

I created this issue to do the migration separately to keep this PR focused [$250] Migrate IOURequestStepDistanceRate to useOnyx

melvin-bot · 2024-10-07T15:31:44Z

@MarioExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

neil-marcellini · 2024-10-07T17:08:18Z

        removed: 'removed',
        transactionPending: 'Transaction pending.',
-        chooseARate: ({unit}: ReimbursementRateParams) => `Select a workspace reimbursement rate per ${unit}`,
+        chooseARate: 'Select a workspace reimbursement rate per mile or kilometer',


This works better when the selected rate is in a different unit than the current policy distance unit. I think it works fine when the unit is all the same too.

s77rt · 2024-10-07T21:03:11Z

@neil-marcellini I believe the distance value should get corrected

s77rt · 2024-10-07T22:05:05Z

+    const unit = DistanceRequestUtils.getDistanceUnit(transaction, mileageRate);
+    const {rate} = mileageRate ?? {};


We have two sources of truth mileageRate.unit and unit which could be confusing and error-prone. Can we have the returned mileageRate already have the correct unit. i.e. write a function DistanceRequestUtils.getRate and in that function pass both the policy and the transaction and set the unit accordingly.

Also I think we can remove the isCustomUnitRateIDForP2P, getRateForP2P and getCustomUnitRateID (not sure about this one) functions or at least not export them. The getRate function should be enough

That's a pretty good idea, thanks. I'll work on it.

I ended up adding two new util functions for getting the rate of existing transactions and then getting the distance unit for use when updating a transaction.

I left one use of these other utils you recommended removing, because making the current code work with the new utils would require creating a temporary transaction, setting the customUnitRateID from the transaction changes into it, then using one util for the rate and another for the unit. It's just as messy as the existing code. Not to mention that I would also need to change a lot of the OnyxEntryOrInput types to work with the OnyxEntry types. So I'm going to leave that as is.

App/src/libs/TransactionUtils/index.ts

Lines 807 to 809 in 7982c26

const mileageRate = isCustomUnitRateIDForP2P(transaction)

? DistanceRequestUtils.getRateForP2P(policyCurrency)

: mileageRates?.[customUnitRateID] ?? DistanceRequestUtils.getDefaultMileageRate(policy);

Please let me know what you think in your next review.

neil-marcellini · 2024-10-08T14:12:59Z

@neil-marcellini I believe the distance value should get corrected

Oh shoot, you're totally right! I can't believe I missed that, thanks for pointing it out. When the rate changes the distance unit, the distance should be converted into the new unit. I'll put this back to a draft while I work on that backend fix.

melvin-bot · 2024-10-16T19:25:30Z

@carlosmiceli Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot · 2024-10-16T19:25:33Z

🎯 @s77rt, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #50941.

MarioExpensify · 2024-10-17T14:08:51Z

Hey @neil-marcellini, everything looks pretty good with the PR. Waiting for all tests to wrap, just a question regarding the ESLint issues (withOnyx deprecated), should we move forward and let this to be fixed by another PR or should we fix in this PR?

neil-marcellini · 2024-10-17T14:33:12Z

Yeah I have a separate issue for that here [$250] Migrate MoneyRequestConfirmationList to useOnyx. Merging!

melvin-bot · 2024-10-17T14:33:31Z

@neil-marcellini looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

neil-marcellini · 2024-10-17T14:33:41Z

See comment above

OSBotify · 2024-10-17T14:43:33Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-10-18T19:04:30Z

🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.51-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mvtglobally · 2024-10-18T19:04:51Z

@neil-marcellini @MarioExpensify Can we use Expensifail account for BETA enable tests and Gmail for non-Beta?

MarioExpensify · 2024-10-21T13:30:19Z

@neil-marcellini @MarioExpensify Can we use Expensifail account for BETA enable tests and Gmail for non-Beta?

I'm not 100% sure on that, @neil-marcellini do you know if that would be ok?

github-actions · 2024-10-22T04:37:46Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.51-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

github-actions · 2024-10-22T17:20:10Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.51-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

paultsimura · 2024-11-07T18:32:00Z

    const hasRoute = TransactionUtils.hasRoute(transactionBackup ?? transaction, isDistanceRequest);
-    const rateID = TransactionUtils.getRateID(transaction) ?? '-1';
-
-    const currency = transactionCurrency ?? CONST.CURRENCY.USD;


Seems like this change re-introduced a bug we fixed in #50142.
Any objections to reverting to using this line instead of currency from DistanceRequestUtils.getRate({transaction, policy})?

@neil-marcellini @s77rt

Bug:

Place a distance request to a workspace

In the WS settings, change the currency

Expected: distance rate on the submitted request shows the original currency.
Actual: distance rate shows the updated currency even though the amount is calculated in the original currency.

I think we need to move that logic to DistanceRequestUtils.getRate.
Is there any case where we don't want to use the transaction currency and instead use the rate's currency? I think there is only one case, that is if we change the rate to a rate that uses a diff currency (same with unit useTransactionDistanceUnit)

Do you know why the currency is displayed correctly on the rate change page?

Do you know why the currency is displayed correctly on the rate change page?

We display the transaction's currency for the selected rate:

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

Line 69 in ce01614

const rateForDisplay = DistanceRequestUtils.getRateForDisplay(unit, rate.rate, isSelected ? transactionCurrency : rate.currency, translate, toLocaleDigit);

I think we need to move that logic to DistanceRequestUtils.getRate

Moving it into DistanceRequestUtils.getRate will complicate the function with an extra parameter and an extra if-else check. If you think it's worth it, I'll add a useTransactionCurrency param.

Using transactionCurrency directly is fine. Having too many sources of truth is confusing and will always lead to such bugs. We probably need to cleanup this at a later time (or at least centralize it)

MarioExpensify · 2024-11-11T12:51:18Z

@neil-marcellini should we reopen this as it caused a regression?

s77rt · 2024-11-11T12:58:52Z

@MarioExpensify Can you please link to the regression?

ishpaul777 · 2024-11-19T09:19:07Z

+            const conversionFactor = existingDistanceUnit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES ? CONST.CUSTOM_UNITS.MILES_TO_KILOMETERS : CONST.CUSTOM_UNITS.KILOMETERS_TO_MILES;
+            const distance = NumberUtils.roundToTwoDecimalPlaces((transaction?.comment?.customUnit?.quantity ?? 0) * conversionFactor);
+            lodashSet(updatedTransaction, 'comment.customUnit.quantity', distance);
+            lodashSet(updatedTransaction, 'pendingFields.waypoints', CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE);


Coming from #51285 {BZ checklist}, setting pending action on waypoint caused the map preview to be blank when selecting distance rate with different unit. There more to RC see detailed Analysis in this Proposal #51285 (comment)

Use transaction distanceUnit to prevent retroactive changes

b9d9f6b

neil-marcellini self-assigned this Oct 1, 2024

neil-marcellini added 6 commits October 1, 2024 08:22

Display rate with transaction's distance unit

3cedd46

Prompt with both units for when transaction unit doesn't match policy

d6c88de

WIP optimistically set distanceUnit

778dd23

Pass existing transaction in all distance cases

540f11a

Optimistically update to current distance unit when editing

3961d4c

Prettify

4520fe3

neil-marcellini mentioned this pull request Oct 4, 2024

[P2P Distance] Rate currency doesn't match expense currency #50142

Merged

49 tasks

Merge branch 'main' into neil-distanceUnit

7579b73

neil-marcellini mentioned this pull request Oct 7, 2024

[HOLD for payment 2024-10-25] [$250] Migrate IOURequestStepDistanceRate to useOnyx #50334

Closed

neil-marcellini added 2 commits October 7, 2024 08:08

Remove unused types

9936790

Missed a spot

56c96fb

neil-marcellini marked this pull request as ready for review October 7, 2024 15:31

neil-marcellini requested a review from a team as a code owner October 7, 2024 15:31

melvin-bot Bot requested a review from MarioExpensify October 7, 2024 15:31

melvin-bot Bot removed the request for review from a team October 7, 2024 15:31

neil-marcellini commented Oct 7, 2024

View reviewed changes

neil-marcellini requested a review from s77rt October 7, 2024 17:09

s77rt suggested changes Oct 7, 2024

View reviewed changes

neil-marcellini marked this pull request as draft October 8, 2024 14:13

neil-marcellini added 5 commits October 9, 2024 11:08

Merge branch 'main' into neil-distanceUnit

ebff76e

WIP use util to get transaction distance rate

9a06cfe

Util to get updated distance unit

7982c26

Use get rate util in another spot

5c43231

Simplify using lodashSet

e07a837

s77rt approved these changes Oct 16, 2024

View reviewed changes

melvin-bot Bot requested a review from carlosmiceli October 16, 2024 19:25

melvin-bot Bot mentioned this pull request Oct 16, 2024

Payment issue for E/App PR#50001 #50941

Closed

neil-marcellini removed the request for review from carlosmiceli October 17, 2024 13:47

Merge branch 'main' into neil-distanceUnit

f0ec762

MarioExpensify approved these changes Oct 17, 2024

View reviewed changes

neil-marcellini merged commit 20b7017 into main Oct 17, 2024

neil-marcellini deleted the neil-distanceUnit branch October 17, 2024 14:33

melvin-bot Bot added the Emergency label Oct 17, 2024

neil-marcellini removed the Emergency label Oct 17, 2024

ntdiary mentioned this pull request Oct 18, 2024

Fix - Distance - Tax amount shows value A then changes to value B after changing distance rate #51006

Closed

50 tasks

FitseTLT mentioned this pull request Oct 18, 2024

[Awaiting Payment] [$250] Distance - Tax amount shows value A then changes to value B after changing distance rate #50641

Closed

6 tasks

github-actions Bot mentioned this pull request Oct 18, 2024

Deploy Checklist: New Expensify 2024-10-18 #51106

Closed

59 tasks

paultsimura reviewed Nov 7, 2024

View reviewed changes

ishpaul777 reviewed Nov 19, 2024

View reviewed changes

etCoderDysto mentioned this pull request Dec 25, 2025

[$250] Confirm details - Unit doesn't change from Km to Mile when switching workspaces after selecting non-default rate #78212

Closed

7 tasks

		const unit = DistanceRequestUtils.getDistanceUnit(transaction, mileageRate);
		const {rate} = mileageRate ?? {};

	const mileageRate = isCustomUnitRateIDForP2P(transaction)
	? DistanceRequestUtils.getRateForP2P(policyCurrency)
	: mileageRates?.[customUnitRateID] ?? DistanceRequestUtils.getDefaultMileageRate(policy);

Conversation

neil-marcellini commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

neil-marcellini commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melvin-bot Bot commented Oct 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s77rt commented Oct 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neil-marcellini commented Oct 8, 2024

Uh oh!

melvin-bot Bot commented Oct 16, 2024

Uh oh!

melvin-bot Bot commented Oct 16, 2024

Uh oh!

MarioExpensify commented Oct 17, 2024

Uh oh!

neil-marcellini commented Oct 17, 2024

Uh oh!

melvin-bot Bot commented Oct 17, 2024

Uh oh!

neil-marcellini commented Oct 17, 2024

Uh oh!

OSBotify commented Oct 17, 2024

Uh oh!

github-actions Bot commented Oct 18, 2024

Uh oh!

mvtglobally commented Oct 18, 2024

Uh oh!

MarioExpensify commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 22, 2024

Uh oh!

github-actions Bot commented Oct 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paultsimura Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarioExpensify commented Nov 11, 2024

Uh oh!

s77rt commented Nov 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

neil-marcellini commented Oct 1, 2024 •

edited

Loading

neil-marcellini commented Oct 7, 2024 •

edited

Loading

MarioExpensify commented Oct 21, 2024 •

edited

Loading

paultsimura Nov 7, 2024 •

edited

Loading