Use transaction distanceUnit to prevent retroactive changes

[neil-marcellini]

Details

Fixed Issues

#43588
PROPOSAL: N/A

Tests

Precondition:

• P2P distance beta is enabled
• Distance rates are enabled

Original bug:

1. Go to workspace chat
2. Submit two distance expenses (if there is no other unsettled expense)
3. Go to workspace settings
4. Go to Distance rates > Settings > Unit
5. Select a different unit (km)
6. Go to transaction thread
7. Verify that the unit is unchanged in the Distance and Rate rows in transaction thread
8. Click on the rate row and verify the unit of the selected rate is unchanged
9. Verify that the transaction thread header shows the unit unchanged
10. Go to expense report
11. Verify the expense preview shows the unit unchanged
12. Update the distance, adding a waypoint
13. Verify the new unit is used on the transaction thread, header, expense preview, on the selected rate

1001Record241002081325.mov

Test without the beta:

1. Sign in with an account not on the p2pDistanceRequests beta, or manually disable it on dev by making this function return false
2. Have a report with 2+ distanced expenses
3. Change the policy distance unit to a different one (km) than the unit used for a distance expense (mi)
4. Verify the distance unit is unchanged on the expense preview, transaction thread header, and distance row
5. Update the distance
6. 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

1. Submit a distance expense on a policy that has multiple rates
2. Go to the policy settings and note the distance unit
3. Update the distance unit
4. Go to the existing expense and verify the unit is unchanged
5. Update the rate
6. Verify the unit updates everywhere it's shown on the expense
7. 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

1. Go to OldDot settings, workspaces, individual, open it
2. Go to reports, verify the report currency is USD
3. Track a distance expense
4. Verify unit is mi and rate is $ and mi
5. Go to OldDot and change the personal policy report currency to EUR
6. Go to NewDot, refresh the page
7. 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

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

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

Optimistic distance unit

1. P2P distance beta Enabled
2. Login with a new account
3. Create a workspace and enable distance rates feature
4. Change distance unit to km
5. Press FAB (+) > Submit request
6. Chose Distance tab, select start and stop waypoints then press Next
7. Select workspace then press Next
8. Go offline
9. Click on Submit
10. Open the expense report
11. 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

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

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]

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

[melvin-bot]

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

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]

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

[s77rt]

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

[neil-marcellini]

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

[neil-marcellini]

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]

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

[neil-marcellini]

Ok, the bug has been fixed and added to the tests. TS is also fixed.

[melvin-bot]

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

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

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

[MarioExpensify]

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]

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

[melvin-bot]

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

See comment above

[OSBotify]

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

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

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

[MarioExpensify]

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

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

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

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

[paultsimura]

Bug:

1. Place a distance request to a workspace
2. 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.