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

Room - Member avatar changes after being invited to the room #34000

Closed
6 tasks done
lanitochka17 opened this issue Jan 5, 2024 · 54 comments
Closed
6 tasks done

Room - Member avatar changes after being invited to the room #34000

lanitochka17 opened this issue Jan 5, 2024 · 54 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 5, 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: 1.4.21-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:

Issue found when executing PR #33077

Action Performed:

  1. Go to FAB > Start chat > Room
  2. Create a room
  3. Open room chat > Click room header
  4. Click Members > Invite
  5. Invite a few users with default avatar

Expected Result:

The member avatar will not change after being invited to the room

Actual Result:

The member avatar changes after being invited to the room
Each member shares the same default avatar after being invited to the room

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

Bug6332185_1704377196286.20240104_155317.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018d16ed2710b34987
  • Upwork Job ID: 1743087330455277568
  • Last Price Increase: 2024-01-26
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 5, 2024
@melvin-bot melvin-bot bot changed the title Room - Member avatar changes after being invited to the room [$500] Room - Member avatar changes after being invited to the room Jan 5, 2024
Copy link

melvin-bot bot commented Jan 5, 2024

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

Copy link

melvin-bot bot commented Jan 5, 2024

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

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

melvin-bot bot commented Jan 5, 2024

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Copy link

melvin-bot bot commented Jan 5, 2024

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

@tienifr
Copy link
Contributor

tienifr commented Jan 5, 2024

Proposal

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

The member avatar changes after being invited to the room
Each member shares the same default avatar after being invited to the room

What is the root cause of that problem?

There're 2 problems:

  1. In Invite MemberPage, we use getIcons, but in RoomMemberPage, we use getAvatar that makes the inconsistency
  2. InviteRoom API return the new avatar so we can see it's changed after member being invited

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

  1. We should use getAvatar for icons in here
result.icons = [{
        source:  UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID),
        type: CONST.ICON_TYPE_AVATAR,
        name: personalDetail.login,
        id: personalDetail.accountID,
    }]

Or we can use getIcons in here
2. BE should be fix to return the correct avatar

What alternative solutions did you explore? (Optional)

NA

@Christinadobrzyn
Copy link
Contributor

Oh yeah, this does happen.

image image

I think this can be part of vip-vsp.

I'm going ooo next week so adding a BZ teammate to monitor this while I'm gone.

Status- we need to review proposals.

@Christinadobrzyn Christinadobrzyn added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jan 7, 2024
Copy link

melvin-bot bot commented Jan 7, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jan 10, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Jan 10, 2024

  1. In Invite MemberPage, we use getIcons, but in RoomMemberPage, we use getAvatar that makes the inconsistency
  2. InviteRoom API return the new avatar so we can see it's changed after member being invited

So far, I observed the second problem (backend). As for the first one, the createOption is used in several other lists, so I think we need to handle it with caution.

image

@melvin-bot melvin-bot bot removed the Overdue label Jan 10, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jan 12, 2024
Copy link

melvin-bot bot commented Jan 15, 2024

@johncschuster, @ntdiary, @Christinadobrzyn Huh... This is 4 days overdue. Who can take care of this?

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jan 16, 2024

I'm going to be ooo this week as well but from what I can see, I think we're still waiting on proposals - @ntdiary should we raise the bounty?

@melvin-bot melvin-bot bot removed the Overdue label Jan 16, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Jan 16, 2024

I'm going to be ooo this week as well but from what I can see, I think we're still waiting on proposals - @ntdiary should we raise the bounty?

@Christinadobrzyn, eh, I feel what we need is to request a backend engineer to check the API InviteToRoom, why it returns the same avatar url. :)

image

@melvin-bot melvin-bot bot added the Overdue label Jan 18, 2024
@Expensify Expensify deleted a comment from melvin-bot bot Jan 18, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 4, 2024
@grgia
Copy link
Contributor

grgia commented Mar 4, 2024

yes @Christinadobrzyn moving to weekly!

