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

Develop #28

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Develop #28

wants to merge 4 commits into from

Conversation

hari-n
Copy link
Contributor

@hari-n hari-n commented Jan 8, 2014

Thanks for the social extension to Flask. I am using it to authenticate against linkedin. So, here is a my small contribution to your effort.

BTW, this is my first development in opensource. So please review thoroughly. Thx

@eriktaubeneck
Copy link
Collaborator

@hari-n thanks for providing this. I'm not quite sure what's going on with the changes you made outside of flask_social/providers/linkedin.py and you very well may be correct that some of this isn't working correctly. I currently use the master branch in production, so I'm not acutely aware of bugs in the Develop branch. I also use it with SQLAlchemy rather than Mongo, so I'm also not aware of what's happening there, except that the Mongo tests do pass on the master branch. So, a plan of action:

  1. @hari-n can you break this first commit into 2 commits, one adding the flask_social/providers/linkedin.py file, the other making your other changes in that first commit? Then I can cherrypick it onto master and test it.
  2. @mattupstate I am going to open another issue to discuss what we want to accomplish with the Develop branch and if we want to add new social connections to master or Develop.

@hari-n
Copy link
Contributor Author

hari-n commented Jan 9, 2014

Here it is. Erik as you suggested, I split the changes into separate commits for you to cherrypick. Hope this helps.

@hari-n
Copy link
Contributor Author

hari-n commented Jan 9, 2014

Unlike the test_app, I have implemented Connection differently using a user_id ReferenceField to User. So please discard that commit.

@eriktaubeneck
Copy link
Collaborator

@hari-n the tests pass locally with the single linkedin file added (and Travis should represent that here soon, knock on wood). Note that the tests to actually touch these provider files, so the fact that the tests pass doesn't say that much.

With respect to the other changes, it's generally the best idea to implement on your end the way that it's done in the tests. Think of the tests as extended documentation (this is in general with open source, not just for this project). With your implementation you may run into other headaches down the road, and we won't be able to help or merge in PR for bug fixes that arise from your revised implementation.

I opened issue #29 to figure out where this update should go.

Also, with respect to the flask_social/providers/linkedin.py itself, I confused as to why the linkedin.LinkedInAuthentication object needs the consumer_key, consumer_secret, and linkedin.PERMISSIONS.enums.values() in the get_api method, but not in the get_provider_user_id method.

@hari-n
Copy link
Contributor Author

hari-n commented Jan 9, 2014

@eriktaubeneck the 'linkedin.LinkedInAuthentication' object is capable of doing oAuth authentication with linkedin by itself. But, when the 'get_provider_user_id' and 'get_connection_values' methods are called, flask-social has already done the oAuth. So, I just passed the token to 'LinkedInAuthentication'.

In get_api, previously stored 'connection' object is passed to the method. If the 'access token' in the connection is expired, then 'LinkedInAuthentication' can acquire new token using the consumer_key and the consumer_secret. Also, the person using the api might need permission from the user for a wider scope.

Of the 4 commits, I agree the last 2 commits (bottom two i.e. changes to 'views.py') are my implementation specific. That said, I would add, instead of using all the values in dict to query for a 'connection', using just the key values would be better (3rd commit from the top).

Also, do you have a test case where the Flask app is run on a sub directory like http://localhost/myapp? If so, you will see the URL redirect issue (2nd commit from the top).

Thanks lot of the guidance on using tests as an extended documentation. It would have stopped me from getting some of the headaches I had, but I also learnt a lot in during the debug process :-)

@eriktaubeneck
Copy link
Collaborator

WRT the subdirectory, what do you have set as the SOCIAL_APP_URL variable? You probably just need to set that to myapp.

@eriktaubeneck eriktaubeneck mentioned this pull request Feb 19, 2014
5 tasks
@hari-n
Copy link
Contributor Author

hari-n commented Feb 28, 2014

@hoprocker please see my last response to eriktaubeneck.

I had that issue because of the way I referenced the User class within the Connection class. Once I saw how User class is referenced in the test_app/mongoengine.py, I corrected my implementation. So please discard that pull request.

@hoprocker
Copy link

Gotcha, and thanks for taking a stab at this implementation.

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