[HOLD for payment 2024-07-24] [$250] Expense - Inconsistency in dismissing report details RHP when deleting expense with comments

[lanitochka17]

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.1-10
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Submit an expense to any user with no unsettled expense
3. Go to transaction thread and add some comments
4. Click on the report header > Delete expense > Delete it
5. Note that report details RHP closes
6. Submit two expenses to any user
7. Go to transaction thread of any of the submitted expense
8. Add some comments
9. Click on the report header > Delete expense > Delete it
10. Note that report details RHP does not close

Expected Result:

When deleting expense with comments in the combined report or transaction thread, the behavior of report details RHP (close or not close) should be the same

Actual Result:

In Step 5, when deleting expense with comments in the combined report, report details RHP closes
In Step 10, when deleting expense with comments in the transaction thread, report details RHP does not close

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6523369_1719259702238.20240625_040146.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0129ce6e9afec28c89
• Upwork Job ID: 1805348927110425747
• Last Price Increase: 2024-07-01
• Automatic offers:
• rojiphil | Reviewer | 102947141
• bernhardoj | Contributor | 102947144

Issue Owner

Current Issue Owner: @lschurr

[melvin-bot]

Triggered auto assignment to @jasperhuangg (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

@jasperhuangg FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0129ce6e9afec28c89

[melvin-bot]

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

[dominictb]

Proposal

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

• Expense - Inconsistency in dismissing report details RHP when deleting expense with comments

What is the root cause of that problem?

• After deleting report, we use navigateBackToAfterDelete.current to navigate user to the correct screen:
  

  App/src/pages/ReportDetailsPage.tsx

  Line 554 in 61dab89

  navigateBackToAfterDelete.current = IOU.deleteMoneyRequest(iouTransactionID, requestParentReportAction, true);

  
  

  App/src/pages/ReportDetailsPage.tsx

  Line 636 in 61dab89

  Navigation.goBack(navigateBackToAfterDelete.current);

• When deleting expense with comments in the combined report, this logic is called:
  

  App/src/libs/actions/IOU.ts

  Line 5571 in 61dab89

  if (iouReport && isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) {

  
  so it will dismiss the RHN.

• When deleting expense with comments in the transaction thread, there is no route is returned:
  

  App/src/libs/actions/IOU.ts

  Lines 5570 to 5580 in 61dab89

  	// STEP 7: Navigate the user depending on which page they are on and which resources were deleted
  	if (iouReport && isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) {
  	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
  	return ROUTES.REPORT_WITH_ID.getRoute(iouReport.reportID);
  	}
  	if (iouReport?.chatReportID && shouldDeleteIOUReport) {
  	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
  	return ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID);
  	}
  	}

  
  so the RHN is not dissmissed.

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

• We can update:
  

  App/src/pages/ReportDetailsPage.tsx

  Lines 633 to 635 in 61dab89

  	if (!navigateBackToAfterDelete.current) {
  	return;
  	}

  
  to:

                        if (!navigateBackToAfterDelete.current) {
                            Navigation.goBack(); // or Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(report.reportID))
                            return;
                        }

What alternative solutions did you explore? (Optional)

NA

[nyomanjyotisa]

Proposal

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

Inconsistency in dismissing report details RHP when deleting expense with comments

What is the root cause of that problem?

On this line, the shouldDeleteTransactionThread become false because we add some comments on the transaction thread

App/src/libs/actions/IOU.ts

Line 5290 in 9ee8806

const shouldDeleteTransactionThread = transactionThreadID ? (reportAction?.childVisibleActionCount ?? 0) === 0 : false;

Because of shouldDeleteTransactionThread is false none of this condition executed

App/src/libs/actions/IOU.ts

Lines 5571 to 5579 in 9ee8806

	if (iouReport && isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) {
	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
	return ROUTES.REPORT_WITH_ID.getRoute(iouReport.reportID);
	}
	if (iouReport?.chatReportID && shouldDeleteIOUReport) {
	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
	return ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID);
	}

Thats why navigateBackToAfterDelete.current is null on this case

App/src/pages/ReportDetailsPage.tsx

Line 554 in 9ee8806

navigateBackToAfterDelete.current = IOU.deleteMoneyRequest(iouTransactionID, requestParentReportAction, true);

Thats make after deletion it not navigate back to the transaction thread

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

Add another condition on deleteMoneyRequest function on IOU.ts to return the transaction thread route
if the shouldDeleteTransactionThread is false and the transactionThreadID is exist

if(!shouldDeleteTransactionThread && transactionThreadID){
        return ROUTES.REPORT_WITH_ID.getRoute(transactionThreadID);
}

So it will make it consistent to dismiss the report details RHP

Proof
https://github.com/Expensify/App/assets/73281575/175df4da-fe26-4f6d-8e8a-5ddbdba89f43

What alternative solutions did you explore? (Optional)

N/A

[rojiphil]

@nyomanjyotisa Where are you suggesting to make the change? In deleteTrackExpense or deleteMoneyRequest?

[bernhardoj]

Proposal

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

Inconsistency of behavior when deleting an expense with comments from report details page.

What is the root cause of that problem?

When we submit a single expense, open the expense report, and add some comments, deleting the expense will also delete the transaction thread.
workspace chat report -> expense report (some comments) -> transaction thread (no comment)

If the transaction thread is going to be deleted, then it will return the route to the expense report,

App/src/libs/actions/IOU.ts

Lines 5571 to 5574 in 621e8fc

	if (iouReport && isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) {
	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
	return ROUTES.REPORT_WITH_ID.getRoute(iouReport.reportID);
	}

