-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Disable Gravatar #37
Disable Gravatar #37
Conversation
thomiceli
commented
May 18, 2023
- If user is using an OAuth provider, use this avatar url, else
- If Gravatar is enabled and user has set an email, use Gravatar url, else
- Use default avatar
@josefandersson thoughts ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great man. I completely forgot this myself so it's awesome that you picked it up... 😅 I really only had some nitpicks, do as you please!
userMap := make(map[string]*User) | ||
|
||
err := db. | ||
Where("email IN ?", emails). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are emails normalized in db or will this create issues with capitalization? A normalizedEmail
field in db may be good otherwise? For this use-case and when looking up by email in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right it might create issues ; the current email
field in db should contain only lowercase emails, and the fetched emails in commits would be set as lowercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. I also think it's best to save the original email also, and not just a normalized one--hence why I would recommend a normalizedEmail field rather than normalizing the current email field. Emails can technically be case sensitive and it could technically screw with future email notification/sending functionality if the original is not kept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your way of doing it makes good enough sense for the scale of this project. Same with not bothering to care about migrating old emails in db to lowercase. Maybe in a few years it's a different story. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff man!
* Disable Gravatar * Lowercase emails * Add migration