The Most circulated items report gives an error when run on a database that has ONLY_FULL_GROUP_BY in it's SQL_MODE. Patch to follow shortly.
Created attachment 124664 [details] [review] Proposed patch Test plan: Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF ) Go to Reports -> Most circulated items submit the form.
I had a go at testing with koha-testing-docker (KTD), but was unable to get this to work. - KTD = started with ku - Strict mode already set in /etc/koha/sites/kohadev/koha-conf.xml Error before patch applied, no change after (with a flush_memcached and restart_all): CGI::Compile::ROOT::kohadevbox_koha_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: 'koha_kohadev.biblio.title' isn't in GROUP BY at /kohadevbox/koha/reports/cat_issues_top.pl line 64 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 I think I will leave the testing to someone who understands how this all works a bit better than I do!
(In reply to David Nind from comment #2) > I had a go at testing with koha-testing-docker (KTD), but was unable to get > this to work. > > - KTD = started with ku > - Strict mode already set in /etc/koha/sites/kohadev/koha-conf.xml > > Error before patch applied, no change after (with a flush_memcached and > restart_all): > > CGI::Compile::ROOT::kohadevbox_koha_reports_cat_issues_top_2epl::calculate(): > DBI Exception: DBD::mysql::st execute failed: 'koha_kohadev.biblio.title' > isn't in GROUP BY at /kohadevbox/koha/reports/cat_issues_top.pl line 64 > at /usr/share/perl5/DBIx/Class/Exception.pm line 77 > > I think I will leave the testing to someone who understands how this all > works a bit better than I do! What options did you use to generate that error? I'm not able to reproduce it. I didn't test this report thoroughly, so I'm sure there is more work to be done here. Now that I'm playing with it a bit more the "distinct" is causing trouble too with Mysql 8.
> What options did you use to generate that error? I'm not able to reproduce > it. This was with and without selecting any date ranges, With koha-testing-docker (started with ku) it is using MariaDB 10.1 with Debian 9.
Created attachment 125124 [details] [review] Proposed patch Fresh patch with fixes for MariaDB in Debian 9.
Created attachment 125208 [details] [review] Bug 28977 - Add GROUP_CONCAT to some of the selected columns To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY Sql mode. Test plan: Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF ) Go to Reports -> Most circulated items submit the form. Without the patch you get an error like: CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues` LEFT JOIN items USING(itemnumber) LEFT JOIN biblio USING(biblionumber) LEFT JOIN biblioitems USING(biblionumber) LEFT JOIN borrowers USING(borrowernumber) WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 With the patch you will see the report results view. Signed-off-by: David Nind <david@davidnind.com>
Testing notes (koha-testing-docker): - tested on environments available by starting KTD with ku, ku-mdb, ku-md9, ku-md10, ky-my8 - check out and check in an item (so that there was some data for the report)
Created attachment 128684 [details] [review] Bug 28977: Add GROUP_CONCAT to some of the selected columns To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY Sql mode. Test plan: Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF ) Go to Reports -> Most circulated items submit the form. Without the patch you get an error like: CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues` LEFT JOIN items USING(itemnumber) LEFT JOIN biblio USING(biblionumber) LEFT JOIN biblioitems USING(biblionumber) LEFT JOIN borrowers USING(borrowernumber) WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 With the patch you will see the report results view. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Do we still need "DISTINCT" on biblio.title ?
(In reply to Fridolin Somers from comment #9) > Do we still need "DISTINCT" on biblio.title ? Maybe so; depends on how likely it is that separate biblios will have the same title, and whether the library would want those separate records grouped together. In my case, we'd probably want the records grouped together. I tried the query in MySQL 8.0 with and without the 'DISTINCT', and there was 1 row difference. Though my case is probably not typical, being a school district with a bunch of schools that migrated from separate databases into one instance of Koha.
(In reply to Michael Hafen from comment #10) > (In reply to Fridolin Somers from comment #9) > > Do we still need "DISTINCT" on biblio.title ? > > Maybe so; depends on how likely it is that separate biblios will have the > same title, and whether the library would want those separate records > grouped together. > In my case, we'd probably want the records grouped together. I tried the > query in MySQL 8.0 with and without the 'DISTINCT', and there was 1 row > difference. Though my case is probably not typical, being a school district > with a bunch of schools that migrated from separate databases into one > instance of Koha. Is there then a need to group by biblionumber ?
Ah that is strange. Looks like callnumber is not displayed in result table. This has been added by Bug 10154 as a filter, but looks there there is no need to add in the SELECT. Can someone confirm ?
(In reply to Fridolin Somers from comment #12) > Ah that is strange. > Looks like callnumber is not displayed in result table. > This has been added by Bug 10154 as a filter, but looks there there is no > need to add in the SELECT. > Can someone confirm ? I would guess that that is intended. The call number shows in the filters, and probably isn't necessary in the results table. If someone wants it there though, it can be added. Here or in an enhancement bug.
(In reply to Michael Hafen from comment #13) > (In reply to Fridolin Somers from comment #12) > > Ah that is strange. > > Looks like callnumber is not displayed in result table. > > This has been added by Bug 10154 as a filter, but looks there there is no > > need to add in the SELECT. > > Can someone confirm ? > > I would guess that that is intended. The call number shows in the filters, > and probably isn't necessary in the results table. > > If someone wants it there though, it can be added. Here or in an > enhancement bug. I propose removing from select here, instead of group_concat
Looking at things again, only the title, rank, id and $colfield columns reach the template. All the other columns in the SELECT clause (all the GROUP_CONCATs except $colfield) could probably be dropped. Also, the $colfield column should probably be pulled out of the GROUP_CONCAT since it's also added to the GROUP BY clause. Also, grouping by biblionumber is required, because the report contains a link to the biblio record. Coming back to the question about DISTINCT. I had to look up how it works; it seems I didn't understand it completely. It works like the GROUP BY clause for all the columns in the SELECT clause. Given that it should probably be dropped for the sake of performance. I tried running the query again, with and without the DISTINCT, and there was no difference in the number or rows returned. I don't know what I was doing before, but I think the previous runs were flawed; this result makes more sense. This amounts to a big change in the patch, and the status should probably be moved back to Needs Signoff if the patch is updated. Which I think it should be at this point; I really didn't understand this report completely when I started this. So I'm going to go and work up another patch now.
Created attachment 129510 [details] [review] Bug 28977: Add GROUP_CONCAT to some of the selected columns
Asking for a signoff. Could you adjust the patch's title?
Created attachment 129982 [details] [review] Bug 28977: Add GROUP_CONCAT to some of the selected columns To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY Sql mode. Test plan: Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF ) Go to Reports -> Most circulated items submit the form. Without the patch you get an error like: CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues` LEFT JOIN items USING(itemnumber) LEFT JOIN biblio USING(biblionumber) LEFT JOIN biblioitems USING(biblionumber) LEFT JOIN borrowers USING(borrowernumber) WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 With the patch you will see the report results view. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 130081 [details] [review] Bug 28977: Fix cat_issues_top.pl report with strict SQL modes To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY Sql mode. Test plan: Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF ) Go to Reports -> Most circulated items submit the form. Without the patch you get an error like: CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues` LEFT JOIN items USING(itemnumber) LEFT JOIN biblio USING(biblionumber) LEFT JOIN biblioitems USING(biblionumber) LEFT JOIN borrowers USING(borrowernumber) WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 With the patch you will see the report results view. Signed-off-by: David Nind <david@davidnind.com> JD Amended patch - adjust commit's title Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.03
Pushed to 21.05.x for 21.05.11
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.