Bug 21725

Summary: Incorrect HAVING in group by in Acquisitions.pm
Product: Koha Reporter: Nick Clemens <nick>
Component: AcquisitionsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
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 21725: Use full statement in HAVING rather than constructed column
Bug 21725: Use full statement in HAVING rather than constructed column
Bug 21725: Use full statement in HAVING rather than constructed column

Description Nick Clemens 2018-10-30 11:59:04 UTC
prove -v t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t

DBD::mysql::st execute failed: Non-grouping field 'expected_items' is used in HAVING clause [for Statement "
        SELECT aqbasket.basketno, aqbasket.basketname, aqbasket.note, aqbasket.booksellernote, aqbasket.contractnumber, aqbasket.creationdate, aqbasket.closedate, aqbasket.booksellerid, aqbasket.authorisedby, aqbasket.booksellerinvoicenumber, aqbasket.basketgroupid, aqbasket.deliveryplace, aqbasket.billingplace, aqbasket.branch, aqbasket.is_standing, aqbasket.create_items,
          SUM(aqorders.quantity) AS total_items,
          SUM(
            IF ( aqorders.orderstatus = 'cancelled', aqorders.quantity, 0 )
          ) AS total_items_cancelled,
          COUNT(DISTINCT aqorders.biblionumber) AS total_biblios,
          SUM(
            IF(aqorders.datereceived IS NULL
              AND (aqorders.datecancellationprinted IS NULL OR aqorders.datecancellationprinted='0000-00-00')
            , aqorders.quantity
            , 0)
          ) AS expected_items
        FROM aqbasket
          LEFT JOIN aqorders ON aqorders.basketno = aqbasket.basketno
        WHERE booksellerid = ? GROUP BY aqbasket.basketno, aqbasket.basketname, aqbasket.note, aqbasket.booksellernote, aqbasket.contractnumber, aqbasket.creationdate, aqbasket.closedate, aqbasket.booksellerid, aqbasket.authorisedby, aqbasket.booksellerinvoicenumber, aqbasket.basketgroupid, aqbasket.deliveryplace, aqbasket.billingplace, aqbasket.branch, aqbasket.is_standing, aqbasket.create_items HAVING (closedate IS NULL OR (expected_items > 0))" with ParamValues: 0='359'] at /home/vagrant/kohaclone/C4/Acquisition.pm line 719.
Comment 1 Nick Clemens 2018-10-30 12:03:17 UTC
Created attachment 81616 [details] [review]
Bug 21725: Use full statement in HAVING rather than constructed column

To test:
1 - prove -v t/db_dpendent/Acquisition/GetBasketsInfosByBookseller.t
2 - Apply patch
3 - prove -v t/db_dpendent/Acquisition/GetBasketsInfosByBookseller.t
4 - It passes
5 - Have some open/closed and some fully and not full received baskets
6 - View a vendor and confirm baskets are shwn as expected
7 - Check 'Show all baskets' shows all baskets
Comment 2 Nick Clemens 2018-10-30 14:52:58 UTC
*** Bug 21714 has been marked as a duplicate of this bug. ***
Comment 3 Martin Renvoize 2018-10-30 15:23:35 UTC
Created attachment 81626 [details] [review]
Bug 21725: Use full statement in HAVING rather than constructed column

To test:
1 - prove -v t/db_dpendent/Acquisition/GetBasketsInfosByBookseller.t
2 - Apply patch
3 - prove -v t/db_dpendent/Acquisition/GetBasketsInfosByBookseller.t
4 - It passes
5 - Have some open/closed and some fully and not full received baskets
6 - View a vendor and confirm baskets are shwn as expected
7 - Check 'Show all baskets' shows all baskets

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2018-10-30 15:24:16 UTC
Works as expected and I'm happy there are no regressions (yay for tests!)
Comment 5 Jonathan Druart 2018-10-30 15:42:44 UTC
As I said on bug 21714 I do not recreate the issue. That would be good to understand why.

The query is (I removed the WHERE clause):
SELECT aqbasket.basketno, aqbasket.basketname, aqbasket.note, aqbasket.booksellernote, aqbasket.contractnumber, aqbasket.creationdate, aqbasket.closedate, aqbasket.booksellerid, aqbasket.authorisedby, aqbasket.booksellerinvoicenumber, aqbasket.basketgroupid, aqbasket.deliveryplace, aqbasket.billingplace, aqbasket.branch, aqbasket.is_standing, aqbasket.create_items,           SUM(aqorders.quantity) AS total_items,           SUM(             IF ( aqorders.orderstatus = 'cancelled', aqorders.quantity, 0 )           ) AS total_items_cancelled,           COUNT(DISTINCT aqorders.biblionumber) AS total_biblios,           SUM(             IF(aqorders.datereceived IS NULL               AND (aqorders.datecancellationprinted IS NULL OR aqorders.datecancellationprinted='0000-00-00')             , aqorders.quantity             , 0)           ) AS expected_items         FROM aqbasket           LEFT JOIN aqorders ON aqorders.basketno = aqbasket.basketno GROUP BY aqbasket.basketno, aqbasket.basketname, aqbasket.note, aqbasket.booksellernote, aqbasket.contractnumber, aqbasket.creationdate, aqbasket.closedate, aqbasket.booksellerid, aqbasket.authorisedby, aqbasket.booksellerinvoicenumber, aqbasket.basketgroupid, aqbasket.deliveryplace, aqbasket.billingplace, aqbasket.branch, aqbasket.is_standing, aqbasket.create_items HAVING (closedate IS NULL OR (expected_items > 0));

Can you confirm it fail for you after having set the modes accordingly?

SET SESSION sql_mode = 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION';


Server version: 10.0.36-MariaDB-0+deb8u1 (Debian)
Comment 6 Jonathan Druart 2018-10-30 15:48:22 UTC
I wanted to move the condition to the WHERE clause. The advantage to have it into the HAVING clause was to not repeat the condition. With this patch we c/p the condition and have it in the HAVING clause... :)
Comment 7 Nick Clemens 2018-10-30 17:18:03 UTC
(In reply to Jonathan Druart from comment #6)
> I wanted to move the condition to the WHERE clause. The advantage to have it
> into the HAVING clause was to not repeat the condition. With this patch we
> c/p the condition and have it in the HAVING clause... :)

I tried moving things to the where, but this removed orders from the total_orders count so I went this route.

I can confirm setting the modes as you explain and then running the query fails for me

However: Server version: 10.1.26-MariaDB-0+deb9u1 Debian 9.1

Possibly:
https://bugs.mysql.com/bug.php?id=51058
Comment 8 Martin Renvoize 2018-10-30 18:46:27 UTC
Indeed.. I couldn't work out how to move it to the WHERE clause.. I tried pretty hard to during work on but 21385.

I agree, this is the MySQL bug highlighted by Nick.
Comment 9 Jonathan Druart 2018-11-01 14:26:25 UTC
Created attachment 81812 [details] [review]
Bug 21725: Use full statement in HAVING rather than constructed column

To test:
1 - prove -v t/db_dpendent/Acquisition/GetBasketsInfosByBookseller.t
2 - Apply patch
3 - prove -v t/db_dpendent/Acquisition/GetBasketsInfosByBookseller.t
4 - It passes
5 - Have some open/closed and some fully and not full received baskets
6 - View a vendor and confirm baskets are shwn as expected
7 - Check 'Show all baskets' shows all baskets

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Note sure this is the best thing but it fixes the issue
Comment 10 Nick Clemens 2018-11-01 19:10:22 UTC
Awesome work all!

Pushed to master for 18.11
Comment 11 Martin Renvoize 2018-11-09 16:16:10 UTC
Pushed to 18.05.x for 18.05.06
Comment 12 Fridolin Somers 2018-11-28 13:53:19 UTC
SQL modes not in 17.11.x