Bug 20177 - Remove GROUP BY clause in GetCourses
Summary: Remove GROUP BY clause in GetCourses
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Julian Maurice
URL:
Keywords:
Depends on:
Blocks: 17258 20521
  Show dependency treegraph
 
Reported: 2018-02-12 13:45 UTC by Julian Maurice
Modified: 2018-10-24 09:13 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20177: [sql_modes] Remove GROUP BY clause in GetCourses (899 bytes, patch)
2018-02-12 13:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses (1.64 KB, patch)
2018-02-14 16:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses (1.76 KB, patch)
2018-09-26 11:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20177: [sql_modes] Fix GROUP BY clause in GetCourses (1.82 KB, patch)
2018-10-05 13:13 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 M. 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