Skip to content

[Odometer] Add backup transaction for purpose of editing from confirmation#86022

Merged
Julesssss merged 28 commits intoExpensify:mainfrom
software-mansion-labs:jakubkalinski0/Odometer_add_backup_transaction_for_editing_from_confirmation
Apr 1, 2026
Merged

[Odometer] Add backup transaction for purpose of editing from confirmation#86022
Julesssss merged 28 commits intoExpensify:mainfrom
software-mansion-labs:jakubkalinski0/Odometer_add_backup_transaction_for_editing_from_confirmation

Conversation

@jakubkalinski0
Copy link
Copy Markdown
Contributor

@jakubkalinski0 jakubkalinski0 commented Mar 21, 2026

Explanation of Change

PR #85105 has to be merged before we can go ahead with this one

Adds backup transaction support for editing odometer readings from the confirmation page. On entry, a fresh backup is created (bypassing AsyncStorage race conditions). Canceling restores the backup and revokes new blob URLs. Saving cleans up the backup and revokes the old URLs. Also skips redundant image re-stitching when source images are unchanged. If user didn't press "Save" then we interpret it as an intention to discard the changes and stick with the original images/readings.

Fixed Issues

$ #85368 - main issue
$ #86355 - related small bug
PROPOSAL: N/A

Tests

We are testing the odometer expense creation flow which you start by doing the following:
Press FAB -> go to "Track distance" -> choose "Odometer" tab

  1. Create a new odometer expense, add both readings and upload start and end odometer images.
  2. Press "Next" to reach the confirmation page and verify the stitched receipt is shown.
  3. Press "Distance" to land on the odometer screen.
  4. Replace one of the odometer images and one of the readings.
  5. Tap "Save" and navigate back to the confirmation page.
  6. Verify the stitched receipt updates to reflect the new image and distance reflects the changed reading.

  1. Follow steps 1-4 above.
  2. Tap <back button instead of "Save".
  3. Verify the confirmation page shows the original odometer images (not the newly selected ones) and distance.
  4. Once again press "Distace" to land on the odometer screen and verify that the odometer images and readings are the original ones, not the changed ones that we dicarded by tapping < back button

