Guard P2P participant selection against negative-amount expenses from self DM#94661
Conversation
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
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.
|
|
@codex review |
|
@MelvinBot Please replace all |
|
🤖 Done — replaced all |
|
@MelvinBot All good now. Please mark this PR as ready for review |
![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: 88d9780b4e
ℹ️ 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".
| // fail at submit. Mirror the resetToDefaultWorkspace behavior in AmountSubmission.ts and keep the | ||
| // expense on the self DM (its default) instead of assigning the P2P participant, so the user is | ||
| // stopped at selection rather than at submit. | ||
| const shouldKeepOnSelfDM = !!selectedParticipant?.isSelfDM || ((transaction?.amount ?? 0) < 0 && isParticipantP2P(selectedParticipant)); |
There was a problem hiding this comment.
When a negative manual expense is already associated with a policy expense chat (for example, global create auto-assigned the default workspace), choosing a P2P recipient now enters this branch and the following lines replace the destination with the self DM/unreported track flow. That is not the resetToDefaultWorkspace behavior referenced in the comment, which keeps auto-reported expenses on the default workspace; in this context the expense is silently converted into a personal tracked expense instead of staying on the workspace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 I checked this — the concern is valid and reachable, and the "mirrors resetToDefaultWorkspace" justification in the code comment is inaccurate for the workspace case.
The bug: A global-create manual expense with an auto-reporting default workspace lands on confirmation already assigned to the policy expense chat (useDefaultParticipants.ts:72-74). The "To" row stays editable because canEditParticipant only checks isFromGlobalCreate, not whether the transaction is already on a workspace (MoneyRequestConfirmationList.tsx:260, :420, :437-449). So a user can open the picker manually and select a P2P recipient on a negative workspace expense — which makes shouldKeepOnSelfDM true (IOURequestStepConfirmation.tsx:322) and the branch unconditionally forces self-DM + UNREPORTED + iouType=TRACK (:324-326), silently converting the workspace expense into a personal track expense.
Why the comment justification is off: the referenced resetToDefaultWorkspace resets to the policy expense chat when auto-reporting is on (self DM only when it's off). The new code drops to self-DM/TRACK in all cases, so it doesn't actually mirror that behavior on a workspace transaction.
Suggested fix: gate the negative-P2P branch so it only forces the self-DM/track path when the transaction's current target is the self DM (i.e. don't trigger it for a transaction already bound to a policy expense chat) — or mirror the auto-report-aware target selection from resetToDefaultWorkspace so an auto-reporting workspace expense stays on the workspace instead of becoming a track expense.
Caveats: requires the NEW_MANUAL_EXPENSE_FLOW beta and an auto-reporting default workspace; if the default is the self DM, the concern doesn't apply. thelullabyy — want me to implement the gating fix? If so, reply tagging me.
There was a problem hiding this comment.
🤖 Implemented in 24ddad22f.
Gated the negative-P2P branch in handleParticipantsAdded so it only forces the self-DM/track path when the transaction is not already bound to a policy expense chat (IOURequestStepConfirmation.tsx:324-325):
const isTransactionOnPolicyExpenseChat = transaction?.participants?.some((participant) => participant?.isPolicyExpenseChat);
const shouldKeepOnSelfDM = !!selectedParticipant?.isSelfDM || ((transaction?.amount ?? 0) < 0 && !isTransactionOnPolicyExpenseChat && isParticipantP2P(selectedParticipant));Now a negative expense that was auto-assigned to a workspace (global create) stays on that workspace when a P2P recipient is selected, instead of being silently converted into a personal track expense. The self-DM "Submit it to someone" case still keeps the expense on the self DM as before. I also updated the code comment, which previously claimed to mirror resetToDefaultWorkspace (inaccurate for the workspace case), and applied the same exclusion to the participant-page path in addParticipant.
Verified: lint, typecheck (tsc + tsgo), React Compiler compliance (no regression), and the related Jest suites (IOURequestStepConfirmationPageTest, IOUUtilsTest, TimeExpenseConfirmationTest — 99 tests) all pass.
|
@thelullabyy 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-30.at.05.59.52.mov |
|
@MelvinBot Please check this comment #94661 (comment) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88d9780b4e
ℹ️ 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".
| // fail at submit. Mirror the resetToDefaultWorkspace behavior in AmountSubmission.ts and keep the | ||
| // expense on the self DM (its default) instead of assigning the P2P participant, so the user is | ||
| // stopped at selection rather than at submit. | ||
| const shouldKeepOnSelfDM = !!selectedParticipant?.isSelfDM || ((transaction?.amount ?? 0) < 0 && isParticipantP2P(selectedParticipant)); |
There was a problem hiding this comment.
This guard only runs when the inline confirmation picker calls handleParticipantsAdded, but the “Submit it to someone” action from a tracked self-DM expense navigates through IOURequestStepParticipants (ChatActionableButtons → createDraftTransactionAndNavigateToParticipantSelector) and useParticipantSubmission.addParticipant saves the selected P2P participant before navigating to confirmation. In that path this callback is never invoked, so a negative tracked expense can still reach confirmation with a P2P destination and fail on submit; the same negative/P2P check needs to be applied before setMoneyRequestParticipants in the participant-page flow as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 I checked this — the bot is correct, this is a real gap. The guard only covers the inline-confirmation picker path; the participant-page path is unguarded.
Why: The new guard lives only in handleParticipantsAdded, whose only call site is the inline ParticipantPicker rendered behind the NEW_MANUAL_EXPENSE_FLOW beta at IOURequestStepConfirmation.tsx:884-892. The "Submit it to someone" flow does not go through it.
Traced path that bypasses the guard
- "Submit it to someone" on a tracked self-DM expense →
createDraftTransactionAndNavigateToParticipantSelectorwithactionName = SUBMIT, which navigates toMONEY_REQUEST_STEP_PARTICIPANTS(ReportUtils.tsSUBMIT branch). IOURequestStepParticipantswires the selector touseParticipantSubmission.addParticipant— nothandleParticipantsAdded.addParticipanthas only one special case —isSelfDM → trackExpense()(L216-L219). A real P2P participant falls through tosetMoneyRequestParticipants(L230-L237) with noamount < 0/isParticipantP2Pcheck, thengoToNextStepnavigates to confirmation with the P2P destination.
So a negative tracked expense can still reach confirmation with a P2P destination via this path and fail at submit — exactly the scenario the guard was meant to stop at selection time.
Suggested fix: apply the same (transaction?.amount ?? 0) < 0 && isParticipantP2P(selectedParticipant) → keep-on-self-DM logic before setMoneyRequestParticipants in addParticipant. Worth confirming which path is active per beta state — when NEW_MANUAL_EXPENSE_FLOW is off, the inline picker isn't rendered at all, so the participant-page path is the only one and the guard never runs.
Reply @MelvinBot implement this and I'll add the equivalent guard to addParticipant.
There was a problem hiding this comment.
🤖 Implemented in 24ddad22f.
Added the same negative-amount/P2P guard to the participant-page path in addParticipant: when the draft transaction's amount is negative and the selected participant is a P2P participant (isParticipantP2P), it now routes to trackExpense() and keeps the expense on the self DM instead of saving the P2P participant via setMoneyRequestParticipants. This stops the "Submit it to someone" flow at selection time even when NEW_MANUAL_EXPENSE_FLOW is off (the path that never reaches handleParticipantsAdded).
To stay consistent with the workspace-preservation fix, the guard is skipped when the expense is already bound to a policy expense chat, so it can't silently convert a workspace expense into a track expense (useParticipantSubmission.ts:223-226).
Verified: lint, typecheck (tsc + tsgo), React Compiler compliance (no regression), and the related Jest suites (IOURequestStepConfirmationPageTest, IOUUtilsTest, TimeExpenseConfirmationTest — 99 tests) all pass.
|
Please check this comment @MelvinBot #94661 (comment) |
…elections Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@puneetlath 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 resolve conflicts |
…2PParticipantSelection Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com> # Conflicts: # src/pages/iou/request/step/IOURequestStepConfirmation.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89ce348a55
ℹ️ 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".
| if ((firstParticipant?.isSelfDM || shouldKeepNegativeExpenseOnSelfDM) && !isSplitRequest) { | ||
| trackExpense(); |
There was a problem hiding this comment.
When this branch is hit from the full participant step, ParticipantSearchResults.addSingleParticipant still calls onFinish(..., newParticipants) immediately after onParticipantsAdded for any non-self row. Returning here only exits addParticipant: trackExpense() queues a TRACK confirmation, then goToNextStep queues its own navigation with the stale selectedReportID and the P2P nextParticipants, so the user can land on a SUBMIT confirmation for the unreported/self-DM draft instead of staying in the track flow. The negative-P2P fallback needs to also prevent or coordinate the subsequent finish callback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — verified and fixed in 734be7c.
You're right that this was a real regression. ParticipantSearchResults.addSingleParticipant calls onParticipantsAdded (→ addParticipant) and then, for any non-self row, also calls onFinish (→ goToNextStep), gated only by if (!option.isSelfDM). Since P2P participants aren't self-DM, both fired: addParticipant ran the negative-P2P fallback (trackExpense() + early return), but goToNextStep still ran afterward and queued a SUBMIT navigation with the stale selectedReportID (the early return happens before selectedReportID.current is updated), which could override the track flow. The pre-existing self-DM path never hit this because self-DM rows are excluded from the onFinish call.
Fix: mirror the same guard in goToNextStep so it bails out when the fallback already routed to the track flow, and extract the shared condition into a shouldKeepNegativeExpenseOnSelfDM helper to avoid duplicating the logic:
const effectiveParticipants = nextParticipants ?? currentParticipants;
// ...skip the submit navigation when the negative-P2P fallback already kept the expense on the self DM.
if (!isSplitRequest && shouldKeepNegativeExpenseOnSelfDM(effectiveParticipants?.at(0))) {
return;
}Verified locally: ESLint, typecheck-tsgo, and React Compiler compliance all pass.
|
@MelvinBot Please check this comment and fix if needed #94661 (comment) |
…ps expense on self DM Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
BUG: @MelvinBot
Expected Result: Actual Result: 
|
…tly keeping on self DM Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@MelvinBot can you address the comment above? |
|
@MelvinBot Could you please check and address this comment #94661 (comment) |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71356f05b6
ℹ️ 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".
| includeOwnedWorkspaceChats: iouType === CONST.IOU.TYPE.SUBMIT || iouType === CONST.IOU.TYPE.CREATE || iouType === CONST.IOU.TYPE.SPLIT || iouType === CONST.IOU.TYPE.TRACK, | ||
| excludeNonAdminWorkspaces: action === CONST.IOU.ACTION.SHARE, | ||
| includeP2P: !isCategorizeOrShareAction && !isPerDiemRequest && !isTimeRequest && !isTransactionFromCreditCardImport, | ||
| includeP2P: !isCategorizeOrShareAction && !isPerDiemRequest && !isTimeRequest && !isTransactionFromCreditCardImport && !shouldExcludeP2P, |
There was a problem hiding this comment.
When shouldExcludeP2P is true for a negative amount, this passes includeP2P: false into the generic report filter. That filter treats includeP2P=false as “policy expense chats only” (!includeP2P && !isPolicyExpenseChat), so it also removes the self-DM even though self-DM is the valid Track destination and includeSelfDM is still requested. In the negative self-DM flow, users without a workspace chat (or users who want to keep the expense personal) can end up with no selectable Personal/self-DM option and cannot proceed from the participant step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We also excluded self DM in original flow
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.27-1 🚀
|
|
🤖 Help site review — no changes required I reviewed this PR against What this PR changes: It adds a validation guard so that a negative-amount track expense created in a self DM can't be assigned to a P2P (individual) participant via Submit it to someone — the expense stays on the self DM as a track expense instead, since P2P chats don't support negative amounts. Positive amounts still submit to individuals, and negative amounts are still allowed on workspace (policy expense chat) expenses and self-DM track expenses. Why no docs change is needed:
Since no changes are required, I did not open a draft help site PR. @thelullabyy, please confirm you agree this doesn't need a help site update. If you'd like an FAQ entry added to |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.27-2 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
P2P chats don't support negative amounts, but the "Submit it to someone" flow from a self DM let a user select a P2P participant after a negative amount was already entered. The picker offered the participant, the participant was assigned to the negative-amount transaction, and "Create expense" then submitted it — producing a backend error.
The only existing negative-P2P guard lived in the standalone amount step (
AmountSubmission.ts), whereresetToDefaultWorkspaceroutes a negative amount back to the default workspace when returning from confirmation through the amount step — a path the new manual expense flow never takes.This change stops the user at selection rather than at submit, mirroring that
resetToDefaultWorkspacebehavior. InhandleParticipantsAdded, when the selected participant is a P2P participant (isParticipantP2P) and the transaction amount is negative, the expense is kept on the self DM (its default, as a track expense) instead of being assigned to the P2P participant. Self-DM track expenses and policy-expense-chat expenses still allow negative amounts; positive amounts can still be submitted to a P2P participant as before.This direction supersedes the earlier
useConfirmationValidationapproach in #94537 (now closed), which only blocked the negative amount at submit time rather than preventing the invalid P2P assignment.Fixed Issues
$ #94261
PROPOSAL: #94261 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
Same as Tests.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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