Completing Groups support for OpenSocial 1.0

Adding support for create, retrieve update and delete of groups to the Social Data, Social API Server and Social Gadget specs.

[Lane LiaBraaten, 2009-12-16 02:34:19]

Hi Dave,

Thanks for putting this patch together. Mostly minor comments, but a couple for
the group...

1) Should the groups service be required or optional

2) should we standardize on create requests for all services returning the
newly-created object (complete w/ container-assigned ID)?  Same question for
update/delete.

http://codereview.appspot.com/179060/diff/1/3
File draft/Social-API-Server.xml (right):

http://codereview.appspot.com/179060/diff/1/3#newcode527
draft/Social-API-Server.xml:527: <t>Containers MUST support the Groups Service.
Individual operations are required or optional as indicated in the sections that
follow. Containers MUST use the following values to define the Groups
Service:</t>
I think groups should be a MAY.

http://codereview.appspot.com/179060/diff/1/3#newcode549
draft/Social-API-Server.xml:549: <eref
target="Core-API-Server.xml#Return-Object">Return-Object</eref>          = null
container should return the group object that is created.  This is useful in the
following flow.

1) Client sends request with title and optionally description
2) The container provisions the group and returns the full group object (i.e.
with the id property)
3) the client can then request the group later, now that it has the ID

http://codereview.appspot.com/179060/diff/1/3#newcode560
draft/Social-API-Server.xml:560: <c>Group</c>
should be lowercase 'group'

http://codereview.appspot.com/179060/diff/1/3#newcode562
draft/Social-API-Server.xml:562: <c>Group object specifying group to be
created.</c>
may want to mention that the id property should not be included in the request
and may be ignored by the container.

http://codereview.appspot.com/179060/diff/1/3#newcode570
draft/Social-API-Server.xml:570: <section title="Get Groups"
Please reorder to be Get, Create, Update, Delete.  This is consistent with the
rest of the doc and also puts the required method (i.e. get) first.

http://codereview.appspot.com/179060/diff/1/3#newcode583
draft/Social-API-Server.xml:583: <eref
target="Core-API-Server.xml#Return-Object">Return-Object</eref>          = <eref
target="Core-Data.xml#Collection">Collection</eref>&lt;<eref
target="Social-Data.xml#Group">Group</eref>&gt;</artwork>
Should be  Group / Collection<Group> 

(in case of /groups/@me/<some group id>) the request will return a single group
object, not a collection.

http://codereview.appspot.com/179060/diff/1/3#newcode592
draft/Social-API-Server.xml:592: <c>User ID of the person initiating the group
creation request.  Defaults to "@me", which MUST return the currently logged in
user.</c>
remove 'creation'

http://codereview.appspot.com/179060/diff/1/3#newcode609
draft/Social-API-Server.xml:609: <eref
target="Core-API-Server.xml#REST-URI-Fragment">REST-URI-Fragment</eref>      =
"/groups/" <eref target="Core-Data.xml#Group-Id">Group-Id</eref>
Seems strange that the userId is in the URL to create this group, but not to
delete it.

http://codereview.appspot.com/179060/diff/1/3#newcode626
draft/Social-API-Server.xml:626: <c>userId</c>
userId is not included in the ABNF above so it doesn't belong here.  (however, I
think userId probably should be in the abnf above, in which case it should be
included in this table.)

http://codereview.appspot.com/179060/diff/1/3#newcode628
draft/Social-API-Server.xml:628: <c>User ID of the person initiating the group
creation request.  Defaults to "@me", which MUST return the currently logged in
user.</c>
creation -> deletion

http://codereview.appspot.com/179060/diff/1/3#newcode645
draft/Social-API-Server.xml:645: <eref
target="Core-API-Server.xml#REST-URI-Fragment">REST-URI-Fragment</eref>      =
"/groups/" <eref target="Core-Data.xml#Group-Id">Group-Id</eref>
same concern as above about userId.  Maybe the solution is to take the userID
out of the path for creation.

http://codereview.appspot.com/179060/diff/1/2
File draft/Social-Data.xml (right):

http://codereview.appspot.com/179060/diff/1/2#newcode670
draft/Social-Data.xml:670: <section title="Group"
Move this section so the sections are alphebetized

http://codereview.appspot.com/179060/diff/1/2#newcode672
draft/Social-Data.xml:672: <t>Representation of a group, which may be a private,
invitation only, public or a personal group used to organize friends.</t>
invitation-only (should be hyphenated)

http://codereview.appspot.com/179060/diff/1/2#newcode673
draft/Social-Data.xml:673: <t>Each group returned MUST include the "id" and
"title" fields. A "description" field is optional.</t>
Probably better to remove the ABNF and just describe the object and its fields.

http://codereview.appspot.com/179060/diff/1/2#newcode681
draft/Social-Data.xml:681: <texttable>
align="left"

http://codereview.appspot.com/179060/diff/1/2#newcode685
draft/Social-Data.xml:685: <c>id</c><c></c><c>Unique ID for this group</c>
1) add <x:ref>Group-ID</x:ref> in type column
2) mark as <x:highlight>Required.<x:highlight/> in the description column

http://codereview.appspot.com/179060/diff/1/2#newcode686
draft/Social-Data.xml:686: <c>title</c><c></c><c>Title of group</c>
1) add 'String' in type column
2) mark as <x:highlight>Required.<x:highlight/> in the description column

http://codereview.appspot.com/179060/diff/1/2#newcode687
draft/Social-Data.xml:687: <c>description</c><c></c><c>Description of group</c>
1) add 'String' in type column
2) mark as <x:highlight>Optional.<x:highlight/> in the description column

http://codereview.appspot.com/179060/diff/1/4
File draft/Social-Gadget.xml (right):

http://codereview.appspot.com/179060/diff/1/4#newcode334
draft/Social-Gadget.xml:334: <t hangText="Returns">A <eref
target="Core-Gadget.xml#osapi.Request">osapi.Request</eref> to create a group
via the Groups service. Executing this request MUST create a group, but does not
return any information.</t>
this should return the newly-created group.

[Lane LiaBraaten, 2010-01-11 16:15:47]

Hi Dave,

Sorry for the delay in reviewing this patch.  The content looks good though
-- Please go ahead and commit it.

Cheers,
Lane

On Thu, Dec 17, 2009 at 10:34 AM, <snoopdave@gmail.com> wrote:

> Reviewers: Lane LiaBraaten,
>
> Message:
> I applied all of Lane's suggested fixes, including making the Groups
> Service a MAY and adding making the Group URLs consistent (create,
> delete and update now include the userId).
>
> Description:
> Adding support for create, retrieve update and delete of groups to the
> Social Data, Social API Server and Social Gadget specs.
>
> Please review this at http://codereview.appspot.com/179060
>
> Affected files:
>
>  M     draft/Social-API-Server.xml
>  M     draft/Social-Data.xml
>  M     draft/Social-Gadget.xml
>
>
>