Fix updates of distance and distance units when changing expense recipients#81017
Conversation
…hen only customUnit: {quantity} is available
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@hoangzinh 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] |
|
|
||
| if (shouldUpdateDistanceUnit) { | ||
| newDistanceUnit = distanceRate.unit; | ||
| } |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The logic for updating newQuantity calls getDistanceInMeters(transaction, transactionDistanceUnit) without verifying that transactionDistanceUnit is defined and valid for the current transactionQuantity. If the transaction has a quantity but no distanceUnit (an inconsistent state), this could lead to incorrect distance conversion.
Suggested fix: Add validation to ensure both quantity and distanceUnit are present before attempting conversion:
if (shouldUpdateQuantity && !!distanceRate?.unit && transactionDistanceUnit) {
newQuantity = DistanceRequestUtils.convertDistanceUnit(
getDistanceInMeters(transaction, transactionDistanceUnit),
distanceRate.unit
);
}Alternatively, add a comment explaining that getDistanceInMeters safely handles undefined units by returning 0, if that's the intended behavior.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
manual distance request updated to set transaction distanceUnit, gps distance request has distance in meters always stored in transaction?.routes?.route0?.distance
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e391a99ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@dukenv0307 part of GPS project, could you review? |
|
I have been assigned to the original issue, so I can help to review if @dukenv0307 is busy |
|
Thank you @hoangzinh, it's our regression so I'll review this PR soon |
|
Bug: The distance changes after changing to another workspace Screen.Recording.2026-02-03.at.15.18.59.mov |
|
Bug:
Screen.Recording.2026-02-03.at.15.19.25.mov |
|
On it now |
@dukenv0307 Yes, when creating Manual expense from FAB > Track distance we show distance and unit based on the default expense policy, which in this case is a policy with km distance unit. This is because when user taps Next in this scenario the default policy is set as the initial recipient, so we have to use it to determine what to show on the screen with input. I added a test for it in the PR's description: See that when you create a distance request from a workspace chat where the distance unit is different than the one used for the default policy, then it uses distance unit of the workspace and tapping Next uses that workspace as the recipient |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppI faced some issues while building Android. The PR author already verified it on Android so I think that's fine Android: mWeb ChromeScreen.Recording.2026-02-04.at.22.51.37.moviOS: HybridAppScreen.Recording.2026-02-04.at.23.03.28.movScreen.Recording.2026-02-04.at.23.06.20.moviOS: mWeb SafariScreen.Recording.2026-02-04.at.22.51.03.movMacOS: Chrome / SafariScreen.Recording.2026-02-04.at.22.36.21.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #80661 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
Part of GPS project, @AndrewGable please review |
|
🚧 @AndrewGable has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.3.16-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.16-9 🚀
|
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.3.17-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
…x-distance-conversion-different-distance-unit Restore "#81017 Fix updates of distance and distance units when changing expense recipients" with fixes
Explanation of Change
setCustomUnitRateID()function updatestransaction.comment.customUnit.customUnitRateID, but does not consider that the transaction may have alsotransaction.comment.customUnit.distanceUnitandtransaction.comment.customUnit.quantityset which depend on the currentcustomUnitRateID. Because of this for the GPS and Manual requests the conversion of distance and update of the distance unit does not work properly when changing the recipient of the expense in the draft state. Fixing this by updating thesetCustomUnitRateID()to make sure that if eithertransaction.comment.customUnit.distanceUnitortransaction.comment.customUnit.quantityvalue is set, they get updated according the the newcustomUnitRateID.Fixed Issues
$ #80661
PROPOSAL: N/A
Tests
Preconditions:
GPS:
Manual:
Manual input:
Offline tests
QA Steps
Same as tests and make sure that there are no regressions when creating other expenses and changing the recipients in draft state
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-01-30.at.15.35.18.mov
Screen.Recording.2026-01-30.at.16.32.00.mov
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari