Bug 5668 - Star ratings in the opac
Summary: Star ratings in the opac
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Mason James
QA Contact: Bugs List
URL:
Keywords:
: 7954 (view as bug list)
Depends on:
Blocks: 1598
  Show dependency treegraph
 
Reported: 2011-01-30 21:07 UTC by Chris Cormack
Modified: 2013-12-05 20:01 UTC (History)
11 users (show)

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


Attachments
search screenshot (222.71 KB, image/png)
2011-01-30 21:15 UTC, Mason James
Details
detail.pl screenshot (172.43 KB, image/png)
2011-01-30 21:16 UTC, Mason James
Details
Bug 5668 - star ratings in the OPAC (44.18 KB, patch)
2011-08-23 00:39 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 5668 - adds the 'delete.gif' image (1.59 KB, patch)
2011-08-23 00:44 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 5668 - star ratings in the OPAC (45.50 KB, patch)
2011-08-25 00:16 UTC, Robin Sheat
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5668 - star ratings in the OPAC (45.23 KB, patch)
2011-08-25 01:08 UTC, Nicole C. Engard
Details | Diff | Splinter Review
patch (52.29 KB, patch)
2011-10-08 10:55 UTC, Mason James
Details | Diff | Splinter Review
pix (107.25 KB, image/png)
2011-10-08 11:16 UTC, Mason James
Details
Bug 5668 - fix comments in the SQL definition so MySQL doesn't choke (1.54 KB, patch)
2011-10-11 02:34 UTC, Robin Sheat
Details | Diff | Splinter Review
new patch (50.94 KB, patch)
2012-01-12 22:29 UTC, Mason James
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5668 - Star ratings in the opac (51.44 KB, patch)
2012-01-15 22:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5668: Follow up - Star ratings (3.25 KB, patch)
2012-01-15 22:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5668: Follow up - Star ratings (3.31 KB, patch)
2012-01-18 12:59 UTC, Mason James
Details | Diff | Splinter Review
patch (37.23 KB, patch)
2012-02-13 04:51 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (37.29 KB, patch)
2012-02-22 06:30 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (35.35 KB, patch)
2012-03-06 05:58 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.92 KB, patch)
2012-03-14 05:55 UTC, Mason James
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5668 - Star ratings in the opac (58.96 KB, patch)
2012-03-14 12:53 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.69 KB, patch)
2012-03-19 17:29 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.95 KB, patch)
2012-03-20 04:23 UTC, Mason James
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.56 KB, patch)
2012-03-20 15:15 UTC, Mason James
Details | Diff | Splinter Review
Enh 5668 Followup: only run borrower-specific unit tests if borrowers exist (3.17 KB, patch)
2012-03-22 02:03 UTC, Ian Walls
Details | Diff | Splinter Review
Enh 5668 Followup: fix BakerTaylorContentURL typo (1.17 KB, patch)
2012-03-22 13:30 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.73 KB, patch)
2012-03-22 16:40 UTC, Mason James
Details | Diff | Splinter Review
[PASSED QA] Bug 5668 - Star ratings in the opac (57.56 KB, patch)
2012-03-22 22:15 UTC, Ian Walls
Details | Diff | Splinter Review
Attachment to Bug 5668 - Star ratings in the opac (1007 bytes, patch)
2012-03-23 16:57 UTC, Mason James
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.45 KB, patch)
2012-03-30 03:49 UTC, Mason James
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.45 KB, patch)
2012-03-30 03:50 UTC, Mason James
Details | Diff | Splinter Review
Bug 5668 - Star ratings in the opac (58.82 KB, patch)
2012-03-30 06:18 UTC, Mason James
Details | Diff | Splinter Review
Bug 5668 Error during during insall with web installer, Step 3 (1.70 KB, patch)
2012-04-12 20:29 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5668 Error during during insall with web installer, Step 3 (2.25 KB, patch)
2012-04-13 05:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5668 Error during during insall with web installer, Step 3 (2.35 KB, patch)
2012-04-13 05:54 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-01-30 21:07:08 UTC
Allow logged in users to submit ratings with a star.
Comment 1 Mason James 2011-01-30 21:15:58 UTC
Created attachment 3034 [details]
search screenshot
Comment 2 Mason James 2011-01-30 21:16:40 UTC
Created attachment 3035 [details]
detail.pl screenshot
Comment 3 Nicole C. Engard 2011-01-30 21:44:05 UTC
Woo Hoo! :) Love it
Comment 4 Katrin Fischer 2011-04-25 00:30:25 UTC
What is the status of this bug?
Comment 5 Robin Sheat 2011-08-17 03:11:20 UTC
I've got a star ratings patch (I think the same one from here) that I'm going to polish up and send upstream.
Comment 6 Mason James 2011-08-22 23:34:20 UTC
(In reply to comment #5)
> I've got a star ratings patch (I think the same one from here) that I'm going
> to polish up and send upstream.

ooh sorry Robin - ive been working on cleaning this feature too, 

http://git.kohaaloha.com/?p=head-dev/.git;a=commit;h=f8d41ba66a6f6e196637a79da29320244ed0a7a6

the good news

mines 99% done, with pod and units tests. and a few other general improvements, too

i've have this ready in my repo for 2 weeks, but got too busy to update the bug

sorry about that, and thanks for your help
i owe you yet another favour :)
Comment 7 Robin Sheat 2011-08-23 00:39:15 UTC Comment hidden (obsolete)
Comment 8 Robin Sheat 2011-08-23 00:44:37 UTC Comment hidden (obsolete)
Comment 9 Nicole C. Engard 2011-08-24 16:18:01 UTC
Notes:

1. White space errors

Applying: Bug 5668 - star ratings in the OPAC
/home/nengard/kohaclone/.git/rebase-apply/patch:210: trailing whitespace.
is 0, then the rating will be deleted. If the value is out of the range of 
/home/nengard/kohaclone/.git/rebase-apply/patch:225: space before tab in indent.
        carp "Attempted to add a rating for borrower number 0";
/home/nengard/kohaclone/.git/rebase-apply/patch:226: space before tab in indent.
        return;
/home/nengard/kohaclone/.git/rebase-apply/patch:354: trailing whitespace.

/home/nengard/kohaclone/.git/rebase-apply/patch:358: trailing whitespace.

warning: squelched 63 whitespace errors
warning: 68 lines add whitespace errors.


2. In the sys pref it says "Dont't" - we need to remove the first 't'

3. When you're not logged in you can click on the stars on the search results and it puts the starts in but says (0 votes) - it shouldn't be clickable if you're not logged in (like when you're on the detail page)

4. On the detail page it might be nice (not necessary) to say - log in to rate this title - just like we do for comments and tags.

Marking Failed QA because of #3 which is a misleading problem - but the other things are here in case they too can be fixed.

I have only tested patch 1 - not the follow up with the delete image.
Comment 10 Robin Sheat 2011-08-25 00:16:54 UTC Comment hidden (obsolete)
Comment 11 Robin Sheat 2011-08-25 00:17:34 UTC
This should fix up the issues you mention, although probably not the whitespace ones. It also includes the delete.gif.
Comment 12 Nicole C. Engard 2011-08-25 00:19:58 UTC
Does not apply:

Bug 5668 - star ratings in the OPAC
Apply? [yn] y

Applying: Bug 5668 - star ratings in the OPAC
/home/nengard/kohaclone/.git/rebase-apply/patch:227: space before tab in indent.
        carp "Attempted to add a rating for borrower number 0";
/home/nengard/kohaclone/.git/rebase-apply/patch:228: space before tab in indent.
        return;
/home/nengard/kohaclone/.git/rebase-apply/patch:356: trailing whitespace.

/home/nengard/kohaclone/.git/rebase-apply/patch:360: trailing whitespace.

/home/nengard/kohaclone/.git/rebase-apply/patch:364: trailing whitespace.

error: patch failed: C4/Auth.pm:347
error: C4/Auth.pm: patch does not apply
error: patch failed: C4/Output.pm:44
error: C4/Output.pm: patch does not apply
error: C4/Ratings.pm: already exists in index
error: patch failed: installer/data/mysql/kohastructure.sql:2612
error: installer/data/mysql/kohastructure.sql: patch does not apply
error: patch failed: installer/data/mysql/updatedatabase.pl:4432
error: installer/data/mysql/updatedatabase.pl: patch does not apply
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref:268
error: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref: patch does not apply
error: koha-tmpl/opac-tmpl/prog/en/css/jquery.rating.css: already exists in index
error: koha-tmpl/opac-tmpl/prog/en/lib/jquery/plugins/jquery.rating.js: already exists in index
error: patch failed: koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt:1
error: koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt: patch does not apply
error: patch failed: koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt:8
error: koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt: patch does not apply
error: koha-tmpl/opac-tmpl/prog/images/star.gif: already exists in index
error: patch failed: opac/opac-detail.pl:37
error: opac/opac-detail.pl: patch does not apply
error: opac/opac-ratings.pl: already exists in index
error: patch failed: opac/opac-search.pl:36
error: opac/opac-search.pl: patch does not apply
Patch failed at 0001 Bug 5668 - star ratings in the OPAC
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/Bug-5668---star-ratings-in-the-OPAC-Bh45g0.patch
nengard@debian:~/kohaclone$
Comment 13 Nicole C. Engard 2011-08-25 01:08:52 UTC Comment hidden (obsolete)
Comment 14 Nicole C. Engard 2011-08-25 01:09:33 UTC
Sorry, it did apply and it does work! I love it :)
Comment 15 Robin Sheat 2011-08-25 01:25:09 UTC
I think you applied on top of the already applied patch, rather than from master, which is what caused those messages. But, all good that it's working :)
Comment 16 Ian Walls 2011-09-22 03:08:53 UTC
Some issues, in order of descending importance:

1) Clearing does not work on search results; no AJAX seems to be posted at all.  -- must fix to pass QA

2) Star ratings are rounded down (a rating of 2 and rating of 3 show a rating of 2 stars, rather than two and a half -- I believe this will be confusing to users if it's not fixed; please attempt.

3) My ratings and other peoples ratings are the same color (yellow), making it difficult to tell quickly what I've already rated -- same as 2, though a little less urgent

4) If I have rated something, my rating should be displayed in stars instead of the average rating (flip star and "your rating:" note)  -- perhaps a matter of personal preference; won't block QA

