Fix: Approved requests shouldn't have a delete option #27952
Conversation
melvin-bot
bot
requested review from
cubuspl42
and removed request for
a team
|
@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@Pujan92
Do I need an .edu e-mail? |
No, any of your email works for a Principal. Only for teacher, I see we need to provide a non-public domain id otherwise it won't take the user to the principal info page. App/src/pages/TeachersUnite/ImTeacherPage.js Lines 23 to 26 in 6836eaf Note: I am using https://tempmailo.com/ for temp mails and it provides random domains that can pass the above non-public domain check. |
| @@ -754,6 +754,17 @@ function isMoneyRequestReport(reportOrID) { | |||
| return isIOUReport(report) || isExpenseReport(report); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Why is this a part of this PR?
There was a problem hiding this comment.
Okey, I think I get it. The added usage cause a lint error.
I'm not sure I understand it though, to be honest, I thought that there was no problem with calling functions in the same module, even when there are cycles.
There was a problem hiding this comment.
It was causing a lint error due to the function being used before it was defined so I needed to rearrange its order.
| @@ -768,22 +779,22 @@ function canDeleteReportAction(reportAction, reportID) { | |||
| return false; | |||
| } | |||
| const isActionOwner = reportAction.actorAccountID === currentUserAccountID; | |||
| if (isActionOwner && ReportActionsUtils.isMoneyRequestAction(reportAction) && !isSettled(reportAction.originalMessage.IOUReportID)) { | |||
| const report = getReport(reportID); | |||
There was a problem hiding this comment.
This function becomes a mess...
Do I understand correctly that it's equivalent to...
const report = getReport(reportID);
const isMoneyRequestActionProtectedFromBeingUserDeleted = () => {
if (ReportActionsUtils.isMoneyRequestAction(reportAction)) {
// For now, users cannot delete split actions
const isSplitAction = lodashGet(reportAction, 'originalMessage.type') === CONST.IOU.REPORT_ACTION_TYPE.SPLIT;
return isSplitAction || isSettled(reportAction.originalMessage.IOUReportID) || isReportApproved(report);
} else {
return false;
}
}
const isActionProtectedFromBeingUserDeleted = () => reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT ||
reportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ||
ReportActionsUtils.isCreatedTaskReportAction(reportAction) ||
reportAction.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE ||
isMoneyRequestActionProtectedFromBeingUserDeleted();
const isAdmin = () => {
const policy = lodashGet(allPolicies, `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`) || {};
return policy.role === CONST.POLICY.ROLE.ADMIN && !isDM(report);
}
return (isActionOwner && !isActionProtectedFromBeingUserDeleted()) || isAdmin();There was a problem hiding this comment.
I think below 2 points look incorrect to me
- Combining these will lead to conflict conditions bcoz for all non
ADDCOMMENTactions it will return true but we have a separate check formoneyRequestaction earlier for early return.
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT ||
reportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ||
ReportActionsUtils.isCreatedTaskReportAction(reportAction) ||
reportAction.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE ||
isMoneyRequestActionProtectedFromBeingUserDeleted();- As we are returning early based on conditions, we should avoid checking the
isAdminotherwise for ADDCOMMENT also the admin will have the option to delete someone else's comment|| isAdmin().
So I think combining all parts here won't work as expected.
(isActionOwner && !isActionProtectedFromBeingUserDeleted()) || isAdmin();
There was a problem hiding this comment.
You're right, early returns matter a lot here.
Still, we introduce code duplication and add extra complexity to already complex function.
How about this?
function canDeleteReportAction(reportAction, reportID) {
const report = getReport(reportID);
const isActionOwner = reportAction.actorAccountID === currentUserAccountID;
if (ReportActionsUtils.isMoneyRequestAction(reportAction)) {
// For now, users cannot delete split actions
const isSplitAction = lodashGet(reportAction, 'originalMessage.type') === CONST.IOU.REPORT_ACTION_TYPE.SPLIT;
if (isSplitAction || isSettled(reportAction.originalMessage.IOUReportID) || isReportApproved(report)) {
return false;
}
if (isActionOwner) {
return true;
}
}
if (
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT ||
reportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ||
ReportActionsUtils.isCreatedTaskReportAction(reportAction) ||
reportAction.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE
) {
return false;
}
const policy = lodashGet(allPolicies, `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`) || {};
const isAdmin = policy.role === CONST.POLICY.ROLE.ADMIN && !isDM(report);
return isActionOwner || isAdmin;
}I proved that these two functions have equivalent truth tables:
function canDeleteAction1(args) {
if (args.isMoneyRequestAction && args.isSplitAction) {
return false;
}
if (args.isActionOwner && args.isMoneyRequestAction && !args.isActionSettled && !args.isApproved) {
return true;
}
if (
args.isNotAddComment ||
args.isPendingDelete ||
args.isCreatedTask ||
(args.isMoneyRequestAction && (args.isActionSettled || args.isApproved)) ||
args.isConcierge
) {
return false;
}
if (args.isActionOwner) {
return true;
}
return args.isAdmin;
}
function canDeleteAction2(args) {
if (args.isMoneyRequestAction) {
if (args.isSplitAction || args.isActionSettled || args.isApproved) {
return false;
} else if (args.isActionOwner) {
return true;
}
}
if (
args.isNotAddComment ||
args.isPendingDelete ||
args.isCreatedTask ||
args.isConcierge
) {
return false;
}
return args.isActionOwner || args.isAdmin;
}There was a problem hiding this comment.
Looks much cleaner to me @cubuspl42 🚀
|
@Pujan92 I'm having trouble reproducing the issue on |
Issue(shows delete option for approved request) Screen.Recording.2023-09-28.at.18.23.29.mp4 |
|
Just checking in, is there anything we can help clear up here? |
|
@Julesssss Not for now. @Pujan92 Could you comment on this behavior? I don't think this is expected, and I believe it falls under the scope of our issue. delete-approved-issue-1.mp4 |
It should not show the delete option and isn't showing for me, can you plz recheck with this PR code Screen.Recording.2023-09-29.at.18.08.47.mov |
|
@Pujan92 I was sure I was on the PR branch, but I can't see the three-dot button now either. I must have been mistaken. |
Reviewer Checklist
Screenshots/VideosWebdelete-approved-web.mp4Mobile Web - Chromedelete-approved-android-web-compressed.mp4Mobile Web - Safaridelete-approved-ios-web.mp4Desktopdelete-approved-desktop2.mp4iOSdelete-approved-ios.mp4Androiddelete-approved-android-compressed.mp4 |
|
@Pujan92 Would you take a look? |
|
Merged with main and no tests failure now. |
|
✋ 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/Julesssss in version: 1.3.79-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
| @@ -84,7 +85,7 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction, | |||
| <HeaderWithBackButton | |||
| shouldShowAvatarWithDisplay | |||
| shouldShowPinButton={false} | |||
| shouldShowThreeDotsButton={isActionOwner && !isSettled} | |||
| shouldShowThreeDotsButton={isActionOwner && !isSettled && !isApproved} | |||
There was a problem hiding this comment.
We forgot to factor in the deleted requests here. This later on caused #26019
Details
#26419
Fixed Issues
$ #26419
PROPOSAL: #26419 (comment)
Tests
Ignore steps(1,2) if you already opted as "I am teacher" option earlier
Offline tests
QA Steps
Ignore steps(1,2) if you already opted as "I am teacher" option earlier
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-09-21.at.18.53.50.mov
Mobile Web - Chrome
m1.webm
Mobile Web - Safari
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-09-21.at.19.41.58.mp4
Desktop
Screen.Recording.2023-09-21.at.19.46.25.mov
iOS
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-09-21.at.19.34.45.mp4
Android
m2.webm