[HOLD for payment 2024-02-20] [$500] Android - Room-Invited room avatar looks different

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.32
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Tap on a room chat
3. Tap header--- members-- invite a user X
4. Launch app
5. Login as user X in Android app
6. Tap on invited room chat
7. Tap on header -- avatar

Expected Result:

Invited room avatar must be shown correctly

Actual Result:

Invited room avatar looks different ( like placeholder)

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6355985_1706287071557.full.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01220f7073da90ab3b
• Upwork Job ID: 1750958302280990720
• Last Price Increase: 2024-01-26
• Automatic offers:
• tienifr | Contributor | 28133955

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01220f7073da90ab3b

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Invited room avatar looks different ( like placeholder)

What is the root cause of that problem?

In here, we're not using the ReportUtils.getPolicyName method to get the policy name.

When user is a member of the room but is not a member of the workspace, they will not have access to the policy data. So the policy?.name used here will be empty.

In this case, the right thing to do is to fallback to the report's report?.policyName, which is what ReportUtils.getPolicyName already handled for us (along with a few other cases).

So it couldn't find the policyName and will fallback to the default avatar (instead of the avatar with workspace initial as it should be).

What changes do you think we should make in order to solve the problem?

In here, use the ReportUtils.getPolicyName method to get the policy name.

const policyName = ReportUtils.getPolicyName(report, false, policy);

(Can consider passing returnEmptyIfNotFound param as true, but false is generally used in such icon-getting logic)

Optionally, in here, we should use the getWorkspaceAvatar since it's a proven method for retrieving the workspace avatar. To be safe, we can pass the policy as a second param into getWorkspaceAvatar and use it instead of getting from allPolicies, if the param is passed in (just like what we did with getPolicyName)

What alternative solutions did you explore? (Optional)

Unrelated, but there's a report.policyAvatar field that was recently added from the back-end to fix another issue, and the front-end changes for that is already being handled via this PR. It will fix some use cases related to the policy avatar but not directly related to this issue. If any additional change related to the report.policyAvatar (like same change for getWorkspaceAvatar), it should be handled in that PR as well.

[Tony-MK]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Android - Room-Invited room avatar looks different

What is the root cause of that problem?

No policy record associated with the room is stored in the onyx because the user was not invited to the policy but to the policy room.

App/src/libs/ReportUtils.ts

Lines 1443 to 1446 in c5ca9a8

	const policyExpenseChatAvatarSource = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.avatar
	? allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.avatar
	: getDefaultWorkspaceAvatar(workspaceName);

Hence, in the getWorkspaceIcon function, the value of policyExpenseChatAvatarSource will be the default icon of the room.

However, The correct workspace avatar source is stored in the policyAvatar attribute is available in the report, but we are not utilizing it in the getWorkspaceIcon.

What changes do you think we should make in order to solve the problem?

We should also check if the report?.policyAvatar is present after the policy fails to be found in the Onyx collection and before it calls getDefaultWorkspaceAvatar.

Therefore, let's change the code snippet mentioned above to modify the getWorkspaceIcon function like below.

const policyExpenseChatAvatarSource = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.avatar
  ? allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]?.avatar : report?.policyAvatar;

