Bug 28514: Remove getletter
authorJonathan Druart <jonathan.druart@bugs.koha-community.org>
Thu, 3 Jun 2021 11:01:43 +0000 (13:01 +0200)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Mon, 21 Jun 2021 11:49:58 +0000 (13:49 +0200)
The way we handle notice templates is confusing (see bug 27660, bug 26787, bug 28487).

This patch remove C4::Letters::getletter and use either Koha::Notice::Templates->find
or the newly created methods ->find_effective_template that will do
all necessary to return the correct template.

Test plan:
- Create and modify notice templates
- Make sure you have TranslateNotices turned on and that some notices
templates have a translated version
- Use holds_reminder.pl and overdue_notices.pl cronjobs and confirm that
the generated notices are the expected ones
- Test also pos/printreceipt.pl
- And finally test some other notices (CHECKIN, RENEWAL for instance)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Amended by removing comment for $params={%$params}

JD amended patch:
* Add missing POD
* Fix spelling (dont  ==> don't)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
C4/Letters.pm
Koha/Notice/Templates.pm
misc/cronjobs/holds/holds_reminder.pl
misc/cronjobs/overdue_notices.pl
pos/printreceipt.pl
t/db_dependent/Koha/Notices.t
t/db_dependent/Letters.t
tools/letter.pl

index b8de25b..d2a3f12 100644 (file)
@@ -205,35 +205,6 @@ sub GetLettersAvailableForALibrary {
 
 }
 
-sub getletter {
-    my ( $module, $code, $branchcode, $message_transport_type, $lang) = @_;
-    $message_transport_type //= '%';
-    $lang = 'default' unless( $lang && C4::Context->preference('TranslateNotices') );
-
-
-    my $only_my_library = C4::Context->only_my_library;
-    if ( $only_my_library and $branchcode ) {
-        $branchcode = C4::Context::mybranch();
-    }
-    $branchcode //= '';
-
-    my $dbh = C4::Context->dbh;
-    my $sth = $dbh->prepare(q{
-        SELECT *
-        FROM letter
-        WHERE module=? AND code=? AND (branchcode = ? OR branchcode = '')
-        AND message_transport_type LIKE ?
-        AND lang =?
-        ORDER BY branchcode DESC LIMIT 1
-    });
-    $sth->execute( $module, $code, $branchcode, $message_transport_type, $lang );
-    my $line = $sth->fetchrow_hashref
-      or return;
-    $line->{'content-type'} = 'text/html; charset="UTF-8"' if $line->{is_html};
-    return { %$line };
-}
-
-
 =head2 DelLetter
 
     DelLetter(
@@ -637,13 +608,23 @@ sub GetPreparedLetter {
         my $branchcode  = $params{branchcode} || '';
         my $mtt         = $params{message_transport_type} || 'email';
 
-        $letter = getletter( $module, $letter_code, $branchcode, $mtt, $lang );
+        my $template = Koha::Notice::Templates->find_effective_template(
+            {
+                module                 => $module,
+                code                   => $letter_code,
+                branchcode             => $branchcode,
+                message_transport_type => $mtt,
+                lang                   => $lang
+            }
+        );
 
-        unless ( $letter ) {
-            $letter = getletter( $module, $letter_code, $branchcode, $mtt, 'default' )
-                or warn( "No $module $letter_code letter transported by " . $mtt ),
-                    return;
+        unless ( $template ) {
+            warn( "No $module $letter_code letter transported by " . $mtt );
+            return;
         }
+
+        $letter = $template->unblessed;
+        $letter->{'content-type'} = 'text/html; charset="UTF-8"' if $letter->{is_html};
     }
 
     my $tables = $params{tables} || {};
index 377a956..6abc1b3 100644 (file)
@@ -35,6 +35,52 @@ Koha::Notice::Templates - Koha notice template Object set class, related to the
 
 =cut
 
+=head3
+
+my $template = Koha::Notice::Templates->find_effective_template(
+    {
+        module     => $module,
+        code       => $code,
+        branchcode => $branchcode,
+        lang       => $lang,
+    }
+);
+
+Return the notice template that must be used for a given primary key (module, code, branchcode, lang).
+
+For instance if lang="es-ES" but there is no "es-ES" template defined for this language,
+the default template will be returned.
+
+lang will default to "default" if not passed.
+
+=cut
+
+sub find_effective_template {
+    my ( $self, $params ) = @_;
+
+    $params = { %$params }; # don't modify original
+    $params->{lang} = 'default' unless C4::Context->preference('TranslateNotices');
+
+    my $only_my_library = C4::Context->only_my_library;
+    if ( $only_my_library and $params->{branchcode} ) {
+        $params->{branchcode} = C4::Context::mybranch();
+    }
+    $params->{branchcode} //= '';
+    $params->{branchcode} = [$params->{branchcode}, ''];
+
+    my $template = $self->SUPER::search( $params, { order_by => { -desc => 'branchcode' } } );
+
+    if (   !$template->count
+        && C4::Context->preference('TranslateNotices')
+        && $params->{lang} ne 'default' )
+    {
+        $params->{lang} = 'default';
+        $template = $self->SUPER::search( $params, { order_by => { -desc => 'branchcode' } } );
+    }
+
+    return $template->next if $template->count;
+}
+
 =head3 type
 
 =cut
index 49bccfd..356e3ba 100755 (executable)
@@ -37,6 +37,8 @@ use C4::Log;
 use Koha::DateUtils;
 use Koha::Calendar;
 use Koha::Libraries;
+use Koha::Notice::Templates;
+use Koha::Patrons;
 use Koha::Script -cron;
 
 =head1 NAME
@@ -223,8 +225,17 @@ else {
 # Loop through each branch
 foreach my $branchcode (@branchcodes) { #BEGIN BRANCH LOOP
     # Check that this branch has the letter code specified or skip this branch
-    my $letter = C4::Letters::getletter( 'reserves', $lettercode , $branchcode );
-    unless ($letter) {
+
+    # FIXME What if we don't want to default if the translated template does not exist?
+    my $template_exists = Koha::Notice::Templates->search(
+        {
+            module     => 'reserves',
+            code       => $lettercode,
+            branchcode => $branchcode,
+            lang       => 'default',
+        }
+    )->count;
+    unless ($template_exists) {
         $verbose and print qq|Message '$lettercode' content not found for $branchcode\n|;
         next;
     }
index 16d708c..74c1fc3 100755 (executable)
@@ -624,8 +624,14 @@ END_SQL
                     }
                 }
 
-                my $letter = C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode, undef, $patron->lang )
-                          || C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode, undef, "default");
+                my $letter = Koha::Notice::Templates->find_effective_template(
+                    {
+                        module     => 'circulation',
+                        code       => $overdue_rules->{"letter$i"},
+                        branchcode => $branchcode,
+                        lang       => $patron->lang
+                    }
+                );
 
                 unless ($letter) {
                     $verbose and warn qq|Message '$overdue_rules->{"letter$i"}' content not found|;
@@ -716,8 +722,16 @@ END_SQL
                     splice @items, $PrintNoticesMaxLines if $effective_mtt eq 'print' && $PrintNoticesMaxLines && scalar @items > $PrintNoticesMaxLines;
                     #catch the case where we are sending a print to someone with an email
 
-                    my $letter_exists = ( C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode, $effective_mtt, $patron->lang )
-                                       || C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode, $effective_mtt, "default") ) ? 1 : 0;
+                    my $letter_exists = Koha::Notice::Templates->find_effective_template(
+                        {
+                            module     => 'circulation',
+                            code       => $overdue_rules->{"letter$i"},
+                            message_transport_type => $effective_mtt,
+                            branchcode => $branchcode,
+                            lang       => $patron->lang
+                        }
+                    );
+
                     my $letter = parse_overdues_letter(
                         {   letter_code     => $overdue_rules->{"letter$i"},
                             borrowernumber  => $borrowernumber,
index 8f18f6b..a387b8e 100755 (executable)
@@ -25,6 +25,7 @@ use CGI qw ( -utf8 );
 use C4::Letters;
 use Koha::Account::Lines;
 use Koha::DateUtils;
+use Koha::Notice::Templates;
 
 my $input = CGI->new;
 
@@ -52,8 +53,15 @@ output_and_exit_if_error(
 ) if $patron;    # Payment could have been anonymous
 
 my $lang = $patron ? $patron->lang : $template->lang;
-my $letter = C4::Letters::getletter( 'pos', 'RECEIPT',
-    C4::Context::mybranch, 'print', $lang );
+my $letter = Koha::Notice::Templates->find_effective_template(
+    {
+        module                 => 'pos',
+        code                   => 'RECEIPT',
+        branchcode             => C4::Context::mybranch,
+        message_transport_type => 'print',
+        lang                   => $lang
+    }
+);
 
 $template->param(
     letter  => $letter,
index dc9646f..c6c4705 100755 (executable)
 
 use Modern::Perl;
 
-use Test::More tests => 3;
+use Test::More tests => 4;
 
 use Koha::Notice::Templates;
 use Koha::Database;
 
 use t::lib::TestBuilder;
+use t::lib::Mocks;
 
 my $schema = Koha::Database->new->schema;
 $schema->storage->txn_begin;
@@ -66,5 +67,72 @@ $retrieved_template->delete;
 is( Koha::Notice::Templates->search->count,
     $nb_of_templates, 'Delete should have deleted the template' );
 
+subtest 'find_effective_template' => sub {
+    plan tests => 7;
+
+    my $default_template = $builder->build_object(
+        { class => 'Koha::Notice::Templates', value => { branchcode => '', lang => 'default' } }
+    );
+    my $key = {
+        module                 => $default_template->module,
+        code                   => $default_template->code,
+        message_transport_type => $default_template->message_transport_type,
+    };
+
+    my $library_specific_template = $builder->build_object(
+        { class => 'Koha::Notice::Templates', value => { %$key, lang => 'default' } }
+    );
+
+    my $es_template = $builder->build_object(
+        {
+            class => 'Koha::Notice::Templates',
+            value => { %$key, lang => 'es-ES' },
+        }
+    );
+
+    $key->{branchcode} = $es_template->branchcode;
+
+    t::lib::Mocks::mock_preference( 'TranslateNotices', 0 );
+
+    my $template = Koha::Notice::Templates->find_effective_template($key);
+    is( $template->lang, 'default', 'no lang passed, default is returned' );
+    $template = Koha::Notice::Templates->find_effective_template( { %$key, lang => 'es-ES' } );
+    is( $template->lang, 'default',
+        'TranslateNotices is off, default is returned' );
+
+    t::lib::Mocks::mock_preference( 'TranslateNotices', 1 );
+    $template = Koha::Notice::Templates->find_effective_template($key);
+    is( $template->lang, 'default', 'no lang passed, default is returned' );
+    $template = Koha::Notice::Templates->find_effective_template( { %$key, lang => 'es-ES' } );
+    is( $template->lang, 'es-ES',
+        'TranslateNotices is on and es-ES is requested, es-ES is returned' );
+
+
+    {    # IndependentBranches => 1
+        t::lib::Mocks::mock_userenv( { branchcode => $library_specific_template->branchcode, flag => 0 } );
+        t::lib::Mocks::mock_preference( 'IndependentBranches', 1 );
+        $template = Koha::Notice::Templates->find_effective_template( { %$key, branchcode => $library_specific_template->branchcode } );
+        is( $template->content, $library_specific_template->content,
+            'IndependentBranches is on, logged in patron is not superlibrarian but asks for their specific template, it is returned'
+        );
+
+        my $another_library = $builder->build_object( { class => 'Koha::Libraries' } );
+        t::lib::Mocks::mock_userenv( { branchcode => $another_library->branchcode, flag => 0 } );
+        $template = Koha::Notice::Templates->find_effective_template($key);
+        is( $template->content, $default_template->content,
+'IndependentBranches is on, logged in patron is not superlibrarian, default is returned'
+        );
+    }
+
+    t::lib::Mocks::mock_preference( 'IndependentBranches', 0 );
+    $es_template->delete;
+
+    $template = Koha::Notice::Templates->find_effective_template( { %$key, lang => 'es-ES' } );
+    is( $template->lang, 'default',
+        'TranslateNotices is on and es-ES is requested but does not exist, default is returned'
+    );
+
+};
+
 $schema->storage->txn_rollback;
 
index cd5d404..f916f08 100755 (executable)
@@ -18,7 +18,7 @@
 # along with Koha; if not, see <http://www.gnu.org/licenses>.
 
 use Modern::Perl;
-use Test::More tests => 86;
+use Test::More tests => 82;
 use Test::MockModule;
 use Test::Warn;
 use Test::Exception;
@@ -209,53 +209,6 @@ 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' );
 
-
-# getletter
-subtest 'getletter' => sub {
-    plan tests => 16;
-    t::lib::Mocks::mock_preference('IndependentBranches', 0);
-    my $letter = C4::Letters::getletter('my module', 'my code', $library->{branchcode}, 'email');
-    is( $letter->{branchcode}, $library->{branchcode}, 'GetLetters gets the branch code correctly' );
-    is( $letter->{module}, 'my module', 'GetLetters gets the module correctly' );
-    is( $letter->{code}, 'my code', 'GetLetters gets the code correctly' );
-    is( $letter->{name}, 'my name', 'GetLetters gets the name correctly' );
-    is( $letter->{is_html}, 1, 'GetLetters gets the boolean is_html correctly' );
-    is( $letter->{title}, $title, 'GetLetters gets the title correctly' );
-    is( $letter->{content}, $content, 'GetLetters gets the content correctly' );
-    is( $letter->{message_transport_type}, 'email', 'GetLetters gets the message type correctly' );
-
-    t::lib::Mocks::mock_userenv({ branchcode => "anotherlib", flags => 1 });
-
-    t::lib::Mocks::mock_preference('IndependentBranches', 1);
-    $letter = C4::Letters::getletter('my module', 'my code', $library->{branchcode}, 'email');
-    is( $letter->{branchcode}, $library->{branchcode}, 'GetLetters gets the branch code correctly' );
-    is( $letter->{module}, 'my module', 'GetLetters gets the module correctly' );
-    is( $letter->{code}, 'my code', 'GetLetters gets the code correctly' );
-    is( $letter->{name}, 'my name', 'GetLetters gets the name correctly' );
-    is( $letter->{is_html}, 1, 'GetLetters gets the boolean is_html correctly' );
-    is( $letter->{title}, $title, 'GetLetters gets the title correctly' );
-    is( $letter->{content}, $content, 'GetLetters gets the content correctly' );
-    is( $letter->{message_transport_type}, 'email', 'GetLetters gets the message type correctly' );
-};
-
-
-
-# Regression test for Bug 14206
-$dbh->do( q|INSERT INTO letter(branchcode,module,code,name,is_html,title,content,message_transport_type) VALUES ('FFL','my module','my code','my name',1,?,?,'print')|, undef, $title, $content );
-my $letter14206_a = C4::Letters::getletter('my module', 'my code', 'FFL' );
-is( $letter14206_a->{message_transport_type}, 'print', 'Bug 14206 - message_transport_type not passed, correct mtt detected' );
-my $letter14206_b = C4::Letters::getletter('my module', 'my code', 'FFL', 'print');
-is( $letter14206_b->{message_transport_type}, 'print', 'Bug 14206 - message_transport_type passed, correct mtt detected'  );
-
-# test for overdue_notices.pl
-my $overdue_rules = {
-    letter1         => 'my code',
-};
-my $i = 1;
-my $branchcode = 'FFL';
-my $letter14206_c = C4::Letters::getletter('my module', $overdue_rules->{"letter$i"}, $branchcode);
-is( $letter14206_c->{message_transport_type}, 'print', 'Bug 14206 - correct mtt detected for call from overdue_notices.pl' );
-
 # GetPreparedLetter
 t::lib::Mocks::mock_preference('OPACBaseURL', 'http://thisisatest.com');
 t::lib::Mocks::mock_preference( 'SendAllEmailsTo', '' );
index faf80e7..d6db271 100755 (executable)
@@ -48,6 +48,7 @@ use C4::Output;
 use C4::Letters;
 use C4::Log;
 
+use Koha::Notice::Templates;
 use Koha::Patron::Attribute::Types;
 
 # $protected_letters = protected_letters()
@@ -303,33 +304,39 @@ sub add_validate {
         my $is_html = $input->param("is_html_$mtt\_$lang");
         my $title   = shift @title;
         my $content = shift @content;
-        my $letter = C4::Letters::getletter( $oldmodule, $code, $branchcode, $mtt, $lang );
+        my $letter = Koha::Notice::Templates->find(
+            {
+                module                 => $oldmodule,
+                code                   => $code,
+                branchcode             => $branchcode,
+                message_transport_type => $mtt,
+                lang                   => $lang
+            }
+        );
 
-        # getletter can return the default letter even if we pass a branchcode
-        # If we got the default one and we needed the specific one, we didn't get the one we needed!
-        if ( $letter and $branchcode and $branchcode ne $letter->{branchcode} ) {
-            $letter = undef;
-        }
         unless ( $title and $content ) {
             # Delete this mtt if no title or content given
             delete_confirmed( $branchcode, $oldmodule, $code, $mtt, $lang );
             next;
         }
-        elsif ( $letter and $letter->{message_transport_type} eq $mtt and $letter->{lang} eq $lang ) {
-            logaction( 'NOTICES', 'MODIFY', $letter->{id}, $content,
+        elsif ( $letter ) {
+            logaction( 'NOTICES', 'MODIFY', $letter->id, $content,
                 'Intranet' )
               if ( C4::Context->preference("NoticesLog")
-                && $content ne $letter->{content} );
-            $dbh->do(
-                q{
-                    UPDATE letter
-                    SET branchcode = ?, module = ?, name = ?, is_html = ?, title = ?, content = ?, lang = ?
-                    WHERE branchcode = ? AND module = ? AND code = ? AND message_transport_type = ? AND lang = ?
-                },
-                undef,
-                $branchcode || '', $module, $name, $is_html || 0, $title, $content, $lang,
-                $branchcode, $oldmodule, $code, $mtt, $lang
-            );
+                && $content ne $letter->content );
+
+            $letter->set(
+                {
+                    branchcode => $branchcode || '',
+                    module     => $module,
+                    name       => $name,
+                    is_html    => $is_html || 0,
+                    title      => $title,
+                    content    => $content,
+                    lang       => $lang
+                }
+            )->store;
+
         } else {
             my $letter = Koha::Notice::Template->new(
                 {
@@ -357,10 +364,10 @@ sub add_validate {
 sub delete_confirm {
     my ($branchcode, $module, $code) = @_;
     my $dbh = C4::Context->dbh;
-    my $letter = C4::Letters::getletter($module, $code, $branchcode);
-    my @values = values %$letter;
+    my $letter = Koha::Notice::Templates->search(
+        { module => $module, code => $code, branchcode => $branchcode } );
     $template->param(
-        letter => $letter,
+        letter => $letter ? $letter->next : undef,
     );
     return;
 }