Completing Groups support for OpenSocial 1.0 (issue179060)

[llia...@google.com]

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.

http://codereview.appspot.com/179060

[Evan Gilbert]

On Tue, Dec 15, 2009 at 6:34 PM, <llia...@google.com> wrote:

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

I'm pretty sure that groups should be a MAY. Many containers only support the "@friends" group and no support for modifications by apps.

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.

Think we should standardize on create + update returning the new object, and delete returning null.

 

--

You received this message because you are subscribed to the Google Groups "OpenSocial and Gadgets Specification Discussion" group.
To post to this group, send email to opensocial-an...@googlegroups.com.
To unsubscribe from this group, send email to opensocial-and-gadg...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en.

[Dave]

Thanks for the review(s). I will change the groups service to a MAY
and address other issues by tomorrow.

If I make the Groups Service a MAY, then what should I do about the
individual operations. Do they all become MAY too? I guess I should
look for examples of this type of thing in the standing specs.

Or, does something like this make sense: Group Service is a MAY, but
if you choose to implement it then the Get operation is a MUST and the
other operations are MAY.

- Dave

[Evan Gilbert]

On Wed, Dec 16, 2009 at 2:44 PM, Dave <snoo...@gmail.com> wrote:

Thanks for the review(s). I will change the groups service to a MAY
and address other issues by tomorrow.

If I make the Groups Service a MAY, then what should I do about the
individual operations. Do they all become MAY too? I guess I should
look for examples of this type of thing in the standing specs.

Or, does something like this make sense: Group Service is a MAY, but
if you choose to implement it then the Get operation is a MUST and the
other operations are MAY.

This sounds right. I mostly can't imagine groups without a get, but other ones seem likely to be a container choice.

[snoo...@gmail.com]

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

[Lane LiaBraaten]

Hi Dave,

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

Cheers,

Lane