Create Group Chats + Splits. #37458

waterim · 2024-02-28T22:42:30Z

Details

Create Group Chats + Splits. Deprecate Group DM creation. Support viewing both Group DMs and Group Chats.

Fixed Issues

$ #32317

Tests

Test creating single user chat

Tap the global create button
Select “Start chat”
Pick a single user
Verify you are navigated to the chat

Test creating single user chat with new account

Tap the global create button
Select “Start chat”
Pick a single user
Verify you are navigated to the chat

Test existing Group DM looks and behaves normal in staging

Create a Group DM (on production) (note the reportID for access later)
Log back into same account (on staging)
Access the Group DM
Verify the avatars and title look correct (multiple avatars)
Tap the + icon
Choose Split bill option and complete the split for a “Manual” request
Verify that the splits are created on each participant’s chat

Test creating new Group Chat

Tap the global create button
Select “Start chat”
Choose two users to add via “Add to group” button
Choose another user to add by entering email (new account)
Press “Next”
Verify the “Group” confirm page displays the members
Verify a member can be de-selected
Verify the current user has a label that says “Admin”
Tap “Start group”
Verify you are navigated to the chat
Verify the group chat name in the header includes the current user
Verify there is a single avatar vs. the multiple avatars that Group DMs have

Create new group from existing DM members

Create a Group DM (on production) (note the reportID for access later)
Log back into same account (on staging)
Tap the global create button
Select “Start chat”
Select the same members with existing DM
Verify a new chat is created.
Repeat and verify that you can create multiple chats with these same users.

Verify that users only see Group Chats when a comment appears

As User A create a Group Chat between User B and User C
As User B verify that the chat is not visible in the LHN
As User C verify the chat is not visible
As User A leave a comment
As User B and C verify that the chat is now visible and the comment from User A displayed

Verify Global Create split creates new Group Chat

Tap the global create button
Select “Request money”
Choose manual
Enter amount and tap “Next”
Select multiple users to split with and tap “Next”
Tap “Split $nn.nn”
Verify that a Group Chat is created and the split preview lands there
Verify that the splits appear on each individual 1:1 chat for the participants

Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

None

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

marcaaron

Looking super good so far! Left some thoughts and a few tips. Overall moving in a great direction. Thanks very much! 🙇

marcaaron · 2024-02-28T23:28:16Z

src/languages/es.ts

@@ -1142,6 +1142,9 @@ export default {
        roomDescriptionOptional: 'Descripción de la sala de chat (opcional)',
        explainerText: 'Establece una descripción personalizada para la sala de chat.',
    },
+    groupConfirmPage: {
+        groupName: 'Nombre del grupo',
+    },


note: If we didn't yet - we should get these final approved by someone in the español channel (can wait for later)

src/libs/ReportUtils.ts

src/libs/actions/Report.ts

src/pages/NewChatConfirmPage.tsx

src/pages/NewChatPage.tsx

waterim · 2024-03-04T23:16:48Z

@marcaaron I have a question reagarding this part:

Here you are showing that we have "chatType", "reportName" and "groupChatAdminLogins" as parameters.
But we don't have them all in our OpenReportParams type:

Should I just add all those params like this?

Because as I can see we already have reportName and chatType in OptimisticData in this request and Im confused a bit :D
Do I need to add it additionally in a new request design?
Or just to update reportName, chatType and add groupChatAdminLogins to OptimisticData which we are sending?

marcaaron · 2024-03-05T03:02:59Z

Yes good question. The OpenReport API request is very generic and can be used to create a few different report types. I would continue down this path for now and add whatever is missing as optionals. We will need:

optimisticAccountIDList: string,
chatType: string,
groupChatAdminLogins: string,
optimisticAccountIDList: string,

Later we can add something to handle the avatar - but those should be enough for this PR.

And you will basically want some check like:

