App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Workspace - Workspace chat shows default avatar when custom avatar is uploaded offline

Open IuliiaHerets opened this issue 1 year ago β€’ 10 comments

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: 9.0.68-0 Reproducible in staging?: Y Reproducible in production?: Y **Issue was found when executing this PR ** https://github.com/Expensify/App/pull/53100 Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go offline.
  3. Create a workspace.
  4. Go to workspace settings > Profile.
  5. Click on the workspace avatar.
  6. Upload a photo.
  7. Go to workspace chat.

Expected Result:

The workspace avatar in workspace chat should show the custom avatar.

Actual Result:

The workspace avatar in workspace chat shows default avatar when custom avatar is uploaded offline.

Workspace switcher shows the custom avatar instead of default avatar for the workspace.

Workaround:

Unknown

Platforms:

  • [x] Android: Standalone
  • [x] Android: HybridApp
  • [x] Android: mWeb Chrome
  • [x] iOS: Standalone
  • [x] iOS: HybridApp
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/2e4fce97-bf55-4db5-8d68-0cc1bfea79bf

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021864244048539221826
  • Upwork Job ID: 1864244048539221826
  • Last Price Increase: 2024-12-04
Issue OwnerCurrent Issue Owner: @paultsimura

IuliiaHerets avatar Nov 28 '24 16:11 IuliiaHerets

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Nov 28 '24 16:11 melvin-bot[bot]

Proposal

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

Workspace - Workspace chat shows default avatar when custom avatar is uploaded offline

What is the root cause of that problem?

When we set workspace avatar in offline personalDetails?.[report?.ownerAccountID ?? -1]?.avatar here will be undefined => icon here will be default avatar

https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/libs/ReportUtils.ts#L2411

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

We should pass policy?.avatar as fallback in here

        source: personalDetails?.[report?.ownerAccountID ?? -1]?.avatar ?? policy?.avatarURL ?? FallbackAvatar,

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

daledah avatar Nov 28 '24 16:11 daledah

Proposal

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

The workspace avatar in workspace chat shows default avatar when custom avatar is uploaded offline.

What is the root cause of that problem?

When setting the workspace avatar, we only update policy avatarUrl

https://github.com/Expensify/App/blob/60e9c7c69fa37a6fd320ea94d6b6047dc718d783/src/libs/actions/Policy/Policy.ts#L1032

So in getWorkspaceIcon function, we will return iconFromCache.icon

https://github.com/Expensify/App/blob/60e9c7c69fa37a6fd320ea94d6b6047dc718d783/src/libs/ReportUtils.ts#L2071-L2073

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

When setting the workspace avatar, we need to update the expense chat report policy avatar URL too

