Allow logged in users to submit ratings with a star.
Created attachment 3034 [details] search screenshot
Created attachment 3035 [details] detail.pl screenshot
Woo Hoo! :) Love it
What is the status of this bug?
I've got a star ratings patch (I think the same one from here) that I'm going to polish up and send upstream.
(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 :)
Created attachment 5097 [details] [review] Bug 5668 - star ratings in the OPAC This takes the work done by Mason James and tidies it up a bit to work with master. This patch allows OPAC users to assign star ratings to biblios, and other users to see the average of what other people have rated.
Created attachment 5098 [details] [review] Bug 5668 - adds the 'delete.gif' image This was accidentally left off the first commit.
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.
Created attachment 5136 [details] [review] Bug 5668 - star ratings in the OPAC This takes the work done by Mason James and tidies it up a bit to work with master. This patch allows OPAC users to assign star ratings to biblios, and other users to see the average of what other people have rated.
This should fix up the issues you mention, although probably not the whitespace ones. It also includes the delete.gif.
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$
Created attachment 5137 [details] [review] [SIGNED-OFF] Bug 5668 - star ratings in the OPAC This takes the work done by Mason James and tidies it up a bit to work with master. This patch allows OPAC users to assign star ratings to biblios, and other users to see the average of what other people have rated. Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Sorry, it did apply and it does work! I love it :)
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 :)
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.
Created attachment 5792 [details] [review] patch
(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
Created attachment 5794 [details] pix
(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
Created attachment 5833 [details] [review] Bug 5668 - fix comments in the SQL definition so MySQL doesn't choke MySQL gets unhappy about SQL comments having three '-' in a row to begin them.
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.
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...
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)
do note this introduces a dependency for Smart::Comments. You will need it for proper testing (it won't work without it).
http://screencast.com/t/tt56vcqqQT38 here is a screencast of what I was experiencing.
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?
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`),
Attachment 5833 [details] from Robin seems to define a different primary key for kohastructure.sql, but not for updatedatabase.pl: PRIMARY KEY (`borrowernumber`,`biblionumber`),
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 ;)
If Smart::Comments are kept, libsmart-comments-perl should probably be added to install_misc/debian.packages?
IIRC Smart::Comments is a development/debugging thing, so there's no real need nor want to keep it in the production code.
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.
(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 :)
Created attachment 7116 [details] [review] new patch new patch, obseletes the existing two patches
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
Created attachment 7165 [details] [review] [SIGNED-OFF] Bug 5668 - Star ratings in the opac Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> 1) All test complete successfully. perl t/db_dependent/Ratings.t 2) Display works according to system preference settings OpacStarRatings = only details OpacStarRatings = no OpacStarRatings = results and details All settings work. 3) Add/change/delete ratings Adding, changing and deleting ratings works. Only logged in users can rate books. Rating works only on OPAC detail pages. Problems found: 1) New system preference is not added to sysprefs.sql 2) It seems there is some javascript code in opac-ragings-ajax.pl: + "window.alert('Your CGI session cookie ($sessid) is not current. " + . "Please refresh the page and try again.');\n"; I was wondering if this should be in the pl file, it's also not translatable.
Created attachment 7166 [details] [review] Bug 5668: Follow up - Star ratings - adds new system preference OpacStarRatings to sysprefs.sql file - changes NULL NULL to 'yes|no|details','Choice' - changes updatedatabase.pl - removes mysqlisms from the table definition - changes INSERT to make it more robust and match sysprefs.sql
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
(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...
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.
Created attachment 7604 [details] [review] patch Added foreign keys. Not sure what other things need rectifying.
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
Created attachment 7798 [details] [review] patch
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.
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.
Created attachment 8027 [details] [review] patch
(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
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?
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 <<<<<<<<<<<<<<<<<<<<<<<<<<<<< -------------------------------------
Created attachment 8199 [details] [review] Bug 5668 - Star ratings in the opac applied to commit 49c387cddf6de083343fe6c88ad9f167844521b4
updated STATUS to 'needs sign-off'... now ready for QA?
Created attachment 8202 [details] [review] [SIGNED-OFF] Bug 5668 - Star ratings in the opac applied to commit 49c387cddf6de083343fe6c88ad9f167844521b4 Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off.
Created attachment 8312 [details] [review] Bug 5668 - Star ratings in the opac applied to commit 49c387cddf6de083343fe6c88ad9f167844521b4 Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-19-12 by Ian Walls
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?
(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
Created attachment 8326 [details] [review] Bug 5668 - Star ratings in the opac applied to commit b3cae766949599c88095930b8fd37dabfc79a5cc Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-19-12 by Ian Walls
Created attachment 8359 [details] [review] Bug 5668 - Star ratings in the opac applied to commit b3cae766949599c88095930b8fd37dabfc79a5cc Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-19-12 by Ian Walls
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.
Created attachment 8459 [details] [review] Enh 5668 Followup: only run borrower-specific unit tests if borrowers exist If the borrowers 102 and 103 don't exist, skip the tests that call for them. This leaves just 2 running tests on most DBs, but this will help quiet the automated test suites, and will still run if the right data is installed.
Created attachment 8514 [details] [review] Enh 5668 Followup: fix BakerTaylorContentURL typo Restores opac-detail.tt reference to BakerTaylorContentURL, instead of 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. 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 :)
(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
(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
Created attachment 8537 [details] [review] Bug 5668 - Star ratings in the opac applied to commit b3cae766949599c88095930b8fd37dabfc79a5cc Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-19-12 by Ian Walls
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.
Created attachment 8548 [details] [review] [PASSED QA] Bug 5668 - Star ratings in the opac Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-22-12 by Ian Walls
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
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.
(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
Created attachment 8613 [details] [review] Attachment to Bug 5668 - Star ratings in the opac mason@xen1:~/g/head2$ perlcritic opac/opac-ratings-ajax.pl opac/opac-ratings-ajax.pl source OK modified: opac/opac-ratings-ajax.pl
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)
(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 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
(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
Created attachment 8704 [details] [review] Bug 5668 - Star ratings in the opac patch applied to commit eb3dc448d2378e01a524d082fc6389484b0cc7d8 Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-19-12 by Ian Walls
Created attachment 8705 [details] [review] Bug 5668 - Star ratings in the opac patch applied to commit eb3dc448d2378e01a524d082fc6389484b0cc7d8 Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-19-12 by Ian Walls
Created attachment 8706 [details] [review] Bug 5668 - Star ratings in the opac patch applied to commit eb3dc448d2378e01a524d082fc6389484b0cc7d8 Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Turned on star ratings in the opac on details and results Searched for titles - saw the stars Clicked on a title Clicked on the stars Clicked on the stars to change my rating Logged out Tried to click on stars Logged in as different user Rated items that were rated already and saw average change Changed preference to show only on detail and repeated tests Changed preference to now show stars All above tests passed. Signing off. Rebased 3-19-12 by Ian Walls
patch pushed, string freeze very very soon ;-)
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`
Created attachment 9159 [details] [review] Bug 5668 Error during during insall with web installer, Step 3
(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 on attachment 8706 [details] [review] Bug 5668 - Star ratings in the opac pushed :)
Hi Mason, can you attach your signed off patch to the bug please? Thank you! Also updating severity, because currently it breaks installation.
Created attachment 9163 [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> 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!
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!
(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 :)
*** Bug 7954 has been marked as a duplicate of this bug. ***
Passed QA. Paul, could you remove backquotes on the following line before pushing: +DROP TABLE IF EXISTS `pending_offline_operations`;
follow-up to fix error during installation pushed