Add function for retrieving user permissions #264

seawolf42 · 2016-01-12T23:44:19Z

This adds a function get_permissions() that returns the user permissions from the Graph API. To use this you must supply a user token.

For testing, set the environment variable FACEBOOK_USER_ACCESS_TOKEN to a token retrieved from the Graph API explorer... if this is not set then the test will be skipped.

…d on the existence of the token

seawolf42 · 2016-01-13T00:14:35Z

Can someone describe to me how I get the Travis CI build to pass? The error is about environment variables being set... presumably I need to set environment variables so that it can test against the Facebook API, but I don't know how to do this.

martey · 2016-01-13T02:26:51Z

The environment variables are set automatically from the .travis.yml file. RIght now, your pull request is causing the build to fail because it doesn't pass the flake8 linting:

facebook/__init__.py:107:19: E201 whitespace after '{'
facebook/__init__.py:107:65: E901 SyntaxError: invalid syntax
facebook/__init__.py:107:79: E202 whitespace before '}'
facebook/__init__.py:107:80: E501 line too long (80 > 79 characters)

test/test_facebook.py:220:1: W293 blank line contains whitespace
test/test_facebook.py:224:1: W293 blank line contains whitespace
test/test_facebook.py:227:1: W293 blank line contains whitespace
test/test_facebook.py:235:1: W293 blank line contains whitespace

seawolf42 · 2016-01-13T05:01:38Z

Ah, I see. Now I know how what to do with Travis CI output... thanks!

It looks like I am using a decorator that exists in unittest in python 2.7 but not in 2.6. Do you have a preferred method for skipping a unit test if the necessary environment variable does not exist? I could import unittest2 but this might require additional changes throughout the project. I could use a regular if statement, but this will skip the test silently. Thoughts?

seawolf42 · 2016-01-13T07:30:35Z

Actually, a slightly different question. Is there any compelling reason to continue supporting python 2.6? Are there metrics anywhere of how many users are using this module on a 2.6 installation?

martey · 2016-01-13T07:56:52Z

It would be simpler if you assumed that FACEBOOK_APP_ID and FACEBOOK_SECRET exist. If they do not exist, the tests should fail.

seawolf42 · 2016-01-13T08:20:41Z

This is a different parameter, FACEBOOK_USER_ACCESS_TOKEN. Since these expire they can't really be inserted into the .travis.yml file, so the test should be skipped in this case. If you want to test locally you will need to retrieve a valid token in addition to the other environment variables.

martey · 2016-01-14T00:38:29Z

