Group chat - Share code subtitle does not include the group creator

[lanitochka17]

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: 1.4.58-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: N/A Issue reported by: Applause -Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Start chat
3. Create a group chat
4. Click on the group chat header
5. Click Share code

Expected Result:

Share code subtitle will include the group creator, as group creator is shown in every part of the group chat/

Actual Result:

Share code subtitle does not include the group creator

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/ccee62e4-dcc3-407d-9d3d-d08b3a59848d

View all open jobs on GitHub

[melvin-bot[bot]]

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

[lanitochka17]

@CortneyOfstad 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

[lanitochka17]

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

[gijoe0295]

Proposal

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

Share code subtitle does not include the group creator

What is the root cause of that problem?

In ShareCodePage, we did not cover the case of group chat when retrieving report name.

https://github.com/Expensify/App/blob/0140912da9f85a1eb17e7942a517c048c64b3c0f/src/pages/ShareCodePage.tsx#L62

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

We should modify getReportName function to cover the case of group chat:

if (isGroupChat(report)) {
    return getGroupChatName(report.participantAccountIDs ?? [], true);
}

But remember to leave the group chat logic under the thread one because any thread in group chat also has chatType: group.

What alternative solutions did you explore? (Optional)

This issue also happens in several places using getReportName so we need to investigate and fix there as well. For example (this group has 3 members):

Report details

Parent navigation subtitle (when reply a group message in thread):

Shortcut

Search page

[ShridharGoel]

Proposal

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

Share code group name doesn't include the creator.

What is the root cause of that problem?

getReportName is the method being used to fetch the title

https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/pages/ShareCodePage.tsx#L62

In ReportUtils.ts, inside getReportName, we are getting first 5 creators while excluding the current user.

https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/libs/ReportUtils.ts#L2889-L2895

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

Current user should be included if the number of total people is <= 5.

    const allParticipantAccountIDs = report?.participantAccountIDs ?? []
    if (allParticipantAccountIDs.length <= 5) {
        return allParticipantAccountIDs.map((accountID) => getDisplayNameForParticipant(accountID, isMultipleParticipantReport)).join(', ');
    }
    const participantAccountIDs = report?.participantAccountIDs?.slice(0, 6) ?? [];

    const participantsWithoutCurrentUser = participantAccountIDs.filter((accountID) => accountID !== currentUserAccountID);
    const isMultipleParticipantReport = participantsWithoutCurrentUser.length > 1;
    if (participantsWithoutCurrentUser.length > 5) {
        participantsWithoutCurrentUser.pop();
    }
    return participantsWithoutCurrentUser.map((accountID) => getDisplayNameForParticipant(accountID, isMultipleParticipantReport)).join(', ');

This will start including the creator at other places also, whenever the total participants is 5 or less.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0190680b621c41acf8

[melvin-bot[bot]]

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

[CortneyOfstad]

@cubuspl42 We have some proposals above — if you have any feedback, feel free to share. Thanks!

[nkdengineer]

Proposal

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

• Group chat - Share code subtitle does not include the group creator

What is the root cause of that problem?

• in here, we do not consider the case report is group chat as we did in other component, for example, HeaderView

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

Option 1:

• We need to update this to:

    const title = isReport
        ? isGroupChat && !ReportUtils.isChatThread(report)
            ? ReportUtils.getGroupChatName(report.participantAccountIDs ?? [], true)
            : ReportUtils.getReportName(report)
        : currentUserPersonalDetails.displayName ?? '';

Option 2:

• We can add the below logic to getReportName function:

    const isGroupChatReport = isGroupChat(report) || isDeprecatedGroupDM(report);
    if(isGroupChatReport && !isChatThread(report)){
        return getGroupChatName(report.participantAccountIDs ?? [], true) ?? ''
    }

• The main issue is fixed after the above change.
• Then update other similar components. For example, this logic https://github.com/Expensify/App/blob/7446c1a711c7740283959ffe04326a552efc676e/src/pages/home/HeaderView.tsx#L92 will become:

    const title = ReportUtils.getReportName(reportHeaderData);

What alternative solutions did you explore? (Optional)

Addtional bug:

• Currently, if we create a group chat with [user A, user B, user C, user D (me)]. Then we send any message ("Hello" for example) to report and reply in the thread. We can see that, the header title is "user D". It is a bug because it should be "Hello" in this case. RCA is that, any thread in group chat is also has chatType: group. So we also need to update this: https://github.com/Expensify/App/blob/a4d01a87b242eef06af37db965bd135745e342e5/src/pages/home/HeaderView.tsx#L92 to:

    const title = (isGroupChat && !isChatThread) ? ReportUtils.getGroupChatName(report.participantAccountIDs ?? [], true) : ReportUtils.getReportName(reportHeaderData);

• We also need to apply this change to other components, such as LHN

[jakub-trzebiatowski]

@gijoe0295 @nkdengineer