if (chatType == 'group') {
    paramters.chatType = 'group';
    parameters.groupChatAdminLogins = logins.join();
    paramters.optimisticAccountIDList = accountIDList.join();
    parameters.reportName ? reportName : '' // pass the empty string here
}

marcaaron · 2024-03-05T03:04:07Z

If there is some fancy way to do that with typescript and make it clear that only a "group chat" should have these additional params that would also be acceptable (but no need to prioritize this).

waterim · 2024-03-05T18:21:48Z

@marcaaron Updated the code, seems to work fine.
As I understand one more thing will be to update participantAccountIDs to participants, right?

(I know that tests are failing, thats ES translations)

marcaaron · 2024-03-05T20:04:09Z

As I understand one more thing will be to update participantAccountIDs to participants, right?

Yes, this is a little bit of a tricky part because we want:

Any chat that is not a Group Chat to keep using the existing logic (the backend is still needing to be migrated for these).
If it's a Group Chat then it should only use participants object and never use participantAccountIDs or visibleChatMemberAccountIDs.

Basically, we are going to:

Have both Group Chats and other kinds of chats exist side by side for some time.
Ensure we have correct participants data being sent for the other kinds of chats in the backend (already should work for Group Chats)
Deprecate participantAccountIDs and visibleChatMemberAccountIDs in the front end
Remove them from the backend code after force upgrading everyone to the new participants object.

I suspect this migration will take a bit longer than the Group Chats feature itself and we are targeting end of May for release so that's why it's happening in this order 🙃

But open to any thoughts on it too. Let me know if you run into any issues or confusing things.

waterim · 2024-03-05T20:11:47Z

@marcaaron Okay, get it, I can do a conditional call for group chats for now with "participants" field.
As I understand we have a separate ticket to handle all places and here I can just modify this one.
I think everything for group chats is done, except participants(feel free to comment if I missed something) and after I will add splits too

waterim · 2024-03-06T23:29:46Z

@marcaaron Was able to create with such a response:

But some issue that I have button "join" on the page:

Looks like Im not an admin of this group chat, but send participants correctly and GroupChatAdminLogins too

GroupChatAdminLogins is there:

marcaaron · 2024-03-07T01:49:12Z

@marcaaron Was able to create with such a response:

Nice!

But some issue that I have button "join" on the page:

👍 We want that "Join" button to not exist if you are a participant. It is showing because the notificationPreferences are hidden see here:

App/src/pages/home/HeaderView.js

Lines 172 to 174 in ffa731a

    
           const canJoinOrLeave = !isSelfDM && (isChatThread || isUserCreatedPolicyRoom || canLeaveRoom); 
        
           const canJoin = canJoinOrLeave && !isWhisperAction && props.report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN; 
        
           const canLeave = canJoinOrLeave && ((isChatThread && props.report.notificationPreference.length) || isUserCreatedPolicyRoom || canLeaveRoom);

Easiest would be to add a check like:

const canJoinOrLeave = !isSelfDM && !isGroupChat && (isChatThread || isUserCreatedPolicyRoom || canLeaveRoom);

Nobody can "Join" a Group Chat like this. Have to be invited by someone for now.

Looks like Im not an admin of this group chat

I think you are the admin, we just don't show it anywhere on the members page yet. So, it's a new thing to add. But, let me know if I didn't understand your question! Thanks!

waterim · 2024-03-07T20:44:31Z

@marcaaron Looks like added everything, but needs to be checked carefully, because could miss something, doc is not really detailed, but hopefully everything is here :)

waterim · 2024-03-07T21:07:12Z

Ah, okay, OptionsSelector was deprecated, rewriting to SelectionList

waterim · 2024-03-07T22:09:42Z

Updated and resolved conflicts

src/libs/actions/Report.ts

src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js

marcaaron · 2024-03-28T17:53:40Z

Ok, thanks for the investigation there (did not have a chance as I discovered the bug later in my day).

I will try to find the answer to this. We have @aldo-expensify working on a PR related to OpenReport and it seems like it would fix the problem.

