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

izarutskaya commented 3 months ago

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?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/8af6bb68-c8e7-40f8-a470-b72bf42c4ce1

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[bot] commented 3 months ago

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 commented 3 months ago

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

FitseTLT commented 3 months ago

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 https://github.com/Expensify/App/blob/7ba66a1542fa815eed5cb4622244bc2935593484/src/libs/ReportUtils.ts#L1632-L1635 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 https://github.com/Expensify/App/blob/7ba66a1542fa815eed5cb4622244bc2935593484/src/libs/ReportUtils.ts#L1624-L1628 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 https://github.com/Expensify/App/blob/7ba66a1542fa815eed5cb4622244bc2935593484/src/libs/ReportUtils.ts#L1635 to

return !isInvoiceReport(linkedReport);

or

return false;

alternatively change https://github.com/Expensify/App/blob/7ba66a1542fa815eed5cb4622244bc2935593484/src/libs/ReportUtils.ts#L1632 to

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

bernhardoj commented 3 months ago

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. https://github.com/Expensify/App/blob/f392cd369131e80b09e1ada8afd239d461257b24/src/libs/ReportUtils.ts#L1616-L1637

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: https://github.com/Expensify/App/blob/f392cd369131e80b09e1ada8afd239d461257b24/src/pages/ReportDetailsPage.tsx#L195

https://github.com/Expensify/App/blob/f392cd369131e80b09e1ada8afd239d461257b24/src/libs/ReportUtils.ts#L1631-L1636 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[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

CortneyOfstad commented 3 months ago

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

jayeshmangwani commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 https://github.com/Expensify/App/blob/b8b99d0a994e3ccf8b91564df0f92a7c440aa6c7/src/libs/ReportUtils.ts#L1623-L1626 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 https://github.com/Expensify/App/blob/b8b99d0a994e3ccf8b91564df0f92a7c440aa6c7/src/libs/ReportUtils.ts#L1634-L1635 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 commented 2 months ago

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, https://github.com/Expensify/App/blob/ccf9591f3c603716afd92a26a11bc137eecd0b5e/src/libs/ReportUtils.ts#L1628-L1634 https://github.com/Expensify/App/blob/ccf9591f3c603716afd92a26a11bc137eecd0b5e/src/pages/ReportDetailsPage.tsx#L196

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. https://github.com/Expensify/App/blob/ccf9591f3c603716afd92a26a11bc137eecd0b5e/src/libs/ReportUtils.ts#L1589-L1592

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 commented 2 months ago

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

https://github.com/Expensify/App/blob/7ba66a1542fa815eed5cb4622244bc2935593484/src/libs/ReportUtils.ts#L1632

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

📣 @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 commented 2 months ago

No update on the PR

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

https://github.com/Expensify/App/pull/46080

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

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[bot] commented 2 months ago

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 commented 2 months ago

@FitseTLT you have been paid!

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

CortneyOfstad commented 2 months ago

Payment Summary

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

jayeshmangwani commented 2 months ago

[x] [@jayeshmangwani] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/40303
[x] [@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: https://github.com/Expensify/App/pull/40303#issuecomment-2284067204
[x] [@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
[x] [@jayeshmangwani] Determine if we should create a regression test for this bug. Yes
[x] [@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 commented 2 months ago

can you complete the checklist at your earliest convenience?

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

CortneyOfstad commented 2 months ago

No worries thank you! Payment summary is linked here!

Test rail regression GH linked above, so closing!

jayeshmangwani commented 2 months ago

Requested on ND as per https://github.com/Expensify/App/issues/45632#issuecomment-2283981817

JmillsExpensify commented 1 month ago

$250 approved for @jayeshmangwani

Expensify / App