Bug 21622

Summary: Incorrect GROUP BY clause in acqui/ scripts
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: aleisha, julian.maurice, katrin.fischer, kyle, lucas, m.de.rooy, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22260
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21723
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17258    
Attachments: Bug 21622: Remove incorrect GROUP BY clauses in acqui script
Bug 21622: Remove incorrect GROUP BY clauses in acqui script
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script
Bug 21622: (RM follow-up) adjust for changes from bug 21282
Bug 21622: (RM follow-up) adjust for changes from bug 21282

Description Jonathan Druart 2018-10-19 15:18:14 UTC
acqui/ordered.pl:    GROUP BY aqorders.ordernumber                                                                                                        
acqui/spent.pl:    GROUP BY aqorders.ordernumber  

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY
Comment 1 Jonathan Druart 2018-10-19 15:20:32 UTC
Created attachment 80943 [details] [review]
Bug 21622: Remove incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch
Comment 2 Jonathan Druart 2018-10-29 21:34:58 UTC
*** Bug 21718 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2018-10-29 22:09:14 UTC
Created attachment 81596 [details] [review]
Bug 21622: Remove incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Julian Maurice 2018-10-30 16:30:27 UTC
The SQL queries have a JOIN clause with aqorders_items. Without the GROUP BY it can return multiple rows for the same order. It shouldn't be removed.

On master, ordered.pl displays a table of 15 rows
With the patch applied, it displays a table of 22 rows
Same thing on spent.pl with different numbers.

Failed QA
Comment 5 Katrin Fischer 2018-10-30 17:55:16 UTC
Thx Julian! It didn't happen on mine, probably not enough data!
Comment 6 Jonathan Druart 2018-10-31 13:38:29 UTC
Created attachment 81724 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch
Comment 7 Jonathan Druart 2018-10-31 13:56:09 UTC
Created attachment 81725 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch
Comment 8 Jasmine Amohia 2019-01-16 22:20:47 UTC
Created attachment 84101 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch

Signed-off-by: Jasmine Amohia <jasmineamohia.student@wegc.school.nz>
Comment 9 Katrin Fischer 2019-02-03 10:35:20 UTC
Created attachment 84679 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch

Signed-off-by: Jasmine Amohia <jasmineamohia.student@wegc.school.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Nick Clemens 2019-02-04 13:10:24 UTC
I missed this one and filed 22260

I am wondering if one situation is preferable to the other (subquery vs adding all fields to group by)

Bug 20182 and 21723 imply there may be some issues here - can we get some opinions here on the best way to fix these situations?
Comment 11 Jonathan Druart 2019-02-17 14:45:15 UTC
(In reply to Nick Clemens from comment #10)
> I missed this one and filed 22260
> 
> I am wondering if one situation is preferable to the other (subquery vs
> adding all fields to group by)
> 
> Bug 20182 and 21723 imply there may be some issues here - can we get some
> opinions here on the best way to fix these situations?

Could you benchmark the 2 queries on a big production DB?
Comment 12 Nick Clemens 2019-02-21 16:40:39 UTC
*** Bug 22260 has been marked as a duplicate of this bug. ***
Comment 13 Nick Clemens 2019-02-21 16:45:11 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Nick Clemens from comment #10)
> > I missed this one and filed 22260
> > 
> > I am wondering if one situation is preferable to the other (subquery vs
> > adding all fields to group by)
> > 
> > Bug 20182 and 21723 imply there may be some issues here - can we get some
> > opinions here on the best way to fix these situations?
> 
> Could you benchmark the 2 queries on a big production DB?

~.1 seconds for this solution vs ~4 seconds for the other

Deprecated in favor of this one, we should invesitgate 21723 on its own I think
Comment 14 Nick Clemens 2019-02-22 15:05:57 UTC
Awesome work all!

Pushed to master for 19.05
Comment 15 Martin Renvoize 2019-02-26 09:22:42 UTC
Pushed to 18.11.x for 18.11.04
Comment 16 Lucas Gass 2019-03-06 23:43:45 UTC
this one fails qa tool in 18.05.x AND i cannot successfully run through the test plan. not backporting to 18.05.x. let me know if it is needed
Comment 17 Nick Clemens 2019-03-25 12:59:37 UTC
Created attachment 86973 [details] [review]
Bug 21622: (RM follow-up) adjust for changes from bug 21282
Comment 18 Nick Clemens 2019-03-25 13:05:28 UTC
Created attachment 86974 [details] [review]
Bug 21622: (RM follow-up) adjust for changes from bug 21282
Comment 19 Nick Clemens 2019-03-29 11:06:55 UTC
(In reply to Nick Clemens from comment #18)
> Created attachment 86974 [details] [review] [review]
> Bug 21622: (RM follow-up) adjust for changes from bug 21282

Followup pushed to master
Comment 20 Martin Renvoize 2019-04-01 10:41:06 UTC
Followup pushed to 18.11.x series.