[HOLD for payment 2023-12-28] [$500] Chat - Avatar of a new user changes when switching from offline to online

[lanitochka17]

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 https://staging.new.expensify.com/
2. Go offline
3. Click FAB icon and select new chat
4. Enter new email and create new chat
5. type @
6. Verify the suggestion list shows custom avatars for users that have them
7. Now type the email of the new user that the conversation was just created
8. Verify the suggestion list shows the same default avatar for that user as elsewhere (chat header, LHN etc)
9. Send a message @‘ing the new user
10. Verify the mentioned email address is displayed in the chat and highlighted
11. Go online
12. Verify it’s still displayed and highlighted, and that the avatar doesn’t change.

Expected Result:

The avatar of the user should not change

Actual Result:

The avatar of the user changes once the device is online

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.75-0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

2023-09-29.16-02-04.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019f97d031d4b4165c
• Upwork Job ID: 1711886127778775040
• Last Price Increase: 2023-11-25

Issue Owner

Current Issue Owner: @JmillsExpensify

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@JmillsExpensify Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[JmillsExpensify]

Oh huh, good catch. I'm not sure why that's happening, but I agree that it shouldn't.

[melvin-bot]

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

[melvin-bot]

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

[anyongjitiger]

Proposal

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

Avatar of a new user changes to another default avatar when switching from offline to online

What is the root cause of that problem?

When we are offline and create a chat with a new user, we use an optimistic ID as their account ID:

When switching from offline to online, the server will update the optimistic ID to use the correct ID instead. At the same time, it also changed the address of the avatar:
https://d2k5ns\2zxldvw.cloudfront.net/images/avatars/default-avatar_7.png

We're using function getAvatar to calculate the avatar here:

App/src/libs/UserUtils.ts

Lines 137 to 139 in 85f372c

	function getAvatar(avatarURL: string, accountID: number): React.FC<SvgProps> | string {
	return isDefaultAvatar(avatarURL) ? getDefaultAvatar(accountID) : avatarURL;
	}

When the new avatar address is passed into the isDefaultAvatar function, it returns true. As a result, we recalculated the avatar address with the new accountID, and it return a different avatar.

App/src/libs/UserUtils.ts

Lines 110 to 114 in 85f372c

	function isDefaultAvatar(avatarURL?: string): boolean {
	if (typeof avatarURL === 'string') {
	if (avatarURL.includes('images/avatars/avatar_') || avatarURL.includes('images/avatars/default-avatar_') || avatarURL.includes('images/avatars/user/default')) {
	return true;
	}

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

When we create a chat(online or offline), we use the user's input email to create an optimistic ID.
This ID is generated using a hash algorithm, and we use this optimistic ID to index the avatar. 

The backend-generated avatar URL(https://d2k5ns\2zxldvw.cloudfront.net/images/avatars/default-avatar_7.png) follows the same approach.
So we can use the URL address passed from the backend to extract the ID, which corresponds to the original avatar.

function getDefaultAvatar(accountID = -1, avatarURL?: string): React.FC<SvgProps> {
    if (accountID <= 0) {
        return FallbackAvatar;
    }
    if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) {
        return ConciergeAvatar;
    }
    // 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.
    let accountIDHashBucket: AvatarRange;
    if( avatarURL && avatarURL.includes('images/avatars/default-avatar_')){
        const lastDigitIndex = avatarURL.lastIndexOf('_');
        const dotIndex = avatarURL.lastIndexOf('.');
        const lastDigit = parseInt(avatarURL.substring(lastDigitIndex + 1, dotIndex), 10);
        accountIDHashBucket = lastDigit as AvatarRange;
    } else {
        accountIDHashBucket = ((accountID % CONST.DEFAULT_AVATAR_COUNT) + 1) as AvatarRange;
    }
    return defaultAvatars[`Avatar${accountIDHashBucket}`];
}

We also may need to update getDefaultAvatarURL to use the optimistic ID generated by user's input email:

function getDefaultAvatarURL(accountID: string | number = '', isNewDot = true): string {
    if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) {
        return CONST.CONCIERGE_ICON_URL;
    }
    let email; let originAccountID;
    if (allPersonalDetails?.[accountID]) {
        if( allPersonalDetails[accountID].login ){
            email = allPersonalDetails[accountID].login;
        } else if (allPersonalDetails[accountID].displayName) {
            email = allPersonalDetails[accountID].displayName;
        }
    }
    if (email) {
        originAccountID = generateAccountID(email);
    } else {
        originAccountID = accountID;
    }
    // Note that Avatar count starts at 1 which is why 1 has to be added to the result (or else 0 would result in a broken avatar link)
    const accountIDHashBucket = (Number(originAccountID) % (isNewDot ? CONST.DEFAULT_AVATAR_COUNT : CONST.OLD_DEFAULT_AVATAR_COUNT)) + 1;
    const avatarPrefix = isNewDot ? `default-avatar` : `avatar`;
    return `${CONST.CLOUDFRONT_URL}/images/avatars/${avatarPrefix}_${accountIDHashBucket}.png`;
}

What alternative solutions did you explore? (Optional)

[melvin-bot]

@JmillsExpensify @jjcoffee 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!

[melvin-bot]

@JmillsExpensify, @jjcoffee Huh... This is 4 days overdue. Who can take care of this?

[jjcoffee]

@anyongjitiger Thanks for your proposal! If I understand correctly what you're proposing, wouldn't the avatar still change, just on refresh? We calculate the avatar from the hash of the account ID, so I'm not sure if there's much we can do here since we can't know the correct ID in advance!

App/src/libs/UserUtils.ts

Lines 73 to 86 in f942acd

	function getDefaultAvatar(accountID = -1): React.FC<SvgProps> {
	if (accountID <= 0) {
	return FallbackAvatar;
	}
	if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) {
	return ConciergeAvatar;
	}
	// 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.
	const accountIDHashBucket = ((accountID % CONST.DEFAULT_AVATAR_COUNT) + 1) as AvatarRange;
	return defaultAvatars[`Avatar${accountIDHashBucket}`];
	}

