[$1000] Web - User details tooltip in workspace members list is different

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to any workspace
2. Go to members
3. Hover over the members icon

Expected Result:

Tooltip should show display name in bold and login in gray

Actual Result:

Only login is shown in gray, no display name

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.35-5
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
Notes/Photos/Videos: Any additional supporting documentation

bandicam.2023-07-01.13-40-56-685.1.mp4

Recording.5285.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @chiragxarora
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688199036733839

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c11ba48600bf6205
• Upwork Job ID: 1676186298947235840
• Last Price Increase: 2023-07-04

[melvin-bot]

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

[melvin-bot]

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

[kbecciv]

Proposal

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

user details tooltip in workspace members list is different, display name is not present

What is the root cause of that problem?

Root cause of the issue is the missing accountID in the icons prop of <OptionRow/> here

App/src/pages/workspace/WorkspaceMembersPage.js

Lines 323 to 340 in c0ece67

	<OptionRow
	boldStyle
	isDisabled={disabled}
	option={{
	text: props.formatPhoneNumber(item.displayName),
	alternateText: props.formatPhoneNumber(item.login),
	participantsList: [item],
	icons: [
	{
	source: UserUtils.getAvatar(item.avatar, item.accountID),
	name: item.login,
	type: CONST.ICON_TYPE_AVATAR,
	},
	],
	keyForList: item.accountID,
	}}
	onSelectRow={() => toggleUser(item.accountID, item.pendingAction)}
	/>

The icons prop is passed to <OptionRow/> which passes the data in icons prop to <MultipleAvatar/> component which finally renders the <UserDetailsTooltip/>

<UserDetaisTooltip/> fetches the display name using the accountID which it expects in its icons prop.

App/src/components/MultipleAvatars.js

Line 83 in c0ece67

displayName: ReportUtils.getDisplayNameForParticipant(props.icons[0].id),

So when we don't pass accountID in the icons prop of <OptionRow/>, missing id field finally lands in <UserDetailsTooltip/> because of which it fails to render the display name.

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

As explained above, icons prop expects an id field which carries accountID for that member, so we need to provide that.
We are using item object to render details of each member which already has the accountID stored in it. We are also using that accountID to fetch the avatar of that member. So along with that, we can pass it to icons prop too to solve our issue.

<OptionRow
    boldStyle
    isDisabled={disabled}
    option={{
        text: props.formatPhoneNumber(item.displayName),
        alternateText: props.formatPhoneNumber(item.login),
        participantsList: [item],
        icons: [
            {   
                id: item.accountID,
                source: UserUtils.getAvatar(item.avatar, item.accountID),
                name: item.login,
                type: CONST.ICON_TYPE_AVATAR,
            },
        ],
        keyForList: item.accountID,
    }}
    onSelectRow={() => toggleUser(item.accountID, item.pendingAction)}
/>

Results

bandicam.2023-07-01.15-17-29-480.mp4

What alternative solutions did you explore? (Optional)

None

[melvin-bot]

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

[melvin-bot]

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[daordonez11]

Proposal

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

The tooltip in the WorkspaceMembersPage is missing displayName

What is the root cause of that problem?

Coming from the PR #21696, where I explain the PR that introduced this issue. I included the fix of searching by ID in MultipleAvatars in my previous PR, but the actual issue is not sending the id, the real issue here is the inconsistency between different pages and components and duplicated code. My original issue was in the search page. Even though sending the id is a workaround, the problem is not reusing the code that already exists for solving this: getIcons in the ReportUtils class, in the future someone could change the code and cause a side effect like this again. Hence, avoiding the anti-pattern of duplicating code is my suggestion. In the future if we want to change how the icon information is loaded we only have to change a single place for easier refactors. The root cause is this commit where getDisplayNameForParticipant was updated to depend on accountID instead of login, that change introduced this issue.

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

Remove the manual creation of the icons in WorkspaceMembersPage

App/src/pages/workspace/WorkspaceMembersPage.js

Lines 323 to 340 in 7e908b1

	<OptionRow
	boldStyle
	isDisabled={disabled}
	option={{
	text: props.formatPhoneNumber(item.displayName),
	alternateText: props.formatPhoneNumber(item.login),
	participantsList: [item],
	icons: [
	{
	source: UserUtils.getAvatar(item.avatar, item.accountID),
	name: item.login,
	type: CONST.ICON_TYPE_AVATAR,
	},
	],
	keyForList: item.accountID,
	}}
	onSelectRow={() => toggleUser(item.accountID, item.pendingAction)}
	/>

