[HOLD on #20394] [HOLD for payment 2023-06-21] [$1000] hmm it’s not there page appears for second when user opens a thread

[kavimuru]

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. Go to any chat
2. send any message
3. go to thread
4. for any message create a new thread
5. from receiver side open first thread
6. from first thread open second thread

Expected Result:

should not show hmm it's not there page

Actual Result:

while opening second thread hmm it's not there page appears for second

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.16-6
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.Recording.2023-05-18.at.11.05.50.AM.mov

Recording.705.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b6eb4ad8101e0fd1
• Upwork Job ID: 1662125710425755648
• Last Price Increase: 2023-06-02

[melvin-bot]

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

[dukenv0307]

Proposal

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

hmm it's not there page appears for second when user opens a thread

What is the root cause of that problem?

when user A create a new thread of the message that is created by user B, user B received eventData that contains childReport that is new thread, childReportID of the message and reportAction of new thread like this

But when user A create a new thread of the message that is created by user A, user B recevied eventData that only contains childReportID of the message

So that when navigateToAndOpenChildReport function is called in user B, childReportID is exist but no report that has reportID is childReportID. And then report prop of ReportScreen is set defaultProps.report that doesn't has reportID and isLoadingReportAction is false for a second . That make shouldShow is true and hmm it's not there page appears for second

App/src/libs/actions/Report.js

Line 448 in 8180113

function navigateToAndOpenChildReport(childReportID = '0', parentReportAction = {}, parentReportID = '0') {

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

In openReport function, we get report from allReports with reportID parameter. And then we will check if the report is empty we change method of optimisticReportData is set to optimisticReportData is updated in Onyx before navigating to report and then optimisticReportData can be set to report prop in ReportScreen instead of defaultProps.report

const report = allReports[reportID];
if (_.isEmpty(report)) {
      optimisticReportData.onyxMethod = Onyx.METHOD.SET;
  }

App/src/libs/actions/Report.js

Line 334 in 2157057

function openReport(reportID, participantList = [], newReportObject = {}, parentReportActionID = '0', isFromDeepLink = false) {

What alternative solutions did you explore? (Optional)

We also could be consistent eventData sends to user B when user A creates a new thread of the message that is created by user A is the same when user A creates a new thread of the message that is created by user B

[melvin-bot]

@twisterdotcom Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[twisterdotcom]

This isn't something I can recreate following the recreation steps:

2023-05-25_23-13-22.mp4

@gadhiyamanan or @kavimuru - could you help me by confirming what the actual steps should be if you can still recreate the bug?

[gadhiyamanan]

try these steps :

1. Go to any chat
2. from userA, send any message
3. select reply in the thread option for that message
4. go to the thread
5. In the thread, send any message
6. select reply in the thread option for that message
7. In the thread, send any message
8. from userB, select message to go that thread

Screen.Recording.2023-05-26.at.1.47.50.PM.mov

cc: @twisterdotcom

[twisterdotcom]

Ah, dead on! I recreated. Have a video, but it's got some personal data in. Shared for internal employees here, but I'm happy to move this on.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@marcochavezf, @twisterdotcom, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!

[fedirjh]

Reviewing shortly

[fedirjh]

@dukenv0307 can you elaborate more on this part of the root cause ?

So that when navigateToAndOpenChildReport function is called in user B, childReportID is exist but no report that has reportID is childReportID. And then report prop of ReportScreen is set defaultProps.report that doesn't has reportID and isLoadingReportAction is false for a second . That make shouldShow is true and hmm it's not there page appears for second

why isLoadingReportAction is set to false for a second ?

[dukenv0307]

@fedirjh That is defaultProp of report.

[dukenv0307]

@fedirjh Additional information why defaultProp.report is set to report prop? In openReport function, because childReportID exists,it leads onyxMethod of optimisticReportData is merge and it will update report after navigate to report. While the report doesn't exists in allReports so that defaultProp.report is set to report.

With this information above we also have a alternative solution is in openReport set onyxMethod of optimisticReportData is set if the report is got from allReports with reportID is empty.

[dukenv0307]

@fedirjh I updated proposal with the last solution

[fedirjh]

@dukenv0307 You can raise the PR

[dukenv0307]

@fedirjh @marcochavezf The PR is ready to review.

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

• Fix page not found appears for a second when user opens a thread #20076

If no regressions arise, payment will be issued on 2023-06-13. 🎊

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

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:

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

[aldo-expensify]

The PR #20076 caused the regression #20394. The PR reached production.

I think this should have been caught in staging by this test: https://expensify.testrail.io/index.php?/cases/view/1971140&group_by=cases:section_id&group_order=asc&display_deleted_cases=0&group_id=229066

@kavimuru @isagoico do you know why this was missed?

[twisterdotcom]

@kavimuru and @isagoico - just waiting on confirmation from you here.

[twisterdotcom]

https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md

Payment for your contributions and bug reports will be made no less than 7 days after the pull request is deployed to production to allow for regression testing. If a regression occurs, payment will be issued 7 days after all regressions are fixed. If you have not received payment after 8 days of the PR being deployed to production and there being no regressions, please email contributors@expensify.com referencing the GH issue and your GH handle.
...
If the PR causes a regression at any point within the regression period (starting when the code is merged and ending 7 days after being deployed to production), contributors are not eligible for the 50% bonus.

Going to HOLD payment on #20394 for now.

[twisterdotcom]

I have paid @gadhiyamanan for reporting.
I have sent $1k offers to @fedirjh and @dukenv0307. We'll pay out with the payout for #20394 whenever that is due. I will update the HOLD for payment... date when that PR is deployed to match.

Thanks for all the hustle here folks.

[isagoico]

@twisterdotcom @aldo-expensify Hey team, apologies for the delay on the answer here. I had missed this bump before.
I was giving a look at our results from the regression where this CP #20076 was pushed to staging and noticed that it was not included in any deploy checklist for some reason.
This is the timeline of what happened with this issue:

1. 6/5 - Applause didn't test this CP on 6/5 because is was not included in this Deploy Checklist - For context, we execute QA from all CPs that are added to the checklist while it's locked.
2. 6/6 - The CP was pushed at 3:55pm EST on June 6 after we had finished all regression tests for the 6/5 checklist. The checklist was closed and the prod deploy was triggered, pushing this PR to prod without verification.
3. 6/6 - Regression on 6/6 starts and we find this issue while executing our regression run [HOLD for payment 2023-06-21] [$1000] Chat - Page loads infinitely instead of error page when accessing invalid link #20394, the issue is reproducible on STG and PROD, because of the last min CP. Adding a screenshot of the TestRail result here.

Hopefully this provides a clear explanation on why was this missed by QA. I was completely confused at first because, as @aldo-expensify mentioned in this comment, this flow is specifically tested on each run, but when I noticed the linked PR that was CP was not included in any checklist, I knew something was off.

[melvin-bot]

@marcochavezf, @twisterdotcom, @fedirjh, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[twisterdotcom]

Great, thanks for the explanation. This is all paid out!

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