Added client, data, live tests and data tests for organization unit provisioning

[shraddhag, 2011-11-23 11:26:29]

http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:244: customer_id: The ID of the Google Apps customer. customer_id: type description Please make this change in all arguments. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:248: users_to_move: Email addresses list of users to move. list Email address .... http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:256: org_unit_entry.SetUsersToMove(', '.join(users_to_move)) I think you can access the property as org_unit_entry.move_users = ','.join() http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:257: print org_unit_entry.GetParentOrgUnitPath() Remove this print statement http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:259: org_unit_entry.SetParentOrgUnitPath('/') try same access for this property too, org_unit_entry.parent_org_unit_path = '/' http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:295: correponding to each orgunit. Isn't that OrgUnitFeed that is returned. Feed is defined as a list of entries http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:297: entry = [] better name would be feed or entry_list http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:326: RetrieveFeedFromUri = retrieve_feed_from_uri Keep this method above retrieve_All_org_units() http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:339: if startKey is not None: if startKey: should be enough for checking its not None http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:360: A list containing a gdata.apps.organization.data.OrgUnitEntry returns: OrgUnitFeed http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:392: return self.delete(self.MakeOrganizationUnitOrgunitProvisioningUri( @Claudio: Should the delete method return anything. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:415: old_user_entry.SetOldOrgUnitPath(old_org_unit_path) Try the same way for accessing property old_user_entry.OldOrgUnitPath = old_org_unit_path http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:448: A list containing a gdata.apps.organization.data.OrgUserEntry Feed http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:476: A gdata.data.GDFeed of the OrgUserEntry gdata.apps.organisation.data.OrgUserFeed http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/data.py File src/gdata/apps/organization/data.py (right): http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/data... src/gdata/apps/organization/data.py:270: value: [string] The comma seperated list of users to move list : it is a list of strings http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/data... src/gdata/apps/organization/data.py:275: move_users = pyproperty(GetMovedUsers, SetUsersToMove) users_to_move might be better http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/data... src/gdata/apps/organization/data.py:281: I am not sure, if you should give move_users as one of the parameters as it is never set while making the instance http://codereview.appspot.com/5436044/diff/1/tests/gdata_tests/apps/organizat... File tests/gdata_tests/apps/organization/data_test.py (right): http://codereview.appspot.com/5436044/diff/1/tests/gdata_tests/apps/organizat... tests/gdata_tests/apps/organization/data_test.py:61: self.assertEquals(self.entry.org_unit_block_inheritance, 'false') You have not tested users_to_move and moved_users attribute http://codereview.appspot.com/5436044/diff/1/tests/gdata_tests/apps/organizat... File tests/gdata_tests/apps/organization/live_client_test.py (right): http://codereview.appspot.com/5436044/diff/1/tests/gdata_tests/apps/organizat... tests/gdata_tests/apps/organization/live_client_test.py:123: self.assertEquals(entry.org_unit_block_inheritance, 'false') test for update orgunit tests for orgunit users

[gunjansharma, 2011-11-23 12:31:59]

http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:244: customer_id: The ID of the Google Apps customer. On 2011/11/23 11:26:30, shraddhag wrote: > customer_id: type description > Please make this change in all arguments. Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:248: users_to_move: Email addresses list of users to move. On 2011/11/23 11:26:30, shraddhag wrote: > list Email address .... Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:256: org_unit_entry.SetUsersToMove(', '.join(users_to_move)) On 2011/11/23 11:26:30, shraddhag wrote: > I think you can access the property as > org_unit_entry.move_users = ','.join() Used function call instead. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:257: print org_unit_entry.GetParentOrgUnitPath() On 2011/11/23 11:26:30, shraddhag wrote: > Remove this print statement Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:259: org_unit_entry.SetParentOrgUnitPath('/') On 2011/11/23 11:26:30, shraddhag wrote: > try same access for this property too, > org_unit_entry.parent_org_unit_path = '/' Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:295: correponding to each orgunit. On 2011/11/23 11:26:30, shraddhag wrote: > Isn't that OrgUnitFeed that is returned. Feed is defined as a list of entries I am making a list out of all the feeds and returning it. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:297: entry = [] On 2011/11/23 11:26:30, shraddhag wrote: > better name would be feed or entry_list Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:326: RetrieveFeedFromUri = retrieve_feed_from_uri On 2011/11/23 11:26:30, shraddhag wrote: > Keep this method above retrieve_All_org_units() Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:339: if startKey is not None: On 2011/11/23 11:26:30, shraddhag wrote: > if startKey: should be enough for checking its not None So that means that he has not specified it. None is the value it will take by default. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:360: A list containing a gdata.apps.organization.data.OrgUnitEntry On 2011/11/23 11:26:30, shraddhag wrote: > returns: OrgUnitFeed List of entries http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:415: old_user_entry.SetOldOrgUnitPath(old_org_unit_path) On 2011/11/23 11:26:30, shraddhag wrote: > Try the same way for accessing property old_user_entry.OldOrgUnitPath = > old_org_unit_path Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:448: A list containing a gdata.apps.organization.data.OrgUserEntry On 2011/11/23 11:26:30, shraddhag wrote: > Feed Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:476: A gdata.data.GDFeed of the OrgUserEntry On 2011/11/23 11:26:30, shraddhag wrote: > gdata.apps.organisation.data.OrgUserFeed Done.

