[Payment Request] [$250] Mention - Avatar is not displaying on Concierge in mention list

[lanitochka17]

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.66-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4516979&group_by=cases:section_id&group_order=asc&group_id=306201&group_by=cases:section_id&group_order=asc&group_id=306201
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Open a chat
2. Press @ and check Concierge

Expected Result:

Avatar should be displayed on Concierge in mention list

Actual Result:

Avatar is not displaying on Concierge in mention list

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6461797_1714061858309.2024-04-25_21-12-29.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015c5e79bf2b62e7b7
• Upwork Job ID: 1783566739983851520
• Last Price Increase: 2024-04-25

Issue Owner

Current Issue Owner: @allroundexperts

[melvin-bot]

Triggered auto assignment to @neil-marcellini (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

@neil-marcellini FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[neil-marcellini]

Ok, taking a look. In general we can add external and the contributors can help us figure out if it's a backend problem.

[melvin-bot]

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

[melvin-bot]

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

[neil-marcellini]

I finally confirmed that the personal detail for concierge has the correct avatar link https://d2k5nsl2zxldvw.cloudfront.net/images/icons/concierge_2022.png. On production it's the same value, so I don't think it's a problem with the data.

[ZhenjaHorbach]

Proposal

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

Mention - Avatar is not displaying on Concierge in mention list

What is the root cause of that problem?

The problem is that when we try to get the default avatar, we use accountID, which in the case of suggestionList , we do not pass

App/src/libs/UserUtils.ts

Lines 85 to 107 in 6060e6f

	function getDefaultAvatar(accountID = -1, avatarURL?: string): IconAsset | undefined {
	if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) {
	return ConciergeAvatar;
	}
	if (Number(accountID) === CONST.ACCOUNT_ID.NOTIFICATIONS) {
	return NotificationsAvatar;
	}
	// There are 24 possible default avatars, so we choose which one this user has based
	// on a simple modulo operation of their login number. Note that Avatar count starts at 1.
	// When creating a chat, we generate an avatar using an ID and the backend response will modify the ID to the actual user ID.
	// But the avatar link still corresponds to the original ID-generated link. So we extract the SVG image number from the backend's link instead of using the user ID directly
	let accountIDHashBucket: AvatarRange;
	if (avatarURL) {
	const match = avatarURL.match(/(default-avatar_|avatar_)(\d+)(?=\.)/);
	const lastDigit = match && parseInt(match[2], 10);
	accountIDHashBucket = lastDigit as AvatarRange;
	} else {
	accountIDHashBucket = ((accountID % CONST.DEFAULT_AVATAR_COUNT) + 1) as AvatarRange;
	}
	return defaultAvatars[`Avatar${accountIDHashBucket}`];
	}

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

To fix this issue we can update this code and add accountID: detail?.accountID,

App/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx

Lines 238 to 250 in 6060e6f

	suggestions.push({
	text: formatLoginPrivateDomain(PersonalDetailsUtils.getDisplayNameOrDefault(detail), detail?.login),
	alternateText: `@${formatLoginPrivateDomain(detail?.login, detail?.login)}`,
	handle: detail?.login,
	icons: [
	{
	name: detail?.login,
	source: detail?.avatar ?? FallbackAvatar,
	type: CONST.ICON_TYPE_AVATAR,
	fallbackIcon: detail?.fallbackIcon,
	id: detail?.accountID,
	},
	],

And also pass accountID for Avatar

App/src/components/MentionSuggestions.tsx

Lines 82 to 89 in 6060e6f

	<Avatar
	source={item.icons[0].source}
	size={isIcon ? CONST.AVATAR_SIZE.MENTION_ICON : CONST.AVATAR_SIZE.SMALLER}
	name={item.icons[0].name}
	type={item.icons[0].type}
	fill={isIcon ? theme.success : undefined}
	fallbackIcon={item.icons[0].fallbackIcon}
	/>

What alternative solutions did you explore? (Optional)

As alternative we can update getUserMentionOptions and pass accountID only in case CONCIERGE, NOTIFICATIONS and so on

[neil-marcellini]

Thanks @ZhenjaHorbach. Can you explain the root cause a bit more please? Why is it that we don't pass the accountID when creating the mention? It looks to me like we do.

[ZhenjaHorbach]

Thanks @ZhenjaHorbach. Can you explain the root cause a bit more please? Why is it that we don't pass the accountID when creating the mention? It looks to me like we do.

In this PR we added accountID for Avatar and added additional checking which use accountID inside getAvatar
But since we didn’t pass this ID for mentionsList
It turns out that this function does not work correctly

https://github.com/Expensify/App/pull/39229/files#diff-244cdd9be92eb59eaa9aa7b7d3e310ab0b3845111e3de28746ec3ea59ae970b4R89-R90

And here we can't get ConciergeAvatar without accountID

App/src/libs/UserUtils.ts

Lines 86 to 88 in 6060e6f

	if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) {
	return ConciergeAvatar;
	}

Actually in this case I think we need to check all places where we use Avatar

[neil-marcellini]

Ok @ZhenjaHorbach I think I finally understand your proposal. I would say the root cause is this.

1. When we build the suggestions the source is set to this

   App/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx

   Line 245 in c611709

   source: detail?.avatar ?? FallbackAvatar,

   
   Previously it was this, and was changed here.
   

   App/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx

   Line 241 in 45cd559

   source: UserUtils.getAvatar(detail?.avatar, detail?.accountID),

2. MentionSuggestions renders the Avatar here and doesn't pass an accountID
   

   App/src/components/MentionSuggestions.tsx

   Lines 82 to 89 in c611709

   	<Avatar
   	source={item.icons[0].source}
   	size={isIcon ? CONST.AVATAR_SIZE.MENTION_ICON : CONST.AVATAR_SIZE.SMALLER}
   	name={item.icons[0].name}
   	type={item.icons[0].type}
   	fill={isIcon ? theme.success : undefined}
   	fallbackIcon={item.icons[0].fallbackIcon}
   	/>

3. The avatar source is set here using UserUtils.getAvatar. The accountID is undefined
   

   App/src/components/Avatar.tsx

   Line 89 in c611709

   const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, accountID);

