Summary: | Quote of the day feature won't pick a quote | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | Tools | Assignee: | Katrin Fischer <katrin.fischer> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P5 - low | CC: | cnighswonger, fridolin.somers, gmcharlt, jonathan.druart, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Attachments: |
Bug 11279: Regression test
Bug 11279: Improve how a new quote of the day is selected Bug 11279: regression test for picking random quote Bug 11279: Improve how a new quote of the day is selected Bug 11279: regression test for picking random quote Bug 11279: Improve how a new quote of the day is selected Bug 11279: Specific case @quotes <= 1 is useless |
Description
Katrin Fischer
2013-11-21 15:34:46 UTC
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! |