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.64k stars 2.93k forks source link

[Internal] Room - Room chat avatar is different for user who creates room & who has been invited to room #33470

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 11 months 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: 1.4.15-4 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. Launch app
  2. Tap fab--start chat
  3. Tap room
  4. Enter room name and select a workspace with custom avatar
  5. Tap create room
  6. Tap header
  7. Tap members---Invite
  8. Invite user X to room 9 .Navigate to LHN and note avatar of room chat
  9. Go to https://staging.new.expensify.com/
  10. Login as user X
  11. Note avatar of room chat

Expected Result:

Room chat avatar must be same for user who creates room and for user who has been invited to room

Actual Result:

Room chat avatar is different for user who creates room and for user who has been invited to room

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/fbc7a3bd-7ef8-4165-83c4-072d8c245c11

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013499a52b4634b0ed
  • Upwork Job ID: 1737991692071854080
  • Last Price Increase: 2023-12-29
Issue OwnerCurrent Issue Owner: @grgia
melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 11 months ago

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

unidev727 commented 11 months ago

Proposal

from: @unicorndev-727

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

Room chat avatar is different for user who creates room & who has been invited to room

What is the root cause of that problem?

The root cause is that the user who has been invited to room doesn't have the policy data about workspace in Onyx so that the default workspace avatar will be showed. This issue only happens when we invite the user who isn't member of this workspace to the only room. https://github.com/Expensify/App/blob/8970aa3cd4ffc73cc59b9de3a50fd3d4da361f15/src/libs/ReportUtils.ts#L1247-L1249

It is a screen shot of the room owner. image It is a screen shot of the room invited user. image

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

We can send the policy update request if ONYXKEYS.COLLECTION.POLICY}${report?.policyID} is null here.

What alternative solutions did you explore? (Optional)

Backend need to send the workspace data to the users who has been invited to the room and isn't the member of the workspace.

dukenv0307 commented 11 months ago

Proposal

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

Room chat avatar is different for user who creates room and for user who has been invited to room

What is the root cause of that problem?

We're using the workspace avatar as the policy room avatar.

But when the user is invited to a policy room, but the user is not a member of that workspace, they don't have access to that information. That's expected because if user is not in the workspace, they shouldn't have access to the policy record.

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

We had the same issue before with the workspace name, and fixed it by adding the policyName as a field in the report of the room.

We should do the same here for the workspace avatar, we need to add a policyAvatar field in the report record, which will be synced with the avatar of the workspace, the back-end will also need to be updated to support this.

Then in front-end we'll use that field as a fallback in case the policy avatar is not found. If even that policyAvatar is not available, now we fallback to the default workspace avatar as we currently do.

This way, even though the user doesn't have any access to the workspace data, they can still access the policyAvatar (beside policyName) to display as room avatar.

What alternative solutions did you explore? (Optional)

NA

alexpensify commented 11 months ago

@getusha - - when you get a chance, can you review if this proposal will fix this issue? Thanks!

Heads up, I will be offline from Friday, December 22, to Thursday, January 4, 2024. I will not be actively watching over this GitHub during that period. If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

getusha commented 11 months ago

@unicorndev-727 do you mind adding more details to the solution you mentioned in your proposal?

unidev727 commented 11 months ago

@unicorndev-727 do you mind adding more details to the solution you mentioned in your proposal?

In fact, this is the best place to call the backend API to allow workspace data to invited users who are not members of the workspace. https://github.com/Expensify/App/blob/595bf4075988c0b64cb0db1e37694284637fbea2/src/libs/actions/Report.ts#L2165-L2175 Frontend is subscribted to policy data properly to get workspace icons here https://github.com/Expensify/App/blob/595bf4075988c0b64cb0db1e37694284637fbea2/src/components/LHNOptionsList/LHNOptionsList.js#L117 and here https://github.com/Expensify/App/blob/595bf4075988c0b64cb0db1e37694284637fbea2/src/libs/ReportUtils.ts#L1270. But it doesn't work because we don't have the workspace data in ONYXKEYS.COLLECTION.POLICY. So we need to update inviteToRoom API to share workspace data with the invited users who aren't member of the workspace by setting role to guest in policy so that the guest can only read policy data like name, avatar and so on, I think.

type InviteToRoomParameters = {
        reportID: string;
        inviteeEmails: string[];
        policyId: string;
        role: string;
    };

    const parameters: InviteToRoomParameters = {
        reportID,
        inviteeEmails,
        policyId: report.policyId,
        role: 'guest'
    };

    API.write('InviteToRoom', parameters, {optimisticData, successData, failureData});

image image This prevents inconsistencies in policy updates and also ensures security.

dukenv0307 commented 11 months ago

@getusha what do you think about my proposal? We already do the same with the policyName, and with that we don't need new authentication system for "guest" to access the workspace data as mentioned above.

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months 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 11 months ago

@alexpensify, @getusha Still overdue 6 days?! Let's take care of this!

getusha commented 11 months ago

Reviewing proposals.

alexpensify commented 11 months ago

@getusha - I'm back online again. Any update regarding the proposals?

getusha commented 11 months ago

🎀 👀 🎀 Pulling up internal engineer to confirm if we can return workspace data (avatar) to a non-member user that joined a room and fix this on the backend initially.

melvin-bot[bot] commented 11 months ago

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

grgia commented 11 months ago

@getusha looking into this

grgia commented 11 months ago

Internal convo link: https://expensify.slack.com/archives/C03TQ48KC/p1704412668387289

melvin-bot[bot] commented 11 months ago

@alexpensify @grgia @getusha 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 11 months ago

@alexpensify, @grgia, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 11 months ago

Current assignee @getusha is eligible for the Internal assigner, not assigning anyone new.

alexpensify commented 11 months ago

Thanks, @grgia for this update!

alexpensify commented 10 months ago

Looks like there is another PR to review here.

dukenv0307 commented 10 months ago

@alexpensify @getusha it looks like the PR opened by @grgia is the front-end part of this issue, which uses the same solution as my proposal. Just checking if I should be assigned here to open the PR instead since the back-end change was done (or if we go with the PR by @grgia, assigned for partial compensation)?

alexpensify commented 10 months ago

@getusha and @grgia - can we get your feedback here? Thanks!

alexpensify commented 10 months ago

@getusha or @grgia - any update here?

chrispader commented 10 months ago

i think @grgia is OOO right now and she asked me to take this over. Just checking, if this should be handled by @dukenv0307 since he/she created the initial proposal or by me (or both?)...

I won't be able to work on the backend part, since i'm not an internal engineer but can give https://github.com/Expensify/App/pull/34115 a look

cc @alexpensify

dukenv0307 commented 10 months ago

Just checking, if this should be handled by @dukenv0307 since he/she created the initial proposal or by me (or both?)...

I'd be happy to take over the PR if @getusha also agrees the PR is implementing the solution in my proposal

alexpensify commented 10 months ago

Thanks, so we will wait for feedback from @getusha. Thanks!

getusha commented 10 months ago

@dukenv0307's proposal looks similar to the raised PR, either compensating partial bounty or other option works for me. the partial compensation sounds good to me, i'll wait for @grgia's input.

dukenv0307 commented 10 months ago

i'll wait for @grgia's input.

@alexpensify Looks like @grgia is OOO and was looking for someone to take over. I can take over if that's ok.

Or we can wait till @grgia 's back 👍

lanitochka17 commented 10 months ago

While testing these steps, the team discovered that the internal avatars were different. The steps are the same. Should we open a separate problem or relate it to this one? Thanks

https://github.com/Expensify/App/assets/78819774/c9393cd6-ecf1-435f-a61e-9d26af2a6563

dukenv0307 commented 10 months ago

Should we open a separate problem or relate it to this one?

@lanitochka17 It's not related (different root cause). I think we can create a new GH issue for that.

alexpensify commented 10 months ago

I agree to separate the GH issue, thanks!

grgia commented 10 months ago

@dukenv0307 if you'd like to open a new PR and do testing instead, that works as well. I can close my PR

dukenv0307 commented 10 months ago

@dukenv0307 if you'd like to open a new PR and do testing instead, that works as well. I can close my PR

@grgia sure I'm happy to do that.

@alexpensify would you mind assigning me to this issue to handle the front-end PR, thank you!

alexpensify commented 10 months ago

Done!

alexpensify commented 10 months ago

Daily update: waiting for the PR

getusha commented 10 months ago

@dukenv0307 don't forget to check https://github.com/Expensify/App/pull/34115#issuecomment-1889007115

dukenv0307 commented 10 months ago

@getusha This maybe BE bug since BE doesn't send the policyAvatar correctly of the report to the invited user.

getusha commented 10 months ago

@grgia could you please check this https://github.com/Expensify/App/issues/33470#issuecomment-1926205427 on BE side? ty!

alexpensify commented 9 months ago

Weekly Update: On Hold, waiting for @grgia's feedback.

alexpensify commented 9 months ago

Weekly Update: I followed up in chat for help here.

grgia commented 9 months ago

@getusha could you please summarize where we're at with this issue and what questions are left outstanding. If there are BE changes required, I can make them

dukenv0307 commented 9 months ago

@getusha this PR is ready for preview.

getusha commented 9 months ago

@getusha could you please summarize where we're at with this issue and what questions are left outstanding. If there are BE changes required, I can make them

@grgia We have 2 issues related to this, more likely a BE bug.

  1. https://github.com/Expensify/App/pull/34115#issuecomment-1889007115
  2. https://github.com/Expensify/App/pull/35792#issuecomment-1930691489

github confused the PR linked to this issue as merged because another PR had commits it doesn't supposed to have, we've got a new PR now :)

alexpensify commented 9 months ago

Weekly Update: Waiting on feedback

getusha commented 9 months ago

@grgia any updates on these issues? https://github.com/Expensify/App/issues/33470#issuecomment-1957586801

grgia commented 9 months ago

I looked into this the other way it's been marked as internal but I don't think this is a high priority bug right now. What we will do is use the policyName pattern as @dukenv0307 suggested for sharing the avatar (if any) as well.