Skip to content
This repository has been archived by the owner on Feb 22, 2024. It is now read-only.

Get email from facebook #1

Closed
wants to merge 2 commits into from
Closed

Conversation

alexef
Copy link
Contributor

@alexef alexef commented Apr 24, 2013

I'm using it like this:

   @login_failed.connect_via(app)
   def on_login_failed(sender, provider, oauth_response):
      cv = get_connection_values_from_oauth_response(provider, oauth_response)
      ds = security.datastore
      user = ds.create_user(display_name=cv.get('display_name', None), email=cv.get('email'))
      ds.commit()
      ...

I know it's poor design, if you guide me, I'll write a better solution. Thanks.

… it to connect_handler, since it won't work.
@mattupstate
Copy link
Collaborator

I can't accept this pull request because its not good to assume that the user wants to register an account with the same email they have associated with their facebook account.

@alexef
Copy link
Contributor Author

alexef commented Apr 25, 2013

Thanks for the other merge :)

Please have a second look at this, the user registration email choice is in the application, not in flask-social. This pull request only tries to fetch email in the same graph api call, and leave it there, for further usage.

@mattupstate
Copy link
Collaborator

I understand you're motivation now. I don't know why I made my previous conclusion. Suppose I didn't consider the context entirely.

@mattupstate mattupstate reopened this Apr 25, 2013
@@ -148,6 +148,8 @@ def connect(response):
if cv is None:
do_flash('Access was denied by %s' % provider.name, 'error')
return redirect(get_url(config_value('CONNECT_DENY_VIEW')))
if 'email' in cv.keys():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part I'm not happy about. Any hints to do it better? Leaving 'email' in cv will cause Connection model changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cv.pop('email', None) is cleaner, if thats what you're looking for.

@eriktaubeneck
Copy link
Collaborator

@alexef maybe I'm misunderstanding, but I think leaving email in cv should be fine. What you don't want to do is make the changes you made to on_login_failed(). The email on the Connection object should be independent from the User object email, but the application would have access to it through the Connection object (and maybe could even offer it as an option for the user to use as their email during the next setup stage).

In this case, you'd add the email to the dict returned from get_connection_values() in each of the provider modules. For example, in twitter.py, you'd add:

return dict(
    ...
    display_name = '@%s' % user.screen_name,
    email = user.email,
    ...
)

assuming that user.email exists (I'm not familiar enough with python-twitter to know off the top of my head, but it would be easy enough to verify.

It would be very similar to this commit I made to add full_name as a value stored in the Connection object.

@alexef
Copy link
Contributor Author

alexef commented Oct 14, 2013

@eriktaubeneck it's been a while since writing this pull, but afair, the reason for removing email was this call:
https://github.com/alexef/flask-social/blob/f58aa6ddb86e1ba649f05ff6075f8d0a8893b344/flask_social/views.py#L125

create_connection(**cv) which failed, given the email.

I don't see in your commit where do you modify the datastore to add full_name.

Despite all of that, it makes sense not to mix user.email and connection.email, but at the time, it was to much of a change from my side.

@eriktaubeneck
Copy link
Collaborator

@alexef ah ha, yes. The data store is actually implemented at the application level, so with respect to flask-social-example, it would be in this file, which I still need to update with respect to that commit I merged earlier today! Glad you mentioned that. The Connection class will become:

class Connection(db.Model):

    __tablename__ = "connections"

    id = db.Column(db.Integer, primary_key=True)
    user_id = db.Column(db.Integer, db.ForeignKey('users.id'))
    provider_id = db.Column(db.String(255))
    provider_user_id = db.Column(db.String(255))
    access_token = db.Column(db.String(255))
    secret = db.Column(db.String(255))
    display_name = db.Column(db.String(255))
    full_name = db.Column(db.String(255))
    profile_url = db.Column(db.String(512))
    image_url = db.Column(db.String(512))
    rank = db.Column(db.Integer)

To add the email, you'd just add

    email = db.Column(db.String(255))

to the class. This would be different, I think, if using the Mongo or PeeWee setup (I haven't used them).

As to not mixing user.email and connection.email, I agree. I think my above example might have been confusing though. There I was referencing the values returned from twitter, not the user object, i.e. this. I do think that there are cases where you'd want both a users email that comes along with a social sign up AND a user provided email (and maybe giving the user the option to simply use that email if they like). In those cases, I think the Connection object is the appropriate place to store that.

alexef added a commit to alexef/flask-social-example that referenced this pull request Nov 3, 2013
@eriktaubeneck eriktaubeneck mentioned this pull request Feb 19, 2014
5 tasks
@eriktaubeneck
Copy link
Collaborator

Merged and add email to all other providers. Some may not work (i.e. return '' if the permissions aren't asked for, and some users may not want to request it.

@alexef
Copy link
Contributor Author

alexef commented Jul 10, 2014

This is awesome, just the other day I revisited this code in a new flask app. Thanks for the merge.

@eriktaubeneck
Copy link
Collaborator

Sorry it took me so long, but I just got a bunch of good stuff merged ;) thanks for your contribution!

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

Successfully merging this pull request may close these issues.

3 participants