Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 51 additions & 10 deletions src/libs/actions/MergeTransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,22 @@
}

/**
* Fetches eligible transactions for merging locally
* This is FE version of READ_COMMANDS.GET_TRANSACTIONS_FOR_MERGING API call
* Fetches eligible transactions for merging locally.
* This is the offline FE equivalent of the READ_COMMANDS.GET_TRANSACTIONS_FOR_MERGING API call.
* Per-candidate admin access is checked against each candidate report’s own workspace so that
* cross-workspace admin merges are not granted based on the target’s workspace alone.
* When the candidate’s policy is not in the local cache the candidate is excluded rather than
* assuming access.
*/
function getTransactionsForMergingLocally(transactionID: string, targetTransaction: Transaction, transactions: OnyxCollection<Transaction>, rules: OnyxCollection<Rule>, isAdmin = false) {
function getTransactionsForMergingLocally(
transactionID: string,
targetTransaction: Transaction,
transactions: OnyxCollection<Transaction>,
rules: OnyxCollection<Rule>,
allPolicies: OnyxCollection<Policy>,
currentUserLogin: string | undefined,
allReports?: OnyxCollection<Report>,
) {
const transactionsArray = Object.values(transactions ?? {});

const eligibleTransactions = transactionsArray.filter((transaction): transaction is Transaction => {
Expand All @@ -207,10 +219,30 @@
}

const isUnreportedExpense = !transaction?.reportID || transaction?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID;
if (isUnreportedExpense) {
return areTransactionsEligibleForMerge(targetTransaction, transaction) && !isTransactionPendingDelete(transaction);
}

if (!transaction.reportID) {
return false;
}

// Resolve admin status against the candidate’s own workspace. If the policy is absent from
// the local cache we cannot verify access, so we exclude the candidate rather than assuming
// admin rights.
const candidateReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transaction.reportID}`] ?? getReportOrDraftReport(transaction.reportID);
const candidatePolicyKey = `${ONYXKEYS.COLLECTION.POLICY}${candidateReport?.policyID}`;
const candidatePolicy = candidateReport?.policyID ? (allPolicies?.[candidatePolicyKey] ?? null) : null;
const isCandidatePolicyMissing = !!candidateReport?.policyID && candidatePolicy === null;
if (isCandidatePolicyMissing) {
return false;
}
const isCandidateAdmin = isPolicyAdmin(candidatePolicy, currentUserLogin);

return (
areTransactionsEligibleForMerge(targetTransaction, transaction) &&
!isTransactionPendingDelete(transaction) &&
(isUnreportedExpense || (!!transaction.reportID && isMoneyRequestReportEligibleForMerge(transaction.reportID, isAdmin, rules)))
isMoneyRequestReportEligibleForMerge(candidateReport, isCandidateAdmin, rules)

Check failure on line 245 in src/libs/actions/MergeTransaction.ts

View workflow job for this annotation

GitHub Actions / typecheck

Argument of type 'OnyxEntry<{ avatarUrl?: string | undefined; created?: string | undefined; submitted?: string | undefined; approved?: string | undefined; submitterUserID?: string | undefined; submitterPayrollID?: string | undefined; ... 62 more ...; nextStep?: ReportNextStep | undefined; } & OfflineFeedback<...>>' is not assignable to parameter of type 'string | ({ avatarUrl?: string | undefined; created?: string | undefined; submitted?: string | undefined; approved?: string | undefined; submitterUserID?: string | undefined; submitterPayrollID?: string | undefined; ... 62 more ...; nextStep?: ReportNextStep | undefined; } & OfflineFeedback<...>)'.

Check failure on line 245 in src/libs/actions/MergeTransaction.ts

View workflow job for this annotation

GitHub Actions / typecheck

Argument of type 'OnyxEntry<{ avatarUrl?: string | undefined; created?: string | undefined; submitted?: string | undefined; approved?: string | undefined; submitterUserID?: string | undefined; submitterPayrollID?: string | undefined; ... 62 more ...; nextStep?: ReportNextStep | undefined; } & OfflineFeedback<...>>' is not assignable to parameter of type 'string | ({ avatarUrl?: string | undefined; created?: string | undefined; submitted?: string | undefined; approved?: string | undefined; submitterUserID?: string | undefined; submitterPayrollID?: string | undefined; ... 62 more ...; nextStep?: ReportNextStep | undefined; } & OfflineFeedback<...>)'.
Comment on lines 243 to +245

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep offline merge candidates on the same submitter

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 👍 / 👎.

);
});

Expand All @@ -227,6 +259,8 @@
report,
currentUserLogin,
rules,
allPolicies,
allReports,
}: {
isOffline: boolean;
targetTransaction: Transaction;
Expand All @@ -236,22 +270,29 @@
currentUserLogin: string | undefined;
cardList?: CardList;
rules: OnyxCollection<Rule>;
allPolicies?: OnyxCollection<Policy>;
allReports?: OnyxCollection<Report>;
}) {
const transactionID = targetTransaction.transactionID;
if (!transactionID) {
return;
}

// Collect/Control workspaces:
// - Admins and approvers: The list of eligible expenses will only contain the expenses from the report that the admin/approver triggered the merge from. This is intentionally limited since they’ll only be reviewing one report at a time.
// - Submitters will see all their editable expenses, including their IOUs/unreported expenses
// Candidate discovery:
// - Admins: The list of eligible expenses now includes expenses from the same submitter across all of their draft
// reports where the admin has access. This replaces the previous same-report-only shortcut so admins can initiate
// cross-report merges from either the card or cash expense.
// - Managers (non-admin approvers): Still limited to same-report transactions on the processing report they
// triggered the merge from, as they review one report at a time.
// - Submitters will see all their editable expenses, including their IOUs/unreported expenses.
// IOU:
// - There are no admins/approvers outside of the submitter in these cases, so there’s no consideration for different roles.
// - The submitter, who is also the admin, will see all their editable expenses, including their IOUs/unreported expenses
// - The submitter, who is also the admin, will see all their editable expenses, including their IOUs/unreported expenses.
const isAdmin = isPolicyAdmin(policy, currentUserLogin);
const isManager = isReportManager(report);

if (isPaidGroupPolicy(policy) && (isAdmin || isManager) && !isCurrentUserSubmitter(report)) {
// Managers (non-admin approvers) reviewing a processing report still see only same-report transactions.
if (isPaidGroupPolicy(policy) && isManager && !isAdmin && !isCurrentUserSubmitter(report)) {
const reportTransactions = getReportTransactions(report?.reportID);
const eligibleTransactions = reportTransactions.filter((transaction): transaction is Transaction => {
if (!transaction || transaction.transactionID === transactionID) {
Expand All @@ -268,7 +309,7 @@
}

if (isOffline) {
getTransactionsForMergingLocally(transactionID, targetTransaction, transactions, rules, isAdmin);
getTransactionsForMergingLocally(transactionID, targetTransaction, transactions, rules, allPolicies ?? null, currentUserLogin, allReports);

Check failure on line 312 in src/libs/actions/MergeTransaction.ts

View workflow job for this annotation

GitHub Actions / typecheck

Argument of type 'Record<string, ({ id: string; name: string; role: ValueOf<{ readonly OWNER: "owner"; readonly ADMIN: "admin"; readonly AUDITOR: "auditor"; readonly USER: "user"; readonly EDITOR: "editor"; readonly CARD_ADMIN: "cardAdmin"; readonly PEOPLE_ADMIN: "peopleAdmin"; readonly PAYMENTS_ADMIN: "paymentsAdmin"; }>; ... 105 mo...' is not assignable to parameter of type 'OnyxCollection<{ id: string; name: string; role: ValueOf<{ readonly OWNER: "owner"; readonly ADMIN: "admin"; readonly AUDITOR: "auditor"; readonly USER: "user"; readonly EDITOR: "editor"; readonly CARD_ADMIN: "cardAdmin"; readonly PEOPLE_ADMIN: "peopleAdmin"; readonly PAYMENTS_ADMIN: "paymentsAdmin"; }>; ... 105 mor...'.

Check failure on line 312 in src/libs/actions/MergeTransaction.ts

View workflow job for this annotation

GitHub Actions / typecheck

Argument of type 'Record<string, ({ id: string; name: string; role: ValueOf<{ readonly OWNER: "owner"; readonly ADMIN: "admin"; readonly AUDITOR: "auditor"; readonly USER: "user"; readonly EDITOR: "editor"; readonly CARD_ADMIN: "cardAdmin"; readonly PEOPLE_ADMIN: "peopleAdmin"; readonly PAYMENTS_ADMIN: "paymentsAdmin"; }>; ... 105 mo...' is not assignable to parameter of type 'OnyxCollection<{ id: string; name: string; role: ValueOf<{ readonly OWNER: "owner"; readonly ADMIN: "admin"; readonly AUDITOR: "auditor"; readonly USER: "user"; readonly EDITOR: "editor"; readonly CARD_ADMIN: "cardAdmin"; readonly PEOPLE_ADMIN: "peopleAdmin"; readonly PAYMENTS_ADMIN: "paymentsAdmin"; }>; ... 105 mor...'.
} else {
getTransactionsForMergingFromAPI(transactionID);
}
Expand Down
17 changes: 16 additions & 1 deletion src/pages/TransactionMerge/MergeTransactionsListContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ function MergeTransactionsListContent({transactionID, mergeTransaction}: MergeTr
const currentUserLogin = session?.email;
const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
const [rules] = useOnyx(ONYXKEYS.COLLECTION.RULE);
const [allPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);
const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);
const {isOffline} = useNetwork();
const {convertToDisplayString, getCurrencyDecimals, getCurrencySymbol} = useCurrencyListActions();

Expand All @@ -73,8 +75,21 @@ function MergeTransactionsListContent({transactionID, mergeTransaction}: MergeTr
report: targetTransactionReport,
currentUserLogin,
rules,
allPolicies,
allReports,
});
}, [transactions, isOffline, mergeTransaction?.eligibleTransactions, targetTransactionPolicy, targetTransactionReport, currentUserLogin, targetTransaction, rules]);
}, [
transactions,
isOffline,
mergeTransaction?.eligibleTransactions,
targetTransactionPolicy,
targetTransactionReport,
currentUserLogin,
targetTransaction,
rules,
allPolicies,
allReports,
]);

const data = !eligibleTransactions
? []
Expand Down
Loading
Loading