The subroutines in C4::Reviews are only doing CRUD operations. This module can be moved easily to Koha::Reviews, especially as it already uses DBIx::Class
Created attachment 54207 [details] [review] Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes The subroutines in C4::Ratings are only CRUD operations. This patch set moves the entire module to Koha::Rating[s] based on Koha::Object[s] Test plan for the whole patch set: 0/ Set OpacStarRatings to "results and details" 1/ Log in at the OPAC 2/ Launch a search, you should see the star ratings 3/ Go on a detail page, you should see the star ratings 4/ Rate the record 5/ Log in with another patron 6/ Go on the same detail page, you should see the previous rating 7/ Rate the racord, you should see "2 votes" and the average rating value. 8/ Cancel your rate 9/ Disable JavaScript and repeat 7 Note: You cannot cancel your rate if JavaScript is disabled (not a regression).
Created attachment 54208 [details] [review] Bug 17089: Koha::Ratings - move tests Rewrite and move the tests.
Created attachment 54209 [details] [review] Bug 17089: Koha::Ratings - Remove AddRating
Created attachment 54210 [details] [review] Bug 17089: Koha::Ratings - Remove ModRating
Created attachment 54211 [details] [review] Bug 17089: Koha::Ratings - Remove DelRating
Created attachment 54212 [details] [review] Bug 17089: Koha::Ratings - Remove GetRating
Created attachment 54213 [details] [review] Bug 17089: Koha::Ratings - Remove C4::Ratings
Created attachment 54214 [details] [review] Bug 17089: Mark out the boundaries of ratings In the doc of C4::Ratings, it was written that the rate values should be >=0 and <=5, but it was not taken into account. This bug should be fixed on stable releases as well.
I'm getting an error in the console when I try to cancel a rating: Can't call method "rating_value" on an undefined value at opac/opac-ratings-ajax.pl line 90
Created attachment 54225 [details] [review] Bug 17089: Improve errors handling
Created attachment 54230 [details] [review] [SIGNED-OFF] Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes The subroutines in C4::Ratings are only CRUD operations. This patch set moves the entire module to Koha::Rating[s] based on Koha::Object[s] Test plan for the whole patch set: 0/ Set OpacStarRatings to "results and details" 1/ Log in at the OPAC 2/ Launch a search, you should see the star ratings 3/ Go on a detail page, you should see the star ratings 4/ Rate the record 5/ Log in with another patron 6/ Go on the same detail page, you should see the previous rating 7/ Rate the racord, you should see "2 votes" and the average rating value. 8/ Cancel your rate 9/ Disable JavaScript and repeat 7 Note: You cannot cancel your rate if JavaScript is disabled (not a regression). Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54231 [details] [review] [SIGNED-OFF] Bug 17089: Koha::Ratings - move tests Rewrite and move the tests. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54232 [details] [review] [SIGNED-OFF] Bug 17089: Koha::Ratings - Remove AddRating Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54233 [details] [review] [SIGNED-OFF] Bug 17089: Koha::Ratings - Remove ModRating Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54234 [details] [review] [SIGNED-OFF] Bug 17089: Koha::Ratings - Remove DelRating Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54235 [details] [review] [SIGNED-OFF] Bug 17089: Koha::Ratings - Remove GetRating Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54236 [details] [review] [SIGNED-OFF] Bug 17089: Koha::Ratings - Remove C4::Ratings Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54237 [details] [review] [SIGNED-OFF] Bug 17089: Mark out the boundaries of ratings In the doc of C4::Ratings, it was written that the rate values should be >=0 and <=5, but it was not taken into account. This bug should be fixed on stable releases as well. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54238 [details] [review] [SIGNED-OFF] Bug 17089: Improve errors handling Signed-off-by: Owen Leonard <oleonard@myacpl.org>
QA: Looking here now.
Created attachment 54332 [details] [review] Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes The subroutines in C4::Ratings are only CRUD operations. This patch set moves the entire module to Koha::Rating[s] based on Koha::Object[s] Test plan for the whole patch set: 0/ Set OpacStarRatings to "results and details" 1/ Log in at the OPAC 2/ Launch a search, you should see the star ratings 3/ Go on a detail page, you should see the star ratings 4/ Rate the record 5/ Log in with another patron 6/ Go on the same detail page, you should see the previous rating 7/ Rate the racord, you should see "2 votes" and the average rating value. 8/ Cancel your rate 9/ Disable JavaScript and repeat 7 Note: You cannot cancel your rate if JavaScript is disabled (not a regression). Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54333 [details] [review] Bug 17089: Koha::Ratings - move tests Rewrite and move the tests. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54334 [details] [review] Bug 17089: Koha::Ratings - Remove AddRating Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54335 [details] [review] Bug 17089: Koha::Ratings - Remove ModRating Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54336 [details] [review] Bug 17089: Koha::Ratings - Remove DelRating Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54337 [details] [review] Bug 17089: Koha::Ratings - Remove GetRating Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54338 [details] [review] Bug 17089: Koha::Ratings - Remove C4::Ratings Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54339 [details] [review] Bug 17089: Mark out the boundaries of ratings In the doc of C4::Ratings, it was written that the rate values should be >=0 and <=5, but it was not taken into account. This bug should be fixed on stable releases as well. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54340 [details] [review] Bug 17089: Improve errors handling Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54341 [details] [review] Bug 17089: [QA Follow-up] Add some test descriptions to Ratings.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Code looks good to me. eval { $avg = $sum / $total }; Not your code, but an eval around a non-zero division seems overkill. [% rating_avg_int = BLOCK %][% rating_avg | format("%.0f") %][% END %] This kind of construct does not look very pretty. Simplify? Passed QA
(In reply to Marcel de Rooy from comment #31) > [% rating_avg_int = BLOCK %][% rating_avg | format("%.0f") %][% END %] > This kind of construct does not look very pretty. Simplify? It's not possible to set a variable to something using a filter: [% SET rating_avg_int = rating_avg | format("%.0f") %] This does not work. The construct is the same I already used in facets.inc: [% url = BLOCK %][% url %][% "&sort_by=" _ sort_by |url %][% END %] If you find something easier to write/read I will be happy to know it :)
Pushed to master for 16.11, thanks Jonathan!