Skip to content
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

Jump to bottom

Don't allow employee to delete submitted request #35744

Merged
merged 8 commits into from
Feb 8, 2024
Merged

Don't allow employee to delete submitted request #35744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e0ae249
don't allow employee to delete submitted request
bernhardoj Feb 3, 2024
07fc3df
Merge branch 'main' into fix/34538-dont-allow-employee-delete-submitt…
bernhardoj Feb 5, 2024
943e2a1
pass null instead of undefined
bernhardoj Feb 5, 2024
8ff8453
Merge branch 'main' into fix/34538-dont-allow-employee-delete-submitt…
bernhardoj Feb 6, 2024
7fe73c2
pass null instead of empty object
bernhardoj Feb 6, 2024
177e65d
only allow deleting the request if it's not submitted
bernhardoj Feb 6, 2024
c5b0efe
Merge branch 'main' into fix/34538-dont-allow-employee-delete-submitt…
bernhardoj Feb 6, 2024
14f1297
update the condition
bernhardoj Feb 6, 2024
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
24 changes: 16 additions & 8 deletions src/components/MoneyRequestHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -82,16 +83,21 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction,

const isScanning = TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction);
const isPending = TransactionUtils.isExpensifyCardTransaction(transaction) && TransactionUtils.isPending(transaction);

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 deleting the request if it's not submitted or the user is the policy admin
canDeleteRequest = canDeleteRequest && (ReportUtils.isDraftExpenseReport(moneyRequestReport) || PolicyUtils.isPolicyAdmin(policy));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested and found out that we can't add a receipt on a submitted expense too, so I decided to move the condition here.

But if the submitted expense has a receipt, we can replace/delete it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested and it seems that I can add a receipt on a submitted expense. Can you please double check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check once again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can add a receipt on a submitted request. Updated

Comment on lines +87 to +92
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let canDeleteRequest = canModifyRequest;
if (ReportUtils.isPaidGroupPolicyExpenseReport(moneyRequestReport)) {
// 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));
}
// 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
const canDeleteRequest =
canModifyRequest &&
(ReportUtils.isPaidGroupPolicyExpenseReport(moneyRequestReport) ? ReportUtils.isDraftExpenseReport(moneyRequestReport) || PolicyUtils.isPolicyAdmin(policy) : true);

NAB. But I think using const is better for this case. It's easier to know where canDeleteRequest value is coming from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I feel the current code is easier to read


useEffect(() => {
if (canModifyRequest) {
if (canDeleteRequest) {
return;
}

setIsDeleteModalVisible(false);
}, [canModifyRequest]);
}, [canDeleteRequest]);
const threeDotsMenuItems = [HeaderUtils.getPinMenuItem(report)];
if (canModifyRequest) {
if (!TransactionUtils.hasReceipt(transaction)) {
Expand All @@ -110,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 (
Expand Down
6 changes: 5 additions & 1 deletion src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1227,6 +1227,7 @@ function canDeleteReportAction(reportAction: OnyxEntry<ReportAction>, reportID:
const report = getReport(reportID);

const isActionOwner = reportAction?.actorAccountID === currentUserAccountID;
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`] ?? null;

if (reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) {
// For now, users cannot delete split actions
Expand All @@ -1237,6 +1238,10 @@ function canDeleteReportAction(reportAction: OnyxEntry<ReportAction>, reportID:
}

if (isActionOwner) {
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);
}
return true;
}
}
Expand All @@ -1250,7 +1255,6 @@ function canDeleteReportAction(reportAction: OnyxEntry<ReportAction>, 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;
Expand Down
Loading