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

Introduce avatar providers. #137

Merged
merged 1 commit into from
Oct 7, 2016
Merged

Introduce avatar providers. #137

merged 1 commit into from
Oct 7, 2016

Conversation

jnns
Copy link
Contributor

@jnns jnns commented Sep 13, 2016

This pull requests add the functionality to implement different providers for third party avatar sources (like Gravatar, Facebook, Google, etc.).

That's especially useful if you're using apps like django-allauth because you can now implement your own provider either displaying the Facebook or Google profile image depending on which service a user used to sign in with.

It's now possible to specify the order in which third party sources are tried if no Avatar can be found for the given user through a new settings directive AVATAR_PROVIDERS.

Backwards compatibility is kept by still relying on AVATAR_FACEBOOK_BACKUP and AVATAR_GRAVATAR_BACKUP.

@grantmcconnaughey
Copy link
Contributor

grantmcconnaughey commented Sep 13, 2016

I love it. I'd like to change up a few things here before merging:

  1. Let's remove FacebookAvatarProvider from the default providers list. But go ahead and keep it documented so users know it is an option.
  2. This broke the build because providers.py needs a new line at the end of the file. Get that put in there and we'll see what happens with the tests.
  3. Let's change Provider.get_avatar to Provider.get_avatar_url so that it's more clear that this is returning a str that is the URL, not the actual Avatar model.
  4. I'd prefer to remove the AVATAR_GRAVATAR_BACKUP and AVATAR_FACEBOOK_BACKUP settings. If users want to backup to either of those providers then they can simply add them to the list of PROVIDERS.
  5. Finally, let's either implement GoogleAvatarProvider or remove it.

This will be a breaking change, so I might have to go version 4.0 for this one. But I think it is worth it to allow users to extend which avatars are used.

I'm looking forward to this one. Thanks a lot for the work!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 85.575% when pulling c588de2 on jnns:master into 0c16c0c on grantmcconnaughey:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 85.575% when pulling c588de2 on jnns:master into 0c16c0c on grantmcconnaughey:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 85.575% when pulling c588de2 on jnns:master into 0c16c0c on grantmcconnaughey:master.

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.4%) to 85.547% when pulling dc145ed on jnns:master into 0c16c0c on grantmcconnaughey:master.

@jnns
Copy link
Contributor Author

jnns commented Sep 14, 2016

Thanks for the fast feedback, Grant.

I addressed all your issues, fixed an import bug that came from wrong indenting and improved the docs a bit by adding Python role directives.

The GoogleProvider should never have been committed - I guess I just wasn't thorough enough. It's not as straight forward to obtain a Google avatar URL because an access token is needed. And as for all the people using django-allauth, they probably have the direct avatar URL stored in their database anyway so I'd vote for leaving it to the user to implement their own GoogleProvider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants