[HOLD for payment 2024-08-09] [$250] Invoice - There is "Delete expense" option in the invoice paid system message #45632

izarutskaya · 2024-07-17T20:29:40Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.7-4
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4728016
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Precondition:

User has received invoice.
The following steps will be performed by the invoice receiver.

Go to staging.new.expensify.com
Go to invoice chat.
Click on the invoice preview.
Click on the pay button.
Pay the invoice.
Right click on the paid system message.

Expected Result:

There should be no "Delete expense" option in the invoice paid system message.

Actual Result:

There is "Delete expense" option in the invoice paid system message. The invoice cannot be deleted.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Bug6544144_1721146172425.20240717_000621.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a731bc49d6a020e0
Upwork Job ID: 1814354277180193004
Last Price Increase: 2024-07-19
Automatic offers:

FitseTLT | Contributor | 103229523

Issue Owner

Current Issue Owner: @CortneyOfstad

melvin-bot · 2024-07-17T20:29:50Z

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

izarutskaya · 2024-07-17T20:30:10Z

We think this issue might be related to the #collect project.

FitseTLT · 2024-07-17T23:33:02Z

Proposal

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

Invoice - There is "Delete expense" option in the invoice paid system message

What is the root cause of that problem?

As the linkedReport is an invoice type canAddOrDeleteTransactions will not be executed and true will be returned here

App/src/libs/ReportUtils.ts

Lines 1632 to 1635 in 7ba66a1

    
           if (!isEmptyObject(linkedReport) && isMoneyRequestReport(linkedReport)) { 
        
               return canAddOrDeleteTransactions(linkedReport); 
        
           } 
        
           return true;

which will make the report action to be deleteable

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

we should disallow delete menu for all pay type IOU report actions by changing

App/src/libs/ReportUtils.ts

Lines 1624 to 1628 in 7ba66a1

    
           const isSplitAction = ReportActionsUtils.getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT; 
        
           if (isSplitAction) { 
        
               return false; 
        
           }

to

const isSplitAction = ReportActionsUtils.getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT;
        const isPayAction = ReportActionsUtils.getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.PAY;

        if (isSplitAction || isPayAction) {
            return false;
        }

We can also include other REPORT_ACTION_TYPE if needed

We can narrow down the logic by applying it to pay actions of only invoice reports isInvoiceReport(linkedReport)

What alternative solutions did you explore? (Optional)

We can change

App/src/libs/ReportUtils.ts

Line 1635 in 7ba66a1

return true;

to

return !isInvoiceReport(linkedReport);

or

return false;

alternatively change

App/src/libs/ReportUtils.ts

Line 1632 in 7ba66a1

