Bug 13914

Summary: The holds statistics report returns random data
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: ReportsAssignee: Sophie MEYNIEUX <sophie.meynieux>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: eivin, kyle, nick, sophie.meynieux
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8126
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10188
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12082
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17294
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 13914: Holds statistics report should add values from reserves and old_reserves
Bug 13914 : Fix UNION in SQL request for reserves statistics
Bug 13914 : Fix UNION in SQL request for reserves statistics
Bug 13914 : Fix UNION in SQL request for reserves statistics
Bug 13914 : Fix UNION in SQL request for reserves statistics

Description Jonathan Druart 2015-03-26 12:17:41 UTC
To reproduce:
Go on Reports › Holds statistics

Fill the form with:
Hold status: All checked (Asked Processing Waiting Satisfied Cancelled)
Hold Date From 01/01/2014 To 31/01/2014

Pickup library => empty
Holding library => empty
Home library => empty

Row is "Hold status", Column is "Pickup library".

Submit!

Look at the results

Restart mysql

Ctrl+R to resubmit the form

Look at the results

Enjoy...
Comment 1 Jonathan Druart 2015-03-26 12:33:09 UTC Comment hidden (obsolete)
Comment 2 Eivin Giske Skaaren 2015-04-26 13:01:19 UTC
I am still very new to Koha so please excuse (and explain) if I am thinking wrong here:

The query that gets fired when testing this bug looks like this:

(SELECT  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  line, reserves.branchcode col,  COUNT(*)  calculation
        FROM reserves
        LEFT JOIN borrowers USING (borrowernumber)
	 WHERE  reservedate >= '2015-04-15'  AND  reservedate <= '2015-04-30'  AND ((  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '3' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '1' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '4' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '2' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '5' ) ) GROUP BY line, col ) UNION (SELECT  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  line, old_reserves.branchcode col,  COUNT(*)  calculation
        FROM old_reserves
        LEFT JOIN borrowers USING (borrowernumber)
	 WHERE  reservedate >= '2015-04-15'  AND  reservedate <= '2015-04-30'  AND ((  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '3' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '1' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '4' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '2' )  OR (  case
					when priority>0 then 1
					when priority=0 then
						(case
						   when found='f' then 4
						   when found='w' then
						   (case
						    when cancellationdate is null then 3
							else 5
							end )
						   else 2
						 end )
				    else 6
					end  = '5' ) ) GROUP BY line, col ) ORDER BY line, col


So one way of getting line numbers that are not unique for the same col is if priority is set to >0 for an item in the old_reserves table.

Question: Should priority ever be anything else than 0 for an item in the old_reserves table?

The patch still gets the same SQL result and then tries to solve the problem by using the value of line as hash key and adds the values of  calculation from any rows with the same line value. But this results in the wrong values being added given my example with priority for an item in the old_reserves table being set to >0. The query will as in Jonathan's example create rows with line value 1 for both the reserves table and the old_reserves table, so the line value should not be used as the hash key since it will then add values from the different tables to the calculation field.

Perhaps this is better solved by getting the SQL correct for the use case, perhaps by adding a field to the result set that is unique?

Also by looking at the reservestatus I suspect that something else is wrong here since line=1 corresponds to 1-placed which should be wrong for anything in the old_reserves table. 