https://github.com/Expensify/App/blob/60e9c7c69fa37a6fd320ea94d6b6047dc718d783/src/libs/actions/Policy/Policy.ts#L1047

       {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT}${getPolicyExpenseChat(accountID, policy?.id ?? '-1')?.reportID ?? '-1'}`,
            value: {
                policyAvatar: file.uri,
            },
        },

And will set to default in failureData

What alternative solutions did you explore? (Optional)

NA

ghost avatar Nov 28 '24 17:11 ghost

Proposal

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

WS chat shows default WS avatar.

What is the root cause of that problem?

When we create a new workspace, the avatar is initially a default avatar. When we change the avatar, the avatar URL is changed in the onyx. However, the icon returns the cached avatar, that is the default avatar. https://github.com/Expensify/App/blob/60e9c7c69fa37a6fd320ea94d6b6047dc718d783/src/libs/ReportUtils.ts#L2059-L2073

isSameAvatarURL is false, but report?.policyAvatar === undefined is true, so we return the cache. The reason we added report?.policyAvatar === undefined is explained here.

The reason is, if we receive an invoice from a workspace that we don't have the data, the only way to get the avatar source is from the invoice report policyAvatar. This avatar is then cached. But just in case there is another report that is related to that invoice policy with missing policyAvatar, the avatar will be the default avatar and we don't want to replace the cached avatar with the default avatar.

But in this issue, report?.policyAvatar is undefined, but we have the policy data avatarURL.

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

To fix this, we should use the cache if both report?.policyAvatar and policy avatarURL are missing (undefined).

if (iconFromCache && (isSameAvatarURL || !policyAvatarURL) && !hasWorkSpaceNameChanged) {
    return iconFromCache.icon;
}

bernhardoj avatar Nov 28 '24 17:11 bernhardoj

@JmillsExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Dec 02 '24 09:12 melvin-bot[bot]

@JmillsExpensify Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] avatar Dec 04 '24 09:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 04 '24 09:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 04 '24 09:12 melvin-bot[bot]

Good polish item for us to handle in the name of offline first, so I'm opening up to the community.

JmillsExpensify avatar Dec 04 '24 09:12 JmillsExpensify

Reviewing soon πŸ‘€

paultsimura avatar Dec 04 '24 11:12 paultsimura

Thanks for the proposals, y'all.

The one by @bernhardoj has the most to-the-root RCA analysis and a proper solution. The other 2, unfortunately, look quite hacky and situational.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

paultsimura avatar Dec 04 '24 17:12 paultsimura

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

melvin-bot[bot] avatar Dec 04 '24 17:12 melvin-bot[bot]

Yep, @bernhardoj's solution sounds great. Thank you @paultsimura, let's move forward!

MarioExpensify avatar Dec 04 '24 17:12 MarioExpensify

πŸ“£ @paultsimura πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] avatar Dec 04 '24 17:12 melvin-bot[bot]

PR is ready

cc: @paultsimura

bernhardoj avatar Dec 05 '24 05:12 bernhardoj

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

melvin-bot[bot] avatar Dec 12 '24 01:12 melvin-bot[bot]

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

  • https://github.com/Expensify/App/pull/53618

If no regressions arise, payment will be issued on 2024-12-19. :confetti_ball:

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

  • @paultsimura requires payment automatic offer (Reviewer)
  • @bernhardoj requires payment through NewDot Manual Requests

melvin-bot[bot] avatar Dec 12 '24 01:12 melvin-bot[bot]

@paultsimura @JmillsExpensify @paultsimura The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Dec 12 '24 01:12 melvin-bot[bot]

Payment summary:

Contributor: @bernhardoj $250 via New Expensify Reviewer: @paultsimura $250 via Upwork

@bernhardoj feel free to request now. @paultsimura please complete the BZ checklist and we can get the regression test and Upwork payment handled.

JmillsExpensify avatar Dec 19 '24 09:12 JmillsExpensify

Requested in ND.

bernhardoj avatar Dec 19 '24 09:12 bernhardoj

Payment summary:

Contributor: @bernhardoj $250 via New Expensify Reviewer: @paultsimura $250 via Upwork

garrettmknight avatar Dec 19 '24 09:12 garrettmknight

$250 approved for @bernhardoj

JmillsExpensify avatar Dec 19 '24 09:12 JmillsExpensify

BugZero Checklist:

  • [x] [Contributor] Classify the bug:
Bug classification

Source of bug:

  • [ ] 1a. Result of the original design (eg. a case wasn't considered)
  • [x] 1b. Mistake during implementation
  • [ ] 1c. Backend bug
  • [ ] 1z. Other:

Where bug was reported:

  • [x] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • [ ] 2b. Reported on staging (eg. found during regression or PR testing)
  • [ ] 2d. Reported on a PR
  • [ ] 2z. Other:

Who reported the bug:

  • [ ] 3a. Expensify user
  • [ ] 3b. Expensify employee
  • [ ] 3c. Contributor
  • [x] 3d. QA
  • [ ] 3z. Other:
  • [x] [Contributor] 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: https://github.com/Expensify/App/pull/49145/files#r1891617459

  • [x] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A

  • [x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [ ] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Test:

  1. Go offline
  2. Create a workspace
  3. Go to workspace settings > Profile
  4. Change the workspace avatar
  5. Go to workspace chat
  6. Verify the workspace avatar on the LHN and workspace chat show the custom avatar

Do we agree πŸ‘ or πŸ‘Ž

paultsimura avatar Dec 19 '24 10:12 paultsimura

Thanks @JmillsExpensify, the checklist is βœ”οΈ

paultsimura avatar Dec 19 '24 10:12 paultsimura