You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rave.apache.org by Sean Cooper <se...@gmail.com> on 2011/10/11 17:32:31 UTC
Review Request: RAVE-71: Rate Widgets Partial Patch
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2338/
-----------------------------------------------------------
Review request for rave.
Summary
-------
Partial Patch which allows users to rate widgets from the detailed widget view.
This patch does NOT include a UI to view the user's current rating for a widget, nor does it include the UI for showing the rating totals for a given widget.
Questions for the team
1) Do we want to add the ability to rate widgets from the widget store (non-detailed page)?
2) Do we want to add the ability to rate widgets from the widget menu on a user's page?
3) Do we want to include totals of the ratings? (e.g. 314 Liked, 2014 Disliked)
3) How should we handle the display of the rating for the user?
a) Iterate over object model to determine user's current rating and totals - This will result in performance issues when a lot of users have provided ratings for widgets.
b) Create a statistics service to query the database directly and attach this statistical information to the object model - This will result in a hard coded SQL query to gather this information and requires attaching more information to the web model, as well as requiring mapping the various widget ratings to the widgets on the UI side.
c) Suggest another approach
This addresses bug RAVE-71.
https://issues.apache.org/jira/browse/RAVE-71
Diffs
-----
/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/Widget.java 1181810
/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/WidgetService.java 1181810
/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultWidgetService.java 1181810
/trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/model/WidgetTest.java 1181810
/trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/service/WidgetServiceTest.java 1181810
/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/api/rest/WidgetApi.java 1181810
/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreController.java 1181810
/trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/api/rest/WidgetApiTest.java 1181810
/trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/WidgetStoreControllerTest.java 1181810
/trunk/rave-portal-resources/src/main/webapp/WEB-INF/views/widget.jsp 1181810
/trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1181810
Diff: https://reviews.apache.org/r/2338/diff
Testing
-------
Thanks,
Sean
Re: Review Request: RAVE-71: Rate Widgets Partial Patch
Posted by Jasha Joachimsthal <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2338/#review2529
-----------------------------------------------------------
Thanks for the submitted patch. I have a few remarks.
/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/Widget.java
<https://reviews.apache.org/r/2338/#comment5698>
Please add tests in JpaWidgetRepositoryTest that proves the correct behaviour in CRUD operations instead of only mocking the result in the Service.
You probably need to set the Widget (this) to the new WidgetRating) to let the OneToMany relation work properly. Add a method addRating(WidgetRating rating) to add a single WidgetRating.
/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/WidgetService.java
<https://reviews.apache.org/r/2338/#comment5699>
shouldn't the save and delete be "void"?
why are the input variables different between these methods?
/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/api/rest/WidgetApi.java
<https://reviews.apache.org/r/2338/#comment5700>
This makes it possible to do a request for any userid. Shouldn't the user id be retrieved from the request (current authenticated user)?
Why is widgetId a long primitive and userId a Long object?
/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/api/rest/WidgetApi.java
<https://reviews.apache.org/r/2338/#comment5701>
This makes it possible to do a request for any userid. Shouldn't the user id be retrieved from the request (current authenticated user)?
/trunk/rave-portal-resources/src/main/webapp/WEB-INF/views/widget.jsp
<https://reviews.apache.org/r/2338/#comment5702>
Please use message keys instead of hard coded text
/trunk/rave-portal-resources/src/main/webapp/WEB-INF/views/widget.jsp
<https://reviews.apache.org/r/2338/#comment5703>
Please move this inline script to an (existing) external scriptfile
- Jasha
On 2011-10-11 15:32:31, Sean Cooper wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2338/
> -----------------------------------------------------------
>
> (Updated 2011-10-11 15:32:31)
>
>
> Review request for rave.
>
>
> Summary
> -------
>
> Partial Patch which allows users to rate widgets from the detailed widget view.
>
> This patch does NOT include a UI to view the user's current rating for a widget, nor does it include the UI for showing the rating totals for a given widget.
>
> Questions for the team
> 1) Do we want to add the ability to rate widgets from the widget store (non-detailed page)?
> 2) Do we want to add the ability to rate widgets from the widget menu on a user's page?
> 3) Do we want to include totals of the ratings? (e.g. 314 Liked, 2014 Disliked)
> 3) How should we handle the display of the rating for the user?
> a) Iterate over object model to determine user's current rating and totals - This will result in performance issues when a lot of users have provided ratings for widgets.
> b) Create a statistics service to query the database directly and attach this statistical information to the object model - This will result in a hard coded SQL query to gather this information and requires attaching more information to the web model, as well as requiring mapping the various widget ratings to the widgets on the UI side.
> c) Suggest another approach
>
>
> This addresses bug RAVE-71.
> https://issues.apache.org/jira/browse/RAVE-71
>
>
> Diffs
> -----
>
> /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/Widget.java 1181810
> /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/WidgetService.java 1181810
> /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultWidgetService.java 1181810
> /trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/model/WidgetTest.java 1181810
> /trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/service/WidgetServiceTest.java 1181810
> /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/api/rest/WidgetApi.java 1181810
> /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreController.java 1181810
> /trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/api/rest/WidgetApiTest.java 1181810
> /trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/WidgetStoreControllerTest.java 1181810
> /trunk/rave-portal-resources/src/main/webapp/WEB-INF/views/widget.jsp 1181810
> /trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1181810
>
> Diff: https://reviews.apache.org/r/2338/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sean
>
>
Re: Review Request: RAVE-71: Rate Widgets Partial Patch
Posted by Jasha Joachimsthal <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2338/#review2612
-----------------------------------------------------------
The WidgetRating class is missing in the patch. Can you create a new patch that includes this class (and possibly other classes that don't exist yet)?
- Jasha
On 2011-10-11 15:32:31, Sean Cooper wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2338/
> -----------------------------------------------------------
>
> (Updated 2011-10-11 15:32:31)
>
>
> Review request for rave.
>
>
> Summary
> -------
>
> Partial Patch which allows users to rate widgets from the detailed widget view.
>
> This patch does NOT include a UI to view the user's current rating for a widget, nor does it include the UI for showing the rating totals for a given widget.
>
> Questions for the team
> 1) Do we want to add the ability to rate widgets from the widget store (non-detailed page)?
> 2) Do we want to add the ability to rate widgets from the widget menu on a user's page?
> 3) Do we want to include totals of the ratings? (e.g. 314 Liked, 2014 Disliked)
> 3) How should we handle the display of the rating for the user?
> a) Iterate over object model to determine user's current rating and totals - This will result in performance issues when a lot of users have provided ratings for widgets.
> b) Create a statistics service to query the database directly and attach this statistical information to the object model - This will result in a hard coded SQL query to gather this information and requires attaching more information to the web model, as well as requiring mapping the various widget ratings to the widgets on the UI side.
> c) Suggest another approach
>
>
> This addresses bug RAVE-71.
> https://issues.apache.org/jira/browse/RAVE-71
>
>
> Diffs
> -----
>
> /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/Widget.java 1181810
> /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/WidgetService.java 1181810
> /trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultWidgetService.java 1181810
> /trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/model/WidgetTest.java 1181810
> /trunk/rave-components/rave-core/src/test/java/org/apache/rave/portal/service/WidgetServiceTest.java 1181810
> /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/api/rest/WidgetApi.java 1181810
> /trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreController.java 1181810
> /trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/api/rest/WidgetApiTest.java 1181810
> /trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/WidgetStoreControllerTest.java 1181810
> /trunk/rave-portal-resources/src/main/webapp/WEB-INF/views/widget.jsp 1181810
> /trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1181810
>
> Diff: https://reviews.apache.org/r/2338/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sean
>
>