That said, I noticed the method only get permissions for the current user (me); it would be better if it allowed using arbitrary user IDs. I am also not sure whether you are willing to write support for deleting specific permissions as well (see my comment on #35), but it would be a great addition.

Finally, from https://developers.facebook.com/docs/graph-api/reference/user/permissions (emphasis mine):

This request must be made with a user access token or an app access token for the current app.

I think it should be possible to rewrite your test case so it uses app access tokens instead of a user access token, which should sidestep the issues of Python 2.6 compatibility and having an automated test that never runs automatically.

seawolf42 · 2016-01-14T00:41:17Z

I’ll give that a try. I had taken a stab at that but I thought I received an error. Let me check.

jeffrey k eliasen - technologist, philosopher, agent of change
blog http://jeff.jke.net/ | linkedin http://www.linkedin.com/pub/jeffrey-eliasen/3/a83/b76 | google+ http://plus.google.com/+JeffreyEliasen | facebook http://facebook.com/jeffrey.eliasen | twitter http://twitter.com/jeffreyeliasen

On Jan 13, 2016, at 14:38, Martey Dodoo [email protected] wrote:

That said, I noticed the method only get permissions for the current user (me); it would be better if it allowed using arbitrary user IDs. I am also not sure whether you are willing to write support for deleting specific permissions as well (see my comment on #35 #35), but it would be a great addition.

Finally, from https://developers.facebook.com/docs/graph-api/reference/user/permissions https://developers.facebook.com/docs/graph-api/reference/user/permissions (emphasis mine):

This request must be made with a user access token or an app access token for the current app.

I think it should be possible to rewrite your test case so it uses app access tokens instead of a user access token, which should sidestep the issues of Python 2.6 compatibility and having an automated test that never runs automatically.

—
Reply to this email directly or view it on GitHub #264 (comment).

seawolf42 · 2016-01-14T00:53:02Z

On second thought, that won't work. The user permissions that I am retrieving are specific to a session. This isn't data about the user, it's data about the token (which permissions the token has been granted)... without a user access token the test is meaningless.

martey · 2016-01-14T05:30:30Z

The user permissions that I am retrieving are specific to a session. This isn't data about the user, it's data about the token (which permissions the token has been granted)...

I don't understand. If you have an app access token and a user ID that has granted permissions to that app, you should be able to use the app token to query said user ID's permissions.

Facebook permissions are per-app and per-user, but (as far as I know) are not per-token.

seawolf42 · 2016-01-14T05:41:00Z

I think you are right about the per-app and per-user, but even for that case we still need a way to grant a known user a specific permission for the app, and that takes user involvement to create in the first place.

Someone will need to login to FB to grant a token for a test user of some sort (do you have a dummy user and a web app for this purpose?), but if/when that is completed then I can possibly set the test up as you ask. If we do not grant at least the base permissions to any user then there is no way to test that the returned dictionary is structured properly (it will be empty), and if the user in question ever deauthenticates the app then the test will start failing for all developers, so it's kind of convoluted no matter how we do it.

I'm totally open to suggestions here, I'd just like to get closure on this enhancement.

seawolf42 · 2016-01-21T00:23:37Z

Any additional thoughts on this? The function itself is straightforward, but as I mentioned and as far as I can tell, no test is possible without first granting at least one permission to at least one user, so we either need a dummy user that we know will never deauthorize the app or we need to skip the test; otherwise we run the risk of the test suddenly starting to fail because the permission for the user went away.

martey · 2016-01-21T00:46:18Z

we either need a dummy user that we know will never deauthorize the app

I can do this with the Facebook application that is used in Travis CI.

seawolf42 · 2016-01-21T00:47:53Z

That would be great! If you let me know the user’s FB ID, I’ll modify the test.

jeffrey k eliasen - technologist, philosopher, agent of change
blog http://jeff.jke.net/ | linkedin http://www.linkedin.com/pub/jeffrey-eliasen/3/a83/b76 | google+ http://plus.google.com/+JeffreyEliasen | facebook http://facebook.com/jeffrey.eliasen | twitter http://twitter.com/jeffreyeliasen

On Jan 20, 2016, at 14:46, Martey Dodoo [email protected] wrote:

we either need a dummy user that we know will never deauthorize the app

I can do this with the Facebook application that is used in Travis CI.

—
Reply to this email directly or view it on GitHub #264 (comment).

seawolf42 · 2016-01-31T01:38:38Z

Where can I find the user ID for the dummy user you have in Travis CI? I'm happy to update the test so we can close this out...

seawolf42 · 2016-02-28T04:33:55Z

ping :)

I'd really like to get closure on this. Just need the Facebook User ID for the dummy user that you mentioned.

martey · 2016-02-28T05:31:54Z

I haven't gotten the chance to create the test user yet. If you can update the test without it, I can make the necessary modifications (to add in the user ID) when I can.

…rror

seawolf42 · 2016-08-15T00:23:08Z

One additional note, I put a dummy user ID in .travis.yml. For the main test to pass, we need a user ID that has authorized the app to replace the 12345 ID in that file.

seawolf42 · 2016-08-15T00:23:59Z

Regarding "I am also not sure whether you are willing to write support for deleting specific permissions as well (see my comment on #35), but it would be a great addition.", I'm actually happy to do this if it still needs to be done.

seawolf42 · 2016-08-15T00:38:56Z

Documentation added.

seawolf42 · 2016-11-24T18:04:17Z

Although this fails tests, this is because it needs a user ID for a FB user who has given permission for the test app to link to his account. Once that user is created, all tests will pass.

seawolf42 · 2016-11-26T20:47:16Z

D'oh! How about if this time I submit actually working code. My bad :)

Now the only thing left is to create a user which has been authorized in the FB app so that I can un-skip the relevant test.

seawolf42 · 2016-12-27T04:16:57Z

Hey @martey, I'd really love to get this finalized, the only thing that's necessary is a user for testing. What do you need from me to get this the last mile?

martey

I have added some comments.

I also think this pull request's commits could also be rebased into one singular commit - see https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request#squash-your-changes for an example of how to do this.

martey · 2017-01-06T01:39:54Z

.travis.yml

@@ -16,3 +16,4 @@ env:
    global:
        - FACEBOOK_APP_ID=198798870326423
        - FACEBOOK_SECRET=2db4d76fe8a336cf292470c20a5a5684
+        - FACEBOOK_USER_ID=12345


@juhopaak added methods to create test users programmatically (fdc36cc). I think using them is a better solution than using a hardcoded user ID.

Agreed, though this feature wasn't there when we first talked about this :) ... happy to figure out how to make that change.

martey · 2017-01-06T01:41:04Z

facebook/__init__.py

@@ -102,6 +102,14 @@ def __init__(self, access_token=None, timeout=None, version=None,
        else:
            self.version = "v" + default_version

+    def get_permissions(self, user_id, **args):


args doesn't seem to be used in this method. Is there a reason that it has been included?

had used it at one point, I think, and forgot to remove it. thanks!

martey · 2017-01-06T01:41:42Z

facebook/__init__.py

+    def get_permissions(self, user_id, **args):
+        """Fetches the permissions object from the graph."""
+        response = self.request(
+            self.version + "/" + str(user_id) + "/permissions")["data"]


Can we use .format here instead of manually concatenating strings?

martey · 2017-01-06T01:44:18Z

facebook/__init__.py

@@ -235,7 +243,7 @@ def request(
            # or it does not need `access_token`.
            if post_args and "access_token" not in post_args:
                post_args["access_token"] = self.access_token
-            elif "access_token" not in args:
+            elif args and "access_token" not in args:


Is this related to user permissions, or is it a fix for a different bug?

This is probably a different bug... I had a test that was failing before, though when I revert this now that same test isn't failing, so I'm not sure what changed. Your call, do you want this reverted or included? If reverted, should I submit a different PR for it?

I take that back, that is necessary for one of my unit tests to pass. Otherwise I get:

Traceback (most recent call last): File "/Users/seawolf/repos/facebook-sdk-jke/test/test_facebook.py", line 223, in test_get_user_permissions_nonexistant_user facebook.GraphAPI(token).get_permissions(1) File "facebook/__init__.py", line 108, in get_permissions "{0}/{1}/permissions".format(self.version, user_id) File "facebook/__init__.py", line 247, in request elif "access_token" not in args: TypeError: argument of type 'NoneType' is not iterable

The method defaults args to None, but if it is unset then line 246 throws the exception.

Per the Contributing Guidelines, a pull request should only contain commits that aren't relevant to its core feature. If it doesn't have to do with user permissions, it should be in a different pull request.

The purpose of that code is to ensure that the arguments passed to Facebook include an access token. With your code, it is necessary to set args, otherwise this will not happen.

This should have been fixed in #341, so if you rebase your branch off of the current master, the error will go away.

Totally makes sense, removed the change to the request method and added an empty args dict to the call.

seawolf42 · 2017-01-06T20:45:43Z

I'll fix those items and squash it all, should have it to you by the end of the weekend. Thanks!

# Conflicts: # docs/api.rst # test/test_facebook.py

seawolf42 · 2017-01-07T21:23:35Z

Because I had made my changes on master (bad me), I will need to close this PR and create a new one. I've created PR 342 which contains the requested changes.

seawolf42 · 2017-01-10T16:55:06Z

@martey hoping I didn't mess this up by closing this PR and opening the other one... did this fall off the radar as a result?

martey · 2017-01-10T19:14:12Z

@seawolf42 No, I have just been really busy over the past couple of days. The other pull request should be merged soon.

seawolf42 · 2017-01-10T21:28:13Z

Awesome, thanks... just making sure I didn't muck something up, this is my first PR on anything in a couple years :)

seawolf42 added 4 commits January 12, 2016 12:58

retrieve permissions object from API

aa30f81

transform result into dictionary for convenient lookups

2e7d5cd

retrieve the token from the environment once and make all tests depen…

fb0ca41

…d on the existence of the token

move test to bottom of file

f3aa306

seawolf42 added 2 commits January 12, 2016 18:42

flake8 compliant

e0485ff

compliance with python 2.6

a1b8d7c

use unittest2 if older python to support skipIf decorator

efb65e2

martey added the enhancement label Jan 14, 2016

martey added the needs-documentation label Aug 9, 2016

seawolf42 added 4 commits August 14, 2016 16:47

Merge branch 'master' of github.com:mobolic/facebook-sdk

fc4ed3d

use user ID for any arbitrary user rather than just the current user

d2d7486

if user does not exist or has not authorized the app, raise GraphAPIE…

17291f9

…rror

lint clean

65f3d00

seawolf42 added 2 commits August 14, 2016 17:37

documentation for get_permissions method

2d94d2f

better grammar

2917c49

seawolf42 mentioned this pull request Aug 15, 2016

Revoke auth method #35

Open

seawolf42 added 3 commits August 18, 2016 10:10

remove file accidentally checked in

e78968b

Merge remote-tracking branch 'upstream/master'

52c73e5

remove support for python 2.6

2223169

seawolf42 added 2 commits November 26, 2016 10:33

flake8 compliance

b1d8876

bug fix

e1a05e8

martey requested changes Jan 6, 2017

View reviewed changes

martey removed the needs-documentation label Jan 6, 2017

martey added this to the v3.0.0 milestone Jan 6, 2017

remove **args parameter

ca15bf8

seawolf42 added 5 commits January 6, 2017 11:05

change to string format instead of concatenation

8ffedb9

Merge remote-tracking branch 'upstream/master'

a69a778

# Conflicts: # docs/api.rst # test/test_facebook.py

create test users rather than using hard-coded user ID

3c8911f

add args to request

c422097

working unit tests

b9e5e61

seawolf42 mentioned this pull request Jan 7, 2017

Add function for retrieving user permissions #342

Merged

seawolf42 closed this Jan 7, 2017

Add function for retrieving user permissions #264

Add function for retrieving user permissions #264

Conversation

seawolf42 commented Jan 12, 2016

seawolf42 commented Jan 13, 2016

martey commented Jan 13, 2016

seawolf42 commented Jan 13, 2016

seawolf42 commented Jan 13, 2016

martey commented Jan 13, 2016

seawolf42 commented Jan 13, 2016

martey commented Jan 14, 2016

seawolf42 commented Jan 14, 2016

seawolf42 commented Jan 14, 2016

martey commented Jan 14, 2016

seawolf42 commented Jan 14, 2016

seawolf42 commented Jan 21, 2016

martey commented Jan 21, 2016

seawolf42 commented Jan 21, 2016

seawolf42 commented Jan 31, 2016

seawolf42 commented Feb 28, 2016

martey commented Feb 28, 2016

seawolf42 commented Aug 15, 2016

seawolf42 commented Aug 15, 2016

seawolf42 commented Aug 15, 2016

seawolf42 commented Nov 24, 2016 • edited Loading

seawolf42 commented Nov 26, 2016

seawolf42 commented Dec 27, 2016

martey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seawolf42 Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

seawolf42 Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seawolf42 commented Jan 6, 2017

seawolf42 commented Jan 7, 2017

seawolf42 commented Jan 10, 2017

martey commented Jan 10, 2017

seawolf42 commented Jan 10, 2017 via email

seawolf42 commented Nov 24, 2016 •

edited

Loading

seawolf42 Jan 6, 2017 •

edited

Loading

seawolf42 Jan 6, 2017 •

edited

Loading