[HOLD App #22500] DEV : Incorrect tooltip on Invite members page

[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. Go to Settings > Workspaces > select any workspace
2. Go to Members > Invite
3. Hover user avatar in any option row

Expected Result:

Tooltip shows with avatar, email and display name

Actual Result:

Tooltip shows only avatar

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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: Dev
Reproducible in staging?: n/a
Reproducible in production?: n/a
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

bug.4.mov

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

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @alexpensify (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

[kavimuru]

Proposal

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

Incorrect tooltip on Invite members page

What is the root cause of that problem?

#21696 was reverted in recent PR. So when get icons without report, fallbackIcon returns -1 as id

App/src/libs/ReportUtils.js

Lines 807 to 816 in 60f4817

	function getIcons(report, personalDetails, defaultIcon = null, isPayer = false) {
	if (_.isEmpty(report)) {
	const fallbackIcon = {
	source: defaultIcon || Expensicons.FallbackAvatar,
	type: CONST.ICON_TYPE_AVATAR,
	name: '',
	id: -1,
	};
	return [fallbackIcon];
	}

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

Add #21696 back:

function getIcons(report, personalDetails, defaultIcon = null, isPayer = false, defaultName = '', defaultAccountID = -1) {
    if (_.isEmpty(report)) {
        const fallbackIcon = {
            source: defaultIcon || Expensicons.FallbackAvatar,
            type: CONST.ICON_TYPE_AVATAR,
            name: defaultName,
            id: defaultAccountID,
        };
        return [fallbackIcon];
    }

[chiragxarora]

Proposal

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

Incorrect tooltip on Invite members page

What is the root cause of that problem?

Root cause of the issue is the implementation of createOption() method in OptionsListUtils here

App/src/libs/OptionsListUtils.js

Line 526 in 60f4817

result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID), false, personalDetail.login, personalDetail.accountID);

Here we are not checking the existence of report before creating the icons for current option using ReportUtils.getIcons()

App/src/libs/OptionsListUtils.js

Lines 459 to 462 in 60f4817

	if (report) {
	result.isChatRoom = ReportUtils.isChatRoom(report);
	result.isDefaultRoom = ReportUtils.isDefaultRoom(report);
	result.isArchivedRoom = ReportUtils.isArchivedRoom(report);

and this util method is supposed to provide the icons from the report object passed to it, as mentioned here

App/src/libs/ReportUtils.js

Lines 796 to 797 in 60f4817

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

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

We should use the createIcons from util for the case where report exists, and when there is no report we should build the icons using accountID in the else part here

App/src/libs/OptionsListUtils.js

Lines 508 to 509 in 60f4817

	} else {
	reportName = ReportUtils.getDisplayNameForParticipant(accountIDs[0]);

And for that, we have a helper utility function defined in the same OptionsListUtils called getAvatarsForAccountIDs() which returns the icons object using accountIDs

App/src/libs/OptionsListUtils.js

Line 154 in 60f4817

function getAvatarsForAccountIDs(accountIDs, personalDetails) {

Results

bandicam.2023-07-09.14-38-59-556.mp4

What alternative solutions did you explore? (Optional)

We can also use ReportUtils.getIconsForParticipants() which does the same task as OptionsListUtils.getAvatarsForAccountIDs() and additionally it sorts the list according to the firstname too which I think is not needed and will add extra complexity in our case. Also, this one in report util is intended for some other use case

[alexpensify]

Not overdue, I was assigned when I was OOO

[alexpensify]

I'm unable to replicate this experience on prod:

Now, in Staging this is an issue:

[alexpensify]

@amyevans and @rushatgabhane - I think this is similar to #22062 (comment) but wanted to double check before closing. If it's a different root cause, then I'll assign External. Thank you for your feedback!

[situchan]

It's a different root cause. This is a regression from #20512 but #22062 existed before.

[amyevans]

It was added back in #22500 cc @grgia

[situchan]

I confirmed fixed on latest main.
Once it's deployed to staging, we can retest and close this.

[alexpensify]

Thanks, I'm putting this one on Hold.

[situchan]

#22500 was deployed to production.
This is not reproducible anymore. We can close this.
I might be eligible for reporting bonus.

[alexpensify]

I've confirmed that this is not reproducible. There is no bonus because the fix was already in the works before this GH. @amyevans can I get a buddy check if you agree with my summary? Thank you!

[amyevans]

@alexpensify I think paying the reporter bonus makes sense since this bug was reported in Slack about an hour before the revert PR was opened.

[alexpensify]

@amyevans I should have asked my question differently. Did this GH help flag that we needed to revert? Thanks!

[amyevans]

Oh honestly I don't know, I wasn't involved in that discovery. @0xmiroslav how did you uncover the bad merge conflict issue that led to #22500?

[0xmiros]

I discovered while random testing during another PR review.
The first discovery was another one - #21092 (review)
And this bug was discovered 2 days later - d64df72

[alexpensify]

Ok, I've reviewed these points and feel like this GH didn't contribute to uncovering the regression. At this point, we should close but will leave it open until tomorrow if there are any remaining points to flag.