Fix: Manually edited amount during receipt scanning is not saved#84484
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63770b2740
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (hasValidModifiedAmount(transaction)) { | ||
| return false; |
There was a problem hiding this comment.
Restrict manual-amount override to UI-only scanning state
This new early return changes the global isScanning() predicate, not just the UI label, so any transaction with receipt.state still SCAN_READY/SCANNING but a non-empty modifiedAmount is now treated as “not scanning” everywhere. That affects non-visual flows that rely on isScanning (for example approval gating in canApproveIOU and merge eligibility), allowing users to submit/approve/merge while SmartScan is still running and may still mutate fields afterward. This should be scoped to display logic (or use a dedicated UI selector) instead of changing the shared scanning primitive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don’t think this is a blocker.
isScanning() is a user-facing helper, not a raw backend-state helper. Its docstring says background audit scanning should not be shown to the user.
We already have isReceiptBeingScanned(), and that is used in places where the backend scan state matters more directly.
So once modifiedAmount exists, the transaction should no longer be treated as scanning in the UI.
There was a problem hiding this comment.
affects non-visual flows that rely on isScanning (for example approval gating in canApproveIOU and merge eligibility)
@JmillsExpensify Agree with @marufsharifi here, but just wanted to double check that we're not missing anything. I'm guessing once a manual amount is set on a scanning expense, it should become ready to be approved, merged, etc.?
|
@jjcoffee, kindly bump. thanks. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Outline product behavior is in line with intended spec.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-app-2026-03-09_15.50.14.mp4Android: mWeb Chromeandroid-chrome-2026-03-09_15.53.35.mp4iOS: HybridAppios-app-2026-03-09_15.28.16.mp4iOS: mWeb Safariios-safari-2026-03-09_15.32.55.mp4MacOS: Chrome / Safaridesktop-chrome-2026-03-09_15.13.57.mp4 |
|
@marufsharifi Could you address the codecov report? |
@jjcoffee, could you please explain a little, which senorios or test cases should be covered? Thanks. |
|
@marufsharifi If you click the file linked in the report, you can see where your changes have reduced coverage. Ideally we'd have a test covering your changes. |
jjcoffee
left a comment
There was a problem hiding this comment.
LGTM! Assuming @JmillsExpensify agrees with the behaviour here.
|
I do though I'll also flag this PR to @Expensify/product-pr for visibility. Anyone disagree? |
|
Is this the Q:
I think there are, and will be more and more in the future, violations which rely on complete SmartScan data (duplicate detection, itemized receipt etc), so I think we always need to wait for a SmartScan to fully complete before an expense is ready to be approved, submitted, merged, split etc. |
|
Is this question in the context of the "background" SmartScanning for SmartScanEverything receipt audit? If so, that has historically been completely invisible to the submitter on the frontend when they "stop" the smartscanning process and enter the details manually themselves. I would think we maintain the same experience, so the expense isn't blocked on submission or anything for that background process to complete? |
I think we should be blocking on this though. It made sense when SmartScans could take 20 minutes, but it doesn't now. We have a few minutes to get a full set of SmartScan data with which to process violations for users. Admins would expect expenses that have violations. would show violations to users and submitters. |
|
The receipt audit violations are approver/admin only, no? 🤔 I don't think we should block on background SmartScanning in a flow where a user has explicitly stopped the SmartScanning process to enter the details themselves manually. It's very confusing. |
Yes, but the itemized receipt violation isn't. The new Duplicate Detection logic won't be. The prohibited expenses violations aren't. Any new SmartScan data based violation will be dependent on SmartScan completing. |
|
Yeah, if we're going to block submitting a report because we're smartscanning the manually attached receipt in the background for a plethora of reasons, we're going to need a new and clear UI pattern to communicate what is going on. |
|
Sounds like that's better suited to a separate/follow-up issue then? |
|
Yes, okay I think this is fine. We can probably test this now with an itemised receipt or prohibited expense violation. I will take one for the team and go buy some alcohol and smartscan the line item and POS receipt. |
|
@mjasikowski I think this is all yours then! |
|
🚧 @mjasikowski 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/mjasikowski in version: 9.3.37-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.37-10 🚀
|
Explanation of Change
Scanning...if backend receipt processing remained inSCANNING, even after the user changed the amount.modifiedAmount), we stop showing the scanning state immediately.Fixed Issues
$ #81316
PROPOSAL: #81316 (comment)
Tests
Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: Native
Screen.Recording.2026-03-07.at.11.04.42.AM.mov
Android: mWeb Chrome
Screen.Recording.2026-03-07.at.11.13.12.AM.mov
iOS: Native
Screen.Recording.2026-03-07.at.10.29.52.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-03-07.at.10.33.10.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-03-07.at.10.14.10.AM.mov