if (!isEmptyObject(linkedReport) && isMoneyRequestReport(linkedReport)) {

to

            if (!isEmptyObject(linkedReport) && (isMoneyRequestReport(linkedReport) || isInvoiceReport(linkedReport))) {

bernhardoj · 2024-07-18T08:34:14Z

Proposal

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

There is an option to delete expense on a paid system message on an invoice report.

What is the root cause of that problem?

The option to delete will show if canDeleteReportAction returns true.

App/src/libs/ReportUtils.ts

Lines 1616 to 1637 in f392cd3

    
           function canDeleteReportAction(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string): boolean { 
        
               const report = getReportOrDraftReport(reportID); 
        
               const isActionOwner = reportAction?.actorAccountID === currentUserAccountID; 
        
               const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`] ?? null; 
        
               if (ReportActionsUtils.isMoneyRequestAction(reportAction)) { 
        
                   // For now, users cannot delete split actions 
        
                   const isSplitAction = ReportActionsUtils.getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT; 
        
                   if (isSplitAction) { 
        
                       return false; 
        
                   } 
        
                   const linkedReport = isThreadFirstChat(reportAction, reportID) ? getReportOrDraftReport(report?.parentReportID) : report; 
        
                   if (isActionOwner) { 
        
                       if (!isEmptyObject(linkedReport) && isMoneyRequestReport(linkedReport)) { 
        
                           return canAddOrDeleteTransactions(linkedReport); 
        
                       } 
        
                       return true; 
        
                   } 
        
               }

For money request action, if the money request action belongs to a money request report (IOU/expense), then it will check using canAddOrDeleteTransactions, otherwise, it will straightly returns true.

In our case, it's not either an IOU/expense report, but an invoice report, so canDeleteReportAction always returns true for the paid system message (which is also an IOU action) which shows the option to delete the message.

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

We can simplify the condition by following the condition from the details page as shown below:

App/src/pages/ReportDetailsPage.tsx

Line 195 in f392cd3

    
           const canDeleteRequest = isActionOwner && (ReportUtils.canAddOrDeleteTransactions(moneyRequestReport) || isSelfDMTrackExpenseReport) && !isDeletedParentAction;

App/src/libs/ReportUtils.ts

Lines 1631 to 1636 in f392cd3

    
           if (isActionOwner) { 
        
               if (!isEmptyObject(linkedReport) && isMoneyRequestReport(linkedReport)) { 
        
                   return canAddOrDeleteTransactions(linkedReport); 
        
               } 
        
               return true; 
        
           }

to

return isActionOwner && (canAddOrDeleteTransactions(linkedReport) || isSelfDM(linkedReport)) && !ReportActionsUtils.isDeletedAction(reportAction);

we actually don't need isDeletedAction because once we delete an aciont, we can't interact with it anymore, even while offline, but I'm gonna leave that

melvin-bot · 2024-07-19T17:39:05Z

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

melvin-bot · 2024-07-19T17:39:10Z

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

CortneyOfstad · 2024-07-19T17:43:15Z

@jayeshmangwani we have a couple of proposals above when you have a chance — thank you!

jayeshmangwani · 2024-07-22T06:19:01Z

@FitseTLT I see you are suggesting 3-4 alternate changes. Can you summarize the best condition to use to avoid displaying the 'Delete expense' option on the system message?`

we should disallow delete menu for all pay type IOU report actions by changing

We can also include other REPORT_ACTION_TYPE if needed

We can narrow down the logic by applying it to pay actions of only invoice reports isInvoiceReport(linkedReport)

We can change

I think any of the above four options will solve this bug, right? If yes, which one do you think is best to add?

jayeshmangwani · 2024-07-22T06:23:54Z

@bernhardoj Can you please break down for us how this issue will be solved if we use the same condition from the details page?

I see you are suggesting removing the checks for isEmptyObject and isMoneyRequestReport. Will this not cause a regression if we do not check these before calling canAddOrDeleteTransactions?

Lastly, do we need to add an isSelfDM check here? I am not sure if the invoice will be for the self DM.

FitseTLT · 2024-07-22T13:36:25Z

I think any of the above four options will solve this bug, right? If yes, which one do you think is best to add?

@jayeshmangwani yes all are working solutions but if you need the best summarized solution here it is:
Because PAY type report action is a system message which cannot be deleted (whether it is in invoice report or money request report) we can early return false (without a need to accessing the linked report to the report action) like we did here in the SPLIT case

App/src/libs/ReportUtils.ts

Lines 1623 to 1626 in b8b99d0

    
           const isSplitAction = ReportActionsUtils.getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT; 
        
           if (isSplitAction) { 
        
               return false;

So we can change the above to

const isSplitAction = ReportActionsUtils.getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT;
        const isPayAction = ReportActionsUtils.getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.PAY;

        if (isSplitAction || isPayAction) {
            return false;
        }

Alternatively if we want to modify the current logic we can change

App/src/libs/ReportUtils.ts

Lines 1634 to 1635 in b8b99d0

    
               return true; 
        
           }

to

return !isInvoiceReport(linkedReport);

but of course we can also early return false here if it is invoice report as we don't need isActionOwner and other checks for invoice report 👍

bernhardoj · 2024-07-22T14:42:16Z

Can you please break down for us how this issue will be solved if we use the same condition from the details page?

The money request action opens the transaction thread and to delete it, we need to delete it from the details page, so it makes sense to use the same condition for both details and money request action. If you compare the condition, they look similar,

App/src/libs/ReportUtils.ts

Lines 1628 to 1634 in ccf9591

    
           const linkedReport = isThreadFirstChat(reportAction, reportID) ? getReportOrDraftReport(report?.parentReportID) : report; 
        
           if (isActionOwner) { 
        
               if (!isEmptyObject(linkedReport) && isMoneyRequestReport(linkedReport)) { 
        
                   return canAddOrDeleteTransactions(linkedReport); 
        
               } 
        
               return true; 
        
           }

App/src/pages/ReportDetailsPage.tsx

Line 196 in ccf9591

    
           const canDeleteRequest = isActionOwner && (ReportUtils.canAddOrDeleteTransactions(moneyRequestReport) || isSelfDMTrackExpenseReport) && !isDeletedParentAction;

but the canDeleteReportAction is not really good. If isActionOwner is false, then it proceeds with the non-money request action logic instead of returning false. If it's not a money request report, then we always return true.

I see you are suggesting removing the checks for isEmptyObject and isMoneyRequestReport. Will this not cause a regression if we do not check these before calling canAddOrDeleteTransactions?

canAddOrDeleteTransactions already contains the condition for isMoneyRequestReport which will returns false if it's not a money request report compared to the current logic which will return true. If it's an empty object, then isMoneyRequestReport will be false.

App/src/libs/ReportUtils.ts

Lines 1589 to 1592 in ccf9591

    
           function canAddOrDeleteTransactions(moneyRequestReport: OnyxEntry<Report>): boolean { 
        
               if (!isMoneyRequestReport(moneyRequestReport)) { 
        
                   return false; 
        
               }

Lastly, do we need to add an isSelfDM check here? I am not sure if the invoice will be for the self DM.

We need it for the self DM track expense.

jayeshmangwani · 2024-07-23T10:12:54Z

I feel that we should only add the missing isInvoiceReport Check here, as suggested by @FitseTLT in their alternate Proposal

App/src/libs/ReportUtils.ts

Line 1632 in 7ba66a1

if (!isEmptyObject(linkedReport) && isMoneyRequestReport(linkedReport)) {

🎀 👀 🎀 C+ reviewed

melvin-bot · 2024-07-23T10:13:06Z

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

melvin-bot · 2024-07-23T10:30:07Z

📣 @FitseTLT 🎉 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 📖

CortneyOfstad · 2024-07-31T13:43:23Z

No update on the PR

melvin-bot · 2024-08-02T02:11:27Z

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

melvin-bot · 2024-08-02T02:11:31Z

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

Fix - Invoice - There is "Delete expense" option in the invoice paid system message #46080

If no regressions arise, payment will be issued on 2024-08-09. 🎊

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

@jayeshmangwani requires payment through NewDot Manual Requests
@FitseTLT requires payment automatic offer (Contributor)

melvin-bot · 2024-08-02T02:11:32Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@jayeshmangwani] The PR that introduced the bug has been identified. Link to the PR:
[@jayeshmangwani] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@jayeshmangwani] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@jayeshmangwani] Determine if we should create a regression test for this bug.
[@jayeshmangwani] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@CortneyOfstad] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

CortneyOfstad · 2024-08-12T13:22:12Z

@FitseTLT you have been paid!

@jayeshmangwani can you complete the checklist at your earliest convenience?

CortneyOfstad · 2024-08-12T13:22:35Z

Payment Summary

@FitseTLT — paid $250 via Upwork
@jayeshmangwani — to be paid $250 via NewDot

jayeshmangwani · 2024-08-12T14:03:10Z

[@jayeshmangwani] The PR that introduced the bug has been identified. Link to the PR: Implement Invoice rooms with individual receivers #40303
[@jayeshmangwani] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Implement Invoice rooms with individual receivers #40303 (comment)
[@jayeshmangwani] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
[@jayeshmangwani] Determine if we should create a regression test for this bug. Yes
[@jayeshmangwani] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

Precondition: Ensure the user has received an invoice.
Open App -> Go the invoice chat.
Click on the invoice preview.
Click on the Pay button to pay the invoice.
Right click on the Paid system message.
Verify that the Delete Expense option does not appear.

Do we agree 👍 or 👎

jayeshmangwani · 2024-08-12T14:04:28Z

can you complete the checklist at your earliest convenience?

@CortneyOfstad Apologies for the delay; I've completed the checklist now.

CortneyOfstad · 2024-08-14T21:19:54Z

No worries thank you! Payment summary is linked here!

Test rail regression GH linked above, so closing!

jayeshmangwani · 2024-08-15T18:11:38Z

Requested on ND as per #45632 (comment)

JmillsExpensify · 2024-08-21T10:55:12Z

$250 approved for @jayeshmangwani

izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 17, 2024

melvin-bot bot assigned CortneyOfstad Jul 17, 2024

CortneyOfstad added the External Added to denote the issue can be worked on by a contributor label Jul 19, 2024

melvin-bot bot changed the title ~~Invoice - There is "Delete expense" option in the invoice paid system message~~ [$250] Invoice - There is "Delete expense" option in the invoice paid system message Jul 19, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 19, 2024

melvin-bot bot assigned jayeshmangwani Jul 19, 2024

melvin-bot bot assigned grgia Jul 23, 2024

grgia assigned FitseTLT Jul 23, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 23, 2024

FitseTLT mentioned this issue Jul 23, 2024

Fix - Invoice - There is "Delete expense" option in the invoice paid system message #46080

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 23, 2024

melvin-bot bot removed the Weekly KSv2 label Aug 2, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Aug 2, 2024

melvin-bot bot changed the title ~~[$250] Invoice - There is "Delete expense" option in the invoice paid system message~~ [HOLD for payment 2024-08-09] [$250] Invoice - There is "Delete expense" option in the invoice paid system message Aug 2, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Aug 2, 2024

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Aug 8, 2024

melvin-bot bot removed the Overdue label Aug 12, 2024

jayeshmangwani mentioned this issue Aug 12, 2024

Implement Invoice rooms with individual receivers #40303

Merged

50 tasks

melvin-bot bot added the Overdue label Aug 14, 2024

CortneyOfstad closed this as completed Aug 14, 2024

melvin-bot bot removed the Overdue label Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-08-09] [$250] Invoice - There is "Delete expense" option in the invoice paid system message #45632