[anyongjitiger]

Hello @jjcoffee
After modifying the code according to my plan, isDefaultAvatar will return false. Therefore, getAvatar will not call getDefaultAvatar but instead directly return avatarURL. The image of this avatarURL is the same as the original image.

[jjcoffee]

@anyongjitiger Ah yes I see what you mean! I see that the BE is returning a PNG of the same avatar after we come back online. The problem with using that URL directly is that we'd switch from using the SVG of the avatar to a PNG.

[melvin-bot]

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

[JmillsExpensify]

@jjcoffee Looks like we're still open for proposals?

[anyongjitiger]

@jjcoffee IMO it's imperceptible to the user whether it's a PNG or SVG. And if the user has customized their avatar, what is displayed here is not an SVG image either.

[jjcoffee]

@JmillsExpensify Yup!

@anyongjitiger We're using SVGs for the default avatars on purpose, so I reckon it would be a regression to suddenly be using PNGs.

[anyongjitiger]

@jjcoffee
Yes, I am aware that we currently use SVG to display user’s default avatars, but I couldn't find the purpose of doing so from the existing code. Could you please help me clarify this? Thanks.

Additionally, during my investigation, I came across this piece of code which suggests that we also use PNG to display default avatars in certain cases:


App/src/libs/UserUtils.ts

Lines 88 to 101 in 229cb5d

	/**
	* Helper method to return default avatar URL associated with login
	*/
	function getDefaultAvatarURL(accountID: string | number = '', isNewDot = false): string {
	if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) {
	return CONST.CONCIERGE_ICON_URL;
	}
	// Note that Avatar count starts at 1 which is why 1 has to be added to the result (or else 0 would result in a broken avatar link)
	const accountIDHashBucket = (Number(accountID) % (isNewDot ? CONST.DEFAULT_AVATAR_COUNT : CONST.OLD_DEFAULT_AVATAR_COUNT)) + 1;
	const avatarPrefix = isNewDot ? `default-avatar` : `avatar`;
	return `${CONST.CLOUDFRONT_URL}/images/avatars/${avatarPrefix}_${accountIDHashBucket}.png`;
	}

[anyongjitiger]

@jjcoffee My PR is ready for review. Thanks.

[JmillsExpensify]

Still going through review

[JmillsExpensify]

Same

[melvin-bot]

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

[melvin-bot]

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

• fix: Avatar changes when from offline to online #32090

If no regressions arise, payment will be issued on 2023-12-28. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @anyongjitiger requires payment (Needs manual offer from BZ)
• @jjcoffee requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
• [@jjcoffee] 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:
• [@jjcoffee] 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:
• [@jjcoffee] Determine if we should create a regression test for this bug.
• [@jjcoffee] 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:

[melvin-bot]

@JmillsExpensify, @anyongjitiger, @Julesssss, @jjcoffee Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@JmillsExpensify, @anyongjitiger, @Julesssss, @jjcoffee Eep! 4 days overdue now. Issues have feelings too...

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: Continue personalDetails -> personalDetailsList migration #20328
• 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: https://github.com/Expensify/App/pull/20328/files#r1439315124
• 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: N/A
• Determine if we should create a regression test for this bug. Yes
• 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 Proposal

1. Go offline
2. Tap FAB and Start Chat
3. Enter an email of an account you haven't chatted with before (and that doesn't have a custom avatar) and tap on create chat
4. Note the avatar displayed in the chat
5. Go back online and verify that the avatar does not change.

Do we agree 👍 or 👎

[JmillsExpensify]

Thanks for that, looks good! Payment summary as follows:

• Contributor: @anyongjitiger $500
• Contributor+: @jjcoffee $500

I will send each of you offers now per the comment above noting that manual offers should be sent.

[JmillsExpensify]

Actually, I just sent an offer to @jjcoffee. @anyongjitiger do you mind sharing your Upwork profile so I can send you an offer. Thanks!

[jjcoffee]

@JmillsExpensify Offer accepted, thanks!

[anyongjitiger]

@JmillsExpensify
Upwork Profile Link: https://www.upwork.com/freelancers/~01225df0c309fea73e
Thanks!

[JmillsExpensify]

@jjcoffee All paid out! @anyongjitiger Offer sent! 😄

[JmillsExpensify]

Everyone is paid out. Thank you!