Attachment 121580 Details for Bug 28487 – Bug 28487: Remove getletter

[patch] Bug 28487: Remove getletter

Bug-28487-Remove-getletter.patch (text/plain), 18.81 KB, created by Jonathan Druart on 2021-06-03 11:02:48 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2021-06-03 11:02:48 UTC

Size: 18.81 KB

patch

obsolete

>From e4b17559e507038eb81cb12c3866e2d78cd8db83 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Thu, 3 Jun 2021 13:01:43 +0200
>Subject: [PATCH] Bug 28487: Remove getletter
>
>---
> C4/Letters.pm                         | 49 ++++++-------------
> Koha/Notice/Templates.pm              | 26 ++++++++++
> misc/cronjobs/holds/holds_reminder.pl | 15 +++++-
> misc/cronjobs/overdue_notices.pl      | 20 +++++++-
> pos/printreceipt.pl                   | 12 ++++-
> t/db_dependent/Koha/Notices.t         | 70 ++++++++++++++++++++++++++-
> t/db_dependent/Letters.t              | 49 +------------------
> tools/letter.pl                       | 51 ++++++++++---------
> 8 files changed, 181 insertions(+), 111 deletions(-)
>
>diff --git a/C4/Letters.pm b/C4/Letters.pm
>index b8de25bdc89..d2a3f1265be 100644
>--- a/C4/Letters.pm
>+++ b/C4/Letters.pm
>@@ -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} || {};
>diff --git a/Koha/Notice/Templates.pm b/Koha/Notice/Templates.pm
>index 377a95690ac..152a8f13e39 100644
>--- a/Koha/Notice/Templates.pm
>+++ b/Koha/Notice/Templates.pm
>@@ -35,6 +35,32 @@ Koha::Notice::Templates - Koha notice template Object set class, related to the
> 
> =cut
> 
>+sub find_effective_template {
>+    my ( $self, $params ) = @_;
>+
>+    #$params = {%$params};
>+    $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
>diff --git a/misc/cronjobs/holds/holds_reminder.pl b/misc/cronjobs/holds/holds_reminder.pl
>index 49bccfdcde6..356e3ba08f8 100755
>--- a/misc/cronjobs/holds/holds_reminder.pl
>+++ b/misc/cronjobs/holds/holds_reminder.pl
>@@ -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;
>     }
>diff --git a/misc/cronjobs/overdue_notices.pl b/misc/cronjobs/overdue_notices.pl
>index bf1a370a111..74c1fc3789c 100755
>--- a/misc/cronjobs/overdue_notices.pl
>+++ b/misc/cronjobs/overdue_notices.pl
>@@ -624,7 +624,14 @@ END_SQL
>                     }
>                 }
> 
>-                my $letter = C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode, undef, $patron->lang );
>+                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|;
>@@ -715,7 +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 ) ? 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,
>diff --git a/pos/printreceipt.pl b/pos/printreceipt.pl
>index 8f18f6bc6b7..a387b8ea4c6 100755
>--- a/pos/printreceipt.pl
>+++ b/pos/printreceipt.pl
>@@ -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,
>diff --git a/t/db_dependent/Koha/Notices.t b/t/db_dependent/Koha/Notices.t
>index dc9646f4b81..c6c47054318 100755
>--- a/t/db_dependent/Koha/Notices.t
>+++ b/t/db_dependent/Koha/Notices.t
>@@ -19,12 +19,13 @@
> 
> 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;
> 
>diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t
>index cd5d404cc9e..f916f0865de 100755
>--- 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 => 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', '' );
>diff --git a/tools/letter.pl b/tools/letter.pl
>index faf80e7d393..d6db271a6c5 100755
>--- a/tools/letter.pl
>+++ b/tools/letter.pl
>@@ -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;
> }
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 28487: 121580 | 121627 | 121726 | 121853