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