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

JmillsExpensify · 2023-08-31T18:01:07Z

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

The text was updated successfully, but these errors were encountered:

melvin-bot · 2023-08-31T18:01:32Z

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 · 2023-08-31T18:43:54Z

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.

App/src/components/MoneyRequestHeader.js

Line 60 in c9c1f47

const isSettled = ReportUtils.isSettled(moneyRequestReport.reportID);

const isApproved = ReportUtils.isReportApproved(moneyRequestReport);

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

App/src/components/MoneyRequestHeader.js

Lines 81 to 84 in c9c1f47

    
           <HeaderWithBackButton 
        
               shouldShowAvatarWithDisplay 
        
               shouldShowPinButton={false} 
        
               shouldShowThreeDotsButton={isActionOwner && !isSettled}

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

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

App/src/libs/ReportUtils.js

Line 749 in 7c13734

    
           if (isActionOwner && ReportActionsUtils.isMoneyRequestAction(reportAction) && !isSettled(reportAction.originalMessage.IOUReportID)) {

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

App/src/components/MoneyRequestHeader.js

Lines 85 to 91 in c9c1f47

    
           threeDotsMenuItems={[ 
        
               { 
        
                   icon: Expensicons.Trashcan, 
        
                   text: translate('reportActionContextMenu.deleteAction', {action: parentReportAction}), 
        
                   onSelected: () => setIsDeleteModalVisible(true), 
        
               }, 
        
           ]}

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 · 2023-08-31T19:13:41Z

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

melvin-bot · 2023-08-31T19:13:44Z

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

melvin-bot · 2023-08-31T19:13:50Z

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

Pujan92 · 2023-08-31T20:51:50Z

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

App/src/components/MoneyRequestHeader.js

Line 84 in b226893

shouldShowThreeDotsButton={isActionOwner && !isSettled}

&& !isReportApproved

Add conditions for the context menu actions also to avoid showing the delete option

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Lines 310 to 320 in b226893

    
           { 
        
               isAnonymousAction: false, 
        
               textTranslateKey: 'reportActionContextMenu.deleteAction', 
        
               icon: Expensicons.Trashcan, 
        
               shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID) => 
        
                   // Until deleting parent threads is supported in FE, we will prevent the user from deleting a thread parent 
        
                   type === CONTEXT_MENU_TYPES.REPORT_ACTION && 
        
                   ReportUtils.canDeleteReportAction(reportAction, reportID) && 
        
                   !isArchivedRoom && 
        
                   !isChronosReport && 
        
                   !ReportActionsUtils.isMessageDeleted(reportAction),

App/src/libs/ReportUtils.js

Line 749 in b226893

    
           if (isActionOwner && ReportActionsUtils.isMoneyRequestAction(reportAction) && !isSettled(reportAction.originalMessage.IOUReportID)) {

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

App/src/libs/ReportUtils.js

Line 756 in b226893

    
           (ReportActionsUtils.isMoneyRequestAction(reportAction) && isSettled(reportAction.originalMessage.IOUReportID)) ||

(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 · 2023-09-01T08:36:13Z

@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 · 2023-09-01T23:25:31Z

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

cubuspl42 · 2023-09-02T07:59:07Z

@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 · 2023-09-02T09:06:48Z

No i didn't had any issues.

Pujan92 · 2023-09-02T09:11:11Z

@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.

App/src/components/MoneyReportHeader.js

Lines 75 to 80 in 9701fbe

    
           const shouldShowApproveButton = useMemo(() => { 
        
               if (policyType !== CONST.POLICY.TYPE.CORPORATE) { 
        
                   return false; 
        
               } 
        
               return isManager && !isApproved && !isSettled; 
        
           }, [policyType, isManager, isApproved, isSettled]);

@Pujan92 Also, would you sum up briefly the difference between your proposal and #26419 (comment)?

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

jeet-dhandha · 2023-09-02T09:17:05Z

@cubuspl42 Let @Pujan92 have this one 👍 .

jeet-dhandha · 2023-09-02T09:21:24Z

I wanted know how we can allow CORPORATE type Workspace/

Pujan92 · 2023-09-02T09:27:03Z

I wanted know how we can allow CORPORATE type Workspace/

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

Pujan92 · 2023-09-02T09:36:35Z

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.

Screen.Recording.2023-09-02.at.15.01.21.mp4

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

cubuspl42 · 2023-09-04T07:33:13Z

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 🎀 👀 🎀

Pujan92 · 2023-09-19T15:58:08Z

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

JmillsExpensify · 2023-09-21T03:10:24Z

Ok great. Looking

JmillsExpensify · 2023-09-21T03:20:23Z

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 · 2023-09-21T03:21:20Z

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 · 2023-09-21T11:20:02Z

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 · 2023-09-21T14:41:06Z

@cubuspl42 PR is ready for review!

JmillsExpensify · 2023-09-25T01:25:44Z

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!

melvin-bot · 2023-10-05T07:52:04Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @Pujan92 got assigned: 2023-09-06 10:16:31 Z
when the PR got merged: 2023-10-05 07:52:04 UTC
days elapsed: 20

On to the next one 🚀

cubuspl42 · 2023-10-05T09:14:18Z

One of the reasons it took so long is that the issue was related to Teachers Unite / corporate policies, which require extra setup to test.

melvin-bot · 2023-10-09T22:22:07Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-10-09T22:22:10Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.79-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

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

If no regressions arise, payment will be issued on 2023-10-16. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@cubuspl42 requires payment offer (Reviewer)
@Pujan92 requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

JmillsExpensify · 2023-10-10T16:02:18Z

@cubuspl42 I think we should add a regression test so that this doesn't happen again. Do you agree and mind suggesting one?

JmillsExpensify · 2023-10-10T16:04:20Z

Based on the time between assignment and merge, here's the payment summary:

Issue reporter: N/A
Contributor: @Pujan92 $500
Contributor+: @cubuspl42 $500 (no payment due; via agency)

cc @Julesssss

cubuspl42 · 2023-10-11T08:01:45Z

@JmillsExpensify

@cubuspl42 I think we should add a regression test so that this doesn't happen again. Do you agree and mind suggesting one?

I don't have a strong opinion about adding the regression test. Here are the testing steps, if we decide to add one:

Navigate to the Teachers Unite page here and select the "I am a teacher" option.
Input the principal's information and an e-mail address you have access to, as the approval request will be sent to this address.
Proceed to the "Teachers Unite!" workspace and divide the bill using the Actions (+) button.
Open a separate tab and approve the request using the Principal account ID you previously logged in with.
On the teacher's side, inspect the IOU report and confirm that the delete option is not displayed in the context menu for the approved request.
Click on the request to view its details. Ensure that the three-dot menu is not visible in the money request header.

JmillsExpensify · 2023-10-16T17:47:09Z

I don't think this is specific to the TU policy, but instead to all approved requests. That said, I can make those tweaks in the regression test. Now that @Pujan92 has been paid out, I'm going to close this issue.

JmillsExpensify added Daily KSv2 NewFeature Something to build that is a new item. labels Aug 31, 2023

JmillsExpensify self-assigned this Aug 31, 2023

melvin-bot bot assigned NicMendonca Aug 31, 2023

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 31, 2023

JmillsExpensify added the External Added to denote the issue can be worked on by a contributor label Aug 31, 2023

melvin-bot bot changed the title ~~Approved requests shouldn't have a delete option~~ [$500] Approved requests shouldn't have a delete option Aug 31, 2023

JmillsExpensify unassigned NicMendonca Aug 31, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 31, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 31, 2023

melvin-bot bot assigned cubuspl42 Aug 31, 2023

melvin-bot bot assigned Julesssss Sep 4, 2023

Pujan92 mentioned this issue Sep 21, 2023

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

Merged

59 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 21, 2023

JmillsExpensify changed the title ~~[HOLD - report archived bug][$500] Approved requests shouldn't have a delete option~~ [$500] Approved requests shouldn't have a delete option Sep 25, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 9, 2023

melvin-bot bot changed the title ~~[$500] Approved requests shouldn't have a delete option~~ [HOLD for payment 2023-10-16] [$500] Approved requests shouldn't have a delete option Oct 9, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Oct 9, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 Daily KSv2 labels Oct 16, 2023

JmillsExpensify closed this as completed Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JmillsExpensify commented Aug 31, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Aug 31, 2023

jeet-dhandha commented Aug 31, 2023 •

edited

Loading

melvin-bot bot commented Aug 31, 2023

melvin-bot bot commented Aug 31, 2023

melvin-bot bot commented Aug 31, 2023

Pujan92 commented Aug 31, 2023

cubuspl42 commented Sep 1, 2023

JmillsExpensify commented Sep 1, 2023

cubuspl42 commented Sep 2, 2023

jeet-dhandha commented Sep 2, 2023

Pujan92 commented Sep 2, 2023

jeet-dhandha commented Sep 2, 2023

jeet-dhandha commented Sep 2, 2023

Pujan92 commented Sep 2, 2023

Pujan92 commented Sep 2, 2023

cubuspl42 commented Sep 4, 2023

Pujan92 commented Sep 19, 2023

JmillsExpensify commented Sep 21, 2023

JmillsExpensify commented Sep 21, 2023

JmillsExpensify commented Sep 21, 2023

Pujan92 commented Sep 21, 2023

Pujan92 commented Sep 21, 2023

JmillsExpensify commented Sep 25, 2023

melvin-bot bot commented Oct 5, 2023

cubuspl42 commented Oct 5, 2023

melvin-bot bot commented Oct 9, 2023

melvin-bot bot commented Oct 9, 2023

JmillsExpensify commented Oct 10, 2023

JmillsExpensify commented Oct 10, 2023 •

edited

Loading

cubuspl42 commented Oct 11, 2023

JmillsExpensify commented Oct 16, 2023

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

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

Comments

JmillsExpensify commented Aug 31, 2023 • edited by melvin-bot bot Loading

melvin-bot bot commented Aug 31, 2023

jeet-dhandha commented Aug 31, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Aug 31, 2023

melvin-bot bot commented Aug 31, 2023

melvin-bot bot commented Aug 31, 2023

Pujan92 commented Aug 31, 2023

Proposal

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

What is the root cause of that problem?

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

cubuspl42 commented Sep 1, 2023

JmillsExpensify commented Sep 1, 2023

cubuspl42 commented Sep 2, 2023

jeet-dhandha commented Sep 2, 2023

Pujan92 commented Sep 2, 2023

jeet-dhandha commented Sep 2, 2023

jeet-dhandha commented Sep 2, 2023

Pujan92 commented Sep 2, 2023

Pujan92 commented Sep 2, 2023

cubuspl42 commented Sep 4, 2023

Pujan92 commented Sep 19, 2023

JmillsExpensify commented Sep 21, 2023

JmillsExpensify commented Sep 21, 2023

JmillsExpensify commented Sep 21, 2023

Pujan92 commented Sep 21, 2023

Pujan92 commented Sep 21, 2023

JmillsExpensify commented Sep 25, 2023

melvin-bot bot commented Oct 5, 2023

cubuspl42 commented Oct 5, 2023

melvin-bot bot commented Oct 9, 2023

melvin-bot bot commented Oct 9, 2023

JmillsExpensify commented Oct 10, 2023

JmillsExpensify commented Oct 10, 2023 • edited Loading

cubuspl42 commented Oct 11, 2023

JmillsExpensify commented Oct 16, 2023

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

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

JmillsExpensify commented Aug 31, 2023 •

edited by melvin-bot bot

Loading

jeet-dhandha commented Aug 31, 2023 •

edited

Loading

JmillsExpensify commented Oct 10, 2023 •

edited

Loading