Skip to content

Filter DELETE-pending transactions out of LHN RBR aggregation #97706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dilshodmackbook-sketch wants to merge 2 commits into
base: main
Choose a base branch
from
+215 −7
Open

Filter DELETE-pending transactions out of LHN RBR aggregation #97706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3efef08
Filter DELETE-pending transactions out of LHN RBR aggregation
dilshodmackbook-sketch Aug 3, 2026
529cdb6
Address review: reuse isTransactionPendingDelete, DRY, reorder filter…
dilshodmackbook-sketch Aug 4, 2026
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
15 changes: 8 additions & 7 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ import {
isReceiptBeingScanned,
isScanning,
isScanRequest as isScanRequestTransactionUtils,
isTransactionPendingDelete,
} from './TransactionUtils';
import addTrailingForwardSlash from './UrlUtils';
import {getDefaultAvatarURL} from './UserAvatarUtils';
Expand Down Expand Up @@ -9280,7 +9281,7 @@ function getPolicyIDsWithEmptyReportsForAccount(
}

// Ignore transactions that are already pending deletion so we treat the report as empty once the removal is queued.
const transactions = (reportsTransactionsParam[report.reportID] ?? []).filter((transaction) => transaction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE);
const transactions = (reportsTransactionsParam[report.reportID] ?? []).filter((transaction) => !isTransactionPendingDelete(transaction));
if (transactions.length === 0) {
policyLookup[report.policyID] = true;
}
Expand Down Expand Up @@ -9450,14 +9451,16 @@ function getViolatingReportIDForRBRInLHN(report: OnyxEntry<Report>, transactionV
if (!potentialReport) {
return false;
}
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${potentialReport.policyID}`];
const transactions = getReportTransactions(potentialReport.reportID);

// Allow both open and processing reports to show RBR for violations
if (!isOpenOrProcessingReport(potentialReport)) {
return false;
}

const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${potentialReport.policyID}`];
// Ignore transactions that are already pending deletion (e.g. a reverted split child) so the LHN RBR stays
// consistent with what the opened report renders, which also filters out DELETE-pending transactions.
const transactions = getReportTransactions(potentialReport.reportID).filter((transaction) => !isTransactionPendingDelete(transaction));

const excludedNoticeNamesForLHN = isProcessingReport(potentialReport) ? [CONST.VIOLATIONS.MODIFIED_AMOUNT] : [];

return (
Expand Down Expand Up @@ -13012,9 +13015,7 @@ function hasExportError(reportActions: OnyxEntry<ReportActions> | ReportAction[]
}

function doesReportContainRequestsFromMultipleUsers(iouReport: OnyxEntry<Report>, shouldExcludeDeletedTransactions = false): boolean {
const transactions = getReportTransactions(iouReport?.reportID).filter(
(transaction) => !shouldExcludeDeletedTransactions || transaction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
);
const transactions = getReportTransactions(iouReport?.reportID).filter((transaction) => !shouldExcludeDeletedTransactions || !isTransactionPendingDelete(transaction));

return isIOUReport(iouReport) && transactions.some((transaction) => (Number(transaction?.modifiedAmount) || transaction?.amount) <= 0);
}
Expand Down
207 changes: 207 additions & 0 deletions tests/unit/ReportUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15383,6 +15383,213 @@ describe('ReportUtils', () => {
await Onyx.clear();
});

it('should return null when the only violating transaction is pending deletion (e.g. a reverted split child)', async () => {
await Onyx.clear();

const policyID = 'policy-rbr-deleting';
const chatReportID = 'chat-rbr-deleting';
const expenseReportID = 'expense-rbr-deleting';
const transactionID = 'transaction-rbr-deleting';

const policyData: Policy = {
id: policyID,
name: 'RBR Pending Delete Test Workspace',
type: CONST.POLICY.TYPE.TEAM,
role: CONST.POLICY.ROLE.ADMIN,
outputCurrency: CONST.CURRENCY.USD,
reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES,
approvalMode: CONST.POLICY.APPROVAL_MODE.BASIC,
employeeList: {
[currentUserEmail]: {
role: CONST.POLICY.ROLE.ADMIN,
},
},
owner: currentUserEmail,
isPolicyExpenseChatEnabled: true,
};

const chatReport: Report = {
...createPolicyExpenseChat(807),
reportID: chatReportID,
ownerAccountID: currentUserAccountID,
policyID,
iouReportID: expenseReportID,
hasOutstandingChildRequest: true,
};

const expenseReport: Report = {
...createExpenseReport(808),
reportID: expenseReportID,
chatReportID,
ownerAccountID: currentUserAccountID,
managerID: 42,
policyID,
type: CONST.REPORT.TYPE.EXPENSE,
currency: CONST.CURRENCY.USD,
total: 5000,
stateNum: CONST.REPORT.STATE_NUM.OPEN,
statusNum: CONST.REPORT.STATUS_NUM.OPEN,
};

const baseTransaction = createRandomTransaction(807);
// The reverted split child stays in Onyx marked for deletion until the server confirms.
const transaction: Transaction = {
...baseTransaction,
transactionID,
reportID: expenseReportID,
amount: 5000,
currency: CONST.CURRENCY.USD,
status: CONST.TRANSACTION.STATUS.POSTED,
reimbursable: true,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
};

const transactionViolationsKey = `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}` as OnyxKey;
const transactionViolationsCollection: OnyxCollection<TransactionViolation[]> = {
[transactionViolationsKey]: [
{
name: CONST.VIOLATIONS.MISSING_CATEGORY,
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
},
],
};

await Onyx.merge(ONYXKEYS.SESSION, {accountID: currentUserAccountID, email: currentUserEmail});
await waitForBatchedUpdates();

await Promise.all([
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, policyData),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, expenseReport),
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction),
Onyx.merge(transactionViolationsKey, transactionViolationsCollection[transactionViolationsKey]),
]);
await waitForBatchedUpdates();