[Claudio Cherubino, 2011-11-23 17:47:35]

You still have to address some of Shraddha's comments. Also, glint reports a lot of errors, please fix them. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:360: A list containing a gdata.apps.organization.data.OrgUnitEntry On 2011/11/23 12:31:59, gunjansharma wrote: > On 2011/11/23 11:26:30, shraddhag wrote: > > returns: OrgUnitFeed > List of entries this is confusing, some methods (like this) return a list of OrgUnitEntry while others (like retrieve_page_of_org_units) return a OrgUnitFeed. The library should be consistent. Aren't sub org units the same kind of object as org units? This also forces you to have all these temp feeds you are using. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:392: return self.delete(self.MakeOrganizationUnitOrgunitProvisioningUri( On 2011/11/23 11:26:30, shraddhag wrote: > @Claudio: Should the delete method return anything. the base delete method has a return statement, so you can return whatever comes from it. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:448: A list containing a gdata.apps.organization.data.OrgUserEntry On 2011/11/23 12:31:59, gunjansharma wrote: > On 2011/11/23 11:26:30, shraddhag wrote: > > Feed > > Done. I don't see it. http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/c... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:286: def retrieve_feed_from_uri(self, uri, desired_class, **kwargs): I don't understand this method. Don't we have a method called GetFeed? Why are you using GetEntry to retrieve a feed? If this does something special, please use a comment to explain. If instead this method can be useful in other clients, please move it to the base class. http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/d... File src/gdata/apps/organization/data.py (right): http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/d... src/gdata/apps/organization/data.py:64: def _GetProperty(self, name): _GetProperty and _SetProperty are the same two methods we have in other classes that extend GDEntry, please move them to the base class. If not all classes extending GDEntry use them, please extend GDEntry with another class called AppsPropertyEntry (or any better name) and have this (and all other object using _GetProperty and _SetProperty) extend AppsPropertyEntry instead.

[gunjansharma, 2011-11-24 09:19:16]

http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:360: A list containing a gdata.apps.organization.data.OrgUnitEntry On 2011/11/23 17:47:35, Claudio Cherubino wrote: > On 2011/11/23 12:31:59, gunjansharma wrote: > > On 2011/11/23 11:26:30, shraddhag wrote: > > > returns: OrgUnitFeed > > List of entries > > this is confusing, some methods (like this) return a list of OrgUnitEntry while > others (like retrieve_page_of_org_units) return a OrgUnitFeed. The library > should be consistent. Aren't sub org units the same kind of object as org units? > > This also forces you to have all these temp feeds you are using. I am doing this because at a time only 100 orgUnits will be retrieved. So I call recursively and make a list of all the entry. I do the same in subOrgUnits. Sun org units are the same kind as orgUnits as you said but what exactly about them? I am not returning a feed here because It makes less sense to me, because a feed have a lot of other options like next_link and all. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:392: return self.delete(self.MakeOrganizationUnitOrgunitProvisioningUri( On 2011/11/23 17:47:35, Claudio Cherubino wrote: > On 2011/11/23 11:26:30, shraddhag wrote: > > @Claudio: Should the delete method return anything. > > the base delete method has a return statement, so you can return whatever comes > from it. Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:392: return self.delete(self.MakeOrganizationUnitOrgunitProvisioningUri( On 2011/11/23 11:26:30, shraddhag wrote: > @Claudio: Should the delete method return anything. Done. http://codereview.appspot.com/5436044/diff/1/src/gdata/apps/organization/clie... src/gdata/apps/organization/client.py:448: A list containing a gdata.apps.organization.data.OrgUserEntry On 2011/11/23 17:47:35, Claudio Cherubino wrote: > On 2011/11/23 12:31:59, gunjansharma wrote: > > On 2011/11/23 11:26:30, shraddhag wrote: > > > Feed > > > > Done. > > I don't see it. It is a list of OrgUnitEntry and not org unit feed http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/c... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:286: def retrieve_feed_from_uri(self, uri, desired_class, **kwargs): On 2011/11/23 17:47:35, Claudio Cherubino wrote: > I don't understand this method. Don't we have a method called GetFeed? Why are > you using GetEntry to retrieve a feed? > > If this does something special, please use a comment to explain. If instead this > method can be useful in other clients, please move it to the base class. Done.

[gunjansharma, 2011-11-24 09:45:41]

http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/d... File src/gdata/apps/organization/data.py (right): http://codereview.appspot.com/5436044/diff/1003/src/gdata/apps/organization/d... src/gdata/apps/organization/data.py:64: def _GetProperty(self, name): On 2011/11/23 17:47:35, Claudio Cherubino wrote: > _GetProperty and _SetProperty are the same two methods we have in other classes > that extend GDEntry, please move them to the base class. > > If not all classes extending GDEntry use them, please extend GDEntry with > another class called AppsPropertyEntry (or any better name) and have this (and > all other object using _GetProperty and _SetProperty) extend AppsPropertyEntry > instead. Done.

[Claudio Cherubino, 2011-11-25 20:26:39]

http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/apps_property_... File src/gdata/apps/apps_property_entry.py (right): http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/apps_property_... src/gdata/apps/apps_property_entry.py:28: class AppsPropertyEntry(gdata.data.GDEntry): Can you also make the MDM client use this class instead? http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/apps_property_... src/gdata/apps/apps_property_entry.py:29: """Represents a Organization Unit Provisioning entry in object form.""" this should be generic, not OU-specific http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:125: A string giving the URI for organization unit orgunit's provisioning for can you please check this comment? It is not very clear what this method returns http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:320: return entries_list I still don't like that some methods return an OrgUnitFeed and others a list of OrgUserEntry, let's discuss the issue in NY and agree on a solution. Anyway, this code you wrote to populate a list is repeated in at least another method, please extract it into a separate method. http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:446: correponding to each orguser. replace all "correponding" with "corresponding" http://codereview.appspot.com/5436044/diff/7002/tests/gdata_tests/apps/organi... File tests/gdata_tests/apps/organization/live_client_test.py (right): http://codereview.appspot.com/5436044/diff/7002/tests/gdata_tests/apps/organi... tests/gdata_tests/apps/organization/live_client_test.py:41: self.client = gdata.apps.organization.client.OrganizationUnitProvisioningClient( long line?

[gunjansharma, 2011-12-01 18:30:42]

http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/apps_property_... File src/gdata/apps/apps_property_entry.py (right): http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/apps_property_... src/gdata/apps/apps_property_entry.py:28: class AppsPropertyEntry(gdata.data.GDEntry): On 2011/11/25 20:26:39, Claudio Cherubino wrote: > Can you also make the MDM client use this class instead? Done. http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/apps_property_... src/gdata/apps/apps_property_entry.py:29: """Represents a Organization Unit Provisioning entry in object form.""" On 2011/11/25 20:26:39, Claudio Cherubino wrote: > this should be generic, not OU-specific Done. http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... File src/gdata/apps/organization/client.py (right): http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:125: A string giving the URI for organization unit orgunit's provisioning for On 2011/11/25 20:26:39, Claudio Cherubino wrote: > can you please check this comment? It is not very clear what this method returns Done. http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:320: return entries_list On 2011/11/25 20:26:39, Claudio Cherubino wrote: > I still don't like that some methods return an OrgUnitFeed and others a list of > OrgUserEntry, let's discuss the issue in NY and agree on a solution. > > Anyway, this code you wrote to populate a list is repeated in at least another > method, please extract it into a separate method. Done. http://codereview.appspot.com/5436044/diff/7002/src/gdata/apps/organization/c... src/gdata/apps/organization/client.py:446: correponding to each orguser. On 2011/11/25 20:26:39, Claudio Cherubino wrote: > replace all "correponding" with "corresponding" Done. http://codereview.appspot.com/5436044/diff/7002/tests/gdata_tests/apps/organi... File tests/gdata_tests/apps/organization/live_client_test.py (right): http://codereview.appspot.com/5436044/diff/7002/tests/gdata_tests/apps/organi... tests/gdata_tests/apps/organization/live_client_test.py:41: self.client = gdata.apps.organization.client.OrganizationUnitProvisioningClient( On 2011/11/25 20:26:39, Claudio Cherubino wrote: > long line? Can't help it. How to break?

[Claudio Cherubino, 2011-12-01 18:43:11]

LGTM