Bug 20177

Summary: Remove GROUP BY clause in GetCourses
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Julian Maurice <julian.maurice>
Severity: normal    
Priority: P5 - low CC: martin.renvoize, mtompset, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20144
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17258, 20521    
Attachments: Bug 20177: [sql_modes] Remove GROUP BY clause in GetCourses
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses

Description Julian Maurice 2018-02-12 13:45:20 UTC
This bug is a continuation of bug 20144
Comment 1 Julian Maurice 2018-02-12 13:46:40 UTC
Created attachment 71468 [details] [review]
Bug 20177: [sql_modes] Remove GROUP BY clause in GetCourses

Fix for:
'koha_kohadev.courses.department' isn't in GROUP BY

t/db_dependent/CourseReserves.t

no need to group by here

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 2 Julian Maurice 2018-02-12 13:48:04 UTC
The GROUP BY is needed here. Otherwise you can see the same course multiple times in /cgi-bin/koha/course_reserves/course-reserves.pl
Comment 3 Julian Maurice 2018-02-12 13:48:44 UTC
But maybe the joins are useless ?
Comment 4 Jonathan Druart 2018-02-14 16:38:54 UTC
Created attachment 71620 [details] [review]
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses

Fix for:
'koha_kohadev.courses.department' isn't in GROUP BY

t/db_dependent/CourseReserves.t

A better fix would be to remove the joins and only return values from
the courses table. But more work is needed to acchieve that goal.
Comment 5 Mark Tompsett 2018-04-11 00:13:42 UTC
(In reply to Julian Maurice from comment #3)
> But maybe the joins are useless ?

I'm thinking the idea was to let courses exist, but if there are no reserves or items, then why list the course?
Comment 6 Mark Tompsett 2018-04-11 02:57:45 UTC
(In reply to Jonathan Druart from comment #4)
> A better fix would be to remove the joins and only return values from
> the courses table. But more work is needed to acchieve that goal.

This sub is only used in one place. What "more work" is needed?
A refactor to Objects? The GROUP BY and JOINs make no sense in the one
template it is used.
Comment 7 Jonathan Druart 2018-04-11 16:10:53 UTC
It's also called from GetCourseReserves
Comment 8 Julian Maurice 2018-04-30 07:31:07 UTC
How can I trigger the bug ?

> mysql> SELECT @@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
But no errors on master while creating/editing courses and course reserves

prove t/db_dependent/CourseReserves.t pass on master too.
Comment 9 Jonathan Druart 2018-05-03 14:23:41 UTC
(In reply to Julian Maurice from comment #8)
> How can I trigger the bug ?
> 
> > mysql> SELECT @@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
> But no errors on master while creating/editing courses and course reserves
> 
> prove t/db_dependent/CourseReserves.t pass on master too.

Yes, since bug 20229 is pushed, the sql mode are set for the connection. Try with  bug 20521 applied.
Comment 10 Owen Leonard 2018-09-26 11:49:45 UTC
Created attachment 79405 [details] [review]
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses

Fix for:
'koha_kohadev.courses.department' isn't in GROUP BY

t/db_dependent/CourseReserves.t

A better fix would be to remove the joins and only return values from
the courses table. But more work is needed to acchieve that goal.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Test passes and the behavior of Course reserves appears to be unchanged
Comment 11 Julian Maurice 2018-10-05 13:13:59 UTC
Created attachment 80080 [details] [review]
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses

Fix for:
'koha_kohadev.courses.department' isn't in GROUP BY

t/db_dependent/CourseReserves.t

A better fix would be to remove the joins and only return values from
the courses table. But more work is needed to acchieve that goal.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Test passes and the behavior of Course reserves appears to be unchanged
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 12 Nick Clemens 2018-10-09 16:25:45 UTC
Awesome work all!

Pushed to master for 18.11
Comment 13 Martin Renvoize 2018-10-15 11:31:34 UTC
Pushed to 18.05.x for 18.05.05