[$250] [MEDIUM] [Groups] - Group chat with one user is allowed in confirmation page, but not in contact list

lanitochka17 commented 8 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.58-4 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to FAB > Start chat
Click Add to group with one user
Click Create chat
Note that user lands on 1:1 DM
Go to FAB > Start chat
Select Add to group with the same user in Step 3 and another user
Click Next
In confirmation page, unselect the other user and click Start group

Expected Result:

If app allows to create a group with just one user, Step 3 should result in the creation of a new group

Actual Result:

n Step 3, app does not allow the creation of group chat with one user when clicking Add to group with just one user in contact list. However, in Step 7, when user selects two users in contact list and unselects one user in confirmation page, app allows to create group chat in one user

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/ba594a0d-175f-4a3c-aa77-c465a3429fd7

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01dea4375c99aa51fe
Upwork Job ID: 1775972935262842880
Last Price Increase: 2024-04-04

Issue Owner

Current Issue Owner: @s77rt

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @puneetlath (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 8 months ago

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

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

lanitochka17 commented 8 months ago

We think that this bug might be related to #vip-vsp

lanitochka17 commented 8 months ago

@puneetlath FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

gijoe0295 commented 8 months ago

Proposal

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

Can create a group chat with one user in confirmation page.

What is the root cause of that problem?

In NewChatConfirmationPage, the condition to show Create group button is incorrect:

https://github.com/Expensify/App/blob/24cf044fad3e7e2ee5d63203f7377af750b2f173/src/pages/NewChatConfirmPage.tsx#L135

selectedOptions in this page also includes the group creator.

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

We have several approaches for this:

Change the condition in RCA to selectedOptions.length > 2.
Disable selection if the selected options list has less than or equal 3 (i.e. the group creator and 2 other members) here. By this, user cannot remove any member from the list when there are only 2 other members. The isDisabled condition will be isAdmin || selectedOptions.length <= 2.
Show an error message when press Create group if the group size is not enough. To do this, we should create an error state variable and inside createGroup function here, we check if selectedOptions.length <= 2 to set the appropriate error message. We then use DotIndicatorMessage to display that error.

What alternative solutions did you explore? (Optional)

NA

ShridharGoel commented 8 months ago

Proposal

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

Allow users to create group chats with only 1 other user + with 0 other users.

What is the root cause of that problem?

Kind of a new feature.

The current behaviour checks for selectedOptions.length > 1

https://github.com/Expensify/App/blob/ae6cc4927ce57e5be59e024109436909ed7e1bb5/src/pages/NewChatPage.tsx#L296

onConfirmSelection is also dependent on selectedOptions

https://github.com/Expensify/App/blob/ae6cc4927ce57e5be59e024109436909ed7e1bb5/src/pages/NewChatPage.tsx#L298

In NewChatConfirmPage, confirm button is shown only if selectedOption length is > 1.

https://github.com/Expensify/App/blob/ae6cc4927ce57e5be59e024109436909ed7e1bb5/src/pages/NewChatConfirmPage.tsx#L135

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

In BaseOptionsSelector, add a new prop called shouldShowConfirmButtonWithoutSelection.

This will be passed as true from NewChatPage, which will lead to showing the footer button even if nothing is selected.

New condition for shouldShowFooter:

const shouldShowFooter =
        !this.props.isReadOnly && (this.props.shouldShowConfirmButton || this.props.footerContent) && (!(this.props.canSelectMultipleOptions && _.isEmpty(this.props.selectedOptions)) || shouldShowConfirmButtonWithoutSelection);

We'll update the confirm text:

confirmButtonText={selectedOptions.length > 0 ? translate('common.next') : 'Proceed with only yourself'} // Add it in en.ts and es.ts and use translate

onConfirmSelection will also need an update, we don't want to check for selectedOptions.length > 1 now,

onConfirmSelection={navigateToConfirmPage}

In NewChatConfirmPage, we'll change the showConfirmButton logic so that the button shows even if there's only the creator of the group present.

showConfirmButton={selectedOptions.length > 0}

https://github.com/Expensify/App/assets/35566748/312928d2-b036-48e5-8dc2-73024ee3b92e

allgandalf commented 8 months ago

Proposal

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

We allow user user to create group chat with just 2 users

What is the root cause of that problem?

Currently, we allow the user to unselect the participants of the group without checking the number of selected participants: https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/pages/NewChatConfirmPage.tsx#L91-L97

But in the main contact page, if you check the video, we will not allow the user to create a group, we will take them to 1:1 DM.

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

So here as per the design, i think it's best to disable deselecting participants if the length is 3:


onst unselectOption = (option: ListItem) => {
        if (!newGroupDraft || selectedOptions.length <= 3) {
            return;
        }
        const newSelectedParticipants = newGroupDraft.participants.filter((participant) => participant.login !== option.login);
        Report.setGroupDraft(newSelectedParticipants);
    };

This way we maintain the group screen also allowing the user to unselect users if the length is more than 3.

We can also show a error message at the bottom stating that you cannot create a group of less than 3 members, if you want to add more members then go back to the contact page and add more participants.

What alternative solutions did you explore? (Optional)

N/A

marcaaron commented 8 months ago

Gonna remove the blocker here as there is not really any bad thing that will happen if the user creates a 1:1 DM.

I sort of agree that we could give them the option to create a Group Chat with just one user - but it feels like the more likely thing is that they will create a 1:1 with them.

Not too sure we need to do anything here right now. cc @JmillsExpensify to see if he has any thoughts.

ShridharGoel commented 8 months ago

@marcaaron @JmillsExpensify Can you check my proposal? I've suggested how we can provide a good experience by converting the group chat flow to normal chat creation flow if the user unselects some options. There's a video showing the flow as well.

marcaaron commented 8 months ago

@ShridharGoel I'm not 100% sure if that's what we want, but doesn't look too bad. Will wait to get @JmillsExpensify thoughts on this before continuing here.

@shawnborton, @dubielzyk-expensify, @dannymcclain might want to weigh in on this one as well.

I think it is implied by pressing on "Add to Group" that we want to make a Group whether it has one or more users.

dannymcclain commented 8 months ago

🤔 Hmm yeah, do we need to just decide if we want to let people create a group of 2 (that is distinct from their DM with each other)?

I kinda think you should be able to create a group with just 2 people, I think. But I'm curious what everyone else thinks.

shawnborton commented 8 months ago

Yeah, I think a Group can be 2 people because at a later date you might invite more people to it.

dubielzyk-expensify commented 8 months ago

I actually kinda think we shouldn't. Cause it allows you to create the same thing as a DM. I even think that if you just add one person to a group, we should toss you over to a DM with them.

Messenger does something similar, but forces you to not create a group with 1 person.

I know it doesn't specifically impact IOUs, but having a DM with Danny and a Group named Danny and Jon, then the IOU goes to my DM with Danny just feels like more confusion.

I don't feel super strongly here so definitely happy to lean on consensus here.

JmillsExpensify commented 8 months ago

Phew, tough one. I mostly think it's ok that you can create a group with one person, which might also be someone that you also have a DM with. That's why the original scope calls for requiring at least one other person, no matter if you have an existing DM with them or not. Equally, the group chat avatar is distinct from the DM avatar, so even if you accidentally created a group there are several UI patterns that help you understand that one is not the same as the other.

All in all, I think the solution is for us to update the Group Chat creation flow, as I think the existing pattern is confusing and already being deprecated for Splits, so we should deprecate it for Groups too, though it's out of scope for this issue.

shawnborton commented 8 months ago

Yeah.... I can see both sides but one use case that comes to mind: let's say a friend and I are planning an event, and we first want to make a Group chat and grab the QR code so that anyone who is coming to the event can join. If we require at least 3 people to make a Group, that means we'd need another organizer to jump in before we can make the group and get our shareable QR code for others to jump in.

So I mean wild 🌶️ take... but could a group be created with even just one person?! Maybe it should be its own item in the global create flow, and the first step is just naming the group (and updating the avatar if you want), and then we ask for group members as an optional piece.

dubielzyk-expensify commented 8 months ago

Well, I'm convinced 😄 I think you make a compelling argument there @shawnborton. If we push it at places like conventions then creating a group ahead of time seems like a valid use case.

dannymcclain commented 8 months ago

Yeah.... I can see both sides but one use case that comes to mind: let's say a friend and I are planning an event, and we first want to make a Group chat and grab the QR code so that anyone who is coming to the event can join. If we require at least 3 people to make a Group, that means we'd need another organizer to jump in before we can make the group and get our shareable QR code for others to jump in.

So I mean wild 🌶️ take... but could a group be created with even just one person?! Maybe it should be its own item in the global create flow, and the first step is just naming the group (and updating the avatar if you want), and then we ask for group members as an optional piece.

@shawnborton my thinking was the exact same. I almost mentioned creating a group with no one else in it but didn't want to stir the pot too hard 🤣

marcaaron commented 8 months ago

Great points all around (especially find myself agreeing with @shawnborton's take on being able to create a Group of 1 via Global Create or shortcut of some kind).

I do think this is sort of obvious enough for now:

2024-04-03_11-58-22

How do we feel about a plan like this:

Patch this for now so that the "Next" button appears when even just one other person is selected.
We'll revisit some of the ideas in this thread and keep discussing how to optimize this.

?

dubielzyk-expensify commented 8 months ago

Sounds good to me!

abzokhattab commented 8 months ago

Proposal

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

enable the "Next" button to show up if 1 or more contacts are selected.

What is the root cause of that problem?

according to the discussion conclusion above , we need to modify the behavior of showing the "Next" button in the start chat page, where we need to make it appears when 1 or more contacts are selected.

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

we need to modify the confirmButtonText here to: https://github.com/Expensify/App/blob/d4da2d7cadbe1c620c0c4b9c0760ac1775b64b9f/src/pages/NewChatPage.tsx#L297

confirmButtonText={translate('common.next')}

OR

confirmButtonText={selectedOptions.length > 0 ? translate('common.next') : translate('newChatPage.createChat')}

and the onConfirmSelection condition to: https://github.com/Expensify/App/blob/d4da2d7cadbe1c620c0c4b9c0760ac1775b64b9f/src/pages/NewChatPage.tsx#L299

onConfirmSelection={selectedOptions.length > 0 ? navigateToConfirmPage : createChat}

this way we allow the user to create a group with 1 or more other contacts.

..........................................................................................................................................................

Extra: to enable creating a group that consist of only one person, we need to modify the showConfirmButton in the group confirm page so that it shows up even if there is only one person in the group setup https://github.com/Expensify/App/blob/4ea37b6c8eb7eb7a2ee4a7678face11a69c0d535/src/pages/NewChatConfirmPage.tsx#L135

showConfirmButton={selectedOptions.length > 0}

ShridharGoel commented 8 months ago

Proposal

Updated based on the new discussion.

ShridharGoel commented 8 months ago

Also included a video. My proposal also includes the case when only the group creator is there (only the admin is present).

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

marcaaron commented 8 months ago

@s77rt @abzokhattab adding you guys to this one too since we are already working together on https://github.com/Expensify/App/issues/39560

abzokhattab commented 8 months ago

will open a PR today.

abzokhattab commented 7 months ago

Sorry for the delay and thanks for your understanding i was super busy the last few days ... The PR is now ready. please have a look and let me know if you have any comments.

mvtglobally commented 7 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 6 months ago

This issue has not been updated in over 15 days. @s77rt, @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!

s77rt commented 6 months ago

PR deployed to production 2 weeks ago. This is awaiting payment

mvtglobally commented 6 months ago

Issue not reproducible during KI retests. (Second week)

mvtglobally commented 5 months ago

Issue not reproducible during KI retests. (Third week) Good to close?

mvtglobally commented 5 months ago

Issue not reproducible during KI retests. (Fourth week)

melvin-bot[bot] commented 5 months ago

@s77rt, @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 commented 5 months ago

This is still due payment. @marcaaron Can you please assign a BZ member?

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @mallenexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 4 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to Design team member for new feature review - @dannymcclain (NewFeature)

mallenexpensify commented 4 months ago

@s77rt @abzokhattab , can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~0123a0fa7b4a2fcc25

s77rt commented 4 months ago

@mallenexpensify Accepted! Thanks!

abzokhattab commented 4 months ago

thank u applied

mvtglobally commented 4 months ago

Issue not reproducible during KI retests. (Fifth week)

mallenexpensify commented 4 months ago

Contributor: @abzokhattab paid $250 via Upwork Contributor+: @s77rt paid $250 via Upwork.

@s77rt plz complete the BZ checklist below? Thx

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:

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

s77rt commented 4 months ago

The PR that introduced the bug has been identified: https://github.com/Expensify/App/pull/37458
The offending PR has been commented on: https://github.com/Expensify/App/pull/37458#discussion_r1686929796
A discussion in #expensify-bugs has been started: Not needed (unique bug)
Determine if we should create a regression test for this bug: No. We already have a test case covering that https://expensify.testrail.io/index.php?/cases/view/2645392

s77rt commented 4 months ago

@mallenexpensify Done! ^

mallenexpensify commented 4 months ago

Thanks @s77rt , closing

melvin-bot[bot] commented 1 month ago

⚠️ 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.

Expensify / App