[HOLD for payment 2023-09-27] [$1000] "Hm... it's not here" message displays for a moment when viewing deleted money request that has sub-thread

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Login to Account A.
2. Send a money request to another account.
3. Open the "IOU preview" >>> then click to view details.
4. Edit the amount and click Save.
5. Return to the "IOU preview" >> a sub-thread will be created after step # 4.
6. Hover over the "IOU Preview" and select the "Delete request" option.
7. Click on the request to open the sub-thread >> Leave Thread.
8. On the LHN: reopen that thread.
9. Observer: A "Hm... it's not here" message displays for a moment.

Expected Result:

We shouldn't allow the person who submitted the request to "leave thread" on their transactions, so that option shouldn't be available.

Actual Result:

"Hm... it's not here" message displays for a moment

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.56-4
Reproducible in staging?: Yes
Reproducible in production?: No
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

hmm-not-here-issue.mp4

Recording.387.mp4

Expensify/Expensify Issue URL:
Issue reported by: @tranvantoan-qn
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692707688922459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0143a231695978749a
• Upwork Job ID: 1694426496996868096
• Last Price Increase: 2023-08-30
• Automatic offers:
• mollfpr | Reviewer | 26455595
• b4s36t4 | Contributor | 26455597
• tranvantoan-qn | Reporter | 26455598

[melvin-bot]

Triggered auto assignment to @trjExpensify (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

[OSBotify]

👋 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.

[melvin-bot]

Triggered auto assignment to @puneetlath (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[b4s36t4]

Proposal

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

Hm... it's not here" message displays for a moment when viewing deleted money request that has sub-thread

What is the root cause of that problem?

THis bug introduced after #23394 this PR.

Before this PR we have a state isReportRemoved which makes the report to not render FullPageNotFoundView.

After we leave the thread API result null as the value for deleted reportID in which we have {} in onyxDB.

here

App/src/pages/home/ReportScreen.js

Line 295 in aa98b6b

shouldShow={(!this.props.report.reportID && !this.props.report.isLoadingReportActions && !isLoading) || shouldHideReport}

in this line this snippet gets !this.props.report.reportID && !this.props.report.isLoadingReportActions && !isLoading to true because this.props.report is empty object {} which resulting FullPageNotFoundView to be true.

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

We add a can check for empty report means the we have removed the report from the our reports.

Updated

!_.isEmpty(this.props.report) && !this.props.report.reportID && !this.props.report.isLoadingReportActions && !isLoading`

would render the loading screen.

What alternative solutions did you explore? (Optional)

NA

Kapture.2023-08-22.at.23.50.59.mp4

[trjExpensify]

Hm, I'm not sure why that option only appears when the request is in the [Deleted request] state to preserve the comment history only. Why don't we simply not allow the person who submitted the request to "leave thread"? CC: @luacmartins

[trjExpensify]

FWIW, if this is the root cause it was deployed to production a week ago so this issue can't be a deploy blocker.

What is the root cause of that problem?
THis bug introduced after #23394 this PR.

[luacmartins]

Why don't we simply not allow the person who submitted the request to "leave thread"

I think this should be the correct behavior. Users shouldn't be able to leave the transaction thread.

[trjExpensify]

Cool, let's fix it in that way then!

I could reproduce this on staging, but not on prod, so that RCA above is likely not accurate. Either way, I don't think this should hold up the deploy. Would you agree with that?

[b4s36t4]

@trjExpensify the issue happens with normal thread too. Not just transaction thread.

[b4s36t4]

Kapture.2023-08-23.at.11.07.55.mp4

Also I can reproduce this on prod too.

[trjExpensify]

Oh interesting! I wasn't actually able to repro on prod, so yeah.. this is very likely not a deploy blocker. I'll wait for @puneetlath to chime in before removing the label though.

[puneetlath]

I agree, not a blocker then. Removing the label and we can handle it like a normal bug.

[b4s36t4]

@mollfpr PR is UP #26602

[trjExpensify]

#25698 (comment) - yup, we'll change the bounty amounts to what they were because this issue was before the change. I'll do that when it's time to settle up 👍

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @b4s36t4 got assigned: 2023-09-01 17:13:40 Z
• when the PR got merged: 2023-09-18 02:44:44 UTC
• days elapsed: 10

On to the next one 🚀

[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 1.3.71-12 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix not found page on thread leave #26602

If no regressions arise, payment will be issued on 2023-09-27. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @mollfpr requires payment offer (Reviewer)
• @b4s36t4 requires payment offer (Contributor)
• @tranvantoan-qn requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

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

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] 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:

It's a bit tricky, but this issue arises after the revert on #23394 for #22787. So this should be expected.

[@mollfpr] 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:

The regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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.

Re-join a thread

1. Start a thread
2. Send some message
3. Leave the thread
4. Try opening the thread again from the main report
5. Verify it's showing the skeleton loader first, then the report screen

Open unexisting report, available test for Web/Desktop

1. On the Web, open the report screen with a random reportID
2. Verify it's showing the skeleton loader first, then the not found page

Open a chat/report while offline

1. Open a chat that has not yet opened
2. Verify it's showing the skeleton loader

[mollfpr]

@b4s36t4 Any suggestion on the above checklist?

[b4s36t4]

Hey, @mollfpr Sorry it was wrong emoji 😅😅. Also I'd suggest let's add tests for offline behavior as well? Sorry again 😅

[mollfpr]

@b4s36t4 No worries! 🤣

I was thinking the same about the offline case; I'll add one. Thanks!

[trjExpensify]

Open unexisting report, available test for Web/Desktop

Open an existing report? Also, why is this limited to web/desktop? Can you click a URL on mobile?

[trjExpensify]

Confirming payments as follows:

• @tranvantoan-qn - $250 for the bug report
• @mollfpr - $1,000 for the C+ review
• @b4s36t4 - $1,000 for the bug fix

#urgency bonus doesn't apply as per here, and this issue was created before Aug 30th, so the old bounties apply.

Sound correct?

[b4s36t4]

I think it unexisting report only, means a report which we're not part of it anymore. Like to say Leaving a thread.

[b4s36t4]

Yes the amount sounds correct to me.

[trjExpensify]

I think it unexisting report only, means a report which we're not part of it anymore. Like to say Leaving a thread.

So is it more accurate to say.. "Open a report you don't have access to"? Also, why web/desktop only?

[trjExpensify]

P.S settled up with everyone!

[b4s36t4]

Also, why web/desktop only?

Should be with all platforms, @mollfpr any issues?

[mollfpr]

Also, why web/desktop only?

Because the test is to change the URL. We can probably test it in all platforms with the deep link to a random report.

[trjExpensify]

Yeah, I think we do that. Cool, so we're all done here. Closing!

[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.