const workspaceIcon: Icon = {
    source: policyExpenseChatAvatarSource ?? getDefaultWorkspaceAvatar(workspaceName) ?? '',
    ....

Extras

1. Change the getWorkspaceAvatar function as well for consistency.
2. Add policyAvatar to the Report type due to typescript errors.

[tienifr]

@Tony-MK Just FYI the report.policyAvatar was recently added from the back-end to fix another issue, and the front-end changes you suggested is already being handled via this PR, if any additional change related to the report.policyAvatar, it should be handled in that PR as well.

Proposal updated to mention this additional information.

[Christinadobrzyn]

reviewing proposals

[allroundexperts]

Thanks for the proposal @tienifr. As far as I've understood, you're just getting the name of the policy but if the policy does not exist, the default avatar would still be shown.

I think what @Tony-MK has proposed is much more simpler and a better solution. I would go with that.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[tienifr]

Thanks for the proposal @tienifr. As far as I've understood, you're just getting the name of the policy but if the policy does not exist, the default avatar would still be shown.

@allroundexperts No, if the policy does not exist, it will use the policyName from the report (please see the full fallback logic here), there won't be a case where it cannot find the policy name, the getPolicyName was designed for that.

I think what @Tony-MK has #35262 (comment) is much more simpler and a better solution. I would go with that.

@allroundexperts I've tried that solution and it doesn't work, I don't think it's actually related to this issue, because in ReportAvatar we currently don't use getWorkspaceIcon (or getWorkspaceAvatar). Could you give it a try as well (if not already)?

[Christinadobrzyn]

Reviewing proposals

[allroundexperts]

Will provide an update tomorrow!

[Tony-MK]

@allroundexperts I've tried that solution and it doesn't work, I don't think it's actually related to this issue, because in ReportAvatar we currently don't use getWorkspaceIcon (or getWorkspaceAvatar). Could you give it a try as well (if not already)?

Hey, @allroundexperts and @tienifr, my proposed solution works suitably and is viable.

Firstly, you try it out for yourself on this test branch.

Furthermore, the video below compares the dev and staging to demonstrate that the changes work and provide consistency throughout the app.

POC.mov

Finally, we use the getWorkspaceIcon, from the getIcons function to render the workspace avatars, and used by many other components.

App/src/libs/ReportUtils.ts

Lines 1517 to 1519 in 953b313

	if (isAdminRoom(report) || isAnnounceRoom(report) || isChatRoom(report) || isArchivedRoom(report)) {
	const workspaceIcon = getWorkspaceIcon(report, policy);
	return [workspaceIcon];

[tienifr]

@Tony-MK I think you've mistaken this one with another issue. The issue you're trying to fix is the same as this issue (which already has a PR raised to fix it)

This GH issue is when the workspace has no avatar uploaded, thus the workspace name initial (A as in the OP video) is shown, also it happens only in the ReportAvatar, as you can see in the OP video, in other pages it still shows A initial correctly

cc @allroundexperts

[allroundexperts]

Okay. I got the incorrect impression here as well that the issue was for custom avatars of a workspace. @tienifr correctly pointed out that the above is being handled in #33470. This issue is about the default avatar not getting displayed correctly because policy name is missing (since the user was invited to the room, not the policy). As such, I think @tienifr's proposal would work well in this case. Thanks for the explanation @tienifr!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @srikarparsi is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[Tony-MK]

Thanks @tienifr .

Seems I did not figure out the expected result properly.

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[Christinadobrzyn]

Awesome! Looks like we're working on a PR!

[Christinadobrzyn]

PR is here - #35630

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.40-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Room-Invited room avatar looks different #35630

If no regressions arise, payment will be issued on 2024-02-20. 🎊

For reference, here are some details about the assignees on this issue:

• @allroundexperts requires payment through NewDot Manual Requests
• @tienifr requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@Christinadobrzyn] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/373547

[Christinadobrzyn]

hi! Sorry I'm ooo sick today - I'll follow up on this tomorrow

[Christinadobrzyn]

Payouts due:

Contributor: $500 @tienifr (paid in Upwork - https://www.upwork.com/nx/wm/workroom/36224169/overview)
Contributor+: $500 @allroundexperts (payment through NewDot)

Eligible for 50% #urgency bonus? NA

Do we need a regression test for this?

[Christinadobrzyn]

nudge @allroundexperts do need a regression test for this?

[Christinadobrzyn]

DMd @allroundexperts about the regression test

[allroundexperts]

Checklist

1. The app should take the user back to the profile page when the user clicks the back button in the web browser #28999
2. https://github.com/Expensify/App/pull/28999/files#r1503761169
3. N/A
4. A regression test would be helpful here. The steps given in the OP look clear enough to me.

[Christinadobrzyn]

Awesome! Thanks @allroundexperts - created the regression test.

I think this can be closed!

[JmillsExpensify]

$500 approved for @allroundexperts based on this summary.