const result = getViolatingReportIDForRBRInLHN(chatReport, transactionViolationsCollection);
expect(result).toBeNull();

@marufsharifi marufsharifi Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a positive control so this test can't pass vacuously.

Right now the test only asserts the DELETE-pending case returns null. That doesn't prove the filter is what produces the null — if any unrelated gate in getViolatingReportIDForRBRInLHN (isCurrentUserSubmitter, isOpenOrProcessingReport, or shouldShowViolation rejecting MISSING_CATEGORY) were the real reason, the test would still pass while testing nothing, and it would keep passing even if the fix were reverted in a way that broke a different gate.

Please assert that the same setup lights the RBR when the transaction is not pending deletion. Cheapest version — flip pendingAction and re-assert right after line 15471:

// Positive control: the same violating transaction lights the RBR when it isn't pending deletion.
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {pendingAction: null});
await waitForBatchedUpdates();
expect(getViolatingReportIDForRBRInLHN(chatReport, transactionViolationsCollection)).toBe(expenseReportID);

This guarantees the test actually exercises the new filter (and would fail on main), instead of relying on QA step 6 to cover the positive case.


// Positive control: the same violating transaction lights the RBR when it isn't pending deletion.
// This proves the null above comes from the DELETE filter and not from some unrelated gate.
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {pendingAction: null});
await waitForBatchedUpdates();
expect(getViolatingReportIDForRBRInLHN(chatReport, transactionViolationsCollection)).toBe(expenseReportID);

await Onyx.clear();
});