And reuse the util code in ReportUtils

icons: ReportUtils.getIcons(undefined, props.personalDetails, UserUtils.getAvatar(item.avatar, item.accountID), false, item.login, item.accountID),

Video of solution working:

New.Recording.-.4_7_2023.19_37_59.webm

What alternative solutions did you explore? (Optional)

Just proposing to avoid code duplication no other alternatives explored.

[chiragxarora]

Update to proposal (can't edit myself)

Improvement:

Didn't know the specific util for getIcons existed after recent refactoring , so we can use that util here too

icons: ReportUtils.getIcons(......)

As a further edit, I suggest,
We are using old method of manually creating the icons prop in some other pages too like ReportParticipantsPage, BaseReactionList, etc.
NOTE: some of these additional ones are also missing the accountID like the one in my bug report, so we can fix that too
We can update all these if we want to utilise this util, otherwise we can keep it and just fix basereactionlist for missing account ID field in icon prop since in many places we are still doing it the old way

EDIT: there's no reports prop for this page and that specific util is meant to be used to get icons from the report as per report type

App/src/libs/ReportUtils.js

Lines 707 to 708 in 85ce1ed

	* Returns the appropriate icons for the given chat report using the stored personalDetails.
	* The Avatar sources can be URLs or Icon components according to the chat type.

We don't have individual member's report and neither we have reports array for this page and we just have policy members account ids and their personalDetails object using which we have collected the details for each member here

App/src/pages/workspace/WorkspaceMembersPage.js

Lines 365 to 378 in 15e547e

	_.each(props.policyMembers, (policyMember, accountID) => {
	if (isDeletedPolicyMember(policyMember)) {
	return;
	}
	const details = props.personalDetails[accountID];
	if (!details) {
	Log.hmmm(`[WorkspaceMembersPage] no personal details found for policy member with accountID: ${accountID}`);
	return;
	}
	data.push({
	...policyMember,
	...details,
	});
	});

This data array is then added to flatlist and we render each item individually after looping over the array. This was implemented this way only because we don't have any util to provide individual report for each member in this page. That is why we can't use this getIcons util here

Ideally we should not use this util because sending undefined report would be a bad code practice and it doesnt serve the purpose.

What we could actually use is ReportUtils.getIconsForParticipants() method which can get us the same icons object but with no dependency on report, by simply using the accountID and personaldetails object like this

icons: ReportUtils.getIconsForParticipants([item.accountID], props.personalDetails[item.accountID]),

There's some JS tweaks required because in getIconsForParticipants method, lodash path is set as

App/src/libs/ReportUtils.js

Line 750 in 60f4817

const avatarSource = UserUtils.getAvatar(lodashGet(personalDetails, [accountID, 'avatar'], ''), accountID);

Basically this method accepts the personal details object of type

{
   accountID1: {
      login: ....,
      avatar: ....,
   },
   accountID2: {
   },
}

So we need to pass the correct object while calling this method like this

 icons: ReportUtils.getIconsForParticipants([item.accountID], {[item.accountID]: props.personalDetails[item.accountID]}),

[daordonez11]

Yeah it is actually an old method. I recommend deep diving into similar components for a more valuable proposal, Also I don't think you can just take another proposal and use it as an alternative solution. I'll just let the C+ decide which proposal is better/simpler.

[chiragxarora]

Yeah it is actually an old method. I recommend deep diving into similar components for a more valuable proposal, Also I don't think you can just take another proposal and use it as an alternative solution. I'll just let the C+ decide which proposal is better/simpler.

You did what is called an "expansion on a solution", even c+ members suggest such improvements at the time of PR

Also, I've found the other pages too and suggested an update for that overall valuable proposal so your second comment is not valid

[daordonez11]

How is it an expansion if it is my own solution from #21696 😂? We even have different RCA. Again, I think it is in the hands of the C+ in charge 😀

[chiragxarora]

How is it an expansion if it is my own solution from #21696 😂? We even have different RCA. Again, I think it is in the hands of the C+ in charge 😀

It's an expansion because you suggested using a util rather than handling it manually in the page itself.

the root cause is not reusing the code that already exists for solving this getIcons in the ReportUtils class.

Also RCA is missing accountID and not the absence of this util so your RCA is plain wrong here, read my RCA I've explained it well. (Note this C+)

And it's not just about using same util everywhere for the consistency, looks like you said deep dive but didn't do it :)

Go see the getIcons method once, for any type of report, its complexity wise heavier, its performing a whole bunch of report type checks before it arrives to the if block of corresponding report type for any type of report. So I'm not sure if we even want to use that for something simple like user icon.

Let the C+ team decide, thankyou

cc @rushatgabhane

[daordonez11]

Also **RCA is missing accountID

That is where you are wrong.

Man, check my PR, you were literally one blame away from seeing that I changed the line of MultipleAvatars getDisplayNameForParticipant to make things work. The root cause as I mentioned in #21696 is the migration that occurred in #21026 here everything changed from login to accountID! That is the real root cause: duplicated code + refactor. Since that change, this issue started happening. That's why BZ List is so important, you didn't even find the original PR that introduced the issue before proposing the solution.

If you had really read my proposal you would see that it enters the first if, because here there is no report. And that I mention the migration which is the real root cause. Here is the specific commit ;) if you want more detail. The moment getDisplayNameForParticipant was updated this issue was introduced.

PS: Updated proposal. Due to the confusion decided to include more details in my root cause analysis.

[chiragxarora]

I changed the line of getDisplayNameForParticipant

I saw that and that's why I've proposed the solution for missing accountID.

but the actual #21099 is not sending the id, that is part of the migration to accountID, the real issue here is the inconsistency between different pages and components. My original issue was in the search page. Even though sending the id is a workaround, the root cause is not reusing the code that already exists for solving this getIcons in the ReportUtils class.

I can't explain more why these statements are technically wrong, let's wait for the review

If you had really read my proposal you would see that it enters the first if, because here there is no report.

Yes you're right about this, but you fail to understand how this change is not making the function generic (what its true purpose is). You are basically passing all the object values in function arguments and returning the formed object on first line.

In the future if we want to change how the icon information is loaded we only have to change a single place.

This line is again very wrong! When there is any change in icon object, can you explain me how your code will handle it in a better way by making the changes on just one place like you mentioned. Say if we add an extra field to icon prop in future or update an existing field, then you will have to pass the same new/updated argument to function call in every place where the icon prop is used and you'll update the icon object in the util too.

Reason this util is generic is we are using the report passed to fetch everything we need, and return from the util (go see the function signature). But you cannot do anything from the report because that is undefined in your case. You will have to pass everything that icon {} needs, explicitly in the arguments and changes will be on both the places.

You are basically forcing this util here when there's no need to do so

I hope you understand how your solution is a bad workaround and not what you are claiming it to be :)

[daordonez11]

You are right! @chiragxarora. Including additional parameters isn't the solution if we want to unify it we should send the personalDetail object and get out the information there or create an specific method like getIconWithPersonalDetail but I'm thinking about the future. If it is a method at least you know that you have to check all places where the method is called, that is the main added value. Without it you would have to manually search all places where information is sent to MultipleAvatars, even though it is a workaround it is easier for future developers that touch the code! Just thinking about future coding and devs. Maybe it can be another util or maybe it can be adding the additional parameter if things are not going to change for that component but since recently there was the update of UserDetailsTooltip I'm afraid it can happen again.

You are right the idea is to force it so it is clear whenever someone touches again this part of the code.

I'm just claiming it is a different solution than yours with different purposes, as you have stated, nothing else. You were right regarding my RCA, it was a mess, that's why I updated it! Thanks for pointing that out.

[dylanexpensify]

@rushatgabhane can we please get reviews?

[chiragxarora]

friendly bump: @rushatgabhane

[rushatgabhane]

Alright so the text and alternateText is used to display the option row. But not the tooltip.
Why is that? Why are we using accountId to get the tooltip? We already are passing the display name as text, and email as alternateText

App/src/pages/workspace/WorkspaceMembersPage.js

Lines 327 to 328 in c0ece67

	text: props.formatPhoneNumber(item.displayName),
	alternateText: props.formatPhoneNumber(item.login),

[daordonez11]

The problem is with MultipleAvatars. For example DisplayNames has an implementation that receives displayNamesWithToolTips via params:

App/src/components/OptionRow.js