5) There is no OPAC page to tell me what materials I've rated.  -- maybe best filed as a separate enhancement... 

Very encouraging stuff, and opens the door to a Koha-based Recommendation system.
Comment 17 Mason James 2011-10-08 10:55:02 UTC Comment hidden (obsolete)
Comment 18 Mason James 2011-10-08 11:11:50 UTC
(In reply to comment #16)
> Some issues, in order of descending importance:

hi Ian 

i have subitted a patch to address your 3 BLOCKER issues...

> 1) Clearing does not work on search results; no AJAX seems to be posted at all.
>  -- must fix to pass QA

i have made the ratings 'readonly' on the search results page. 
i think the removal of this feature is wise, and will help stop people (read: teenagers) abusing the ratings system

my intention was for this to be an *experimental* feature, as i had reservations about including this feature in my patch to kc.org

> 2) Star ratings are rounded down (a rating of 2 and rating of 3 show a rating
> of 2 stars, rather than two and a half -- I believe this will be confusing to
> users if it's not fixed; please attempt.

i did attempt this enhancment, and discovered it breaks the page in no-JS  mode, which is bad...

i think it is wise to keep the ratings as whole-numbers only, until a solution is found that does not break paging viewing in no-JS mode (which will probably be another rating system?)

> 3) My ratings and other peoples ratings are the same color (yellow), making it
> difficult to tell quickly what I've already rated -- same as 2, though a little
> less urgent

the difference between my-ratings and average ratings is now clearly stated as text
 'your rating: 5, average: 3.2 (12 votes)'

is issue is now fixed
Comment 19 Mason James 2011-10-08 11:16:22 UTC
Created attachment 5794 [details]
pix
Comment 20 Mason James 2011-10-08 11:18:58 UTC
(In reply to comment #17)
> Created attachment 5792 [details] [review]
> patch

patch applied to commit cfe8b6bd9a4f1921cb728d784083300344bcf2b1
 Author: Jonathan Druart <jonathan.druart@biblibre.com> 
 Date:   Thu Apr 7 17:08:48 2011 +0200
Comment 21 Robin Sheat 2011-10-11 02:34:01 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2011-11-02 20:31:16 UTC
I am confused by the patch status here, but it seems Robin's patch still needs a sign-off? I am changing the status to that, so it does not get lost. Please change if I am wrong.
Comment 23 Magnus Enger 2011-11-04 09:53:14 UTC
I might be revelaing my utter ignorance here... Looks to me like attachment 5792 [details] [review] introduces a new file called t/test-config.txt which starts with these comments: 

# This configuration file lets the t/Makefile prepare a test koha-conf.xml file.
# It is generated by the top-level Makefile.PL.
# It is separate from the standard koha-conf.xml so that you can edit this by hand and test with different configurations.

Is this related to the star ratings? As far as I can see, the file is not referred to anywhere else in the patch...
Comment 24 Liz Rea 2011-11-04 20:31:34 UTC
It doesn't seem to matter what I do, I can't get the ratings to stick? It always says "your rating: none" and "average rating: null"

The ratings don't seem to stick when I refresh the page, either. 

(It is possible that I have some kind of corruption in my database, but please check on these issues all the same)
Comment 25 Liz Rea 2011-11-04 20:33:57 UTC
do note this introduces a dependency for Smart::Comments. You will need it for
proper testing (it won't work without it).
Comment 26 Liz Rea 2011-11-04 20:36:06 UTC
http://screencast.com/t/tt56vcqqQT38 here is a screencast of what I was experiencing.
Comment 27 Katrin Fischer 2011-12-21 06:27:34 UTC
Notes:
Both patches still apply on master, although first patch needs git am -3 instead of using git bz.
Patches also include tests.
The comment from Liz seems to be unresolved - should this be failed qa?
Comment 28 Magnus Enger 2011-12-29 19:27:38 UTC
When I run updatedatabase.pl I get this error: "DBD::mysql::db do failed: Key column 'rating_id' doesn't exist in table at installer/data/mysql/updatedatabase.pl line 4590." Looks like rating_id is declared as the primary key without actuelly being defined first? 

+ CREATE TABLE `ratings` (
+  `borrowernumber` int(11) NOT NULL,
+  `biblionumber` int(11) NOT NULL,
+  `value` tinyint(1) NOT NULL,
+  `timestamp` timestamp NOT NULL default CURRENT_TIMESTAMP,
+  PRIMARY KEY  (`rating_id`),
Comment 29 Magnus Enger 2011-12-29 19:31:19 UTC
Attachment 5833 [details] from Robin seems to define a different primary key for kohastructure.sql, but not for updatedatabase.pl: 

PRIMARY KEY  (`borrowernumber`,`biblionumber`),
Comment 30 Robin Sheat 2011-12-29 19:34:05 UTC
The dependency on Smart::Comments should be removed, and the SQL error is due to a change from having an ID column to having a composite key (as you don't really need an ID when biblio+borrower are unique :)

Not sure when I'll get time to look at this. Hopefully Mason beats me to it ;)
Comment 31 Magnus Enger 2011-12-29 19:39:44 UTC
If Smart::Comments are kept, libsmart-comments-perl should probably be added to install_misc/debian.packages?
Comment 32 Robin Sheat 2011-12-29 19:44:13 UTC
IIRC Smart::Comments is a development/debugging thing, so there's no real need nor want to keep it in the production code.
Comment 33 Magnus Enger 2011-12-29 20:13:20 UTC
Adding the syspref takes this form: 

INSERT INTO `systempreferences` VALUES ('OpacStarRatings','0',NULL,NULL,NULL)

It would be more robust if it were:

INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES ('OpacStarRatings','0',NULL,NULL,NULL)

Based on the recent comments from Liz, Robin and myself I'm marking this as failed QA.
Comment 34 Mason James 2012-01-09 02:47:59 UTC
(In reply to comment #30)
> The dependency on Smart::Comments should be removed, and the SQL error is due
> to a change from having an ID column to having a composite key (as you don't
> really need an ID when biblio+borrower are unique :)
> 
> Not sure when I'll get time to look at this. Hopefully Mason beats me to it ;)

thanks for the input everyone, 

i'll make some corrections and send a patch up this week :)
Comment 35 Mason James 2012-01-12 22:29:42 UTC Comment hidden (obsolete)
Comment 36 Mason James 2012-01-12 22:56:29 UTC
for the curious i have the ratings stuff running on a demo (user:pwd = test:testtest)

http://demo2.kohaaloha.com/cgi-bin/koha/opac-search.pl?q=the
http://demo2.kohaaloha.com/cgi-bin/koha/opac-detail.pl?biblionumber=1

to test, after successfully applying the patch...

1) run the updatedatedase.pl script
$ perl installer/data/mysql/updatedatabase.pl
Upgrade to 9.99.99.XXX done (Add 'ratings' table and 'OpacStarRatings' syspref)

2) run the test suite
$ perl  t/db_dependent/Ratings.t
1..9
ok 1 - use C4::Ratings;
ok 2 - add a rating
ok 3 - add another rating
ok 4 - update a rating
ok 5 - get a rating, with userid
ok 6 - get a rating, without userid
ok 7 - verify user's bib rating
ok 8 - delete a rating
ok 9 - delete another rating

3) set syspref 'OpacStarRatings' to 'Show star-ratings on "results and details" pages'

4) search the OPAC

fyi: this patch was created from commit 756a1f6b72c976a342b510f3cfc25105f3719cd0
Comment 37 Katrin Fischer 2012-01-15 22:05:44 UTC Comment hidden (obsolete)
Comment 38 Katrin Fischer 2012-01-15 22:05:54 UTC Comment hidden (obsolete)
Comment 39 Mason James 2012-01-18 12:59:58 UTC Comment hidden (obsolete)
Comment 40 Mason James 2012-01-18 13:05:43 UTC
(In reply to comment #39)
> Created attachment 7215 [details] [review]
> [SIGNED-OFF] Bug 5668: Follow up - Star ratings
> 
> Signed-off-by: Mason James                                                  
> 
> 1) 2 changes to 2, both tested OK

both patches 'signed-off', now to be QA-ed...
Comment 41 Jonathan Druart 2012-01-19 15:08:07 UTC
QA comment:

1/ Why did you comment some unit tests ? They are ok.

2/ You declare OpacStarRatings for template in C4::Auth, but it is only used in 2 tmpl (opac-results.tt and opac-detail.tt). Isn't it better to declare them in their respective pl file ?

3/ Why have you set the POD in the end of the module and not before each function like (nearly) everywhere in Koha ? (C4::Ratings)

4/ There is a TODO in opac/opac-ratings-ajax.pl :) Moreover, in this script, why the "use warnings" line is commented ?
+ I think the use of C4::Dates can be deleted

