Description
Jonathan Druart
2015-03-26 12:17:41 UTC
Created attachment 37267 [details] [review] Bug 13914: Holds statistics report should add values from reserves and old_reserves And not pick one randomly... To reproduce: 1/ Go on Reports › Holds statistics 2/ 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". 3/ Submit! 4/ Look at the results 5/ Restart mysql 6/ Ctrl+R to resubmit the form 7/ Look at the results The generated query looks like ( SELECT stuffs FROM reserves ) UNION ( SELECT stuffs FROM old_reserves ) It returns something like: +------+---------------+-------------+ | line | col | calculation | +------+---------------+-------------+ | 1 | BRANCHCODE1 | 1 | < from the reserves table | 1 | BRANCHCODE1 | 11 | < from the old_reserves table | 1 | BRANCHCODE2 | 2 | | 1 | BRANCHCODE2 | 22 | | 2 | BRANCHCODE1 | 4 | | 2 | BRANCHCODE1 | 44 | | 2 | BRANCHCODE2 | 5 | | 2 | BRANCHCODE2 | 55 | +------+---------------+-------------+ The screen will display 1 OR 11 for the BRANCHCODE1 line 1. Since ( line, col ) is not uniq, fetchall_hashref (l.280) was not a good idea here. This patch replaces it with fetchall_arrayref, reconstruct the variables used later and add the values from reserves and old_reserves. Test plan: Confirm that the results are now relevant. 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. (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. (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. (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. I don't think this is 'Needs Signoff' anymore, but not sure whether 'Failed QA' - RESOLVED-WON'T FIX - or Assigned? It definitely should be fixed sometime so not "resolved" or "wont fix". Created attachment 54271 [details] [review] Bug 13914 : Fix UNION in SQL request for reserves statistics Created attachment 54279 [details] [review] Bug 13914 : Fix UNION in SQL request for reserves statistics 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) Created attachment 55173 [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> 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> This script does not work under plack, see bug 17294 for a fix. Pushed to master for 16.11, thanks Sophie! |