This test steps below verify that this issue is fixed -> Expense - Odometer photo is closed immediately after clicking rotate
11. Create a new odometer expense, add both readings and upload ONLY ONE ODOMETER IMAGE (start or end).
12. Press "Next" to reach the confirmation page and verify the stitched receipt is shown.
13. Press "Distance" to land on the odometer screen.
14. Click the uploaded photo.
15. Click "Rotate".
16. Verify that the photo is rotated and you are NOT brought back to the Odometer Tab (thats what happened in a bug described here #86355)

  • Verify that no errors appear in the JS console

Offline tests

Same as Tests

QA Steps

Same as 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
  • 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 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 used JaimeGPT to get English > Spanish translation. I then posted it 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.ts 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 new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • 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.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • 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

iOS needs to be tested on a physcical device

Android: Native
android.mp4
Android: mWeb Chrome
androidweb.mp4
iOS: mWeb Safari
ios.mp4
MacOS: Chrome / Safari
web.mp4

…o_confirmation_page' into jakubkalinski0/Odometer_add_backup_transaction_for_editing_from_confirmation
…o_confirmation_page' into jakubkalinski0/Odometer_add_backup_transaction_for_editing_from_confirmation
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

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.

Files with missing lines Coverage Δ
.../step/IOURequestStepOdometerImage/index.native.tsx 0.00% <0.00%> (ø)
...request/step/IOURequestStepOdometerImage/index.tsx 0.00% <0.00%> (ø)
src/libs/stitchOdometerImages/index.native.ts 0.00% <0.00%> (ø)
src/libs/stitchOdometerImages/index.ts 0.00% <0.00%> (ø)
...alScreen/routes/TransactionReceiptModalContent.tsx 0.00% <0.00%> (ø)
src/libs/actions/IOU/index.ts 74.17% <0.00%> (+0.20%) ⬆️
...es/iou/request/step/IOURequestStepConfirmation.tsx 55.03% <20.00%> (-0.66%) ⬇️
src/libs/OdometerImageUtils.ts 0.00% <0.00%> (ø)
...ou/request/step/IOURequestStepDistanceOdometer.tsx 0.00% <0.00%> (ø)
src/libs/actions/TransactionEdit.ts 55.20% <6.06%> (-26.05%) ⬇️
... and 9 files with indirect coverage changes

@jakubkalinski0 jakubkalinski0 force-pushed the jakubkalinski0/Odometer_add_backup_transaction_for_editing_from_confirmation branch 2 times, most recently from ada69e9 to f7c0578 Compare March 24, 2026 13:37
@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

TS check fail unrelated (at least I see no corelation)

@jakubkalinski0 jakubkalinski0 marked this pull request as ready for review March 25, 2026 18:16
@jakubkalinski0 jakubkalinski0 requested review from a team as code owners March 25, 2026 18:16
@melvin-bot melvin-bot bot requested review from DylanDylann and heyjennahay and removed request for a team March 25, 2026 18:16
@melvin-bot melvin-bot bot requested a review from Julesssss March 27, 2026 17:19
@DylanDylann
Copy link
Copy Markdown
Contributor

DylanDylann commented Mar 27, 2026

@jakubkalinski0 Hey, I noticed an issue on Android. I can’t save the cropped image. It only happens the first time I open the app.

Screen.Recording.2026-03-28.at.00.40.45.mov

@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

@DylanDylann I am now testing it and cannot save the cropped image at all on android. Something fishy is going on, I will look into that

@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

Some big-ish changes were made to image processing about a week ago which could have something to do with this

I will try to verify today before I go to sleep whether that could be the cause of that issue since it's also present on main.

@Julesssss
Copy link
Copy Markdown
Contributor

In my opinion user should be able to change his mind and not save the changes he made, which is basically the point of this PR. My question is, should we show DiscardChangesConfirmation modal when user taps < back button to make it seem more like an intentional decision and not a bug?

Yeah good idea. Missed this comment yesterday!

In this PR, as discussed above, we have 2 improvement points... ...that we should handle this separately (at least for point 1, since it’s worth cleaning up other usages of transactionBackup as well). So I will approve this PR in advance, then you can merge it if you agree

Yeah thanks for confirming. I'm happy with how the changes look currently. To be honest I am now wondering how important these improvements are... I might suggest we don't apply the partial transactionBackup changes to other expense edits if we can get avoid that refactor. Anyway that is a discussion for later.

@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

Yeah good idea. Missed this comment yesterday!

No problem man! Would you like me to introduce that in this PR or should we do that in a followup since Dylan already approved this PR in a current state?

Btw I just came home from the office and will continue my investigation with the latest issue that Dylan found here (which I believe really is introduced by the PR I linked in this comment #86022 (comment) but we should wait for the definite proof)

@Julesssss
Copy link
Copy Markdown
Contributor

@jakubkalinski0 follow up is fine, assuming you don't need to make changes for the cropping issue 👍

@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

@jakubkalinski0 follow up is fine, assuming you don't need to make changes for the cropping issue 👍

Yeah, I will fix that today as I managed to identify the root cause of the issue. I will describe it with more details when I am done testing the fix

@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

Works now, sorry for the delay - I had to take care of something private. I will explain the issue to my best understanding in a moment

works.webm

@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

The saveCrop function in TransactionReceiptModalContent.tsx passes cropRect values directly to the native image manipulator without rounding. ReceiptCropView/index.tsx computes these via width = displayWidth × (imageWidth / displayWidth). Due to IEEE 754 floating-point arithmetic, this a × (b/a) operation produces 793.0000000000001 instead of 793.0 for the odometer image. It's an issue since the native CropTransformer.kt validates if rect.width <= bitmap.width. For the odometer we have the following 793.0000000000001 <= 793 -> false -> ImageInvalidCropException.

@DylanDylann
Copy link
Copy Markdown
Contributor

Working well

Screen.Recording.2026-03-31.at.11.10.38.mov

@DylanDylann
Copy link
Copy Markdown
Contributor

We got a failed test; I think it’s flaky

@jakubkalinski0
Copy link
Copy Markdown
Contributor Author

We got a failed test; I think it’s flaky

Yeah, I agree. Seems like a faulty test

@DylanDylann
Copy link
Copy Markdown
Contributor

@jakubkalinski0 Could you merge main again to see if we get lucky?

@DylanDylann
Copy link
Copy Markdown
Contributor

@Julesssss All your

@Julesssss
Copy link
Copy Markdown
Contributor

Okay great, lets get this merged and move onto release 2 issues!

@Julesssss Julesssss merged commit 3342e31 into Expensify:main Apr 1, 2026
31 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify
Copy link
Copy Markdown
Contributor

OSBotify commented Apr 2, 2026

🚀 Deployed to staging by https://github.com/Julesssss in version: 9.3.52-0 🚀

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

Bundle Size Analysis (Sentry):

@MelvinBot
Copy link
Copy Markdown
Contributor

🤖 No help site changes are required for this PR.

This PR adds an internal backup/restore mechanism for editing odometer readings from the confirmation page. The changes are:

  • Creating a backup transaction on edit entry
  • Restoring backup on cancel (back button)
  • Cleaning up backup on save
  • Skipping redundant image re-stitching

These are all internal behavior fixes — no new user-facing features, buttons, labels, or workflows are introduced. The user flow for creating/editing odometer expenses remains the same; it just works correctly now when canceling edits.

Note: Pre-existing documentation gap

The Odometer distance method is not documented in the New Expensify help site at all. The Distance-Expenses.md article covers GPS, Map, and Manual methods but omits Odometer entirely (it's only mentioned in the Expensify Classic docs). This is a pre-existing gap unrelated to this PR — if you'd like me to create a separate PR to add Odometer documentation to the New Expensify help articles, let me know.

@OSBotify
Copy link
Copy Markdown
Contributor

OSBotify commented Apr 7, 2026

🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.52-9 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants