[HOLD for payment 2023-07-10] [$1000] Web- Chat - Unable to leave the Concierge "Welcome Message" thread

[kbecciv]

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. Sign in with a new account
2. Go to the Concierge chat
3. Hover over the welcome message from Concierge
4. Click on Reply in thread icon
5. Click on thread header
6. Click on Leave thread

Note: this only occurs on the Welcome message

Expected Result:

Thread is no longer there on LHN

Actual Result:

Thread is still there on LHN

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: v1.3.24-4

Reproducible in staging?: yes

*Reproducible in production?: n/a

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-29.at.8.47.50.PM.mov

Expensify/Expensify Issue URL:

Issue reported by: @adeel0202

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0168a95ee497fd9964
• Upwork Job ID: 1665923109054201856
• Last Price Increase: 2023-06-06

[melvin-bot]

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

Unable to leave the thread that was created with Concierge

What is the root cause of that problem?

When leaving chat we will navigate to Conciege Chat as default

App/src/libs/actions/Report.js

Line 1667 in 9fa12f3

navigateToConciergeChat();

But in here

App/src/libs/actions/Report.js

Lines 769 to 771 in 9fa12f3

	if (ReportUtils.isConciergeChatReport(report)) {
	conciergeChatReportID = report.reportID;
	}

Both the main Concierge chat and thread chat with Concierge will be assigned to conciergeChatReportID

So that when leaving the thread with Conciegre, the App will re-direct again to this chat

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

Because isConciergeChatReport function is used in many places. So I suggest we should add one param called includeThreadWithConcierge in this function for more flexibility in the future like this

function isConciergeChatReport(report, includeThreadWithConcierge = false) {
    return lodashGet(report, 'participants', []).length === 1 && report.participants[0] === CONST.EMAIL.CONCIERGE && (includeThreadWithConcierge || !isThread(report))
}

And then we will update all places that use this function and this is my suggestion, maybe we need to confirm more

1. And in here:

   App/src/libs/actions/Report.js

   Lines 769 to 771 in 9fa12f3

   	if (ReportUtils.isConciergeChatReport(report)) {
   	conciergeChatReportID = report.reportID;
   	}

   
   we should pass includeThreadWithConcierge === false

2. In here:
   

   App/src/libs/ReportUtils.js

   Lines 750 to 753 in b8d4576

   	if (isConciergeChatReport(report)) {
   	result.source = CONST.CONCIERGE_ICON_URL;
   	return [result];
   	}

   
   we should pass includeThreadWithConcierge === true

3. In here:
   

   App/src/libs/SidebarUtils.js

   Line 105 in b8d4576

   const conciergeChatReport = _.find(allReports, ReportUtils.isConciergeChatReport);

   
   we should pass includeThreadWithConcierge === false (default value)

4. In here
   

   App/src/pages/home/report/ContextMenu/ContextMenuActions.js

   Line 304 in b8d4576

   !ReportUtils.isConciergeChatReport(reportID) &&

   
   we should pass includeThreadWithConcierge === true

