Use fallback user avatar in cases where the user is unknown to us#41846
Conversation
src/components/Avatar.tsx
Outdated
| // We pass the color styles down to the SVG for the workspace and fallback avatar. | ||
| const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, Number(accountID)); | ||
| // if it's user avatar then accountID will be a number | ||
| const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, accountID as number); |
There was a problem hiding this comment.
@s77rt any better ideas for "accountID as number"?
the logic here is
- if
isWorkspaceis true -> theaccountIDwill be apolicyIDwhich is astring - if
isWorkspacefalse -> accountID is standard user account id which is anumber
I dislike casting via as, but not sure how to improve this line
There was a problem hiding this comment.
We can split the types into two and use union
type AvatarProps = {
...
} & (
| {
type: typeof CONST.ICON_TYPE_AVATAR;
accountID?: number;
}
| {
type: typeof CONST.ICON_TYPE_WORKSPACE;
accountID?: string;
}
);However for this to work we need to make the type prop required and remove its default value.
There was a problem hiding this comment.
The changes here seem unrelated. Can you please double check
There was a problem hiding this comment.
You're asking about ProfilePage? So the changes are deliberate, and related to fixing #40989.
Basically right now (on main) the decision to show most of the content is based on this line:
const hasMinimumDetails = !isEmptyObject(details.avatar);(https://github.com/Expensify/App/blob/main/src/pages/ProfilePage.tsx#L154)
But we know that there will be no avatar in case we're offline and rely on optimistic data - as I removed the default avatar from optimistic data almost everywhere.
All the other basic data is there, so I just allow everything to display and the Fallback avatar will be correctly shown.
I also disable clicking on the avatar when it's the fallback displayed
All the other changes are just indentation moved 1 level up. I tested this both in offline and online and it behaves correct I believe.
src/pages/home/report/ReportActionCompose/SuggestionMention.tsx
Outdated
Show resolved
Hide resolved
| source?: UserUtils.AvatarSource; | ||
|
|
||
| /** account id if it's user avatar */ | ||
| accountID?: number; |
There was a problem hiding this comment.
| accountID?: number; | |
| accountID: number | undefined; |
This should help catch cases where we are missing to pass accountID.
(do same with Avatar component)
There was a problem hiding this comment.
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:
- https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspaceProfilePage.tsx#L141 - this is workspace avatar, no accountID
- https://github.com/Expensify/App/blob/main/src/pages/NewChatConfirmPage.tsx#L115 - this is for a group chat, it will never be 1 user's avatar
|
@s77rt thanks for all the comments, some notes from me:
|
| userToInvite.icons = [ | ||
| { | ||
| source: UserUtils.getAvatar('', optimisticAccountID), | ||
| source: FallbackAvatar, | ||
| name: searchValue, | ||
| type: CONST.ICON_TYPE_AVATAR, | ||
| }, |
src/libs/SidebarUtils.ts
Outdated
| 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); |
There was a problem hiding this comment.
| result.icons = ReportUtils.getIcons(report, personalDetails, personalDetail?.avatar, '', -1, policy); | |
| result.icons = ReportUtils.getIcons(report, personalDetails, personalDetail?.avatar, personalDetail?.login, personalDetail?.accountID, policy); |
The avatar relays on the account id, if we pass -1 we will end up using the fallback avatar instead of the real avatar (if the used avatar is the default colorful one)
|
|
||
| secondaryAvatar = { | ||
| source: UserUtils.getAvatar(secondaryUserAvatar, secondaryAccountId), | ||
| source: secondaryUserAvatar, |
There was a problem hiding this comment.
NAB. secondaryUserAvatar is using an empty string as the fallback, let's use the fallback avatar instead
src/components/Avatar.tsx
Outdated
| // We pass the color styles down to the SVG for the workspace and fallback avatar. | ||
| const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, Number(accountID)); | ||
| // if it's user avatar then accountID will be a number | ||
| const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, accountID as number); |
There was a problem hiding this comment.
We can split the types into two and use union
type AvatarProps = {
...
} & (
| {
type: typeof CONST.ICON_TYPE_AVATAR;
accountID?: number;
}
| {
type: typeof CONST.ICON_TYPE_WORKSPACE;
accountID?: string;
}
);However for this to work we need to make the type prop required and remove its default value.
|
Bug: In the task assignee the assign to me row has the fallback avatar 
|
|
All changes pushes @s77rt For the bug that you found there is something wrong in the whole chain of calls :
|
|
Can you please test if that bug is still reproducible? (without the workaround) I'm getting a different icon now which is also wrong but same as staging 
|
|
So without workaround it reproduces for me every time on web. I have a feeling that whether you get this bug is related to state of However I tried running it on ios and there I had the correct avatar :/ |
|
In main / staging can you reproduce the bug? If not then it's a regression and we should fix it without workarounds |
|
I'm sorry I misunderstood. We are talking about Task > "Assign to me" bug correct?
I strongly believe the bug is related to current state of one's local DB and what That being said I would still keep the workaround as it uses |
|
Can you please resolve the conflicts and remove the workaround? I cannot reproduce this constantly but feels like we should handle it separately |
|
@shawnborton great find. I checked that the same bug is currently appearing on web so its not introduced by my code, but rather another bug after this: #39637 However if thats okey with you guys I could try to fix this within this PR as hopefully that will be faster. |
|
@shawnborton @grgia rec-bug.mov |
|
Amazing, thanks! |
| type: isWorkspaceActor ? CONST.ICON_TYPE_WORKSPACE : CONST.ICON_TYPE_AVATAR, | ||
| name: primaryDisplayName ?? '', | ||
| id: avatarAccountId, | ||
| id: isWorkspaceActor ? report.policyID : avatarAccountId, |
There was a problem hiding this comment.
NAB. avatarAccountId already takes into account the isWorkspaceActor condition. Change the variable name to avatarId to avoid confusion
|
@mountiny Done and done |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
6 participants
Details
<Avatar>component, almost all calls togetAvatarandgetDefaultAvatarwere removed as they were mostly concerned with showing a correct SVG avatar version - now<Avatar>does all thisavatarprop frompersonalDetails. It is returned from backend, and any user "known" to us will have it setFallbackAvatarwhen:accountIDaccountIDwas optimistically set in TS codeFallbackAvataras source. Otherwise nothing would be shown because of patterns like this one: https://github.com/Expensify/App/blob/main/src/components/OptionRow.tsx#L209Extra notes for reviewer:
accountIDfor both user account and policy id.Because of this we were already passing accountID to Avatar component in some places
CC @s77rt
Fixed Issues
$ #38743
$ #40989
$ #40996
PROPOSAL:
Tests
verify that small avatars in mentions from chat are correct including Concierge avatar
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
rec-android-fallback.mp4
Android: mWeb Chrome
iOS: Native
recsm-ios-fallback-demo.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
rec-web-fallback-demo.mp4
rec-web-fallback-page-err.mp4
rec-web-fallback-quickaction.mp4
MacOS: Desktop