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