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 #41846

Merged
merged 23 commits into from
May 28, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1839a03
simplify avatar logic and use FallbackAvatar when user is unknown
Kicu May 7, 2024
a8879a1
fix not found Page when opening ProfilePage from workspace members li…
Kicu May 8, 2024
00ca93d
Remove more unnecessary usages of .getAvatar() function
Kicu May 8, 2024
7214e1a
Apply some fixes after review
Kicu May 10, 2024
8b3ac5d
Make accountID optional in AvatarWithImagePicker
Kicu May 10, 2024
f886a6b
Add updates after review
Kicu May 13, 2024
1b09f2e
Merge branch 'main' into kicu/38743-fallback-avatar-fix
Kicu May 13, 2024
9091250
Fix typo after merge conflict
Kicu May 13, 2024
4120f36
Fix lint
Kicu May 14, 2024
ee05310
Remove task assignee avatar workaround
Kicu May 14, 2024
42605f5
Merge branch 'main' into kicu/38743-fallback-avatar-fix
Kicu May 14, 2024
a71d08b
Fix bug after merge conflicts
Kicu May 14, 2024
b549d46
Remove instance of unneeded FallbackAvatar
Kicu May 14, 2024
5f355ae
Update tests for `getDisplayNamesWithTooltips`
Kicu May 14, 2024
456d62f
Merge branch 'main' into kicu/38743-fallback-avatar-fix
Kicu May 16, 2024
0b8a92d
Update Avatar code after workspace avatar styling changes
Kicu May 16, 2024
83f1b5f
Merge branch 'main' into kicu/38743-fallback-avatar-fix
Kicu May 16, 2024
ff2dab5
Merge branch 'main' into kicu/38743-fallback-avatar-fix
Kicu May 17, 2024
d9844a6
Fix wrong WorkspaceAvatar colors on workspace profile page
Kicu May 17, 2024
21c9e86
Rename avatarId variable
Kicu May 20, 2024
0319ead
Merge branch 'main' into kicu/38743-fallback-avatar-fix
Kicu May 23, 2024
b87b586
Add minor comment fixes in avatars
Kicu May 23, 2024
0daa461
Merge branch 'main' into kicu/38743-fallback-avatar-fix
Kicu May 24, 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
13 changes: 7 additions & 6 deletions src/components/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,19 @@ function Avatar({
}, [originalSource]);

const isWorkspace = type === CONST.ICON_TYPE_WORKSPACE;
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 source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, Number(avatarID));
// If it's user avatar then accountID will be a number
const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, avatarID as number);
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;

// We pass the color styles down to the SVG for the workspace and fallback avatar.
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;

