[HOLD for payment 2022-10-31] [$250] After Creating group, email address of only one user is used in alternate Text reported by @sobitneupane

[kavimuru]

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

---

Action Performed:

1. New Group > Create Group
2. Scroll down to newly created group in LHN.

Expected Result:

Comma separated emails of group members as alternateText

Actual Result:

Email of only one user is shown

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web

Version Number: v1.2.7-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: https://user-images.githubusercontent.com/43996225/192380802-709e5c68-d010-4ee0-b82e-e921f07717d5.mp4 Expensify/Expensify Issue URL: Issue reported by: @sobitneupane Slack conversation:

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @davidcardoza (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[davidcardoza]

I am not sure if the way we present the alternateText in the LHN is by design or is an actual bug. I am going to assign out to engineering to get a second pair of eyes.

[melvin-bot[bot]]

Triggered auto assignment to @justicea83 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Justicea83]

Looking into the code to see if something might be wrong here

[Justicea83]

Wanted to clarify something before I assign the internal label, you can follow the thread here.

[Justicea83]

We want to replace the alternate text with This is the beginning of your chat history with Person, Person, and Person

[melvin-bot[bot]]

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[Ollyws]

Proposal

This problem occurs because the alternateText is assigned to use the single user login in OptionsListUtils: https://github.com/Expensify/App/blob/1ff9206ec817a44a368125fec9910bfd75ce0157/src/libs/OptionsListUtils.js#L325-L327

If the desired result is a Comma separated emails of group members as alternateText we can get all the user logins for the current chat from personalDetailList and join them into a comma separated string to use as the alternateText:

+       const participantLogins = _.pluck(personalDetailList, 'login').join(', ');

        result.alternateText = (showChatPreviewLine && lastMessageText)
            ? lastMessageText
-           : Str.removeSMSDomain(personalDetail.login);
+           : Str.removeSMSDomain(participantLogins);

Result:

If the desired result is, as @Justicea83 suggested This is the beginning of your chat history with Person, Person, and Person we can get the relevant localized text and append the list of user logins to use as the alternateText:

+       const participantLogins = _.pluck(personalDetailList, 'login').join(', ');

+       if(!lastMessageText && hasMultipleParticipants){
+           lastMessageText = Localize.translate(preferredLocale, 'reportActionsView.beginningOfChatHistory') + Str.removeSMSDomain(participantLogins)
+       }

        result.alternateText = (showChatPreviewLine && lastMessageText)
            ? lastMessageText
            : Str.removeSMSDomain(personalDetail.login);

Result:

[melvin-bot[bot]]

@puneetlath, @davidcardoza, @eVoloshchak Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[puneetlath]

@eVoloshchak what do you think of the proposal?

[eVoloshchak]

@eVoloshchak what do you think of the proposal?

I wasn't able to fully test it yet, experiencing some weird problems (createOption is called multiple times, which breaks the proposed solution), still trying to figure out if it's the problem with the code or my environment.

Expect an answer in 14 hours

[eVoloshchak]

@Ollyws, your proposal doesn't work for me, but you've correctly identified the root cause of the problem

1. Correct me if I'm wrong, but createOption function is not the one responsible for alternateText for LNH. getOptionData is the correct function, we need to change the alternateText there
2. We need to match the logic implemented here, i.e.

• separate emails with commas, add "and" before the last email, add a dot after the last email ([email protected], [email protected] and [email protected].) not ([email protected], [email protected], [email protected])
• display pronouns for users if there are any

Essentially we need to make sure the text is identical in LHN and in the chat window itself when you select it

[Ollyws]

@eVoloshchak Ah strange, it is no longer working for me either after pulling the latest updates. Thanks for having a look at it anyway.

I agree getOptionData is the better place for it, perhaps we could do something like this: https://github.com/Expensify/App/blob/1ff9206ec817a44a368125fec9910bfd75ce0157/src/libs/SidebarUtils.js#L306-L310

