[HOLD for payment 2023-06-02] [HOLD for payment 2023-06-01] [$1000] Duplicate Concierge chat created when user sign in with https://new.expensify.com/concierge link

[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. logout if you already login
2. go to https://new.expensify.com/concierge
3. enter email address on login screen
4. enter magic code which was sent to mail
5. click on sign in button

Expected Result:

it should be redirect to concierge chat

Actual Result:

creates a duplicate Concierge with error(Report no longer exists)

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.4
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-04-24.at.11.27.22.AM.mov

Recording.342.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0154b2968f8b6e72b7
• Upwork Job ID: 1650924413916291072
• Last Price Increase: 2023-05-16

[MelvinBot]

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

[MelvinBot]

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

[sonialiap]

Reproducible. When I click out of this new chat and send messages in other threads, this new one remains

[MelvinBot]

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

[mountiny]

Interesting, after clicking the X the chat should be cleared, I think this can be external

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0154b2968f8b6e72b7

https://www.upwork.com/jobs/~01e18fda75e2b74a37

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[alitoshmatov]

Proposal

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

Duplicate Concierge chat created when user sign in with https://new.expensify.com/concierge link

What is the root cause of that problem?

When user signs in, the OpenApp request is sent which is responsible for getting all data including reports list from backend. Meanwhile navigageToConciergeChat is also triggered.

App/src/libs/actions/Report.js

Lines 1016 to 1025 in 3066281

	function navigateToConciergeChat() {
	// If we don't have a chat with Concierge then create it
	if (!conciergeChatReportID) {
	navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]);
	return;
	}
	Navigation.navigate(ROUTES.getReportRoute(conciergeChatReportID));
	}

Here it is using conciergeChatReportID value to check if concierge is available or not, but this value will is not set yet since OpenApp request is not completed.

App/src/libs/actions/Report.js

Lines 687 to 711 in 3066281

	function handleReportChanged(report) {
	if (!report || ReportUtils.isIOUReport(report)) {
	return;
	}
	if (report && report.reportID) {
	allReports[report.reportID] = report;
	if (ReportUtils.isConciergeChatReport(report)) {
	conciergeChatReportID = report.reportID;
	}
	}
	// A report can be missing a name if a comment is received via pusher event and the report does not yet exist in Onyx (eg. a new DM created with the logged in person)
	// In this case, we call reconnect so that we can fetch the report data without marking it as read
	if (report.reportID && report.reportName === undefined) {
	reconnect(report.reportID);
	}
	}
	Onyx.connect({
	key: ONYXKEYS.COLLECTION.REPORT,
	callback: handleReportChanged,
	});

Thus it is sending another request to initialize a new concierge chat, and backend responding with error.

Here you can see that within duration of OpenApp, OpenReport request for new concierge is sent.

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

We should wait for OpenApp to load all report data before checking if concierge report exists. Also, when OpenApp request fires, it sets ONYXKEYS.IS_LOADING_REPORT_DATA to true. Based on this value we can write this:

let resolveIsReadyPromise;
const isReportReadyPromise = new Promise((resolve) => {
    resolveIsReadyPromise = resolve;
});

Onyx.connect({
    key: ONYXKEYS.IS_LOADING_REPORT_DATA,
    initWithStoredValues: false,
    callback: (val) => {
        if (!val) {
            resolveIsReadyPromise();
        }
    },
});

And we can put all actions inside navigateToConciergeChat function to above promise:

function navigateToConciergeChat() {
    isReportReadyPromise.then(() => {
    // If we don't have a chat with Concierge then create it
        if (!conciergeChatReportID) {
            console.error('No Concierge chat found, creating one');
            navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]);
            return;
        }
        Navigation.navigate(ROUTES.getReportRoute(conciergeChatReportID));
    });
}

That is just what I came to my mind. But the main idea is that we should wait OpenApp to load all data first before checking for concierge chat

What alternative solutions did you explore? (Optional)

[MelvinBot]

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.

[hellohublot]

Proposal

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

Duplicate Concierge chat created when user sign in with https://new.expensify.com/concierge link

What is the root cause of that problem?

Linking.getInitialURL -> Report.openReportFromDeepLink -> Navigation.isReportScreenReady -> Report.navigateToConciergeChat()

If our OpenApp interface has not been responsed yet, we cannot judge whether conciergeChatReportID exists here, so we will repeatedly create a new conciergeChatReportID

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

In fact, we should wait for OpenApp to execute successfully before continuing to judge whether conciergeChatReportID exists

We have similar logic in here

App/src/libs/actions/Welcome.js

Lines 28 to 34 in 76c9338

	function checkOnReady() {
	if (!_.isBoolean(isFirstTimeNewExpensifyUser) || isLoadingReportData) {
	return;
	}
	resolveIsReadyPromise();
	}

so we can add export isReadyPromise in Welcome, it will ensure that we successfully request OpenApp, because it has isFirstTimeNewExpensifyUser, then we change Report.navigateToConciergeChat to

Welcome.isReadyPromise.then(() => {
    if (!conciergeChatReportID) {
        navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]);
            return;
    }
    Navigation.navigate(ROUTES.getReportRoute(conciergeChatReportID));
})

[Santhosh-Sellavel]

@mountiny
What's expected here?

After clicking the X the chat should be cleared, I think this can be external

If the above is what we want, that's what already happening.

Staging

Screen.Recording.2023-04-27.at.12.01.48.AM.mov

Production

Screen.Recording.2023-04-27.at.12.00.19.AM.mov

If not let me what's expected here?

[mountiny]

@Santhosh-Sellavel sorry that was not clear, essentially those would be two separate issues.

We should not create the duplicate chat in a first place.

[Santhosh-Sellavel]

@mountiny I'm unassigning it due to low bandwidth Can you assign a new C+ here by reapplying the external label?

Thanks!

[MelvinBot]

📣 @0xmiroslav You have been assigned to this job by @mountiny!
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 📖

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

• Fix duplicate concierge report was created  #19294

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

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:

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

[sonialiap]

@mountiny I see mentions of regressions in the PR but doesn't look like the PR itself was reverted, so there weren't actual regressions on this issue, is that correct?

[mountiny]

No regression, issue foudn existed in production before

[sonialiap]

Dates

• PR assigned May 19
• Merged May 24

4 business days

EDIT: I miscalculated, it was merged within 3 business days (correction)

[sonialiap]

@gadhiyamanan - offer sent for reporting issue - paid
@hellohublot - offer sent for fixing issue - paid + bonus
@0xmiroslav - offer sent for review - paid + bonus

[gadhiyamanan]

@sonialiap accepted, thanks!

[0xmiros]

@sonialiap do you have any reason for PR being merged in 4 business days? It's merged in 5 days including weekend so eligible for bonus.

• Contributor assigned on May 19, 11:58 AM
• C+ approved PR on May 24, 11:07 AM
• PR merged on May 24, 11:47 AM

[0xmiros]

I don't think any PR caused regression. This bug always existed after we implement creating concierge chat optimistically.

Regression Test Proposal

1. Logout if you already logged in
2. Go to https://new.expensify.com/concierge directly from address bar
3. Enter new email address on login screen
4. Enter magic code which was sent to email
5. Click on sign in button
6. Verify that app navigates to the correct concierge report and no duplicate concierge report are created

[sonialiap]

Thanks for the regression steps 👍

@0xmiroslav I miscalculated, I agree it was merged within 3 business days. Sending offer and will add bonus.

@hellohublot bonuses are added as a separate step after the offer is accepted. You do not need to change the offer amount to receive the bonus.

[sonialiap]

Everyone has been paid, closing ✅