let iconColors;
if (isWorkspace) {
iconColors = StyleUtils.getDefaultWorkspaceAvatarColor(avatarID?.toString() ?? '');
Expand Down
5 changes: 5 additions & 0 deletions src/components/AvatarWithImagePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ type AvatarWithImagePickerProps = {
/** Avatar source to display */
source?: AvatarSource;

/** Account id of user for which avatar is displayed */
avatarID?: number | string;

/** Additional style props */
style?: StyleProp<ViewStyle>;

Expand Down Expand Up @@ -136,6 +139,7 @@ function AvatarWithImagePicker({
errorRowStyles,
onErrorClose = () => {},
source = '',
avatarID,
fallbackIcon = Expensicons.FallbackAvatar,
size = CONST.AVATAR_SIZE.DEFAULT,
type = CONST.ICON_TYPE_AVATAR,
Expand Down Expand Up @@ -329,6 +333,7 @@ function AvatarWithImagePicker({
containerStyles={avatarStyle}
imageStyles={[avatarStyle, styles.alignSelfCenter]}
source={source}
avatarID={avatarID}
fallbackIcon={fallbackIcon}
size={size}
type={type}
Expand Down
10 changes: 7 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
accountID?: number;
accountID: number | undefined;

This should help catch cases where we are missing to pass accountID.

(do same with Avatar component)

Copy link
Contributor Author

@Kicu Kicu May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will do it for every Avatar.

What do you think about doing the same for source?
Now it looks kinda suspicious that I will have:source?: string but accountID as explicit: number | undefined almost as if accountID is more important than source.

I used this pattern to find all cases for TS fails, but after this I reverted back to optional with ?. In 2 cases I had to explicitly pass undefined and it looked weird IMO. We don't do this pattern anywhere else in code.
There are 2 cases where accountID will be undefined on purpose:


Kicu marked this conversation as resolved.
Show resolved Hide resolved
/** 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,8 +38,9 @@ function AvatarWithIndicator({source, tooltipText = '', fallbackIcon = Expensico
<>
<Avatar
size={CONST.AVATAR_SIZE.SMALL}
source={UserUtils.getSmallSizeAvatar(source)}
source={UserUtils.getSmallSizeAvatar(source, accountID)}
fallbackIcon={fallbackIcon}
avatarID={accountID}
/>
<Indicator />
</>
Expand Down
5 changes: 3 additions & 2 deletions src/components/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ function MenuItem(
<Avatar
imageStyles={[styles.alignSelfCenter]}
size={CONST.AVATAR_SIZE.DEFAULT}
source={icon as AvatarSource}
source={icon}
fallbackIcon={fallbackIcon}
name={title}
avatarID={avatarID}
Expand All @@ -537,7 +537,8 @@ function MenuItem(
{iconType === CONST.ICON_TYPE_AVATAR && (
<Avatar
imageStyles={[styles.alignSelfCenter]}
source={icon as AvatarSource}
source={icon}
avatarID={avatarID}
fallbackIcon={fallbackIcon}
size={avatarSize}
/>
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 @@ -152,7 +152,7 @@ function TaskView({report, ...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)}
iconType={CONST.ICON_TYPE_AVATAR}
avatarSize={CONST.AVATAR_SIZE.SMALLER}
titleStyle={styles.assigneeTextStyle}
Expand Down
5 changes: 2 additions & 3 deletions src/components/UserDetailsTooltip/BaseUserDetailsTooltip/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import useThemeStyles from '@hooks/useThemeStyles';
import {isAnonymousUser} from '@libs/actions/Session';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import * as ReportUtils from '@libs/ReportUtils';
import * as UserUtils from '@libs/UserUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';

Expand Down Expand Up @@ -60,11 +59,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}
avatarID={icon?.id ?? userAccountID}
type={icon?.type ?? CONST.ICON_TYPE_AVATAR}
name={icon?.name ?? userLogin}
fallbackIcon={icon?.fallbackIcon}
avatarID={icon?.id}
/>
</View>
<Text style={[styles.mt2, styles.textMicroBold, styles.textReactionSenders, styles.textAlignCenter]}>{title}</Text>
Expand Down
26 changes: 11 additions & 15 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 @@ -337,13 +338,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 @@ -366,9 +368,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 @@ -397,6 +397,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 @@ -407,7 +408,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 @@ -781,13 +782,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, null);
result.subtitle = subtitle;

return result;
Expand Down Expand Up @@ -1640,7 +1635,8 @@ function getUserToInviteOption({
// If user doesn't exist, use a fallback avatar
userToInvite.icons = [
{
source: UserUtils.getAvatar('', optimisticAccountID),
source: FallbackAvatar,
id: optimisticAccountID,
name: searchValue,
type: CONST.ICON_TYPE_AVATAR,
},
Expand Down Expand Up @@ -2043,7 +2039,7 @@ function getIOUConfirmationOptionsFromPayeePersonalDetail(personalDetail: Person
alternateText: formattedLogin || PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, '', false),
icons: [
{
source: UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID),
source: personalDetail.avatar ?? FallbackAvatar,
name: personalDetail.login ?? '',
type: CONST.ICON_TYPE_AVATAR,
id: 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
Loading
Loading