marcaaron · 2024-03-28T18:31:48Z

Ah ha actually - the problem is that everyone is "hidden" until we leave a comment on the chat. Of course, apologies for not catching that sooner. My head was on backwards yesterday.

This is expected as the visibleChatMemberAccountIDs would be all the users who have hidden: true. If we can use the participants then they should always be displayed for the case of a Group Chat. I think we can fix it by using this field for the members, but no need for right now.

Optimistic chat report should have:

{hidden: true} for all the users

Real chat report returned should have:

{hidden: true} for all the users

When someone leaves a comment it will end up as

{hidden: false}

I think there is nothing we need to do here for now. We will update this to use participants only eventually. The optimistic report should have {hidden: true} as well. But we don't need to block on that. I'm going to give this a merge and see how the QA goes. 🤞

Thanks everyone!

melvin-bot · 2024-03-28T18:39:11Z

@marcaaron looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2024-03-28T18:45:37Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

marcaaron · 2024-03-28T18:49:10Z

Not emergency. Looks like tests have passed.

OSBotify · 2024-03-29T02:26:41Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.4.58-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

stitesExpensify · 2024-03-29T16:10:03Z

👋 I think that this deploy blocker is probably a result of this PR. Would you mind taking a look please?

waterim · 2024-03-29T16:18:10Z

@stitesExpensify It was not a scope of this issue, should not be a blocker
Cc: @marcaaron

marcochavezf · 2024-03-30T00:46:00Z

Also seems this issue is coming from this PR, but I don't think it should be a deploy blocker.

OSBotify · 2024-04-02T11:02:57Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.58-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

s77rt · 2024-04-09T18:16:05Z

src/pages/NewChatPage.tsx

+        let newSelectedOptions;
+        if (newGroupDraft?.participants) {
+            const selectedParticipants = newGroupDraft.participants.filter((participant) => participant.accountID !== personalData.accountID);
+            newSelectedOptions = selectedParticipants.map((participant): OptionData => {
+                const baseOption = OptionsListUtils.getParticipantsOption({accountID: participant.accountID, login: participant.login, reportID: ''}, personalDetails);
+                return {...baseOption, reportID: baseOption.reportID ?? ''};
+            });
+            setSelectedOptions(newSelectedOptions);
+        }


This overwrites the selected participants with the draft values everytime the options are re-rendered making it impossible to control the selection properly. Caused issue #39295. FIx #39312

jjcoffee · 2024-04-26T14:13:39Z

src/pages/NewChatConfirmPage.tsx

+                onSelectRow={unselectOption}
+                showConfirmButton={selectedOptions.length > 1}
+                confirmButtonText={translate('newChatPage.startGroup')}
+                onConfirm={createGroup}


We missed adding a condition here to check if anything is selected before calling onConfirm - this caused an issue where CMD+enter can still proceed past the step.

Pujan92 · 2024-06-06T14:02:22Z

src/libs/ReportUtils.ts

