From 5216646bdab14c6ba3e1f12c61f1d4a4fed57da8 Mon Sep 17 00:00:00 2001 From: thelullabyy <182625428+thelullabyy@users.noreply.github.com> Date: Thu, 9 Jul 2026 20:53:57 -0500 Subject: [PATCH 1/3] Refactor delete action logic to include current user account ID in relevant functions --- src/hooks/useSearchBulkActions.ts | 2 +- src/libs/ReportSecondaryActionUtils.ts | 18 +++++++++++------- src/libs/ReportUtils.ts | 10 ++++++---- src/libs/SearchUIUtils.ts | 5 +++-- .../report/ContextMenu/ContextMenuActions.tsx | 16 ++++++++++++++-- tests/perf-test/ReportUtils.perf-test.ts | 2 +- tests/unit/ReportUtilsTest.ts | 12 ++++++------ tests/unit/Search/SearchUIUtilsTest.ts | 4 ++-- 8 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/hooks/useSearchBulkActions.ts b/src/hooks/useSearchBulkActions.ts index 812260daf452..c643150cbf75 100644 --- a/src/hooks/useSearchBulkActions.ts +++ b/src/hooks/useSearchBulkActions.ts @@ -2063,7 +2063,7 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { const firstTransactionKey = selectedTransactionsKeys.at(0); const firstTransactionMeta = firstTransactionKey ? selectedTransactions[firstTransactionKey] : undefined; - const canShowDeleteAction = shouldShowDeleteOption(selectedTransactions, currentSearchResults?.data, selectedReports, queryJSON?.type); + const canShowDeleteAction = shouldShowDeleteOption(selectedTransactions, currentSearchResults?.data, accountID, selectedReports, queryJSON?.type); const isSplittable = !!firstTransactionMeta?.canSplit; const isAlreadySplit = !!firstTransactionMeta?.hasBeenSplit; diff --git a/src/libs/ReportSecondaryActionUtils.ts b/src/libs/ReportSecondaryActionUtils.ts index fb56c2dfa493..7ec6181a8de8 100644 --- a/src/libs/ReportSecondaryActionUtils.ts +++ b/src/libs/ReportSecondaryActionUtils.ts @@ -676,8 +676,8 @@ function isChangeWorkspaceAction(report: Report, policies: OnyxCollection, isProduction: boolean, + currentUserAccountID: number, ): boolean { if (reportTransactions.length !== 1) { return false; @@ -697,7 +698,10 @@ function shouldShowEditSplitInDeleteAction( } const isSelfDMSplit = isSelfDMReportUtils(report); - return shouldRedirectDeleteToSplitExpenseEdit(reportTransaction, originalTransaction, isSelfDMSplit, isProduction) && isDeleteAction(report, reportTransactions, reportActions); + return ( + shouldRedirectDeleteToSplitExpenseEdit(reportTransaction, originalTransaction, isSelfDMSplit, isProduction) && + isDeleteAction(report, reportTransactions, currentUserAccountID, reportActions) + ); } function isRetractAction(report: Report, policy?: Policy): boolean { @@ -1048,7 +1052,7 @@ function getSecondaryReportActions({ if ( isSplitAction(report, reportTransactions, originalTransaction, currentUserLogin, currentUserAccountID, policy, parentReport, isProduction) && - !shouldShowEditSplitInDeleteAction(report, reportTransactions, reportActions, originalTransaction, isProduction) + !shouldShowEditSplitInDeleteAction(report, reportTransactions, reportActions, originalTransaction, isProduction, currentUserAccountID) ) { options.push(CONST.REPORT.SECONDARY_ACTIONS.SPLIT); } @@ -1103,7 +1107,7 @@ function getSecondaryReportActions({ options.push(CONST.REPORT.SECONDARY_ACTIONS.VIEW_DETAILS); - if (isDeleteAction(report, reportTransactions, reportActions ?? [])) { + if (isDeleteAction(report, reportTransactions, currentUserAccountID, reportActions ?? [])) { options.push(CONST.REPORT.SECONDARY_ACTIONS.DELETE); } @@ -1180,7 +1184,7 @@ function getSecondaryTransactionThreadActions({ if ( isSplitAction(parentReport, [reportTransaction], originalTransaction, currentUserLogin, currentUserAccountID, policy, grandParentReport, isProduction) && - !shouldShowEditSplitInDeleteAction(parentReport, [reportTransaction], reportAction ? [reportAction] : [], originalTransaction, isProduction) + !shouldShowEditSplitInDeleteAction(parentReport, [reportTransaction], reportAction ? [reportAction] : [], originalTransaction, isProduction, currentUserAccountID) ) { options.push(CONST.REPORT.TRANSACTION_SECONDARY_ACTIONS.SPLIT); } @@ -1210,7 +1214,7 @@ function getSecondaryTransactionThreadActions({ options.push(CONST.REPORT.TRANSACTION_SECONDARY_ACTIONS.VIEW_DETAILS); - if (isDeleteAction(parentReport, [reportTransaction], reportAction ? [reportAction] : [])) { + if (isDeleteAction(parentReport, [reportTransaction], currentUserAccountID, reportAction ? [reportAction] : [])) { options.push(CONST.REPORT.TRANSACTION_SECONDARY_ACTIONS.DELETE); } diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index a26024a8a525..f010157237ca 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -3184,10 +3184,10 @@ function canDeleteCardTransactionByLiabilityType(transaction: OnyxEntry, reportTransactions: Transaction[], reportActions: ReportAction[]): boolean { +function canDeleteMoneyRequestReport(report: OnyxEntry, reportTransactions: Transaction[], reportActions: ReportAction[], currentUserAccountID: number): boolean { const transaction = reportTransactions.at(0); const transactionID = transaction?.transactionID; - const isOwner = transactionID ? getIOUActionForTransactionID(reportActions, transactionID)?.actorAccountID === deprecatedCurrentUserAccountID : false; + const isOwner = transactionID ? getIOUActionForTransactionID(reportActions, transactionID)?.actorAccountID === currentUserAccountID : false; const isReportOpenOrProcessing = isOpenReport(report) || isProcessingReport(report); const isSingleTransaction = reportTransactions.length === 1; @@ -3202,7 +3202,7 @@ function canDeleteMoneyRequestReport(report: OnyxEntry, reportTransactio } if (isInvoiceReport(report)) { - return report?.ownerAccountID === deprecatedCurrentUserAccountID && isReportOpenOrProcessing; + return report?.ownerAccountID === currentUserAccountID && isReportOpenOrProcessing; } // Users cannot delete a report in the unreported or IOU cases, but they can delete individual transactions. @@ -3216,7 +3216,7 @@ function canDeleteMoneyRequestReport(report: OnyxEntry, reportTransactio return false; } - const isReportSubmitter = isCurrentUserSubmitter(report); + const isReportSubmitter = isCurrentUserSubmitter(report, currentUserAccountID); return isReportSubmitter && (isOpenReport(report) || (isProcessingReport(report) && isAwaitingFirstLevelApproval(report))); } @@ -3233,6 +3233,7 @@ function canDeleteReportAction( transaction: OnyxEntry | undefined, transactions: OnyxCollection, childReportActions: OnyxCollection, + currentUserAccountID: number, ): boolean { const report = getReportOrDraftReport(reportID); const isActionOwner = reportAction?.actorAccountID === deprecatedCurrentUserAccountID; @@ -3267,6 +3268,7 @@ function canDeleteReportAction( report, Object.values(transactions ?? {}).filter((t): t is Transaction => !!t), Object.values(childReportActions ?? {}).filter((action): action is ReportAction => !!action), + currentUserAccountID, ); } diff --git a/src/libs/SearchUIUtils.ts b/src/libs/SearchUIUtils.ts index d00c4e63e6ec..adeab9b8b5de 100644 --- a/src/libs/SearchUIUtils.ts +++ b/src/libs/SearchUIUtils.ts @@ -6353,6 +6353,7 @@ function navigateToSearchRHP(route: {route: string; getRoute: (backTo?: string) function shouldShowDeleteOption( selectedTransactions: Record, currentSearchResults: SearchResults['data'] | undefined, + currentUserAccountID: number, selectedReports: SelectedReports[] = [], searchDataType?: SearchDataTypes, ) { @@ -6377,7 +6378,7 @@ function shouldShowDeleteOption( reportTransactions.push(item); } } - return canDeleteMoneyRequestReport(fullReport, reportTransactions, reportActionsArray); + return canDeleteMoneyRequestReport(fullReport, reportTransactions, reportActionsArray, currentUserAccountID); }) : selectedTransactionsKeys.every((id) => { const transaction = currentSearchResults?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`] ?? selectedTransactions[id]?.transaction; @@ -6391,7 +6392,7 @@ function shouldShowDeleteOption( Object.values(reportActions ?? {}).find((action) => (isMoneyRequestAction(action) ? getOriginalMessage(action)?.IOUTransactionID : undefined) === id) ?? selectedTransactions[id].reportAction; - return canDeleteMoneyRequestReport(parentReport, [transaction], parentReportAction ? [parentReportAction] : []); + return canDeleteMoneyRequestReport(parentReport, [transaction], parentReportAction ? [parentReportAction] : [], currentUserAccountID); }); } diff --git a/src/pages/inbox/report/ContextMenu/ContextMenuActions.tsx b/src/pages/inbox/report/ContextMenu/ContextMenuActions.tsx index 6ca9df4815b4..c7cbcfcd0cc1 100644 --- a/src/pages/inbox/report/ContextMenu/ContextMenuActions.tsx +++ b/src/pages/inbox/report/ContextMenu/ContextMenuActions.tsx @@ -1510,7 +1510,19 @@ const ContextMenuActions: ContextMenuAction[] = [ isAnonymousAction: false, textTranslateKey: 'common.delete', icon: 'Trashcan', - shouldShow: ({type, reportAction, isArchivedRoom, isChronosReport, reportID: reportIDParam, moneyRequestAction, iouTransaction, transactions, childReportActions, isProduction}) => { + shouldShow: ({ + type, + reportAction, + isArchivedRoom, + isChronosReport, + reportID: reportIDParam, + moneyRequestAction, + iouTransaction, + transactions, + childReportActions, + isProduction, + currentUserAccountID, + }) => { // Until deleting parent threads is supported in FE, we will prevent the user from deleting a thread parent let reportID = reportIDParam; @@ -1535,7 +1547,7 @@ const ContextMenuActions: ContextMenuAction[] = [ return ( !!reportIDParam && type === CONST.CONTEXT_MENU_TYPES.REPORT_ACTION && - canDeleteReportAction(moneyRequestAction ?? reportAction, reportID, iouTransaction, transactions, childReportActions) && + canDeleteReportAction(moneyRequestAction ?? reportAction, reportID, iouTransaction, transactions, childReportActions, currentUserAccountID) && !isArchivedRoom && !isChronosReport && !isMessageDeleted(reportAction) diff --git a/tests/perf-test/ReportUtils.perf-test.ts b/tests/perf-test/ReportUtils.perf-test.ts index 20d26430dc2f..87afd9d9b506 100644 --- a/tests/perf-test/ReportUtils.perf-test.ts +++ b/tests/perf-test/ReportUtils.perf-test.ts @@ -104,7 +104,7 @@ describe('ReportUtils', () => { const reportAction = {...createRandomReportAction(1), actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT} as unknown as ReportAction; await waitForBatchedUpdates(); - await measureFunction(() => canDeleteReportAction(reportAction, reportID, transaction, undefined, undefined)); + await measureFunction(() => canDeleteReportAction(reportAction, reportID, transaction, undefined, undefined, 1)); }); test('[ReportUtils] getReportRecipientAccountID on 1k participants', async () => { diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 22b715f09ff8..ce7dc6b00c56 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -5313,7 +5313,7 @@ describe('ReportUtils', () => { }, }); }); - expect(canDeleteMoneyRequestReport(invoiceReport, [], [])).toBe(true); + expect(canDeleteMoneyRequestReport(invoiceReport, [], [], currentUserAccountID)).toBe(true); }); it('should allow deletion if the expense report is submitted but not yet approved by anyone', async () => { @@ -5352,7 +5352,7 @@ describe('ReportUtils', () => { }); }); - expect(canDeleteMoneyRequestReport(expenseReport, [], [])).toBe(true); + expect(canDeleteMoneyRequestReport(expenseReport, [], [], currentUserAccountID)).toBe(true); }); }); @@ -7911,7 +7911,7 @@ describe('ReportUtils', () => { }, }; - expect(canDeleteReportAction(moneyRequestAction, currentReportId, transaction, undefined, undefined)).toBe(false); + expect(canDeleteReportAction(moneyRequestAction, currentReportId, transaction, undefined, undefined, currentUserAccountID)).toBe(false); }); it('should return true for demo transaction', () => { @@ -7955,7 +7955,7 @@ describe('ReportUtils', () => { }, }; - expect(canDeleteReportAction(moneyRequestAction, '1', transaction, undefined, undefined)).toBe(true); + expect(canDeleteReportAction(moneyRequestAction, '1', transaction, undefined, undefined, currentUserAccountID)).toBe(true); }); it('should return false for unreported card expense imported with deleting disabled', async () => { @@ -8001,7 +8001,7 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${selfDMReport.reportID}`, selfDMReport); // Then it should return false since the unreported card expense is imported with deleting disabled - expect(canDeleteReportAction(trackExpenseAction, selfDMReport.reportID, transaction, undefined, undefined)).toBe(false); + expect(canDeleteReportAction(trackExpenseAction, selfDMReport.reportID, transaction, undefined, undefined, currentUserAccountID)).toBe(false); }); it("should return false for ADD_COMMENT report action the current user (admin of the personal policy) didn't comment", async () => { @@ -8028,7 +8028,7 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${adminPolicy.id}`, adminPolicy); - expect(canDeleteReportAction(reportAction, report.reportID, undefined, undefined, undefined)).toBe(false); + expect(canDeleteReportAction(reportAction, report.reportID, undefined, undefined, undefined, currentUserAccountID)).toBe(false); }); }); diff --git a/tests/unit/Search/SearchUIUtilsTest.ts b/tests/unit/Search/SearchUIUtilsTest.ts index 1de1532d9fc9..ce6f03339b7a 100644 --- a/tests/unit/Search/SearchUIUtilsTest.ts +++ b/tests/unit/Search/SearchUIUtilsTest.ts @@ -10207,7 +10207,7 @@ describe('SearchUIUtils', () => { await Onyx.merge(ONYXKEYS.SESSION, {accountID: TEST_ACCOUNT_ID}); - expect(SearchUIUtils.shouldShowDeleteOption(selectedTransactions, currentSearchResults)).toBe(true); + expect(SearchUIUtils.shouldShowDeleteOption(selectedTransactions, currentSearchResults, TEST_ACCOUNT_ID)).toBe(true); }); it('should show delete option for unreported expense which can be deleted', async () => { @@ -10467,7 +10467,7 @@ describe('SearchUIUtils', () => { await Onyx.merge(ONYXKEYS.SESSION, {accountID: TEST_ACCOUNT_ID}); - expect(SearchUIUtils.shouldShowDeleteOption(selectedTransactions, currentSearchResults)).toBe(true); + expect(SearchUIUtils.shouldShowDeleteOption(selectedTransactions, currentSearchResults, TEST_ACCOUNT_ID)).toBe(true); }); }); describe('getToFieldValueForTransaction', () => { From 4972db5495a47f4823732ebe9185e4d36c979e6b Mon Sep 17 00:00:00 2001 From: thelullabyy <182625428+thelullabyy@users.noreply.github.com> Date: Mon, 13 Jul 2026 14:56:44 -0500 Subject: [PATCH 2/3] fix: codex --- src/libs/ReportUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index d6bbb85415e2..2876a2d644ba 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -3257,7 +3257,7 @@ function canDeleteReportAction( currentUserAccountID: number, ): boolean { const report = getReportOrDraftReport(reportID); - const isActionOwner = reportAction?.actorAccountID === deprecatedCurrentUserAccountID; + const isActionOwner = reportAction?.actorAccountID === currentUserAccountID; const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`] ?? null; if (isDemoTransaction(transaction)) { From 77ab89d413298c1029caf341ea5a2027f9699e7a Mon Sep 17 00:00:00 2001 From: thelullabyy <182625428+thelullabyy@users.noreply.github.com> Date: Mon, 13 Jul 2026 15:19:19 -0500 Subject: [PATCH 3/3] fix: test --- tests/unit/ReportUtilsTest.ts | 51 +++++++++++++++++------------------ 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 7282e43d4b4d..015068169279 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -7951,35 +7951,21 @@ describe('ReportUtils', () => { }); describe('canDeleteReportAction', () => { - it('should return false for delete button visibility if transaction is not allowed to be deleted', () => { - const parentReport = LHNTestUtils.getFakeReport(); - const report = LHNTestUtils.getFakeReport(); - const parentReportAction: ReportAction = { - ...LHNTestUtils.getFakeReportAction(), - message: [ - { - type: 'COMMENT', - html: 'hey', - text: 'hey', - isEdited: false, - whisperedTo: [], - isDeletedParentAction: false, - moderationDecision: { - decision: CONST.MODERATION.MODERATOR_DECISION_PENDING_REMOVE, - }, - }, - ], - childReportID: report.reportID, + it('should return false for delete button visibility if transaction is not allowed to be deleted', async () => { + // Given a restricted managed-card expense on an open expense report owned by the current user + const expenseReport = { + ...LHNTestUtils.getFakeReport(), + type: CONST.REPORT.TYPE.EXPENSE, + stateNum: CONST.REPORT.STATE_NUM.OPEN, + statusNum: CONST.REPORT.STATUS_NUM.OPEN, + ownerAccountID: currentUserAccountID, }; - report.parentReportID = parentReport.reportID; - report.parentReportActionID = parentReportAction.reportActionID; - const currentReportId = ''; const transactionID = 1; const moneyRequestAction = { - ...parentReportAction, + ...LHNTestUtils.getFakeReportAction(), actorAccountID: currentUserAccountID, actionName: CONST.REPORT.ACTIONS.TYPE.IOU, - reportID: '1', + reportID: expenseReport.reportID, originalMessage: { IOUTransactionID: '1', amount: 100, @@ -7988,6 +7974,16 @@ describe('ReportUtils', () => { type: CONST.IOU.REPORT_ACTION_TYPE.PAY, paymentType: CONST.IOU.PAYMENT_TYPE.EXPENSIFY, }, + message: [ + { + type: 'COMMENT', + html: 'hey', + text: 'hey', + isEdited: false, + whisperedTo: [], + isDeletedParentAction: false, + }, + ], }; const transaction: Transaction = { @@ -7995,14 +7991,17 @@ describe('ReportUtils', () => { category: '', tag: '', created: testDate, - reportID: currentReportId, + reportID: expenseReport.reportID, managedCard: true, comment: { liabilityType: CONST.TRANSACTION.LIABILITY_TYPE.RESTRICT, }, }; - expect(canDeleteReportAction(moneyRequestAction, currentReportId, transaction, undefined, undefined, currentUserAccountID)).toBe(false); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, expenseReport); + + // Then the owner cannot delete it because the card transaction's liability type restricts deletion + expect(canDeleteReportAction(moneyRequestAction, expenseReport.reportID, transaction, undefined, undefined, currentUserAccountID)).toBe(false); }); it('should return true for demo transaction', () => {