4. That calls getDefaultAvatar

   App/src/libs/UserUtils.ts

   Line 151 in c611709

   return isDefaultAvatar(avatarSource) ? getDefaultAvatar(accountID, avatarSource) : avatarSource;

5. Since there's not accountID, it incorrectly returns a default avatar
   

   App/src/libs/UserUtils.ts

   Line 106 in c611709

   return defaultAvatars[`Avatar${accountIDHashBucket}`];

I'm wondering how we can test this. I can't reproduce on dev because the avatar is always blank for me. I'll try against the staging api instead of our dev api.

Or we could just revert Use fallback user avatar in cases where the user is unknown to us. @grgia what do you think since you were involved there?

[neil-marcellini]

@ZhenjaHorbach I put up a PR based loosely on your proposal. I hope that's cool with you and I really appreciate you pointing me in the right direction. I would have approved your proposal earlier, but I didn't really understand the root cause until I investigate it thoroughly myself.

[ZhenjaHorbach]

Fix concierge avatar in mention suggestion #41034

Happy to help )

[grgia]

@Kicu could you take a look at this?

[grgia]

@neil-marcellini I doubt we will need to revert, I'm fairly certain this will be an easy fix

[Kicu]

hey I'm the author of #39229 and I took a look at this, few points from me in random order

• this is a great description of root cause: [Payment Request] [$250] Mention - Avatar is not displaying on Concierge in mention list #41014 (comment) - kudos to you @neil-marcellini
• I can easily reproduce on dev - on main you get what we call "FallbackAvatar" image but if I pass accountID then I get concierge avatar
• before my PR there was 20+ unneeded calls to getAvatar() that basically repeated logic, so my intention in PR 39229 was to keep pushing down details.avatar prop and only run getAvatar() once - IMO this simplifies the already complex avatar logic

Because of this I believe the best fix for this is to actually do pass accountID prop correctly. This PR seems to do just that (#41034).

I tested locally on my dev the same fix, and it looks good

before (main)

after passing accountID

In general we only need accountID to be able to display the "pretty" svg versions of: default avatars, ConciergeAvatar and NotificationsAvatar. Otherwise .avatar from user details is always correct.
In addition I can supply a separate PR that adds the new accountID prop in a few places where Avatar is used, because it seems I might miss them.

@grgia

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[allroundexperts]

The PR was closed and the offending PR was reverted. As such, we don't need to wait for the 7 days for the payment.

[melvin-bot]

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

• [CP Staging] Revert "Use fallback user avatar in cases where the user is unknown to us" #41087

If no regressions arise, payment will be issued on 2024-05-06. 🎊

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

• @Kicu does not require payment (Contractor)
• @allroundexperts requires payment through NewDot Manual Requests

[neil-marcellini]

Sounds like this is all done then. Please correct me if I'm wrong.

[allroundexperts]

Can I have a payment summary @neil-marcellini? Thanks!

[JmillsExpensify]

Re-opening for payment summary.

[neil-marcellini]

Assigning a bug zero member to handle payment

[melvin-bot]

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

[strepanier03]

Payment Summary

• $250 - @allroundexperts via Manual Requests in New Expensify

@JmillsExpensify - Payment request incoming.

[JmillsExpensify]

$250 approved for @allroundexperts