@grgia grgia added the Weekly KSv2 label Mar 4, 2024
@melvin-bot melvin-bot bot removed the Overdue label Mar 4, 2024
@grgia grgia removed the Daily KSv2 label Mar 4, 2024
@grgia
Copy link
Contributor

grgia commented Mar 6, 2024

Sorry for the delay here- here's my thoughts

We can probably solve some of these if we

  • start storing the avatar in the avatar field regardless of default or not (right now, we only store custom avatars for newdot)

The main problem I see is that because we now use accountIDs, something that we cannot predict for a new user- any user that is invited that doesn't have an account will not use the correct avatar. This is partially why I used emails to hash in the original design.

With the privacy issue, storing the default avatar in known users would solve this problem- but I'm not sure what we would do about "guessing" a user's default avatar for a user that doesn't exist yet. Maybe we should go back to using fallback avatar for any users that you don't know?

What do you think @puneetlath @Beamanator (accountIDs/privacy perspective) and @shawnborton / @dannymcclain / @dubielzyk-expensify (design perspective)

Please let me know if you need any clarification

@dannymcclain
Copy link
Contributor

@grgia so are you suggesting something like this?

image

Where Quad Cowboy is a known user with one of our default avatars and [email protected] is not yet a known user, so we give them the fallback avatar instead of assigning them a default avatar? And then once they become a known user, we'd assign them a random default avatar?

@shawnborton
Copy link
Contributor

Ooo yeah that's a tricky one, but I quite like Danny's suggestion.

@dubielzyk-expensify
Copy link
Contributor

Agree that I like what D-man has suggested here

@puneetlath
Copy link
Contributor

I like @dannymcclain's suggestion. So we'd use that fallback avatar whenever we have user with an optimistic accountID. But replace it whenever we have a real accountID.

@melvin-bot melvin-bot bot added the Overdue label Mar 14, 2024
@grgia
Copy link
Contributor

grgia commented Mar 20, 2024

whenever we have user with an optimistic accountID

@puneetlath how do we calculate/set optimistic accountIDs?

Edit: nvm, I see you're agreeing we use this fallback for optimistic accountIDs (AKA any time we dont have the real accountID for a user)

@melvin-bot melvin-bot bot removed the Overdue label Mar 20, 2024
@grgia
Copy link
Contributor

grgia commented Mar 20, 2024

One question I have, do we care on a privacy level if users know whether or not you have an account? Is it better to have a mismatched avatar temporarily, but every account appears "created". Versus distinguishing to users between accounts that do and don't exist

Screen.Recording.2024-03-20.at.1.22.31.PM.mov

@grgia
Copy link
Contributor

grgia commented Mar 20, 2024

If the above video looks good, I can push these changes :)

@dannymcclain
Copy link
Contributor

I think the above video looks good, and I can actually see the ability to distinguish between the two being beneficial for users—but I didn't consider any privacy concerns, so I want to let someone else weigh in on that point too.

@shawnborton
Copy link
Contributor

Hmm great question - I say bring that one to Slack for others to weigh in, as I'm not sure what the best answer is.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Mar 22, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

This issue has not been updated in over 15 days. @ntdiary, @grgia, @Christinadobrzyn eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Apr 16, 2024

hi! Just checking on this, is there anything I can do to help? It looks like we're working on a PR - #38674

@Christinadobrzyn
Copy link
Contributor

PR - #38674 is closed - I'll retest tomorrow

@Christinadobrzyn
Copy link
Contributor

Testing again, I think this is fixed. Going to ask QA to test. https://expensify.slack.com/archives/C9YU7BX5M/p1718242220561929

@kavimuru
Copy link

@Christinadobrzyn bug is fixed.

20240613_231100.mp4

@Christinadobrzyn
Copy link
Contributor

Awesome! I'll close this since the bug is fixed. @grgia let me know if you feel otherwise and we can reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review
Projects
No open projects
Archived in project
Development

No branches or pull requests