Bug 21593 - Remove Group by clause in GetAuthValueDropbox
Summary: Remove Group by clause in GetAuthValueDropbox
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 21597
  Show dependency treegraph
 
Reported: 2018-10-18 04:57 UTC by Alex Buckley
Modified: 2018-11-28 15:35 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21593: Remove GROUP BY from GetAuthValueDropbox (1.17 KB, patch)
2018-10-18 10:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21593: Remove GROUP BY from GetAuthValueDropbox (1.25 KB, patch)
2018-10-18 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2018-10-18 04:57:47 UTC
When going to pay a users fine the following error is displayed:
Template process failed: undef error - DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_kohadev.me.id' isn't in GROUP BY at /home/vagrant/kohaclone/Koha/Objects.pm line 129
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

Viewing the logs you see this is caused by this DB error:
DBD::mysql::st execute failed: 'koha_kohadev.me.id' isn't in GROUP BY [for Statement "SELECT `me`.`id`, `me`.`category`, `me`.`authorised_value`, `me`.`lib`, `me`.`lib_opac`, `me`.`imageurl` FROM `authorised_values` `me` LEFT JOIN `authorised_values_branches` `authorised_values_branches` ON `authorised_values_branches`.`av_id` = `me`.`id` WHERE ( ( ( `authorised_values_branches`.`branchcode` IS NULL OR `authorised_values_branches`.`branchcode` = ? ) AND `category` = ? ) ) GROUP BY `lib` ORDER BY `category`, `lib`, `lib_opac`" with ParamValues: 0="CPL", 1='PAYMENT_TYPE'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.

Doing a git bisect I found this error started when the patch 20521 was pushed to master: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20521
Comment 1 Nick Clemens 2018-10-18 10:08:15 UTC
Created attachment 80800 [details] [review]
Bug 21593: Remove GROUP BY from GetAuthValueDropbox

With new MYSQL modes we are permitted only full group by.

In this case I don't think we need a GROUP BY at all

To test:
1 - Try to pay a fine or create a borrower
2 - Internal server error
3 - Apply patch
4 - Page should load
5 - Test various authorised values ot ensure they work
   e.g. Set bsort1 and bsort2 to have some authorised values with and
   without branch limits
   set PAYMENT_TYPE to have some values with and without branch limits
Comment 2 Martin Renvoize 2018-10-18 11:15:58 UTC
This looks sane and the code scans.. Trivial change and I can't see any reason we would need the group_by clause here. 

I'm running through the whole test suite to check for any fallout but then I think it's a 'straight to QA' candidate.
Comment 3 Martin Renvoize 2018-10-18 11:17:44 UTC
Created attachment 80803 [details] [review]
Bug 21593: Remove GROUP BY from GetAuthValueDropbox

With new MYSQL modes we are permitted only full group by.

In this case I don't think we need a GROUP BY at all

To test:
1 - Try to pay a fine or create a borrower
2 - Internal server error
3 - Apply patch
4 - Page should load
5 - Test various authorised values ot ensure they work
   e.g. Set bsort1 and bsort2 to have some authorised values with and
   without branch limits
   set PAYMENT_TYPE to have some values with and without branch limits

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2018-10-18 11:18:49 UTC
Tests pass, QA script pass, test plan pass.

Passing QA
Comment 5 Jonathan Druart 2018-10-18 11:31:39 UTC
ok for me, I had the same fix.

However I noted:

    Hum This one is weird, I have no idea why there is a group by lib here.
    This patch will introduce a change in behaviour /!\
    If several AV have the same description they will not all be displayed in the dropdown list.
    But it seems like a bug fix, otherwise only 1 will be display, but the user
    will not know that there are two different codes
Comment 6 Nick Clemens 2018-10-18 11:55:35 UTC
Pushed to master for 18.11
Comment 7 Martin Renvoize 2018-10-26 13:22:03 UTC
Pushed to 18.05.x for 18.05.06
Comment 8 Fridolin SOMERS 2018-11-09 15:17:10 UTC
I prefer not change behaviour in oldstable 17.11.x