[Odometer] Photo is not cropped after saving cropped photo#85438
Conversation
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.
|
|
@heyjennahay Hi! Sorry for the trouble but @DylanDylann is the C+ assigned to the odometer project and I think he will be the one taking care of this review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-18.at.11.37.24.movAndroid: mWeb ChromeScreen.Recording.2026-03-18.at.11.21.35.moviOS: HybridAppScreen.Recording.2026-03-18.at.11.31.37.moviOS: mWeb SafariScreen.Recording.2026-03-18.at.11.20.21.movMacOS: Chrome / SafariScreen.Recording.2026-03-18.at.11.17.10.mov |
|
@jakubkalinski0 Could you review all usages of setMoneyRequestReceipt to check if there are any similar cases we might be missing? |
|
I noticed a case where the stitched image on the confirmation page can be replaced by uploading another one. We should not allow the stitched image to be replaced on the confirmation page. Screen.Recording.2026-03-17.at.17.45.59.mov |
@DylanDylann I already did that and all other usages seem to be fine |
Wow, thats interesting, I am on it |
|
@DylanDylann Simple fix, you can continue with your review. Thanks for catching that! Screen.Recording.2026-03-17.at.12.32.32.mov |
|
@jakubkalinski0 Thanks. It's looking fine now |
|
@jakubkalinski0 The crop and rotate buttons only appear if we are going from the confirmation page. Is this expected? |
|
@DylanDylann No, I don't think it should look this way. On it |
|
I think I've fixed it but I am testing it on all platforms now and looking for possible regressions |
|
@Julesssss One question, while testing I noticed that we also had this It's a small change and I have it ready but we can do it as a lower priority followup if you want (if we want to do that at all of course) |
|
@DylanDylann Besides the comment above this PR is all yours once again |
No it's fine to not allow that. |
Unlucky then that I took some time to figure out a fix 😅 Thanks for the info, then this PR is all for @DylanDylann |
|
Improvement: (Should fix separately in another PR) When clicking “Replace,” the app navigates to the odometer distance page and then to the odometer image page. I think we should open the picker directly instead of navigating the user back to the odometer image page. Screen.Recording.2026-03-18.at.11.13.54.mov |
src/pages/media/AttachmentModalScreen/routes/TransactionReceiptModalContent.tsx
Outdated
Show resolved
Hide resolved
DylanDylann
left a comment
There was a problem hiding this comment.
The rest looks fine to me
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8d61224f7
ℹ️ 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".
src/pages/media/AttachmentModalScreen/routes/TransactionReceiptModalContent.tsx
Show resolved
Hide resolved
|
@jakubkalinski0 Please take a look at above comments |
@DylanDylann Why? I don't think I actually agree. Why do we want to limit the user to attachment picker only? I think that design would have to make that call anyway, so we can ask them if you wish. |
Let’s move forward for now, this is just my suggestion for improvement. We can discuss it later. |
Thanks, bug created here. @jakubkalinski0 IMO it should be improved, to not show the distance tabs page. Navigating back seems odd |
|
🚧 @Julesssss 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/Julesssss in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
Explanation of Change
Fixed odometer photo crop/rotate not persisting after save. The
TransactionReceiptModalContentwas usingsetMoneyRequestReceipt/replaceReceiptfor all images, but odometer images requiresetMoneyRequestOdometerImage. Added anisOdometerImagebranch in both the rotate and crop save handlers, and updated their dependency arrays accordingly.Fixed Issues
$ #85365
PROPOSAL: N/A
Tests
FAB-> go to"Track distance"-> choose"Odometer"tab"Next"<back button until you are brought back to to the"Odometer"tab"Next""Distance"and when you land on odometer screen repeat steps 6 through 9"Save"and verify that the stitched image uses edited odometer photosOffline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
iOS needs to be tested on a physcical device
Android: Native
androidnative.mp4
Android: mWeb Chrome
androidweb.mp4
iOS: mWeb Safari
iosweb.mp4
MacOS: Chrome / Safari
web.mov