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.

Sign up for GitHub

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

Use fallback user avatar in cases where the user is unknown to us #39229

Merged
merged 9 commits into from
Apr 24, 2024
Merged

Use fallback user avatar in cases where the user is unknown to us #39229

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
441d495
simplify avatar logic and remove unnecessary calls to getAvatar() fn
Kicu Mar 29, 2024
3248bd2
Do not set default avatar URL for optimistic data to have consistent …
Kicu Apr 12, 2024
c8fd46b
Add small fixes to avatars logic
Kicu Apr 15, 2024
6b3528a
Add fixes after review and always generate icons property
Kicu Apr 16, 2024
445a70e
Fix wrong avatar on workspace invite page
Kicu Apr 16, 2024
d42fc2d
Fixes sometimes getting default avatar when source was undefined
Kicu Apr 18, 2024
674af05
add more fixes after review
Kicu Apr 19, 2024
53b3251
Merge branch 'main' into kicu/38743-use-fallback-avatar
Kicu Apr 22, 2024
ad3e041
remove unneeded variable in ProfilePage
Kicu Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/components/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import * as ReportUtils from '@libs/ReportUtils';
import type {AvatarSource} from '@libs/UserUtils';
import * as UserUtils from '@libs/UserUtils';
import type {AvatarSizeName} from '@styles/utils';
import CONST from '@src/CONST';
import type {AvatarType} from '@src/types/onyx/OnyxCommon';
Expand Down Expand Up @@ -49,10 +50,13 @@ type AvatarProps = {

/** Owner of the avatar. If user, displayName. If workspace, policy name */
name?: string;

/** Optional account id if it's user avatar */
accountID?: number;
};

