Bug 17089

Summary: Move the star ratings related code to Koha::Ratings
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: kyle, m.de.rooy
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15449    
Attachments: Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes
Bug 17089: Koha::Ratings - move tests
Bug 17089: Koha::Ratings - Remove AddRating
Bug 17089: Koha::Ratings - Remove ModRating
Bug 17089: Koha::Ratings - Remove DelRating
Bug 17089: Koha::Ratings - Remove GetRating
Bug 17089: Koha::Ratings - Remove C4::Ratings
Bug 17089: Mark out the boundaries of ratings
Bug 17089: Improve errors handling
[SIGNED-OFF] Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes
[SIGNED-OFF] Bug 17089: Koha::Ratings - move tests
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove AddRating
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove ModRating
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove DelRating
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove GetRating
[SIGNED-OFF] Bug 17089: Koha::Ratings - Remove C4::Ratings
[SIGNED-OFF] Bug 17089: Mark out the boundaries of ratings
[SIGNED-OFF] Bug 17089: Improve errors handling
Bug 17089: Koha::Ratings - Add Koha::Rating[s] classes
Bug 17089: Koha::Ratings - move tests
Bug 17089: Koha::Ratings - Remove AddRating
Bug 17089: Koha::Ratings - Remove ModRating
Bug 17089: Koha::Ratings - Remove DelRating
Bug 17089: Koha::Ratings - Remove GetRating
Bug 17089: Koha::Ratings - Remove C4::Ratings
Bug 17089: Mark out the boundaries of ratings
Bug 17089: Improve errors handling
Bug 17089: [QA Follow-up] Add some test descriptions to Ratings.t

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!