This patch then does not solve the problem if the things that I have assumed are correct.
Comment 3 Jonathan Druart 2015-04-27 11:18:34 UTC
(In reply to Eivin Giske Skaaren from comment #2)
> I am still very new to Koha so please excuse (and explain) if I am thinking
> wrong here:
> 
> The query that gets fired when testing this bug looks like this:

[skip]

> So one way of getting line numbers that are not unique for the same col is
> if priority is set to >0 for an item in the old_reserves table.
> 
> Question: Should priority ever be anything else than 0 for an item in the
> old_reserves table?

Yes, I think so.
At least I have some on local installation.
I suppose the priority is not updated when a reserve is canceled. So if it has a priority > 0, it will be copied as it to the old_reserves.

> Perhaps this is better solved by getting the SQL correct for the use case,
> perhaps by adding a field to the result set that is unique?

I assumed that the query was correct.
But as you can see, it is ugly and there is certainly a better way to do the same work.

> This patch then does not solve the problem if the things that I have assumed
> are correct.

Did you have the patience to test/compare the results?
I didn't :)
But the customer validated the patch, so I supposed he tested it.
If you are able to give me a simple example to recreate a issue on my side, I will have a look and try to provide a fix.
Comment 4 Eivin Giske Skaaren 2015-04-27 11:49:26 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Eivin Giske Skaaren from comment #2)
> 
> > So one way of getting line numbers that are not unique for the same col is
> > if priority is set to >0 for an item in the old_reserves table.
> > 
> > Question: Should priority ever be anything else than 0 for an item in the
> > old_reserves table?
> 
> Yes, I think so.
> At least I have some on local installation.
> I suppose the priority is not updated when a reserve is canceled. So if it
> has a priority > 0, it will be copied as it to the old_reserves.

Since the query evaluates on priority this means that the result will be wrong if the rows from old_reserves are not to be shown as "1- placed" in the holds report. 

> Did you have the patience to test/compare the results?
> I didn't :)

Yes I did test it ;)

> But the customer validated the patch, so I supposed he tested it.
> If you are able to give me a simple example to recreate a issue on my side,
> I will have a look and try to provide a fix.

This is my test result that you can modify to your installation:
I have 2 books in the reserves table with priority 1
I have 3 books in the old_reserves table with priority 0

Run the report and it will say:
1- placed: 2
2- processed : 3

Change the priority to 1 for one of the books in the old_reserves table and rerun the report:
1- placed: 3
2- processed: 2

Perhaps it would be good to refactor out the use of magic numbers so the whole thing gets more maintainable.
Comment 5 Jonathan Druart 2015-04-27 13:21:03 UTC
(In reply to Eivin Giske Skaaren from comment #4)
> Perhaps it would be good to refactor out the use of magic numbers so the
> whole thing gets more maintainable.

Actually I think that all the script should be rewritten.
It looks much more complicated than I expected and at the moment I don't plan to work more on this.
Comment 6 Nick Clemens 2015-06-24 19:42:27 UTC
I don't think this is 'Needs Signoff' anymore,  but not sure whether 'Failed QA' - RESOLVED-WON'T FIX - or Assigned?
Comment 7 Eivin Giske Skaaren 2015-06-24 19:47:13 UTC
It definitely should be fixed sometime so not "resolved" or "wont fix".
Comment 8 Sophie MEYNIEUX 2016-08-10 14:10:57 UTC Comment hidden (obsolete)
Comment 9 Sophie MEYNIEUX 2016-08-10 15:52:01 UTC Comment hidden (obsolete)
Comment 10 Sophie MEYNIEUX 2016-08-10 16:12:13 UTC
This patch fixes how the SQL request is built.
Union must be done before aggregation and not after, you have 2 results for each value and not the sum of it.

Test plan
=========
On Reports > reservations, select all statuses as line, pickup library as column (empty) and a range of hold date. Keep the results
Run the same reports but with statuses as column and pickup library as line
You should get the same results on another display.

* Without patch, if you've got representative data, most of the time you can observe significant differences between both results

* With the patch, results are always the same (and right if you request directly your database)
Comment 11 Chris Cormack 2016-09-05 10:39:43 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-09-13 09:09:28 UTC
Created attachment 55521 [details] [review]
Bug 13914 : Fix UNION in SQL request for reserves statistics

Test plan
=========
On Reports > reservations, select all statuses as line, pickup library as column (empty) and a range of hold date. Keep the results
Run the same reports but with statuses as column and pickup library as line
You should get the same results on another display.

* Without patch, if you've got representative data, most of the time you can observe significant differences between both results

* With the patch, results are always the same (and right if you request directly your database)

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2016-09-13 09:14:21 UTC
This script does not work under plack, see bug 17294 for a fix.
Comment 14 Kyle M Hall 2016-09-13 18:12:00 UTC
Pushed to master for 16.11, thanks Sophie!