Repository navigation
Reapply Reconciliation - All Expenses export for card group selections with an ungrouped query - #102431
Conversation
…card selection" This reverts commit b629f76.
… searchKey for a card group selection Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of the branch so you can push updates: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Android test steps verification results: This PR's Tests section is still empty, so I ran the steps from Extend Reconciliation - All Expenses export to any group-by:card selection. Steps 1 and 9 pass; the card-group steps (2–7, 10) are blocked because the fresh test account has no card transactions (linking company cards requires an emailed magic code CI can't read), and step 8 is blocked by a Group by sheet that froze on Android after tapping Apply.
|
|
🤖 Web test steps verification results: This PR's Tests section is still empty, so I ran the steps from Extend Reconciliation - All Expenses export to any group-by:card selection. Steps 1, 8 and 9 pass; the card-group steps (2–7, 10) are blocked because the test account has no card transactions (adding company cards needs a live bank login Melvin doesn't have).
|
|
@MelvinBot |
…liationExportForCardGroups Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # tests/unit/hooks/useSearchBulkActionsExportTest.ts
|
I merged The only conflict was in
Checks: the test file passes (52/52), lint is clean on the changed files, and |
|
@MelvinBot |
|
I updated the PR description with the Tests, Offline tests, QA Steps, and PR Author Checklist from Extend Reconciliation - All Expenses export to any group-by:card selection.
You still need to run the Tests on an account with imported card transactions, because the CI test account has no card groups to select. |
|
@MelvinBot |
|
I couldn't mark this PR as ready for review. My environment blocked the To open it, click Ready for review at the bottom of the PR page. The branch is up to date with |
|
@ZhenjaHorbach 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // When the export menu is built for a search grouped by card | ||
| const {result} = renderHook(() => useSearchBulkActions({queryJSON: cardGroupedExpenseQueryJSON}), {wrapper: OnyxListItemProvider}); | ||
|
|
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
The comment uses the banned AI-generated phrase "carve-out". This phrasing makes the codebase read like it was written by a chatbot; CONSISTENCY-16 asks for plain, direct language and explicitly covers comments.
Replace "reach the carve-out" with a plain substitute, e.g.:
// Then the template is offered just the same, because both shapes a card group selection can take have to reach the special-case rule, or the option would come and go with how far the list happened to be scrolledReviewed at: 148911b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| // When the export menu is built for a search grouped by category instead of by card | ||
| const {result} = renderHook(() => useSearchBulkActions({queryJSON: groupedExpenseQueryJSON}), {wrapper: OnyxListItemProvider}); | ||
|
|
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
The comment uses the banned AI-generated phrase "carve-out". Prefer plain, direct language in comments.
Rewrite "the carve-out is limited to card groups" without the jargon, e.g.:
// Then no template is offered at all, because the exception is limited to card groups and every other grouping keeps the existing rule that a group cannot be scopedReviewed at: 148911b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| // When the export menu is built for a search grouped by card | ||
| const {result} = renderHook(() => useSearchBulkActions({queryJSON: cardGroupedExpenseQueryJSON}), {wrapper: OnyxListItemProvider}); | ||
|
|
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
The comment uses the banned AI-generated phrase "carve-out". Prefer plain, direct language in comments.
Rewrite "the carve-out narrows the templates" without the jargon, e.g.:
// Then only Current view is offered, because the exception only narrows the templates the user was already entitled to and must never hand out one they were notReviewed at: 148911b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@MelvinBot |
Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
NikkiWines
left a comment
NikkiWines
left a comment
There was a problem hiding this comment.
looks good overall, just one thing
| ); | ||
| } else { | ||
| const isGroupExport = !!queryJSON?.groupBy && selectedTransactionsKeys.some((key) => key.startsWith(CONST.SEARCH.GROUP_PREFIX)); | ||
| const {isGroupExport, transactionIDList} = getGroupExportScope(queryJSON, selectedTransactions); |
There was a problem hiding this comment.
The logic above (from 1019 - 1033) needs adjusting. When all matching items are selected, the Reconciliation template export skips the new group-query handling. It uses the original query, which still has groupBy:card, and does not include searchKey.
That could recreate the problem we had in the last PR: the template export may contain one row per card instead of all expenses, or include different results from the Reconciliation search.
Let's flatten the all-results query too and include searchKey.
There was a problem hiding this comment.
Also, let's add a regression test for the all matching items selected path once this is updated
There was a problem hiding this comment.
I fixed this and pushed b6ba75e.
When all matching items are selected, the template export now does two things:
- It drops
groupByandlimitand sorts by date, so the export lists every matching expense instead of one row per card. This uses a new shared helper,getUngroupedTemplateExportQuery, which the group-selection path also uses now. An ungrouped query is sent unchanged. - It sends
searchKey, asCurrent viewalready does (useSearchBulkActions.ts:1031-1048).
I added a regression test for the select-all path (useSearchBulkActionsExportTest.ts:1483). It fails without the fix.
Checks run
useSearchBulkActionsExportTest.ts: 53 passed- Related search suites: 801 passed across 12 suites
npm run typecheck: passed- ESLint on both changed files: clean
npm run fmt: no changesnpm run spell-changed: no issuesreact-compiler-compliance-check: fails with the same errors as before this change. This change adds no new errors.
|
@ZhenjaHorbach can you update your reviewer checklist please 🙇 |
Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
🚧 trjExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Works nicely!
2026-10-02_14-22-51.mp4
| * ExportSearchWithTemplate has no isGroupExport flag, so a grouped query exports one row per group instead of the group's expenses. | ||
| * Drop groupBy and limit the way buildSpecificGroupQuery does, so the export covers every expense the query matches. | ||
| * `limit` has to go too, because once groupBy is gone it would cap the exported expenses instead of the number of groups. |
There was a problem hiding this comment.
Can we make this comment less AI-y?
There was a problem hiding this comment.
I rewrote the comment in plain language and pushed 69015b1 (useSearchBulkActions.ts:349-353).
| it('scopes the Reconciliation export to the selected card groups instead of a transaction ID list', async () => { | ||
| // Given a user who qualifies for the Reconciliation template | ||
| mockTemplatesIncludingReconciliation(); | ||
| // Given a ticked card group | ||
| selectCardGroup(); | ||
| // Given one expense ticked on its own alongside it, because that expense belongs to another card and so is not covered by the group's filter | ||
| mockSelectedTransactions.tx2 = makeSelectedTransaction(); | ||
| const searchResults = makeSearchResults([]); | ||
| // Given the card group row as it arrives in the search snapshot, which the group's filter entry is derived from | ||
| Object.assign(searchResults.data, {[CARD_GROUP_KEY]: {cardID: 1234}}); | ||
| mockCurrentSearchResults = searchResults; | ||
| jest.mocked(getSelectedGroupFilterEntry).mockReturnValue({key: CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID, value: 1234}); | ||
|
|
||
| const {result} = renderHook(() => useSearchBulkActions({queryJSON: cardGroupedExpenseQueryJSON}), {wrapper: OnyxListItemProvider}); |
There was a problem hiding this comment.
| it('scopes the Reconciliation export to the selected card groups instead of a transaction ID list', async () => { | |
| // Given a user who qualifies for the Reconciliation template | |
| mockTemplatesIncludingReconciliation(); | |
| // Given a ticked card group | |
| selectCardGroup(); | |
| // Given one expense ticked on its own alongside it, because that expense belongs to another card and so is not covered by the group's filter | |
| mockSelectedTransactions.tx2 = makeSelectedTransaction(); | |
| const searchResults = makeSearchResults([]); | |
| // Given the card group row as it arrives in the search snapshot, which the group's filter entry is derived from | |
| Object.assign(searchResults.data, {[CARD_GROUP_KEY]: {cardID: 1234}}); | |
| mockCurrentSearchResults = searchResults; | |
| jest.mocked(getSelectedGroupFilterEntry).mockReturnValue({key: CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID, value: 1234}); | |
| const {result} = renderHook(() => useSearchBulkActions({queryJSON: cardGroupedExpenseQueryJSON}), {wrapper: OnyxListItemProvider}); | |
| it('scopes the Reconciliation export to the selected card groups instead of a transaction ID list', async () => { | |
| // Given a user who qualifies for the Reconciliation template | |
| mockTemplatesIncludingReconciliation(); | |
| // Given a ticked card group | |
| selectCardGroup(); | |
| // Given one expense ticked on its own alongside it, because that expense belongs to another card and so is not covered by the group's filter | |
| mockSelectedTransactions.tx2 = makeSelectedTransaction(); | |
| const searchResults = makeSearchResults([]); | |
| // Given the card group row as it arrives in the search snapshot, which the group's filter entry is derived from | |
| Object.assign(searchResults.data, {[CARD_GROUP_KEY]: {cardID: 1234}}); | |
| mockCurrentSearchResults = searchResults; | |
| jest.mocked(getSelectedGroupFilterEntry).mockReturnValue({key: CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID, value: 1234}); | |
| const {result} = renderHook(() => useSearchBulkActions({queryJSON: cardGroupedExpenseQueryJSON}), {wrapper: OnyxListItemProvider}); |
There was a problem hiding this comment.
Same for other comments, our style guide says newlines above comments
There was a problem hiding this comment.
I applied your suggestion and pushed 69015b1. I also added a blank line above the other comments in the card group tests that this PR adds (useSearchBulkActionsExportTest.ts:1299).
|
@MelvinBot |
…test comments Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
I fixed both of
Only comments and blank lines changed. The export test file passes (53/53), and lint and spell check are clean. |
|
@MelvinBot |
…liationExportForCardGroups Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # tests/unit/hooks/useSearchBulkActionsExportTest.ts
|
Merged
The test file passes (58/58). ESLint and |
|
🚧 NikkiWines 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/NikkiWines in version: 9.5.3-0 🚀
|
|
No help site update needed. This PR adds Reconciliation - All Expenses as an export option for card groups. It doesn't make any article sentence wrong. Current view still works for grouped exports, as Search-and-Download-Expenses.md:60-80 describes. This matches the decision on the earlier docs PR for the same change, Docs updates for E/A#101522 Reconciliation export for card group selections, which was closed as not needed. |
|
This PR failing because of the issue #103163 |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.3-4 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
This reapplies Extend Reconciliation - All Expenses export to any group-by:card selection, which was reverted in [CP Staging] Revert "Extend Reconciliation - All Expenses export to any group-by:card selection", and fixes the deploy blocker that caused the revert: Spend- Click "Reconciliation - All Expenses" exported file not covered all expenses.
Commit 1 (
Reapply …) is a straightgit revertof the revert. It is identical to the merged #101522: on a search grouped by card,Reconciliation - All Expensesstays in the Export menu for a card group selection, scoped to the selected cards through acardID:filter.Commit 2 fixes the bug from the deploy blocker investigation. Exporting one card group produced a CSV with a single line. The template export sent the query that
addSelectedGroupsFilterbuilt, and that query still carriedgroupBy:cardandlimit.ExportSearchWithTemplatehas noisGroupExportflag (unlike theCurrent viewexport), so the backend ran a grouped search and exported one row per selected card.Changes in
src/hooks/useSearchBulkActions.ts:getTemplateGroupExportQueryhelper. It adds thecardID:filter, then dropsgroupByandlimitand sorts by date, the same waybuildSpecificGroupQuerybuilds a group's drill-down query. The query now lists the card's expenses.limithas to go too, because oncegroupByis gone it would cap the exported expenses instead of the number of groups.searchKey, as theCurrent viewexport already does.searchKeychanges which expenses the backend matches (for example, Reconciliation includes Expensify Card cash back), so leaving it out would make the exported set differ from the viewed set.cardID:filter, the export shows the download error instead of sending an ungrouped query with no card filter. That query would export every card in the search.AI tests run locally
npm test -- tests/unit/hooks/useSearchBulkActionsExportTest.ts: 46 passed (2 new for this fix)npm test -- --silent --testPathPattern "useSearchBulkActions|Search/SearchUIUtils|SearchActions": 794 passed across 12 suitesnpm run lint-changed: cleannpm run typecheck: passednpm run fmt: no changesnpm run spell-changed: no issuesnpm run react-compiler-compliance-check check src/hooks/useSearchBulkActions.ts: fails with the sameCannot access refs during rendererrors onmain. This PR adds no new errors.groupBy:cardsearch has no group rows to select. The card-group export still needs a manual run on an account with real card data.Fixed Issues
$ #99172
PROPOSAL: #99172 (comment)
Tests
Prerequisite: sign in as a workspace admin (or card admin) of a workspace that has Company cards or the Expensify Card enabled and that has imported card transactions. Without imported card transactions a
groupBy:cardsearch returns no group rows, so steps 2–7 cannot be reached.Current viewandReconciliation - All Expenses. Verify no other template is listed — noAll Data - expense level, noAll Data - report level, noBasic export, and no custom workspace template.Reconciliation - All Expenses. Verify the export starts, and that the resulting CSV has one line per expense on the selected card, not a single line for the card (the bug from Spend- Click "Reconciliation - All Expenses" exported file not covered all expenses). Verify it covers every expense on the selected card — including expenses further down the list that were never scrolled into view — not only the rows that happened to be loaded on screen, and no expenses from other cards.Reconciliation - All Expensesagain. Verify the CSV contains the whole selected card group plus that one extra expense.Current viewinstead ofReconciliation - All Expenses. Verify the same rows are covered — the two export paths must agree on scope.Current viewis the only option and no templates are listed at all.Current view,All Data - expense level,All Data - report level,Basic export,Reconciliation - All Expenses).Reconciliation - All Expensesis not offered for a card group selection — onlyCurrent view.Offline tests
Selecting a card group and choosing
Reconciliation - All Expenseswhile offline shows the existing offline modal instead of starting an export — unchanged by this PR, which only decides whether the option is listed and which query is exported.QA Steps
Same as the Tests section above, including the prerequisite: the QA account must be a workspace or card admin of a card-enabled workspace with imported card transactions, otherwise the
Card statementssearch has no group rows to select and steps 2–7 cannot be run.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