Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[$250] QAB - Workspace avatar remains custom when change do default #49448

Open IuliiaHerets opened 1 month ago

IuliiaHerets commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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+a2177@gmail.com 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:

Screenshots/Videos

https://github.com/user-attachments/assets/7ba18b69-148b-4f61-9332-c5cb3f610240

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-04
Issue OwnerCurrent Issue Owner: @c3024
melvin-bot[bot] commented 1 month ago

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 commented 1 month ago

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

IuliiaHerets commented 1 month ago

@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 commented 1 month ago

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

https://github.com/Expensify/App/blob/37bf6b2fef128786dfcd83257a56f56c0cd1eb3d/src/libs/ReportUtils.ts#L1958-L1965

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;

https://github.com/Expensify/App/blob/37bf6b2fef128786dfcd83257a56f56c0cd1eb3d/src/libs/ReportUtils.ts#L1958-L1965

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.

melvin-bot[bot] commented 1 month ago

📣 @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[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

hayes102 commented 1 month ago

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 https://github.com/Expensify/App/issues/33470, so i think the safest solution would be to fix it from the backend

melvin-bot[bot] commented 1 month ago

📣 @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 commented 1 month ago

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

mkzie2 commented 1 month ago

@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.

https://github.com/user-attachments/assets/29cdc6b6-1359-4241-a2f9-3235c2932406

situchan commented 1 month ago

I am inclined to https://github.com/Expensify/App/issues/49448#issuecomment-2369231324. Can we fix this in backend? 🎀👀🎀

melvin-bot[bot] commented 1 month ago

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

mkzie2 commented 1 month ago

@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 commented 1 month ago

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

mkzie2 commented 1 month ago

@luacmartins Yes that is what I proposed.

luacmartins commented 1 month ago

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 commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

situchan commented 1 month ago

@techievivek kind bump https://github.com/Expensify/App/issues/49448#issuecomment-2372636151

techievivek commented 1 month ago

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

I see, I wasn’t aware we already had a method to retrieve policy data for non-members. Since that's the case, I agree we should use that instead of introducing a new field. It also seems that adding the avatar to reports follows a similar approach to when we added the policyName field https://github.com/Expensify/App/pull/37003/files#diff-5f549daaaaca967d763b3671e371465143185bab398c16825ff9cb2d41a7de7fL120, which also goes against the principle of keeping the DB and Onyx data consistent. Should we look into that as well?

luacmartins commented 1 month ago

Yes, I think we should

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@slafortune @luacmartins @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 month ago

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

slafortune commented 1 month ago

@situchan @luacmartins is there a proposal that will work - or an update?

luacmartins commented 1 month ago

I think there's a bit of internal work that needs to be done before we can change anything in App itself. @techievivek are you able to work on these changes?

techievivek commented 1 month ago

@luacmartins This only requires a frontend change, right? As you mentioned, we already sent all those details via the backend, so can we not pass this to an external contributor via this GH? Or do we need some other backend changes as well?

luacmartins commented 1 month ago

Do non-members already have access to that data? If they do, then I agree we only need FE changes.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@slafortune, @luacmartins, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

situchan commented 1 month ago

@mkzie2 can you please confirm that your solution works for non-members and share video?

mkzie2 commented 1 month ago

@situchan Here is the video. In the video, I invited a user who isn't a member of the WS to the WS chat via mention, and the avatar is displayed correctly. In this case, the invited user will not have the data of the policy and it's used policyAvatar from report data to display

https://github.com/user-attachments/assets/6d9e26c8-b6d0-4bf7-b569-b8b4c140d0aa

situchan commented 1 month ago

it's used policyAvatar from report data to display

So it's outdated avatar, which is wrong

melvin-bot[bot] commented 3 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

wildan-m commented 3 weeks ago

Proposal

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

QAB - Workspace avatar remains custom when change do default

What is the root cause of that problem?

We are using outdated avatar from report?.policyAvatar

https://github.com/Expensify/App/blame/f4a225d9d8f03e46c8b9d8cc22932571df1a1eda/src/libs/ReportUtils.ts#L1942

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

report?.policyAvatar originally created to get policy avatar url because at the time PR created, non-member user doesn't have information about the URL https://github.com/Expensify/App/issues/33470#issuecomment-1867249606, but then this solution will cause synchronization issue.

Since we can now access the policy avatarUrl even when someone is not the member of a workspace, we can revert this PR https://github.com/Expensify/App/pull/35792 and replace all report.policyAvatar usage to policy.avatarUrl

What alternative solutions did you explore? (Optional)

N/A

situchan commented 3 weeks ago

Since we can now access the policy avatarUrl even when someone is not the member of a workspace

@wildan-m can you please double check? Please also share test steps where non-member can see workspace. Also make sure that reverting doesn't cause original bug

wildan-m commented 3 weeks ago

@situchan thanks for your comment, I've tried with fresh account and it's not working as expected, (maybe previously working because of cache) seems BE is not returning any info for non member.

image

FWIW we have a way to retrieve non-member policy data

@luacmartins @techievivek do we need to call some functions to fill that data? I think it will be automatically populated with nonmember info at the time someone invited?

mkzie2 commented 3 weeks ago

So it's outdated avatar, which is wrong

@situchan

melvin-bot[bot] commented 3 weeks ago

@slafortune @luacmartins @situchan this issue is now 4 weeks old, please consider:

Thanks!

situchan commented 3 weeks ago

Do non-members already have access to that data? If they do, then I agree we only need FE changes.

can we get correct answer? I don't think they have access. cc: @luacmartins @techievivek

luacmartins commented 3 weeks ago

@situchan we can probably test this in the frontend by logging in and seeing what kind of data a non-member gets once they try to join a policy.

wildan-m commented 3 weeks ago

@luacmartins I've tried that, no data on the initial invite https://github.com/Expensify/App/issues/49448#issuecomment-2413068913

mkzie2 commented 3 weeks ago

@luacmartins We have two types of none-member:

  1. The user is requesting to join the WS, this user will have the policyDetailsForNonMembers
Screenshot 2024-10-18 at 14 12 26
  1. The user is invited to a WS chat of a member via mention, this user doesn't have policy data and we use report.policyAvatar to display the avatar of the report.
wildan-m commented 3 weeks ago

How can non member request join if they can't access the WS page? is it only for public room?

luacmartins commented 3 weeks ago

One way that I know of is via an invite link

luacmartins commented 3 weeks ago

The user is invited to a WS chat of a member via mention, this user doesn't have policy data and we use report.policyAvatar to display the avatar of the report.

Hmm it seems like we'd need some backend changes to accommodate for this.

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 weeks ago

@slafortune, @luacmartins, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

luacmartins commented 2 weeks ago

Haven't been able to look into this. @techievivek do you have bandwidth to take the internal part of this issue?