Lines 213 to 216 in c0ece67

	<DisplayNames
	accessibilityLabel={this.props.translate('accessibilityHints.chatUserDisplayNames')}
	fullTitle={this.props.option.text}
	displayNamesWithTooltips={displayNamesWithTooltips}

On the other side MultipleAvatars only receives the icons prop, which doesn't even contain the displayName and builds the information manually here:

App/src/components/MultipleAvatars.js

Lines 72 to 87 in c0ece67

	const tooltipTexts = props.shouldShowTooltip ? _.pluck(props.icons, 'name') : [''];
	if (!props.icons.length) {
	return null;
	}
	if (props.icons.length === 1 && !props.shouldStackHorizontally) {
	return (
	<UserDetailsTooltip
	accountID={props.icons[0].id}
	fallbackUserDetails={{
	displayName: ReportUtils.getDisplayNameForParticipant(props.icons[0].id),
	login: lodashGet(props.icons[0], 'name', tooltipTexts[0]),
	avatar: lodashGet(props.icons[0], 'source', ''),
	}}
	>

We could definitely send the displayName via props on the icon but it would also require a lot of changes for consistency in points like: SearchPage state, ReportParticipantsPage, BaseReactionList or we could do something like

displayName: props.icons[0].displayName || ReportUtils.getDisplayNameForParticipant(props.icons[0].id),

For retrocompat Still the main idea is to unify the way icons are built through the different instances of OptionRow ideally with a util method to ensure future changes in the behavior of tooltips. Basically we can create something like getDisplayNamesWithTooltips for icons or even reuse that method to ensure all OptionRow behave the same way

@rushatgabhane

[chiragxarora]

Basically <MultipleAvatar/> (as the name suggests) is built this way to gather all the information from avatar icon
And display name, Alternate text props are for <DisplayName/> component
@rushatgabhane

[grgia]

Copying over from slack - https://expensify.slack.com/archives/C01GTK53T8Q/p1689152366088119?thread_ts=1689077322.531679&cid=C01GTK53T8Q

[melvin-bot]

@amyevans @rushatgabhane @chiragxarora @dylanexpensify 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!

[dylanexpensify]

Reviewing today!

[chiragxarora]

Copying over from slack - https://expensify.slack.com/archives/C01GTK53T8Q/p1689152366088119?thread_ts=1689077322.531679&cid=C01GTK53T8Q

@amyevans the PR that fixed this issue and #22376 is deployed to production now

Quoting thread for context

[melvin-bot]

@amyevans @rushatgabhane @chiragxarora @dylanexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[dylanexpensify]

@rushatgabhane @chiragxarora mind recapping where we're at here?

[chiragxarora]

I reported this current bug and #22376 and submitted the proposals along with them, then review process took a long time and like 10 days later we had this PR #20512 by @grgia which did major refactor to the tooltips. This PR kind of aggravated the above two bugs
initially what I reported: tooltips didn't show display names and showed only avatar and login
after this PR by georgia: tooltips showed only avatar and nothing else

Later, this PR #22500 was raised to solve the regressions caused by the other one but this included some additional fixes which weren't related to the regression caused. These changes solved both of my bugs.

I was assigned this issue late because of proposal review process and by the time I was assigned, the regression fixing PR was already open so I was told to hold mine.

This slack conversation has additional context

cc @dylanexpensify

[rushatgabhane]

@chiragxarora what action is pending? Do we still need to fix this bug?

[chiragxarora]

no, the bugs are fixed as stated above

[dylanexpensify]

@chiragxarora we can pay for the reporting bonus! Have you applied?

[melvin-bot]

@amyevans @rushatgabhane @chiragxarora @dylanexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignee @rushatgabhane is eligible for the Internal assigner, not assigning anyone new.

[dylanexpensify]

bump @chiragxarora

[amyevans]

@dylanexpensify this bug was fixed elsewhere, can you pay the $250 reporting bonus and then we can close this out?

[chiragxarora]

Hello all,
These bugs were fixed by another pr but I proposed the solutions weeks before, also I was assigned the issue too?
And delay was due to proposal review process.

cc @grgia @0xmiroslav

[chiragxarora]

Also, there are 2 bug reports not one😅
This and #22376
Both of these were present before any of the tool tip refactoring happened

[dylanexpensify]

Yes! @chiragxarora will pay out for both

[dylanexpensify]

Done!