it('should still surface RBR when a report has a live violating transaction alongside a DELETE-pending one', async () => {
await Onyx.clear();

const policyID = 'policy-rbr-mixed';
const chatReportID = 'chat-rbr-mixed';
const expenseReportID = 'expense-rbr-mixed';
const deletedTransactionID = 'transaction-rbr-mixed-deleted';
const liveTransactionID = 'transaction-rbr-mixed-live';

const policyData: Policy = {
id: policyID,
name: 'RBR Mixed Transactions Test Workspace',
type: CONST.POLICY.TYPE.TEAM,
role: CONST.POLICY.ROLE.ADMIN,
outputCurrency: CONST.CURRENCY.USD,
reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES,
approvalMode: CONST.POLICY.APPROVAL_MODE.BASIC,
employeeList: {
[currentUserEmail]: {
role: CONST.POLICY.ROLE.ADMIN,
},
},
owner: currentUserEmail,
isPolicyExpenseChatEnabled: true,
};

const chatReport: Report = {
...createPolicyExpenseChat(807),
reportID: chatReportID,
ownerAccountID: currentUserAccountID,
policyID,
iouReportID: expenseReportID,
hasOutstandingChildRequest: true,
};

const expenseReport: Report = {
...createExpenseReport(808),
reportID: expenseReportID,
chatReportID,
ownerAccountID: currentUserAccountID,
managerID: 42,
policyID,
type: CONST.REPORT.TYPE.EXPENSE,
currency: CONST.CURRENCY.USD,
total: 10000,
stateNum: CONST.REPORT.STATE_NUM.OPEN,
statusNum: CONST.REPORT.STATUS_NUM.OPEN,
};

// Two violating transactions on the SAME expense report: a reverted split child (DELETE-pending)
// and one that is still live. The report's RBR must survive because the live child still violates.
const deletedTransaction: Transaction = {
...createRandomTransaction(809),
transactionID: deletedTransactionID,
reportID: expenseReportID,
amount: 5000,
currency: CONST.CURRENCY.USD,
status: CONST.TRANSACTION.STATUS.POSTED,
reimbursable: true,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
};
const liveTransaction: Transaction = {
...createRandomTransaction(810),
transactionID: liveTransactionID,
reportID: expenseReportID,
amount: 5000,
currency: CONST.CURRENCY.USD,
status: CONST.TRANSACTION.STATUS.POSTED,
reimbursable: true,
pendingAction: null,
};

const deletedViolationsKey = `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${deletedTransactionID}` as OnyxKey;
const liveViolationsKey = `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${liveTransactionID}` as OnyxKey;
const transactionViolationsCollection: OnyxCollection<TransactionViolation[]> = {
[deletedViolationsKey]: [
{
name: CONST.VIOLATIONS.MISSING_CATEGORY,
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
},
],
[liveViolationsKey]: [
{
name: CONST.VIOLATIONS.MISSING_CATEGORY,
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
},
],
};

await Onyx.merge(ONYXKEYS.SESSION, {accountID: currentUserAccountID, email: currentUserEmail});
await waitForBatchedUpdates();

await Promise.all([
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, policyData),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, expenseReport),
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${deletedTransaction.transactionID}`, deletedTransaction),
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${liveTransaction.transactionID}`, liveTransaction),
Onyx.merge(deletedViolationsKey, transactionViolationsCollection[deletedViolationsKey]),
Onyx.merge(liveViolationsKey, transactionViolationsCollection[liveViolationsKey]),
]);
await waitForBatchedUpdates();

const result = getViolatingReportIDForRBRInLHN(chatReport, transactionViolationsCollection);
expect(result).toBe(expenseReportID);

await Onyx.clear();
});

@marufsharifi marufsharifi Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing test: mixed DELETE-pending + live transaction on the same report.

Both this new test and the existing positive test use a single transaction, so nothing proves the filter is per-transaction (drops only the reverted child) rather than "if any transaction is DELETE-pending, drop the whole report's RBR." That's the exact split-report shape this fix targets.

Please add, right after this test:

it('should still surface RBR when a report has a live violating transaction alongside a DELETE-pending one', async () => {
    // ...same policy / chatReport / expenseReport setup as the pending-delete test...

    // Two violating transactions on the SAME expense report:
    // one reverted split child (DELETE-pending) and one still live.
    const deletedChild: Transaction = {
        ...createRandomTransaction(809),
        transactionID: 'transaction-rbr-mixed-deleted',
        reportID: expenseReportID,
        amount: 5000,
        currency: CONST.CURRENCY.USD,
        status: CONST.TRANSACTION.STATUS.POSTED,
        reimbursable: true,
        pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
    };
    const liveChild: Transaction = {
        ...createRandomTransaction(810),
        transactionID: 'transaction-rbr-mixed-live',
        reportID: expenseReportID,
        amount: 5000,
        currency: CONST.CURRENCY.USD,
        status: CONST.TRANSACTION.STATUS.POSTED,
        reimbursable: true,
        // no pendingAction
    };

    // Give BOTH transactions a MISSING_CATEGORY violation, then merge policy/reports/
    // transactions/violations into Onyx as in the tests above.

    const result = getViolatingReportIDForRBRInLHN(chatReport, transactionViolationsCollection);
    expect(result).toBe(expenseReportID); // RBR still shown because the live child violates

    await Onyx.clear();
});

Verdict: Covered 2/3 changed behaviors — the mixed-transaction case is the one gap, and it's the realistic split-report scenario the fix is about.


it('should return null when all expense reports in the policy are closed', async () => {
await Onyx.clear();

Expand Down
Loading