Bug 17089 - Move the star ratings related code to Koha::Ratings
Summary: Move the star ratings related code to Koha::Ratings
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 15449
  Show dependency treegraph
 
Reported: 2016-08-09 13:39 UTC by Jonathan Druart
Modified: 2017-06-14 22:16 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes (3.48 KB, patch)
2016-08-09 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - move tests (6.64 KB, patch)
2016-08-09 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove AddRating (3.29 KB, patch)
2016-08-09 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove ModRating (3.19 KB, patch)
2016-08-09 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove DelRating (2.02 KB, patch)
2016-08-09 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove GetRating (12.41 KB, patch)
2016-08-09 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove C4::Ratings (5.25 KB, patch)
2016-08-09 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Mark out the boundaries of ratings (2.31 KB, patch)
2016-08-09 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17089: Improve errors handling (2.61 KB, patch)
2016-08-09 16:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes (3.59 KB, patch)
2016-08-09 16:40 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Koha::Ratings - move tests (6.75 KB, patch)
2016-08-09 16:40 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove AddRating (3.39 KB, patch)
2016-08-09 16:40 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove ModRating (3.29 KB, patch)
2016-08-09 16:40 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove DelRating (2.12 KB, patch)
2016-08-09 16:40 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove GetRating (12.52 KB, patch)
2016-08-09 16:40 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove C4::Ratings (5.35 KB, patch)
2016-08-09 16:40 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Mark out the boundaries of ratings (2.41 KB, patch)
2016-08-09 16:41 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17089: Improve errors handling (2.71 KB, patch)
2016-08-09 16:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes (3.64 KB, patch)
2016-08-12 08:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - move tests (6.79 KB, patch)
2016-08-12 08:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove AddRating (3.44 KB, patch)
2016-08-12 08:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove ModRating (3.34 KB, patch)
2016-08-12 08:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove DelRating (2.17 KB, patch)
2016-08-12 08:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove GetRating (12.56 KB, patch)
2016-08-12 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Koha::Ratings - Remove C4::Ratings (5.40 KB, patch)
2016-08-12 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Mark out the boundaries of ratings (2.46 KB, patch)
2016-08-12 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: Improve errors handling (2.76 KB, patch)
2016-08-12 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17089: [QA Follow-up] Add some test descriptions to Ratings.t (1.56 KB, patch)
2016-08-12 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-08-09 13:39:06 UTC
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
Comment 1 Jonathan Druart 2016-08-09 14:13:46 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-08-09 14:13:50 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-08-09 14:13:59 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-08-09 14:14:05 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-08-09 14:14:09 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-08-09 14:14:12 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-08-09 14:14:35 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-08-09 14:14:38 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2016-08-09 15:19:57 UTC
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
Comment 10 Jonathan Druart 2016-08-09 16:07:20 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2016-08-09 16:40:14 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2016-08-09 16:40:21 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2016-08-09 16:40:28 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2016-08-09 16:40:34 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2016-08-09 16:40:40 UTC Comment hidden (obsolete)
Comment 16 Owen Leonard 2016-08-09 16:40:46 UTC Comment hidden (obsolete)
Comment 17 Owen Leonard 2016-08-09 16:40:54 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2016-08-09 16:41:02 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2016-08-09 16:41:08 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2016-08-12 07:32:42 UTC
QA: Looking here now.
Comment 21 Marcel de Rooy 2016-08-12 08:18:32 UTC
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>
Comment 22 Marcel de Rooy 2016-08-12 08:18:43 UTC
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>
Comment 23 Marcel de Rooy 2016-08-12 08:18:49 UTC
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>
Comment 24 Marcel de Rooy 2016-08-12 08:18:52 UTC
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>
Comment 25 Marcel de Rooy 2016-08-12 08:18:56 UTC
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>
Comment 26 Marcel de Rooy 2016-08-12 08:19:00 UTC
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>
Comment 27 Marcel de Rooy 2016-08-12 08:19:04 UTC
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>
Comment 28 Marcel de Rooy 2016-08-12 08:19:07 UTC
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>
Comment 29 Marcel de Rooy 2016-08-12 08:19:11 UTC
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>
Comment 30 Marcel de Rooy 2016-08-12 08:19:14 UTC
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>
Comment 31 Marcel de Rooy 2016-08-12 08:20:42 UTC
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
Comment 32 Jonathan Druart 2016-08-12 08:37:56 UTC
(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 %][% "&amp;sort_by=" _ sort_by |url %][% END %]

If you find something easier to write/read I will be happy to know it :)
Comment 33 Kyle M Hall 2016-09-09 10:42:02 UTC
Pushed to master for 16.11, thanks Jonathan!