Support 'Select all matching' when moving expenses to a report - #98411
Support 'Select all matching' when moving expenses to a report#98411MelvinBot wants to merge 3 commits into
Conversation
Send the search jsonQuery + hash to CHANGE_TRANSACTIONS_REPORT so the backend moves all matching unreported expenses, not just the loaded page. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@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.
|
|
I analyzed the checks on this PR — there's no code failure for me to fix. Every code/test check is green (ESLint, The only two failing checks require human action, not a code change:
So: no fix to push. To get this green, complete the author checklist (and fill in the Tests/QA sections), then get an independent approval. |
|
@MelvinBot |
…ingMoveExpenses Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # config/eslint/eslint.seatbelt.tsv # src/hooks/useSearchBulkActions.ts
|
🤖 Merged Conflicts resolved:
The merge also correctly picked up Validation run locally:
|
|
@MelvinBot |
|
@codex |
![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: 2e2ff610e9
ℹ️ 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".
| Log.warn('[SearchTransactionsChangeReport] All matching expenses are selected but the search query is unavailable; only the loaded expenses will be moved.'); | ||
| return {}; | ||
| } | ||
| return {jsonQuery: serializeQueryJSONForBackend(currentSearchQueryJSON), hash: currentSearchQueryJSON.hash}; |
There was a problem hiding this comment.
If the connection drops after the user opens this RHP but before they choose a destination, this still supplies the all-matching query to API.write; the earlier offline check in useSearchBulkActions only runs while navigating here. The retryable write is then queued, and because hash identifies the query rather than a frozen result set, reconnecting can move expenses that started matching while offline. Check the current network state at submission and block the query-based move instead of queuing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c499c08.
The move RHP (SearchTransactionsChangeReport) now re-checks isOffline at submission time — in selectReport, removeFromReport, and createReport — and, when the selection is an all-matching query move, blocks it and shows the reconnect modal (same UX as Export) instead of queuing the retryable write. The guard only trips for query-based moves (getAllMatchingQueryParams() returning a jsonQuery); explicit-list moves still queue normally offline since they carry a frozen transaction list.
| // The backend moves the whole query, so the query must guarantee every match is movable: a non-movable | ||
| // expense hiding in unloaded results makes it reject the entire move. Unreported-only queries are safe | ||
| // because those expenses live in their owner's self DM. Exclusions can't be expressed in a query. | ||
| const isAllMatchingSelectionMovable = isEmptyObject(excludedTransactions) && (!hasUnloadedMatchingExpenses || isUnreportedOnlyQuery); |
There was a problem hiding this comment.
For a status:unreported search spanning multiple pages, this bypasses hasUnloadedMatchingExpenses after checking canChangeReport only on the loaded rows. Unreported expenses are not universally movable: canUnreportedBeMoved() rejects per-diem transactions when their policy or rates are unavailable, so an invalid expense on a later page makes the backend reject the entire query move even though this option is offered. Do not treat the status filter alone as proof that all unseen matches are eligible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is a real edge case, but I didn't change the behavior here because the honest fix is a product tradeoff that belongs to the PR author rather than a mechanical change, and I don't want to silently reverse the deliberate || isUnreportedOnlyQuery bypass.
The core constraint: the client can't enumerate the unloaded matches, so it can't prove every unseen unreported expense passes canUnreportedBeMoved() (per-diem with unavailable policy/rates being the counter-example you flagged). The backend is the authoritative enforcer either way. Two options:
- Keep the bypass (current), accept the tradeoff. Rare per-diem-on-a-later-page cases make the backend reject the whole move. This should be paired with surfacing that failure to the user (today the all-matching failure path only clears
pendingFields.reportID— same silent behavior as the existing explicit-list path, so it isn't a regression, but it's worth fixing). - Drop the bypass — require
!hasUnloadedMatchingExpenses. The move option then only appears when the client has loaded every match, socanAllTransactionsBeMovedfully covers the set. Cost: all-matching move isn't offered for unreported searches with more than one page of results.
I'd lean toward (2) if guaranteed correctness matters more than covering the >1-page unreported case. ZhenjaHorbach — which tradeoff do you want? Reply and I'll implement it.
Code Review — Support "Select all matching" when moving expensesReviewed the diff at What it does
Concerns worth discussing
Minor / nits
TestsGood coverage in ValidationI ran locally against this branch: Nothing here is blocking — the silent-failure path and the unreported-only always-movable assumption are the two I'd want an answer on before shipping. |
|
No code fix is needed — all automated checks pass. The two remaining ❌ checks aren't fixable by a code change:
When this request came in, ✓ ESLint check · ✓ typecheck · ✓ test (jobs 1–8, Jest) · ✓ Bun tests · ✓ Storybook tests · ✓ spellcheck · ✓ Check for unused styles · ✓ Compare knip issues against main · ✓ React Compiler Compliance · ✓ Oxfmt Nothing to push. To clear the last two, this PR just needs a reviewer to complete the checklist and grant approval. |
|
Nothing has changed since my previous analysis — there's still no code fix to push. The only two remaining ❌ checks are human-gated, not automated:
Every substantive automated check is green (ESLint, typecheck, Jest jobs 1–8, Bun, Storybook, spellcheck, unused styles, knip, React Compiler, Oxfmt). To clear the last two, this PR just needs a reviewer to complete the checklist and approve. |
|
@MelvinBot |
If the connection drops after the move RHP is open, block the query-based move and prompt the user to reconnect instead of queuing a retryable write. A queued all-matching move replays a stale query on reconnect (the hash resolves the match set at backend execution time), so expenses that started matching while offline could be swept in. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
🤖 Worked through the three comments. Summary: P1 — recheck connectivity before the query move (r3812170462): fixed in c499c08. P2 — "unreported ⇒ always movable" assumption (r3812170470): needs your call, not changed. The client can't enumerate unloaded matches, so it can't prove every unseen unreported expense passes My code review (comment): the two open items are judgment calls, left for you:
Validation for the P1 change: |
|
The latest commit ( The only remaining ❌ checks are human-gated and can't be resolved by a push:
Green on this commit: ✓ ESLint · ✓ typecheck · ✓ test (jobs 1–8, Jest) · ✓ Bun tests · ✓ Storybook · ✓ knip. (The perf-test jobs are still running but are non-blocking benchmarks.) To clear the last two, this PR just needs a reviewer to complete the checklist and approve. |
|
Still no code fix needed — all automated checks are green. The only two ❌ checks are human-gated and can't be resolved by a commit:
Everything substantive passes: ✓ ESLint · ✓ typecheck · ✓ test (jobs 1–8, Jest) · ✓ Bun tests · ✓ Storybook · ✓ spellcheck · ✓ unused styles · ✓ knip · ✓ React Compiler. To clear the last two, this PR just needs a reviewer to fill in the checklist and approve — there's nothing for me to push. |
2 participants
Explanation of Change
When an admin picks Select all matching expenses and then Add to report, the client was only able to move the first page of loaded results (50 items), even though the UI claimed all matching expenses were selected.
This change lets the "all matching" move be resolved by the backend from the search query itself instead of an explicit transaction list:
jsonQuery+hashtoChangeTransactionsReportParamsand a dedicated path inchangeTransactionsReportthat sends the serialized query with an emptytransactionList, so the backend moves the whole matching set. Without ahashit falls back to the explicit list (and logs a warning), so only intentional all-matching moves take the query path.useSearchBulkActionsnow surfaces the Move expenses option for all-matching selections only when it is safe: no excluded rows, and either the client has loaded every match or the query is unreported-only (those expenses live in the owner's self DM and are always movable). Offline all-matching moves prompt the reconnect modal, matching Export.SearchTransactionsChangeReportandIOURequestStepUpgradeforward the query + hash only when all-matching is selected with no exclusions.SearchQueryProvidernow joinsqandrawQuerybehind a null-byte separator so the restored query and its hash can't drift apart (restoring onlyqwould droprawFilterListwhile keeping the same hash). This also removes twono-unsafe-type-assertionseatbelt entries.Includes unit tests in
tests/unit/TransactionTest.tscovering the query+hash path, the unreported fallback reportID, optimistic pending flag on the destination report, and the explicit-list fallback when a hash is passed without a query.Fixed Issues
$ #92370
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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