rave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mfrank...@apache.org
Subject Re: Review Request: RAVE-103. Support shared spaces.
Date Sun, 29 Apr 2012 22:54:49 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4843/#review7370
-----------------------------------------------------------



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/PageStatus.java
<https://reviews.apache.org/r/4843/#comment16254>

    Nit:  not really the Page's status here but the invitation to use the page



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PageRepository.java
<https://reviews.apache.org/r/4843/#comment16255>

    Shouln't spring manage the dependency injection, if a page user repository is really needed
in a page repository?



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PageUserRepository.java
<https://reviews.apache.org/r/4843/#comment16256>

    Not sure what the intent of getPagesForUser is over getAllViewablePages.  
    
    AllViewable might be a large set depending on what you assume visibility rules are.  For
instance, everyone's profile page is visible to everyone else...
    
    Would it make sense to rely on the PageRepository to get pages that a person can view?
 



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PageUserRepository.java
<https://reviews.apache.org/r/4843/#comment16258>

    Since we are changing the logical model of how pages are handled, would it make sense
to just update the PageRepository to handle the exra use cases?



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/impl/JpaPageRepository.java
<https://reviews.apache.org/r/4843/#comment16257>

    You shouldn't need to persist the PageUser collection separately if the cascade rules
are set appropriately.  This will allow you to remove the dependency on PageUserRepository



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/PageService.java
<https://reviews.apache.org/r/4843/#comment16259>

    Need authorization



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultPageService.java
<https://reviews.apache.org/r/4843/#comment16260>

    Probably a completely separate discussion, but I think we need to decide on whether or
not we will have top level model classes with repositories responsible for persisiting the
entire subgraph for that object or repositories for every model class.  This will work for
JPA, as it manages the entire object and all relations; but also with something like Mongo.
 



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultPageService.java
<https://reviews.apache.org/r/4843/#comment16261>

    see comment for 164



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultPageService.java
<https://reviews.apache.org/r/4843/#comment16262>

    See comments from 164



/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultPageService.java
<https://reviews.apache.org/r/4843/#comment16263>

    interface methods should probably go above private helpers for readability



/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/PageController.java
<https://reviews.apache.org/r/4843/#comment16264>

    Isn't this property navigable from Page?  


- mfranklin


On 2012-04-27 09:42:17, Paul Sharples wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4843/
> -----------------------------------------------------------
> 
> (Updated 2012-04-27 09:42:17)
> 
> 
> Review request for rave.
> 
> 
> Summary
> -------
> 
> RAVE-103. support shared spaces.  I've submitted this patch here rather than commit the
code directly, as the changes affect the UI and I wanted a request for comments type approach
first. This is a page sharing patch which allows a user to share his/her page with other rave
users, as well as also allowing the user to revoke page shares. A user who receives a shared
page can opt to confirm the share (meaning the page will always appear in his/her tabbed page
list or decline it (i.e I don't want this shared page).  There's still more to improve on
this, but the basic functionality is there. Note I have removed the render sequencing away
from the page object into the new pageUser object.  This is because with the possibility of
having several users of a page, they all need to have their own page render sequencing. See
RAVE-103.
> 
> 
> Diffs
> -----
> 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/Page.java
1306906 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/PageStatus.java
PRE-CREATION 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/PageUser.java
PRE-CREATION 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PageRepository.java
1308947 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/PageUserRepository.java
PRE-CREATION 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/impl/JpaPageRepository.java
1308947 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/impl/JpaPageUserRepository.java
PRE-CREATION 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/security/impl/DefaultPagePermissionEvaluator.java
1306906 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/PageService.java
1306906 
>   /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultPageService.java
1310534 
>   /trunk/rave-components/rave-core/src/main/resources/META-INF/persistence.xml 1306906

>   /trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/model/PageTest.java
1306906 
>   /trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/repository/impl/JpaPageRepositoryTest.java
1308947 
>   /trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/service/impl/DefaultPageServiceTest.java
1327947 
>   /trunk/rave-components/rave-core/src/test/resources/test_data.sql 1308947 
>   /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/api/rpc/PageApi.java
1306906 
>   /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/api/rpc/UserApi.java
PRE-CREATION 
>   /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/PageController.java
1306906 
>   /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/util/ModelKeys.java
1330718 
>   /trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/PageControllerTest.java
1330044 
>   /trunk/rave-portal-resources/src/main/resources/messages.properties 1330724 
>   /trunk/rave-portal-resources/src/main/webapp/WEB-INF/db/initial_data.sql 1327941 
>   /trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/page.jsp 1330737 
>   /trunk/rave-portal-resources/src/main/webapp/css/default.css 1330727 
>   /trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1306906 
>   /trunk/rave-portal-resources/src/main/webapp/script/rave_layout.js 1330737 
>   /trunk/rave-portal/src/test/resources/test-data.sql 1306906 
> 
> Diff: https://reviews.apache.org/r/4843/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Paul
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message