Fix invoice amount showing minus sign when edited offline#84048
Fix invoice amount showing minus sign when edited offline#84048Beamanator merged 3 commits intomainfrom
Conversation
…seReport during amount edit Invoice transactions store amounts as negative (like expense reports), but getUpdateMoneyRequestParams and calculateDiffAmount only checked isExpenseReport(), which returns false for invoices. This caused modifiedAmount to be stored as positive, while the display path (which uses isPaidGroupPolicy) negated it, showing a minus sign. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
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.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariInvoice 20260303204100499.mp4Expense Report with one transaction 20260303204231701.mp4Expense Report with two transaction 20260303204426682.mp4 |
|
@MelvinBot mark it as "Ready for review" |
|
@MelvinBot please mark this PR as ready for review |
|
PR has been marked as Ready for review as requested. |
|
@ahmedGaber93 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] |
|
This PR is already marked as Ready for review — no changes needed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ad07ecd32
ℹ️ 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".
| return 0; | ||
| } | ||
| const isExpenseReportLocal = isExpenseReport(iouReport); | ||
| const isExpenseReportLocal = isExpenseReport(iouReport) || isInvoiceReportReportUtils(iouReport); |
There was a problem hiding this comment.
Handle legacy positive modifiedAmount before diffing invoices
This now treats every invoice as an expense-sign transaction during calculateDiffAmount, which breaks the offline re-edit flow for invoices already edited on older builds (where modifiedAmount was saved as positive, per this bug’s root cause). In that case getAmount(transaction, true) returns a negative current amount while the report total is still based on the prior positive interpretation, so a second offline amount edit computes an inflated diff and temporarily drives the optimistic invoice total to the wrong value until server sync. Please normalize legacy invoice amounts (or guard this branch) before computing the diff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think we don’t need to handle any legacy cases here.
The old modifiedAmount values with the incorrect positive sign will be overridden with the correct value once the app comes back online and syncs with the server.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #83546 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
@ahmedGaber93 let's ask Melvin to write tests for this fix 🙏 |
|
@MelvinBot add unit tests for this fix. |
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Added unit tests for
These tests ensure that the fix (treating invoice reports like expense reports for amount sign convention) is covered by automated tests. |
|
@ahmedGaber93 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] |
|
@MelvinBot please add a test for |
Verify that modifiedAmount is negated when isFromExpenseReport is true and left positive when isFromExpenseReport is false. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Added two tests for
|
|
The failing To fix: Re-run the failed CLA job from the Actions tab. No code changes are needed. |
|
@Beamanator All yours! |
|
🚧 @Beamanator 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/Beamanator in version: 9.3.32-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.32-3 🚀
|
Explanation of Change
When editing an invoice amount while offline, the chat view incorrectly displays a "-" (minus) sign before the amount. This happens because of a sign convention mismatch between how the edited amount is stored and how it's read for display.
Invoice transactions store amounts as negative values (same as expense reports). However,
getUpdateMoneyRequestParamsandcalculateDiffAmountonly checkedisExpenseReport(iouReport)to decide whether to negate the amount — which returnsfalsefor invoices (since invoices have type'invoice', not'expense'). This causedmodifiedAmountto be stored as positive. When the display path reads the amount, it uses a broader check that includesisPaidGroupPolicy(policy)(true for Team/Corporate workspaces), which then negates the positivemodifiedAmount, producing a minus sign.The fix adds
|| isInvoiceReportReportUtils(iouReport)to bothisFromExpenseReportchecks, matching the existing pattern already used at line 4672 for the report total calculation.Fixed Issues
$ #83546
PROPOSAL: #83546 (comment)
Tests
Offline tests
QA Steps
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari