Bug 11279 - Quote of the day feature won't pick a quote
Summary: Quote of the day feature won't pick a quote
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Katrin Fischer
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-21 15:34 UTC by Katrin Fischer
Modified: 2015-06-04 23:23 UTC (History)
5 users (show)

See Also:
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 (3.50 KB, patch)
2013-12-12 20:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11279: Improve how a new quote of the day is selected (1.84 KB, patch)
2013-12-12 20:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11279: regression test for picking random quote (3.58 KB, patch)
2013-12-19 00:10 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11279: Improve how a new quote of the day is selected (2.45 KB, patch)
2013-12-19 00:12 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11279: regression test for picking random quote (3.64 KB, patch)
2013-12-19 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11279: Improve how a new quote of the day is selected (2.51 KB, patch)
2013-12-19 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11279: Specific case @quotes <= 1 is useless (3.25 KB, patch)
2013-12-19 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-11-21 15:34:46 UTC
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         }
Comment 1 Chris Nighswonger 2013-11-21 16:17:05 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.
Comment 2 Katrin Fischer 2013-12-12 12:05:57 UTC
I think I have a working patch, now trying to figure out some unit tests :)
Comment 3 Chris Nighswonger 2013-12-12 12:40:36 UTC
Yay Katrin!
Comment 4 Katrin Fischer 2013-12-12 20:02:13 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2013-12-12 20:02:38 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2013-12-12 20:03:00 UTC
Feedback welcome :)
Comment 7 Galen Charlton 2013-12-19 00:10:27 UTC Comment hidden (obsolete)
Comment 8 Galen Charlton 2013-12-19 00:12:42 UTC Comment hidden (obsolete)
Comment 9 Galen Charlton 2013-12-19 00:13:34 UTC
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.
Comment 10 Katrin Fischer 2013-12-19 06:49:55 UTC
Hi Galen, thx for testing and fixing it :)
Comment 11 Jonathan Druart 2013-12-19 10:25:15 UTC
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>
Comment 12 Jonathan Druart 2013-12-19 10:25:24 UTC
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>
Comment 13 Jonathan Druart 2013-12-19 10:25:32 UTC
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).
Comment 14 Jonathan Druart 2013-12-19 10:27:46 UTC
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.
Comment 15 Galen Charlton 2013-12-19 15:08:41 UTC
Pushed to master, along with Jonathan's follow-up.  Thanks, Katrin!
Comment 16 Fridolin Somers 2014-01-27 12:40:23 UTC
Patches pushed to 3.14.x, will be in 3.14.3
Comment 17 Tomás Cohen Arazi 2014-02-07 17:12:51 UTC
This patch has been pushed to 3.12.x, will be in 3.12.11.

Thanks Katrin!