Repository navigation
feat: add Auto report option for expenses spanning multiple submitters - #99688
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
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.
|
|
@shawnborton @mkhutornyi One of you needs to 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}
![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: 4c1c461bcb
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.movweb-offline.mov |
|
🤖 Code review — the approach is sound, but three things should be settled before merge. Blocking1. A failed 2. Auto report is offered for expenses that are already on reports. The owner check was removed from 3. Auto report skips every guard the other two destinations run. Worth confirming
Nits
Test planThe steps cover the happy path well. Missing: the single-submitter regression (report list + Create report must still render), offline, and per-diem/distance selections. "Offline tests: Same as tests" also contradicts the new offline block — that path now shows a confirm modal and should be written out. I reviewed this statically; I did not run the flow, since it needs a company-card workspace with two cardholders holding unreported expenses. view run · no recording available |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202cbd2071
ℹ️ 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".
| if (selectedTransactionsKeys.length === 0) { | ||
| return; | ||
| } | ||
| autoReportTransactions(selectedTransactionsKeys); |
There was a problem hiding this comment.
When a multi-submitter selection includes expenses from a workspace whose owner is past due, this path sends ChangeTransactionsReport without the shouldRestrictUserBillableActions check used by explicit report selection and report creation. Because Auto report can move expenses into or create draft reports, it either bypasses the billing restriction or is rejected server-side after the RHP closes and the selection is cleared; check every involved policy and route to the restricted-action screen before invoking the write.
Useful? React with 👍 / 👎.
|
🤖 Regression scan — the existing single-submitter paths are provably untouched. Four regressions live in the code paths this PR newly opens up. RegressionsR1 — Expenses already on reports can now be pushed back into drafts. The owner gate was removed unconditionally at R2 — A failed move is now silent and unrecoverable. R3 — Auto report bypasses every pre-move guard. R4 — Verified clean
This is static analysis; I did not run the flow, since it needs a company-card workspace with two cardholders holding unreported expenses. view run · no recording available |
|
Please check AI review comments and address valid ones |
|
Should this also be supported in expense detail header menu? Screen.Recording.2026-09-13.at.5.38.23.PM.mov |
|
Bug: cross-submitter selections now reach the move flow Steps to reproduce
Screen.Recording.2026-09-13.at.5.48.30.PM.movScreen.Recording.2026-09-13.at.5.49.53.PM.mov |
@mkhutornyi that's intentional. |
@mkhutornyi I've updated it: |
|
Are all comments addressed? |
|
@mkhutornyi yes, please review again |
mkhutornyi
left a comment
mkhutornyi
left a comment
There was a problem hiding this comment.
A few product concerns.
Otherwise looks good.
| // Across submitters the only destination the App can offer is "Auto report". Every other mixed-owner selection | ||
| // stays hidden as before, so there is no entry into a screen that could only offer one submitter's reports to | ||
| // everybody else's expenses. Requirements: | ||
| // - every owner resolved, or the count below cannot tell one cardholder's bulk selection from a mixed one | ||
| // - every expense on a managed card, because the backend resolves each destination through the card; one | ||
| // expense without a card fails the whole request with "404 Card not found" | ||
| // - nothing whose validity depends on the destination workspace, which the backend picks: per diem rates and | ||
| // the map/GPS rules on manual and odometer distance can only be checked against a known workspace | ||
| // An expense we cannot read fails all three, so it withholds the flow rather than risking a rejected move. | ||
| const canAutoReportAcrossSubmitters = | ||
| ownerAccountIDs.size > 1 && | ||
| !hasUnknownOwner && | ||
| selectedTransactionsKeys.every((id) => { | ||
| const transaction = selectedTransactions[id]?.transaction ?? allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`]; | ||
| if (!transaction || !isManagedCardTransaction(transaction)) { | ||
| return false; | ||
| } | ||
| return !(isPerDiemRequest(transaction) || isManualDistanceRequest(transaction) || isOdometerDistanceRequest(transaction)); | ||
| }); |
There was a problem hiding this comment.
Auto report reaches expenses on Processing reports - Is this expected?
canAutoReportAcrossSubmitters checks owners, managed cards and per diem/distance, but not report state.
canChangeReport is true for an admin on any outstanding report, including Processing, so submitted company-card expenses from two employees now reach Move to report → Auto report.
On main a mixed-owner selection never reaches the move flow.
- As a workspace admin, have members A and B each submit a report containing one company-card expense (Processing, awaiting first approval).
- Go to Search → Expenses and select both expenses.
- Open the bulk-action menu and tap Move to report.
Should Move to report be offered (with Auto report) or not?
| const failureData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION>> = transactionIDs.map((transactionID) => ({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, | ||
| value: {errors: getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericEditFailureMessage')}, | ||
| })); |
There was a problem hiding this comment.
Auto report failures don't show in Search
failureData writes errors onto each transaction, but getTransactionsSections sets errors: undefined on every Search row (src/libs/SearchUIUtils.ts:2450, :3461), and report-level red brick roads don't read transaction.errors.
- As a workspace admin, go to Search → Expenses and select unreported card expenses from two cardholders.
- Tap Move to report → Auto report.
- Make the
ChangeTransactionsReportrequest fail.
Expected: The affected expenses show an error in the list.
Actual: Nothing appears in Search; the expenses just stay unreported.
| // "Auto report" has the backend resolve each destination through the expense's card, so one expense without a card | ||
| // fails the whole request with "404 Card not found". | ||
| const areAllManagedCardTransactions = selectedTransactionsKeys.length > 0 && transactions.length === selectedTransactionsKeys.length && transactions.every(isManagedCardTransaction); | ||
| const hasMultipleSubmitters = useMemo(() => { |
There was a problem hiding this comment.
One cardholder's selection with unresolved owners gets no Move to report - expected?
This now ignores unresolved owners, but the entry gate at src/hooks/useSearchBulkActions.ts:2717 still counts them as a second owner, so the selection never reaches the fixed screen. Behavior is unchanged from main.
- As a workspace admin, go to Search → Expenses and select two unreported card expenses from one cardholder whose rows have no money-request action.
- Open the bulk-action menu.
Should Move to report be offered (with the cardholder's reports and Create report) or not?
| // Kept separate from `targetOwnerAccountID`, which stops at the first owner it finds. Counting needs them all. | ||
| // Only distinct resolved owners count. An owner we cannot resolve must not stand in for a second submitter: for an | ||
| // unreported expense the report lookup can never resolve one (its reportID is `0`), so a search snapshot missing | ||
| // the money-request action would otherwise file one cardholder's bulk selection as mixed and strip its report list. | ||
| // "Auto report" has the backend resolve each destination through the expense's card, so one expense without a card | ||
| // fails the whole request with "404 Card not found". |
There was a problem hiding this comment.
Please simplify all inline comments which are too long.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #87954 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
🤖 Re-review at BlockingThe
So the merge creates an empty Confirm before merge
Card expenses already on other submitters' reports. The managed-card gate fixes Remaining skipped guards. Excluding per diem / manual distance / odometer is the right call, but a managed-card expense is unlikely to be any of those, so those three checks may be dead while the real gaps stay open: Nits
CI is clean — only I reviewed this statically; I did not run the flow, since it needs a company-card workspace with two cardholders holding unreported card expenses. |
|
🤖 Code quality review at Headline: one eligibility rule is written twice in two files and the two copies disagree, and one of the three new props exists only to re-ask a question the caller already answered. Extracting a single helper fixes both and makes the logic unit-testable. Worth changing1. 2. The eligibility formula is duplicated and the copies aren't equivalent. 3. Owner counting is now written three times. 4. Two different definitions of "multiple owners" sit two lines apart and feed the same 5. Comment style. Em dashes at Verify before actingManual memoization. This PR adds three: Either way, one inconsistency stands on its own: Nits
The comments in this PR do explain why rather than what, which is the harder half and is genuinely well done. The issue is length and placement, not intent. |
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
Product change LGTM 👍
|
🚧 yuwenmemon 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/yuwenmemon in version: 9.4.90-0 🚀
|
|
🤖 Yes — one help site article needed updating. Draft PR: Docs updates for E/A#99688 Auto report for multi-submitter expense moves ( Why: this PR changes what an admin sees in an already-documented flow. Getting-Started-with-the-Spend-Page.md:62-82 tells readers that Move to report offers Create report, an existing report, or Remove from report. For a selection spanning submitters, none of those appear — the panel now shows only Auto report. The article also doesn't explain that Move to report is offered for mixed-submitter selections at all, or why it stays hidden when one expense isn't on a company card. What the docs PR adds to that one article:
I verified the UI labels against the live web app before writing: the Spend tab, the Move to report bulk action, and the Create report row in the right-hand panel titled Report. Auto report and its description come from Articles I checked and left aloneI searched every file under The article already departs from @daledah, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #101767 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.90-2 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Fixed Issues
$ #87954
PROPOSAL:
Tests
Preconditions:
Steps:
Auto reportrow:Create reportrowAuto report.Offline tests
Same as tests
QA Steps
Same as tests
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
MacOS: Chrome / Safari
Screen.Recording.2026-09-02.at.18.21.22.mov