From b82c91d8cb99e10da8e78477de4a68352b562955 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 3 Jun 2021 13:01:43 +0200 Subject: [PATCH] Bug 28514: Remove getletter 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 Signed-off-by: Marcel de Rooy [EDIT] Amended by removing comment for $params={%$params} JD amended patch: * Add missing POD * Fix spelling (dont ==> don't) Signed-off-by: Jonathan Druart --- C4/Letters.pm | 49 ++++++------------- Koha/Notice/Templates.pm | 46 ++++++++++++++++++ misc/cronjobs/holds/holds_reminder.pl | 15 +++++- misc/cronjobs/overdue_notices.pl | 22 +++++++-- pos/printreceipt.pl | 12 ++++- t/db_dependent/Koha/Notices.t | 70 ++++++++++++++++++++++++++- t/db_dependent/Letters.t | 49 +------------------ tools/letter.pl | 51 ++++++++++--------- 8 files changed, 201 insertions(+), 113 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index b8de25bdc8..d2a3f1265b 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 377a95690a..6abc1b36d3 100644 --- a/Koha/Notice/Templates.pm +++ b/Koha/Notice/Templates.pm @@ -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 diff --git a/misc/cronjobs/holds/holds_reminder.pl b/misc/cronjobs/holds/holds_reminder.pl index 49bccfdcde..356e3ba08f 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 16d708c0dc..74c1fc3789 100755 --- a/misc/cronjobs/overdue_notices.pl +++ b/misc/cronjobs/overdue_notices.pl @@ -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, diff --git a/pos/printreceipt.pl b/pos/printreceipt.pl index 8f18f6bc6b..a387b8ea4c 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 dc9646f4b8..c6c4705431 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 cd5d404cc9..f916f0865d 100755 --- 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 => 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 faf80e7d39..d6db271a6c 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.39.5