5. In here
   

   App/src/pages/home/ReportScreen.js

   Line 289 in b8d4576

   {Boolean(this.props.accountManagerReportID) && ReportUtils.isConciergeChatReport(this.props.report) && this.state.isBannerVisible && (

   
   we should pass includeThreadWithConcierge === false

### What alternative solutions did you explore? (Optional)

After leaving the child thread, we can navigate to parent chat instead of Concierge chat. if do that we can pass parentReportID to

App/src/libs/actions/Report.js

Line 1638 in 9fa12f3

function leaveRoom(reportID) {

and in here

App/src/libs/actions/Report.js

Line 1667 in 9fa12f3

navigateToConciergeChat();

navigate to parent chat instead of conciege chat

Or we can remove navigate function in leaveRoom function and navigate to parent chat in here

App/src/pages/ReportDetailsPage.js

Line 116 in 9fa12f3

action: () => Report.leaveRoom(props.report.reportID),

like this

action: () => {
                    Report.leaveRoom(this.props.report.reportID)
                    Navigation.navigate(ROUTES.getReportRoute(this.props.report.parentReportID));
                },

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[jliexpensify]

We're on staging v1.3.24-4, please re-test on the newest version.

I am not seeing this issue on my end.

[esh-g]

@jliexpensify I'm still able to reproduce on staging v1.3.24-4

Cool.mov

[esh-g]

Proposal

posted in slack 5 days ago

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

Unable to leave the thread that was created with Concierge

What is the root cause of that problem?

App/src/libs/actions/Report.js

Lines 761 to 772 in edf9b18

	function handleReportChanged(report) {
	if (!report || ReportUtils.isIOUReport(report)) {
	return;
	}
	if (report && report.reportID) {
	allReports[report.reportID] = report;
	if (ReportUtils.isConciergeChatReport(report)) {
	conciergeChatReportID = report.reportID;
	}
	}

In Report.js we are updating conciergeChatReportID in the handleReportChanged function and when we create a new thread from concierge chat, this gets updated to the chat thread instead of the actual concierge report. Therefore, when we exit the thread, it tries to direct the user to what it thinks is concierge chat but actually is the chat that we left.

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

Before updating conciergeChatReportID, we are checking if it is concierge by the ReportUtils.isConciergeChatReport() method

App/src/libs/ReportUtils.js

Lines 376 to 378 in edf9b18

	function isConciergeChatReport(report) {
	return lodashGet(report, 'participants', []).length === 1 && report.participants[0] === CONST.EMAIL.CONCIERGE;
	}

So, we need to add an additional condition to this method report.parentReportID === null . So we can use the function isThread(report) to do that as well. This will make sure that we don't set a thread of concierge as the conciergeChatReportID and this successfully eliminates this issue.

Complete implementation

function isConciergeChatReport(report) {
    return lodashGet(report, 'participants', []).length === 1 && report.participants[0] === CONST.EMAIL.CONCIERGE && !isThread(report);
}

What alternative solutions did you explore? (Optional)

[jliexpensify]

Thanks @esh-g for testing - so I misunderstood the original issue and tested on a standard thread in Concierge, which works as normal.

I can repro the behaviour with the Concierge auto-message, but I would also say an additional issue is that Concierge is duplicating the exact same message in-thread (when you leave it):

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0168a95ee497fd9964

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[esh-g]

additional issue is that Concierge is duplicating the exact same message in-thread (when you leave it):

There is an issue for that also

[jliexpensify]

Oh, you're right - #19982. I'll amend the title.

[Santhosh-Sellavel]

@tylerkaraszewski @esh-g's proposal looks good to me. Let me know your thoughts, thanks!

[dukenv0307]

@Santhosh-Sellavel What do you think about my proposal. My proposal comes first.
@esh-g Note that only the reporter can be posted a proposal in Slack. So that your proposal in Slack is invalid

[Santhosh-Sellavel]

It's okay to proposal the solution on slack as per guideline. And Bring to github for review before it's too late.

[melvin-bot]

📣 @esh-g You have been assigned to this job by @tylerkaraszewski!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[esh-g]

@Santhosh-Sellavel The PR is here #21481

[jliexpensify]

Hired @adeel0202 and @Santhosh-Sellavel . @esh-g - your profile (I assume it's "Esh G.") seems to have an issue when I invite it...it says it doesn't exist?

[esh-g]

@jliexpensify I submitted a proposal for the job. Here is my profile: https://www.upwork.com/freelancers/~0186b1d7cc69656f22

[jliexpensify]

Oh, ok - different profile, thanks @esh-g!

[esh-g]

Oh, ok - different profile, thanks @esh-g!

Thanks for sending the offer! I have accepted it 👍

[melvin-bot]

🎯 ⚡️ Woah @Santhosh-Sellavel / @esh-g, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @esh-g got assigned: 2023-06-23 23:26:22 Z
• when the PR got merged: 2023-06-28 19:50:18 UTC

On to the next one 🚀

[tylerkaraszewski]

PR is merged.

[tylerkaraszewski]

@jliexpensify - I am leaving on sabbatical today. I don't expect this issue to need any further engineering input, but if it does, please unassign me and re-apply the Engineering label to get a new engineer to finish this issue, as I'll be out until the end of August.

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

• Is thread concierge report #21481

If no regressions arise, payment will be issued on 2023-07-10. 🎊

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:

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

[jliexpensify]

@Santhosh-Sellavel before any payments are made, can you please complete the checklist? I'll also need to assign a new Engineer to review this checklist, as Tyler is on sabbatical.

[esh-g]

bump @Santhosh-Sellavel

[Santhosh-Sellavel]

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:

• [@Santhosh-Sellavel] The PR that introduced the bug has been identified. Link to the PR: Update Details Page for Threads #18772
• [@Santhosh-Sellavel] 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: Update Details Page for Threads #18772 (comment)
• [@Santhosh-Sellavel] 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: Not need we could have caught this while testing.
• [@Santhosh-Sellavel] Determine if we should create a regression test for this bug. N/A We can skip here as it a minor.
• [@Santhosh-Sellavel] 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. N/A
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Let me know your thoughts thanks @jliexpensify!

[jliexpensify]

Paid @adeel0202 and @esh-g . @Santhosh-Sellavel - I'll let you record your own payment (you qualify for a bonus). Job closed and removed.

[Santhosh-Sellavel]

Requested Payment on ND

[anmurali]

Approved $1500 with bonus for Santhosh on ND