Bug 21603 - Incorrect GROUP BY clause in SearchCourses
Summary: Incorrect GROUP BY clause in SearchCourses
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 17258
  Show dependency treegraph
 
Reported: 2018-10-18 13:44 UTC by Nick Clemens
Modified: 2020-01-06 20:17 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21603: Remove incorrect GROUP BY from C4::CourseReserves (2.18 KB, patch)
2018-10-18 22:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21603: Remove incorrect GROUP BY from C4::CourseReserves (2.24 KB, patch)
2018-10-25 14:13 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21603: Remove incorrect GROUP BY from C4::CourseReserves (2.27 KB, patch)
2018-10-26 10:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-10-18 13:44:47 UTC
I am not certain we can simply remove here, but the GROUP by seems to stem form adding course instructors, though we fetch the instructors for each course after this query

 971     $query .= "
 972         LEFT JOIN course_instructors ci
 973             ON ( c.course_id = ci.course_id )
 974         LEFT JOIN borrowers b
 975             ON ( ci.borrowernumber = b.borrowernumber )
 976         LEFT JOIN authorised_values av
 977             ON ( c.department = av.authorised_value )
 978         WHERE
 979             ( av.category = 'DEPARTMENT' OR av.category = 'TERM' )
 980             AND
 981             (
 982                 department LIKE ? OR
 983                 course_number LIKE ? OR
 984                 section LIKE ? OR
 985                 course_name LIKE ? OR
 986                 term LIKE ? OR
 987                 public_note LIKE ? OR
 988                 CONCAT(surname,' ',firstname) LIKE ? OR
 989                 CONCAT(firstname,' ',surname) LIKE ? OR
 990                 lib LIKE ? OR
 991                 lib_opac LIKE ?
 992            )
 993            AND
 994            c.enabled LIKE ?
 995         GROUP BY c.course_id
 996     ";
Comment 1 Jonathan Druart 2018-10-18 20:42:48 UTC
'koha_kohadev.c.department' isn't in GROUP BY

then later:
Can't use an undefined value as an ARRAY reference at /usr/lib/x86_64-linux-gnu/perl5/5.20/DBI.pm line 2064
Comment 2 Jonathan Druart 2018-10-18 22:19:18 UTC
Created attachment 80884 [details] [review]
Bug 21603: Remove incorrect GROUP BY from C4::CourseReserves

'koha_kohadev.c.department' isn't in GROUP BY

Test plan:
Prove that the test fail without this patch and pass with this patch
applied (switch on the SQL modes)
Comment 3 Andrew Isherwood 2018-10-25 14:13:33 UTC
Created attachment 81163 [details] [review]
Bug 21603: Remove incorrect GROUP BY from C4::CourseReserves

'koha_kohadev.c.department' isn't in GROUP BY

Test plan:
Prove that the test fail without this patch and pass with this patch
applied (switch on the SQL modes)

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Comment 4 Marcel de Rooy 2018-10-26 10:00:38 UTC
Created attachment 81263 [details] [review]
Bug 21603: Remove incorrect GROUP BY from C4::CourseReserves

'koha_kohadev.c.department' isn't in GROUP BY

Test plan:
Prove that the test fail without this patch and pass with this patch
applied (switch on the SQL modes)

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Comment 5 Marcel de Rooy 2018-10-26 10:01:33 UTC
> Prove that the test fail without this patch and pass with this patch
> applied (switch on the SQL modes)

Could not reproduce the warning with strict_sql_modes..
Lower severity ?
Comment 6 Jonathan Druart 2018-10-26 12:06:10 UTC
(In reply to Marcel de Rooy from comment #5)
> > Prove that the test fail without this patch and pass with this patch
> > applied (switch on the SQL modes)
> 
> Could not reproduce the warning with strict_sql_modes..
> Lower severity ?

The test plan is not cleared: you should
- apply the new test (so manually)
- prove the new test fails
- apply the pm changes
- prove the new test passes

I should have provided two patches.
Comment 7 Nick Clemens 2018-10-26 17:19:55 UTC
Awesome work all!

Pushed to master for 18.11
Comment 8 Martin Renvoize 2018-11-06 11:56:31 UTC
Pushed to 18.05.x for 18.05.06
Comment 9 Fridolin Somers 2018-11-28 10:01:09 UTC
I pass for sql modes in 17.11.x