Skip to content

Fix default WS avatar is shown instead of the custom one #49145

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

Merged
rlinoz merged 2 commits into from
Sep 16, 2024
Merged

Fix default WS avatar is shown instead of the custom one #49145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc53f95
fix default ws avatar is shown instead of the custom one
bernhardoj Sep 13, 2024
837e413
fix ws avatar isn't dynamically updated
bernhardoj Sep 13, 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
22 changes: 7 additions & 15 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1866,12 +1866,6 @@ function getDefaultWorkspaceAvatarTestID(workspaceName: string): string {
return !alphaNumeric ? defaultAvatarBuildingIconTestID : `SvgDefaultAvatar_${alphaNumeric[0]} Icon`;
}

function getWorkspaceAvatar(report: OnyxEntry<Report>): AvatarSource {
const workspaceName = getPolicyName(report, false, allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]);
const avatar = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.avatarURL ?? '';
return !isEmpty(avatar) ? avatar : getDefaultWorkspaceAvatar(workspaceName);
}

/**
* Helper method to return the default avatar associated with the given reportID
*/
Expand Down Expand Up @@ -1939,20 +1933,19 @@ function getWorkspaceIcon(report: OnyxInputOrEntry<Report>, policy?: OnyxInputOr
const workspaceName = getPolicyName(report, false, policy);
const cacheKey = report?.policyID ?? workspaceName;
const iconFromCache = workSpaceIconsCache.get(cacheKey);
const avatarURL = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.avatarURL;

const isSameAvatarURL = iconFromCache?.icon?.source === avatarURL;
const isDefaultWorkspaceAvatar = !avatarURL && typeof iconFromCache?.icon?.source !== 'string';
const hasWorkSpaceNameChanged = iconFromCache?.name !== workspaceName;
if (iconFromCache && (isSameAvatarURL || isDefaultWorkspaceAvatar) && !hasWorkSpaceNameChanged) {
return iconFromCache.icon;
}
// disabling to protect against empty strings
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const policyAvatarURL = report?.policyAvatar || allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.avatarURL;
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const policyExpenseChatAvatarSource = policyAvatarURL || getDefaultWorkspaceAvatar(workspaceName);

const isSameAvatarURL = iconFromCache?.icon?.source === policyExpenseChatAvatarSource;
const hasWorkSpaceNameChanged = iconFromCache?.name !== workspaceName;

if (iconFromCache && (isSameAvatarURL || report?.policyAvatar === undefined) && !hasWorkSpaceNameChanged) {
Copy link
Contributor

@Pujan92 Pujan92 Sep 13, 2024

Choose a reason for hiding this comment

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

I doubt why we need condition || report?.policyAvatar === undefined, isn't isSameAvatarURL enough?

Copy link
Contributor Author

@bernhardoj bernhardoj Sep 13, 2024

Choose a reason for hiding this comment

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

Let's say we already have a cache for workspace A custom avatar. If there is a report that has a missing policyAvatar, then policyExpenseChatAvatarSource will contain the default workspace avatar and override the cache.

Copy link
Contributor

@Pujan92 Pujan92 Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If the report policyAvatar is missing then we are looking for the policy avatarURL, if that is missing too then we need to set the defaultAvatar and it should update the cache. isn't it? Why do we need to return old cache value?

Copy link
Contributor Author

@bernhardoj bernhardoj Sep 14, 2024

Choose a reason for hiding this comment

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

If the policyAvatar is an empty string, then yes, we should update the cache because that means the avatar is removed, but if it's missing (undefined), then it's just missing, not that the avatar is removed.

Copy link
Contributor

@Pujan92 Pujan92 Sep 14, 2024

Choose a reason for hiding this comment

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

Ok, I am not sure if that case will exist or not but I Got your point 👍

Copy link
Contributor

@paultsimura paultsimura Dec 19, 2024

Choose a reason for hiding this comment

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

We've ended up using || policyAvatarURL === undefined instead of report?.policyAvatar to handle the offline change of the WS icon: #53618

return iconFromCache.icon;
}

const workspaceIcon: Icon = {
source: policyExpenseChatAvatarSource ?? '',
type: CONST.ICON_TYPE_WORKSPACE,
Expand Down Expand Up @@ -7945,7 +7938,6 @@ export {
getTransactionsWithReceipts,
getUserDetailTooltipText,
getWhisperDisplayNames,
getWorkspaceAvatar,
getWorkspaceChats,
getWorkspaceIcon,
goBackToDetailsPage,
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReportAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function ReportAvatar({report = {} as Report, route, policies, isLoadingApp = tr
const policyID = route.params.policyID ?? '-1';
const policy = policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`];
const policyName = ReportUtils.getPolicyName(report, false, policy);
const avatarURL = ReportUtils.getWorkspaceAvatar(report);
const avatarURL = ReportUtils.getWorkspaceIcon(report).source;

return (
<AttachmentModal
Expand Down
1 change: 1 addition & 0 deletions src/pages/home/report/ReportActionItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,7 @@ export default withOnyx<ReportActionItemProps, ReportActionItemOnyxProps>({
prevProps.shouldHideThreadDividerLine === nextProps.shouldHideThreadDividerLine &&
prevProps.report?.total === nextProps.report?.total &&
prevProps.report?.nonReimbursableTotal === nextProps.report?.nonReimbursableTotal &&
prevProps.report?.policyAvatar === nextProps.report?.policyAvatar &&
prevProps.linkedReportActionID === nextProps.linkedReportActionID &&
lodashIsEqual(prevProps.report.fieldList, nextProps.report.fieldList) &&
lodashIsEqual(prevProps.transactionThreadReport, nextProps.transactionThreadReport) &&
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionItemSingle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function ReportActionItemSingle({
if (isWorkspaceActor) {
displayName = ReportUtils.getPolicyName(report);
actorHint = displayName;
avatarSource = ReportUtils.getWorkspaceAvatar(report);
avatarSource = ReportUtils.getWorkspaceIcon(report).source;
avatarId = report.policyID;
} else if (action?.delegateAccountID && personalDetails[action?.delegateAccountID]) {
// We replace the actor's email, name, and avatar with the Copilot manually for now. And only if we have their
Expand Down
7 changes: 0 additions & 7 deletions tests/perf-test/ReportUtils.perf-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,6 @@ describe('ReportUtils', () => {
await measureFunction(() => ReportUtils.temporary_getMoneyRequestOptions(report, policy, reportParticipants));
});

test('[ReportUtils] getWorkspaceAvatar on 1k policies', async () => {
const report = createRandomReport(1);

await waitForBatchedUpdates();
await measureFunction(() => ReportUtils.getWorkspaceAvatar(report));
});

test('[ReportUtils] getWorkspaceChat on 1k policies', async () => {
const policyID = '1';
const accountsID = Array.from({length: 20}, (v, i) => i + 1);
Expand Down