> On 2012-01-20 16:35:55, Stanton Sievers wrote: > > Small nits on whitespace. > > > > I do have some bigger questions regarding the implementation and maybe > > these are more for the spec group. What's to stop one person from updating > > another person? The rest API says that the User-Id defaults to @me, but > > doesn't say you can't list another user. Shouldn't we only allow @me to be > > updated, which would be equivalent to the viewer id in the security token? > > > > You mentioned implementing osapi.people.update() as a gadget API, but I > > neither see it in this review (is it there by virtue of the REST API being > > implemented?) nor do I see any tests for it. I also don't see > > osapi.people.update() in the social gadget spec and if the intent is for it > > to be there, you should try to get the spec updated as well. > > Evgeny Bogdanov wrote: > Thanks Stanton for your points. > > 1. What's to stop one person from updating another person? > I also had this question, but then I thought that maybe it depends on > container, > because anyways it's done in PersonServiceDb which is container specific > normally. > I'll start a discussion on it in the spec mailing list. > > > 2. osapi.people.update() > it's true, it is not in the spec, only REST api, should be added I > believe. > Once one implements the rest api in shindig, javascript api is > automatically available. > I m not sure whether it should be tested or not, since this code was not > written by me :)
I had some offline conversations about #1 and I think you are right that it depends on the container. For instance, to enable administration oriented use cases, where you have an admin with elevated privileges, you would need to allow one person to update another in the server code. The PersonServiceDb would have to handle the rights management. Re #2. If you don't mind writing up the tests I think they would be useful. It's reassuring (at least to me :) to have explicit tests for code like that as a place where we can point and say "yes, osapi.people.update()" works, and say it with a greater level of confidence. This doesn't seem to be the case with the osapi.people.get, et al, however... I can't find explicit feature code tests for it. Maybe this is a larger question for the whole dev list. - Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3544/#review4491 ----------------------------------------------------------- On 2012-01-19 09:30:25, Evgeny Bogdanov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3544/ > ----------------------------------------------------------- > > (Updated 2012-01-19 09:30:25) > > > Review request for shindig. > > > Summary > ------- > > A patch that enables API for updating person data > PUT people/12/@self > osapi.people.update() > > > for this JIRA reports > https://issues.apache.org/jira/browse/SHINDIG-1674 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/main/java/org/apache/shindig/social/opensocial/jpa/spi/PersonServiceDb.java > 1231912 > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/test/java/org/apache/shindig/social/opensocial/jpa/spi/PersonServiceDbTest.java > 1231912 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/PersonHandler.java > 1231912 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/PersonService.java > 1231912 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialService.java > 1231912 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/opensocial/service/PersonHandlerTest.java > 1231912 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialServiceTest.java > 1231912 > > Diff: https://reviews.apache.org/r/3544/diff > > > Testing > ------- > > tests are in the patch > > > Thanks, > > Evgeny > >
