> 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 :) > > Stanton Sievers wrote: > 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. > > Evgeny Bogdanov wrote: > well, it seems as osapi.people.get, etc. are tested. > 1) in peopleTest.xml file > 2) in peoplesuite.js (suite.xml) > > I don't know if they run automatically with mvn, but at least they can be > open as a gadget that will run tests > I will add those tests
I think I'd like to see the default shindig impl only allow users to edit their own data with this api. I'd also like to see a place where container implementations can override this behavior easily. Where would they do that today, inside the entiyManager? - Dan ----------------------------------------------------------- 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 > >
