Issue 5500095: Create and Manage groups
|
Created:
12 years, 4 months ago by shraddhag Modified:
12 years, 2 months ago CC:
gdata-python-client-library-contributors_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 17
Patch Set 2 : addresesd the comments #
Total comments: 2
Patch Set 3 : updated file name #Patch Set 4 : corrected the link #Patch Set 5 : try to recover history #Patch Set 6 : Removing create_update_groups.py #
Total comments: 18
Patch Set 7 : Addressed comments #
Total comments: 6
Patch Set 8 : updated #MessagesTotal messages: 34
Please review Shraddha Gupta Developer Programs Engineer Hyderabad, Google India. On Fri, Dec 30, 2011 at 3:41 PM, <shraddhag@google.com> wrote: > http://codereview.appspot.com/**5500095/<http://codereview.appspot.com/5500095/> >
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py File create_manage_groups.py (right): http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode19 create_manage_groups.py:19: Sample to use the Groups Settings API in google-api-python client library Link to google-api-python-client http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode96 create_manage_groups.py:96: def CreateManageGroup(http, groups_client, domain): Bad function name, "Manage" doesn't really mean anything here. I would prefer something like "CreateAndUpdateGroup", or think of something better. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode108 create_manage_groups.py:108: email_permission = raw_input('Enter the email permission ') Improve these prompts, for example like: 'Enter the group id: ' http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode110 create_manage_groups.py:110: if '' in (group_id, group_name): if not (group_id and group_name): http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode111 create_manage_groups.py:111: print 'One or more Required fields missing: group_id, group_name' "required" http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode118 create_manage_groups.py:118: print 'Group Created' Put the group in the feedback message: http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode132 create_manage_groups.py:132: except gdata.client.RequestError, e: I am not sure that the exception handler is any better than just allowing the exception to raise, since you just show the traceback and exception. If you want to handle the exceptions, you need to give something useful back to the user. Otherwise just remove the try: block. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode162 create_manage_groups.py:162: domain = raw_input('Enter the domain ') Better prompt, as above.
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py File create_manage_groups.py (right): http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode19 create_manage_groups.py:19: Sample to use the Groups Settings API in google-api-python client library On 2012/01/02 23:25:37, Ali Afshar wrote: > Link to google-api-python-client Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode96 create_manage_groups.py:96: def CreateManageGroup(http, groups_client, domain): On 2012/01/02 23:25:37, Ali Afshar wrote: > Bad function name, "Manage" doesn't really mean anything here. I would prefer > something like "CreateAndUpdateGroup", or think of something better. Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode108 create_manage_groups.py:108: email_permission = raw_input('Enter the email permission ') On 2012/01/02 23:25:37, Ali Afshar wrote: > Improve these prompts, for example like: > > 'Enter the group id: ' Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode110 create_manage_groups.py:110: if '' in (group_id, group_name): On 2012/01/02 23:25:37, Ali Afshar wrote: > if not (group_id and group_name): Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode111 create_manage_groups.py:111: print 'One or more Required fields missing: group_id, group_name' On 2012/01/02 23:25:37, Ali Afshar wrote: > "required" Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode118 create_manage_groups.py:118: print 'Group Created' On 2012/01/02 23:25:37, Ali Afshar wrote: > Put the group in the feedback message: Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode118 create_manage_groups.py:118: print 'Group Created' On 2012/01/02 23:25:37, Ali Afshar wrote: > Put the group in the feedback message: Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode132 create_manage_groups.py:132: except gdata.client.RequestError, e: On 2012/01/02 23:25:37, Ali Afshar wrote: > I am not sure that the exception handler is any better than just allowing the > exception to raise, since you just show the traceback and exception. If you want > to handle the exceptions, you need to give something useful back to the user. > Otherwise just remove the try: block. Done. http://codereview.appspot.com/5500095/diff/1/create_manage_groups.py#newcode162 create_manage_groups.py:162: domain = raw_input('Enter the domain ') On 2012/01/02 23:25:37, Ali Afshar wrote: > Better prompt, as above. Done.
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/3001/create_manage_groups.py File create_manage_groups.py (right): http://codereview.appspot.com/5500095/diff/3001/create_manage_groups.py#newco... create_manage_groups.py:20: <http://code.google.com/p/google-api-java-client/source/checkout> This is a link to the java client.
Sign in to reply to this message.
On 2012/01/04 16:55:42, shraddhag wrote: Last patch is missing create_manage_groups.py
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/3001/create_manage_groups.py File create_manage_groups.py (right): http://codereview.appspot.com/5500095/diff/3001/create_manage_groups.py#newco... create_manage_groups.py:20: <http://code.google.com/p/google-api-java-client/source/checkout> Soory for the silly mistake. On 2012/01/04 16:55:00, Ali Afshar wrote: > This is a link to the java client.
Sign in to reply to this message.
I'm not sure what is going on in this issue, but it seems that all history on create_update_group.py is gone. In fact, I can't find any contextual comments from previous revisions... Are you sure you updated this issue correctly?
Sign in to reply to this message.
Ali had commented on the name of the method, so I renamed the method and also updated the name of the file accordingly. I uploaded the renamed file. I think that led to this. I am not sure if uploading the file with previous name can bring back the history. Thanks, Shraddha Gupta Developer Programs Engineer Hyderabad, Google India. On Wed, Jan 4, 2012 at 10:42 PM, <vicfryzel@google.com> wrote: > I'm not sure what is going on in this issue, but it seems that all > history on create_update_group.py is gone. In fact, I can't find any > contextual comments from previous revisions... Are you sure you updated > this issue correctly? > > http://codereview.appspot.com/**5500095/<http://codereview.appspot.com/5500095/> >
Sign in to reply to this message.
What is hg diff showing? Perhaps you should create a new issue. On Wed, Jan 4, 2012 at 5:25 PM, Shraddha Gupta <shraddhag@google.com> wrote: > Ali had commented on the name of the method, so I renamed the method and > also updated the name of the file accordingly. > I uploaded the renamed file. I think that led to this. > I am not sure if uploading the file with previous name can bring back the > history. > > > Thanks, > > Shraddha Gupta > Developer Programs Engineer > Hyderabad, Google India. > > > > On Wed, Jan 4, 2012 at 10:42 PM, <vicfryzel@google.com> wrote: >> >> I'm not sure what is going on in this issue, but it seems that all >> history on create_update_group.py is gone. In fact, I can't find any >> contextual comments from previous revisions... Are you sure you updated >> this issue correctly? >> >> http://codereview.appspot.com/5500095/ > > -- Ali Afshar | www.googplus.org/ali | Google Developer Relations
Sign in to reply to this message.
On 2012/01/04 18:40:37, Ali Afshar wrote: > What is hg diff showing? Perhaps you should create a new issue. > Actually, you should definitely figure out what went wrong with this issue and fix it.
Sign in to reply to this message.
On 2012/01/04 20:25:44, shraddhag wrote: Patch Set for create_manage_groups.py are available now. hg diff was not showing the history for the file. I uploaded the file create_manage_groups.py again and the patches became visible.
Sign in to reply to this message.
On 2012/01/04 20:32:50, shraddhag wrote: > On 2012/01/04 20:25:44, shraddhag wrote: > > Patch Set for create_manage_groups.py are available now. hg diff was not showing > the history for the file. I uploaded the file create_manage_groups.py again and > the patches became visible. Hi, it seems both files are now in the patchset, is this correct?
Sign in to reply to this message.
On 2012/01/06 21:45:44, Ali Afshar wrote: > On 2012/01/04 20:32:50, shraddhag wrote: > > On 2012/01/04 20:25:44, shraddhag wrote: > > > > Patch Set for create_manage_groups.py are available now. hg diff was not > showing > > the history for the file. I uploaded the file create_manage_groups.py again > and > > the patches became visible. > > Hi, it seems both files are now in the patchset, is this correct? Yes both files are present in patch set. They are same in content. Please review any of those. The previous patches for create_manage_groups.py can be seen in the patches 1 and 2
Sign in to reply to this message.
On 2012/01/07 09:43:01, shraddhag wrote: > On 2012/01/06 21:45:44, Ali Afshar wrote: > > On 2012/01/04 20:32:50, shraddhag wrote: > > > On 2012/01/04 20:25:44, shraddhag wrote: > > > > > > Patch Set for create_manage_groups.py are available now. hg diff was not > > showing > > > the history for the file. I uploaded the file create_manage_groups.py again > > and > > > the patches became visible. > > > > Hi, it seems both files are now in the patchset, is this correct? > > Yes both files are present in patch set. They are same in content. Please review > any of those. The previous patches for create_manage_groups.py can be seen in > the patches 1 and 2 Ok thanks, that makes sense. Can you remove one of them please?
Sign in to reply to this message.
On 2012/01/07 14:47:59, shraddhag wrote: please review.
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py File create_manage_group.py (right): http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:22: and update group's settings. s/to create group and update group's settings/to create a group and update its settings http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:77: def GetAuth2Token(client_id, client_secret, access_token, refresh_token): s/GetAuth2Token/GetOAuth2Token http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:78: """Get the OAuth 2.0 token for using with the Groups Provisioning API. s/for using with/to be used with http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:112: print 'One or more required fields missing: group_id, group_name' replace group_id and group_name with "group id" and "group name", those are the names you showed to the user http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:143: # the OAuth scope is 'https://www.googleapis.com/auth/calendar' this is not the scope you are using http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:151: # with our good Credentials. why the capital C? also, I'd call them "valid" instead of "good" http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:157: # Create an OAuth 2.0 token suitable for use with the Gdata client library s/Gdata/GData
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py File create_manage_group.py (right): http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:22: and update group's settings. On 2012/01/13 02:02:49, Claudio Cherubino wrote: > s/to create group and update group's settings/to create a group and update its > settings Done. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:77: def GetAuth2Token(client_id, client_secret, access_token, refresh_token): On 2012/01/13 02:02:49, Claudio Cherubino wrote: > s/GetAuth2Token/GetOAuth2Token Done. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:78: """Get the OAuth 2.0 token for using with the Groups Provisioning API. On 2012/01/13 02:02:49, Claudio Cherubino wrote: > s/for using with/to be used with Done. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:112: print 'One or more required fields missing: group_id, group_name' On 2012/01/13 02:02:49, Claudio Cherubino wrote: > replace group_id and group_name with "group id" and "group name", those are the > names you showed to the user Done. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:143: # the OAuth scope is 'https://www.googleapis.com/auth/calendar' On 2012/01/13 02:02:49, Claudio Cherubino wrote: > this is not the scope you are using Done. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:151: # with our good Credentials. On 2012/01/13 02:02:49, Claudio Cherubino wrote: > why the capital C? > > also, I'd call them "valid" instead of "good" Done. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:157: # Create an OAuth 2.0 token suitable for use with the Gdata client library On 2012/01/13 02:02:49, Claudio Cherubino wrote: > s/Gdata/GData Done.
Sign in to reply to this message.
Something is wrong here, you marked Claudio's comments as done but changed nothing. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py File create_manage_group.py (right): http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:139: print '%s\\nUsage: %s ARGS\\n%s' % (e, argv[0], FLAGS) Please tell me what is wrong with this line. Have you tested this line? http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:143: # the OAuth scope is 'https://www.googleapis.com/auth/calendar' On 2012/01/21 14:58:03, shraddhag wrote: > On 2012/01/13 02:02:49, Claudio Cherubino wrote: > > this is not the scope you are using > > Done. You marked Claudio's comment as Done, but I do not see any changes to this line... Please actually address Claudio's comment. http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:151: # with our good Credentials. On 2012/01/21 14:58:03, shraddhag wrote: > On 2012/01/13 02:02:49, Claudio Cherubino wrote: > > why the capital C? > > > > also, I'd call them "valid" instead of "good" > > Done. Again, you marked Claudio's comment as Done, but the uppercase C is still there.
Sign in to reply to this message.
On 2012/01/23 17:34:33, shraddhag wrote: I missed uploading the updated file. Sorry for that. I should checked the uploaded file before publishing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py File create_manage_group.py (right): http://codereview.appspot.com/5500095/diff/14001/create_manage_group.py#newco... create_manage_group.py:139: print '%s\\nUsage: %s ARGS\\n%s' % (e, argv[0], FLAGS) On 2012/01/22 06:39:50, Vic Fryzel wrote: > Please tell me what is wrong with this line. Have you tested this line? I am not using this code, I should have removed this as I am not taking input from my defined command line flags. I added this before to take additional command line arguments.
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py File create_manage_group.py (right): http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... create_manage_group.py:65: # Groups Provisioning and Group Settings APIs I'd recommend listing the two APIs in the same order of the scopes, i.e. Groups (notice the 's') Settings first and Groups Provisioning second http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... create_manage_group.py:69: # Set up a Flow object to be used if we need to authenticate. can we do anything in this sample without authentication? http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... create_manage_group.py:119: print new_group a few lines below you are pretty printing the group, why don't you do the same here?
Sign in to reply to this message.
Bump @Shraddha. Please address Claudio's comments when you get a chance :)
Sign in to reply to this message.
http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py File create_manage_group.py (right): http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... create_manage_group.py:65: # Groups Provisioning and Group Settings APIs On 2012/01/24 01:13:18, Claudio Cherubino wrote: > I'd recommend listing the two APIs in the same order of the scopes, i.e. Groups > (notice the 's') Settings first and Groups Provisioning second Done. http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... create_manage_group.py:69: # Set up a Flow object to be used if we need to authenticate. On 2012/01/24 01:13:18, Claudio Cherubino wrote: > can we do anything in this sample without authentication? Done. http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... create_manage_group.py:119: print new_group On 2012/01/24 01:13:18, Claudio Cherubino wrote: > a few lines below you are pretty printing the group, why don't you do the same > here? pprint is the method from the new library. I tried it on group object but it prints the object address not the property values. Can you tell how to use pprint with xml group object.
Sign in to reply to this message.
On 2012/02/07 06:58:50, shraddhag wrote: > http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py > File create_manage_group.py (right): > > http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... > create_manage_group.py:65: # Groups Provisioning and Group Settings APIs > On 2012/01/24 01:13:18, Claudio Cherubino wrote: > > I'd recommend listing the two APIs in the same order of the scopes, i.e. > Groups > > (notice the 's') Settings first and Groups Provisioning second > > Done. > > http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... > create_manage_group.py:69: # Set up a Flow object to be used if we need to > authenticate. > On 2012/01/24 01:13:18, Claudio Cherubino wrote: > > can we do anything in this sample without authentication? > > Done. > > http://codereview.appspot.com/5500095/diff/21001/create_manage_group.py#newco... > create_manage_group.py:119: print new_group > On 2012/01/24 01:13:18, Claudio Cherubino wrote: > > a few lines below you are pretty printing the group, why don't you do the same > > here? > > pprint is the method from the new library. I tried it on group object but it > prints the object address not the property values. Can you tell how to use > pprint with xml group object. LGTM
Sign in to reply to this message.
|