[HOLD for payment 2024-08-09] [$250] Invoice - There is "Delete expense" option in the invoice paid system message #45632

izarutskaya commented Jul 17, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jul 17, 2024

izarutskaya commented Jul 17, 2024

FitseTLT commented Jul 17, 2024

bernhardoj commented Jul 18, 2024

melvin-bot bot commented Jul 19, 2024

melvin-bot bot commented Jul 19, 2024

CortneyOfstad commented Jul 19, 2024

jayeshmangwani commented Jul 22, 2024

jayeshmangwani commented Jul 22, 2024

FitseTLT commented Jul 22, 2024

bernhardoj commented Jul 22, 2024

jayeshmangwani commented Jul 23, 2024

melvin-bot bot commented Jul 23, 2024

melvin-bot bot commented Jul 23, 2024

CortneyOfstad commented Jul 31, 2024

melvin-bot bot commented Aug 2, 2024

melvin-bot bot commented Aug 2, 2024

melvin-bot bot commented Aug 2, 2024

CortneyOfstad commented Aug 12, 2024

CortneyOfstad commented Aug 12, 2024

jayeshmangwani commented Aug 12, 2024

jayeshmangwani commented Aug 12, 2024

CortneyOfstad commented Aug 14, 2024

jayeshmangwani commented Aug 15, 2024

