[CP Staging] Block negative sign for IOU expense inline editing in IOU report#90882
Conversation
… report TotalCell was reading report/policy from transactionItem, which is undefined in the IOU report view (MoneyRequestReportTransactionItem passes a raw Transaction). This caused shouldEnableNegative to hit a fallback path that incorrectly allowed negative amounts. Thread the report and policy props that TransactionItemRow already receives down to TotalCell, falling back to transactionItem fields for the Search context where they are enriched. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
@MelvinBot Update Tests steps to be Tests
Offline testsSame as Tests step QA StepsSame as Tests step |
|
@MelvinBot complete your checklists. |
|
Updated the Tests, Offline tests, and QA Steps sections as requested. |
|
@melvin Mark this as ready for review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-05-17.at.3.11.27.PM.mov |
|
Completed the PR Author Checklist. Items checked as N/A (no new files, no new CSS, no new assets, no UI changes, no deeplinks, no Storybook, no message editing code, no new pages). Testing steps and failure scenario steps were already filled in. Left unchecked the items that require manual human testing (offline testing, high traffic account testing, screenshots, and platform-specific test runs). |
| transactionItem={transactionItem} | ||
| shouldShowTooltip={shouldShowTooltip} | ||
| shouldUseNarrowLayout | ||
| report={report} |
There was a problem hiding this comment.
@MelvinBot could you explain why we need to pass the report here?
There was a problem hiding this comment.
In the IOU report view, MoneyRequestReportTransactionItem passes a raw Transaction object as transactionItem — this object doesn't have .report or .policy populated on it. The actual report and policy are passed as separate props to TransactionItemRow, but TotalCell was only reading from transactionItem.report / transactionItem.policy.
Because both were undefined, shouldEnableNegative(undefined, undefined, ...) evaluated isFirstTimeCreatingReport as true, which incorrectly allowed negative amounts in IOU expenses.
In the Search context, transactionItem.report / transactionItem.policy are populated (the transaction object is enriched), so the fallback report ?? transactionItem.report preserves that existing behavior.
There was a problem hiding this comment.
@MelvinBot Yes, but that’s why we added both report and policy to TransactionItemRowWide.tsx. I’m asking why we also need to pass report here in TransactionItemRowNarrow.tsx.
There was a problem hiding this comment.
TransactionItemRow dynamically switches between the Wide and Narrow layouts based on screen size — on mobile or medium-width screens, the Narrow layout is used. Since TotalCell is rendered in both layouts, omitting report from the Narrow path would leave the same shouldEnableNegative(undefined, ...) bug on smaller screens.
That said, in the Narrow layout TotalCell is rendered without canEdit/onSave (read-only mode), so the user can't actually type a negative sign there. The report prop is still needed because allowNegative feeds into allowFlippingAmount, which controls the +/- toggle UI even in display mode. Without the correct report, the toggle could appear when it shouldn't.
|
@MelvinBot Please mark all your checklist items as checked. |
|
All checklist items have been marked as checked. |
|
@MelvinBot Please mark this PR as ready for review. |
|
PR has been marked as ready for review. |
melvin-bot
Bot
requested review from
JmillsExpensify and
ahmedGaber93
and removed request for
a team
|
@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] |
![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: 9451328655
ℹ️ 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".
| transactionItem={transactionItem} | ||
| shouldShowTooltip={shouldShowTooltip} | ||
| shouldUseNarrowLayout | ||
| report={report} |
There was a problem hiding this comment.
TotalCell now derives allowNegative from shouldEnableNegative(report, policy, ...), but the narrow row only forwards report. In contexts where transactionItem is a raw transaction (e.g., money request rows) and policy is only available via TransactionItemRow props, policy becomes undefined on narrow layouts, so negative-amount behavior can diverge from wide layout for the same transaction (notably group-policy flows). Forwarding policy={policy} here keeps edit behavior consistent across layouts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think we need either of them in TransactionItemRowNarrow.tsx since editing is not allowed in the narrow layout.
In the narrow layout, only the children (the read-only TextWithTooltip with amountToDisplay) are rendered, and allowFlippingAmount is never used.
There was a problem hiding this comment.
@MelvinBot don't pass report={report} to TransactionItemRowNarrow.tsx due to the above comment.
There was a problem hiding this comment.
You're right — editing is not allowed in the narrow layout so report is unnecessary for TotalCell there. Removed it in eb27a8d.
Editing is not allowed in the narrow layout, so report is unnecessary for TotalCell there - only the read-only TextWithTooltip children are rendered and allowFlippingAmount is never used. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
@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] |
|
The failing ESLint check is unrelated to this PR's code changes. Analysis: The ESLint runner crashed with Evidence: The logs show no lint warnings or errors before the crash — just Recommendation: Re-run the failed ESLint check. If it continues to segfault, it may be a transient CI resource issue. |
|
@ahmedGaber93 @MelvinBot ESLint is failing. |
|
The ESLint check failed with a Segmentation fault (exit code 139), not an actual lint error. This is a CI runner/infrastructure issue unrelated to this PR's code changes. Evidence: The log shows Recommended action: Re-run the failed ESLint check — this should resolve on retry. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
mountiny
changed the title
|
🚧 @puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
…eport Block negative sign for IOU expense inline editing in IOU report (cherry picked from commit a63f963) (cherry-picked to staging by mountiny)
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.75-6 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a pure bug fix that corrects internal prop threading ( |
|
CP is passed for the QA team negative.sign.mp4 |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.75-10 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is an internal bug fix that corrects prop threading ( |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.75-14 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.76-0 🚀
|
|
No help site changes are required for this PR. This is a bug fix that corrects internal prop threading ( |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.77-3 🚀
|
5 participants
Explanation of Change
TotalCellcomputes whether negative amounts are allowed by readingtransactionItem.reportandtransactionItem.policy. In the IOU report view,MoneyRequestReportTransactionItempasses a rawTransactionobject astransactionItem— it doesn't have.reportor.policyfields populated. The actualreportandpolicyare passed as separate props toTransactionItemRow, but were never forwarded toTotalCell.When
shouldEnableNegative(undefined, undefined, ...)is called, theisFirstTimeCreatingReportfallback evaluates totrue, incorrectly enabling negative amount input for IOU expenses.This PR threads the
reportandpolicyprops fromTransactionItemRowdown toTotalCell, using them as the primary source with a fallback totransactionItem.report/transactionItem.policyfor the Search context where they are enriched on the transaction object.Fixed Issues
$ #90853
PROPOSAL: #90853 (comment)
Tests
Offline tests
Same as Tests step
QA Steps
Same as Tests step
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