Skip to content

fix: Delete option is missing in More menu of an unpaid invoice report #73914

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

Merged
cristipaval merged 3 commits into from
Nov 4, 2025
Merged

fix: Delete option is missing in More menu of an unpaid invoice report #73914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79e2971
show delete button when invoice is open
etCoderDysto Oct 31, 2025
da8a356
add a test for canDeleteMoneyRequestReport
etCoderDysto Oct 31, 2025
4c913e1
make comment clear
etCoderDysto Oct 31, 2025
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
2 changes: 1 addition & 1 deletion src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,7 @@
const parsedReportActionMessageCache: Record<string, string> = {};

let conciergeReportID: OnyxEntry<string>;
Onyx.connect({

Check warning on line 939 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.CONCIERGE_REPORT_ID,
callback: (value) => {
conciergeReportID = value;
Expand All @@ -944,7 +944,7 @@
});

const defaultAvatarBuildingIconTestID = 'SvgDefaultAvatarBuilding Icon';
Onyx.connect({

Check warning on line 947 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.SESSION,
callback: (value) => {
// When signed out, val is undefined
Expand All @@ -962,7 +962,7 @@
let allPersonalDetails: OnyxEntry<PersonalDetailsList>;
let allPersonalDetailLogins: string[];
let currentUserPersonalDetails: OnyxEntry<PersonalDetails>;
Onyx.connect({

Check warning on line 965 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (value) => {
if (currentUserAccountID) {
Expand All @@ -974,14 +974,14 @@
});

let allReportsDraft: OnyxCollection<Report>;
Onyx.connect({

Check warning on line 977 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_DRAFT,
waitForCollectionCallback: true,
callback: (value) => (allReportsDraft = value),
});

let allPolicies: OnyxCollection<Policy>;
Onyx.connect({

Check warning on line 984 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (value) => (allPolicies = value),
Expand All @@ -989,7 +989,7 @@

let allReports: OnyxCollection<Report>;
let reportsByPolicyID: ReportByPolicyMap;
Onyx.connect({

Check warning on line 992 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (value) => {
Expand Down Expand Up @@ -1030,14 +1030,14 @@
});

let allBetas: OnyxEntry<Beta[]>;
Onyx.connect({

Check warning on line 1033 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.BETAS,
callback: (value) => (allBetas = value),
});

let allTransactions: OnyxCollection<Transaction> = {};
let reportsTransactions: Record<string, Transaction[]> = {};
Onyx.connect({

Check warning on line 1040 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.TRANSACTION,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -1063,7 +1063,7 @@
});

let allReportActions: OnyxCollection<ReportActions>;
Onyx.connect({

Check warning on line 1066 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
waitForCollectionCallback: true,
callback: (actions) => {
Expand All @@ -1076,7 +1076,7 @@

let allReportMetadata: OnyxCollection<ReportMetadata>;
const allReportMetadataKeyValue: Record<string, ReportMetadata> = {};
Onyx.connect({

Check warning on line 1079 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_METADATA,
waitForCollectionCallback: true,
callback: (value) => {
Expand Down Expand Up @@ -2744,7 +2744,7 @@
}

if (isInvoiceReport(report)) {
return report?.ownerAccountID === currentUserAccountID && isReportOpenOrProcessing && policy?.approvalMode !== CONST.POLICY.APPROVAL_MODE.OPTIONAL;
return report?.ownerAccountID === currentUserAccountID && isReportOpenOrProcessing;

@github-actions github-actions Bot Oct 31, 2025

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.

Question: Invoice deletion vs Expense Report deletion logic

The change simplifies invoice deletion by removing the policy?.approvalMode !== CONST.POLICY.APPROVAL_MODE.OPTIONAL check. This makes sense as the previous logic was counterintuitive - it required approval workflows to be enabled to delete invoices, which seems backwards.

However, comparing this to the expense report deletion logic (lines 2756-2766), I notice expense reports have protection against deleting forwarded reports:

const isForwarded = isProcessingReport(report) && isApprovalEnabled && !isAwaitingFirstLevelApproval(report);
return isReportSubmitter && isReportOpenOrProcessing && !isForwarded;

Question for reviewers: Should invoices have similar protection against deletion once they're in an approval workflow? Or are invoices intentionally different because they don't follow the same approval workflow as expense reports?

The current change allows invoice deletion based solely on:

  • Owner check
  • Report status (open or processing)

This seems appropriate if invoices don't participate in approval workflows the same way expense reports do. But if they do, we might want to add the !isForwarded check here as well for consistency.

@suneox suneox Nov 3, 2025

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.

The delete invoice flow doesn’t depend on the approval mode as mentioned.

@github-actions github-actions Bot Oct 31, 2025

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.

✅ Change looks correct - Removes illogical constraint

The change fixes a bug where invoices could ONLY be deleted when workflows were ENABLED (approvalMode !== OPTIONAL), which prevented deletion when workflows were disabled. This was backwards logic.

After the change, invoice deletion now follows the same pattern as IOU reports (line 2752-2753):

  • ✅ Owner check: report?.ownerAccountID === currentUserAccountID
  • ✅ State protection: isReportOpenOrProcessing (prevents deleting approved/reimbursed invoices)
  • ✅ No approval mode dependency (consistent with IOU reports)

Security note: The isReportOpenOrProcessing check (line 2733) provides adequate protection by ensuring only OPEN or SUBMITTED invoices can be deleted. Once an invoice is approved or paid, it cannot be deleted.

The change is logically sound and aligns with the existing codebase patterns.

}

// Users cannot delete a report in the unreported or IOU cases, but they can delete individual transactions.
Expand Down
19 changes: 19 additions & 0 deletions tests/unit/ReportUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
buildTransactionThread,
canAddTransaction,
canCreateRequest,
canDeleteMoneyRequestReport,
canDeleteReportAction,
canDeleteTransaction,
canEditMoneyRequest,
Expand Down Expand Up @@ -3410,6 +3411,24 @@ describe('ReportUtils', () => {
});
});

describe('canDeleteMoneyRequestReport', () => {
it('should allow deletion if the report is open invoice report', async () => {
const invoiceReport = {...createInvoiceReport(343), ownerAccountID: currentUserAccountID, stateNum: CONST.REPORT.STATE_NUM.OPEN, statusNum: CONST.REPORT.STATUS_NUM.OPEN};
// Wait for Onyx to load session data before calling canDeleteMoneyRequestReport,
// since it relies on the session subscription for currentUserAccountID.
await new Promise<void>((resolve) => {
const connection = Onyx.connectWithoutView({
key: `${ONYXKEYS.SESSION}`,
callback: () => {
Onyx.disconnect(connection);
resolve();
},
});
});
expect(canDeleteMoneyRequestReport(invoiceReport, [], [])).toBe(true);
});
});

describe('canEditMoneyRequest', () => {
it('it should return false for archived invoice', async () => {
const invoiceReport: Report = {
Expand Down
Loading