function Avatar({
source,
source: originalSource,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the originalSource?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just the source prop renamed to originalSource. We defined a new variable source variable inside.

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

The new source variable will take the original source and use the svg version of the avatar if possible.

imageStyles,
iconAdditionalStyles,
containerStyles,
Expand All @@ -62,6 +66,7 @@ function Avatar({
fallbackIconTestID = '',
type = CONST.ICON_TYPE_AVATAR,
name = '',
accountID,
}: AvatarProps) {
const theme = useTheme();
const styles = useThemeStyles();
Expand All @@ -72,16 +77,17 @@ function Avatar({

useEffect(() => {
setImageError(false);
}, [source]);
}, [originalSource]);

const isWorkspace = type === CONST.ICON_TYPE_WORKSPACE;
const iconSize = StyleUtils.getAvatarSize(size);

const iconSize = StyleUtils.getAvatarSize(size);
const imageStyle: StyleProp<ImageStyle> = [StyleUtils.getAvatarStyle(size), imageStyles, styles.noBorderRadius];
const iconStyle = imageStyles ? [StyleUtils.getAvatarStyle(size), styles.bgTransparent, imageStyles] : undefined;

// We pass the color styles down to the SVG for the workspace and fallback avatar.
const useFallBackAvatar = imageError || source === Expensicons.FallbackAvatar || !source;
const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, accountID);
const useFallBackAvatar = imageError || !source || source === Expensicons.FallbackAvatar;
const fallbackAvatar = isWorkspace ? ReportUtils.getDefaultWorkspaceAvatar(name) : fallbackIcon || Expensicons.FallbackAvatar;
const fallbackAvatarTestID = isWorkspace ? ReportUtils.getDefaultWorkspaceAvatarTestID(name) : fallbackIconTestID || 'SvgFallbackAvatar Icon';
const avatarSource = useFallBackAvatar ? fallbackAvatar : source;
Expand Down
9 changes: 6 additions & 3 deletions src/components/AvatarWithIndicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import Tooltip from './Tooltip';

type AvatarWithIndicatorProps = {
/** URL for the avatar */
source: UserUtils.AvatarSource;
source?: UserUtils.AvatarSource;

/** account id if it's user avatar */
accountID?: number;

/** To show a tooltip on hover */
tooltipText?: string;
Expand All @@ -23,7 +26,7 @@ type AvatarWithIndicatorProps = {
isLoading?: boolean;
};

function AvatarWithIndicator({source, tooltipText = '', fallbackIcon = Expensicons.FallbackAvatar, isLoading = true}: AvatarWithIndicatorProps) {
function AvatarWithIndicator({source, accountID, tooltipText = '', fallbackIcon = Expensicons.FallbackAvatar, isLoading = true}: AvatarWithIndicatorProps) {
const styles = useThemeStyles();

return (
Expand All @@ -35,7 +38,7 @@ function AvatarWithIndicator({source, tooltipText = '', fallbackIcon = Expensico
<>
<Avatar
size={CONST.AVATAR_SIZE.SMALL}
source={UserUtils.getSmallSizeAvatar(source)}
source={UserUtils.getSmallSizeAvatar(source, accountID)}
fallbackIcon={fallbackIcon}
/>
<Indicator />
Expand Down
2 changes: 2 additions & 0 deletions src/components/MultipleAvatars.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ function MultipleAvatars({
name={icons[0].name}
type={icons[0].type}
fallbackIcon={icons[0].fallbackIcon}
accountID={icons[0].id}
/>
</View>
</UserDetailsTooltip>
Expand Down Expand Up @@ -207,6 +208,7 @@ function MultipleAvatars({
name={icon.name}
type={icon.type}
fallbackIcon={icon.fallbackIcon}
accountID={icon.id}
/>
</View>
</UserDetailsTooltip>
Expand Down
2 changes: 1 addition & 1 deletion src/components/ReportActionItem/TaskView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ function TaskView({report, shouldShowHorizontalRule, ...props}: TaskViewProps) {
<MenuItem
label={translate('task.assignee')}
title={ReportUtils.getDisplayNameForParticipant(report.managerID)}
icon={OptionsListUtils.getAvatarsForAccountIDs(report.managerID ? [report.managerID] : [], personalDetails)}
icon={OptionsListUtils.getAvatarsForAccountIDs([report.managerID], personalDetails)}
Kicu marked this conversation as resolved.
Show resolved Hide resolved
iconType={CONST.ICON_TYPE_AVATAR}
avatarSize={CONST.AVATAR_SIZE.SMALLER}
titleStyle={styles.assigneeTextStyle}
Expand Down
4 changes: 2 additions & 2 deletions src/components/UserDetailsTooltip/BaseUserDetailsTooltip/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import * as ReportUtils from '@libs/ReportUtils';
import * as UserUtils from '@libs/UserUtils';
import CONST from '@src/CONST';

function BaseUserDetailsTooltip({accountID, fallbackUserDetails, icon, delegateAccountID, shiftHorizontal, children}: UserDetailsTooltipProps) {
Expand Down Expand Up @@ -55,10 +54,11 @@ function BaseUserDetailsTooltip({accountID, fallbackUserDetails, icon, delegateA
<View style={styles.emptyAvatar}>
<Avatar
containerStyles={[styles.actionAvatar]}
source={icon?.source ?? UserUtils.getAvatar(userAvatar, userAccountID)}
source={icon?.source ?? userAvatar}
type={icon?.type ?? CONST.ICON_TYPE_AVATAR}
name={icon?.name ?? userLogin}
fallbackIcon={icon?.fallbackIcon}
accountID={userAccountID}
/>
</View>
<Text style={[styles.mt2, styles.textMicroBold, styles.textReactionSenders, styles.textAlignCenter]}>{title}</Text>
Expand Down
37 changes: 13 additions & 24 deletions src/libs/OptionsListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import lodashSet from 'lodash/set';
import lodashSortBy from 'lodash/sortBy';
import Onyx from 'react-native-onyx';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import {FallbackAvatar} from '@components/Icon/Expensicons';
import type {SelectedTagOption} from '@components/TagPicker';
import CONST from '@src/CONST';
import type {TranslationPaths} from '@src/languages/types';
Expand Down Expand Up @@ -303,13 +304,14 @@ function getAvatarsForAccountIDs(accountIDs: number[], personalDetails: OnyxEntr
Object.entries(defaultValues).forEach((item) => {
reversedDefaultValues[item[1]] = item[0];
});

return accountIDs.map((accountID) => {
const login = reversedDefaultValues[accountID] ?? '';
const userPersonalDetail = personalDetails?.[accountID] ?? {login, accountID, avatar: ''};
const userPersonalDetail = personalDetails?.[accountID] ?? {login, accountID};

return {
id: accountID,
source: UserUtils.getAvatar(userPersonalDetail.avatar, userPersonalDetail.accountID),
source: userPersonalDetail.avatar ?? FallbackAvatar,
type: CONST.ICON_TYPE_AVATAR,
name: userPersonalDetail.login ?? '',
};
Expand All @@ -332,9 +334,7 @@ function getPersonalDetailsForAccountIDs(accountIDs: number[] | undefined, perso
}
let personalDetail: OnyxEntry<PersonalDetails> = personalDetails[accountID];
if (!personalDetail) {
personalDetail = {
avatar: UserUtils.getDefaultAvatar(cleanAccountID),
} as PersonalDetails;
personalDetail = {} as PersonalDetails;
}

if (cleanAccountID === CONST.ACCOUNT_ID.CONCIERGE) {
Expand Down Expand Up @@ -363,6 +363,7 @@ function getParticipantsOption(participant: ReportUtils.OptionData | Participant
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const login = detail?.login || participant.login || '';
const displayName = PersonalDetailsUtils.getDisplayNameOrDefault(detail, LocalePhoneNumber.formatPhoneNumber(login));

return {
keyForList: String(detail?.accountID),
login,
Expand All @@ -373,7 +374,7 @@ function getParticipantsOption(participant: ReportUtils.OptionData | Participant
alternateText: LocalePhoneNumber.formatPhoneNumber(login) || displayName,
icons: [
{
source: UserUtils.getAvatar(detail?.avatar ?? '', detail?.accountID ?? -1),
source: detail?.avatar ?? FallbackAvatar,
name: login,
type: CONST.ICON_TYPE_AVATAR,
id: detail?.accountID,
Expand Down Expand Up @@ -757,13 +758,7 @@ function createOption(
// Disabling this line for safeness as nullish coalescing works only if the value is undefined or null
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
result.searchText = getSearchText(report, reportName, personalDetailList, !!result.isChatRoom || !!result.isPolicyExpenseChat, !!result.isThread);
result.icons = ReportUtils.getIcons(
report,
personalDetails,
UserUtils.getAvatar(personalDetail?.avatar ?? '', personalDetail?.accountID),
personalDetail?.login,
personalDetail?.accountID,
);
result.icons = ReportUtils.getIcons(report, personalDetails, personalDetail?.avatar, personalDetail?.login, personalDetail?.accountID);
result.subtitle = subtitle;

return result;
Expand Down Expand Up @@ -1851,7 +1846,6 @@ function getOptions(
[optimisticAccountID]: {
accountID: optimisticAccountID,
login: searchValue,
avatar: UserUtils.getDefaultAvatar(optimisticAccountID),
},
};
userToInvite = createOption([optimisticAccountID], personalDetailsExtended, null, reportActions, {
Expand All @@ -1864,10 +1858,10 @@ function getOptions(
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
userToInvite.alternateText = userToInvite.alternateText || searchValue;

// If user doesn't exist, use a default avatar
// If user doesn't exist, use a fallback avatar
userToInvite.icons = [
{
source: UserUtils.getAvatar('', optimisticAccountID),
source: FallbackAvatar,
name: searchValue,
type: CONST.ICON_TYPE_AVATAR,
},
Expand Down Expand Up @@ -1941,17 +1935,12 @@ function getShareLogOptions(options: OptionList, searchValue = '', betas: Beta[]
*/
function getIOUConfirmationOptionsFromPayeePersonalDetail(personalDetail: PersonalDetails | EmptyObject, amountText?: string): PayeePersonalDetails {
const formattedLogin = LocalePhoneNumber.formatPhoneNumber(personalDetail.login ?? '');
const icons = [{source: personalDetail.avatar ?? FallbackAvatar, name: personalDetail.login ?? '', type: CONST.ICON_TYPE_AVATAR, id: personalDetail.accountID}];

return {
text: PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, formattedLogin),
alternateText: formattedLogin || PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, '', false),
icons: [
{
source: UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID),
name: personalDetail.login ?? '',
type: CONST.ICON_TYPE_AVATAR,
id: personalDetail.accountID,
},
],
icons,
descriptiveText: amountText ?? '',
login: personalDetail.login ?? '',
accountID: personalDetail.accountID,
Expand Down
1 change: 0 additions & 1 deletion src/libs/PersonalDetailsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ function getPersonalDetailsOnyxDataForOptimisticUsers(newLogins: string[], newAc
personalDetailsNew[accountID] = {
login,
accountID,
avatar: UserUtils.getDefaultAvatarURL(accountID),
displayName: LocalePhoneNumber.formatPhoneNumber(login),
};

Expand Down
34 changes: 17 additions & 17 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type {OnyxCollection, OnyxEntry, OnyxUpdate} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import type {FileObject} from '@components/AttachmentModal';
import * as Expensicons from '@components/Icon/Expensicons';
import {FallbackAvatar} from '@components/Icon/Expensicons';
import * as defaultGroupAvatars from '@components/Icon/GroupDefaultAvatars';
import * as defaultWorkspaceAvatars from '@components/Icon/WorkspaceDefaultAvatars';
import CONST from '@src/CONST';
Expand Down Expand Up @@ -1629,7 +1629,7 @@ function getIconsForParticipants(participants: number[], personalDetails: OnyxCo
const participantsList = participants || [];

for (const accountID of participantsList) {
const avatarSource = UserUtils.getAvatar(personalDetails?.[accountID]?.avatar ?? '', accountID);
const avatarSource = personalDetails?.[accountID]?.avatar ?? FallbackAvatar;
const displayNameLogin = personalDetails?.[accountID]?.displayName ? personalDetails?.[accountID]?.displayName : personalDetails?.[accountID]?.login;
participantDetails.push([accountID, displayNameLogin ?? '', avatarSource, personalDetails?.[accountID]?.fallbackIcon ?? '']);
}
Expand Down Expand Up @@ -1690,12 +1690,12 @@ function getPersonalDetailsForAccountID(accountID: number): Partial<PersonalDeta
if (!accountID) {
return {};
}
return (
allPersonalDetails?.[accountID] ?? {
avatar: UserUtils.getDefaultAvatar(accountID),
isOptimisticPersonalDetail: true,
}
);

const defaultDetails = {
isOptimisticPersonalDetail: true,
};

return allPersonalDetails?.[accountID] ?? defaultDetails;
}

/**
Expand Down Expand Up @@ -1817,7 +1817,7 @@ function getIcons(
): Icon[] {
if (isEmptyObject(report)) {
const fallbackIcon: Icon = {
source: defaultIcon ?? Expensicons.FallbackAvatar,
source: defaultIcon ?? FallbackAvatar,
type: CONST.ICON_TYPE_AVATAR,
name: defaultName,
id: defaultAccountID,
Expand All @@ -1828,7 +1828,7 @@ function getIcons(
const parentReportAction = ReportActionsUtils.getParentReportAction(report);
const workspaceIcon = getWorkspaceIcon(report, policy);
const memberIcon = {
source: UserUtils.getAvatar(personalDetails?.[parentReportAction.actorAccountID ?? -1]?.avatar ?? '', parentReportAction.actorAccountID ?? -1),
source: personalDetails?.[parentReportAction.actorAccountID ?? -1]?.avatar ?? FallbackAvatar,
id: parentReportAction.actorAccountID,
type: CONST.ICON_TYPE_AVATAR,
name: personalDetails?.[parentReportAction.actorAccountID ?? -1]?.displayName ?? '',
Expand All @@ -1844,7 +1844,7 @@ function getIcons(
const actorDisplayName = PersonalDetailsUtils.getDisplayNameOrDefault(allPersonalDetails?.[actorAccountID ?? -1], '', false);
const actorIcon = {
id: actorAccountID,
source: UserUtils.getAvatar(personalDetails?.[actorAccountID ?? -1]?.avatar ?? '', actorAccountID ?? -1),
source: personalDetails?.[actorAccountID ?? -1]?.avatar ?? FallbackAvatar,
name: actorDisplayName,
type: CONST.ICON_TYPE_AVATAR,
fallbackIcon: personalDetails?.[parentReportAction.actorAccountID ?? -1]?.fallbackIcon,
Expand All @@ -1859,7 +1859,7 @@ function getIcons(
if (isTaskReport(report)) {
const ownerIcon = {
id: report?.ownerAccountID,
source: UserUtils.getAvatar(personalDetails?.[report?.ownerAccountID ?? -1]?.avatar ?? '', report?.ownerAccountID ?? -1),
source: personalDetails?.[report?.ownerAccountID ?? -1]?.avatar ?? FallbackAvatar,
type: CONST.ICON_TYPE_AVATAR,
name: personalDetails?.[report?.ownerAccountID ?? -1]?.displayName ?? '',
fallbackIcon: personalDetails?.[report?.ownerAccountID ?? -1]?.fallbackIcon,
Expand Down Expand Up @@ -1891,7 +1891,7 @@ function getIcons(
if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
const workspaceIcon = getWorkspaceIcon(report, policy);
const memberIcon = {
source: UserUtils.getAvatar(personalDetails?.[report?.ownerAccountID ?? -1]?.avatar ?? '', report?.ownerAccountID ?? -1),
source: personalDetails?.[report?.ownerAccountID ?? -1]?.avatar ?? FallbackAvatar,
id: report?.ownerAccountID,
type: CONST.ICON_TYPE_AVATAR,
name: personalDetails?.[report?.ownerAccountID ?? -1]?.displayName ?? '',
Expand All @@ -1901,15 +1901,15 @@ function getIcons(
}
if (isIOUReport(report)) {
const managerIcon = {
source: UserUtils.getAvatar(personalDetails?.[report?.managerID ?? -1]?.avatar ?? '', report?.managerID ?? -1),
source: personalDetails?.[report?.managerID ?? -1]?.avatar ?? FallbackAvatar,
id: report?.managerID,
type: CONST.ICON_TYPE_AVATAR,
name: personalDetails?.[report?.managerID ?? -1]?.displayName ?? '',
fallbackIcon: personalDetails?.[report?.managerID ?? -1]?.fallbackIcon,
};
const ownerIcon = {
id: report?.ownerAccountID,
source: UserUtils.getAvatar(personalDetails?.[report?.ownerAccountID ?? -1]?.avatar ?? '', report?.ownerAccountID ?? -1),
source: personalDetails?.[report?.ownerAccountID ?? -1]?.avatar ?? FallbackAvatar,
type: CONST.ICON_TYPE_AVATAR,
name: personalDetails?.[report?.ownerAccountID ?? -1]?.displayName ?? '',
fallbackIcon: personalDetails?.[report?.ownerAccountID ?? -1]?.fallbackIcon,
Expand Down Expand Up @@ -1955,7 +1955,7 @@ function getDisplayNamesWithTooltips(
const accountID = Number(user?.accountID);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const displayName = getDisplayNameForParticipant(accountID, isMultipleParticipantReport, shouldFallbackToHidden, shouldAddCurrentUserPostfix) || user?.login || '';
const avatar = UserUtils.getDefaultAvatar(accountID);
const avatar = user && 'avatar' in user ? user.avatar : FallbackAvatar;

let pronouns = user?.pronouns ?? undefined;
if (pronouns?.startsWith(CONST.PRONOUNS.PREFIX)) {
Expand Down Expand Up @@ -3191,7 +3191,7 @@ function buildOptimisticAddCommentReportAction(text?: string, file?: FileObject,
},
],
automatic: false,
avatar: allPersonalDetails?.[accountID ?? -1]?.avatar ?? UserUtils.getDefaultAvatarURL(accountID),
avatar: allPersonalDetails?.[accountID ?? -1]?.avatar,
created: DateUtils.getDBTimeWithSkew(),
message: [
{
Expand Down
3 changes: 1 addition & 2 deletions src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import * as OptionsListUtils from './OptionsListUtils';
import * as ReportActionsUtils from './ReportActionsUtils';
import * as ReportUtils from './ReportUtils';
import * as TaskUtils from './TaskUtils';
import * as UserUtils from './UserUtils';

const reportActionsByReport: OnyxCollection<ReportActions> = {};
const visibleReportActionItems: ReportActions = {};
Expand Down Expand Up @@ -414,7 +413,7 @@ function getOptionData({
result.subtitle = subtitle;
result.participantsList = participantPersonalDetailList;

result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail?.avatar ?? {}, personalDetail?.accountID), '', -1, policy);
result.icons = ReportUtils.getIcons(report, personalDetails, personalDetail?.avatar, '', -1, policy);
result.searchText = OptionsListUtils.getSearchText(report, reportName, participantPersonalDetailList, result.isChatRoom || result.isPolicyExpenseChat, result.isThread);
result.displayNamesWithTooltips = displayNamesWithTooltips;

Expand Down
Loading
Loading