Issue 4983056: Adding sample app for using provisiong api and email settings api via oauth2.0
|
Created:
12 years, 7 months ago by shraddhag Modified:
12 years, 7 months ago CC:
gdata-python-client-library-contributors_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 28
Patch Set 2 : Updated with changes #
Total comments: 58
Patch Set 3 : Reviewed #Patch Set 4 : corrected #
Total comments: 24
Patch Set 5 : Corrected #
Total comments: 16
Patch Set 6 : Updated #Patch Set 7 : Updated1 #
Total comments: 7
Patch Set 8 : Updated patch #
Total comments: 14
Patch Set 9 : Updated #
Total comments: 9
Patch Set 10 : Updated #MessagesTotal messages: 28
http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:5: import gdata.apps.emailsettings.client sort the imports, pylint can help you with that http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:7: API_VERSION = "2.0" use single quotes for strings http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:9: SCOPE = 'https://apps-apis.google.com/a/feeds/ https://apps-apis.google.com/a/feeds/emailsettings/2.0/' this line is too long, pylint also helps with this http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:13: """ OAuth2ClientSample object demos the use of OAuth2Token for Retrieving Members of a Group and performing Email Settings for them.""" why the capital R in Retrieving? Instead of "performing Email Settings" use "updating Email Settings" http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:16: """Constructor for the OAuth2ClientSampl object. s/OAuth2ClientSampl/OAuth2ClientSample http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:18: Takes domain, client_id, client_secret corresponding to a google apps admin I'm not sure client_id and client_secret belong to the admin, but instead they are related to the application. Am I wrong? http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:26: client_secret=client_secret, scope=SCOPE, move scope to a new line http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:30: print 'Please visit this URL to authorize the application:' this should go before the line where you print the URL http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:36: self.client = gdata.client.GDClient(host=HOST) leave a blank line before this one and add a comment to explain what is this client used for http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:41: self.email_client = gdata.apps.emailsettings.client.EmailSettingsClient(domain=self.domain, auth_token=self.token) long line http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:47: return gdata.apps.PropertyFeedFromString(str(self.client.GetFeed(uri=uri))) you can split this line in two http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:58: user_name, domain = property.value.split("@",1) add a whitespace after the , http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:79: help="The registered CLIENT_ID of domain.") s/CLIENT_ID/CLIENT_SECRET
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:1: from optparse import OptionParser remember to add the file header with author information and license
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:1: from optparse import OptionParser On 2011/09/10 23:50:58, Claudio Cherubino wrote: > remember to add the file header with author information and license Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:5: import gdata.apps.emailsettings.client Could not run pylint, sorted myself. Please review and give inputs On 2011/09/10 23:48:51, Claudio Cherubino wrote: > sort the imports, pylint can help you with that http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:7: API_VERSION = "2.0" On 2011/09/10 23:48:51, Claudio Cherubino wrote: > use single quotes for strings Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:9: SCOPE = 'https://apps-apis.google.com/a/feeds/ https://apps-apis.google.com/a/feeds/emailsettings/2.0/' On 2011/09/10 23:48:51, Claudio Cherubino wrote: > this line is too long, pylint also helps with this Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:13: """ OAuth2ClientSample object demos the use of OAuth2Token for Retrieving Members of a Group and performing Email Settings for them.""" On 2011/09/10 23:48:51, Claudio Cherubino wrote: > why the capital R in Retrieving? > > Instead of "performing Email Settings" use "updating Email Settings" Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:16: """Constructor for the OAuth2ClientSampl object. On 2011/09/10 23:48:51, Claudio Cherubino wrote: > s/OAuth2ClientSampl/OAuth2ClientSample Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:18: Takes domain, client_id, client_secret corresponding to a google apps admin Yes, I checked, they belong to application. On 2011/09/10 23:48:51, Claudio Cherubino wrote: > I'm not sure client_id and client_secret belong to the admin, but instead they > are related to the application. > Am I wrong? http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:26: client_secret=client_secret, scope=SCOPE, On 2011/09/10 23:48:51, Claudio Cherubino wrote: > move scope to a new line Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:30: print 'Please visit this URL to authorize the application:' On 2011/09/10 23:48:51, Claudio Cherubino wrote: > this should go before the line where you print the URL Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:36: self.client = gdata.client.GDClient(host=HOST) On 2011/09/10 23:48:51, Claudio Cherubino wrote: > leave a blank line before this one and add a comment to explain what is this > client used for Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:41: self.email_client = gdata.apps.emailsettings.client.EmailSettingsClient(domain=self.domain, auth_token=self.token) On 2011/09/10 23:48:51, Claudio Cherubino wrote: > long line Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:47: return gdata.apps.PropertyFeedFromString(str(self.client.GetFeed(uri=uri))) On 2011/09/10 23:48:51, Claudio Cherubino wrote: > you can split this line in two Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:58: user_name, domain = property.value.split("@",1) On 2011/09/10 23:48:51, Claudio Cherubino wrote: > add a whitespace after the , Done. http://codereview.appspot.com/4983056/diff/1/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:79: help="The registered CLIENT_ID of domain.") On 2011/09/10 23:48:51, Claudio Cherubino wrote: > s/CLIENT_ID/CLIENT_SECRET Done.
Sign in to reply to this message.
Upload the updated code with the upload-diffs.py script (remember to pass the correct issue number) so that we can check the new code
Sign in to reply to this message.
Vic, can you check this CL? I'm about to takeoff, see you from India ;) Claudio On Mon, Sep 12, 2011 at 3:02 PM, <shraddhag@google.com> wrote: > http://codereview.appspot.com/**4983056/<http://codereview.appspot.com/4983056/> >
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:17: """A sample app for Google Apps Provisioning and Email Settings features using OAuth2.0. > 80 chars http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:19: OAuth2ClientSample: demonstrates retrieving users using Provisioning API and setting filters using Email Settings API > 80 chars http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:22: __author__ = 'Shraddha Gupta <shraddhag@google.com>' Add blank line. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:28: Two blank lines. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:29: API_VERSION = 2.0' Syntax error. API_VERSIOn = '2.0' ? http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:34: Two blank lines. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:35: class OAuth2ClientSample(): Should be a subclass of object. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:38: Trailing whitespace. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:40: """Constructor for the OAuth2ClientSample object. Unnecessary description, __init__ is always the constructor. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:42: Takes domain, client_id, client_secret corresponding to a google apps admin These are described in Args. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:46: client_id: [string] The client_id of a domain admin account got on registering. > 80 characters. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:47: client_secret: [string] The client_secret of a domain admin account got on registering. > 80 characters. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:50: client_secret=client_secret, scope=SCOPE, Not aligned correctly with parameter above. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:51: user_agent='create-filters') Not aligned correctly with parameter above. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:51: user_agent='create-filters') Use a better User Agent that indicates the sample name and version. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:57: self.token.get_access_token(code) What if the token is invalid? How would you catch this and communicate with the user? http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:62: # Authorize the client. Add some small info like, "this will add the Authorization header to all future requests." I think this is important because the example is explicitly showing how to use OAuth2. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:65: self.email_client = gdata.apps.emailsettings.client.EmailSettingsClient(domain=self.domain, auth_token=self.token) > 80 characters. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:67: Trailing whitespace. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:73: print 'Exception %s' % e Do you want to give the user a better message here? http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:74: Trailing whitespace. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:77: member = {'memberType' : '' , 'memberId' : ''} Whitespace like {'memberType': '', http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:77: member = {'memberType' : '' , 'memberId' : ''} Not exactly sure why you use a dict here to store the values instead of local variables. Also it is traditional to set to None unless you are expecting a string later. memberType = None memberId = None <for loop> memberType = property.value memberId = user_name # etc then if memberType == 'User' and memberId: etc. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:83: if domain == property.value: #Check that the member belongs to your primary domain Put the comment on its own line, and make sure it is < 80 chars. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:87: self.email_client.CreateFilter(user_name, does_not_have_the_word = self.domain, mark_as_read=True) remove the spaces around ' = self.domain'. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:87: self.email_client.CreateFilter(user_name, does_not_have_the_word = self.domain, mark_as_read=True) Line > 80 characters. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:88: Trailing whitespace. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:92: Trailing whitespace. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:108: if (options.DOMAIN is None or options.CLIENT_ID is None or options.CLIENT_SECRET == None or Why == None, None is a singleton, so "is None" is always the canonical way to test. However, this line could be replaced by using OptionParser's required=True for add_option for all options as they are all required. http://docs.python.org/library/optparse.html
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:17: """A sample app for Google Apps Provisioning and Email Settings features using OAuth2.0. On 2011/09/13 14:14:48, Ali Afshar wrote: > > 80 chars Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:19: OAuth2ClientSample: demonstrates retrieving users using Provisioning API and setting filters using Email Settings API On 2011/09/13 14:14:48, Ali Afshar wrote: > > 80 chars Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:22: __author__ = 'Shraddha Gupta <shraddhag@google.com>' On 2011/09/13 14:14:48, Ali Afshar wrote: > Add blank line. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:28: On 2011/09/13 14:14:48, Ali Afshar wrote: > Two blank lines. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:29: API_VERSION = 2.0' On 2011/09/13 14:14:48, Ali Afshar wrote: > Syntax error. API_VERSIOn = '2.0' ? Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:34: On 2011/09/13 14:14:48, Ali Afshar wrote: > Two blank lines. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:35: class OAuth2ClientSample(): On 2011/09/13 14:14:48, Ali Afshar wrote: > Should be a subclass of object. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:38: On 2011/09/13 14:14:48, Ali Afshar wrote: > Trailing whitespace. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:40: """Constructor for the OAuth2ClientSample object. On 2011/09/13 14:14:48, Ali Afshar wrote: > Unnecessary description, __init__ is always the constructor. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:42: Takes domain, client_id, client_secret corresponding to a google apps admin On 2011/09/13 14:14:48, Ali Afshar wrote: > These are described in Args. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:46: client_id: [string] The client_id of a domain admin account got on registering. On 2011/09/13 14:14:48, Ali Afshar wrote: > > 80 characters. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:47: client_secret: [string] The client_secret of a domain admin account got on registering. On 2011/09/13 14:14:48, Ali Afshar wrote: > > 80 characters. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:50: client_secret=client_secret, scope=SCOPE, On 2011/09/13 14:14:48, Ali Afshar wrote: > Not aligned correctly with parameter above. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:51: user_agent='create-filters') On 2011/09/13 14:14:48, Ali Afshar wrote: > Not aligned correctly with parameter above. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:51: user_agent='create-filters') On 2011/09/13 14:14:48, Ali Afshar wrote: > Use a better User Agent that indicates the sample name and version. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:57: self.token.get_access_token(code) On 2011/09/13 14:14:48, Ali Afshar wrote: > What if the token is invalid? How would you catch this and communicate with the > user? Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:62: # Authorize the client. On 2011/09/13 14:14:48, Ali Afshar wrote: > Add some small info like, "this will add the Authorization header to all future > requests." I think this is important because the example is explicitly showing > how to use OAuth2. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:65: self.email_client = gdata.apps.emailsettings.client.EmailSettingsClient(domain=self.domain, auth_token=self.token) On 2011/09/13 14:14:48, Ali Afshar wrote: > > 80 characters. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:67: On 2011/09/13 14:14:48, Ali Afshar wrote: > Trailing whitespace. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:73: print 'Exception %s' % e I am printing the exception 'e'. which gives pretty good information. The response code,tells if the token is outside the scope On 2011/09/13 14:14:48, Ali Afshar wrote: > Do you want to give the user a better message here? http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:74: On 2011/09/13 14:14:48, Ali Afshar wrote: > Trailing whitespace. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:77: member = {'memberType' : '' , 'memberId' : ''} On 2011/09/13 14:14:48, Ali Afshar wrote: > Not exactly sure why you use a dict here to store the values instead of local > variables. Also it is traditional to set to None unless you are expecting a > string later. > > memberType = None > memberId = None > > <for loop> > > memberType = property.value > memberId = user_name # etc > > then > > if memberType == 'User' and memberId: > > etc. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:77: member = {'memberType' : '' , 'memberId' : ''} On 2011/09/13 14:14:48, Ali Afshar wrote: > Whitespace like {'memberType': '', Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:83: if domain == property.value: #Check that the member belongs to your primary domain On 2011/09/13 14:14:48, Ali Afshar wrote: > Put the comment on its own line, and make sure it is < 80 chars. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:87: self.email_client.CreateFilter(user_name, does_not_have_the_word = self.domain, mark_as_read=True) On 2011/09/13 14:14:48, Ali Afshar wrote: > remove the spaces around ' = self.domain'. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:87: self.email_client.CreateFilter(user_name, does_not_have_the_word = self.domain, mark_as_read=True) On 2011/09/13 14:14:48, Ali Afshar wrote: > Line > 80 characters. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:88: On 2011/09/13 14:14:48, Ali Afshar wrote: > Trailing whitespace. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:92: On 2011/09/13 14:14:48, Ali Afshar wrote: > Trailing whitespace. Done. http://codereview.appspot.com/4983056/diff/3/samples/apps/provisioning_oauth2... samples/apps/provisioning_oauth2_example.py:108: if (options.DOMAIN is None or options.CLIENT_ID is None or options.CLIENT_SECRET == None or I could not find how to use required for parser. And I want to print help if any argument is missing On 2011/09/13 14:14:48, Ali Afshar wrote: > Why == None, None is a singleton, so "is None" is always the canonical way to > test. > > However, this line could be replaced by using OptionParser's required=True for > add_option for all options as they are all required. > http://docs.python.org/library/optparse.html
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """A sample app for Google Apps Provisioning and Email Settings features This first sentence should be a sentence that fits on one line and is < 80 characters total. On the rest of this comment, do not indent. Example: """This module does something important. Application X needs some specific stuff provided by this module, and only this module can provide it. """ http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:26: from optparse import OptionParser Your imports are out of order, see our readability guide. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:32: 2 blank lines go between imports and the rest of the code http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:36: https://apps-apis.google.com/a/feeds/emailsettings/2.0/' SCOPE = ('https://apps-apis.google.com/a/feeds/ ' 'https://apps-apis.google.com/a/feeds/emailsettings/2.0/') http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:47: domain: [string] The domain name (e.g. domain.com) Remove square brackets [ and ] http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:73: domain=self.domain, auth_token=self.token) indentation is incorrect, 4 spaces http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:76: def get_users(self, group): add pydoc comments http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:79: return gdata.apps.PropertyFeedFromString(str(self.client.GetFeed(uri=uri))) longer than 80 chars http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:83: def create_filter(self, feed): Add pydoc comments http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:111: """Demos the Provisioning API along with Email Settings API and authorization done via Auth2.0 .""" Longer than 80 chars. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:124: if (options.DOMAIN is None or options.CLIENT_ID is None or options.CLIENT_SECRET is None or Replace this with: if None in (options.DOMAIN, options.CLIENT_ID, options.CLIENT_SECRET, options.GROUP): http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:124: if (options.DOMAIN is None or options.CLIENT_ID is None or options.CLIENT_SECRET is None or Longer than 80 chars http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:129: sample = OAuth2ClientSample(options.DOMAIN, options.CLIENT_ID, options.CLIENT_SECRET) Longer than 80 chars
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """A sample app for Google Apps Provisioning and Email Settings features On 2011/09/13 19:07:33, Vic Fryzel wrote: > This first sentence should be a sentence that fits on one line and is < 80 > characters total. > > On the rest of this comment, do not indent. > > Example: > """This module does something important. > > Application X needs some specific stuff provided by this > module, and only this module can provide it. > """ Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:26: from optparse import OptionParser On 2011/09/13 19:07:33, Vic Fryzel wrote: > Your imports are out of order, see our readability guide. Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:32: On 2011/09/13 19:07:33, Vic Fryzel wrote: > 2 blank lines go between imports and the rest of the code Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:36: https://apps-apis.google.com/a/feeds/emailsettings/2.0/' On 2011/09/13 19:07:33, Vic Fryzel wrote: > SCOPE = ('https://apps-apis.google.com/a/feeds/ ' > 'https://apps-apis.google.com/a/feeds/emailsettings/2.0/') Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:47: domain: [string] The domain name (e.g. domain.com) On 2011/09/13 19:07:33, Vic Fryzel wrote: > Remove square brackets [ and ] Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:73: domain=self.domain, auth_token=self.token) On 2011/09/13 19:07:33, Vic Fryzel wrote: > indentation is incorrect, 4 spaces Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:76: def get_users(self, group): On 2011/09/13 19:07:33, Vic Fryzel wrote: > add pydoc comments Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:79: return gdata.apps.PropertyFeedFromString(str(self.client.GetFeed(uri=uri))) On 2011/09/13 19:07:33, Vic Fryzel wrote: > longer than 80 chars Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:83: def create_filter(self, feed): On 2011/09/13 19:07:33, Vic Fryzel wrote: > Add pydoc comments Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:111: """Demos the Provisioning API along with Email Settings API and authorization done via Auth2.0 .""" On 2011/09/13 19:07:33, Vic Fryzel wrote: > Longer than 80 chars. Done. http://codereview.appspot.com/4983056/diff/14001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:129: sample = OAuth2ClientSample(options.DOMAIN, options.CLIENT_ID, options.CLIENT_SECRET) On 2011/09/13 19:07:33, Vic Fryzel wrote: > Longer than 80 chars Done.
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:52: client_secret=client_secret, are those tabs? http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:79: group: string The group id of the group s/The group id of the group/The id of the group http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:89: """Method that updates emailsettings for member feed from the group This is not very clear, say that the method creates a mail filter for each member of the group
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """A sample app for Google Apps Provisioning and Email Settings using OAuth2.0. s/A sample/Sample/, s/OAuth2.0/OAuth 2.0/ http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:20: and setting filters using Email Settings API add a . http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:69: #This will add the Authorization header to all future requests. Space after # http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:105: if memberType == 'User' and domain == self.domain: Space after # http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:121: """Demos the Provisioning API and Email Settings API with Auth2.0 .""" s/Auth2.0 /OAuth 2.0/
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """A sample app for Google Apps Provisioning and Email Settings using OAuth2.0. On 2011/09/23 20:50:38, Vic Fryzel wrote: > s/A sample/Sample/, s/OAuth2.0/OAuth 2.0/ Done. http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:20: and setting filters using Email Settings API On 2011/09/23 20:50:38, Vic Fryzel wrote: > add a . Done. http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:52: client_secret=client_secret, On 2011/09/23 20:31:16, Claudio Cherubino wrote: > are those tabs? Done. http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:69: #This will add the Authorization header to all future requests. On 2011/09/23 20:50:38, Vic Fryzel wrote: > Space after # Done. http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:79: group: string The group id of the group On 2011/09/23 20:31:16, Claudio Cherubino wrote: > s/The group id of the group/The id of the group Done. http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:89: """Method that updates emailsettings for member feed from the group On 2011/09/23 20:31:16, Claudio Cherubino wrote: > This is not very clear, say that the method creates a mail filter for each > member of the group Done. http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:105: if memberType == 'User' and domain == self.domain: On 2011/09/23 20:50:38, Vic Fryzel wrote: > Space after # Done. http://codereview.appspot.com/4983056/diff/11002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:121: """Demos the Provisioning API and Email Settings API with Auth2.0 .""" On 2011/09/23 20:50:38, Vic Fryzel wrote: > s/Auth2.0 /OAuth 2.0/ Done.
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:41: Members of a Group and performing Email Settings for them.""" s/performing/updating http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:57: print 'Please visit this URL to authorize the application:' this line should go before the one where you actually print the url http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:89: """Method that creates a mail filter for each member of the group since it is hard-coded, add a description of the filter being created, i.e. a filter that marks as read all messages not containing the domain name as one of their words. http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:104: # Check that the member belongs to your primary domain and a User s/Check that the member belongs to your primary domain and a User/Check that the member is a User belonging to the primary domain http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:121: """Demos the Provisioning API and Email Settings API with Auth 2.0 .""" s/Auth 2.0/OAuth 2.0 http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:129: help='The registered CLIENT_ID of domain.') s/CLIENT_ID/CLIENT_SECRET http://codereview.appspot.com/4983056/diff/27001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:131: help='The group id for the group') s/The group id for the group/The group identifier
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """Sample app for Google Apps Provisioning and Email Settings using OAuth 2.0. Remove this line, and use the one below. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:19: OAuth2ClientSample: demonstrates retrieving users using Provisioning API s/demonstrates/Demonstrates/, s/using/using the/ http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:20: and setting filters using Email Settings API. s/using/using the/ http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:46: domain: string The domain name (e.g. domain.com) s/The d/D/, here and everywhere http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:76: """Method that retrieves members from the group Retrieves members from the given group. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:79: group: string The id of the group Returns: http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:89: """Method that creates a mail filter that marks as read all messages not containing the domain name as one of their words for each member of the group. Creates a mail filter... 80 chars
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """Sample app for Google Apps Provisioning and Email Settings using OAuth 2.0. On 2011/09/26 07:34:07, Vic Fryzel wrote: > Remove this line, and use the one below. Done. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:19: OAuth2ClientSample: demonstrates retrieving users using Provisioning API On 2011/09/26 07:34:07, Vic Fryzel wrote: > s/demonstrates/Demonstrates/, s/using/using the/ Done. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:20: and setting filters using Email Settings API. On 2011/09/26 07:34:07, Vic Fryzel wrote: > s/using/using the/ Done. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:46: domain: string The domain name (e.g. domain.com) On 2011/09/26 07:34:07, Vic Fryzel wrote: > s/The d/D/, here and everywhere Done. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:76: """Method that retrieves members from the group On 2011/09/26 07:34:07, Vic Fryzel wrote: > Retrieves members from the given group. Done. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:79: group: string The id of the group On 2011/09/26 07:34:07, Vic Fryzel wrote: > Returns: Done. http://codereview.appspot.com/4983056/diff/22002/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:89: """Method that creates a mail filter that marks as read all messages not containing the domain name as one of their words for each member of the group. On 2011/09/26 07:34:07, Vic Fryzel wrote: > Creates a mail filter... > > 80 chars Done.
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """OAuth2ClientSample: Demonstrates retrieving users using the Provisioning API The first line of a pydoc must be <= 80 chars, and describe the program. You'll have to reword this to make it shorter, and remove the line below. http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:45: client_id: string The client_id of domain admin account. Don't start param comments with articles of speech, e.g. "The". Fix this everywhere in the program. This would read: client_id: string Client ID of domain admin account. http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:80: The member feed for the given group No The http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:127: help='The Google Apps Domain, e.g. "domain.com".') s/The //everywhere
Sign in to reply to this message.
http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... File samples/apps/provisioning_oauth2_example.py (right): http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:17: """OAuth2ClientSample: Demonstrates retrieving users using the Provisioning API On 2011/09/27 08:08:15, Vic Fryzel wrote: > The first line of a pydoc must be <= 80 chars, and describe the program. You'll > have to reword this to make it shorter, and remove the line below. Done. http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:45: client_id: string The client_id of domain admin account. On 2011/09/27 08:08:15, Vic Fryzel wrote: > Don't start param comments with articles of speech, e.g. "The". Fix this > everywhere in the program. > > This would read: > client_id: string Client ID of domain admin account. Done. http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:45: client_id: string The client_id of domain admin account. On 2011/09/27 08:08:15, Vic Fryzel wrote: > Don't start param comments with articles of speech, e.g. "The". Fix this > everywhere in the program. > > This would read: > client_id: string Client ID of domain admin account. Done. http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:80: The member feed for the given group On 2011/09/27 08:08:15, Vic Fryzel wrote: > No The Done. http://codereview.appspot.com/4983056/diff/35001/samples/apps/provisioning_oa... samples/apps/provisioning_oauth2_example.py:127: help='The Google Apps Domain, e.g. "domain.com".') On 2011/09/27 08:08:15, Vic Fryzel wrote: > s/The //everywhere Done.
Sign in to reply to this message.
|
