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-09-01] [$250] iOS - Random avatar color changes after creating a group for a new user #10220

Closed
kavimuru opened this issue Aug 2, 2022 · 19 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Aug 2, 2022

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


Issue was found when executing #9963

Action Performed:

  1. Click FAB > New Group
  2. Select a user you've never had a chat with
  3. Add 2 more users and create a group

Expected Result:

Randomly generated avatar color should be same when selecting a user to create a group and after group chat initiated

Actual Result:

Avatar color is different in the group chat than initially selected

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS

Version Number: 1.1.87-8

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:

Bug5673400_DHBY0250_1_.mp4

Expensify/Expensify Issue URL:

Issue reported by:
Applause internal team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2022

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

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Aug 3, 2022

When you are entering the name on the New Group page, name starts with an uppercase letter. When you're on Details page, name starts with a lowercase letter. getDefaultAvatar is case sensitive, those are two different names, hence two different colors.

If that is not the expected behavior, all that's needed is changing this line in getDefaultAvatar

function getDefaultAvatar(login = '') {
    // There are 8 possible default avatars, so we choose which one this user has based
    // on a simple hash of their login (which is converted from HEX to INT)
-   const loginHashBucket = (parseInt(md5(login).substring(0, 4), 16) % 8) + 1;
+   const loginHashBucket = (parseInt(md5(login.toLowerCase()).substring(0, 4), 16) % 8) + 1;
    return `${CONST.CLOUDFRONT_URL}/images/avatars/avatar_${loginHashBucket}.png`;
}

@madmax330 madmax330 added the External Added to denote the issue can be worked on by a contributor label Aug 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 6, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2022

@stephanieelliott Huh... This is 4 days overdue. Who can take care of this?

@stephanieelliott
Copy link
Contributor

Hey @kavimuru, heads up we skipped a few steps with this one - this (and all new issues) should get the Auto Assigner Triage label first, then the assigned triager will apply the Engineering label 😊.

@melvin-bot melvin-bot bot removed the Overdue label Aug 9, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2022

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

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

melvin-bot bot commented Aug 9, 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 iOS - Random avatar color changes after creating a group for a new user [$250] iOS - Random avatar color changes after creating a group for a new user Aug 9, 2022
@stephanieelliott
Copy link
Contributor

stephanieelliott commented Aug 9, 2022

There was discussion on #9876 (comment) and we determined this was not caused by a regression, so treating this as a normal job.

Messed this one up and closed the job post after just hiring C+, so had to create separate jobs:
C+: https://www.upwork.com/jobs/~01c69c0915df9c82b8
C: https://www.upwork.com/jobs/~0172aa16b79ead5420

@rushatgabhane heads up there is a proposal on this one already: #10220 (comment)

@aimane-chnaif
Copy link
Contributor

Proposal

This happens when email is capitalized (first letter is uppercased). Email is usually capitalized because search input component (TextInput)'s autoCapitalize prop is not set, which means 'sentences' as default. If this prop is set as 'none', any input such as email will not be capitalized unless user input uppercased letter manually. But search keyword can be person name as well so capitalize is normal.

This is the main issue:
When "Create group" button is triggered, DeprecatedAPI.CreateChatReport() is called with emails as emailList param.
Backend converts all capitalized emails to lowercased (this is normal flow since emails are usually not case sensitive) and create group with updated emails and return them in response data.
That's why default avatars are different between "before create group" and "after create group", based on capitalized status of email.

Solution
getDefaultAvatar can be non-case sensitive but I think this is just a workaround.
getDefaultAvatar is not only for email but also for person name, phone number, etc.
Person name is case sensitive so getDefaultAvatar should also be case sensitive if person name is used.
So my first proposal is to make getDefaultAvatar non-case sensitive only for email.
In getDefaultAvatar function, update this line

function getDefaultAvatar(login = '') {
    // There are 8 possible default avatars, so we choose which one this user has based
    // on a simple hash of their login (which is converted from HEX to INT)
    const loginHashBucket = (parseInt(md5(Str.isValidEmail(login) ? login.toLowerCase() : login).substring(0, 4), 16) % 8) + 1;
    return `${CONST.CLOUDFRONT_URL}/images/avatars/avatar_${loginHashBucket}.png`;
}

Before: md5(login)
After: md5(Str.isValidEmail(login) ? login.toLowerCase() : login)

Second proposal:

  • autoCapitalize prop of TextInput set to 'none'
  • don't change capitalized email to lowercased in backend, assuming frontend will send lowercased email
    So if user capitalize email manually in keyboard (mostly won't happen) and submit, it will be saved without change in backend
    BTW, I don't think this proposal is perfect because search keyword can be person name which is capitalized

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 9, 2022
@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 9, 2022

@eVoloshchak the issue and your proposal make sense! (also it looks like it's not a regression)

@puneetlath I like @eVoloshchak's proposal.

🎀 👀 🎀 C+ reviewed

@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 Aug 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 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 📖

@puneetlath
Copy link
Contributor

I agree.

@aimane-chnaif thanks for your proposal, but we're going to go with @eVoloshchak given that they proposed first and I think the check for whether it is email before setting to lowercase is overkill. Thanks!

@stephanieelliott
Copy link
Contributor

stephanieelliott commented Aug 16, 2022

Hey @eVoloshchak, any update on progress?

Also, I sent you an offer for this job in Upwork, please accept when you get a chance: https://www.upwork.com/jobs/~0172aa16b79ead5420

@eVoloshchak
Copy link
Contributor

Hey @eVoloshchak, any update on progress?

Either something was broken by recent changes or in npm registry, but my npm I fails with react-native-web being at fault. I'll try to resolve this by the end of the week

Also, I sent you an offer for this job in Upwork, please accept when you get a chance: https://www.upwork.com/jobs/~0172aa16b79ead5420

This is the posting for completely different issue
image

@eVoloshchak
Copy link
Contributor

Hey @eVoloshchak, any update on progress?

Either something was broken by recent changes or in npm registry, but my npm I fails with react-native-web being at fault. I'll try to resolve this by the end of the week

Also, I sent you an offer for this job in Upwork, please accept when you get a chance: https://www.upwork.com/jobs/~0172aa16b79ead5420

This is the posting for completely different issue image

And there's no need, we already have a contract running for this job on Upwork

@eVoloshchak
Copy link
Contributor

PR is up

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.89-4 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-09-01. 🎊

@melvin-bot melvin-bot bot changed the title [$250] iOS - Random avatar color changes after creating a group for a new user [HOLD for payment 2022-09-01] [$250] iOS - Random avatar color changes after creating a group for a new user Aug 25, 2022
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 1, 2022
@stephanieelliott
Copy link
Contributor

All paid up, thanks!

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

No branches or pull requests

7 participants