[Pay 4/29][$500] 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 contributors@expensify.com 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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6432388_1711779508540.20240330_141620.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0190680b621c41acf8
• Upwork Job ID: 1774823882828201984
• Last Price Increase: 2024-04-01
• Automatic offers:
• cubuspl42 | Reviewer | 0
• gijoe0295 | Contributor | 0

[melvin-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.

App/src/pages/ShareCodePage.tsx

Line 62 in 0140912

const title = isReport ? ReportUtils.getReportName(report) : currentUserPersonalDetails.displayName ?? '';

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

App/src/pages/ShareCodePage.tsx

Line 62 in 14ff944

const title = isReport ? ReportUtils.getReportName(report) : currentUserPersonalDetails.displayName ?? '';

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

App/src/libs/ReportUtils.ts

Lines 2889 to 2895 in 14ff944

	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(', ');

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]

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

[melvin-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
  

  App/src/pages/home/HeaderView.tsx

  Line 92 in 7446c1a

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

  
  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:
  

  App/src/pages/home/HeaderView.tsx

  Line 92 in a4d01a8

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

  
  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

[cubuspl42]

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

[cubuspl42]

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. 🙂

[cubuspl42]

@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 #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.

[cubuspl42]

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]

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

[cubuspl42]

@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

[cubuspl42]

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

Screen.Recording.2024-04-03.at.19.45.36.mov

[marcaaron]

Oh ok, yes sorry I misunderstood you. I think a good best practice would be to update the proposal so there is clarity about what we decided to do. Less confusion later for future reference. Thanks for your understanding 🙇

[CortneyOfstad]

PR is still in review here 👍

[cubuspl42]

Reviewed and merged 👍

[melvin-bot]

@cubuspl42 @CortneyOfstad @marcaaron @gijoe0295 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[CortneyOfstad]

Payment Summary

• @gijoe0295 paid $500 via Upwork

Waiting for @cubuspl42 to accept the proposal in Upwork so they can be paid 👍

[cubuspl42]

I accepted 👍

Just for cross-reference, this wasn't processed in #41121 (which I didn't notice)

[nkdengineer]

@CortneyOfstad @marcaaron Please help check our decision here

[gijoe0295]

@CortneyOfstad Oh yes, @cubuspl42, @nkdengineer and me were supposed to be paid $330 each. Could you please create a refund request?

[CortneyOfstad]

Sorry about that! @gijoe0295 I requested a refund for $170, so you should see that in Upwork now 👍

@cubuspl42 — unfortunately Upwork would not let me modify the original proposal, so I had to send you a new proposal. Please let me know once you accept and I can get that paid ASAP.

@nkdengineer — I sent you a proposal in Upwork, so please let me know once you accept and I'll get that paid ASAP.

Thank you!

[CortneyOfstad]

@cubuspl42 and @nkdengineer please let me know once you accept the proposals above ^^^ Thanks!

@gijoe0295 — thanks for the refund on the overpayment and sorry about that again!

[nkdengineer]

@CortneyOfstad I accepted the offer, appreciate it 🙏

[CortneyOfstad]

Payment made!

@cubuspl42 waiting for you to accept the payment proposal — thanks!

[CortneyOfstad]

Bump @cubuspl42 ^^^

[CortneyOfstad]

Bump @cubuspl42 ^^^

[cubuspl42]

I'm sorry! I wasn't active on GitHub or Upwork in the recent days. I accepted the offer.

[CortneyOfstad]

No worries and payment made! Payment summary below!

Payment Summary

@cubuspl42 — paid $330 via Upwork
@gijoe0295 — paid $330 via Upwork
@nkdengineer — paid $330 via Upwork

Closing!