5/ Why don't you create a foreign key for biblionumber and borrowernumber with "on delete cascade" in the ratings table ?

6/ It appears that a more recent version of jquery.ratings.js exists (v3.13 vs v3.10) it is certainly not important, but why don't have choose the lastest ? I don't have checked the difference between both.

7/ Some useless comment lines are still present (#use Smart::Comments '####';)

8/ As said Katrin, javascript code is generated into a .pl file, isn't it possible to open an alert into the javascript code ? After returns of ajax response for example ?

Else, all works fine, great job :)

Marked as Failed QA.
Comment 42 Srdjan Jankovic 2012-02-13 04:51:50 UTC Comment hidden (obsolete)
Comment 43 Paul Poulain 2012-02-17 14:39:10 UTC
The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply".

The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix.

If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script.
If the patch applies does not apply as I think, please rebase and resubmit, thanks
Comment 44 Srdjan Jankovic 2012-02-22 06:30:46 UTC Comment hidden (obsolete)
Comment 45 Owen Leonard 2012-02-22 16:02:43 UTC
A couple of markup issues: 

<input class="auto-submit-star" type="radio"  name="rating[% biblionumber %]"  value="1" [% IF (rating_val_1) %]checked="1"[% END %] [% IF (rating_readonly) %]disabled="disabled"[% END %]   />

'checked="1"' should be 'checked="checked"', and embedding [% IF %] inside a tag goes against a coding guideline we have to prevent errors with the translation script:

oleonard@oleonard-desktop:~/kohaclone$ prove xt
xt/permissions.t .. ok    
xt/tt_valid.t ..... 1/1 
#   Failed test 'TT syntax: not using TT directive within HTML tag'
#   at xt/tt_valid.t line 55.
# Files list: 
# opac-tmpl/prog/en/modules/opac-detail.tt: 501, 502, 503, 504, 505
# opac-tmpl/prog/en/modules/opac-results.tt: 539, 540, 541, 542, 543
# Looks like you failed 1 test of 1.

It also looks to me like several of the issues raised in comment 41 have not been addressed.
Comment 46 Srdjan Jankovic 2012-03-06 04:34:06 UTC
This is a patch that I'm trying to upstream, so I cannot give exact answers for all:

1/ Why did you comment some unit tests ? They are ok.

Fixed

2/ You declare OpacStarRatings for template in C4::Auth, but it is only used in 2 tmpl (opac-results.tt and opac-detail.tt). Isn't it better to declare them in their respective pl file ?

I'm not sure. I will move it if requested.

3/ Why have you set the POD in the end of the module and not before each function like (nearly) everywhere in Koha ? (C4::Ratings)

I cannot see it being in the end. I've added missing doc headers.

4/ There is a TODO in opac/opac-ratings-ajax.pl :) Moreover, in this script, why the "use warnings" line is commented ?
+ I think the use of C4::Dates can be deleted

That's fixed

5/ Why don't you create a foreign key for biblionumber and borrowernumber with "on delete cascade" in the ratings table ?

Done

6/ It appears that a more recent version of jquery.ratings.js exists (v3.13 vs v3.10) it is certainly not important, but why don't have choose the lastest ? I don't have checked the difference between both.

Updated

7/ Some useless comment lines are still present (#use Smart::Comments '####';)

Renoved

8/ As said Katrin, javascript code is generated into a .pl file, isn't it possible to open an alert into the javascript code ? After returns of ajax response for example ?

I believe this has been changed

Comment 45: 'checked="1"' should be 'checked="checked"', and embedding [% IF %] inside a tag goes against a coding guideline we have to prevent errors with the translation script:

Changed checked. 
Embeding IF tags: that is a great pain. I have a workaround, but I don't really understand the issue here. "checked" needs no translation, but regardless. I think that if this is a problem for the translation script, then the script needs fixing.
Comment 47 Srdjan Jankovic 2012-03-06 05:58:19 UTC Comment hidden (obsolete)
Comment 48 Owen Leonard 2012-03-06 17:03:00 UTC
(In reply to comment #46)

> Embeding IF tags: that is a great pain. I have a workaround, but I don't
> really understand the issue here. "checked" needs no translation, but
> regardless. I think that if this is a problem for the translation script,
> then the script needs fixing.

It is a pain, but it's the way things work right now. There are still two test failures:

11:44 ~/kohaclone (t-bug-5668 $% u+1)$ prove xt
xt/permissions.t .. ok    
xt/tt_valid.t ..... 1/1 
#   Failed test 'TT syntax: not using TT directive within HTML tag'
#   at xt/tt_valid.t line 55.
# Files list: 
# opac-tmpl/prog/en/modules/opac-detail.tt: 503
# opac-tmpl/prog/en/modules/opac-results.tt: 541
# Looks like you failed 1 test of 1.
xt/tt_valid.t ..... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests
Comment 49 Srdjan Jankovic 2012-03-07 00:18:05 UTC
Yes, it uses ?. I thought I could trick, it but the script is too clever for me :)
I would change it if there was no other way, but the whole thing *is* wrong:
* ? (or IF, does not matter) is used to set an attribute conditionally; that is not such a rare occasion
* in this particular case, we have 2 conditional attributes, which brings us to one line being replaced with four; I hate to think of a poor soul that would have to deal with three conditional attributes

I will not make any changes now, because Mason James is working on an improved version, so we'll deal with it when the new version is ready.

BTW, how do you set that DO-NOT-USE status?
Comment 50 Mason James 2012-03-14 04:44:36 UTC
so heres my patch... with all the outstanding issued resolved ;)


1/ Why did you comment some unit tests ? They are ok.
FIXED

2/ You declare OpacStarRatings for template in C4::Auth, but it is only used in 2 tmpl (opac-results.tt and opac-detail.tt). Isn't it better to declare them in their respective pl file ?
FIXED

3/ Why have you set the POD in the end of the module and not before each function like (nearly) everywhere in Koha ? (C4::Ratings)
FIXED

4/ There is a TODO in opac/opac-ratings-ajax.pl :) 
FIXED

Moreover, in this script, why the "use warnings" line is commented ?
FIXED

I think the use of C4::Dates can be deleted
FIXED

5/ Why don't you create a foreign key for biblionumber and borrowernumber with "on delete cascade" in the ratings table ?
FIXED

6/ It appears that a more recent version of jquery.ratings.js exists (v3.13 vs v3.10) it is certainly not important, but why don't have choose the lastest ? I don't have checked the difference between both.
FIXED

