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 2022-08-11] [$250] The create group chat list uses different default avatars than the actual users who you invite #9876

Closed
mvtglobally opened this issue Jul 13, 2022 · 28 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mvtglobally
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. open app
  2. Click + > Create group
  3. select few existing and new users to be added to the group

Expected Result:

The avatars should be consistent

Actual Result:

The create group chat list uses different default avatars than the actual users who you invite

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.82-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
image - 2022-07-12T234310 342
image - 2022-07-12T234308 937

Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1657639279664059

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@tylerkaraszewski
Copy link
Contributor

This seems super low priority but I do agree it would be better.

@tylerkaraszewski tylerkaraszewski added the External Added to denote the issue can be worked on by a contributor label Jul 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

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

@Christinadobrzyn
Copy link
Contributor

@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

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

@melvin-bot melvin-bot bot changed the title The create group chat list uses different default avatars than the actual users who you invite [$250] The create group chat list uses different default avatars than the actual users who you invite Jul 13, 2022
@jeet-dhandha
Copy link
Contributor

@mvtglobally Not able to reproduce at my end on Android. Is it happening in any other OS or web/desktop app.

@puneetlath
Copy link
Contributor

Hm, I'm also not able to reproduce this on Desktop. Perhaps this was already fixed. @Christinadobrzyn are you able to reproduce it?

@Santhosh-Sellavel
Copy link
Collaborator

This issue is possible in one scenario where you never initiated a chat with the account you are adding to the group.

@Santhosh-Sellavel
Copy link
Collaborator

I believe that's expected behaviour

@shawnborton
Copy link
Contributor

Interesting, I think I would have expected all of the avatar colors to appear random and not all in green. Thoughts on that?

@Santhosh-Sellavel
Copy link
Collaborator

Interesting, I think I would have expected all of the avatar colors to appear random and not all in green. Thoughts on that?

Yeah we could do that.

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Jul 18, 2022

Proposal:
Nothing prevents us from showing avatar of the correct color even for users you never initiated a chat with. For the Details page we generate avatar colors based on login on this line, but for the New group page, we use defaultAvatarForUserToInvite, intentionally not passing login to ReportUtils.getDefaultAvatar.
All we need to do is change this line

userToInvite.icons = [defaultAvatarForUserToInvite];

+   userToInvite.icons = [ReportUtils.getDefaultAvatar(login)];

Before:

cinnamon-20220718-5.mp4

After:

cinnamon-20220718-7.mp4

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 18, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@puneetlath

Need to ensure this case alone,

Consider users A & B both have set a profile picture. Also, User A & User B never had a one-to-one chat.
Now when User A tries to create a group with User B we would show a random colored avatar only in suggestion.

If are expecting just to randomize the avatar color then we are good to go with @eVoloshchak's proposal else we need to look for a different proposal.

@shawnborton
Copy link
Contributor

I think we can keep it simple and just randomize the color per the proposal above.

@puneetlath
Copy link
Contributor

Hm, yeah it's interesting, it looks like we did it this way on purpose at some point to keep them all the same color. But I agree with doing it random, it feels like it fits better with how the rest of the app behaves. Let's do it.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Help Wanted Apply this label when an issue is open to proposals by contributors Daily KSv2 labels Jul 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 18, 2022

📣 @eVoloshchak You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork 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 📖

@eVoloshchak
Copy link
Contributor

Opened a PR

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jul 20, 2022

Ah so sorry, @puneetlath I missed your ping about testing - #9876 (comment). It looks like we're moving forward with a fix!

The original job posting is closed so I can't hire @Santhosh-Sellavel as the C+.

I created a duplicate Upwork posting and hired you as the C+ @Santhosh-Sellavel

Internal posting: https://www.upwork.com/ab/applicants/1549878109834797056/job-details
External posting: https://www.upwork.com/jobs/~01fe88959409bc009c

@melvin-bot melvin-bot bot added the Overdue label Jul 29, 2022
@Christinadobrzyn
Copy link
Contributor

QA is ongoing for the PR - #9963 (comment)

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Overdue Weekly KSv2 labels Aug 1, 2022
@melvin-bot melvin-bot bot changed the title [$250] The create group chat list uses different default avatars than the actual users who you invite [HOLD for payment 2022-08-11] [$250] The create group chat list uses different default avatars than the actual users who you invite Aug 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.87-9 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 2022-08-11. 🎊

@eVoloshchak
Copy link
Contributor

Is #10220 considered a regression, should I submit the fix for it?

@Santhosh-Sellavel
Copy link
Collaborator

@puneetlath
Looks mostly related to root of this issue.
I think this is not a treated as regression. Also issue not introduced by fix provided here.

I'll looking forward to hear your thoughts too on this thanks!

@Santhosh-Sellavel
Copy link
Collaborator

@puneetlath bump

@puneetlath
Copy link
Contributor

Sorry! I was off last week. Thanks for the bumps.

I agree that it's not a regression. That's how we agreed to implement it. So I think it's fine to just go ahead and handle that on the separate issue.

@melvin-bot

This comment was marked as resolved.

1 similar comment
@melvin-bot

This comment was marked as resolved.

@Christinadobrzyn
Copy link
Contributor

Looks like we're good to pay this!

Paid @eVoloshchak through this job - https://www.upwork.com/jobs/~01b994676f0c2a7858
Paid @Santhosh-Sellavel as C+ through this job - https://www.upwork.com/jobs/~01fe88959409bc009c

Closed the jobs in Upwork and closing this GH

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 Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants