From e0ae249d36c8b829c452ad51d05860da9890c9f9 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Sat, 3 Feb 2024 16:13:56 +0800 Subject: [PATCH 1/5] don't allow employee to delete submitted request --- src/components/MoneyRequestHeader.js | 7 ++++++- src/libs/PolicyUtils.ts | 2 +- src/libs/ReportUtils.ts | 12 ++++++++---- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/components/MoneyRequestHeader.js b/src/components/MoneyRequestHeader.js index e907f798051b..db73bba4eb51 100644 --- a/src/components/MoneyRequestHeader.js +++ b/src/components/MoneyRequestHeader.js @@ -9,6 +9,7 @@ import useWindowDimensions from '@hooks/useWindowDimensions'; import compose from '@libs/compose'; import * as HeaderUtils from '@libs/HeaderUtils'; import Navigation from '@libs/Navigation/Navigation'; +import * as PolicyUtils from '@libs/PolicyUtils'; import * as ReportActionsUtils from '@libs/ReportActionsUtils'; import * as ReportUtils from '@libs/ReportUtils'; import * as TransactionUtils from '@libs/TransactionUtils'; @@ -82,8 +83,12 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction, const isScanning = TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction); const isPending = TransactionUtils.isExpensifyCardTransaction(transaction) && TransactionUtils.isPending(transaction); + let canModifyRequest = isActionOwner && !isSettled && !isApproved && !ReportActionsUtils.isDeletedAction(parentReportAction); - const canModifyRequest = isActionOwner && !isSettled && !isApproved && !ReportActionsUtils.isDeletedAction(parentReportAction); + if (ReportUtils.isPaidGroupPolicyExpenseReport(moneyRequestReport)) { + // If it's a paid policy expense report, only allow modifying the request if it's not submitted or the user is the policy admin + canModifyRequest = canModifyRequest && (ReportUtils.isDraftExpenseReport(moneyRequestReport) || PolicyUtils.isPolicyAdmin(policy)); + } useEffect(() => { if (canModifyRequest) { diff --git a/src/libs/PolicyUtils.ts b/src/libs/PolicyUtils.ts index b6ee4ab3a353..e89bbee5c103 100644 --- a/src/libs/PolicyUtils.ts +++ b/src/libs/PolicyUtils.ts @@ -107,7 +107,7 @@ function isExpensifyGuideTeam(email: string): boolean { /** * Checks if the current user is an admin of the policy. */ -const isPolicyAdmin = (policy: OnyxEntry): boolean => policy?.role === CONST.POLICY.ROLE.ADMIN; +const isPolicyAdmin = (policy?: OnyxEntry): boolean => policy?.role === CONST.POLICY.ROLE.ADMIN; const isPolicyMember = (policyID: string, policies: Record): boolean => Object.values(policies).some((policy) => policy?.id === policyID); diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 568ce49ff961..ef0e50cb4c62 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -563,7 +563,7 @@ function getPolicy(policyID: string): Policy | EmptyObject { * Get the policy type from a given report * @param policies must have Onyxkey prefix (i.e 'policy_') for keys */ -function getPolicyType(report: OnyxEntry, policies: OnyxCollection): string { +function getPolicyType(report: OnyxEntry | EmptyObject, policies: OnyxCollection): string { return policies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.type ?? ''; } @@ -805,7 +805,7 @@ function isGroupPolicy(report: OnyxEntry): boolean { /** * Whether the provided report belongs to a Control or Collect policy */ -function isPaidGroupPolicy(report: OnyxEntry): boolean { +function isPaidGroupPolicy(report: OnyxEntry | EmptyObject): boolean { const policyType = getPolicyType(report, allPolicies); return policyType === CONST.POLICY.TYPE.CORPORATE || policyType === CONST.POLICY.TYPE.TEAM; } @@ -827,7 +827,7 @@ function isControlPolicyExpenseReport(report: OnyxEntry): boolean { /** * Whether the provided report belongs to a Control or Collect policy and is an expense report */ -function isPaidGroupPolicyExpenseReport(report: OnyxEntry): boolean { +function isPaidGroupPolicyExpenseReport(report: OnyxEntry | EmptyObject): boolean { return isExpenseReport(report) && isPaidGroupPolicy(report); } @@ -1239,6 +1239,7 @@ function canDeleteReportAction(reportAction: OnyxEntry, reportID: const report = getReport(reportID); const isActionOwner = reportAction?.actorAccountID === currentUserAccountID; + const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]; if (reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) { // For now, users cannot delete split actions @@ -1249,6 +1250,10 @@ function canDeleteReportAction(reportAction: OnyxEntry, reportID: } if (isActionOwner) { + if (isPaidGroupPolicyExpenseReport(report)) { + // If it's a paid policy expense report, only allow deleting the request if it's not submitted or the user is the policy admin + return isDraftExpenseReport(report) || PolicyUtils.isPolicyAdmin(policy); + } return true; } } @@ -1262,7 +1267,6 @@ function canDeleteReportAction(reportAction: OnyxEntry, reportID: return false; } - const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]; const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN && !isEmptyObject(report) && !isDM(report); return isActionOwner || isAdmin; From 943e2a12d130ddcdd291e5c42f05e122721f0024 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Mon, 5 Feb 2024 23:03:10 +0800 Subject: [PATCH 2/5] pass null instead of undefined --- src/libs/PolicyUtils.ts | 2 +- src/libs/ReportUtils.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/PolicyUtils.ts b/src/libs/PolicyUtils.ts index e89bbee5c103..b6ee4ab3a353 100644 --- a/src/libs/PolicyUtils.ts +++ b/src/libs/PolicyUtils.ts @@ -107,7 +107,7 @@ function isExpensifyGuideTeam(email: string): boolean { /** * Checks if the current user is an admin of the policy. */ -const isPolicyAdmin = (policy?: OnyxEntry): boolean => policy?.role === CONST.POLICY.ROLE.ADMIN; +const isPolicyAdmin = (policy: OnyxEntry): boolean => policy?.role === CONST.POLICY.ROLE.ADMIN; const isPolicyMember = (policyID: string, policies: Record): boolean => Object.values(policies).some((policy) => policy?.id === policyID); diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index ef0e50cb4c62..1d43c65a25ad 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -1239,7 +1239,7 @@ function canDeleteReportAction(reportAction: OnyxEntry, reportID: const report = getReport(reportID); const isActionOwner = reportAction?.actorAccountID === currentUserAccountID; - const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]; + const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`] ?? null; if (reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) { // For now, users cannot delete split actions From 7fe73c2e13a40f6071a4e22dea09aafa64839156 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Tue, 6 Feb 2024 11:08:13 +0800 Subject: [PATCH 3/5] pass null instead of empty object --- src/libs/ReportUtils.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index e7ca44d4d740..89c2b902b7fc 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -545,7 +545,7 @@ function getPolicy(policyID: string | undefined): Policy | EmptyObject { * Get the policy type from a given report * @param policies must have Onyxkey prefix (i.e 'policy_') for keys */ -function getPolicyType(report: OnyxEntry | EmptyObject, policies: OnyxCollection): string { +function getPolicyType(report: OnyxEntry, policies: OnyxCollection): string { return policies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.type ?? ''; } @@ -793,7 +793,7 @@ function isGroupPolicy(report: OnyxEntry): boolean { /** * Whether the provided report belongs to a Control or Collect policy */ -function isPaidGroupPolicy(report: OnyxEntry | EmptyObject): boolean { +function isPaidGroupPolicy(report: OnyxEntry): boolean { const policyType = getPolicyType(report, allPolicies); return policyType === CONST.POLICY.TYPE.CORPORATE || policyType === CONST.POLICY.TYPE.TEAM; } @@ -815,7 +815,7 @@ function isControlPolicyExpenseReport(report: OnyxEntry): boolean { /** * Whether the provided report belongs to a Control or Collect policy and is an expense report */ -function isPaidGroupPolicyExpenseReport(report: OnyxEntry | EmptyObject): boolean { +function isPaidGroupPolicyExpenseReport(report: OnyxEntry): boolean { return isExpenseReport(report) && isPaidGroupPolicy(report); } @@ -1238,7 +1238,7 @@ function canDeleteReportAction(reportAction: OnyxEntry, reportID: } if (isActionOwner) { - if (isPaidGroupPolicyExpenseReport(report)) { + if (isPaidGroupPolicyExpenseReport(!isEmptyObject(report) ? report : null)) { // If it's a paid policy expense report, only allow deleting the request if it's not submitted or the user is the policy admin return isDraftExpenseReport(report) || PolicyUtils.isPolicyAdmin(policy); } From 177e65da18161b9e8bdf0151422138fe4c9ed9be Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Tue, 6 Feb 2024 11:31:34 +0800 Subject: [PATCH 4/5] only allow deleting the request if it's not submitted --- src/components/MoneyRequestHeader.js | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/components/MoneyRequestHeader.js b/src/components/MoneyRequestHeader.js index db73bba4eb51..e46c81cc9e91 100644 --- a/src/components/MoneyRequestHeader.js +++ b/src/components/MoneyRequestHeader.js @@ -83,20 +83,21 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction, const isScanning = TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction); const isPending = TransactionUtils.isExpensifyCardTransaction(transaction) && TransactionUtils.isPending(transaction); - let canModifyRequest = isActionOwner && !isSettled && !isApproved && !ReportActionsUtils.isDeletedAction(parentReportAction); + const canModifyRequest = isActionOwner && !isSettled && !isApproved && !ReportActionsUtils.isDeletedAction(parentReportAction); + let canDeleteRequest = canModifyRequest; if (ReportUtils.isPaidGroupPolicyExpenseReport(moneyRequestReport)) { - // If it's a paid policy expense report, only allow modifying the request if it's not submitted or the user is the policy admin - canModifyRequest = canModifyRequest && (ReportUtils.isDraftExpenseReport(moneyRequestReport) || PolicyUtils.isPolicyAdmin(policy)); + // If it's a paid policy expense report, only allow deleting the request if it's not submitted or the user is the policy admin + canDeleteRequest = canDeleteRequest && (ReportUtils.isDraftExpenseReport(moneyRequestReport) || PolicyUtils.isPolicyAdmin(policy)); } useEffect(() => { - if (canModifyRequest) { + if (canDeleteRequest) { return; } setIsDeleteModalVisible(false); - }, [canModifyRequest]); + }, [canDeleteRequest]); const threeDotsMenuItems = [HeaderUtils.getPinMenuItem(report)]; if (canModifyRequest) { if (!TransactionUtils.hasReceipt(transaction)) { @@ -115,11 +116,13 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction, ), }); } - threeDotsMenuItems.push({ - icon: Expensicons.Trashcan, - text: translate('reportActionContextMenu.deleteAction', {action: parentReportAction}), - onSelected: () => setIsDeleteModalVisible(true), - }); + if (canDeleteRequest) { + threeDotsMenuItems.push({ + icon: Expensicons.Trashcan, + text: translate('reportActionContextMenu.deleteAction', {action: parentReportAction}), + onSelected: () => setIsDeleteModalVisible(true), + }); + } } return ( From 14f12974b26b9e1865a3998e16416715c2be0b19 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Tue, 6 Feb 2024 21:42:02 +0800 Subject: [PATCH 5/5] update the condition --- 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 89c2b902b7fc..3a8c8b1625b2 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -1238,7 +1238,7 @@ function canDeleteReportAction(reportAction: OnyxEntry, reportID: } if (isActionOwner) { - if (isPaidGroupPolicyExpenseReport(!isEmptyObject(report) ? report : null)) { + if (!isEmptyObject(report) && isPaidGroupPolicyExpenseReport(report)) { // If it's a paid policy expense report, only allow deleting the request if it's not submitted or the user is the policy admin return isDraftExpenseReport(report) || PolicyUtils.isPolicyAdmin(policy); }