Group bulk export options by integration for multi-workspace selections - #96728
Group bulk export options by integration for multi-workspace selections#96728MelvinBot wants to merge 8 commits into
Conversation
Co-authored-by: ahmedGaber93 <ahmedGaber93@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. |
|
🤖 I checked all the checks on this PR. No code fix is needed — every real CI check passes. The only failing check is
This is expected for a draft PR: the Everything else is green: Next step: |
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.
|
|
@MelvinBot Update the bulk action sorting to group each integration's actions together:
instead of:
|
…ether Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Done — pushed The two loops in
See Also extended the multi-workspace unit test to assert the new grouped ordering. AI checks run locally:
|
|
I analyzed the failing checks — there's no code failure to fix here. The only failing check is PR Author Checklist / checklist, and it's not a code/CI test. It fails because the manual checklist hasn't been filled out:
Since this is an automated draft PR, the checklist boxes and the Every functional and AI check is green: I didn't push any change because there's nothing broken in the code. |
|
@MelvinBot Please update with the following changes. 1) Add a partial-export confirmation modalWhen a selection spans multiple integrations, clicking "Export to " only exports the subset of reports connected to that integration. Before exporting, if it's a partial export (the integration's report count is less than the total number of selected reports), show a confirmation modal:
2) Show BOTH confirmations in sequence when applicableIf the chosen integration's subset ALSO contains reports that were already exported, show two confirmations in sequence (never combined):
Confirming both proceeds with the export; cancelling either one aborts. Present them sequentially (present the second only after the first resolves) so they don't overlap. 3) Apply the SAME flow to "Mark as exported"Route mark-as-exported through the same shared confirmation flow used by export so the behavior stays identical. 4) TestsUpdate
Please run |
- Show a partial-export modal when Export/Mark-as-exported to an integration only covers a subset of the multi-integration selection - Present the partial modal and the existing export-again modal sequentially - Detect already-exported reports from report actions (live Onyx first, then the search snapshot) instead of the stale isExportedToIntegration field - Route Mark as exported through the same shared confirmation flow - Cover the new flows in useSearchBulkActionsExportTest Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Done — pushed to What changed1 & 2 — Partial-export modal, shown in sequence with export-again New copy lives under 3 — Already-exported detection from report actions 4 — Mark as exported uses the exact same 5 — Tests — Checks (run locally)
One note for the human co-author: the plural |
|
The only failing check is PR Author Checklist / checklist, and it's not a code problem — there's nothing for me to fix or push. It failed with:
This is a human-only step: the PR Author Checklist must be filled out (and the
Everything else is green or still building — no failing code checks:
|
|
Hi @MelvinBot — please add these changes. This builds on the existing partial-export confirmation work in that PR. GoalRestructure the bulk export confirmation modals so each one shows a fixed subtitle on top and a scrollable list of report names below it. To support this, add a reusable What to do
Resulting modal (example)
Please run our formatter, typecheck, lint, and the |
…ed subtitle + scrollable report list - Add subtitle/subtitleStyles props to ConfirmContent + ConfirmModal; subtitle renders between title and prompt and stays fixed above a scrollable prompt - Partial-export modal: descriptive text moves to the subtitle, the scrollable prompt lists the report names that will be exported - Export-again modal: reuse exportAgainModal.description (without the inline list) as the subtitle, list already-exported report names in the prompt - Update useSearchBulkActionsExportTest to assert the subtitle/prompt split Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Done — pushed to What changed1 — 2 — Partial-export modal ( 3 — Export-again modal ( Checks (run locally)
Same note as before: the non-English translations for the new |
|
Still the same, and there's nothing for me to fix in code. The only failing check is PR Author Checklist / checklist, failing with:
This is a human-only attestation — I don't tick those boxes on your behalf. The remaining |
|
@MelvinBot Merge main branch and fix the conflict |
…iWorkspaceIntegration Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com> # Conflicts: # src/hooks/useSearchBulkActions.ts
|
🤖 Merged Conflict: one overlapping change in the import type {BillingGraceEndPeriod, ExportTemplate, Policy, Report, ReportAction, ReportActions, ReportNameValuePairs, SearchResults, Transaction, TransactionViolations} from '@src/types/onyx';Both types are used in the merged file — |
|
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. |
|
Hi @MelvinBot — please add the following changes. 1. Show the export option when only some selected reports are eligibleIn
2. Align already-exported detection with the backend responseRevert 3. Split the partial-export subtitle into two case-based reasonsTests
Please run the formatter, |
- Show Export/Mark-as-exported when at least one report in a group is eligible; act only on the eligible subset - Revert already-exported detection to the backend-aligned pendingFields.export/isExportedToIntegration check - Split the partial-export subtitle into per-case reasons (other integrations and/or ineligible reports) - Update and add useSearchBulkActionsExport tests Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Done — pushed to 1. Show the export option when only some selected reports are eligible — replaced the 2. Align already-exported detection with the backend response — reverted 3. Split the partial-export subtitle into two case-based reasons — the description now takes Tests — added a mixed approved + submitted (same workspace) case that asserts the option still shows, the partial-export modal appears, and only the approved report is exported; updated the Checks run: Two notes:
|
| exportPartialModal: { | ||
| title: ({exportableCount, selectedCount, integration}: ExportPartialModalTitleParams) => | ||
| `Export ${exportableCount}/${selectedCount} reports to ${CONST.POLICY.CONNECTIONS.NAME_USER_FRIENDLY[integration]}?`, | ||
| description: ({integration, hasReportsOnOtherIntegrations, hasIneligibleReports}: ExportPartialModalDescriptionParams) => { | ||
| const reasons: string[] = []; | ||
| if (hasReportsOnOtherIntegrations) { | ||
| reasons.push(`Only reports connected to ${CONST.POLICY.CONNECTIONS.NAME_USER_FRIENDLY[integration]} will be exported.`); | ||
| } | ||
| if (hasIneligibleReports) { | ||
| reasons.push(`Only reports that are eligible to export will be exported.`); | ||
| } | ||
| return `${reasons.join('\n\n')}\n\nThe following reports will be exported:`; | ||
| }, | ||
| confirmText: ({count}: {count: number}) => ({ | ||
| one: `Export ${count} report`, | ||
| other: `Export ${count} reports`, | ||
| }), | ||
| cancelText: 'Cancel', | ||
| }, |
There was a problem hiding this comment.
@MelvinBot The non-English locale translations are now outdated. Please update them to match the latest English copy.
…copy
Update the exportPartialModal.description in all non-English locales to match
the new English logic, which conditionally builds up to two reasons from the
hasReportsOnOtherIntegrations and hasIneligibleReports params (previously the
translations only handled the single other-integrations reason).
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Updated the non-English What was outdated: English changed Fix: rewrote
|
Test stepsPrecondition
Case 1: Export reports from a single workspace
Case 2: Export reports from multiple workspaces
Case 3: Mixed eligibility across workspaces
|
|
@MelvinBot Please update TestsMelvin, please follow steps from here #96728 (comment) Offline testsN/A. QA StepsSame as Test step PR Author ChecklistMelvin, please mark all author checklist as checked. |
|
To make it a one-click edit, here's the exact content ready to paste into the PR body: Tests (paste from your test-steps comment)**Precondition**
- Create Workspace A and connect it to an accounting integration (e.g. QuickBooks Online).
- Create Workspace B and connect it to a different accounting integration (e.g. Sage Intacct).
- Create several reports in both workspaces with `Draft` and `Approved` statuses.
- Navigate to Spend > Reports.
**Case 1: Export reports from a single workspace**
1. Select 1 `Approved` report and 1 `Draft` report from Workspace A.
2. Verify the bulk export menu displays: Export to QuickBooks Online; Mark as exported (QuickBooks Online).
3. Click Export to QuickBooks Online.
4. Verify the confirmation modal shows Title `Export 1/2 reports to QuickBooks Online?` and Message `Only reports that are eligible to export will be exported.`
5. Click Export 1 report.
6. Open the approved report and verify it displays `Exported to QuickBooks Online and ...`.
7. Repeat steps 1–3.
8. Verify another confirmation modal appears with Title `Careful!` and Message `The following reports have already been exported to QuickBooks Online. Are you sure ...`.
9. Click `Yes, export again` and verify the report is exported again.
10. Repeat the entire flow using Mark as exported (QuickBooks Online).
**Case 2: Export reports from multiple workspaces**
1. Select 1 `Approved` report from Workspace A and 1 `Approved` report from Workspace B.
2. Verify the bulk export menu displays: Export to QuickBooks Online; Export to Sage Intacct; Mark as exported (QuickBooks Online); Mark as exported (Sage Intacct).
3. Click Export to QuickBooks Online.
4. Verify the confirmation modal shows Title `Export 1/2 reports to QuickBooks Online?` and Message `Only reports connected to QuickBooks Online will be exported.`
5. Click Export 1 report.
6. Open the approved report from Workspace A and verify it displays `Exported to QuickBooks Online and ...`.
7. Repeat the entire flow using Mark as exported (QuickBooks Online).
**Case 3: Mixed eligibility across workspaces**
1. Select 1 `Approved` + 1 `Draft` report from Workspace A and 1 `Approved` report from Workspace B.
2. Verify the bulk export menu displays: Export to QuickBooks Online; Export to Sage Intacct; Mark as exported (QuickBooks Online); Mark as exported (Sage Intacct).
3. Click Export to QuickBooks Online.
4. Verify the confirmation modal shows Title `Export 1/3 reports to QuickBooks Online?` and Message:
`Only reports connected to QuickBooks Online will be exported.`
`Only reports that are eligible to export will be exported.`
5. Click Export 1 report.
6. Open the approved report from Workspace A and verify it displays `Exported to QuickBooks Online and ...`.
7. Repeat the entire flow using Mark as exported (QuickBooks Online).
Then check off the author checklist items yourself once you've verified them. Happy to update anything else here — just not the human-attestation parts. |
|
@MelvinBot You have the permission, Just copy paste them from #96728 (comment). |
|
Done — updated the PR body:
|
|
@MelvinBot Mark this PR ready for review |
|
🤖 Done — this PR is now marked ready for review. A heads-up for reviewers/QA: the human-only Tests and QA Steps sections in the PR body are still placeholders. Please fill those in (and complete the author checklist) so QA has steps to run. |
|
@ahmedGaber93 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] |
|
@MelvinBot fix the speelcheck error https://github.com/Expensify/App/actions/runs/30330803792/job/90185305967?pr=96728 |
![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: 9739a1a016
ℹ️ 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".
| icon: integrationIcon, | ||
| onSelected: () => handleExportAction(() => markAsManuallyExported(selectedReportIDs, connectedIntegration)), | ||
| icon: getIntegrationIcon(integration, expensifyIcons), | ||
| onSelected: () => handleMarkAction(() => markAsManuallyExported(markableReportIDs, integration)), |
There was a problem hiding this comment.
When the user chooses Mark as exported for a mixed-workspace or mixed-eligibility selection, this routes through buildIntegrationHandleExportAction, whose partial confirmation always uses exportPartialModal copy such as “Export 1/2 reports…” and “reports will be exported,” while the final action only calls markAsManuallyExported. That confirmation describes a real accounting export instead of a manual mark, so users can confirm or cancel based on the wrong action; pass the action type into the helper or use separate copy for manual marking.
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-28.at.8.09.11.AM.movAndroid: mWeb ChromeScreen.Recording.2026-07-28.at.8.13.05.AM.moviOS: HybridAppi.moviOS: mWeb Safariiw.movMacOS: Chrome / SafariScreen.Recording.2026-07-26.at.9.19.21.AM.movScreen.Recording.2026-07-26.at.9.19.45.AM.mov |
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Fixed the spellcheck failure — pushed in bbdbb84. cspell was flagging the coined word |
|
@Beamanator 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] |
|
@MelvinBot add this link as a refrence at the first of the PR's OP https://expensify.slack.com/archives/C049HHMV9SM/p1783960809681689?thread_ts=1783618031.883769&cid=C049HHMV9SM |
|
Done — added the Slack thread reference as the first line of the PR description. |
2 participants
Reference: Slack thread
Explanation of Change
Bulk integration export options (export to accounting + "Mark as exported") only appeared when every selected report belonged to a single workspace, because
getExportOptions()inuseSearchBulkActionsderived a singleconnectedIntegrationfromselectedPolicyIDs.length === 1. When a selection spanned multiple workspaces (e.g. one QBO report + one Sage Intacct report),policywasundefined, so no integration options were shown at all — even though each report was individually exportable. This matched neither Classic Expensify nor user expectations.This PR groups the selected reports by their connected accounting integration and builds one "Export to " option and one "Mark as exported" option per integration, each scoped to just the reports for that integration. Export options are listed first, followed by the mark-as-exported options. A single-workspace selection collapses to one group, so the previous behavior is unchanged.
AI checks run locally (by MelvinBot):
npm test tests/unit/hooks/useSearchBulkActionsExportTest.ts— ✅ pass (3 tests, includes a new multi-workspace case)npm run typecheck-tsgo— ✅ passnpm run lint-changed— ✅ passnpm run fmt(oxfmt) — ✅ applied, only the two changed files reformattednpm run react-compiler-compliance-check check src/hooks/useSearchBulkActions.ts— no new divergence: the reported ref-access errors are pre-existing (identical error set onorigin/main; the file already carries areact-hooks/refseslint-disable)Fixed Issues
$ #95820
PROPOSAL: #95820 (comment)
Tests
Precondition
DraftandApprovedstatuses.Case 1: Export reports from a single workspace
Approvedreport from Workspace ADraftreport from Workspace AExport 1/2 reports to QuickBooks Online?Only reports that are eligible to export will be exported.Exported to QuickBooks Online and ...Careful!The following reports have already been exported to QuickBooks Online. Are you sure ...Yes, export againand verify the report is exported again.Case 2: Export reports from multiple workspaces
Approvedreport from Workspace AApprovedreport from Workspace BExport 1/2 reports to QuickBooks Online?Only reports connected to QuickBooks Online will be exported.Exported to QuickBooks Online and ...Case 3: Mixed eligibility across workspaces
Approvedreport from Workspace ADraftreport from Workspace AApprovedreport from Workspace BTitle:
Export 1/3 reports to QuickBooks Online?Message:
Exported to QuickBooks Online and ...Offline tests
N/A.
QA Steps
Same as Test step
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