Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Avatar changes when from offline to online #32090

Merged
merged 13 commits into from
Dec 15, 2023
56 changes: 42 additions & 14 deletions src/libs/UserUtils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import Str from 'expensify-common/lib/str';
import {SvgProps} from 'react-native-svg';
import {ValueOf} from 'type-fest';
import Onyx, { OnyxEntry } from 'react-native-onyx';
import _ from 'lodash';
import ONYXKEYS from '@src/ONYXKEYS';
import { PersonalDetails } from '@src/types/onyx';
import * as defaultAvatars from '@components/Icon/DefaultAvatars';
import {ConciergeAvatar, FallbackAvatar} from '@components/Icon/Expensicons';
import CONST from '@src/CONST';
Expand All @@ -13,6 +17,12 @@ type AvatarSource = React.FC<SvgProps> | string;

type LoginListIndicator = ValueOf<typeof CONST.BRICK_ROAD_INDICATOR_STATUS> | '';

let allPersonalDetails: OnyxEntry<Record<string, PersonalDetails>>;
Onyx.connect({
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (val) => (allPersonalDetails = _.isEmpty(val) ? {} : val),
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link doesn't work for me for some reason 😅 Can you post some more detail here maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump on this question

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to get the link to load by pasting it into a chat with myself on Slack! 😆

As far as I can tell we aren't memoizing any of the calls to getDefaultAvatarURL so I don't think the issue is particularly relevant here. Many of the calls to getDefaultAvatarURL are also from ReportUtils which also uses this pattern so it wouldn't be a very useful optimization 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Julesssss Friendly bump on the above - I think we should be good to merge!


/**
* Searches through given loginList for any contact method / login with an error.
*
Expand Down Expand Up @@ -68,12 +78,19 @@ function hashText(text: string, range: number): number {
return Math.abs(hashCode(text.toLowerCase())) % range;
}

/**
* Generate a random accountID base on searchValue.
*/
function generateAccountID(searchValue: string): number {
return hashText(searchValue, 2 ** 32);
}

/**
* Helper method to return the default avatar associated with the given accountID
* @param [accountID]
* @returns
*/
function getDefaultAvatar(accountID = -1): React.FC<SvgProps> {
function getDefaultAvatar(accountID = -1, avatarURL?: string): React.FC<SvgProps> {
if (accountID <= 0) {
return FallbackAvatar;
}
Expand All @@ -83,21 +100,39 @@ function getDefaultAvatar(accountID = -1): React.FC<SvgProps> {

// 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;

let accountIDHashBucket: AvatarRange;
if (avatarURL && /images\/avatars\/default-avatar_\d+\./.test(avatarURL)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm convinced this can be tidied up 😉 Let's only use one regex pattern and we probably don't need to test for it first if we're going to match later anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I have removed it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add an explanatory comment here as it won't be super clear why we're extracting the ID from avatarURL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added

const match = avatarURL.match(/(?<=default-avatar_)\d+(?=\.)/);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have one more default avatar prefix avatar_ which is missed here, that caused a regression more details here

const lastDigit = match && parseInt(match[0], 10);
accountIDHashBucket = lastDigit as AvatarRange;
} else {
accountIDHashBucket = ((accountID % CONST.DEFAULT_AVATAR_COUNT) + 1) as AvatarRange;
}
return defaultAvatars[`Avatar${accountIDHashBucket}`];
}

/**
* Helper method to return default avatar URL associated with login
*/
function getDefaultAvatarURL(accountID: string | number = '', isNewDot = false): string {
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]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment also needed here to explain why this code is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added

if (allPersonalDetails[accountID].login) {
email = allPersonalDetails[accountID].login;
} else if (allPersonalDetails[accountID].displayName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how useful this is, since the displayName could have changed and may no longer be an email?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I have already updated my PR. Thank you.

email = allPersonalDetails[accountID].displayName;
}
}
if (email) {
originAccountID = generateAccountID(email);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand right, I think this would clearer if it was named originalOptimisticAccountID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I have updated it to originalOptimisticAccountID.

} 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(accountID) % (isNewDot ? CONST.DEFAULT_AVATAR_COUNT : CONST.OLD_DEFAULT_AVATAR_COUNT)) + 1;
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`;
Expand Down Expand Up @@ -135,7 +170,7 @@ function isDefaultAvatar(avatarURL?: string | React.FC<SvgProps>): boolean {
* @param accountID - the accountID of the user
*/
function getAvatar(avatarURL: AvatarSource, accountID: number): AvatarSource {
return isDefaultAvatar(avatarURL) ? getDefaultAvatar(accountID) : avatarURL;
return isDefaultAvatar(avatarURL) ? getDefaultAvatar(accountID, avatarURL as string) : avatarURL;
}

/**
Expand Down Expand Up @@ -184,13 +219,6 @@ function getSmallSizeAvatar(avatarURL: string, accountID: number): AvatarSource
return `${source.substring(0, lastPeriodIndex)}_128${source.substring(lastPeriodIndex)}`;
}

/**
* Generate a random accountID base on searchValue.
*/
function generateAccountID(searchValue: string): number {
return hashText(searchValue, 2 ** 32);
}

/**
* Gets the secondary phone login number
*/
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/PersonalDetails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ function deleteAvatar() {
}

// We want to use the old dot avatar here as this affects both platforms.
const defaultAvatar = UserUtils.getDefaultAvatarURL(currentUserAccountID);
const defaultAvatar = UserUtils.getDefaultAvatarURL(currentUserAccountID, false);

const optimisticData: OnyxUpdate[] = [
{
Expand Down