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 <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/C4/Letters.pm b/C4/Letters.pm
index 42893f1..219779b 100644 (file)
--- 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 e20cefb..d8e96a6 100644 (file)
--- a/t/db_dependent/Letters.t
+++ b/t/db_dependent/Letters.t
@@ -18,7 +18,7 @@
 # along with Koha; if not, see <http://www.gnu.org/licenses>.
 
 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: <<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 4649a33..8ebc4fe 100644 (file)
--- 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',