Both your proposals are relatively similar, I understand that @nkdengineer discovered another bug and provides a solution of that bug, too.

But what bothers me is that you both suggest to synchronize the code, so it's the same, instead of de-duplicating it. Why is that?

[nkdengineer]

@cubuspl42 I do not fully understand the term "de-duplicating it" that you mentioned. But I assume that you mean is that, we should create a function, named getReportName and use it in LHN, Header, Sharecode, ... to reduce the duplication. Is that right?

[nkdengineer]

@cubuspl42 I updated my proposal (Option 2)

[jakub-trzebiatowski]

should create a function, named getReportName and use it in LHN, Header, Sharecode, ... to reduce the duplication.

Yeah, something like that.

De-duplication is a broad class of actions performed to reduce duplication, which is typically understood as the presence of the same ("copied-and-pasted") blocks of code or very similar blocks of code in the codebase.

I didn't suggest any exact strategy of de-duplication in this specific case; it's typically easier to spot the duplication than to solve it. 🙂

[jakub-trzebiatowski]

@nkdengineer What you suggest in Option 2 (extending getReportName) sounds elegant.

Have you ensured there is no specific reason why the code wasn't phrased this way? Are there any spots where we might want the "old" getReportName behavior in the case of group chats?

[nkdengineer]

Are there any spots where we might want the "old" getReportName behavior in the case of group chats

There is a lot of logic that we use getReportName. And I cannot verify that whether we want to use the old getReportName behavior right now

[gijoe0295]

Have you ensured there is no specific reason why the code wasn't phrased this way?

This logic was introduced in https://github.com/Expensify/App/pull/28991 where we want to ensure the order of display names matches that of avatars in welcome text, but only in report header and LHN. We no longer used multiple avatars for group chats so I think it's safe to move the isGroupChat check to getReportName.

[jakub-trzebiatowski]

Thanks everyone for their input!

I approve the proposal by @gijoe0295

The element that shifted my decision towards their proposal is the investigation of the reason why the code is as it is, which is very important information when changing something.

C+ reviewed 🎀 👀 🎀

[melvin-bot[bot]]

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

[jakub-trzebiatowski]

@gijoe0295 While this is not related, it would be great if you added pronouns and a first name to your GitHub account; it can help refer to a person.

[nkdengineer]

@cubuspl42 I see that the selected proposal will lead to regression with a thread in group report. Can you help check again. Also, my proposal does not encountered that regression

[jakub-trzebiatowski]

@nkdengineer

Do you mean what you described here...

Currently, if we create a group chat with [user A, user B, user C, user D (me)]. Then we send any message ("Hello" for example) to report and reply in the thread. We can see that, the header title is "user D".

...? I thought that this is another problem which also happens on main, not related to any proposed solutions.

[nkdengineer]

@cubuspl42 No. I mean this case, you can see the below screen recording.

https://github.com/Expensify/App/assets/161821005/98af2e9f-3bcf-43f0-aaf4-effa70e0b734

[nkdengineer]

In the above, the sharecode's title in the thread which belongs to the group chat displayed incorrectly. Expected: "Hello". Actual: "[email protected]". This bug does not appear in main branch

[gijoe0295]

We no longer used multiple avatars for group chats so I think it's safe to move the isGroupChat check to getReportName.

If we moved the group check into getReportName, we wouldn't need to worry about the issue mentioned above. And I think it's the C+'s approved solution though I didn't explicitly mention in my proposal.

[nkdengineer]

@gijoe0295

• In your main solution, you say that we will apply the change same as Header View. If we do like that, the regression will appear
• In your alternative solution, I do not see any suggestion about "moved the group check into getReportName", and how to implement it. You just said "need to investigate and fix there as well".

[gijoe0295]

That's my bad not updated the proposal accordingly but my investigation here already implied that. Anyway I'm happy if @nkdengineer got assigned for this issue. I appreciated their input in finding the regression and the final solution before me.

[CortneyOfstad]

Posted in VIP-VSB for confirmation here

[jakub-trzebiatowski]

Thanks for the constructive discussion!

Both Contributors (@gijoe0295, @nkdengineer) had constructive input:

• @gijoe0295 was the first one to provide a solution and did a valuable investigation
• @nkdengineer provided a proposal with improvements preventing the introduction of a regression

We could leave the assignment, but use @nkdengineer improvements and split the compensation:

• $330 for @gijoe0295
• $330 for @nkdengineer
• $330 for me

[louisdeb]

I made a proposal for a slightly worse solution to @nkdengineer's Option 2 (before seeing this thread and seeing the other thread was a dupe). I have one comment though, could someone more familiar with the code than me check why ReportUtils.getGroupChatName returns string | undefined and not just a string? To me it appears it can't return undefined and removing it allows some cleaning of ?? checks.

[srikarparsi]

This breakdown sounds good to me. @CortneyOfstad does it sound good for you as well?