Repository navigation
[HOLD] Allow admins to merge card + cash expenses across draft reports - #101028
youssef-lr wants to merge 3 commits into
Conversation
Remove the same-report shortcut for admins in getTransactionsForMerging. Admins now discover cross-report and cross-workspace candidates via the API (online) or the local filter (offline). Only non-admin managers reviewing a processing report keep the same-report restriction. The offline filter (getTransactionsForMergingLocally) now resolves admin access per-candidate against that candidate's own workspace policy. Candidates are excluded when their workspace policy is absent from the local cache rather than assumed accessible. MergeTransactionsListContent subscribes to allPolicies and allReports and forwards them for the offline per-candidate check. Tests: 5 new cases covering admin cross-report (same workspace), non-admin workspace rejection, missing-cache exclusion, manager same-report restriction, and admin-as-submitter cross-report.
|
@madmax330 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: 354bee55b8
ℹ️ 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".
| areTransactionsEligibleForMerge(targetTransaction, transaction) && | ||
| !isTransactionPendingDelete(transaction) && | ||
| (isUnreportedExpense || (!!transaction.reportID && isMoneyRequestReportEligibleForMerge(transaction.reportID, isAdmin, rules))) | ||
| isMoneyRequestReportEligibleForMerge(candidateReport, isCandidateAdmin, rules) |
There was a problem hiding this comment.
When an offline admin has cached open reports for multiple employees, this filter accepts any candidate whose workspace grants admin access because neither areTransactionsEligibleForMerge() nor isMoneyRequestReportEligibleForMerge() compares the candidate report owner with the target report owner. This exposes expenses belonging to different employees as merge choices, despite isMergeActionForSelectedTransactions() explicitly rejecting that case in ReportSecondaryActionUtils.ts:862-885, and can initiate a destructive cross-user merge. Pass the target report into this filter and require matching ownerAccountID values (with the existing current-user rule for reported/unreported pairs).
Useful? React with 👍 / 👎.
…-manual-merge-expenses-drafts
…-manual-merge-expenses-drafts
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Moved to #102438 |
1 participant
Explanation of Change
Removes the admin same-report-only shortcut in
getTransactionsForMerging. Admins can now discover merge candidates across different draft reports and workspaces, not just the report they triggered merge from. Non-admin managers reviewing a processing report keep the same-report restriction unchanged.The offline filter now checks admin access per-candidate against that candidate's own workspace policy. If the policy is not in the local cache the candidate is excluded rather than assumed accessible.
MergeTransactionsListContentsubscribes toallPoliciesandallReportsand forwards them for the offline per-candidate check.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/678389
PROPOSAL:
Tests
Offline tests
QA Steps
As a workspace admin, open a card expense on a draft report, tap Merge — verify cash expenses from other draft reports of the same submitter (same workspace) appear.
As a workspace admin, select a card expense and a cash expense from different draft reports of the same submitter in Search, verify Merge is available.
As a non-admin approver, open a card expense on a submitted (processing) report, tap Merge — verify only other expenses on that same report appear.
As an admin who is also the submitter, tap Merge — verify all own eligible expenses appear (cross-report).
Verify that no errors appear in the JS console
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