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 3 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 👍
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.
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.
|
…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.
|
@FitseTLT good catch — the I added a matching test in Verified the test catches the pre-fix behavior: temporarily replacing |
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
Automated coverage: a unit test was added in
tests/actions/ReportTest.ts(convertIOUReportToExpenseReport › should negate convertedAmount and convertedTaxAmount...). It converts an IOU report whose transaction has positiveconvertedAmount/convertedTaxAmountand asserts the optimistic data stores them negated (and that the failure data restores the originals). The test fails onmainand passes with this change.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)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.