-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Don't allow employee to delete submitted request #35744
Don't allow employee to delete submitted request #35744
Conversation
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)); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Here is the video showing that the admin can still successfully add a receipt and delete a submitted report. Screen.Recording.2024-02-03.at.16.29.07.mov |
src/libs/PolicyUtils.ts
Outdated
@@ -107,7 +107,7 @@ function isExpensifyGuideTeam(email: string): boolean { | |||
/** | |||
* Checks if the current user is an admin of the policy. | |||
*/ | |||
const isPolicyAdmin = (policy: OnyxEntry<Policy>): boolean => policy?.role === CONST.POLICY.ROLE.ADMIN; | |||
const isPolicyAdmin = (policy?: OnyxEntry<Policy>): boolean => policy?.role === CONST.POLICY.ROLE.ADMIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert. Pass null
if policy is not available. (same for other util changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I don't know that. Updated.
src/libs/ReportUtils.ts
Outdated
@@ -805,7 +805,7 @@ function isGroupPolicy(report: OnyxEntry<Report>): boolean { | |||
/** | |||
* Whether the provided report belongs to a Control or Collect policy | |||
*/ | |||
function isPaidGroupPolicy(report: OnyxEntry<Report>): boolean { | |||
function isPaidGroupPolicy(report: OnyxEntry<Report> | EmptyObject): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert. Use null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see that we use EmptyObject in many places. Updated
src/libs/ReportUtils.ts
Outdated
@@ -827,7 +827,7 @@ function isControlPolicyExpenseReport(report: OnyxEntry<Report>): boolean { | |||
/** | |||
* Whether the provided report belongs to a Control or Collect policy and is an expense report | |||
*/ | |||
function isPaidGroupPolicyExpenseReport(report: OnyxEntry<Report>): boolean { | |||
function isPaidGroupPolicyExpenseReport(report: OnyxEntry<Report> | EmptyObject): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert. Use null
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)); | ||
} |
There was a problem hiding this comment.
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?
src/libs/ReportUtils.ts
Outdated
@@ -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<Report>, policies: OnyxCollection<Policy>): string { | |||
function getPolicyType(report: OnyxEntry<Report> | EmptyObject, policies: OnyxCollection<Policy>): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert. Use null
Reviewer Checklist
Screenshots/Videos |
src/libs/ReportUtils.ts
Outdated
@@ -1237,6 +1238,10 @@ function canDeleteReportAction(reportAction: OnyxEntry<ReportAction>, reportID: | |||
} | |||
|
|||
if (isActionOwner) { | |||
if (isPaidGroupPolicyExpenseReport(!isEmptyObject(report) ? report : null)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isPaidGroupPolicyExpenseReport(!isEmptyObject(report) ? report : null)) { | |
if (!isEmptyObject(report) && isPaidGroupPolicyExpenseReport(report)) { |
NAB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I like this one better, updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please push the changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's weird. I though it's pushed. Done
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)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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
@bernhardoj Can you please merge main see it fixes the failing tests |
Perf test still failing, can you please check it? |
Hmm, it shouldn't be related to this PR as it fails on SearchPage. It has happened there many times on my previous PRs. I guess it's just flaky |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/Gonals in version: 1.4.39-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.39-8 🚀
|
Details
Currently, an employee can delete a submitted expense request, but the BE will gives an error.
Fixed Issues
$ #34538
PROPOSAL: #34538 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-02-03.at.16.30.04.mov
Android: mWeb Chrome
Screen.Recording.2024-02-03.at.16.07.41.mov
iOS: Native
Screen.Recording.2024-02-03.at.15.59.15.mov
iOS: mWeb Safari
Screen.Recording.2024-02-03.at.16.03.12.mov
MacOS: Chrome / Safari
Screen.Recording.2024-02-03.at.15.54.22.mov
MacOS: Desktop
Screen.Recording.2024-02-03.at.15.56.07.mov