| Summary: | Incorrect HAVING in group by in Acquisitions.pm | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
| Component: | Acquisitions | Assignee: | Nick Clemens (kidclamp) <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: | ||
| Circulation function: | |||
| 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 |
||
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 *** Bug 21714 has been marked as a duplicate of this bug. *** 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> Works as expected and I'm happy there are no regressions (yay for tests!) 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) 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... :) (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 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. 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 Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.06 SQL modes not in 17.11.x |
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.