[$250] mWeb - Login - Inconsistency between the mention displayed in chat and LHN preview

[IuliiaHerets]

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: 9.0.62-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5223745 Issue reported by: Applause Internal Team

Action Performed:

1.Log in the app 2. Open any conversation 3. In the composer, type '@[email protected]'

Expected Result:

The display name for the mention result is Concierge

Actual Result:

There is an inconsistency between the mention in the chat and the LHN preview. In the chat the email is displayed, while in LHN, the name is displayed.

Workaround:

Unknown

Platforms:

• [ ] Android: Standalone
• [ ] Android: HybridApp
• [x] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/15c9e857-99ea-4fb3-8745-fddba734a250

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021858426697613842955
• Upwork Job ID: 1858426697613842955
• Last Price Increase: 2024-11-25

Issue Owner

Current Issue Owner: @youssef-lr

[melvin-bot[bot]]

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[klajdipaja]

Edited by proposal-police: This proposal was edited at 2024-11-18 15:53:49 UTC.

Proposal

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

The displayed text when the Expensfiy Concierge is mentioned is different between the comment and previewed message in LHN

What is the root cause of that problem?

The incosistency comes from having two different logics in how the text is constructed for mentioned accounts. In the chat we use MentionUserRenderer component to render the content of the action item when we have a comment that mentions another account. In this line we check wether we should display a formated phone number or the display name depending on if the user has logged in with a phone number or email.

https://github.com/Expensify/App/blob/63c8dd2bb44deb2d62e223530fa0d734f1cfba5c/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx#L62 But the second part of the condition never validates and is a de facto dead code because of this part of the format phone number function: https://github.com/Expensify/App/blob/63c8dd2bb44deb2d62e223530fa0d734f1cfba5c/src/libs/LocalePhoneNumber.ts#L26-L28 We are returning there the number that was passed as input when the input was not a valid number. And since we pass the user login in LocalePhoneNumber.formatPhoneNumber(user?.login ?? '') which for email accounts is the actual email we end up having as a display in mentionDisplayText the user email for all cases. In that case, we have another bug where you cannot see the actual display name of the user but their email address.

On the other end the LHN preview uses in OptionRowLHNDatato build the display data for each row uses this function: https://github.com/Expensify/App/blob/63c8dd2bb44deb2d62e223530fa0d734f1cfba5c/src/libs/SidebarUtils.ts#L272 Which uses this function to parse the display name from the last message of the report:

https://github.com/Expensify/App/blob/63c8dd2bb44deb2d62e223530fa0d734f1cfba5c/src/libs/Parser.ts#L39-L44

Which returns the display name from the login info of the mentioned account in onyx: https://github.com/Expensify/App/blob/63c8dd2bb44deb2d62e223530fa0d734f1cfba5c/src/libs/Parser.ts#L18

So the inconsistency is from the message using the plain user.login info which contains the email address and the LHN showing the display name of the user.

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

Assuming that we want to show the display name as Concierge in both cases we have to split the solution in two parts. Part 1. Fix the mention text displayed in the Comment to show Concierge instead of @[email protected]. Part 2. Fix the preview in LHN to show @Concierge instead of @Expensify Concierge.

Options Part 1.

We have a few options how to fix the MentionUserRenderer so that it shows the display name: Option a. In the formatPhoneNumber return an empty string if the number is not valid:

   // do not parse the string, if it doesn't contain the SMS domain and it's not a phone number
    if (number.indexOf(CONST.SMS.DOMAIN) === -1 && !CONST.REGEX.DIGITS_AND_PLUS.test(number)) {
        return '';
    }

Easy fix but I'm afraid it might have a regression in one of the many uses that it has

Option b. Flip the parts of the condition in the mentionDisplayText so that we check the displayName first:

        mentionDisplayText = PersonalDetailsUtils.getDisplayNameOrDefault(user) ||  LocalePhoneNumber.formatPhoneNumber(user?.login ?? '');

I tested this one and it works well for phone number accounts as well where it shows the full name that the user has created during the on boarding phase.

Option C. Check what the login

If(Str.isSMSLogin(passedPersonalDetails?.login)){
     mentionDisplayText = LocalePhoneNumber.formatPhoneNumber(user?.login ?? '');
}else{
   mentionDisplayText = PersonalDetailsUtils.getDisplayNameOrDefault(user) ||  LocalePhoneNumber.formatPhoneNumber(user?.login ?? '');
}

