Mailing-List: contact dev-help@shindig.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@shindig.apache.org
Content-Type: multipart/alternative;
	boundary="===============0951320082559451973=="
MIME-Version: 1.0
Subject: Re: Review Request: A patch that enables API for updating person data
From: "Evgeny Bogdanov" <evgeny.bogdanov@epfl.ch>
To: "shindig" <dev@shindig.apache.org>,
 "Stanton Sievers" <sieverssj@gmail.com>,
	"Evgeny Bogdanov" <evgeny.bogdanov@epfl.ch>
Date: Mon, 23 Jan 2012 13:50:29 -0000
Message-ID: <20120123135029.8384.96605@reviews.apache.org>
In-Reply-To: <20120120163555.9614.27972@reviews.apache.org>
References: <20120120163555.9614.27972@reviews.apache.org>

--===============0951320082559451973==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable


> 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 d=
oesn'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.peop=
le.update() in the social gadget spec and if the intent is for it to be the=
re, 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 speci=
fic 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 be=
lieve.
>     Once one implements the rest api in shindig, javascript api is automa=
tically 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 t=
hat it depends on the container.  For instance, to enable administration or=
iented use cases, where you have an admin with elevated privileges, you wou=
ld need to allow one person to update another in the server code.  The Pers=
onServiceDb 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 e=
xplicit feature code tests for it.  Maybe this is a larger question for the=
 whole dev list.

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 o=
pen as a gadget that will run tests
I will add those tests


- Evgeny


-----------------------------------------------------------
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/jav=
a/org/apache/shindig/social/opensocial/jpa/spi/PersonServiceDb.java 1231912 =

>   http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/test/jav=
a/org/apache/shindig/social/opensocial/jpa/spi/PersonServiceDbTest.java 123=
1912 =

>   http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/=
java/org/apache/shindig/social/opensocial/service/PersonHandler.java 123191=
2 =

>   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 1231=
912 =

>   http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/=
java/org/apache/shindig/social/opensocial/service/PersonHandlerTest.java 12=
31912 =

>   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
> =

>


--===============0951320082559451973==--