Repository navigation
Fix bulk Pay/Mark as Paid disappearing for whole-report selections - #94075
Conversation
…s Paid options appear for whole-report selections Co-authored-by: Eugene Voloshchak <eVoloshchak@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. |
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.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppscreen-20260622-185330-1782147197521.mp4Android: mWeb Chromescreen-20260622-185204-1782147111599.mp4iOS: HybridAppScreen.Recording.2026-06-22.at.18.49.22.moviOS: mWeb SafariScreen.Recording.2026-06-22.at.18.49.51.movMacOS: Chrome / SafariScreen.Recording.2026-08-31.at.10.30.35.mov |
|
Validated this writing on all of the platforms using the tests from |
|
@eVoloshchak 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: a4fea404a4
ℹ️ 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".
| ), | ||
| ], | ||
| [selectedTransactions], | ||
| () => [...new Set([...Object.values(selectedTransactions).map((transaction) => transaction.policyID), ...selectedReports.map((report) => report.policyID)].filter(Boolean))], |
There was a problem hiding this comment.
When a whole-report selection spans more than one workspace and the selected transaction entries are missing policyID (the case this patch is fixing), this appends policy IDs in selectedReports order while SearchBulkActionsButton still chooses currentSelectedReportID from selectedTransactionReportIDs. Those arrays are derived from different sources/orderings, so selecting reports in a different order (or numeric transaction-key ordering) can pair one report's iouReport with another workspace's policy for KYC/add-bank-account and restriction checks, routing payment setup to the wrong workspace. Derive the policy from the same report ID used for the current selected report instead of just appending report policies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot, make changes according to this review comment
There was a problem hiding this comment.
@MelvinBot Could you please check codex's comment?
There was a problem hiding this comment.
Checked it — Codex is right, and this PR is what made it reachable. Fixed in 9b1f3cb.
The problem. SearchBulkActionsButton.tsx:107-108 picks the policy and the report from two separately ordered and separately filtered arrays:
const currentSelectedPolicyID = selectedPolicyIDs?.at(0);
const currentSelectedReportID = selectedTransactionReportIDs?.at(0) ?? selectedReportIDs?.at(0);selectedReports is built in display order, Object.values(selectedTransactions) in click order, and the two arrays apply different filters — so index 0 is not a shared anchor. Both values are then consumed together: currentPolicy feeds handleBulkPayItemSelected (:151) → triggerKYCFlow and shouldRestrictUserBillableActions, and currentSelectedPolicyID is the policyID in addBankAccountRoute next to iouReport={selectedIOUReport} (:210-215). Mismatch = KYC / add-bank-account / restricted-action routed to the wrong workspace.
Why this PR made it live. Before, selectedPolicyIDs came from transactions only, so in exactly this scenario (report selection, transaction entries with no policyID) it was [] and currentSelectedPolicyID was undefined — no policy, no misrouting. This PR added ...selectedReports.map((report) => report.policyID), so it now resolves to a real policy that may belong to a different report than currentSelectedReportID.
The fix — anchor the policy to the report actually being acted on, keeping the old value as a last-resort fallback so the bug this PR fixes stays fixed:
const currentSelectedPolicyID =
selectedIOUReport?.policyID ??
searchData?.[`${ONYXKEYS.COLLECTION.REPORT}${currentSelectedReportID}`]?.policyID ??
selectedReports.find((report) => report.reportID === currentSelectedReportID)?.policyID ??
Object.values(selectedTransactions).find((transaction) => transaction.reportID === currentSelectedReportID)?.policyID ??
selectedPolicyIDs?.at(0);The snapshot fallback matters because this component renders outside SearchScopeProvider, so the live Onyx read misses reports that exist only in the search snapshot.
Scope, and two things worth knowing
Blast radius is payment setup, not payment execution. The actual payment loop is already correctly paired — useSearchBulkActions.ts:1357-1400 uses item.policyID and item.reportID from the same item. So the bug decided which workspace's bank-account/KYC screen you landed on, not which reports got paid.
useBulkPayOptions was never affected. payScopedPolicyIDs.at(0) and selectedBulkPayReportID can also mismatch, but selectedPolicyID is only a truthiness guard there (useBulkPayOptions.ts:146) — it's never used to look up a policy or build a route. I checked every reference. So the payScoped* work added earlier in this PR re-scoped the Pay menu only; it did not touch the pairing Codex flagged, which is why the concern survived.
Verification: npm run typecheck passed; eslint on the changed file 0 errors (2 pre-existing seatbelt warnings); react-compiler-compliance-check exit 0; oxfmt produced no changes; 10 bulk-action suites 189/189 passed.
Test gap I did not close: the existing SearchBulkActionsButtonTest.tsx only exercises the all-matching count label through a helper — it never renders the component with Onyx, so there's no scaffolding to assert the policy/report pairing without building it from scratch. I left that rather than bolt on a fragile test. Worth adding if you want a regression guard on this.
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
@eVoloshchak confirming a couple of things...
On this:
Proposal items 1 (offering a standalone Mark as Paid that is fully decoupled from the native Pay submenu)
... confirming that we are not pulling "Mark as paid" out of the Pay submenu, but just in certain cases we'll still show "Pay > Mark as paid" when it's available and in-app payment is not?
Secondly, related to this slack thread, confirming whether "Select all" will now show this manual bulk pay option, whereas today on "Select all on this page" does?
@trjExpensify, that is correct
As it stands now, it will not, that's a separate issue. But we can handle it here if it's needed (I suspect additional BE work will be needed, but not 100% sure) |
Okay cool, great... good to know. 👍
@luacmartins best to separate this out then, I guess? |
|
Yea, if we're not addressing that here. It'd require BE changes, so we can look for an external BE contributor to work on this |
|
What BE changes would we need, perhaps @allgandalf would be up for helping with that? |
|
I think we'd need to pass a flag like That said, we just removed the capability from the server looping over these reports since it was making the command go to the blocking commit, so I suspect a "pay all" would also lead to stability issues 🤔 We should probably discuss in engineering chat how we wanna do this |
Sure thing! Please assign me to the issue if my help is needed 🙇 |
|
Thanks @allgandalf. All yours! |
|
@luacmartins can you summarise the discussions happened on slack, i don't have access to that chat |
|
Yea, this is the plan: Follow the pattern in https://github.com/Expensify/Auth/pull/22402
This way if 1 report out of 100 fails, it doesn't fail the whole batch, and it's easier to retry |
|
No new changes this time — this request re-fired on the batch I already handled, and the work is already on the branch. All ten "remove this comment" threads from your last review pass were answered in What I did do: closed out five older threads of yours that had never gotten a reply.
Still open: the thread anchored to |
|
@MelvinBot #94075 (review) could you check this comment? |
Resolve currentSelectedPolicyID from the report currentSelectedReportID points at instead of taking the first entry of the independently ordered selectedPolicyIDs array, so a cross-workspace selection can't route KYC and the add-bank-account flow to a different workspace than the report being paid. Co-authored-by: huult <huult@users.noreply.github.com>
@trjExpensify what do you think? thank you 😄 |
| // Resolve the policy from the report that is actually being paid. selectedPolicyIDs is ordered and filtered | ||
| // independently of currentSelectedReportID, so on a selection spanning workspaces its first entry can belong to a | ||
| // different workspace than this report, which would send the KYC wall and the add-bank-account flow to the wrong | ||
| // workspace. Fall back to the first selected policy only when the report itself yields nothing. |
There was a problem hiding this comment.
Removed — pushed in 83b17c0.
Dropped the four lines above currentSelectedPolicyID ("Resolve the policy from the report that is actually being paid…"). Comment-only deletion, no code changed.
Verification
npm run typecheck— passedeslinton the changed file — 0 errors (2 pre-existing seatbelt warnings, unchanged)npm run react-compiler-compliance-check— passed for this fileoxfmt— ran repo-wide, produced no changes- Jest — 61/61 passed across
SearchBulkActionsButtonTest,useSearchBulkActionsTest,useSearchBulkActionsPayTest,SearchActionsTest
Co-authored-by: huult <huult@users.noreply.github.com>
| * so this covers reports beyond the currently loaded page(s) when "Select all" is checked in Search. | ||
| */ | ||
| function queueBulkPayReports(jsonQuery: string) { | ||
| write(WRITE_COMMANDS.QUEUE_BULK_PAY_REPORTS, {jsonQuery}); |
There was a problem hiding this comment.
@MelvinBot Why are we using the QUEUE_BULK_PAY_REPORTS API? Where did you get this information from?
There was a problem hiding this comment.
I didn't pick this command name — it was given to me. allgandalf supplied it and eVoloshchak instructed me to implement against it.
The chain:
allgandalf, Aug 25 — "the command name isQueueBulkPayReportsand the param it takes isjsonQuery"eVoloshchak, Aug 30 — "@MelvinBot, implement this: On Search → Reports (Expenses), show manual bulk pay option when 'Select all' is checked. It should call the new API method, the command name isQueueBulkPayReportsand the param it takes isjsonQuery. Make sure to account for pagination, there could be more than one page of selected reports."
That instruction is also why select-all bulk pay is in this PR at all — it's outside the original proposal. eVoloshchak had earlier flagged it as a separate issue "we can handle here if needed", then decided to fold it in.
Backend side, per earlier comments on this PR:
luacmartins, Jun 29 described the design — an Auth command queries all matching reports and callsCreateJobsto create one job per report, spaced 1–2s apart, so a single failure doesn't sink the batch.trjExpensify, Jul 28 named the implementing PRs:Expensify/Auth#22681andExpensify/Web-Expensify#54319.
One thing you should confirm before merging
I have not verified the backend command exists. My access is limited to Expensify/App, so I can't open those two backend PRs or grep for the command — I'm relaying what was stated in this thread, not something I checked. QueueBulkPayReports isn't on App main; this PR adds the client half only. If those backend PRs haven't shipped, select-all Pay will fail at runtime with no client-side signal.
Worth a direct check with allgandalf or eVoloshchak that both have deployed.
Where it lands in this PR
Introduced in c3bb029 ("Add manual bulk pay for Select-all via QueueBulkPayReports"):
src/libs/API/types.ts:570— command registrationsrc/libs/API/parameters/QueueBulkPayReportsParams.ts— new params type, singlejsonQueryfieldsrc/libs/actions/Search.ts:1959-1961—queueBulkPayReports()src/hooks/useSearchBulkActions.ts:1345-1351— theareAllMatchingItemsSelectedbranch that calls it
It now fires for any "Select all", not only multi-page selections — which resolves the concern eVoloshchak raised on Aug 31 about it being skipped when the selection fits on one page.
huult
left a comment
huult
left a comment
There was a problem hiding this comment.
Screen.Recording.2026-09-06.at.10.48.41.mov
LGTM.
|
@MelvinBot can you please resolve merge conflicts ? @luacmartins i think the PR is now ready for your review |
|
🤖 I resolved and verified the merge locally, but I'm not allowed to push it — see the note at the bottom. The resolution is a two-word change, so it's fast to apply by hand. The conflictMerging It's a pure adjacency collision — the two sides don't overlap semantically:
The resolution — keep both sides// Reconciliation - All Expenses reports on card spend, so it is offered whenever the current user is a workspace admin or card
// admin of a workspace with a card product enabled, regardless of which rows are selected.
const hasCardEnabledAdminPolicy = useMemo(() => Object.values(policies ?? {}).some((policy) => isAdminOfCardEnabledPolicy(policy)), [policies]);
const selectedBulkCurrency = payScopedReports.at(0)?.currency ?? Object.values(selectedTransactions).at(0)?.currency;
const totalFormattedAmount = getTotalFormattedAmount(convertToDisplayString, payScopedReports, selectedTransactions, selectedBulkCurrency);Take Verification I ran on the resolved merge
I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author should reply with a request tagging me. Concretely: this PR's assignee is |
|
@MelvinBot i'm the assignee now, can you please resolve merge conflicts, thanks |
…aidPolicyIDs Resolved one conflict in src/hooks/useSearchBulkActions.ts. main added hasCardEnabledAdminPolicy directly above selectedBulkCurrency, while this branch changed selectedBulkCurrency and totalFormattedAmount to read from payScopedReports instead of selectedReports. Kept both sides. Co-authored-by: Gandalf <allgandalf@users.noreply.github.com>
|
🤖 Merge conflicts resolved — pushed as Merged The one conflict
It was a pure adjacency collision, so I kept both sides:
Net result vs AI Tests
Compiler-compliance baseline comparisonThe check fails on this file, but it fails identically on
Same counts, same lines (babel 449/2052, oxc 507/739/1805) — all outside this PR's diff. Suites run
What I did not run locally: the full I also didn't do a browser pass — your |
|
@luacmartins back to you 👍 |
|
🚧 luacmartins 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/luacmartins in version: 9.4.75-0 🚀
|
|
Yes — help site changes are required. Draft PR: #100748 This PR changed bulk Pay behavior in the Reports view in three ways that the help site either didn't cover or actively contradicted.
All edits landed in How each change maps to the diff, and what I could not verifySelect all → Pay elsewhere only. Partial selections. Label verification. Confirmed against Verification limits — please double-check these. The labels above come from the source strings, not the running app. The web tester confirmed the left nav (Spend with Expenses and Reports sub-tabs) but could not reach the bulk-pay UI: the test account has no workspaces and Reports sits behind a Collect-plan upgrade wall, so no report rows existed to select. The Select all / Mark as paid strings and the partial-selection behavior are unverified against a real workspace. Left alone deliberately. The single-report section of the same article still says Pay elsewhere where the New Expensify dropdown reads Mark as paid. That inaccuracy predates this PR, so I didn't widen the change — flagged in the docs PR body in case you want it fixed there. Guidelines applied. Both new headings are task-based and searchable, only @eVoloshchak, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
9 participants
Explanation of Change
When multiple whole reports are selected in the Search/Reports view, the bulk action menu intermittently dropped both Pay and Mark as Paid, leaving only Export — even when the selected reports shared the same currency and month.
Root cause:
selectedPolicyIDswas derived only fromselectedTransactions, never fromselectedReports. When the user selects whole reports (no individual transaction rows),selectedTransactionsis empty, soselectedPolicyIDswas empty andselectedPolicyIDwasundefined.useBulkPayOptionsshort-circuits tobulkPayButtonOptions = undefinedwhenselectedPolicyIDis missing, and the menu gate… && !!bulkPayButtonOptions?.lengththen removed the entire Pay group — including its Mark as Paid submenu item. (selectedReportIDalready falls back toselectedReportIDs, butselectedPolicyIDdid not — this gap was introduced alongside the cross-workspace bulk-pay change in #87148.)The fix derives
selectedPolicyIDsfromselectedReportsas well asselectedTransactions, so whole-report selections produce a validselectedPolicyIDanduseBulkPayOptionscan build the native Pay options (and, for multi-currency selections, the Mark as Paid / Pay Elsewhere fallback) instead of returningundefined.This implements item 2 of the approved proposal — the regression that explains the dominant reported symptom (options disappearing even for same-currency/same-month selections). Proposal items 1 (offering a standalone Mark as Paid that is fully decoupled from the native Pay submenu) and 3 (degrading rather than removing the group when one selected report has
canPay === false) change bulk-pay eligibility semantics and warrant dedicated multi-workspace QA, so they are intentionally left out of this PR and called out as follow-ups for the reviewer.Fixed Issues
$ #93699
PROPOSAL: #93699 (comment)
Tests
Offline tests
QA Steps
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