Repository navigation
Conversation
Co-authored-by: Jayesh Mangwani <[email protected]>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
🤖 Android test steps verification results: ✅ pass Odometer expense quantity is not converted when the destination workspace's distance unit changes: after switching from the kilometer workspace to the mile workspace, the confirm page and the Distance page both still read 100 (not ~62.14), only the unit label changed.
Run on a standalone Android NewDot view run · no recording available |
|
🤖 Web test steps verification results: ✅ pass On PR #101050 (
Run on dev NewDot web against branch view run · no recording available |
|
Can anyone from @Expensify/product please confirm the expected behavior here? For distance rates, we have Kilometers set as the unit for the default workspace and Miles for other workspace(s). When changing the workspace on the Confirm Details page, the quantity doesn't update from km to mi. Is this the expected behavior, or should the quantity update based on the selected workspace's distance unit? expected-result-pr-101050.mov |
|
Anyone from the @Expensify/product team, please have a look at this comment #101050 (comment) and confirm the expected behavior for this issue #100973. |
|
From your video, it was a little tough to understand what you're asking. But it sounds like the question is whether we should be doing an automatic conversion of km <> mi when the workspace is changed and uses a different unit, right? From the OP of the parent issue, it sounds like it's expected for Odometer with this rationale:
... but it doesn't make sense to me to be excluded. They could be reading KM on the car, but need to submit on another workspace in MI and would appreciate the conversion. Similarly, manual distance is a "plain number" entered as well, why treat it differently to manual Odometer entry? CC: @Julesssss @neil-marcellini @JmillsExpensify do you know why? I think we should do the conversion for all distance types (Map, GPS, Odometer, Manual). As a side-note, it isn't super clear in the UI when it automatically happens, so I'd be open to some kind of confirmation modal on changing the workspace when it does. I.e
|
Yes, correct—that’s what I’m confirming here. Sorry if the video above was a little ambiguous. There’s another issue I’d like to highlight here, which is also happening in prod: When we manually add the start and end values under Track Distance → Odometer, with Km set as the unit for the default workspace, and then go to Confirm Details and switch to a workspace that uses Mi, the quantity is correctly converted from Km to Mi. However, when we tap on Distance, it shows the original start and end quantities instead of the converted values.So, if we convert the values, wouldn’t that cause different values to be displayed on the Details page versus the Edit Start and End Quantity pages? Adding a video below showing the flow. bug-on-details.mov |
|
Here was the original decision to align with classic. I think the idea was that converting two raw odometer readings made less sense than converting a single distance reading. The main problem is that readings don't have their original unit saved to NVP. They just have the workspaces unit applied so when the workspace changes from km to mi the readings 1000 and 1100 are changed to miles, with no knowledge that they were originally km. Converting the readings means they won't match odometer photos. Classic avoids this by keeping raw odometer numbers as entered and only changing the unit label. So the fix would be to store the original unit the readings were made in in the NVP data. Then the backend can convert. Readings stay matching the photos and distance can show the converted distance. This is a separate change though i think. |
|
Confirmed: odometer readings are stored unitless. The inconsistency on Answering Honest cost of this PR: it fixes the reported mismatch (both screens read 100), but 100 km is then priced as 100 mi at the destination rate. The amount is semantically wrong, just consistently wrong — where before, the two screens disagreed. Neither option produces a correct amount, because the data needed to compute one isn't stored. Why neither option is correct without the missing fieldkm → mi workspace switch, readings 1000 → 1100:
Also worth noting: on the edit path for an already-created expense, the client does no quantity conversion at all — it only marks the merchant modified so the field goes pending and lets the backend recalculate ( Not verified: I traced the App side only. I could not confirm how the backend recomputes Suggested path: merge this as the narrow fix for Wrong distance at confirm page when create from FAB for ws set different unit with default ws — it removes the screen-to-screen contradiction and makes the four conversion sites agree — and track "store the readings' unit and convert server-side" as the separate follow-up Next Steps: Reply with |
|
@trjExpensify, just tagging you here regarding @Julesssss’s comment: #101050 (comment). I’m not sure what the expected result is for these changes. If we need to convert the reading from km to miles, we may need broader changes, as we currently don’t convert these values (Start and End reading) anywhere in ND. We change the units and calculate the amount based on the WS unit. If we want to address the inconsistency mentioned in the OP #100973 (comment) between the Distance row and the expense details, then we can exclude the reading quantity from the conversion for the Distance row and move forward with the current changes: https://github.com/Expensify/App/pull/101050/changes . Could you please help confirm the expected behavior? Or do we need to reach a consensus on Slack first regarding the expected behavior? What do you think? |
|
Thanks, guys. @Julesssss so we're converting the total distance from Before: After (confirmation page) After (readings page) 
So should we minimally change that so the "Total distance" always matches? |
That seems reasonable, yeah. I would like to make it clearer to users the conversion is happening due to the workspace config, or to additionally show the original unit type. But not enough to push for it today. |
Sounds good. 👍
Fair. |
@Julesssss, what changes do we need to make here? I’m not sure what the expected behavior should be 😅 |
Sorry, to be clearer, 'make it clearer to users' was for the future. For now, we should keep converting quantity when workspace unit changes (like 
|
|
@MelvinBot Lets do this change #101050 (comment) here |
Revert the odometer exemption in setCustomUnitRateID so the quantity is converted again when the workspace distance unit changes, and make the odometer page's Total distance read that converted quantity instead of end - start. The readings themselves stay as entered so they match the odometer photos. Co-authored-by: Jayesh Mangwani <[email protected]>
|
I made the change from
The page also uses the helper when you save ( Typecheck, lint, spell check, React Compiler check, and 33 related test suites pass. I didn't test this in a browser. I also updated the PR description and the suggested test steps. How the helper decides
The same value is used for the amount-limit check ( |
Co-authored-by: Jayesh Mangwani <[email protected]>
|
Fixed the failing
|
|
@MelvinBot, we can see the kilometer-to-mile conversion on the total page, but once we create the expense, it shows the original kilometer value as the total. For example, I entered 2 and 6 as the start and end values, giving a total of 4 km. After switching to a workspace that uses miles, the value was correctly converted to 2.49 mi, but the report page still shows 4 mi. |
|
The App sends the converted distance correctly. The saved expense shows 4.00 mi because the server appears to recalculate the distance from the odometer readings and ignore the converted value. A change in this PR can't fix that without also converting the readings, which This appears to be a backend/internal issue. A separate internal investigation is needed. In your example, the create request sends:
The saved result is 4.00 mi and ₹3.04 (4 × ₹0.76). That matches Decision needed (
I haven't pushed any changes. Tell me which option to use. I can do 2 or 3 in this PR. Option 1 needs an internal engineer. App-side evidence
I couldn't check the server code. The conclusion above comes from the request values and the saved result. |
|
@Julesssss Jules, it seems we need to fix this on the BE side too #101050 (comment). We send the converted distance (from km to miles) to the For example, we send these values to BE:
But BE returns the following {
"comment": {
"customUnit": {
"distanceUnit": "mi",
"name": "Distance",
"quantity": 3
}
}
}The |
Hmm yeah. Okay I'll find someone to work on that. Lets hold the PR for the moment. |
|
@MelvinBot, please merge |
…ceUnitConversion Co-authored-by: Jayesh Mangwani <[email protected]>
…MoneyRequestDistance Store the unit the odometer readings were entered in on the transaction comment, so the backend can convert end - start into the rate's unit when they differ. Co-authored-by: Jayesh Mangwani <[email protected]>
|
I merged
How the unit is tracked:
For edits to keep working after a reload, the backend should return Changes and checksChanges
Checks
I didn't test this in a browser. The converted distance only saves correctly once the backend change is live. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@MelvinBot please fix the ESLint |
…ceUnitConversion Co-authored-by: Jayesh Mangwani <[email protected]>
|
Fixed the ESLint check by merging The failure was an unused |
|
@Julesssss, passing Let me know if we should handle this separately from this PR. Since it also occurs in staging, I can report it in the bugs channel. I’m attaching Steps and a staging video. If you think we should fix it in this PR, we can try to resolve it with Melvin. Steps:
Actual behavior:
Expected behavior:
bug-on-staging.mov |
|
@Julesssss , whenever you get a chance, please check this comment #101050 (comment). |
Explanation of Change
When an odometer expense moves to a workspace with a different distance unit,
setCustomUnitRateIDconvertscomment.customUnit.quantityinto the new unit (unchanged frommain), whilecomment.odometerStart/comment.odometerEndstay exactly as entered so they keep matching the odometer photos.The reported bug was that the odometer page's "Total distance" ignored that conversion and showed
end - startlive from the readings. With readings of 1000 → 1100 saved on a km workspace and then switched to a mi workspace, the confirmation page showed 62.14 mi while the Distance page showed 100 mi. Pressing Save on the Distance page without changing anything also reset the quantity back to 100.The fix adds
getOdometerTotalDistanceinOdometerUtils, which the odometer page now uses for the displayed total, the saved distance, and the amount-limit check:end - start, same as before.AI tests run
All run locally against this branch:
npm run typecheck— passednpm run linton the changed files — passednpm run spell-changed— passed (4 files, 0 issues)npm run react-compiler-compliance-check checkonIOURequestStepDistanceOdometer.tsx— passednpm test -- tests/unit/OdometerUtils.test.ts tests/actions/IOU tests/unit/DistanceRequestUtilsTest.ts tests/actions/OdometerTransactionUtilsTest.ts— 33 suites, 1038 tests passed, including the newgetOdometerTotalDistancecasesNot run: browser verification of the end-to-end flow.
Fixed Issues
$ #100973
PROPOSAL: #100973 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
// Suggested starting point, matching the issue's repro: create two workspaces with Distance rates enabled and different units (one km, one mi); FAB > Track distance > Odometer; enter 1000 and 1100 > Next; on the confirm page switch the recipient to the other workspace; verify the Distance row shows the converted distance (about 62.14 mi for km → mi); tap the Distance row and verify "Total distance" shows the same value while the readings still show 1000 and 1100; tap Save without changes and verify the confirm page still shows the converted distance.
Offline tests
// TODO: The human co-author must fill this out before marking this PR as "ready for review".
QA Steps
Please tests from this PR description AND all of the test cases in browserstack
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari