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

Fix: Approved requests shouldn't have a delete option #27952

Merged
merged 5 commits into from
Oct 5, 2023
Merged

Fix: Approved requests shouldn't have a delete option #27952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
109465e
hide delete option for approved report
Pujan92 Sep 21, 2023
db2d24e
Merge branch 'main' into fix/26419
Pujan92 Sep 27, 2023
ce27512
simplified canDeleteReportAction func
Pujan92 Sep 27, 2023
ac8c1ca
Merge branch 'main' into fix/26419
Pujan92 Sep 29, 2023
90df38f
Merge branch 'main' into fix/26419
Pujan92 Oct 3, 2023
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
3 changes: 2 additions & 1 deletion src/components/MoneyRequestHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction,
const [isDeleteModalVisible, setIsDeleteModalVisible] = useState(false);
const moneyRequestReport = parentReport;
const isSettled = ReportUtils.isSettled(moneyRequestReport.reportID);
const isApproved = ReportUtils.isReportApproved(moneyRequestReport);
const {isSmallScreenWidth, windowWidth} = useWindowDimensions();

// Only the requestor can take delete the request, admins can only edit it.
Expand All @@ -84,7 +85,7 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction,
<HeaderWithBackButton
shouldShowAvatarWithDisplay
shouldShowPinButton={false}
shouldShowThreeDotsButton={isActionOwner && !isSettled}
shouldShowThreeDotsButton={isActionOwner && !isSettled && !isApproved}
Copy link
Contributor

Choose a reason for hiding this comment

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

We forgot to factor in the deleted requests here. This later on caused #26019

threeDotsMenuItems={[
...(TransactionUtils.hasReceipt(transaction)
? []
Expand Down
53 changes: 30 additions & 23 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,17 @@ function isMoneyRequestReport(reportOrID) {
return isIOUReport(report) || isExpenseReport(report);
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a part of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was causing a lint error due to the function being used before it was defined so I needed to rearrange its order.

* Get the report given a reportID
*
* @param {String} reportID
* @returns {Object}
*/
function getReport(reportID) {
// Deleted reports are set to null and lodashGet will still return null in that case, so we need to add an extra check
return lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {}) || {};
}

/**
* Can only delete if the author is this user and the action is an ADDCOMMENT action or an IOU action in an unsettled report, or if the user is a
* policy admin
Expand All @@ -764,29 +775,36 @@ function isMoneyRequestReport(reportOrID) {
* @returns {Boolean}
*/
function canDeleteReportAction(reportAction, reportID) {
// For now, users cannot delete split actions
if (ReportActionsUtils.isMoneyRequestAction(reportAction) && lodashGet(reportAction, 'originalMessage.type') === CONST.IOU.REPORT_ACTION_TYPE.SPLIT) {
return false;
}
const report = getReport(reportID);
Copy link
Contributor

@cubuspl42 cubuspl42 Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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();

Copy link
Contributor Author

@Pujan92 Pujan92 Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think below 2 points look incorrect to me

  1. Combining these will lead to conflict conditions bcoz for all non ADDCOMMENT actions it will return true but we have a separate check for moneyRequest action 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();
  1. As we are returning early based on conditions, we should avoid checking the isAdmin otherwise 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();

Copy link
Contributor

Choose a reason for hiding this comment

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

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks much cleaner to me @cubuspl42 🚀


const isActionOwner = reportAction.actorAccountID === currentUserAccountID;
if (isActionOwner && ReportActionsUtils.isMoneyRequestAction(reportAction) && !isSettled(reportAction.originalMessage.IOUReportID)) {
return true;

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) ||
(ReportActionsUtils.isMoneyRequestAction(reportAction) && isSettled(reportAction.originalMessage.IOUReportID)) ||
reportAction.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE
) {
return false;
}
if (isActionOwner) {
return true;
}
const report = lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {});

const policy = lodashGet(allPolicies, `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`) || {};
return policy.role === CONST.POLICY.ROLE.ADMIN && !isDM(report);
const isAdmin = policy.role === CONST.POLICY.ROLE.ADMIN && !isDM(report);

return isActionOwner || isAdmin;
}

/**
Expand Down Expand Up @@ -1194,17 +1212,6 @@ function getDisplayNamesWithTooltips(personalDetailsList, isMultipleParticipantR
});
}

/**
* Get the report given a reportID
*
* @param {String} reportID
* @returns {Object}
*/
function getReport(reportID) {
// Deleted reports are set to null and lodashGet will still return null in that case, so we need to add an extra check
return lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {}) || {};
}

/**
* Determines if a report has an IOU that is waiting for an action from the current user (either Pay or Add a credit bank account)
*
Expand Down