Do not block Search page payments on a missing chat report - #98760
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bd48b79a3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 925351a796
ℹ️ 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".
|
@MelvinBot review |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safariweb.movweb.2.mov |
|
@VickyStash could you please add some unit tests to this PR? |
|
@MelvinBot review |
ReviewSolid, well-reasoned fix. The core idea — treat a missing chat-report object as a blocker only for the optimistic chat updates, not for the API call itself (which only needs the IDs) — is correct, and the change is backward-compatible: Verified correct
Suggestions (non-blocking)
One thing worth confirmingThe linked issue pins the root cause on a stale/revoked auth-token (407) window, whereas this PR fixes a distinct silent-failure path (missing chat-report object). Both produce the same "nothing happens / only one paid" symptom, and your reproduction (evict chat from Onyx) is a genuine bug, so this is a good defensive fix regardless. Just flagging that it may not fully cover the original 407 scenario described in the issue — worth confirming with the reporter that this maps to what they hit. I did not run a live reproduction: it needs a specific data setup (2+ approved payable reports plus a chat evicted from Onyx) that isn't reproducible in an automated smoke test, and none of the changed lines affect initial render, so a page-load check wouldn't exercise them. |
|
@daledah Please, take another look! |
trjExpensify
left a comment
There was a problem hiding this comment.
Nice find, great to knock down these silent failures. 👍
mountiny
left a comment
There was a problem hiding this comment.
One NAB comment, but lets address that in follow up
There was a problem hiding this comment.
I wont block on this, but can you please follow up to update the tests to follow the Given, When, Then comments format? https://github.com/Expensify/App/blob/main/tests/README.md#documenting-tests
|
🚧 mountiny 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/mountiny in version: 9.4.57-0 🚀
|
|
🤖 No help site changes required. This PR is an internal bug fix — it stops bulk Pay > Mark as paid (and the row-level Pay dropdown) on the Reports page from silently skipping a report when its chat report object isn't loaded in Onyx. It restores the already-documented behavior; it does not add or change any feature, workflow, tab, setting, or button label. Why no docs update is needed
Because no doc PR was needed, there's no linked help site PR to review. |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.4.57-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Bulk Mark as paid on the Search page requires each report's chat report object to be resolvable from the search snapshot or Onyx. The
Searchsnapshot contains matched expense reports but not their parent policy-expense chats, so any chat not already cached in Onyx caused the report to be silently unpayable in bulk. The row-level Pay dropdown (PayActionCell) had the same unlogged early return. Paying worked only after opening the report (which loads the chat viaOpenReport) — matching the "sometimes nothing, sometimes only one" reports from the affected user.The chat report object is only used by
payMoneyRequestto build optimistic chat updates (chat preview,iouReportIDclearing, last message); the API call itself needs just the IDs. This PR stops treating the missing object as a blocker for money requests:useSearchBulkActions.ts(onBulkPaySelected): whengetChatReportForBulkPayfinds nothing, derivechatReportIDfromitem.chatReportID ?? iouReport.chatReportID ?? iouReport.parentReportIDand pay with a fallback{reportID, policyID}. ThepolicyID(same workspace as the expense report) keeps the billing-restriction check insidepayMoneyRequestworking. Invoices — or items with no derivable chatReportID — are still skipped, now with a[BulkPay]log.PayActionCell.tsx(row Pay dropdown):chatReportremoved from the top guard; money requests use the samefallbackChatReportpattern; invoices keep the strict check with a[SearchPay]log.Fixed Issues
$ #98444
PROPOSAL: N/A
Tests
chatReportIDof the chat where expenses are posted.Onyx.set('report_<chatReportID>', null).PayMoneyRequestsent for each, row label turns into paid).Offline tests
Same, as in Tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in Tests
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
MacOS: Chrome / Safari
web_bulk.mp4
pay_perRow.mp4