You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rave.apache.org by Viknes B <vi...@msn.com> on 2012/11/10 03:02:22 UTC
Review Request: Fix for Rave-845
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8005/
-----------------------------------------------------------
Review request for rave.
Description
-------
Inserted a delete query to remove associations of a particular user when deleting the user.
This addresses bug Rave-845.
https://issues.apache.org/jira/browse/Rave-845
Diffs
-----
https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java 1406782
https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultUserService.java 1406782
https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/model/JpaPersonAssociation.java 1406782
https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/repository/impl/JpaPersonRepository.java 1406782
https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/test/java/org/apache/rave/portal/repository/impl/JpaPersonRepositoryTest.java 1406782
https://svn.apache.org/repos/asf/rave/trunk/rave-providers/rave-opensocial-provider/rave-opensocial-core/src/main/java/org/apache/rave/opensocial/repository/impl/DecoratingOpenSocialPersonRepository.java 1406782
Diff: https://reviews.apache.org/r/8005/diff/
Testing
-------
Thanks,
Viknes B
Re: Review Request: Fix for Rave-845
Posted by Matt Franklin <mf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8005/#review13392
-----------------------------------------------------------
IMO, I think this needs to be a local repository function. We don't expose the concept of PersonAssociation through the Model interfaces and it doesn't seem to make sense that we would not rely on the repository to actually ensure an object is removed.
- Matt Franklin
On Nov. 10, 2012, 2:02 a.m., Viknes B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8005/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2012, 2:02 a.m.)
>
>
> Review request for rave.
>
>
> Description
> -------
>
> Inserted a delete query to remove associations of a particular user when deleting the user.
>
>
> This addresses bug Rave-845.
> https://issues.apache.org/jira/browse/Rave-845
>
>
> Diffs
> -----
>
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultUserService.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/model/JpaPersonAssociation.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/repository/impl/JpaPersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/test/java/org/apache/rave/portal/repository/impl/JpaPersonRepositoryTest.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-providers/rave-opensocial-provider/rave-opensocial-core/src/main/java/org/apache/rave/opensocial/repository/impl/DecoratingOpenSocialPersonRepository.java 1406782
>
> Diff: https://reviews.apache.org/r/8005/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Viknes B
>
>
Re: Review Request: Fix for Rave-845
Posted by Viknes B <vi...@msn.com>.
> On Nov. 12, 2012, 4:17 p.m., Chris Geer wrote:
> > https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java, line 171
> > <https://reviews.apache.org/r/8005/diff/1/?file=188291#file188291line171>
> >
> > Does it make more sense to call this "removeAllAssociations" since all the variables, comments and such refer to associations?
I just named it that way so that it is not confused with connections of a particular user(which would include groups) and also the method name would be similar to other methods in the class. But i can go ahead and rename it if needed.
- Viknes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8005/#review13356
-----------------------------------------------------------
On Nov. 10, 2012, 2:02 a.m., Viknes B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8005/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2012, 2:02 a.m.)
>
>
> Review request for rave.
>
>
> Description
> -------
>
> Inserted a delete query to remove associations of a particular user when deleting the user.
>
>
> This addresses bug Rave-845.
> https://issues.apache.org/jira/browse/Rave-845
>
>
> Diffs
> -----
>
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultUserService.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/model/JpaPersonAssociation.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/repository/impl/JpaPersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/test/java/org/apache/rave/portal/repository/impl/JpaPersonRepositoryTest.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-providers/rave-opensocial-provider/rave-opensocial-core/src/main/java/org/apache/rave/opensocial/repository/impl/DecoratingOpenSocialPersonRepository.java 1406782
>
> Diff: https://reviews.apache.org/r/8005/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Viknes B
>
>
Re: Review Request: Fix for Rave-845
Posted by Viknes B <vi...@msn.com>.
> On Nov. 12, 2012, 4:17 p.m., Chris Geer wrote:
> > https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java, line 171
> > <https://reviews.apache.org/r/8005/diff/1/?file=188291#file188291line171>
> >
> > Does it make more sense to call this "removeAllAssociations" since all the variables, comments and such refer to associations?
>
> Viknes B wrote:
> I just named it that way so that it is not confused with connections of a particular user(which would include groups) and also the method name would be similar to other methods in the class. But i can go ahead and rename it if needed.
>
> Chris Geer wrote:
> So if I understand correctly, groups are not stored in the associations table but you were concerned that by calling it removeAllAssociations it might be confusing because it wasn't removing groups. I can see that. So, the follow-up question, is the term associations used everywhere going to be confusing by the same token? Not really relevant to this ticket so maybe we take that to the list.
>
> I'm ok leaving it as it is. I still need to test it so I'll do that today as soon as I can.
Ok. Yes, we can talk on that on the list.
- Viknes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8005/#review13356
-----------------------------------------------------------
On Nov. 10, 2012, 2:02 a.m., Viknes B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8005/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2012, 2:02 a.m.)
>
>
> Review request for rave.
>
>
> Description
> -------
>
> Inserted a delete query to remove associations of a particular user when deleting the user.
>
>
> This addresses bug Rave-845.
> https://issues.apache.org/jira/browse/Rave-845
>
>
> Diffs
> -----
>
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultUserService.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/model/JpaPersonAssociation.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/repository/impl/JpaPersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/test/java/org/apache/rave/portal/repository/impl/JpaPersonRepositoryTest.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-providers/rave-opensocial-provider/rave-opensocial-core/src/main/java/org/apache/rave/opensocial/repository/impl/DecoratingOpenSocialPersonRepository.java 1406782
>
> Diff: https://reviews.apache.org/r/8005/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Viknes B
>
>
Re: Review Request: Fix for Rave-845
Posted by Chris Geer <ch...@cxtsoftware.com>.
> On Nov. 12, 2012, 4:17 p.m., Chris Geer wrote:
> > https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java, line 171
> > <https://reviews.apache.org/r/8005/diff/1/?file=188291#file188291line171>
> >
> > Does it make more sense to call this "removeAllAssociations" since all the variables, comments and such refer to associations?
>
> Viknes B wrote:
> I just named it that way so that it is not confused with connections of a particular user(which would include groups) and also the method name would be similar to other methods in the class. But i can go ahead and rename it if needed.
So if I understand correctly, groups are not stored in the associations table but you were concerned that by calling it removeAllAssociations it might be confusing because it wasn't removing groups. I can see that. So, the follow-up question, is the term associations used everywhere going to be confusing by the same token? Not really relevant to this ticket so maybe we take that to the list.
I'm ok leaving it as it is. I still need to test it so I'll do that today as soon as I can.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8005/#review13356
-----------------------------------------------------------
On Nov. 10, 2012, 2:02 a.m., Viknes B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8005/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2012, 2:02 a.m.)
>
>
> Review request for rave.
>
>
> Description
> -------
>
> Inserted a delete query to remove associations of a particular user when deleting the user.
>
>
> This addresses bug Rave-845.
> https://issues.apache.org/jira/browse/Rave-845
>
>
> Diffs
> -----
>
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultUserService.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/model/JpaPersonAssociation.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/repository/impl/JpaPersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/test/java/org/apache/rave/portal/repository/impl/JpaPersonRepositoryTest.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-providers/rave-opensocial-provider/rave-opensocial-core/src/main/java/org/apache/rave/opensocial/repository/impl/DecoratingOpenSocialPersonRepository.java 1406782
>
> Diff: https://reviews.apache.org/r/8005/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Viknes B
>
>
Re: Review Request: Fix for Rave-845
Posted by Chris Geer <ch...@cxtsoftware.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8005/#review13356
-----------------------------------------------------------
https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java
<https://reviews.apache.org/r/8005/#comment28625>
Does it make more sense to call this "removeAllAssociations" since all the variables, comments and such refer to associations?
- Chris Geer
On Nov. 10, 2012, 2:02 a.m., Viknes B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8005/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2012, 2:02 a.m.)
>
>
> Review request for rave.
>
>
> Description
> -------
>
> Inserted a delete query to remove associations of a particular user when deleting the user.
>
>
> This addresses bug Rave-845.
> https://issues.apache.org/jira/browse/Rave-845
>
>
> Diffs
> -----
>
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultUserService.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/model/JpaPersonAssociation.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/main/java/org/apache/rave/portal/repository/impl/JpaPersonRepository.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-components/rave-jpa/src/test/java/org/apache/rave/portal/repository/impl/JpaPersonRepositoryTest.java 1406782
> https://svn.apache.org/repos/asf/rave/trunk/rave-providers/rave-opensocial-provider/rave-opensocial-core/src/main/java/org/apache/rave/opensocial/repository/impl/DecoratingOpenSocialPersonRepository.java 1406782
>
> Diff: https://reviews.apache.org/r/8005/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Viknes B
>
>