From 666cbed5e17f8b644300c43091231554eaeb3eb0 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 5 Feb 2018 18:26:05 -0300 Subject: [PATCH] Bug 20144: [sql_modes] Fix GROUP BY clause in GetLetters This subroutine is wrong and must be rewritten using Koha::Notice::Templates. Mainly because the DB structure is bad. Meanwhile we remove the branchcode from the SELECT to get correct results, it was not used by callers anyway. Fix for: 'koha_kohadev.letter.module' isn't in GROUP BY t/db_dependent/Letters.t Signed-off-by: Josef Moravec Signed-off-by: Julian Maurice Signed-off-by: Jonathan Druart --- C4/Letters.pm | 7 +++++-- t/db_dependent/Letters.t | 3 +-- .../Letters/GetLettersAvailableForALibrary.t | 20 +++++++++---------- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index 42893f1bd3..219779bc3e 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -70,6 +70,9 @@ C4::Letters - Give functions for Letters management returns informations about letters. if needed, $module filters for letters given module + DEPRECATED - You must use Koha::Notice::Templates instead + The group by clause is confusing and can lead to issues + =cut sub GetLetters { @@ -80,14 +83,14 @@ sub GetLetters { my $dbh = C4::Context->dbh; my $letters = $dbh->selectall_arrayref( q| - SELECT module, code, branchcode, name + SELECT code, module, name FROM letter WHERE 1 | . ( $module ? q| AND module = ?| : q|| ) . ( $code ? q| AND code = ?| : q|| ) . ( defined $branchcode ? q| AND branchcode = ?| : q|| ) - . q| GROUP BY code ORDER BY name|, { Slice => {} } + . q| GROUP BY code, module, name ORDER BY name|, { Slice => {} } , ( $module ? $module : () ) , ( $code ? $code : () ) , ( defined $branchcode ? $branchcode : () ) diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index e20cefb8b9..d8e96a6a7a 100644 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -18,7 +18,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 77; +use Test::More tests => 76; use Test::MockModule; use Test::Warn; @@ -176,7 +176,6 @@ Look at this wonderful biblio timestamp: <>. $dbh->do( q|INSERT INTO letter(branchcode,module,code,name,is_html,title,content,message_transport_type) VALUES (?,'my module','my code','my name',1,?,?,'email')|, undef, $library->{branchcode}, $title, $content ); $letters = C4::Letters::GetLetters(); is( @$letters, 1, 'GetLetters returns the correct number of letters' ); -is( $letters->[0]->{branchcode}, $library->{branchcode}, 'GetLetters gets the branch code correctly' ); is( $letters->[0]->{module}, 'my module', 'GetLetters gets the module correctly' ); is( $letters->[0]->{code}, 'my code', 'GetLetters gets the code correctly' ); is( $letters->[0]->{name}, 'my name', 'GetLetters gets the name correctly' ); diff --git a/t/db_dependent/Letters/GetLettersAvailableForALibrary.t b/t/db_dependent/Letters/GetLettersAvailableForALibrary.t index 4649a33742..8ebc4fea29 100644 --- a/t/db_dependent/Letters/GetLettersAvailableForALibrary.t +++ b/t/db_dependent/Letters/GetLettersAvailableForALibrary.t @@ -15,7 +15,7 @@ my $letters = [ module => 'circulation', code => 'code1', branchcode => '', - name => 'B default name for code1 circ', + name => 'B name for code1 circ', is_html => 0, title => 'default title for code1 email', content => 'default content for code1 email', @@ -25,7 +25,7 @@ my $letters = [ module => 'circulation', code => 'code1', branchcode => '', - name => 'B default name for code1 circ', + name => 'B name for code1 circ', is_html => 0, title => 'default title for code1 sms', content => 'default content for code1 sms', @@ -35,7 +35,7 @@ my $letters = [ module => 'circulation', code => 'code2', branchcode => '', - name => 'A default name for code2 circ', + name => 'A name for code2 circ', is_html => 0, title => 'default title for code2 email', content => 'default content for code2 email', @@ -45,7 +45,7 @@ my $letters = [ module => 'circulation', code => 'code3', branchcode => '', - name => 'C default name for code3 circ', + name => 'C name for code3 circ', is_html => 0, title => 'default title for code3 email', content => 'default content for code3 email', @@ -56,7 +56,7 @@ my $letters = [ module => 'cataloguing', code => 'code1', branchcode => '', - name => 'default name for code1 cat', + name => 'D name for code1 cat', is_html => 0, title => 'default title for code1 cat email', content => 'default content for code1 cat email', @@ -67,7 +67,7 @@ my $letters = [ module => 'circulation', code => 'code1', branchcode => 'CPL', - name => 'B CPL name for code1 circ', + name => 'B name for code1 circ', is_html => 0, title => 'CPL title for code1 email', content => 'CPL content for code1 email', @@ -77,17 +77,17 @@ my $letters = [ module => 'circulation', code => 'code2', branchcode => 'CPL', - name => 'A CPL name for code1 circ', + name => 'A name for code2 circ', is_html => 0, - title => 'CPL title for code1 sms', - content => 'CPL content for code1 sms', + title => 'CPL title for code2 sms', + content => 'CPL content for code2 sms', message_transport_type => 'sms', }, { module => 'circulation', code => 'code1', branchcode => 'MPL', - name => 'B MPL name for code1 circ', + name => 'B name for code1 circ', is_html => 0, title => 'MPL title for code1 email', content => 'MPL content for code1 email', -- 2.20.1