[Bug 11279] New: Quote of the day feature won't pick a quote
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Bug ID: 11279 Summary: Quote of the day feature won't pick a quote Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Tools Assignee: gmcharlt@gmail.com Reporter: katrin.fischer@bsz-bw.de QA Contact: testopia@bugs.koha-community.org CC: cnighswonger@foundations.edu In order to select a new quote the feature does a count on the quotes table, then it tries to select an id in that range. But it can easily happen that you get out of the range with your ids. In my example I had loaded the English sample file, the library decided to add new quotes and deleted all samples. So the smallest id in the table ended up to be around 40, with less than 40 entries total. We allow deleting entries, there is no way to edit the id. Code in Koha.pm: 1439 unless ($quote) { # if there are not matches, choose a random quote 1440 # get a list of all available quote ids 1441 $sth = C4::Context->dbh->prepare('SELECT count(*) FROM quotes;'); 1442 $sth->execute; 1443 my $range = ($sth->fetchrow_array)[0]; 1444 if ($range > 1) { 1445 # chose a random id within that range if there is more than one quote 1446 my $id = int(rand($range)); 1447 # grab it 1448 $query = 'SELECT * FROM quotes WHERE id = ?;'; 1449 $sth = C4::Context->dbh->prepare($query); 1450 $sth->execute($id); 1451 } -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #1 from Chris Nighswonger <cnighswonger@foundations.edu> --- Wow. The guy who wrote that code should be banned from eating choclate for a week at least! Unless Galen gets to it before me, I will fix this in the next few days. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |katrin.fischer@bsz-bw.de -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I think I have a working patch, now trying to figure out some unit tests :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Chris Nighswonger <cnighswonger@foundations.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED --- Comment #3 from Chris Nighswonger <cnighswonger@foundations.edu> --- Yay Katrin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 23493 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23493&action=edit Bug 11279: Regression test This patch adds new tests based on the db_dependent tests in t/db_dependent/Koha.t for GetDailyQuote(). Tests run on an empty database and don't require Test::Deep. Tests will fail before applying the patch for 11297, but will pass after applying the patch. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 23494 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23494&action=edit Bug 11279: Improve how a new quote of the day is selected When a lot of quotes were deleted, it's possible that no new quote will be selected. This will happen when a lot of the older quotes with low ids have been deleted. This patch improves the selection of a new quote. To test: - Load sample quotes - Delete the first half of the quotes. Note: With 34 quotes, delete the quotes with ids from 1-17 - Activate the QuoteOfTheDay system preference - Check if a quote is displayed in OPAC - Reload the page a few times, no quote should be displayed Note: make sure you don't have a quote with the current date in your quotes table before running those tests - Run 'perl t/db_dependent/Koha.t' Note: requires sample quotes! - Apply patch - Reload the OPAC start page - Verify a quote was now picked - Run 'perl t/db/dependent/Koha.t' again - all tests should still pass -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Feedback welcome :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23493|0 |1 is obsolete| | Attachment #23494|0 |1 is obsolete| | --- Comment #7 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 23662 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23662&action=edit Bug 11279: regression test for picking random quote This patch adds new tests based on the db_dependent tests in t/db_dependent/Koha.t for GetDailyQuote(). Tests run on an empty database and don't require Test::Deep. Tests will fail before applying the patch for 11297, but will pass after applying the patch. Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #8 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 23663 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23663&action=edit Bug 11279: Improve how a new quote of the day is selected If there is a gap in the id sequence for the quotes table, it is possible that no new quote will be selected. This will happen particularly when a lot of the older quotes with low ids have been deleted. This patch improves the selection of a new quote. To test: - Load sample quotes - Delete the first half of the quotes. Note: With 34 quotes, delete the quotes with ids from 1-17 - Activate the QuoteOfTheDay system preference - Check if a quote is displayed in OPAC - Reload the page a few times, no quote should be displayed Note: make sure you don't have a quote with the current date in your quotes table before running those tests - Run 'perl t/db_dependent/Koha.t' Note: requires sample quotes! - Apply patch - Reload the OPAC start page - Verify a quote was now picked - Run 'perl t/db/dependent/Koha.t' again - all tests should still pass Signed-off-by: Galen Charlton <gmc@esilibrary.com> Patch modified to use "LIMIT 1 OFFSET ?" rather than "LIMIT ?, 1"; the latter construction does not work in PostgreSQL. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |gmcharlt@gmail.com --- Comment #9 from Galen Charlton <gmcharlt@gmail.com> --- I've signed off on it -- note the modification to deal with the fact that Pg doesn't support limit x,y -- however, both MySQL and Pg support LIMIT y OFFSET x. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Galen, thx for testing and fixing it :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23662|0 |1 is obsolete| | --- Comment #11 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 23668 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23668&action=edit Bug 11279: regression test for picking random quote This patch adds new tests based on the db_dependent tests in t/db_dependent/Koha.t for GetDailyQuote(). Tests run on an empty database and don't require Test::Deep. Tests will fail before applying the patch for 11297, but will pass after applying the patch. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23663|0 |1 is obsolete| | --- Comment #12 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 23669 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23669&action=edit Bug 11279: Improve how a new quote of the day is selected If there is a gap in the id sequence for the quotes table, it is possible that no new quote will be selected. This will happen particularly when a lot of the older quotes with low ids have been deleted. This patch improves the selection of a new quote. To test: - Load sample quotes - Delete the first half of the quotes. Note: With 34 quotes, delete the quotes with ids from 1-17 - Activate the QuoteOfTheDay system preference - Check if a quote is displayed in OPAC - Reload the page a few times, no quote should be displayed Note: make sure you don't have a quote with the current date in your quotes table before running those tests - Run 'perl t/db_dependent/Koha.t' Note: requires sample quotes! - Apply patch - Reload the OPAC start page - Verify a quote was now picked - Run 'perl t/db/dependent/Koha.t' again - all tests should still pass Signed-off-by: Galen Charlton <gmc@esilibrary.com> Patch modified to use "LIMIT 1 OFFSET ?" rather than "LIMIT ?, 1"; the latter construction does not work in PostgreSQL. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 --- Comment #13 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 23670 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23670&action=edit Bug 11279: Specific case @quotes <= 1 is useless It is not necessary to process the case where the number of quotes. (int(rand(1)) will always produce 0). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m QA Contact|testopia@bugs.koha-communit |jonathan.druart@biblibre.co |y.org |m --- Comment #14 from Jonathan Druart <jonathan.druart@biblibre.com> --- Galen, I completely agree with your changes and with the Katrin's patch. I am under the impression the routine could be simplify, I submit a followup. Feel free to reject it if you have a doubt about it. Marked as Passed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #15 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master, along with Jonathan's follow-up. Thanks, Katrin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolyn.somers@biblibre.co | |m --- Comment #16 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Patches pushed to 3.14.x, will be in 3.14.3 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11279 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This patch has been pushed to 3.12.x, will be in 3.12.11. Thanks Katrin! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org