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.

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

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [$500] IOU - Deletion Button Remains Visible After Changing Date after Delete the Requested Money #26019

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 27, 2023 · 64 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 27, 2023

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


Action Performed:

  1. Click on the plus sign and select "Request Money"
  2. Enter the desired amount and email address for the money request
  3. Go to the IOU and edit the Date
  4. Click on the three dots and select "Delete"
  5. Observe that the deletion button and three dots still exist, and if we delete again, an unexpected error occurs in IOU

Expected Result:

he deletion button and three dots should disappear after deleting the requested money in any case

Actual Result:

The deletion button remains visible after deleting the requested money and changing the date, which causes an unexpected error if we try to delete it again

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • Windows / Chrome
  • MacOS / Desktop

Version Number: 1.3.57-5

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

screen-capture.-.2023-08-18T015705.366.mp4
Recording.2996.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Tewodros-GirmaA

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692348919083289

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019e57dec696456a6d
  • Upwork Job ID: 1699228556598366208
  • Last Price Increase: 2023-09-20
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 27, 2023

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 27, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@namhihi237
Copy link
Contributor

Proposal

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

The deletion button and three dots should disappear after deleting the requested money in any case.

What is the root cause of that problem?

We have conditions to show MoneyRequestHeader here:

if (isSingleTransactionView && !isDeletedParentAction) {
headerView = (
<MoneyRequestHeader

Here we are using isDeletedParentAction to check if the parent of the report has been deleted.

const isDeletedParentAction = ReportActionsUtils.isDeletedParentAction(parentReportAction);

But in this function return true when isDeletedParentAction = true and no child action.

function isDeletedParentAction(reportAction) {
return lodashGet(reportAction, ['message', 0, 'isDeletedParentAction'], false) && lodashGet(reportAction, 'childVisibleActionCount', 0) > 0;
}

Since the message changes the date or change description, the amount we don't count as 1 childVisibleAction, because this function will return false when there is no childVisibleActionCount.

There is also 1 case, when we have a comment in the thread. we delete request money for the first time this function will return true, and then remove the comment this function returns false. Leads to show MoneyRequestHeader.

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

Here we should use isDeletedAction instead of isDeletedParentAction.

const isDeletedParentAction = ReportActionsUtils.isDeletedAction(parentReportAction);

What alternative solutions did you explore? (Optional)

Also, I think we should also update the getTransactionReportName function here

 if (ReportActionsUtils.isDeletedAction(reportAction)) {

@melvin-bot melvin-bot bot added the Overdue label Aug 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2023

@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

@anmurali
Copy link

Will reproduce and slap the label on tomorrow

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

@anmurali Huh... This is 4 days overdue. Who can take care of this?

@anmurali
Copy link

anmurali commented Sep 6, 2023

Adding an external label

@melvin-bot melvin-bot bot removed the Overdue label Sep 6, 2023
@anmurali anmurali added External Added to denote the issue can be worked on by a contributor Overdue labels Sep 6, 2023
@melvin-bot melvin-bot bot changed the title IOU - Deletion Button Remains Visible After Changing Date after Delete the Requested Money [$500] IOU - Deletion Button Remains Visible After Changing Date after Delete the Requested Money Sep 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

Job added to Upwork: https://www.upwork.com/jobs/~019e57dec696456a6d

@anmurali anmurali removed the Overdue label Sep 6, 2023
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

Current assignee @anmurali is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

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

@dukenv0307
Copy link
Contributor

Proposal

Bring a proposal from this issue #24413

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

Request money - Delete request reopens on deleting message in it and again deleting it triggers red dot error

What is the root cause of that problem?

We have two errors here

  1. Inconsistent header when the request money is deleted or not

When the request money is deleted, the header render HeaderView because isDeletedParentAction is true. That makes the avatar can clickable, and after clicking on this the detail page appears

if (isSingleTransactionView && !isDeletedParentAction) {
headerView = (
<MoneyRequestHeader

  1. The title of the header is wrong when we delete the message of the request money report

In getTransactionReportName function, we use ReportActionsUtils.isDeletedParentAction function to check whether the report action is deleted or not

App/src/libs/ReportUtils.js

Lines 1258 to 1260 in dd0d98f

if (ReportActionsUtils.isDeletedParentAction(reportAction)) {
return Localize.translateLocal('parentReportAction.deletedRequest');
}

When we delete the only message of the report, because childVisibleActionCount is updated to 0, this function returns false and then the title of the header is wrong

return lodashGet(reportAction, ['message', 0, 'isDeletedParentAction'], false) && lodashGet(reportAction, 'childVisibleActionCount', 0) > 0;

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

  1. We should always display MoneyRequestHeader for transaction view to make the avatar cannot clickable.
if (isSingleTransactionView) {
    headerView = (
        <MoneyRequestHeader
        ....

if (isSingleTransactionView && !isDeletedParentAction) {
headerView = (
<MoneyRequestHeader

And in MoneyRequestHeader we will hide the three-dot if the money request is deleted

shouldShowThreeDotsButton={!isPayer && !isSettled && !ReportActionsUtils.isDeletedAction(parentReportAction)}

shouldShowThreeDotsButton={!isPayer && !isSettled}

2. In getTransactionReportName, we should use ReportActionsUtils.isDeletedAction to check whether the action is deleted or not instead of using isDeletedParentAction function

if (ReportActionsUtils.isDeletedAction(reportAction)) {
    return Localize.translateLocal('parentReportAction.deletedRequest');
}

App/src/libs/ReportUtils.js

Lines 1258 to 1260 in dd0d98f

if (ReportActionsUtils.isDeletedParentAction(reportAction)) {
return Localize.translateLocal('parentReportAction.deletedRequest');
}

What alternative solutions did you explore? (Optional)

NA

Result

Screencast.from.11-08-2023.18.30.35.webm

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

@anmurali, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

@anmurali, @allroundexperts Huh... This is 4 days overdue. Who can take care of this?

@tsa321
Copy link
Contributor

tsa321 commented Sep 12, 2023

Similar to #25997

@dukenv0307
Copy link
Contributor

It think this is not the same.

Copy link

melvin-bot bot commented Nov 3, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Nov 7, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@joelbettner
Copy link
Contributor

@anmurali I'll let you field this question: #26019 (comment)

Sorry, @dhanashree-sawant I'm not involved with the payouts, so I don't know.

@melvin-bot melvin-bot bot removed the Overdue label Nov 7, 2023
@dhanashree-sawant
Copy link

Sure that's okay, @anmurali can you check it once you are available?

@melvin-bot melvin-bot bot added the Overdue label Nov 10, 2023
Copy link

melvin-bot bot commented Nov 13, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Nov 13, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Nov 13, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

@allroundexperts
Copy link
Contributor

Checklist

  1. Fix: Approved requests shouldn't have a delete option #27952
  2. https://github.com/Expensify/App/pull/27952/files#r1391750954
  3. I don't think that anything in the checklist could have prevented this. This should have been caught in manual testing. As such, Slack discussion is not needed.
  4. Regression test would be helpful. The steps given in the OP are clear enough and should be good.

@melvin-bot melvin-bot bot removed the Overdue label Nov 13, 2023
@anmurali
Copy link

anmurali commented Nov 14, 2023

Payment Summary

@allroundexperts
Copy link
Contributor

@anmurali This one is eligible for the 50% bonus. The PR was created before Oct 24, and was merged in just a single day.

@dhanashree-sawant
Copy link

Thanks @anmurali for going through that issue and awarding reporting bonus to me. Actually, this as well as #24413 is reported before 30 August, can you send us 250$ offer for it? (If it is decided to provide 50$ each as we are awarding 2 reporters reporting bonus then that's okay)

@tewodrosGirmaA
Copy link

Hello, @anmurali. This matter was reported prior to August 30th. Could you please provide us with a $250 offer

@JmillsExpensify
Copy link

$750 payment approved for @allroundexperts based on this comment.

@melvin-bot melvin-bot bot added the Overdue label Nov 17, 2023
@dukenv0307
Copy link
Contributor

@JmillsExpensify @anmurali Do I need to apply for the job in Upwork? If yes, please send me the Upwork link so I can accept this. Thanks.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 17, 2023
Copy link

melvin-bot bot commented Nov 20, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot removed the Overdue label Nov 20, 2023
@tewodrosGirmaA
Copy link

@anmurali Thank you, I accepted it

@melvin-bot melvin-bot bot added the Overdue label Nov 23, 2023
Copy link

melvin-bot bot commented Nov 24, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Nov 28, 2023

@joelbettner, @anmurali, @allroundexperts, @dukenv0307 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@anmurali
Copy link

Paid.

@melvin-bot melvin-bot bot removed the Overdue label Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests