[$1000] Web - Delete Button Fails to Delete Changed Description in IOU

[kbecciv]

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 desired amount and email address for Request Money
3. Open the IOU and change the description
4. Click on the three dots and select "Delete Request"
5. Observe that the Request is deleted, but the changed description remains in the IOU

Expected Result:

The Delete button should delete the changed description from the IOU as well.

Actual Result:

The Delete button fails to delete the changed description inside the IOU.

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.57-5
Reproducible in staging?: y
Reproducible in production?: y
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-17T112708.989.webm

Recording.4040.mp4

Expensify/Expensify Issue URL:
Issue reported by: @tewodrosGirmaA
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692252491955339

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01462271c6de352d0a
• Upwork Job ID: 1696152844806086656
• Last Price Increase: 2023-09-18

[melvin-bot]

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

[melvin-bot]

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

[CortneyOfstad]

Was able to recreate, as shown in the screenshots below. Getting eyes on this!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01462271c6de352d0a

[melvin-bot]

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

[melvin-bot]

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

[WaqasIbrahim]

This seems like an internal issue. I sent two requests.
Without description change:

With description change:

Look at the value of hasOutstandingIOU in both cases.

[dukenv0307]

Proposal

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

The Delete button fails to delete the changed description inside the IOU.

What is the root cause of that problem?

In these lines, we have the logic to check whether we should delete the transaction threads, we'll delete it if there's no visible message. However, we're not excluding the MODIFIEDEXPENSE action when calculating that visible message. So if there's any MODIFIEDEXPENSE action, the transaction thread will not be deleted and the actions will still be visible.

It doesn't make sense to keep all the MODIFIEDEXPENSE actions once the transaction thread is deleted by the user, so we should also delete the transaction thread if it contains only MODIFIEDEXPENSE actions.

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

We need to exclude the MODIFIEDEXPENSE report actions when calculating the last message here

App/src/libs/actions/IOU.js

Line 1072 in 64ff471

const shouldDeleteTransactionThread = transactionThreadID ? ReportActionsUtils.getLastVisibleMessage(transactionThreadID).lastMessageText.length === 0 : false;

.

This can be done by, for example, adding a shouldExclude callback to the getLastVisibleAction and getLastVisibleMessage. The default value of shouldExclude in those definitions will be shouldExclude = () => false

And then we can update this line to

const visibleActions = _.filter(actions, (action) => shouldReportActionBeVisibleAsLastAction(action) && !shouldExclude(action);

We'll use it to filter out the MODIFIEDEXPENSE actions here like

const shouldDeleteTransactionThread = transactionThreadID ? ReportActionsUtils.getLastVisibleMessage(transactionThreadID, {}, ReportActionsUtils.isModifiedExpenseAction).lastMessageText.length === 0 : false;

Similarly, we need to update the usage of getLastVisibleAction, getLastVisibleMessage in that file for consistency.

If it's desirable to delete all MODIFIEDEXPENSE actions even though there're other non-MODIFIEDEXPENSE actions in the list, we can remove them in the optimisticData here as well.

The back-end will need to be fixed similarly

What alternative solutions did you explore? (Optional)

There're other ways to do the above besides shouldExclude, like adding shouldExcludeModifiedExpense boolean, or create a new set of functions getLastVisibleAction, getLastVisibleMessage similar to to use in that delete money request case.

Details on the shouldExcludeModifiedExpense solution:

1. Adding a shouldExcludeModifiedExpense boolean param to the getLastVisibleAction and getLastVisibleMessage. The default value of shouldExcludeModifiedExpense in those definitions will be false

2. Update this line to

const visibleActions = _.filter(actions, (action) => shouldReportActionBeVisibleAsLastAction(action) && !(shouldExcludeModifiedExpense && isModifiedExpenseAction(action)));

so it won't include the modified expense action if the shouldExcludeModifiedExpense is true.

3. We'll use it to filter out the MODIFIEDEXPENSE actions here (by passing shouldExcludeModifiedExpense as true)
   Same for the usage of getLastVisibleAction, getLastVisibleMessage in that file for consistency.

The back-end will also need to be fixed to delete the transaction thread in that case.

[davewish]

Proposal

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

The Delete request button fails to delete the changed description inside the IOU.

What is the root cause of that problem?

The transactionThread is deleted if there are not visible comments in thread . incase of ModifiedExpense actionName and has comment or message , hence shouldDeleteTransactionThread becomes false

App/src/libs/actions/IOU.js

Lines 1075 to 1078 in cf1d50a

	// 1. Delete the transactionThread - delete if there are no visible comments in the thread
	// 2. Update the moneyRequestPreview to show [Deleted request] - update if the transactionThread exists AND it isn't being deleted
	const shouldDeleteTransactionThread = transactionThreadID ? ReportActionsUtils.getLastVisibleMessage(transactionThreadID).lastMessageText.length === 0 : false;
	const shouldShowDeletedRequestMessage = transactionThreadID && !shouldDeleteTransactionThread;

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

we have to exclude the ModifiedExpense from visibleActions in getLastVisibleAction ``
const visibleActions = _.filter(actions, (action) => shouldReportActionBeVisibleAsLastAction(action) && !isModifiedExpense(action));

What alternative solutions did you explore? (Optional)

N/A

[redpanda-bit]

I think we should remove the reportActions of actionName === "MODIFIEDEXPENSE" instead of making them invisible. Keeping them will result in accumulating unnecessary data in the FE.

I also think deleting the change description actions alone will result in the FE adjusting the report to the desired experience.

[binary3oul]

Proposal

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

Delete Button Fails to Delete Changed Description in IOU

What is the root cause of that problem?

When request is deleted, the function that filters action with the name "MODIFIEDEXPENSE" is missing.

App/src/pages/home/report/ReportActionsView.js

Line 186 in 64ff471

sortedReportActions={props.reportActions}

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

So, I updated the that code line as shown below and solved this issue simply.

sortedReportActions={props.reportActions.filter(e => !ReportActionsUtils.isDeletedAction (ReportActionsUtils.getParentReportAction(props.report)) || e.actionName !== CONST.REPORT.ACTIONS.TYPE.MODIFIEDEXPENSE)}

What alternative solutions did you explore? (Optional)

N/A

[mollfpr]

I'll review the proposals in the morning, thanks!

[CortneyOfstad]

@mollfpr Just checking to see if you had time to check the proposals? TIA!

[mollfpr]

Hi @CortneyOfstad, I'm in the middle of testing the proposals now. For an update now, @davewish proposal is similar to what @dukenv0307 proposed.

, and for @binary3oul

Clear the contents of the following directory path: Application\IndexedDB\OnyxDB\keyvaluepairs.

I don't understand what this step has to do with the solution.

[binary3oul]

We can ignore that step :)
Alternatively, we can sign out and login again.
This operation is for clearing Onyx data.

[mollfpr]

@binary3oul I tried your solution, but it deletes the thread regardless of whether it only has a comment.

App/src/libs/actions/IOU.js

Line 1075 in 5db2445

// 1. Delete the transactionThread - delete if there are no visible comments in the thread

The thread shouldn't be deleted.

[mollfpr]

@dukenv0307 Could you elaborate on the solution for adding shouldExcludeModifiedExpense? I think, for now, we are good with handling the filter in getLastVisibleAction instead of passing the function as a parameter.

[bernhardoj]

Sharing my thoughts on this issue.

I think we should get a confirmation from the team on whether we should delete the transaction thread or not if the thread only contains MODIFIEDEXPENSE actions because from my testing, looks like BE keep the thread. It would be weird if we optimistically delete the thread on the FE but we actually still have the thread on the BE.

How do we know if BE doesn't delete the thread? Assuming we delete the thread, here are the steps I use:

1. Open the transaction thread. Make sure you modify the request/transaction.
2. Note down the report id of the thread
3. Delete the transaction. The thread will be deleted in FE.
4. Open back the thread using the report id you noted down in step 2

expected: if BE also deletes the thread, we will get not found page
actual: you will have access again to the thread

Compare the behavior when we delete the transaction thread with empty actions.

[binary3oul]

Updated proposal

[tsa321]

Proposal

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

The Delete button fails to delete the changed description inside the IOU.

What is the root cause of that problem?

Because IOU's thread delete check is based on last message of the thread and without checking the type of the message.

In this line:

App/src/libs/actions/IOU.js

Line 1072 in 64ff471

const shouldDeleteTransactionThread = transactionThreadID ? ReportActionsUtils.getLastVisibleMessage(transactionThreadID).lastMessageText.length === 0 : false;

shouldDeleteTransactionThread should be true to delete IOU's thread, but currently it detect on the last message of IOU thread without checking the type of the message. So it will contains false while reproducing this issue.

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

My solution is to use already available function library : ReportActionsUtils.hasCommentThread or use lodashGet(reportAction, 'childVisibleActionCount', 0) to determine if the thread has comments (manually generated comment and not automatic information comment). It checks whether the thread has comment (the real comment that user posted on thread). So the code line above will be modified into:

const shouldDeleteTransactionThread = reportAction ? !ReportActionsUtils.hasCommentThread(reportAction): false;

or

const shouldDeleteTransactionThread = lodashGet(reportAction, 'childVisibleActionCount', 0) < 1

What alternative solutions did you explore? (Optional)

commenterCount field of reportAction could be used to determine the value of shouldDeleteTransactionThread

[melvin-bot]

📣 @tsa321! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[tsa321]

Contributor details
Your Expensify account email: tsaiinkwa@yahoo.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01bfc26d267bade652

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot]

@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[mollfpr]

The PR is still in review.

[melvin-bot]

@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?

[CortneyOfstad]

@mollfpr update on the PR? Thank you!

[melvin-bot]

@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

[jasperhuangg]

The linked PR is still under review. #28995

Some changes have occurred to the associated back-end behavior, so the PR has had to be updated accordingly.

[CortneyOfstad]

Thanks @jasperhuangg!

[mollfpr]

@CortneyOfstad This specific issue is not reproducible anymore because the thread report is now deleted when it has no chat message.

We still show the edit expense message after the request is deleted. I don't know if this was intended, but I remember not showing the message before.

cc @jasperhuangg @dukenv0307

[jasperhuangg]

I asked for clarification on how to proceed, will let you know what the decision is.

[melvin-bot]

@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[jasperhuangg]

a PR isn't necessary here anymore, but @dukenv0307 and @mollfpr have spent a considerable amount of time with investigating this issue. Even though we didn't end up merging any code, I think we should move ahead with paying them out for the time they've spent on this.

@CortneyOfstad can we be sure that this happens?

[CortneyOfstad]

@jasperhuangg I think that would be fair. Do you have a price in mind? Or would you be more comfortable opening it up to the BZ for a broader discussion?

[jasperhuangg]

I already posted here in BZ without any response yet.

Definitely think we should see if we can get some more eyes on that thread.

[dukenv0307]

@jasperhuangg @CortneyOfstad For this case I think the payment should be the full price since we spent a considerable amount of time with investigating this issue. We have some same cases in the part here #27570 (comment) and #30692 (comment)

[mallenexpensify]

For #27570 (comment)

A proposal was #27570 (comment) and while implementing the #29259, it was decided that the issue would be best solved on the backend.

and our internal doc states

We also pay 100% to the contributor and C+ if we hired them then decided to fix the issue internally or close the issue (assuming the contributor and C+ were both actively working on the issue).

This issue is different because we didn't make a decision to fix it on the backend.

For #30692 (comment)
Both Jasper and Vivek agreed to close the issue, so that's also different than issue and also fits with the stated guideline above.

Part of what leads me to believe 50% is fair is because the hire date was Oct 5 and the PR was still being worked on as of Dec 28th and there were multiple bumps to move the PR along. If this PR would have been merged before the one that fixed it, would it have made a difference? (I don't know). If so, then we could have saved time both working on this one AND the other by closing them both sooner.

We appreciate and value the time and investment, our main goal is to get bugs fixed and to do it with urgency, which is what I consider the a benchmark for similar issues like there where another PR (appears to) magically fix the bug (from what I know, I'm not the most well versed on this specific issue or PR)

[dukenv0307]

the hire date was #25997 (comment) and the PR was still being worked on as of #28995 (comment) and there were multiple bumps to move the PR along.

@mallenexpensify I believe part of the reason for the delay is this is a fairly complex issue, we faced many technical challenges during the implementation and tried to make sure to resolve all of them (~50 commits) before merging to not cause regressions/affect the app performance.

So I'd appreciate if we can keep the full payment here since the PR is mostly ready for merging before it was made outdated due to another change. Also the reason why the other PR was able to resolve the bugs is not because that PR resolves the technical issues we faced, just that they achieved a different expectation that conflicts with the expectations in this issue so this issue is no longer reproducible.

If this PR would have been merged before the one that fixed it, would it have made a difference? (I don't know)

@mallenexpensify regarding this, it will not make a difference since the 2 PRs have 2 conflicting expectations, if this PR is merged first, the other PR would still override the expectation in this PR.

[mallenexpensify]

Thanks @dukenv0307 , we're discussing internally.

[jasperhuangg]

@dukenv0307 @mollfpr Thank you for your patience with this matter. We've discussed this internally and have decided to keep full payment on your work considering the sheer amount of time and effort you've put into implementation and review. Thank you for sticking with it, we really appreciate your hard work!

[jasperhuangg]

@CortneyOfstad can you help with issuing payment here?

[CortneyOfstad]

Created a new Job post here — https://www.upwork.com/jobs/~016e3532aa6b8d64fd

@dukenv0307 sent you a proposal in Upwork

@mollfpr you'll be paid via NewDot — I'll create the proposal once Duc accepts in Upwork 👍

[dukenv0307]

@dukenv0307 sent you a proposal in Upwork

@CortneyOfstad I've accepted, thank you!

[tewodrosGirmaA]

@CortneyOfstad @jasperhuangg May I inquire about my eligibility for compensation regarding the bug report? Thank you.

[mallenexpensify]

ah.. yes, @tewodrosGirmaA you are eligible since this was reported forever ago, ha. Amount is $250 because it was before the decrease to $50. .
@CortneyOfstad can you pay out plz?

[CortneyOfstad]

Sorry about that @tewodrosGirmaA! I sent you a proposal in Upwork — let me know once you accept and I'll get that paid ASAP. Thanks!

[tewodrosGirmaA]

Hello @CortneyOfstad i accept it , Thank you

[CortneyOfstad]

Payment Summary

@tewodrosGirmaA (reporter) — paid $250 via Upwork
@dukenv0307 (contributor) — paid $1000 via Upwork
@mollfpr (C+) — to be paid $1000 via NewDot

[tewodrosGirmaA]

Thank you @CortneyOfstad

[JmillsExpensify]

$1,000 approved for @mollfpr for based on summary above.