[HOLD for payment 2023-10-16] [$500] Approved requests shouldn't have a `delete` option

[JmillsExpensify]

While we've already implemented logic preventing paid requests from being deleted, the same isn't true for approved requests, which we've just added. Accordingly, let's remove the Delete request option from the three dot overflow menu anytime a request has been approved.

Here's mock for clarity. Notice how I've removed the Delete request option.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bd5a594a7f989566
• Upwork Job ID: 1697326804261687296
• Last Price Increase: 2023-08-31
• Automatic offers:
• cubuspl42 | Reviewer | 26523887
• Pujan92 | Contributor | 26523889

[melvin-bot[bot]]

Triggered auto assignment to @NicMendonca (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[jeet-dhandha]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• Currently we can delete approved requests, which is not ideal.

What is the root cause of that problem?

• We don't have logic preventing the delete option from being displayed.

What changes do you think we should make in order to solve the problem?

• We can add a isApproved const here and to check if report is approved or not, we can use isReportApproved function from ReportUtils.js file.

https://github.com/Expensify/App/blob/c9c1f47442546f2254ede5c3fab99246dd381e94/src/components/MoneyRequestHeader.js#L60

const isApproved = ReportUtils.isReportApproved(moneyRequestReport);

• And use it here to show/hide delete option, like we did for isSettled.

https://github.com/Expensify/App/blob/c9c1f47442546f2254ede5c3fab99246dd381e94/src/components/MoneyRequestHeader.js#L81-L84

shouldShowThreeDotsButton={isActionOwner && !isSettled && !isApproved}

• We will need to update canDeleteReportAction also in ReportUtils. For context menu.

https://github.com/Expensify/App/blob/7c13734d048c2b8e61f33c4e24bdc4f42d782952/src/libs/ReportUtils.js#L749

const report = getReport(reportID);
if (isActionOwner && ReportActionsUtils.isMoneyRequestAction(reportAction) && !isSettled(reportAction.originalMessage.IOUReportID) && !isReportApproved(report)) {

What alternative solutions did you explore? (Optional)

• If we are looking to just remove the Delete Option and retain other options (which we currently don't have any on main branch.)

For Ref of which options are available

https://github.com/Expensify/App/blob/c9c1f47442546f2254ede5c3fab99246dd381e94/src/components/MoneyRequestHeader.js#L85-L91

• We can update the above code conditionally.
  Updated Code

threeDotsMenuItems={[
    ...(!isApproved
        ? [
                {
                    icon: Expensicons.Trashcan,
                    text: props.translate('reportActionContextMenu.deleteAction', {action: parentReportAction}),
                    onSelected: deleteTransaction,
                },
            ]
        : []),
    ...restOptions,
]}

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01bd5a594a7f989566

[melvin-bot[bot]]

Current assignees @JmillsExpensify and @NicMendonca are eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External)

[Pujan92]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Approved requests show a delete option which should not be available

What is the root cause of that problem?

Currently, we do not have applied conditions to not show delete action in the report header as well in the context menu.

What changes do you think we should make in order to solve the problem?

I am assuming when the request is settled up, it also gets approved indirectly(which means the approved fields are set to the correct status). Consider adding a check isReportApproved by using ReportUtils.isReportApproved(report) below places

1. https://github.com/Expensify/App/blob/b226893b81f1e160bef4a6821cee19c8a03bcd24/src/components/MoneyRequestHeader.js#L84 && !isReportApproved

2. Add conditions for the context menu actions also to avoid showing the delete option https://github.com/Expensify/App/blob/b226893b81f1e160bef4a6821cee19c8a03bcd24/src/pages/home/report/ContextMenu/ContextMenuActions.js#L310-L320

https://github.com/Expensify/App/blob/b226893b81f1e160bef4a6821cee19c8a03bcd24/src/libs/ReportUtils.js#L749

const report = getReport(reportAction.originalMessage.IOUReportID)
&& !isReportApproved(report)

https://github.com/Expensify/App/blob/b226893b81f1e160bef4a6821cee19c8a03bcd24/src/libs/ReportUtils.js#L756

(ReportActionsUtils.isMoneyRequestAction(reportAction) && isReportApproved(report)) ||

At the moment I am not able to test as not having the approved feature/permission for my account.

[cubuspl42]

@Pujan92 Isn't this feature behind a beta, like most other features? Typically it's enough to manually opt-in by returning true from an appropriate function in Permissions.js.

[JmillsExpensify]

@Pujan92 Were you able to figure out the beta consideration?

[cubuspl42]

@jeet-dhandha Did you have any issues with testing the proposed solution?

@Pujan92 Also, would you sum up briefly the difference between your proposal and the earlier one?

[jeet-dhandha]

No i didn't had any issues.

[Pujan92]

@cubuspl42 @JmillsExpensify actually approve button only shows for the corporate policy with manager account id. Now, I am making it true directly and able to test.

https://github.com/Expensify/App/blob/9701fbe5ec24bf5031da390075b2a9fb125b3ce3/src/components/MoneyReportHeader.js#L75-L80

@Pujan92 Also, would you sum up briefly the difference between your proposal and https://github.com/Expensify/App/issues/26419#issuecomment-1701577670?

I see that the earlier proposal has been updated laterwards and now it looks quite similar.

[jeet-dhandha]

@cubuspl42 Let @Pujan92 have this one 👍 .

[jeet-dhandha]

I wanted know how we can allow CORPORATE type Workspace/

[Pujan92]

I wanted know how we can allow CORPORATE type Workspace/

I don't think we have an option for creating corporate policy.

[Pujan92]

Now, I am making it true directly and able to test.

Seems incorrect permission or data which we passed to ApproveMoneyRequest api call closing the report(in the backend checks) and make it archived.

https://github.com/Expensify/App/assets/14358475/914b2cc1-7f89-4d75-ae6c-52467f19c5c9

Considering that I think it can be tested correctly only on corporate policy.

[cubuspl42]

Both proposals, in their final form, looked similar and contained convincing root cause analysis and solution plan. Honoring the suggestion by @jeet-dhandha, I say we go with @Pujan92 proposal.

At the implementation phase, we'll need to ensure that we have appropriate permissions and capabilities to test the solution thoroughly.

C+ reviewed 🎀 👀 🎀

[melvin-bot[bot]]

Triggered auto assignment to @Julesssss, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[cubuspl42]

@Julesssss Would you take a look at the discussion above, and help us with figuring out what permissions or other backend state we need to test this solution? How can we put our hands on a "corporate policy"?

To be honest, this is my first encounter with such entity.

[Julesssss]

Would you take a look at the discussion above, and help us with figuring out what permissions or other backend state we need to test this solution? How can we put our hands on a "corporate policy"?

I'm pretty sure this is just a request made to a workspace. The workspace admin is the manager:

• Opt into all betas, just to be safe
• Locate or create a workspace
• As a user invited to the workspace, locate the workspace chat
• Request money from the workspace (NOT DIRECTLY FROM THE ADMIN)
• Sign into admin account

[cubuspl42]

@Pujan92

Considering that I think it can be tested correctly only on corporate policy.

Would you confront the assumption above with tips by @Julesssss?

[Pujan92]

@cubuspl42 With the tips given by @Julesssss and by commenting below condition I am able to see the Approve button on the admin side. https://github.com/Expensify/App/blob/1e0a249ba030b779d749ba07774a896b0f2c931f/src/components/MoneyReportHeader.js#L76-L78

On approving the request the report has been archived and also on the admin side seeing the crash, is archiving the report intentional in corporate policy too?

Requestor	Manager/Admin

I think it should set the statusNum to 3 which is approved status instead it set to 2.

https://github.com/Expensify/App/blob/1e0a249ba030b779d749ba07774a896b0f2c931f/src/CONST.ts#L578-L582

[cubuspl42]

On approving the request the report has been archived and also on the admin side seeing the crash, is archiving the report intentional in corporate policy too?

I don't know the answer.

@Julesssss @JmillsExpensify Do you have any idea?

[Julesssss]

We shouldn't be archiving a report on approval, discussing with the team currently...

[Julesssss]

is archiving the report intentional in corporate policy too?

Hey @Pujan92 can you please create a bug for this? (to ensure you claim the bounty).

[Pujan92]

is archiving the report intentional in corporate policy too?

Hey @Pujan92 can you please create a bug for this? (to ensure you claim the bounty).

Created, https://expensify.slack.com/archives/C049HHMV9SM/p1693912029208039

[Julesssss]

Okay, so with that being treated as a separate issue I think we're good to move forward with @Pujan92's proposal.

[melvin-bot[bot]]

📣 @cubuspl42 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @Pujan92 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[Pujan92]

@Julesssss The condition for solving this issue depends on the report status, so we can't test this change until the report not gets archived and is in the approved state. Can we hold this issue until other issue of approving report correctly takes place?

[Julesssss]

Ah right. Sure

[Julesssss]

Issue on hold

[JmillsExpensify]

Can someone clarify the issue/PR we're holding on please?

[Julesssss]

Can someone clarify the issue/PR we're holding on please?

https://expensify.slack.com/archives/C049HHMV9SM/p1693912029208039

Applause is unable to reproduce, can anyone spot the missing step that they might need to configure the correct env? They should be included in all betas already 😕

[Pujan92]

@Julesssss it is shown only for the Corporate Policy as per this comment snippet. In the dev we just commented out the condition to test based on earlier discussion.

[Julesssss]

But QA should be able to reproduce it then? 😕

[Pujan92]

But QA should be able to reproduce it then? 😕

Do they create a new corporate policy? If yes then I think can be reproducible for them and able to see approve button for admin.

[JmillsExpensify]

It's impossible to create a new corporate policy. This only applies to the one policy that we've added for Teachers Unite! Basically something that only Expensify can/has done.

[Pujan92]

Ok, in that case I am a member of the Teachers Unite and tried splitting the bill and currently I am seeing a request. To test my solution of not providing the delete action option after approval, the admin/manager needs to approve the request. Is that something we can do?

[melvin-bot[bot]]

@JmillsExpensify, @cubuspl42, @Julesssss, @Pujan92 Eep! 4 days overdue now. Issues have feelings too...

[JmillsExpensify]

Sorry for the delay, don't hesitate to reach out via DM on NewDot if you don't get a fast response. I can go in and approve your request now.

[cubuspl42]

Okey, this issue definitely needs an update. Formally, we're holding on https://github.com/Expensify/App/issues/27242, but that one is closed because it's not reproducible (?).

[JmillsExpensify]

I think we should take this issue off hold as soon as I approve the request shown in the screenshot above.

[JmillsExpensify]

@Pujan92 Can you try a fresh one in USD? I'm not seeing an approve button and I'm betting it's because you've requested an amount that is less than a dollar.

[Pujan92]

@JmillsExpensify I just made a new request of greater than 1 USD and hope now approve button is visible to you.

[JmillsExpensify]

Ok great. Looking

[JmillsExpensify]

Ok I checked the Onyx for this report. You've set your "principal" and approver as a@a.com. I'm using the volunteer@expensify.com account, but that's only used for payment. So accordingly, I'm not seeing an approve button since the principal/approver you set is a@a.com. Does that make sense?

[JmillsExpensify]

Based on that, I think you have the tools you need to test this yourself. Just make sure that when you sign up using the Teachers Unite form that you have access to both the "teacher" and "principal" emails. At that point, you'll be able to verify your solution works.

[Pujan92]

Thanks @JmillsExpensify , I got your point and I believe I can test with having a known "principal" email bcoz it is set as a manager so surely the approve button should be visible in the report.

[Pujan92]

@cubuspl42 PR is ready for review!

[JmillsExpensify]

Thanks @JmillsExpensify , I got your point and I believe I can test with having a known "principal" email bcoz it is set as a manager so surely the approve button should be visible in the report.

Yes exactly. Thank you!