With those we and up having the Display Name from the logged user in the mention but for Concierge we change that explictely here:

https://github.com/Expensify/App/blob/63c8dd2bb44deb2d62e223530fa0d734f1cfba5c/src/libs/PersonalDetailsUtils.ts#L71-L73 and we end in the end having as Display Name Expensify Concierge we use Concierge.

Options Part 2.

I have tried a couple of things to change the LHN Preview to Concierge but they all have some kind of regression. I think this part should be treated on the BE side as we rely on the lastMessageText coming from the BE and showing it in the LHN mostly as it is.

The change below that I had in my original proposal had a regression:

We need to add the same logic for what we show in LHN Preview by changing the mapper in Parser.ts to something like this:

accountIDToNameMap[personalDetails.accountID] = getDisplayName(personalDetails);

where we use this new function:

const getDisplayName=(personalDetails) =>{
    if (personalDetails?.accountID === CONST.ACCOUNT_ID.CONCIERGE) {
        return CONST.CONCIERGE_DISPLAY_NAME;
    }
    return personalDetails.login ?? personalDetails.displayName ?? '';
}

An alternative to that one which by far from what I have tested does not have a regression would be to derive the text message of the report by using the latest action and their originalMessage.html which contains all the mentions in the expected format.

To do that we can extend the logic for building the text that the LHN uses for each row lastMessageTextFromReport from this function: https://github.com/Expensify/App/blob/bd6c90d8493baf70d61937188c94815650f18988/src/libs/OptionsListUtils.ts#L544 We can add a new case in there that would go through all the mentions in the Comment html and map each mention to the Display Name. When doing that we can add a special check that when the mentioned account is the Concierge instead of displaying the Display Name from the BE we can return a const Concierge as we have done for the MentionUserRenderer.

else if (lastReportAction?.actionName && ReportActionUtils.isAddCommentAction(lastReportAction)) {
        const mentionUserRegex = /<mention-user accountID="(\d+)" *\/>/gi;
        const accountIDToName: Record<string, string> = {};
        const {html: originalMessageHtml} = ReportActionUtils.getOriginalMessage(lastReportAction) ?? {};
        const accountIDs = Array.from(originalMessageHtml?.matchAll(mentionUserRegex), (mention) => Number(mention[1]));
        const getDisplayName = (personalDetails: unknown): string => {
            if (!personalDetails) {
                return;
            }
            return personalDetails.login ?? (personalDetails?.displayName as string);
        };

        if (accountIDs && originalMessageHtml) {
            const personalDetailsForAccountIDs = getPersonalDetailsForAccountIDs(accountIDs, personalDetailList);
            accountIDs.forEach((id) => (accountIDToName[id] = getDisplayName(personalDetailsForAccountIDs[id])));
        }

        if (accountIDToName) {
            lastMessageTextFromReport = Str.removeSMSDomain(Parser.htmlToText(originalMessageHtml, {accountIDToName}));
        }
    }

We need to also modify the function to include all the personalDetails:

getLastMessageTextForReport(
    report: OnyxEntry<Report>,
    lastActorDetails: Partial<PersonalDetails> | null,
    personalDetailList: OnyxInputOrEntry<PersonalDetailsList>,
    policy?: OnyxEntry<Policy>,
): string {

And when we use this function we have to pass the personalDetails also:

1. Convert this line: https://github.com/Expensify/App/blob/4c44827398f884af626872c33f64dd358d19eff4/src/components/LHNOptionsList/LHNOptionsList.tsx#L168 to this:

 const lastMessageTextFromReport = OptionsListUtils.getLastMessageTextForReport(itemFullReport, lastActorDetails, personalDetails, itemPolicy);

2. This line: https://github.com/Expensify/App/blob/4c44827398f884af626872c33f64dd358d19eff4/src/libs/SidebarUtils.ts#L422 into this:

 lastMessageTextFromReport = OptionsListUtils.getLastMessageTextForReport(report, lastActorDetails, personalDetails, policy);

3. This line: https://github.com/Expensify/App/blob/4c44827398f884af626872c33f64dd358d19eff4/src/libs/OptionsListUtils.ts#L714 into this:

        const lastMessageTextFromReport = getLastMessageTextForReport(report, lastActorDetails, personalDetails);

What alternative solutions did you explore? (Optional)

[klajdipaja]

Updated proposal https://github.com/Expensify/App/issues/52584#issuecomment-2482787909 as the last section mentioend caused a regression. I suggest that change to be done in the BE. Or we align our front end to the same value the BE brings us for the mention name.

[klajdipaja]

Updated proposal https://github.com/Expensify/App/issues/52584#issuecomment-2482787909 with another option for the 2nd part of the issue that so far is not causing any regressions.

[hungvu193]

My first thought was that this issue should be fixed from Backend. But I'll take a closer look at the proposal above this week if It can change my mind 😄

[greg-schroeder]

Thanks @hungvu193 - let me know if does need to be updated to Internal

[hungvu193]

We don't format the lastMessageText if it includes mention yet, so the lastMessageText with mention is currently an message from our BE.

@klajdipaja If you can provide the code for your Options Part 2. without any code warning and clean, I think we can consider it. Also can you please provide an evidence of solution? I think The lastMessageText should display the email like inside report.

[klajdipaja]

@hungvu193 I will provide that shortly but first I'd like to confirm the expectation. In the comment in the chat it is expected that the mention shows @concierge.exepnsify.com as it currently does? From the code on this line: https://github.com/Expensify/App/blob/63c8dd2bb44deb2d62e223530fa0d734f1cfba5c/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx#L62 It sounds like the author of the code intended to do something like if the user has logged in with phone show the formatted phone number in the @mention otherwise try to show the display name of the user. But due to the return value of the LocalePhoneNumber.formatPhoneNumber being always true-ish we never get an evaluation of the second part of the condition.

In the LHN preview is then expected to have the same @concierge.exepnsify.com?

[klajdipaja]

Updated Options Part 2 in the proposal https://github.com/Expensify/App/issues/52584#issuecomment-2482787909 with clean code as suggested by @hungvu193. I have created this draft PR with those changes: https://github.com/Expensify/App/pull/52913 The outcome of that is this:

https://github.com/user-attachments/assets/c95a250d-bef0-4426-82a4-85ea8db3735e

[hungvu193]

Thanks I'll take other look today!

[melvin-bot[bot]]

@hungvu193, @greg-schroeder Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[hungvu193]

Still under review

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[hungvu193]

Thanks @klajdipaja for the detailed solution. After I took a few tests and this issue is only reproducible with report or self DM, not with group chat. I think the best way to handle this is fix it from our BE.

🎀 👀 🎀

[melvin-bot[bot]]

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

[klajdipaja]

@hungvu193 what about this part of the proposal. Is it desired that in the message in chat we display the email instead of Concierge ?

In this line we check wether we should display a formated phone number or the display name depending on if the user has logged in with a phone number or email.

 mentionDisplayText = LocalePhoneNumber.formatPhoneNumber(user?.login ?? '') || PersonalDetailsUtils.getDisplayNameOrDefault(user); 

But the second part of the condition never validates and is a de facto dead code because of this part of the format phone number function:

 if (number.indexOf(CONST.SMS.DOMAIN) === -1 && !CONST.REGEX.DIGITS_AND_PLUS.test(number)) { 
     return number; 
 } 

We are returning there the number that was passed as input when the input was not a valid number.

[melvin-bot[bot]]

@youssef-lr @hungvu193 @greg-schroeder this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[hungvu193]

@hungvu193 what about this part of the proposal. Is it desired that in the message in chat we display the email instead of Concierge ?

Yes, I think that's expected.

[klajdipaja]

@hungvu193 in that case it seems like a happy accident not like it was intended by the code

[melvin-bot[bot]]

@youssef-lr, @hungvu193, @greg-schroeder Huh... This is 4 days overdue. Who can take care of this?

[greg-schroeder]

bump @youssef-lr on contributor assignment

[youssef-lr]

@greg-schroeder does mentioning Concierge do anything in the product right now? I'm not sure this bug is something real world users will run into otherwise and I'm not sure it's worth adding a specific check to avoid it.

[greg-schroeder]

I don't think it does. If you're not passionate about it we can close for now until it's encountered by real world users. I'll close, but please reopen if you strongly disagree.