which we use here, thus the details page is closed (or replaced to be specific):

App/src/pages/ReportDetailsPage.tsx

Lines 632 to 637 in 621e8fc

	onModalHide={() => {
	if (!navigateBackToAfterDelete.current) {
	return;
	}
	Navigation.goBack(navigateBackToAfterDelete.current);
	}}

However, that shouldn't happen. If we see this condition once again,

App/src/libs/actions/IOU.ts

Lines 5571 to 5574 in 621e8fc

	if (iouReport && isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) {
	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
	return ROUTES.REPORT_WITH_ID.getRoute(iouReport.reportID);
	}

we can notice that isSingleTransactionView should be true. We currently always pass it as true, but it should only be true if the report is a transaction thread. We can see that we already define isSingleTransactionView in the details page component, but we don't use it in this case.

App/src/pages/ReportDetailsPage.tsx

Line 124 in 621e8fc

const isSingleTransactionView = isMoneyRequest || ReportUtils.isTrackExpenseReport(report);

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

Pass isSingleTransactionView instead of true.

App/src/pages/ReportDetailsPage.tsx

Lines 551 to 555 in 621e8fc

	if (ReportActionsUtils.isTrackExpenseAction(requestParentReportAction)) {
	navigateBackToAfterDelete.current = IOU.deleteTrackExpense(moneyRequestReport?.reportID ?? '', iouTransactionID, requestParentReportAction, true);
	} else {
	navigateBackToAfterDelete.current = IOU.deleteMoneyRequest(iouTransactionID, requestParentReportAction, true);
	}

If we want to close the RHP when deleting the expense, then we should call Navigation.dismissModal

[nyomanjyotisa]

@nyomanjyotisa Where are you suggesting to make the change? In deleteTrackExpense or deleteMoneyRequest?

@rojiphil in deleteMoneyRequest

[rojiphil]

Thanks all for your proposals.

The correct RCA is that while deletion of an expense from transaction thread report without comments would return the iou report for navigation, deletion of an expense from transaction thread report with comments would not return the transaction thread report for navigation which caused this issue.

@dominictb proposal suggest calling Navigation.goBack in OnModalHide but this is not required as this will be unnecessarily called in the case of deletion the task.

@nyomanjyotisa suggest returning back the transaction thread report id for navigation when it is not supposed to be deleted. This will also dismiss the report details view
@nyomanjyotisa proposal LGTM
🎀👀🎀 C+ reviewed

Awaiting more clarity on @bernhardoj proposal before concluding on this.

[melvin-bot]

Current assignee @jasperhuangg is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[bernhardoj]

@rojiphil what about my proposal

[rojiphil]

@bernhardoj Sorry. I missed mentioning feedback about your proposal.
Passing isSingleTransactionView instead of true would not solve the problem. Right? Or did I miss something here?

[bernhardoj]

The current behavior is:

• Deleting an expense from the transaction thread doesn't close the RHP
• Deleting an expense from the one-transaction IOU report closes the RHP

By passing isSingleTransactionView, both cases won't close the RHP.

Using the selected solution will cause a double transaction thread in the nav stack.

Screen.Recording.2024-06-25.at.18.27.31.mov

[bernhardoj]

The expected result actually says either close or not close, but they should be the same. It makes sense that we close the details page when the expense report detail is deleted too because the report is deleted so nothing to show, but I don't see the reason yet why we want to close it when the report is not deleted (we only delete the transaction).

[rojiphil]

I don't see the reason yet why we want to close it when the report is not deleted (we only delete the transaction).

Well I think it's debatable as it may not be required to show the Report details page on deletion of the expense.

Let's see what the @Expensify/design team thinks.
Once we delete an expense in the Report Details page of the Transaction Thread Report, should we close or keep Report Details page?
Here is a test video demonstrating this.

44336-for-feedback.mp4

[dannymcclain]

Hmm, curious for what the other designers think, but it feels a little weird to me to keep the RHP open.

[shawnborton]

Agree, I would think we would close the RHP once the delete action is taken.

[bernhardoj]

Thanks for the confirmation! PR is ready

cc: @rojiphil

[dubielzyk-expensify]

+1 to what Danny and Shawn said

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[jasperhuangg]

^ Issue isn't actually a deploy blocker because the expected behavior of the integration test has changed. I'm leaving that issue open to see if we need to update the integration test.

Otherwise everyone involved here can ignore!

[melvin-bot]

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

[melvin-bot]

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

• Fix inconsistency in dismissing report details RHP when deleting expense with comments #44777

If no regressions arise, payment will be issued on 2024-07-24. 🎊

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

• @rojiphil requires payment automatic offer (Reviewer)
• @bernhardoj requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@rojiphil] The PR that introduced the bug has been identified. Link to the PR:
• [@rojiphil] 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:
• [@rojiphil] 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:
• [@rojiphil] Determine if we should create a regression test for this bug.
• [@rojiphil] 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.
• [@lschurr] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[lschurr]

@rojiphil - please accept the offer in Upwork so that we can finalize payments.

@bernhardoj - Are you paid via Upwork still or should this be a manual request in NewDot?

[rojiphil]

please accept the offer in Upwork so that we can finalize payments.

@lschurr Accepted offer. Thanks.

[bernhardoj]

@lschurr I'll request in NewDot. Thanks!

EDIT: Requested in ND.

[lschurr]

Payment summary:

• Contributor: $250 @bernhardoj (to be paid in NewDot)
• Reviewer: $250 @rojiphil (paid in Upwork)

[JmillsExpensify]

$250 approved for @bernhardoj