7/ Some useless comment lines are still present (#use Smart::Comments '####';)
FIXED

8/ As said Katrin, javascript code is generated into a .pl file, isn't it possible to open an alert into the javascript code ? After returns of ajax response for example ?
FIXED

A couple of markup issues: 
<input class="auto-submit-star" type="radio"  name="rating[% biblionumber %]"  value="1" [% IF (rating_val_1) %]checked="1"[% END %] [% IF (rating_readonly) %]disabled="disabled"[% END %]   />
FIXED

'checked="1"' should be 'checked="checked"', and embedding [% IF %] inside a tag goes against a coding guideline we have to prevent errors with the translation script:
FIXED

oleonard@oleonard-desktop:~/kohaclone$ prove xt
# Looks like you failed 1 test of 1.

FIXED
-------------------------------------
mason@xen1:~/g/head$ prove xt
xt/permissions.t .. ok    
xt/tt_valid.t ..... ok   
All tests successful.
Files=2, Tests=27,  3 wallclock secs ( 0.02 usr  0.01 sys +  2.92 cusr  0.03 csys =  2.98 CPU)
Result: PASS  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<
-------------------------------------
Comment 51 Mason James 2012-03-14 05:55:44 UTC Comment hidden (obsolete)
Comment 52 Mason James 2012-03-14 06:40:00 UTC
updated STATUS to 'needs sign-off'... now ready for QA?
Comment 53 Nicole C. Engard 2012-03-14 12:53:22 UTC Comment hidden (obsolete)
Comment 54 Ian Walls 2012-03-19 17:29:26 UTC Comment hidden (obsolete)
Comment 55 Ian Walls 2012-03-19 17:32:29 UTC
I've rebased this as part of the QA process, and figured I'd attach it here since I'm at a stopping point for the moment.

This seems to work alright, but t/db_dependent/Ratings.t fails on me.  There is a note that it will fail on any database that doesn't have a blank 'ratings' table... this seems like a less than useful test to me.  And, it failed both with and without lines in the ratings table.

I'm not sure if this is a problem with the code or the unit test.  Could someone take a look before I resume QA?
Comment 56 Mason James 2012-03-20 04:22:07 UTC
(In reply to comment #55)
> I've rebased this as part of the QA process, and figured I'd attach it here
> since I'm at a stopping point for the moment.
> 
> This seems to work alright, but t/db_dependent/Ratings.t fails on me.  There
> is a note that it will fail on any database that doesn't have a blank
> 'ratings' table... this seems like a less than useful test to me.  And, it
> failed both with and without lines in the ratings table.
> 
> I'm not sure if this is a problem with the code or the unit test.  Could
> someone take a look before I resume QA?


oops - i was using non-existant borrowers to test (10001, 10002, etc...)

ive corrected the test, which now uses valid 'test_database' borrowers (102,103)

and i've removed the incorrect "test will fail on any database that doesn't have a blank 'ratings' table" statement


all ratings tests now pass
------------------------------------------------------
mason@xen1:~/g/head$ perl   t/db_dependent/Ratings.t 
1..12
ok 1 - use C4::Ratings;
ok 2 - add a rating
ok 3 - add another rating
ok 4 - update a rating
ok 5 - get a rating, with borrowernumber
ok 6 - get a rating, without borrowernumber
ok 7 - get a bib's average(float) rating
ok 8 - get a bib's average(int) rating
ok 9 - get a bib's total number of ratings
ok 10 - verify user's bib rating
ok 11 - delete a rating
ok 12 - delete another rating
Comment 57 Mason James 2012-03-20 04:23:41 UTC Comment hidden (obsolete)
Comment 58 Mason James 2012-03-20 15:15:29 UTC Comment hidden (obsolete)
Comment 59 Ian Walls 2012-03-22 02:02:40 UTC
Looking good, almost there.

Problems:

Unit tests don't work on other database than the testing db, leading to errors in auto testing suites.  --- I have a followup patch to fix this, will attach shortly

Breaks Baker & Taylor on opac-details.tt (easy fix, change BakerTaylorContenturl to BakerTaylorContentURL)

System preference is classed as "Choice", between 'yes', 'no' and 'details', but the default value is 0, and the code tests on 1|details.  Need to sort out this to consistent terms, either all textual (recommended) or all numeric.


Non-problems:

Lots of extra whitespace lines are introduced... no need for them, but no problem, either.  I'm going on the notion that it's better to avoid unnecessary changes, even if they're benign.

opac-ratings-ajax.pl references opac-detail.tmpl instead of opac-detail.tt... no biggie, that happens a lot, but better to clean it up as new code is created.

COinSinOPACResults gets moved in opac-search.pl... again, unnecessary and benign, just observing.
Comment 60 Ian Walls 2012-03-22 02:03:11 UTC Comment hidden (obsolete)
Comment 61 Ian Walls 2012-03-22 13:30:44 UTC Comment hidden (obsolete)
Comment 62 Mason James 2012-03-22 14:51:00 UTC
> 
> System preference is classed as "Choice", between 'yes', 'no' and 'details',
> but the default value is 0, and the code tests on 1|details.  Need to sort
> out this to consistent terms, either all textual (recommended) or all
> numeric.

i'll fix this issue now

> 
> opac-ratings-ajax.pl references opac-detail.tmpl instead of
> opac-detail.tt... no biggie, that happens a lot, but better to clean it up
> as new code is created.

i'll fix this issue now, too


Ian, thanks for your help on this one :)
Comment 63 Mason James 2012-03-22 16:10:48 UTC
(In reply to comment #62)
> > 
> > System preference is classed as "Choice", between 'yes', 'no' and 'details',
> > but the default value is 0, and the code tests on 1|details.  Need to sort
> > out this to consistent terms, either all textual (recommended) or all
> > numeric.
> 
> i'll fix this issue now

hmm, this is almost a wont-fix, for me
agreed, the numeric values idea is bad bad bad

some bug (or designed feature?) in preferences.pl makes it currently impossible to save the literal strings 'yes' and 'no' to the sys-prefs table

either preferences.pl (or mysql itself?), seems to transform the values 'yes' and 'no', to 1 or 0 when saving

i think the only work around here, is to use some-other values (disable|details|all)? rather than (no|details|yes)

thats the best i can do...


ps: its also the reason why the goofy '1|details' syntax was used in the first place, i had simply forgotten the initial problem
Comment 64 Mason James 2012-03-22 16:39:04 UTC
(In reply to comment #62)
> > 
> > System preference is classed as "Choice", between 'yes', 'no' and 'details',
> > but the default value is 0, and the code tests on 1|details.  Need to sort
> > out this to consistent terms, either all textual (recommended) or all
> > numeric.

FIXED
choices are now renamed to 'disable|details|all'

> > opac-ratings-ajax.pl references opac-detail.tmpl instead of
> > opac-detail.tt... no biggie, that happens a lot, but better to clean it up
> > as new code is created.

FIXED

FYI: both of Ian's following patchs are *incorperated* in my patch 
 - Enh 5668 Followup: only run borrower-specific unit tests if borrowers exist 
 - Enh 5668 Followup: fix BakerTaylorContentURL typo
Comment 65 Mason James 2012-03-22 16:40:07 UTC Comment hidden (obsolete)
Comment 66 Ian Walls 2012-03-22 21:57:09 UTC
Okay, now we're good.  Marking Passed QA.

One small followup (forthcoming): this introduces an unnecessary dependency on C4::Serials in opac-detail.pl.  No big thing, but as we're actively working to streamline what we load on each page, this should be stripped out.
Comment 67 Ian Walls 2012-03-22 22:15:30 UTC Comment hidden (obsolete)
Comment 68 Ian Walls 2012-03-22 22:18:11 UTC
Comment on attachment 8537 [details] [review]
Bug 5668 - Star ratings in the opac

Had to rebase anyway, so the above mention change (remove use C4::Serials line) is followed into rebase, along with some whitespace cleanup
Comment 69 Paul Poulain 2012-03-23 09:03:54 UTC
QA comment:
I checked perlcritic and there's an error:
opac/opac-ratings-ajax.pl: Subroutine prototypes used at line 107, column 1.  See page 194 of PBP.  (Severity: 5)


please fix it & switch to passed QA again.
Comment 70 Mason James 2012-03-23 16:56:21 UTC
(In reply to comment #69)
> QA comment:
> I checked perlcritic and there's an error:
> opac/opac-ratings-ajax.pl: Subroutine prototypes used at line 107, column 1.
> See page 194 of PBP.  (Severity: 5)
> 
> 
> please fix it & switch to passed QA again.

fixed - additional one-line patch attached
Comment 71 Mason James 2012-03-23 16:57:00 UTC Comment hidden (obsolete)
Comment 72 Paul Poulain 2012-03-26 13:50:41 UTC
sorry, there's still a problem:
* many files now have a conflict (just pushed bug 7470, that is probably the cause)
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
CONFLICT (content): Merge conflict in opac/opac-search.pl

I thought they were easy to solve, but seems I can't merge opac-search.pl easily. After trying, I get a :
syntax error at opac/opac-search.pl line 746, near "elsif"
Missing right curly or square bracket at opac/opac-search.pl line 798, at end of line


I prefer not doing something wrong, and ask for a rebased patch, really sorry

(Even if I "feature freeze" today, i'll accept this patch if you provide a rebased version this week)
Comment 73 Mason James 2012-03-30 02:42:11 UTC
(In reply to comment #68)
> Comment on attachment 8537 [details] [review]
> Bug 5668 - Star ratings in the opac
> 
> Had to rebase anyway, so the above mention change (remove use C4::Serials
> line) is followed into rebase, along with some whitespace cleanup

hmm, Ian, it looks like you grabbed and rebased an old patch of mine, rather than my latest patch (attachment 8537 [details] [review])

http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=8537&action=edit
Comment 74 Mason James 2012-03-30 02:44:43 UTC
Comment on attachment 8548 [details] [review]
[PASSED QA] Bug 5668 - Star ratings in the opac

this patch was a rebase from an older patch, marking this obsolete
Comment 75 Mason James 2012-03-30 03:49:00 UTC
(In reply to comment #72)
> sorry, there's still a problem:
> * many files now have a conflict (just pushed bug 7470, that is probably the
> cause)
> I prefer not doing something wrong, and ask for a rebased patch, really sorry
> 
> (Even if I "feature freeze" today, i'll accept this patch if you provide a
> rebased version this week)

heya Paul, no problem :)

here's the rebased patch, 
tested and applied to commit eb3dc448d2378e01a524d082fc6389484b0cc7d8
Comment 76 Mason James 2012-03-30 03:49:26 UTC Comment hidden (obsolete)
Comment 77 Mason James 2012-03-30 03:50:30 UTC Comment hidden (obsolete)
Comment 78 Mason James 2012-03-30 06:18:34 UTC Comment hidden (obsolete)
Comment 79 Paul Poulain 2012-04-10 12:59:31 UTC
patch pushed, string freeze very very soon ;-)
Comment 80 Marc Véron 2012-04-12 18:51:42 UTC
I got the following Error in the Web Installer (Step 3)

ERROR 1064 (42000) at line 2821: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '- biblionumber int(11) NOT NULL, - rating_value tinyint(1) NOT NULL, - ' at line 2 

The sql code near the Error is:
--------------------
CREATE TABLE ratings (
    borrowernumber int(11) NOT NULL, --- the borrower this rating is for
    biblionumber int(11) NOT NULL, --- the biblio it's for
    rating_value tinyint(1) NOT NULL, --- the rating, from 1-5
    timestamp timestamp NOT NULL default CURRENT_TIMESTAMP,
    PRIMARY KEY  (borrowernumber,biblionumber),
    CONSTRAINT ratings_ibfk_1 FOREIGN KEY (borrowernumber) REFERENCES borrowers (borrowernumber) ON DELETE CASCADE ON UPDATE CASCADE,
    CONSTRAINT ratings_ibfk_2 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
---------------------

The error is due to the comments (--- instead of --)


Additional issue:

After fixing the error, the scipt complains about an existing table pending_offline_operations

There should be at line:
DROP TABLE IF EXISTS `pending_offline_operations`;
...at the begin of the section:
-- Table structure for table `pending_offline_operations`
Comment 81 Marc Véron 2012-04-12 18:52:48 UTC
I got the following Error in the Web Installer (Step 3)

ERROR 1064 (42000) at line 2821: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '- biblionumber int(11) NOT NULL, - rating_value tinyint(1) NOT NULL, - ' at line 2 

The sql code near the Error is:
--------------------
CREATE TABLE ratings (
    borrowernumber int(11) NOT NULL, --- the borrower this rating is for
    biblionumber int(11) NOT NULL, --- the biblio it's for
    rating_value tinyint(1) NOT NULL, --- the rating, from 1-5
    timestamp timestamp NOT NULL default CURRENT_TIMESTAMP,
    PRIMARY KEY  (borrowernumber,biblionumber),
    CONSTRAINT ratings_ibfk_1 FOREIGN KEY (borrowernumber) REFERENCES borrowers (borrowernumber) ON DELETE CASCADE ON UPDATE CASCADE,
    CONSTRAINT ratings_ibfk_2 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
---------------------

The error is due to the comments (--- instead of --)


Additional issue:

After fixing the error, the scipt complains about an existing table pending_offline_operations

There should be at line:
DROP TABLE IF EXISTS `pending_offline_operations`;
...at the begin of the section:
-- Table structure for table `pending_offline_operations`
Comment 82 Marc Véron 2012-04-12 20:29:03 UTC Comment hidden (obsolete)
Comment 83 Mason James 2012-04-12 21:29:00 UTC
(In reply to comment #81)
> I got the following Error in the Web Installer (Step 3)
> 
> ERROR 1064 (42000) at line 2821: You have an error in your SQL syntax; check
> 
> The error is due to the comments (--- instead of --)
> 


bah, Robin had spotted and fixed that typo before

thanks Marc!
Comment 84 Katrin Fischer 2012-04-13 05:19:27 UTC
Comment on attachment 8706 [details] [review]
Bug 5668 - Star ratings in the opac

pushed :)
Comment 85 Katrin Fischer 2012-04-13 05:21:19 UTC
Hi Mason, can you attach your signed off patch to the bug please? Thank you! Also updating severity, because currently it breaks installation.
Comment 86 Katrin Fischer 2012-04-13 05:39:50 UTC Comment hidden (obsolete)
Comment 87 Mason James 2012-04-13 05:54:01 UTC
Created attachment 9164 [details] [review]
[SIGNED-OFF] Bug 5668 Error during during insall with web installer, Step 3

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Signed-off-by: Mason James <mtj at kohaaloha.com>
Before patch web installer dies with error message on creating
database tables:
ERROR 1064 (42000) at line 2821: You have an error in your SQL
syntax; check the manual that corresponds to your MySQL server
version for the right syntax to use near '- biblionumber int(11)
NOT NULL, - rating_value tinyint(1) NOT NULL, - ' at line 2

After patch installation completes normally. I also checked
that tables ratings and pending_offline_operations were
created correctly.

Note: This is Marc Veron's first patch for Koha - congratulations!
Comment 88 Mason James 2012-04-13 05:55:06 UTC
(In reply to comment #85)
> Hi Mason, can you attach your signed off patch to the bug please? Thank you!
> Also updating severity, because currently it breaks installation.

yep, done :)
Comment 89 Katrin Fischer 2012-04-13 09:39:34 UTC
*** Bug 7954 has been marked as a duplicate of this bug. ***
Comment 90 Jonathan Druart 2012-04-13 09:50:56 UTC
Passed QA.

Paul, could you remove backquotes on the following line before pushing:
+DROP TABLE IF EXISTS `pending_offline_operations`;
Comment 91 Paul Poulain 2012-04-13 13:32:14 UTC
follow-up to fix error during installation pushed