+  const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips((personalDetailList || []).slice(0, 10), hasMultipleParticipants);

    if (result.isChatRoom || result.isPolicyExpenseChat) {
        result.alternateText = lastMessageText || subtitle;
    } else {

+        if(hasMultipleParticipants && !lastMessageText){   
+            lastMessageText = Localize.translate(preferredLocale, 'reportActionsView.beginningOfChatHistory')
+                + _.map(displayNamesWithTooltips, ({displayName, pronouns}, index) => {
+                    const formattedText = !_.isEmpty(pronouns) ? displayName + ` (${pronouns})` : displayName
    
+                    return index === displayNamesWithTooltips.length - 1 ? formattedText + '.'
+                         : index === displayNamesWithTooltips.length - 2 ? formattedText + ' ' + Localize.translate(preferredLocale, 'common.and')
+                         : index < displayNamesWithTooltips.length - 2 ? formattedText + ','
+                         : null
    
+                }).join(' ')
+        }

        result.alternateText = lastMessageText || Str.removeSMSDomain(personalDetail.login);
    }

This matches the logic in ReportWelcomeText, however an issue is ReportUtils.getDisplayNamesWithTooltips would be called twice, in the above code and here: https://github.com/Expensify/App/blob/7fbe704ac8291944a7a8533bbcfa1df0e207b182/src/components/LHNOptionsList/OptionRowLHN.js#L98-L100

This could be rectified by returning displayNamesWithTooltips at the end of GetOptionData:

result.displayNamesWithTooltips = displayNamesWithTooltips

and then using optionItem.displayNamesWithTooltips in OptionRowLHN instead of calling it there.

[melvin-bot[bot]]

@puneetlath, @davidcardoza, @eVoloshchak Huh... This is 4 days overdue. Who can take care of this?

[puneetlath]

@eVoloshchak what do you think?

[eVoloshchak]

@Ollyws's proposal looks great! Good job spotting that potential performance issue with ReportUtils.getDisplayNamesWithTooltips

🎀👀🎀 C+ reviewed @puneetlath

[melvin-bot[bot]]

📣 @Ollyws You have been assigned to this job by @puneetlath! Please apply to this job in Upwork 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 📖

[Ollyws]

Thanks guys. Has an UpWork job been created for this? I can't see it anywhere. PR should be ready within the next 24 hours.

[puneetlath]

Ah, right you are.

Internal upwork job: https://www.upwork.com/ab/applicants/1579892682555052032/job-details External upwork job: https://www.upwork.com/jobs/~01d614951b96cdb960

@Ollyws @sobitneupane @eVoloshchak can you all apply there? Thanks!

[Ollyws]

I had to make some aesthetic changes to make the code play nice with ESLint. Let me know if this looks okay and I'll proceed with the PR. Thanks.

       if (hasMultipleParticipants && !lastMessageText) {
            lastMessageText = Localize.translate(preferredLocale, 'reportActionsView.beginningOfChatHistory')
                + _.map(displayNamesWithTooltips, ({displayName, pronouns}, index) => {
                    const formattedText = !_.isEmpty(pronouns) ? `${displayName} (${pronouns})` : displayName;

                    if (index === displayNamesWithTooltips.length - 1) { return `${formattedText}.`; }
                    if (index === displayNamesWithTooltips.length - 2) { return `${formattedText} ${Localize.translate(preferredLocale, 'common.and')}`; }
                    if (index < displayNamesWithTooltips.length - 2) { return `${formattedText},`; }
                }).join(' ');
        }

[eVoloshchak]

@Ollyws, looks readable to me, let's proceed with the PR

UPD: I think this is effectively the same const formattedText = _.isEmpty(pronouns) ? displayName : '${displayName} (${pronouns})';

[Ollyws]

@eVoloshchak const formattedText = _.isEmpty(pronouns) ? displayName : '${displayName} (${pronouns})'; You would prefer it this way round?

[eVoloshchak]

@eVoloshchak const formattedText = _.isEmpty(pronouns) ? displayName : '${displayName} (${pronouns})'; You would prefer it this way round?

Yup

[puneetlath]

PR has been merged. Just waiting for deploy and then regression period.

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.18-10 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/11765

If no regressions arise, payment will be issued on 2022-10-31. :confetti_ball:

[puneetlath]

@Ollyws @sobitneupane @eVoloshchak sent hiring offers to all three of you via Upwork.

[puneetlath]

All paid. Thanks everyone!