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.

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

[HOLD for payment 2024-12-07] [$250] QAB - Workspace avatar remains custom when change do default #49448

Closed
3 of 6 tasks
IuliiaHerets opened this issue Sep 19, 2024 · 105 comments
Closed
3 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 19, 2024

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.38-0
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): gocemate [email protected]
Issue reported by: Applause Internal Team

Action Performed:

  1. Sign-up for an gmail account as a brand new user
  2. Create a workspace and give it a custom avatar
  3. Go to FAB
  4. Click Submit expense and send a manual request to the workspace
  5. Change the workspace avatar to default
  6. Open FAB
  7. Take a look at the workspace avatar in the QAB

Expected Result:

Default workspace avatar should be displayed in the quick action button

Actual Result:

Custom workspace avatar is displayed in the quick action button when user change the workspace avatar to default. Avatar doesn't get removed not only on the QAB but also for the workspace chat also.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6607577_1726658158165.Recording__3962.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838248313041984995
  • Upwork Job ID: 1838248313041984995
  • Last Price Increase: 2024-11-18
  • Automatic offers:
    • c3024 | Reviewer | 105022549
    • mkzie2 | Contributor | 105022550
Issue OwnerCurrent Issue Owner: @slafortune
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

Triggered auto assignment to @slafortune (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.

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-collect - Release 1

@IuliiaHerets
Copy link
Author

@slafortune FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@mkzie2
Copy link
Contributor

mkzie2 commented Sep 19, 2024

Proposal

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

Custom workspace avatar is displayed in the quick action button when user change the workspace avatar to default. Avatar doesn't get removed not only on the QAB but also for the workspace chat also.

What is the root cause of that problem?

When we remove the avatar of the policy, report. policyAvatar hasn't changed. It leads after we remove the avatar, isSameAvatarURL still true then we return the policy avatar from workSpaceIconsCache which is still old policy avatar

App/src/libs/ReportUtils.ts

Lines 1958 to 1965 in 37bf6b2

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) {

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

When we get the policyAvatarURL here, we should check if the policy still exists we should prioritize the avatarURL of policy first

const reportPolicy = policy ?? allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`];

// disabling to protect against empty strings
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const policyAvatarURL = reportPolicy ? reportPolicy?.avatarURL : report?.policyAvatar;

App/src/libs/ReportUtils.ts

Lines 1958 to 1965 in 37bf6b2

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) {

What alternative solutions did you explore? (Optional)

We have some other solutions like updating policyAvatar to empty for all related reports of the workspace when we remove the custom avatar or we can update workSpaceIconsCache of the policy to the default icon when we delete the custom avatar.

Copy link

melvin-bot bot commented Sep 19, 2024

📣 @1234abd! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
@slafortune slafortune added the External Added to denote the issue can be worked on by a contributor label Sep 23, 2024
@melvin-bot melvin-bot bot changed the title QAB - Workspace avatar remains custom when change do default [$250] QAB - Workspace avatar remains custom when change do default Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Sep 23, 2024
@hayes102
Copy link

I think this is more a backend issue, where the
the report?.policyAvatar doesnt update on policy avatar removal.

FYI: the report?.policyAvatar was introduced to solve this issue #33470, so i think the safest solution would be to fix it from the backend

Copy link

melvin-bot bot commented Sep 23, 2024

📣 @hayes102! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@situchan
Copy link
Contributor

Thanks for the proposals. Please explain in root cause why this bug only happens in QAB.

@mkzie2
Copy link
Contributor

mkzie2 commented Sep 24, 2024

@situchan This bug also happens for policy-related reports like the video below. The old policy avatar displays briefly until the OpenReport API is complete that update policyAvatar to empty.

Screen.Recording.2024-09-24.at.13.36.33.mov

@situchan
Copy link
Contributor

I am inclined to #49448 (comment).
Can we fix this in backend? 🎀👀🎀

Copy link

melvin-bot bot commented Sep 24, 2024

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

@mkzie2
Copy link
Contributor

mkzie2 commented Sep 24, 2024

@situchan When we update the avatar of the workspace back-end update the policyAvatar of the report but this will not be returned in the response because maybe we can have many related reports for a workspace. Then when we open a related report this field will be updated. So I think we should update the way we get the avatar in frontend.

cc @luacmartins

@luacmartins
Copy link
Contributor

This is yet another example of duplicated data causing issues. Can we use the data in the policy_ key instead?

@mkzie2
Copy link
Contributor

mkzie2 commented Sep 24, 2024

@luacmartins Yes that is what I proposed.

@luacmartins
Copy link
Contributor

Yea, I think we should be using that. FWIW we have a way to retrieve non-member policy data, so I'm not even sure why we added policyAvatar to the report key as this also goes against our principles of keeping DB and Onyx data the same cc @techievivek

@mkzie2
Copy link
Contributor

mkzie2 commented Sep 25, 2024

@luacmartins I think we used policyAvatar for the case the policy is deleted

Copy link

melvin-bot bot commented Sep 27, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 22, 2024
Copy link

melvin-bot bot commented Nov 22, 2024

📣 @c3024 🎉 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

Copy link

melvin-bot bot commented Nov 22, 2024

📣 @mkzie2 🎉 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 📖

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 22, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 Overdue Weekly KSv2 labels Nov 25, 2024
@melvin-bot melvin-bot bot changed the title [$250] QAB - Workspace avatar remains custom when change do default [HOLD for payment 2024-12-07] [$250] QAB - Workspace avatar remains custom when change do default Nov 30, 2024
Copy link

melvin-bot bot commented Nov 30, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 30, 2024
Copy link

melvin-bot bot commented Nov 30, 2024

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

If no regressions arise, payment will be issued on 2024-12-07. 🎊

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

Copy link

melvin-bot bot commented Nov 30, 2024

@c3024 @slafortune @c3024 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]

@garrettmknight garrettmknight moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Dec 3, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 7, 2024
@c3024
Copy link
Contributor

c3024 commented Dec 8, 2024

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

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

Where bug was reported:

  • 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
  • 3d. QA
  • 3z. Other:
  • [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: This is an edge case and the offending lines changed over many PRs so no specific PR can be held responsible.

  • [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: No discussion was started because this could not have been identified earlier.

  • [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.

Regression Test Proposal Template
  • [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

Precondition:

  • None

Test:

  1. Create a workspace
  2. Go to workspace settings > Profile > Edit workspace avatar (change to custom avatar)
  3. Go to FAB > Submit expense to submit a manual request to the workspace
  4. Go to workspace settings > Profile > Remove photo (change to default avatar)
  5. Press FAB (do not go to Inbox)
  6. Verify the workspace avatar in QAB is correct

Do we agree 👍 or 👎

@melvin-bot melvin-bot bot added the Overdue label Dec 9, 2024
@techievivek
Copy link
Contributor

Regression test sounds good to me.

@techievivek
Copy link
Contributor

@slafortune Can we please get that added to the testRail? Thanks.

@slafortune
Copy link
Contributor

@c3024 Paid $250 automatic offer (Reviewer)
@mkzie2 Paid $250 automatic offer (Contributor)

@melvin-bot melvin-bot bot removed the Overdue label Dec 10, 2024
@github-project-automation github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

12 participants