@@ -3649,6 +3693,15 @@ function buildOptimisticChatReport(
    parentReportID = '',
    description = '',
 ): OptimisticChatReport {
+    const participants = participantList.reduce((reportParticipants: Participants, accountID: number) => {
+        const participant: ReportParticipant = {
+            hidden: false,


This is causing issue #41832, for the transaction threads this needs to be true to be consistent with BE.

iwiznia · 2024-07-12T14:38:17Z

src/languages/es.ts

@@ -1343,7 +1347,7 @@ export default {
    },
    newChatPage: {
        createChat: 'Crear chat',
-        createGroup: 'Crear grupo',
+        startGroup: 'Grupo de inicio',


This translation is wrong. I search in slack and I could not find any mention of this particular string, so I think it was not reviewed by anyone and if that's correct it means that @waterim you checked this checklist item without actually doing it:

If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

BTW changed this here #45342

s77rt · 2024-07-22T17:58:19Z

src/pages/NewChatPage.tsx

+                            confirmButtonText={selectedOptions.length > 1 ? translate('common.next') : translate('newChatPage.createChat')}
                            textInputAlert={isOffline ? [`${translate('common.youAppearToBeOffline')} ${translate('search.resultsAreLimited')}`, {isTranslated: true}] : ''}
-                            onConfirmSelection={createGroup}
+                            onConfirmSelection={selectedOptions.length > 1 ? navigateToConfirmPage : createChat}


We should have removed the requirement that a group chat needs more than 1 other participant to be created because this requirement was only applicable in the NewChatPage and could be skipped in NewChatConfirmPage causing inconsistency. Also because this was not really a requirement after all.

hoangzinh · 2024-08-29T10:24:23Z

src/pages/NewChatConfirmPage.tsx

+        return options;
+    }, [allPersonalDetails, newGroupDraft?.participants]);
+
+    const groupName = ReportUtils.getGroupChatName(participantAccountIDs ?? []);


Coming from this issue #46502, this is a case that we might miss when testing/implementing this issue. For the details, we check here #46502 (comment)

waterim added 2 commits February 28, 2024 23:38

initial commit

0452b5c

merge main

df535cb

marcaaron reviewed Feb 29, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat-32317-group_chats

49e7b96

updates, api request

91de72d

waterim added 4 commits March 6, 2024 22:51

Merge remote-tracking branch 'upstream/main' into feat-32317-group_chats

d960f50

fix errors

a31e70a

fix lint

89040f1

fix lint

24ae5ec

waterim added 3 commits March 7, 2024 19:46

Merge remote-tracking branch 'upstream/main' into feat-32317-group_chats

5ec343f

split changes, Join remove

1d69009

merge

5c0fc87

waterim added 2 commits March 7, 2024 21:56

fix lint, ta

b6c23d2

ts fix

0928709

waterim added 2 commits March 7, 2024 23:06

updated to SelectionList

481a491

Merge remote-tracking branch 'upstream/main' into feat-32317-group_chats

6fbce1f

rezkiy37 reviewed Mar 8, 2024

View reviewed changes

src/libs/actions/Report.ts Outdated Show resolved Hide resolved

rezkiy37 reviewed Mar 8, 2024

View reviewed changes

src/libs/actions/Report.ts Outdated Show resolved Hide resolved

rezkiy37 reviewed Mar 8, 2024

View reviewed changes

src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js Outdated Show resolved Hide resolved

marcaaron self-requested a review March 28, 2024 18:31

marcaaron approved these changes Mar 28, 2024

View reviewed changes

marcaaron merged commit 2b96626 into Expensify:main Mar 28, 2024
16 checks passed

melvin-bot bot added the Emergency label Mar 28, 2024

marcaaron removed the Emergency label Mar 28, 2024

github-actions bot mentioned this pull request Mar 29, 2024

Deploy Checklist: New Expensify 2024-03-29 #39222

Closed

ZhenjaHorbach mentioned this pull request Mar 29, 2024

[HOLD for payment 2024-04-15] [HOLD for payment 2024-04-09] Thread - Group chat thread header shows unavailable workspace icon #39247

Closed

6 tasks

kbecciv mentioned this pull request Mar 29, 2024

[HOLD for payment 2024-04-15] [$500] Web - Group chat - Unable to create a group chat via search input after deselecting members #39295

Closed

6 tasks

blimpich mentioned this pull request Apr 1, 2024

[HOLD for payment 2024-04-25] [$250] Group chat - User can proceed without Start group button via CMD+Enter shortcut #39323

Closed

6 tasks

melvin-bot bot mentioned this pull request Apr 2, 2024

[$250] [HOLD for payment 2024-04-09] [CRITICAL] [Groups] Frontend (Part 1): Create Group Chats + Splits. Deprecate Group DM creation. Support viewing both Group DMs and Group Chats. #32317

Closed

4 tasks

s77rt reviewed Apr 9, 2024

View reviewed changes

s77rt mentioned this pull request Apr 12, 2024

[HOLD for payment 2024-04-15] [HOLD for payment 2024-04-09] Group chat - Group chat avatar leads to not here page #39249

Closed

6 tasks

jjcoffee reviewed Apr 26, 2024

View reviewed changes

Pujan92 reviewed Jun 6, 2024

View reviewed changes

Pujan92 mentioned this pull request Jun 6, 2024

[HOLD for payment 2024-06-06] [$250] IOU - "Members" appears in IOU details page and then disappears after a few seconds #41832

Closed

6 tasks

iwiznia reviewed Jul 12, 2024

View reviewed changes

s77rt reviewed Jul 22, 2024

View reviewed changes

s77rt mentioned this pull request Jul 22, 2024

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

Closed

6 tasks

hoangzinh reviewed Aug 29, 2024

View reviewed changes

hoangzinh mentioned this pull request Aug 29, 2024

[$250] mWeb - Group - New users are not displayed in group name #46502

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Group Chats + Splits. #37458

Create Group Chats + Splits. #37458

waterim commented Feb 28, 2024 •

edited by marcaaron

Loading

marcaaron left a comment

marcaaron Feb 28, 2024

waterim commented Mar 4, 2024 •

edited

Loading

marcaaron commented Mar 5, 2024

marcaaron commented Mar 5, 2024 •

edited

Loading

waterim commented Mar 5, 2024 •

edited

Loading

marcaaron commented Mar 5, 2024

waterim commented Mar 5, 2024

waterim commented Mar 6, 2024 •

edited

Loading

marcaaron commented Mar 7, 2024

waterim commented Mar 7, 2024

waterim commented Mar 7, 2024

waterim commented Mar 7, 2024

marcaaron commented Mar 28, 2024

marcaaron commented Mar 28, 2024

melvin-bot bot commented Mar 28, 2024

OSBotify commented Mar 28, 2024

marcaaron commented Mar 28, 2024

OSBotify commented Mar 29, 2024

stitesExpensify commented Mar 29, 2024

waterim commented Mar 29, 2024

marcochavezf commented Mar 30, 2024

OSBotify commented Apr 2, 2024

s77rt Apr 9, 2024

jjcoffee Apr 26, 2024

Pujan92 Jun 6, 2024

iwiznia Jul 12, 2024

iwiznia Jul 12, 2024

s77rt Jul 22, 2024

hoangzinh Aug 29, 2024

Create Group Chats + Splits. #37458

Create Group Chats + Splits. #37458

Conversation

waterim commented Feb 28, 2024 • edited by marcaaron Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterim commented Mar 4, 2024 • edited Loading

marcaaron commented Mar 5, 2024

marcaaron commented Mar 5, 2024 • edited Loading

waterim commented Mar 5, 2024 • edited Loading

marcaaron commented Mar 5, 2024

waterim commented Mar 5, 2024

waterim commented Mar 6, 2024 • edited Loading

marcaaron commented Mar 7, 2024

waterim commented Mar 7, 2024

waterim commented Mar 7, 2024

waterim commented Mar 7, 2024

marcaaron commented Mar 28, 2024

marcaaron commented Mar 28, 2024

melvin-bot bot commented Mar 28, 2024

OSBotify commented Mar 28, 2024

marcaaron commented Mar 28, 2024

OSBotify commented Mar 29, 2024

stitesExpensify commented Mar 29, 2024

waterim commented Mar 29, 2024

marcochavezf commented Mar 30, 2024

OSBotify commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterim commented Feb 28, 2024 •

edited by marcaaron

Loading

waterim commented Mar 4, 2024 •

edited

Loading

marcaaron commented Mar 5, 2024 •

edited

Loading

waterim commented Mar 5, 2024 •

edited

Loading

waterim commented Mar 6, 2024 •

edited

Loading