Fix invoice Pay from Search transaction rows (resolve chatReport in PayActionCell)#95873
Fix invoice Pay from Search transaction rows (resolve chatReport in PayActionCell)#95873MelvinBot wants to merge 4 commits into
Conversation
…transaction rows Co-authored-by: Pujan Shah <Pujan92@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. |
Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
Fixed the failing Spell check — pushed Cause: cspell flagged the word The other failing check, |
|
Update: after my push ( The only remaining failing check is |
|
✅ The Spell check is now green after my earlier fix ( The only remaining red check is |
|
Two of the three failing checks are handled; the third is unrelated to this PR. ✅ Spell check — fixed in
This PR only changes I did not touch those files or the shared seatbelt baseline, since that's outside this PR's scope and would absorb an unrelated change's violations. This should be resolved on checklist (PR Author Checklist) — human-only checkboxes; those need to be filled out by the co-author, not me. |
|
@MelvinBot reasoning why you haven't applied your suggested approach? |
|
Good question, Reasoning for the change:
On the proposal's secondary suggestion (key the invoice check on Happy to switch to the row-level approach if you feel the fix belongs there — let me know. |
|
@MelvinBot I think better to pass it with the prop as that is what follows for group item. We can think of refactoring by taking out chatReport prop entirely and use directly in PayActionCell in any of the future PR. cc: @luacmartins as it is added recenetly in #93308 |
…l as a prop Mirror the grouped-report path: resolve the invoice/chat room in TransactionListItemWide and thread it through TransactionItemRow to the Pay action, instead of falling back inside PayActionCell. Revert the PayActionCell self-resolution accordingly and repoint the regression test to the prop contract. Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
Done — switched to the prop approach in What changed:
Agreed on taking out the Checks run locally: oxfmt, |
|
✅ The Spell check failure is fixed and now passing (pushed The only remaining red check is |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot you forgot to add the prop in TransactionItemRow component |
|
@MelvinBot mark this ready for review and complete the author checklist by ticking all checkboxes. I will take care of recordings and other stuff in my reviewer checklist |
TransactionListItemWide already resolves the invoice/chat room and passes it as chatReport to TransactionItemRow, and TransactionItemRowWide already forwards it to DeferredActionCell/PayActionCell. But TransactionItemRow dropped the prop (missing from both the destructure and wideForwardedProps), so PayActionCell received chatReport=undefined on the flat type:invoice row and its confirmPayment guard bailed out. Forwarding the prop completes the chain so payInvoice fires. Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
You're right — good catch, and it was a real bug, not just a cosmetic omission. Fixed in 241f325. After the switch to the prop-threading approach, the chain was wired everywhere except the middle layer:
Net effect: Verification
One coverage caveat: |
|
Status update on the checks:
Nothing further for me to fix here — the code-level failure is resolved. Once the PR Author Checklist is completed the remaining red check will clear. |
|
Done — marked this ready for review. ✅ On the author checklist: I ticked the boxes I can genuinely stand behind from the diff itself, and left the rest for you, since several are personal attestations of work I didn't actually perform. Ticked:
I intentionally left these unchecked for you to confirm, because they attest to manual/runtime work I can't truthfully self-certify:
Also heads-up: the Tests and QA Steps sections still have the |
|
The only actionable failure — Spell check — was already fixed in The one check still red is No other checks are failing ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 241f32587e
ℹ️ 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".
|
|
||
| // Resolve the transaction's chat report (e.g. the invoice room) and pass it to the Pay action, mirroring the grouped | ||
| // report rows in ReportListItemHeader. Without it, Pay from a flat transaction row (e.g. `type:invoice columns:...`) is a no-op. | ||
| const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(transactionItem.report?.chatReportID ?? transactionItem.report?.parentReportID)}`); |
There was a problem hiding this comment.
Read the chat report from live Onyx
In Search rows, @hooks/useOnyx is snapshot-aware, so this subscription reads from the current search snapshot instead of the live report collection. For flat type:invoice columns:... results where the snapshot contains the invoice/IOU report but not the parent invoice room, chatReport stays undefined and PayActionCell still returns at its !chatReport guard. This component's parent already uses originalUseOnyx for live parent report data; the pay path needs the same live fallback here.
Useful? React with 👍 / 👎.
|
No code fix needed this round. The only remaining failing checks are:
Both are human-only checkbox gates — they pass once the author and reviewer tick every box in the checklist. I can't complete those on anyone's behalf. The previously-failing Spell check was already fixed in |
Explanation of Change
When searching
type:invoicewith a customcolumns:...list, invoices render as flat transaction rows (TransactionListItem→TransactionItemRowWide), not grouped report rows. That path mounts the Action column'sDeferredActionCell→PayActionCellwithout achatReportprop (the grouped-report path viaReportListItemHeaderdoes pass it).As a result, when the user picks Pay → Pay as an individual → Mark as paid,
PayActionCell.confirmPaymenthits its guardif (!type || !reportID || !hash || !amount || !chatReport)and returns immediately becausechatReportisundefined.payInvoice()is never called — the reported "nothing happens" symptom. (payInvoice/payMoneyRequestalso need the invoice room forinvoiceReceiver/policy context, so the missing chat report matters beyond just the guard.)This change resolves
chatReportinsidePayActionCellitself — the single convergence point every per-row Pay flows through. When the prop is absent it falls back to the report referenced byiouReport.chatReportID, reusing theallReportsOnyx collection the component already subscribes to (no new Onyx connection). ThechatReportprop still takes precedence, so grouped report rows are byte-identical, and every downstream consumer (invoiceReceiverPolicyID,canIOUBePaid,useReportPaymentContext, the guard, and both pay calls) automatically gets a real chat report. This fixes flat-row Pay for both invoices and expense reports without threading a new prop through the sharedTransactionItemRow, which is consumed by many non-Search surfaces.Added
tests/ui/components/PayActionCellTest.tsxcovering the regression: an invoice row with nochatReportprop resolves it from Onyx and callspayInvoice, and still bails out when no chat report can be resolved from either source.Fixed Issues
$ #95422
PROPOSAL: #95422 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
Suggested manual test:
type:invoice columns:receipt,date,status,merchant,from,category,tag,amount,action.Offline tests
Same as tests.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssection/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))src/libs/SearchParser/searchParser.js,src/libs/Pusher/pusher.js, etc.), I verified that the changes are made to the source file and the generated file is regenerateden.ts,es.ts), I verified that any changes made to translations are also reflected in the other translation filesTextInputfor text inputs,AmountFormfor amount inputs, etc./** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videosundefined