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 }
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.
I think I have a working patch, now trying to figure out some unit tests :)
Yay Katrin!
Created attachment 23493 [details] [review] 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.
Created attachment 23494 [details] [review] 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
Feedback welcome :)
Created attachment 23662 [details] [review] 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>
Created attachment 23663 [details] [review] 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.
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.
Hi Galen, thx for testing and fixing it :)
Created attachment 23668 [details] [review] 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>
Created attachment 23669 [details] [review] 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>
Created attachment 23670 [details] [review] 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).
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.
Pushed to master, along with Jonathan's follow-up. Thanks, Katrin!
Patches pushed to 3.14.x, will be in 3.14.3
This patch has been pushed to 3.12.x, will be in 3.12.11. Thanks Katrin!