[$250] [Group Chats] Selecting a user via Search creates a Group Chat incorrectly

[marcaaron]

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

---

Version Number:
Reproducible in staging?:
Reproducible in production?:
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
Expensify/Expensify Issue URL:
Issue reported by: @slafortune @amyevans
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712159111998949

Action Performed:

1. Navigate to Search page
2. Type partial email for user you already have Group DM with
3. Tap the row

Expected Result:

1. Navigated to existing 1:1 DM with this user

Actual Result:

1. A new Group Chat is getting created

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011a118694cb14efc8
• Upwork Job ID: 1775637141660168192
• Last Price Increase: 2024-04-04
• Automatic offers:
• s77rt | Reviewer | 0
• abzokhattab | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011a118694cb14efc8

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mananjadhav (Internal)

[melvin-bot]

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

[MelvinBot]

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

[marcaaron]

We are not totally sure how to reproduce this. But it seems possibly related to the fact that Stevie has an incorrect value for report.visibleChatMemberAccountIDs and there is a known issue here. So, I suspect this could be related to an existing bug in the back end (I'm looking into the fix already).

report data for correct chat in Amy's account: Has correct visibleChatMemberAccountIDs

report data for correct chat in Stevie's account: Is identical (a.k.a. it is wrong and should be an array with Amy's accountID, but instead shows Stevie's accountID)

Still unsure why the frontend would be trying to create a Group Chat in that case.

@s77rt might be able to figure it out since they have the most context and worked closely with us on this feature.

[s77rt]

Will check this asap

[abzokhattab]

Proposal

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

Selecting a user via search after creating a group chat creates a group instead of 1-1 DM

What is the root cause of that problem?

The issue occurs because when creating a group we set the Draft group when navigating to the confirm group page and, dont clear the draft state after creating the group

App/src/pages/NewChatPage.tsx

Line 199 in d4da2d7

Report.setGroupDraft(logins);

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

Two possible solutions here:

1. Clear Draft State After Group Creation:

   • After creating a group chat, we need to ensure the draft state is cleared by calling clearGroupChat in the code. This will partially solve the issue but not entirely. If a user starts a chat, selects some contacts, goes to the next step, dismisses the modal, and then tries to create a DM, the draft state would still exist, causing the same issue.

2. Restrict Draft State Usage:

   • To fully resolve the issue, we should limit the draft state's use to only the start chat and confirm group pages. We can do this by introducing a new optional boolean parameter, isFromStartNewChat, to the navigateToAndOpenReport function. This parameter will be true only for the start chat and confirm group pages here and here .
   • finally inside the navigateToAndOpenReport we should then use the isFromStartNewChat var here :

   if (!newGroupDraft || !isFromStartNewChat) {

• and here to:

        if (newGroupDraft && isFromStartNewChat) {

alternatively

another solution would be to call the clearGroupChat function on opening the search page same as we do with startchat here

App/src/libs/actions/Report.ts

Lines 225 to 228 in be315d6

	function startNewChat() {
	clearGroupChat();
	Navigation.navigate(ROUTES.NEW);
	}

3rd solution

another solution would be to remove the newGroupDraft usages in the navigateToAndOpenReport function then add a new bool arg isGroup and replace all newGroupDraft occurrences within the function with the new var.. then when calling the navigateToAndOpenReport this bool would be set to true if the create report is a group report, this way it will not be dependent on the draft state.

[s77rt]

@abzokhattab Thanks a lot for looking into this. Your RCA is correct. However for the solution I have something else in mind. I just realised that we don't need newGroupDraft in navigateToAndOpenReport.

[abzokhattab]

after thinking more about it, yes i agree, we can just add isGroup parameter to the navigateToAndOpenReport and this should be true in case the created report is a group report then we replace the newGroupDraft conditions with the isGroup.

[mananjadhav]

@s77rt are you sending in a proposal ?

[s77rt]

Oh sorry I thought I'm assigned here. Was going straight with a PR

[s77rt]

Posting a proposal, a moment please

[mananjadhav]

No worries. Just ensuring that we are on the same page.

[abzokhattab]

added a third solution incase my contribution will be counted 😂

[s77rt]

Proposal

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

After creating a group chat, you can't create a 1:1 chat from search page

What is the root cause of that problem?

In navigateToAndOpenReport the "isGroupChat" logic is based on the newGroupDraft value from onyx i.e.

isGroupChat = newGroupDraft

We will be creating a group as long as there is a draft value

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

Don't rely on the presence of the draft value to deterimne if we are navigating to a group chat. Instead rely on the number of participants i.e.

isGroupChat = participantAccountIDs.length > 1

What alternative solutions did you explore? (Optional)

None

[mananjadhav]

Thanks for the proposal. It looks straightforward and reliable to use participantAccountIds. Quick question, do we need to update/clear anything on the Onyx value after the method?

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[s77rt]

We can keep relying on the newGroupDraft but we will have to clear it every time we complete/discard a group creating process. It's better to avoid this as it's complex to detect when a group creation is discarded.

[abzokhattab]

Just a quick note here, there's a an ongoing discussion that we might allow creating a group chat with only one participant here #39317 (comment) .. in this case using participantAccountIDs.length > 1 will not work ...

the correct condition i think would be:

isGroupChat = participantAccountIDs.includes(currentUserAccountID) 

Because if the report is direct dm, the participantAccountIDs array will not include the current user ID. so i think this would be the way to go if we want to have a group of only one participant

[s77rt]

The draft is cleared only when starting a new group. Same way with IOU. The IOU draft is cleared only when starting a new IOU.

[s77rt]

@abzokhattab That will still work because when creating a group chat even with one person the participantAccountIDs.length will be 2.

• On 1:1: participantAccountIDs will include only the other person's id
• On group chats: participantAccountIDs will include the other persons' ids and your id

[abzokhattab]

On group chats: participantAccountIDs.length will include the other persons' ids and your id

no I mean the group will contain only one participant which is the author in this case the length is 1.

[s77rt]

Ah I see. Good point. However I'm feeling like this should be handled when the feature is implemented

[marcaaron]

I was a bit confused by this proposal as the bug reported is that a Group Chat was created via the "Search" page and not the "New Chat" page. But sounds like we have definitely uncovered a way this could happen. Great investigation.

Confirming with @slafortune now to see if it's possible she took these steps.

@mananjadhav do you mind if we make @s77rt the C+ here and have @abzokhattab collaborate on the PR? I think that would be a fair way to handle this since both have different ideas on the solution. Let me know if it's any problem.

[melvin-bot]

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

[melvin-bot]

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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]

📣 @abzokhattab 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[marcaaron]

Gonna assign everyone so we can make some progress, but let's collaborate in the PR since @s77rt already has it open.

[abzokhattab]

Great, should we go with the approach mentioned here then? what do you think @s77rt ?

[s77rt]

@abzokhattab Let's do that change when it becomes needed (by that time we may have other options).

isGroupChat = participantAccountIDs.includes(currentUserAccountID) 

An extreme edge case, what if the generated optimistic account id in participantAccountIDs happens to be your account id? (collusion)

[mananjadhav]

@mananjadhav do you mind if we make @s77rt the C+ here and have @abzokhattab collaborate on the PR? I think that would be a fair way to handle this since both have different ideas on the solution. Let me know if it's any problem.

Okay sure.

[melvin-bot]

This issue has not been updated in over 15 days. @mananjadhav, @s77rt, @kevinksullivan, @marcaaron, @abzokhattab eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

@s77rt, @kevinksullivan, @marcaaron, @abzokhattab, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[s77rt]

This is closed already. Do we need to remove te Reviewing label here?