Fix negative transaction total after moving IOU report to a workspace#91079
Fix negative transaction total after moving IOU report to a workspace#91079trasnake87 wants to merge 11 commits into
Conversation
melvin-bot
Bot
requested review from
FitseTLT and
joekaufmanexpensify
and removed request for
a team and
FitseTLT
melvin-bot
Bot
requested review from
FitseTLT and
inimaga
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ...transaction, | ||
| amount: -transaction.amount, | ||
| modifiedAmount: hasValidModifiedAmount(transaction) ? -Number(transaction.modifiedAmount) : '', | ||
| ...(transaction.convertedAmount != null && {convertedAmount: -transaction.convertedAmount}), |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The transaction sign-conversion logic (negating amount, modifiedAmount, convertedAmount, convertedTaxAmount) is now identically duplicated between convertIOUReportToExpenseReport in src/libs/actions/Report/index.ts and createWorkspaceFromIOUPayment in src/libs/actions/Policy/Policy.ts. Adding new fields to negate (like this PR does) requires updating both locations in lockstep, which is error-prone.
Extract the negation logic into a shared helper function, for example:
function negateTransactionAmounts(transaction: Transaction): Transaction {
return {
...transaction,
amount: -transaction.amount,
modifiedAmount: hasValidModifiedAmount(transaction) ? -Number(transaction.modifiedAmount) : '',
...(transaction.convertedAmount != null && {convertedAmount: -transaction.convertedAmount}),
...(transaction.convertedTaxAmount != null && {convertedTaxAmount: -transaction.convertedTaxAmount}),
};
}Then use it in both call sites:
transactionsOptimisticData[...] = negateTransactionAmounts(transaction);Reviewed at: 36d06bc | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| ...transaction, | ||
| amount: -transaction.amount, | ||
| modifiedAmount: hasValidModifiedAmount(transaction) ? -Number(transaction.modifiedAmount) : '', | ||
| ...(transaction.convertedAmount != null && {convertedAmount: -transaction.convertedAmount}), |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This is the same duplicated transaction sign-conversion block that also exists in src/libs/actions/Report/index.ts (convertIOUReportToExpenseReport). See the comment on that file for the suggested shared helper extraction.
Reviewed at: 36d06bc | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d06bcf4b
ℹ️ 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".
| ...(transaction.convertedAmount != null && {convertedAmount: -transaction.convertedAmount}), | ||
| ...(transaction.convertedTaxAmount != null && {convertedTaxAmount: -transaction.convertedTaxAmount}), |
There was a problem hiding this comment.
When an IOU transaction already has a negative convertedAmount (the transaction helpers explicitly handle IOU amounts that are stored negative), this unconditional negation stores a positive converted amount on the new expense report. Expense report rendering flips converted amounts for display, so that input will make the converted total/tax show negative again; use the expense-report sign convention directly, e.g. negate the absolute value, and apply the same fix to the duplicate conversion in createWorkspaceFromIOUPayment.
Useful? React with 👍 / 👎.
|
Addressed the review feedback in 7876b36:
|
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Makes sense 👍
trasnake87
force-pushed
the
fix-iou-negative-amount-90219
branch
from
7876b36 to
2c637cd
Compare
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.
|
|
@FitseTLT good catch — the I added a matching test in Verified the test catches the pre-fix behavior: temporarily replacing |
When an IOU report is converted to an expense report, the optimistic transaction data negated amount and modifiedAmount to match the expense-report sign convention but left convertedAmount and convertedTaxAmount untouched. Because getConvertedAmount flips the sign for expense reports, the per-row TOTAL column rendered the still-positive convertedAmount as a negative value. Negate convertedAmount and convertedTaxAmount in the optimistic data in both convertIOUReportToExpenseReport and createWorkspaceFromIOUPayment, using a conditional spread so absent values are not overwritten. Rollback data is built from the unmodified transaction, so failure restores the original values.
…unts Move the IOU-to-expense-report transaction sign conversion into a single getExpenseReportSignedTransaction helper in TransactionUtils so the logic is no longer duplicated between convertIOUReportToExpenseReport and createWorkspaceFromIOUPayment. IOU transactions can be stored with either sign, so negate the absolute value of convertedAmount/convertedTaxAmount. The expense-report convention flips the stored sign for display, so a negative magnitude keeps the table total positive even when the source IOU stored the converted amounts negative. Added a regression test covering that case.
…conversion Cover the Policy.ts call site of getExpenseReportSignedTransaction with a test that feeds an IOU transaction whose converted amounts are stored negative and asserts the optimistic data stores a negative magnitude (so the expense-report sign convention renders a positive table total) and that the failure data restores the original values.
trasnake87
force-pushed
the
fix-iou-negative-amount-90219
branch
from
4bd5eca to
c2e1d23
Compare
|
Will review on Monday |
|
@FitseTLT Added the Android Native recording — same IOU → More → Change workspace → JAG-01 flow on the Android native build, the per-row Amount stays at $75.00 (positive) and Total renders at ₹7,177.54 (positive). iOS Native follows next. |
|
@FitseTLT iOS Native is now attached as well. All three platform recordings show the same flow — IOU created in a DM, then More → Change workspace → JAG-01, and the per-row Amount + Total stay positive after the conversion. Web, Android Native, iOS Native rows are ticked. |
| const apiWriteSpy = jest.spyOn(require('@libs/API'), 'write').mockImplementation(() => Promise.resolve()); | ||
| const isIOUReportUsingReportSpy = jest.spyOn(ReportUtils, 'isIOUReportUsingReport').mockReturnValue(true); | ||
| const getReportTransactionsSpy = jest.spyOn(ReportUtils, 'getReportTransactions').mockReturnValue([transaction]); | ||
|
|
||
| const mockTranslate = ((key: string) => key) as unknown as Parameters<typeof Policy.createWorkspaceFromIOUPayment>[8]; | ||
| Policy.createWorkspaceFromIOUPayment(iouReport, undefined, ESH_ACCOUNT_ID, ESH_EMAIL, iouReportOwnerEmail, undefined, CONST.CURRENCY.USD, undefined, mockTranslate, {}); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| const writeOptions = apiWriteSpy.mock.calls.at(0)?.at(2) as { | ||
| optimisticData?: Array<{key?: string; value?: Record<string, Transaction> | null}>; | ||
| failureData?: Array<{key?: string; value?: Record<string, Transaction> | null}>; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The correct way to test is save the positive amount transaction in onyx then after the action createWorkspaceFromIOUPayment read the transaction from onyx and assert the correct value. Read other tests for example.
There was a problem hiding this comment.
Done in c20917c — the test now seeds the IOU report and transaction (with positive convertedAmount) in Onyx, calls createWorkspaceFromIOUPayment with fetch paused, and reads the transaction back via Onyx.connect to assert the optimistic merge. The failure-rollback branch then fails+resumes fetch and re-reads from Onyx.
| const iouReport: OnyxTypes.Report = { | ||
| ...createRandomReport(3, undefined), | ||
| reportID: 'iouReport302', | ||
| type: CONST.REPORT.TYPE.IOU, | ||
| ownerAccountID: 3, | ||
| reportName: 'Original IOU Report Name', | ||
| total: 5000, | ||
| }; |
There was a problem hiding this comment.
Same — in c20917c, the test now seeds the transaction in Onyx, calls convertIOUReportToExpenseReport, applies result.optimisticData via Onyx.update, and asserts the stored transaction via Onyx.connect. The follow-up rollback case applies result.failureData and re-reads. The obsolete "negative IOU input" sibling test was removed alongside the Math.abs handling.
| ...(transaction.convertedAmount != null && {convertedAmount: -Math.abs(transaction.convertedAmount)}), | ||
| ...(transaction.convertedTaxAmount != null && {convertedTaxAmount: -Math.abs(transaction.convertedTaxAmount)}), |
There was a problem hiding this comment.
IOU dooesn't have tax amount remove the tax amount part and remove abs too like the amount case.
There was a problem hiding this comment.
Done in c20917c — dropped the convertedTaxAmount branch and switched to a plain -transaction.convertedAmount so this mirrors the amount case. Doc-comment trimmed to match.
- getExpenseReportSignedTransaction: drop the convertedTaxAmount branch (IOUs don't carry tax) and use a plain negation on convertedAmount (mirrors the amount line; IOU convertedAmount is positive). - PolicyTest createWorkspaceFromIOUPayment: rewrite to seed the IOU report and transaction in Onyx with a positive convertedAmount, run the action, and assert the stored transaction via Onyx (pause fetch for the optimistic check, then fail+resume for the rollback). - ReportTest convertIOUReportToExpenseReport: same Onyx-read pattern via Onyx.update on the returned optimistic and failure data; drop the obsolete "negative IOU input" test alongside the abs handling.
|
@trasnake87 Why don't u complete your checklist and include a proper video that shows the steps in the issues OP which will show this pr fixed it? This is the last time I am asking you. We will have to reassign another contributor. |
|
@FitseTLT Updated Screenshots/Videos → Web with an end-to-end walkthrough of the OP repro: open a DM with another user, submit two expenses ( Checklist:
The Android Native and iOS Native videos already in the same section cover the same fix on those builds. |
|
All 5 platform videos are required with the same steps as the current web video. If u can't provide tell me we will reassign another contributor. |
|
@FitseTLT All five platforms below, each running through the OP repro (open the Submitter Emp DM → submit two expenses
|
|
@FitseTLT Re-shot the iOS Native recording with the |
| await Onyx.update(result.failureData); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| const rolledBackTransaction: OnyxEntry<OnyxTypes.Transaction> = await new Promise((resolve) => { | ||
| const connection = Onyx.connect({ | ||
| key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, | ||
| callback: (value) => { | ||
| Onyx.disconnect(connection); | ||
| resolve(value); | ||
| }, | ||
| }); | ||
| }); | ||
| expect(rolledBackTransaction?.amount).toBe(5000); | ||
| expect(rolledBackTransaction?.convertedAmount).toBe(6000); |
| it('should negate convertedAmount on the optimistic transactions so the table total stays positive', async () => { | ||
| // Given a policy and an IOU report with a transaction that has positive converted amounts (IOU sign convention) | ||
| const policyID = '302'; | ||
| const policyWithEmptyFieldList: OnyxTypes.Policy = { |
There was a problem hiding this comment.
| const policyWithEmptyFieldList: OnyxTypes.Policy = { | |
| const policy: OnyxTypes.Policy = { |
|
|
||
| // When converting the IOU report to an expense report, apply the resulting optimistic data to Onyx | ||
| const result = Report.convertIOUReportToExpenseReport(iouReport, policyWithEmptyFieldList, policyID, 'expenseChat302', [transaction]); | ||
| await Onyx.update(result.optimisticData); |
| // Failure rollback: fail the network and verify the original positive values are restored in Onyx | ||
| mockFetch?.fail?.(); | ||
| await mockFetch?.resume?.(); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| const rolledBackTransaction: OnyxEntry<Transaction> = await new Promise((resolve) => { | ||
| const connection = Onyx.connect({ | ||
| key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, | ||
| callback: (value) => { | ||
| Onyx.disconnect(connection); | ||
| resolve(value); | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| expect(rolledBackTransaction?.amount).toBe(5000); | ||
| expect(rolledBackTransaction?.convertedAmount).toBe(6000); | ||
| }); |
| * so `amount`, `modifiedAmount` and `convertedAmount` are negated to match the expense-report convention. | ||
| * Absent converted values are not added so they keep being derived from the amount. | ||
| */ | ||
| function getExpenseReportSignedTransaction(transaction: Transaction): Transaction { |
There was a problem hiding this comment.
| function getExpenseReportSignedTransaction(transaction: Transaction): Transaction { | |
| function getNegatedAmountTransaction(transaction: Transaction): Transaction { |
- Rename getExpenseReportSignedTransaction to getNegatedAmountTransaction - ReportTest: inspect optimisticData directly, drop Onyx round-trip and failure-rollback block - PolicyTest: drop failure-rollback block on the negate-amount test
| // When converting the IOU report to an expense report | ||
| const result = Report.convertIOUReportToExpenseReport(iouReport, policy, policyID, 'expenseChat302', [transaction]); | ||
|
|
||
| // Then the optimistic transactions update negates amount and convertedAmount to the expense-report sign convention | ||
| const transactionsUpdate = result.optimisticData.find((update) => update.onyxMethod === Onyx.METHOD.MERGE_COLLECTION && update.key === ONYXKEYS.COLLECTION.TRANSACTION); | ||
| const negatedTransaction = (transactionsUpdate?.value as Record<string, OnyxTypes.Transaction> | undefined)?.[ | ||
| `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}` | ||
| ]; | ||
| expect(negatedTransaction?.amount).toBe(-5000); | ||
| expect(negatedTransaction?.convertedAmount).toBe(-6000); |
There was a problem hiding this comment.
U shouldn't check on optimisiticData, you need to read from onyx and assert.
There was a problem hiding this comment.
Done in 3a0aca7 — the test now seeds the policy + transaction in Onyx, calls Report.convertIOUReportToExpenseReport, applies result.optimisticData via Onyx.update, and asserts the stored transaction's amount / convertedAmount via an Onyx.connect read. This matches the read-from-Onyx pattern used by the createWorkspaceFromIOUPayment test.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chrome2026-05-28.19-03-56.mp4iOS: HybridApp2026-05-28.22-41-31.mp4iOS: mWeb Safari2026-05-28.19-06-48.mp4MacOS: Chrome / Safari2026-05-28.19-00-58.mp4 |
…ExpenseReport test
|
|
||
| // When converting the IOU report to an expense report and applying the optimistic data to Onyx | ||
| const result = Report.convertIOUReportToExpenseReport(iouReport, policy, policyID, 'expenseChat302', [transaction]); | ||
| await Onyx.update(result.optimisticData); |
There was a problem hiding this comment.
U don't have to explicitly apply the optimistic data running the action convertIOUReportToExpenseReport is enough
There was a problem hiding this comment.
Moved that onto the action path. convertIOUReportToExpenseReport only builds and returns the optimistic data, so to read the negated value back from Onyx (rather than asserting on the returned optimisticData) the test has to go through the action that actually dispatches the write. The existing moveIOUReportToPolicyAndInviteSubmitter test already does that, so I folded the convertedAmount check in there — it now asserts amount, modifiedAmount, and convertedAmount are all negated straight from Onyx after the action runs — and dropped the standalone builder-level test. a2f977b
| mockFetch?.pause?.(); | ||
|
|
||
| const mockTranslate = ((key: string) => key) as unknown as Parameters<typeof Policy.createWorkspaceFromIOUPayment>[8]; |
There was a problem hiding this comment.
Removed it. The convertedAmount negation is already asserted through the action path in the moveIOUReportToPolicyAndInviteSubmitter test in ReportTest — it runs the action and reads the transaction back from Onyx, checking that amount, modifiedAmount, and convertedAmount are all negated — so this createWorkspaceFromIOUPayment case was duplicating that coverage. Dropped it along with the now-unused imports. 544b927
There was a problem hiding this comment.
I only told you to remove the three lines not the whole test.
There was a problem hiding this comment.
Sorry, I misread that as the whole block. I've restored the test and dropped just the mockFetch?.pause?.() line — the negated optimistic amounts persist in Onyx after the action resolves, so the assertions still read back -5000/-6000 without pausing the fetch mock. c072bca
I'll get the last two checklist items (offline + high-traffic) wrapped up so the checklist gate goes green.
There was a problem hiding this comment.
I'll get the last two checklist items (offline + high-traffic) wrapped up so the checklist gate goes green.
BTW for this pr you don't have to apply those tests but only include the checklist checked in the OP so that the author checklist passes
|
complete the author checklist |
The convertedAmount negation is already asserted through the action path in the ReportTest moveIOUReportToPolicyAndInviteSubmitter test, so the createWorkspaceFromIOUPayment-based test here duplicated that coverage. Drop it along with the now-unused imports.
|
And why don't you complete the author checklist it is failing. U are wasting my time @trasnake87 |
The negated optimistic amounts persist in Onyx after the action resolves, so the test reads them back without pausing the fetch mock.
| await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| const mockTranslate = ((key: string) => key) as unknown as Parameters<typeof Policy.createWorkspaceFromIOUPayment>[8]; |
There was a problem hiding this comment.
Removed the explanatory comment above the transaction setup in 54ed55e — the test name already describes the expected behavior. I left the mockTranslate line itself in place since it's the required localeTranslate argument used the same way by the other createWorkspaceFromIOUPayment tests above; let me know if you meant a different line.
| // IOU transactions are stored with a positive `convertedAmount`; the expense-report | ||
| // sign convention flips it on storage so the table total renders positive. |
There was a problem hiding this comment.
// IOU transactions are stored with a positive convertedAmount but the expense-report
// sign convention flips it on storage so the table total renders positive.
|
@trasnake87 remove the other parts except the test steps in Tests section. |
|
Done — trimmed the Tests section down to just the test steps. |
|
@inimaga to you |
3 participants
Explanation of Change
When an IOU report is moved to a workspace,
convertIOUReportToExpenseReportbuilds optimistic transaction data that negatesamountandmodifiedAmountto match the expense-report sign convention, but it never negatesconvertedAmount/convertedTaxAmount.The per-row TOTAL column renders through
getConvertedAmount, which returns the opposite sign of the storedconvertedAmountfor expense reports (expense-report transactions are stored with negative converted amounts — see the existing fixture intests/unit/ReportUtilsTest.ts). Because the IOUconvertedAmountwas left positive,getConvertedAmountreturns a negative value once the report becomes an expense report, so each transaction's TOTAL cell flips negative. The report-level header total stays correct because it is negated separately, which matches the reported behavior (only the table cells go wrong).This change negates
convertedAmountandconvertedTaxAmountin the optimistic transaction data so the stored shape matches the backend representation of expense-report transactions. A conditional spread is used so absent values (e.g. an offline expense with no backend-computed conversion yet) are not overwritten with-undefined/-0. The same buggy loop is duplicated increateWorkspaceFromIOUPayment(paying an IOU by creating a new workspace), so the fix is applied in both places to fully close the bug class.transactionFailureDatais built from the unmodified transaction, so rollback on failure already restores the original positive values.Fixed Issues
$ #90219
PROPOSAL: #90219 (comment)
Tests
Offline tests
QA Steps
Same as the Tests section above.
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
Web
End-to-end walkthrough of the OP repro on the Web build:
$50.00then$35.00) — the chat shows the IOU card Submitter Emp owes $85.00 · 2 expenses.May 26).Totalcolumn holds at $50.00 and $35.00 (positive) — without this PR's fix both rows would flip to a negative amount on the table.web_91079_op_20260526_082744.mp4
Android: Native
Same OP repro on the Android native build (Pixel 7 / API 35 emulator):
$50.00then$35.00) via + → Create expense → keypad → Next → Create $X.00 expense.android_native_91079_op_FINAL.mp4
Android: mWeb Chrome
Same OP repro driven through Chrome on the Android emulator (mobile web). After the conversion, the per-row Totals stay positive at $50.00 and $35.00.
android_mweb_91079_op_cropped.mp4
iOS: Native
Same OP repro on the iOS native build (iPhone 17e simulator, iOS 26.5):
$50.00then$35.00) via + → Create expense → keypad → Next → Create $X.00 expense.ios_91079.mp4
iOS: mWeb Safari
Same OP repro on Mobile Safari (iPhone 17e simulator, iOS 26.5). Post-conversion the per-row Totals hold at $50.00 and $35.00 (positive).
ios_mweb_91079_op_20260526_162655_cropped.mp4