Skip to content
12 changes: 11 additions & 1 deletion src/libs/actions/Search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -804,11 +804,21 @@ function bulkDeleteReports(
const transactionIDList: string[] = [];
const reportIDList: string[] = [];

// Collect all report IDs that are being deleted
for (const key of Object.keys(selectedTransactions)) {
const selectedItem = selectedTransactions[key];
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

First pass: Collect all report IDs that are being deleted
Second pass: Collect transaction IDs, but exclude any transactions whose reportID is in the list of reports being deleted

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.

Can you add comments for the two loops?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added @eh2077

if (selectedItem.action === CONST.SEARCH.ACTION_TYPES.VIEW && key === selectedItem.reportID) {
reportIDList.push(selectedItem.reportID);
} else {
}
}

// Collect transaction IDs, but exclude any transactions whose reportID is in the list of reports being deleted
for (const key of Object.keys(selectedTransactions)) {
const selectedItem = selectedTransactions[key];
if (selectedItem.action === CONST.SEARCH.ACTION_TYPES.VIEW && key === selectedItem.reportID) {
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.

❌ CONSISTENCY-3 (docs)

The condition selectedItem.action === CONST.SEARCH.ACTION_TYPES.VIEW && key === selectedItem.reportID is duplicated across both loops (first used to collect report IDs, then used as a continue guard in the second loop). The iteration over Object.keys(selectedTransactions) and the const selectedItem = selectedTransactions[key] lookup are also duplicated.

Since the two-pass approach is needed (the second loop depends on the completed reportIDList), you can still eliminate the duplication by using a Set of report keys from the first loop, then simply checking membership in the second loop instead of re-evaluating the same condition:

const reportIDSet = new Set<string>();

for (const key of Object.keys(selectedTransactions)) {
    const selectedItem = selectedTransactions[key];
    if (selectedItem.action === CONST.SEARCH.ACTION_TYPES.VIEW && key === selectedItem.reportID) {
        reportIDSet.add(selectedItem.reportID);
    }
}

const reportIDList = Array.from(reportIDSet);

for (const key of Object.keys(selectedTransactions)) {
    if (reportIDSet.has(key)) {
        continue;
    }
    const selectedItem = selectedTransactions[key];
    if (\!selectedItem.reportID || \!reportIDSet.has(selectedItem.reportID)) {
        transactionIDList.push(key);
    }
}

This removes the duplicated condition and the duplicated variable lookup, and as a bonus, using a Set makes the .includes() check O(1) instead of O(n).


Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

continue;
}
if (!selectedItem.reportID || !reportIDList.includes(selectedItem.reportID)) {
transactionIDList.push(key);
}
}
Expand Down
226 changes: 226 additions & 0 deletions tests/unit/Search/deleteSelectedItemsOnSearchTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
import Onyx from 'react-native-onyx';
import type {SelectedTransactionInfo} from '@components/Search/types';
import {bulkDeleteReports} from '@libs/actions/Search';
import {write} from '@libs/API';
import {deleteAppReport} from '@userActions/Report';
import CONST from '@src/CONST';
import {WRITE_COMMANDS} from '@src/libs/API/types';
import ONYXKEYS from '@src/ONYXKEYS';
import {createRandomReport} from '../../utils/collections/reports';

Expand Down Expand Up @@ -298,5 +300,229 @@ describe('bulkDeleteReports', () => {
// Should not call deleteAppReport for transactions
expect(deleteAppReport).not.toHaveBeenCalled();
});

it('should exclude transactions whose reportID is in the list of reports being deleted', () => {
const hash = 12345;
const reports = {
report_123: createRandomReport(123, undefined),
};
const selectedTransactions: Record<string, SelectedTransactionInfo> = {
// This is an empty report (key === reportID), should be deleted
123: {
reportID: '123',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: false,
canChangeReport: false,
isHeld: false,
canUnhold: false,
policyID: 'policy123',
amount: 0,
currency: 'USD',
},
// Transaction belonging to report_123 - should NOT be deleted separately since report is being deleted
transaction_789: {
reportID: '123',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: true,
canChangeReport: true,
isHeld: false,
canUnhold: false,
policyID: 'policy123',
amount: 1000,
currency: 'USD',
},
// Transaction belonging to a different report - should be deleted
transaction_456: {
reportID: '456',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: true,
canChangeReport: true,
isHeld: false,
canUnhold: false,
policyID: 'policy456',
amount: 500,
currency: 'USD',
},
};

const currentUserEmail = '';
const transactions = {};
const transactionsViolations = {};
bulkDeleteReports(reports, undefined, hash, selectedTransactions, currentUserEmail, 1, transactions, transactionsViolations, {});

// Should call deleteAppReport for the report
expect(deleteAppReport).toHaveBeenCalledTimes(1);
expect(deleteAppReport).toHaveBeenCalledWith(reports.report_123, undefined, currentUserEmail, 1, transactions, transactionsViolations, {});

// Should call API.write with DELETE_MONEY_REQUEST_ON_SEARCH only for transaction_456 (not transaction_789 since its report is being deleted)
expect(write).toHaveBeenCalledWith(
WRITE_COMMANDS.DELETE_MONEY_REQUEST_ON_SEARCH,
expect.objectContaining({
hash,
transactionIDList: ['transaction_456'],
}),
expect.any(Object),
);
});

it('should delete all transactions when none belong to reports being deleted', () => {
const hash = 12345;
const selectedTransactions: Record<string, SelectedTransactionInfo> = {
transaction_789: {
reportID: '999',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: true,
canChangeReport: true,
isHeld: false,
canUnhold: false,
policyID: 'policy999',
amount: 1000,
currency: 'USD',
},
transaction_456: {
reportID: '888',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: true,
canChangeReport: true,
isHeld: false,
canUnhold: false,
policyID: 'policy888',
amount: 500,
currency: 'USD',
},
};

bulkDeleteReports(undefined, undefined, hash, selectedTransactions, '', 1, {}, {}, {});

// Should not call deleteAppReport
expect(deleteAppReport).not.toHaveBeenCalled();

// Should call API.write with DELETE_MONEY_REQUEST_ON_SEARCH with all transaction IDs
// eslint-disable-next-line rulesdir/no-multiple-api-calls
expect(write).toHaveBeenCalledWith(
WRITE_COMMANDS.DELETE_MONEY_REQUEST_ON_SEARCH,
expect.objectContaining({
hash,
transactionIDList: expect.arrayContaining(['transaction_789', 'transaction_456']),
}),
expect.any(Object),
);
});

it('should not call deleteMoneyRequestOnSearch when all transactions belong to reports being deleted', () => {
const hash = 12345;
const reports = {
report_123: createRandomReport(123, undefined),
};
const selectedTransactions: Record<string, SelectedTransactionInfo> = {
// This is an empty report (key === reportID), should be deleted
123: {
reportID: '123',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: false,
canChangeReport: false,
isHeld: false,
canUnhold: false,
policyID: 'policy123',
amount: 0,
currency: 'USD',
},
// Transaction belonging to report_123 - should NOT be deleted separately since report is being deleted
transaction_789: {
reportID: '123',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: true,
canChangeReport: true,
isHeld: false,
canUnhold: false,
policyID: 'policy123',
amount: 1000,
currency: 'USD',
},
};

const currentUserEmail = '';
const transactions = {};
const transactionsViolations = {};
bulkDeleteReports(reports, undefined, hash, selectedTransactions, currentUserEmail, 1, transactions, transactionsViolations, {});

// Should call deleteAppReport for the report
expect(deleteAppReport).toHaveBeenCalledTimes(1);
expect(deleteAppReport).toHaveBeenCalledWith(reports.report_123, undefined, currentUserEmail, 1, transactions, transactionsViolations, {});

// Should NOT call API.write with DELETE_MONEY_REQUEST_ON_SEARCH since all transactions belong to the report being deleted
// eslint-disable-next-line rulesdir/no-multiple-api-calls
expect(write).not.toHaveBeenCalledWith(WRITE_COMMANDS.DELETE_MONEY_REQUEST_ON_SEARCH, expect.anything(), expect.anything());
});

it('should handle transactions with no reportID', () => {
const hash = 12345;
const selectedTransactions: Record<string, SelectedTransactionInfo> = {
transaction_789: {
reportID: '',
isFromOneTransactionReport: false,
action: CONST.SEARCH.ACTION_TYPES.VIEW,
isSelected: true,
canSplit: false,
canReject: false,
hasBeenSplit: false,
canHold: true,
canChangeReport: true,
isHeld: false,
canUnhold: false,
policyID: 'policy123',
amount: 1000,
currency: 'USD',
},
};

bulkDeleteReports(undefined, undefined, hash, selectedTransactions, '', 1, {}, {}, {});

// Should call API.write with DELETE_MONEY_REQUEST_ON_SEARCH for transaction with no reportID
// eslint-disable-next-line rulesdir/no-multiple-api-calls
expect(write).toHaveBeenCalledWith(
WRITE_COMMANDS.DELETE_MONEY_REQUEST_ON_SEARCH,
expect.objectContaining({
hash,
transactionIDList: ['transaction_789'],
}),
expect.any(Object),
);
});
});
});
Loading