[HOLD for payment 2024-03-07] [$500] Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview #33740

lanitochka17 · 2023-12-29T01:50:06Z

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.19-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Split bill with at least 5 people with display name
Hover over counter on the split preview

Expected Result:

The tooltip shows the display name for participants that have a display name

Actual Result:

The tooltip shows the contact details of the participants

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Bug6328028_1703813445736.20231229_084143.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0194c00ce94ea69983
Upwork Job ID: 1740550740664324096
Last Price Increase: 2024-01-05
Automatic offers:

dukenv0307 | Contributor | 28091548

melvin-bot · 2023-12-29T01:50:11Z

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

melvin-bot · 2023-12-29T01:50:11Z

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

melvin-bot · 2023-12-29T01:50:13Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-12-29T01:50:16Z

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

unidev727 · 2023-12-29T02:13:13Z

Proposal

from: @unicorndev-727

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

The compose box displays duplicate characters upon pressing a single key

What is the root cause of that problem?

The root cause is that we shows the logins not full name ,FirstName and lastName, when we create icons data here.

App/src/libs/OptionsListUtils.js

Lines 134 to 150 in 595bf40

    
           function getAvatarsForAccountIDs(accountIDs, personalDetails, defaultValues = {}) { 
        
               const reversedDefaultValues = {}; 
        
               _.map(Object.entries(defaultValues), (item) => { 
        
                   reversedDefaultValues[item[1]] = item[0]; 
        
               }); 
        
               return _.map(accountIDs, (accountID) => { 
        
                   const login = lodashGet(reversedDefaultValues, accountID, ''); 
        
                   const userPersonalDetail = lodashGet(personalDetails, accountID, {login, accountID, avatar: ''}); 
        
                   return { 
        
                       id: accountID, 
        
                       source: UserUtils.getAvatar(userPersonalDetail.avatar, userPersonalDetail.accountID), 
        
                       type: CONST.ICON_TYPE_AVATAR, 
        
                       name: userPersonalDetail.login, 
        
                   }; 
        
               });

App/src/components/MultipleAvatars.tsx

Line 108 in 595bf40

    
           const tooltipTexts = useMemo(() => (shouldShowTooltip ? icons.map((icon) => icon.name) : ['']), [shouldShowTooltip, icons]);

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

We need to update this part to show full name here

function getAvatarsForAccountIDs(accountIDs, personalDetails, defaultValues = {}) {
    const reversedDefaultValues = {};
    _.map(Object.entries(defaultValues), (item) => {
        reversedDefaultValues[item[1]] = item[0];
    });

    return _.map(accountIDs, (accountID) => {
        const login = lodashGet(reversedDefaultValues, accountID, '');
        const userPersonalDetail = lodashGet(personalDetails, accountID, {login, accountID, firstName, lastName, avatar: ''});

        return {
            id: accountID,
            source: UserUtils.getAvatar(userPersonalDetail.avatar, userPersonalDetail.accountID),
            type: CONST.ICON_TYPE_AVATAR,
            name: userPersonalDetail.firstName || userPersonalDetail.lastName ? `${userPersonalDetail.firstName ?? ''} ${userPersonalDetail.lastName ?? ''}` : userPersonalDetail.login,
        };
    });
}

screen-capture.3.webm

What alternative solutions did you explore? (Optional)

 name: ReportUtils.getDisplayNameForParticipant(accountId)

We can use getDisplayNameForParticipant instead of setting full name manually here.

dukenv0307 · 2023-12-29T02:25:44Z

Proposal

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

The tooltip shows the contact details of the participants

What is the root cause of that problem?

In here, we're not retrieving the proper name to display for the user, we're only using the login.

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

We should use getDisplayNameForParticipant here to get the display name, it already handles all the cases we need.

name: ReportUtils.getDisplayNameForParticipant(accountID)

The cases are:

Optimistic personal detail
Hidden
Long name (which uses the personalDetails.displayName)
...
All are handled in that method and the method is already used everywhere we want to get the user display name as well, so we can just use that here.

We can adjust the shouldUseShortForm, shouldFallbackToHidden of that method depending on design opinion, but I think the default values are good enough for us in this case.

Additionally, we should make sure to use the same code when displaying the tooltip on Avatar and when displaying the concatenated text for extra users, by creating an util like getUserDetailsTooltipText that will accept a list of accountIDs, and return the list of tooltip text based on the those accountIDs, then we can reuse between the BaseUserDetailsTooltip and the concatenated tooltips that we have, more detail here.

What alternative solutions did you explore? (Optional)

There might be other places that are still using the login, if there're any other such cases, we can use the same solution

unidev727 · 2023-12-29T02:34:05Z

Proposal updated. Just added an alternative solution.
But it seems good to get full name immediately without calling extra function, I think.

dukenv0307 · 2023-12-29T02:38:26Z

Proposal updated with minor changes for clarity, also just so there's no confusion, it was the first to recommend using getDisplayNameForParticipant

kaushiktd · 2024-01-01T08:26:56Z

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

Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview

What is the root cause of that problem?

https://github.com/Expensify/App/blob/main/src/libs/OptionsListUtils.js#L148

`name: userPersonalDetail.login,`

The participant details are retrieved from the system using the userPersonalDetail object.
The name attribute in the tooltip generation code (userPersonalDetail.login) is causing the issue by fetching email addresses (login) instead of display names.

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

Logic ensures that the displayName is not empty and defaults to the email when the name is not set which indicates that the displayName field is consistently set either with a name or an email, which effectively serves the purpose of a display name.
Therefore, using the displayName directly for the name attribute in your code should suffice:

return { id: accountID, source: UserUtils.getAvatar(userPersonalDetail.avatar, userPersonalDetail.accountID), type: CONST.ICON_TYPE_AVATAR, name: userPersonalDetail.displayName, // Using the displayName directly 
};

Video:

https://drive.google.com/file/d/1m-cTSQtsJq15gAQp31efnN78xnYnbIKA/view?usp=sharing

On a side note, we do not need an expression like the one provided (userPersonalDetail.firstName || userPersonalDetail.lastName ? ...) to determine the name attribute for the user.

melvin-bot · 2024-01-01T12:25:04Z

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

melvin-bot · 2024-01-01T18:22:42Z

@JmillsExpensify, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

parasharrajat · 2024-01-02T11:50:14Z

@dukenv0307's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot · 2024-01-02T11:50:22Z

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

parasharrajat · 2024-01-02T11:51:28Z

@unicorndev-727

Please re-state the problem that we are trying to solve in this issue.
The compose box displays duplicate characters upon pressing a single key

Please mind these questions they are important.

cead22 · 2024-01-02T18:37:22Z

It looks like we identified the root cause for this particular occurrence, but given other tooltips on avatars worked like they should, doesn't that mean we're using different code to display tooltips on avatars in different places?

There might be other places that are still using the login, if there're any other such cases, we can use the same solution

This makes the proposed solution sound incomplete, and I think our solution here should be one that makes it so we always use the same code for the same functionality -- in this case, displaying tooltips on avatars. Is that possible?

dukenv0307 · 2024-01-04T14:22:36Z

It looks like we identified the root cause for this particular occurrence, but given other tooltips on avatars worked like they should, doesn't that mean we're using different code to display tooltips on avatars in different places?

@cead22 Other tooltips on avatars work like they should because they are relying on UserDetailsTooltip, which also uses getDisplayNameForParticipant

App/src/components/UserDetailsTooltip/BaseUserDetailsTooltip/index.tsx

Line 21 in f7e61f4

    
           let userDisplayName = ReportUtils.getDisplayNameForParticipant(accountID) ?? (userDetails.displayName ? userDetails.displayName.trim() : '');

So technically the name of those icons are still wrong, just the tooltip text is correct because the BaseUserDetailsTooltip will retrieve the name again based on accountID

This case is for displaying the extra users that will be compressed into 1 tooltip, where it will use a concatenated strings of icon.name

App/src/components/MultipleAvatars.tsx

Line 217 in f7e61f4

    
           text={tooltipTexts.slice(avatarRows.length * maxAvatarsInRow - 1, avatarRows.length * maxAvatarsInRow + 9).join(', ')}

This makes the proposed solution sound incomplete, and I think our solution here should be one that makes it so we always use the same code for the same functionality -- in this case, displaying tooltips on avatars. Is that possible?

I think we can maybe create an util like getUserDetailsTooltipText that will accept a list of icons, and return the list of tooltip text based on the accountId of those icons, then we can reuse between the BaseUserDetailsTooltip and the concatenated tooltips that we have.

But I think we still should fix so that the icon.name is correct, same as what the tooltip will display, as suggested in my proposal. So it's more like the tooltips on avatars "happen" to be correct because of the UserDetailsTooltip implementation, but the icon name is still wrong in both the avatar and the concatenated extra users tooltip.

What do you think?

cead22 · 2024-01-04T14:52:19Z

Thanks for the detailed explanation. That sounds good to me! The only suggestion I have on this idea is to make getUserDetailsTooltipText accept accountIDs, and not icons to pull accountIDs from

dukenv0307 · 2024-01-04T15:04:49Z

@cead22 sure thing! Proposal updated to include the getUserDetailsTooltipText.

melvin-bot · 2024-01-05T16:00:15Z

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

cead22 · 2024-01-05T22:44:08Z

There might be other places that are still using the login

Thanks @dukenv0307, can you check if there are? Ideally updating those places would be part of this solution.

@parasharrajat what do you think about the updated proposal?

melvin-bot · 2024-02-29T23:30:32Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2024-02-29T23:30:35Z

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

Prioritize display name to display in tooltip text #34213

If no regressions arise, payment will be issued on 2024-03-07. 🎊

For reference, here are some details about the assignees on this issue:

@parasharrajat requires payment through NewDot Manual Requests
@dukenv0307 requires payment automatic offer (Contributor)

melvin-bot · 2024-02-29T23:30:36Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
[@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@parasharrajat] Determine if we should create a regression test for this bug.
[@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

cead22 · 2024-03-13T00:51:33Z

On payment stage, so not overdue

melvin-bot · 2024-03-18T19:09:44Z

@cead22, @JmillsExpensify, @parasharrajat, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

JmillsExpensify · 2024-03-19T00:00:51Z

@parasharrajat Are you able to complete the BZ checklist above, so we can get payments kicked off?

JmillsExpensify · 2024-03-19T00:04:57Z

Posting my understanding of the payment summary in the meantime though, assuming no regressions:

Contributor: @dukenv0307 $500
Contributor+: @parasharrajat $500

parasharrajat · 2024-03-19T08:37:40Z

I tried finding the root cause PR but history has become quite confusing which is creating a challenge in finding the root cause PR. I think #20473 might be behind it but couldn't validate it as the App does not allow signing in at that commit.

So I am going to leave that part from the checklist.

parasharrajat · 2024-03-19T08:40:31Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Not Found
[@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: NA
[@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not needed
[@parasharrajat] Determine if we should create a regression test for this bug. Yes
[@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

1

Create a group chat with at least 5 users with the custom display name and have new accounts.
Create a split bill with all participants in the group
Hover the counter in the split bill preview.
Verify that the display names of these users are displayed.

2

Go to workspace > Invite
Choose some recent contact and enter some new users after this. Make sure the total invited users are more than 8.
Go to the next page
Hover the count number on the avatar previews and verify that the tooltip text is displayed correctly for new users (display the email or phone number).
Hover on the user with the custom display name in LHN and verify that the display name is displayed.

Do you agree 👍 or 👎 ?

parasharrajat · 2024-03-20T06:03:24Z

@JmillsExpensify Feel free to close it if nothing is pending here. Thanks. I will request it later.

JmillsExpensify · 2024-03-23T20:00:00Z

Thank you! All good on the root cause for this one. I'll go ahead and create a regression test request now and then close this issue.

dukenv0307 · 2024-04-01T04:06:23Z

Posting my understanding of the payment summary in the meantime though, assuming no regressions:

@JmillsExpensify It seems payment wasn't made here yet, would you mind reopening the issue to handle that?

Thank you!

dukenv0307 · 2024-04-09T16:54:03Z

@JmillsExpensify Friendly bump on this 🙏

JmillsExpensify · 2024-04-11T18:13:04Z

Oh I'm sorry I missed this payment earlier. Payment issued!

parasharrajat · 2024-05-13T06:41:47Z

Payment requested as per #33740 (comment)

JmillsExpensify · 2024-05-19T16:42:09Z

Need someone else on the BZ team to confirm the payment summary before I can approve.

mallenexpensify · 2024-05-20T17:25:23Z

Contributor: @dukenv0307 paid $500 via Upwork.
Contributor+: @parasharrajat owed $500 via NewDot

JmillsExpensify · 2024-05-31T03:35:54Z

$500 approved for @parasharrajat

lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 29, 2023

melvin-bot bot changed the title ~~Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview~~ [$500] Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview Dec 29, 2023

melvin-bot bot assigned JmillsExpensify Dec 29, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 29, 2023

melvin-bot bot assigned parasharrajat Dec 29, 2023

melvin-bot bot added the Overdue label Jan 1, 2024

melvin-bot bot removed the Overdue label Jan 2, 2024

melvin-bot bot assigned cead22 Jan 2, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Feb 29, 2024

melvin-bot bot changed the title ~~[$500] Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview~~ [HOLD for payment 2024-03-07] [$500] Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview Feb 29, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Feb 29, 2024

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Mar 7, 2024

melvin-bot bot added Overdue and removed Overdue labels Mar 13, 2024

melvin-bot bot removed the Overdue label Mar 19, 2024

JmillsExpensify closed this as completed Mar 23, 2024

[HOLD for payment 2024-03-07] [$500] Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview #33740

[HOLD for payment 2024-03-07] [$500] Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview #33740

Comments

lanitochka17 commented Dec 29, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Dec 29, 2023

melvin-bot bot commented Dec 29, 2023

melvin-bot bot commented Dec 29, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Dec 29, 2023

unidev727 commented Dec 29, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

dukenv0307 commented Dec 29, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

unidev727 commented Dec 29, 2023 • edited Loading

dukenv0307 commented Dec 29, 2023 • edited Loading

kaushiktd commented Jan 1, 2024

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

What is the root cause of that problem?

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

Video:

melvin-bot bot commented Jan 1, 2024

melvin-bot bot commented Jan 1, 2024

parasharrajat commented Jan 2, 2024

melvin-bot bot commented Jan 2, 2024

parasharrajat commented Jan 2, 2024

cead22 commented Jan 2, 2024

dukenv0307 commented Jan 4, 2024

cead22 commented Jan 4, 2024

dukenv0307 commented Jan 4, 2024

melvin-bot bot commented Jan 5, 2024

cead22 commented Jan 5, 2024 • edited Loading

melvin-bot bot commented Feb 29, 2024

melvin-bot bot commented Feb 29, 2024

melvin-bot bot commented Feb 29, 2024

cead22 commented Mar 13, 2024

melvin-bot bot commented Mar 18, 2024

JmillsExpensify commented Mar 19, 2024

JmillsExpensify commented Mar 19, 2024

parasharrajat commented Mar 19, 2024

parasharrajat commented Mar 19, 2024

Regression Test Steps

1

2

parasharrajat commented Mar 20, 2024

JmillsExpensify commented Mar 23, 2024

dukenv0307 commented Apr 1, 2024

dukenv0307 commented Apr 9, 2024

JmillsExpensify commented Apr 11, 2024

parasharrajat commented May 13, 2024

JmillsExpensify commented May 19, 2024

mallenexpensify commented May 20, 2024

JmillsExpensify commented May 31, 2024

lanitochka17 commented Dec 29, 2023 •

edited by melvin-bot bot

Loading

unidev727 commented Dec 29, 2023 •

edited

Loading

dukenv0307 commented Dec 29, 2023 •

edited

Loading

unidev727 commented Dec 29, 2023 •

edited

Loading

dukenv0307 commented Dec 29, 2023 •

edited

Loading

cead22 commented Jan 5, 2024 •

edited

Loading