JmillsExpensify commented Aug 21, 2024

[HOLD for payment 2024-08-09] [$250] Invoice - There is "Delete expense" option in the invoice paid system message #45632

[HOLD for payment 2024-08-09] [$250] Invoice - There is "Delete expense" option in the invoice paid system message #45632

Comments

izarutskaya commented Jul 17, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jul 17, 2024

izarutskaya commented Jul 17, 2024

FitseTLT commented Jul 17, 2024

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)

bernhardoj commented Jul 18, 2024

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?

melvin-bot bot commented Jul 19, 2024

melvin-bot bot commented Jul 19, 2024

CortneyOfstad commented Jul 19, 2024

jayeshmangwani commented Jul 22, 2024

jayeshmangwani commented Jul 22, 2024

FitseTLT commented Jul 22, 2024

bernhardoj commented Jul 22, 2024

jayeshmangwani commented Jul 23, 2024

melvin-bot bot commented Jul 23, 2024

melvin-bot bot commented Jul 23, 2024

CortneyOfstad commented Jul 31, 2024

melvin-bot bot commented Aug 2, 2024

melvin-bot bot commented Aug 2, 2024

melvin-bot bot commented Aug 2, 2024

CortneyOfstad commented Aug 12, 2024

CortneyOfstad commented Aug 12, 2024

Payment Summary

jayeshmangwani commented Aug 12, 2024

Regression Test Proposal

jayeshmangwani commented Aug 12, 2024

CortneyOfstad commented Aug 14, 2024

jayeshmangwani commented Aug 15, 2024

JmillsExpensify commented Aug 21, 2024

izarutskaya commented Jul 17, 2024 •

edited by melvin-bot bot

Loading