From e2eb15ecd4f979e1fbe5ec9dcbd5d9f4156b8784 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Thu, 6 Aug 2020 12:43:17 -0300 Subject: [PATCH] Bug 22343: Make C4::Letters use the new SMTP server config This patch makes the different methods in C4::Letters use: - Koha::SMTP::Servers: to get the effective SMTP server for the library or the fallback default if no library in context. - New Koha::Email->create method for crafting the email envelope for sending. The tests are adapted so they behave the same way, but the trapped (in the mock) $email object has the right type and its attributes are accessed correctly. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Letters.t => SUCCESS: Tests pass. YAY! 3. Sign off :-D --- C4/Letters.pm | 174 +++++++++++++++++++++++++++------------ t/db_dependent/Letters.t | 67 ++++++++++----- 2 files changed, 166 insertions(+), 75 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index dbbb6fdf9f..4b4b163b4e 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -20,13 +20,15 @@ package C4::Letters; use Modern::Perl; use MIME::Lite; -use Mail::Sendmail; use Date::Calc qw( Add_Delta_Days ); +use Email::Sender::Simple qw(sendmail); use Encode; use Carp; use Template; use Module::Load::Conditional qw(can_load); +use Try::Tiny; + use C4::Members; use C4::Log; use C4::SMS; @@ -39,6 +41,7 @@ use Koha::Notice::Messages; use Koha::Notice::Templates; use Koha::DateUtils qw( format_sqldatetime dt_from_string ); use Koha::Patrons; +use Koha::SMTP::Servers; use Koha::Subscriptions; use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); @@ -338,27 +341,38 @@ sub SendAlerts { ) or return; # ... then send mail - my $message = Koha::Email->new(); - my %mail = $message->create_message_headers( + my $mail = Koha::Email->create( { to => $email, from => $library->branchemail, replyto => $library->branchreplyto, sender => $library->branchreturnpath, subject => Encode::encode( "UTF-8", "" . $letter->{title} ), - message => $letter->{'is_html'} - ? _wrap_html( Encode::encode( "UTF-8", $letter->{'content'} ), - Encode::encode( "UTF-8", "" . $letter->{'title'} )) - : Encode::encode( "UTF-8", "" . $letter->{'content'} ), - contenttype => $letter->{'is_html'} - ? 'text/html; charset="utf-8"' - : 'text/plain; charset="utf-8"', } ); - unless( Mail::Sendmail::sendmail(%mail) ) { - carp $Mail::Sendmail::error; - return { error => $Mail::Sendmail::error }; + + if ( $letter->{is_html} ) { + $mail->html_body( + _wrap_html( + Encode::encode( "UTF-8", $letter->{content} ), + Encode::encode( "UTF-8", "" . $letter->{title} ) + ) + ); } + else { + $mail->text_body( + Encode::encode( "UTF-8", "" . $letter->{'content'} ) + ); + } + + try { + my $smtp_server = Koha::SMTP::Servers->get_effective_server({ library => $library }); + Email::Sender::Simple::sendmail( $mail, { transport => $smtp_server->transport } ); + } + catch { + carp "$_"; + return { error => "$_" }; + }; } } elsif ( $type eq 'claimacquisition' or $type eq 'claimissues' or $type eq 'orderacquisition' ) { @@ -477,8 +491,7 @@ sub SendAlerts { # ... then send mail my $library = Koha::Libraries->find( $userenv->{branch} ); - my $email = Koha::Email->new(); - my %mail = $email->create_message_headers( + my $mail = Koha::Email->create( { to => join( ',', @email ), cc => join( ',', @cc ), @@ -487,28 +500,38 @@ sub SendAlerts { C4::Context->preference("ClaimsBccCopy") && ( $type eq 'claimacquisition' || $type eq 'claimissues' ) - ) ? ( bcc => $userenv->{emailaddress} ) + ) + ? ( bcc => $userenv->{emailaddress} ) : () ), from => $library->branchemail || C4::Context->preference('KohaAdminEmailAddress'), subject => Encode::encode( "UTF-8", "" . $letter->{title} ), - message => $letter->{'is_html'} ? _wrap_html( - Encode::encode( "UTF-8", $letter->{'content'} ), - Encode::encode( "UTF-8", "" . $letter->{'title'} ) - ) - : Encode::encode( "UTF-8", "" . $letter->{'content'} ), - contenttype => $letter->{'is_html'} - ? 'text/html; charset="utf-8"' - : 'text/plain; charset="utf-8"', } ); - unless ( Mail::Sendmail::sendmail(%mail) ) { - carp $Mail::Sendmail::error; - return { error => $Mail::Sendmail::error }; + if ( $letter->{is_html} ) { + $mail->html_body( + _wrap_html( + Encode::encode( "UTF-8", $letter->{'content'} ), + Encode::encode( "UTF-8", "" . $letter->{'title'} ) + ) + ); + } + else { + $mail->text_body( + Encode::encode( "UTF-8", "" . $letter->{'content'} ) ); } + try { + my $smtp_server = Koha::SMTP::Servers->get_effective_server({ library => $library }); + Email::Sender::Simple::sendmail( $mail, { transport => $smtp_server->transport } ); + } + catch { + carp "$_"; + return { error => "$_" }; + }; + logaction( "ACQUISITION", $action, @@ -523,27 +546,26 @@ sub SendAlerts { } # send an "account details" notice to a newly created user elsif ( $type eq 'members' ) { - my $library = Koha::Libraries->find( $externalid->{branchcode} )->unblessed; + my $library = Koha::Libraries->find( $externalid->{branchcode} ); my $letter = GetPreparedLetter ( module => 'members', letter_code => $letter_code, branchcode => $externalid->{'branchcode'}, lang => $externalid->{lang} || 'default', tables => { - 'branches' => $library, + 'branches' => $library->unblessed, 'borrowers' => $externalid->{'borrowernumber'}, }, substitute => { 'borrowers.password' => $externalid->{'password'} }, want_librarian => 1, ) or return; return { error => "no_email" } unless $externalid->{'emailaddr'}; - my $email = Koha::Email->new(); - my %mail = $email->create_message_headers( + my $mail = Koha::Email->create( { to => $externalid->{'emailaddr'}, - from => $library->{branchemail}, - replyto => $library->{branchreplyto}, - sender => $library->{branchreturnpath}, + from => $library->branchemail, + replyto => $library->branchreplyto, + sender => $library->branchreturnpath, subject => Encode::encode( "UTF-8", "" . $letter->{'title'} ), message => $letter->{'is_html'} ? _wrap_html( Encode::encode( "UTF-8", $letter->{'content'} ), @@ -554,10 +576,14 @@ sub SendAlerts { : 'text/plain; charset="utf-8"', } ); - unless( Mail::Sendmail::sendmail(%mail) ) { - carp $Mail::Sendmail::error; - return { error => $Mail::Sendmail::error }; + try { + my $smtp_server = Koha::SMTP::Servers->get_effective_server({ library => $library }); + Email::Sender::Simple::sendmail( $mail, { transport => $smtp_server->transport } ); } + catch { + carp "$_"; + return { error => "$_" }; + }; } # If we come here, return an OK status @@ -1296,17 +1322,20 @@ sub _send_message_by_email { my $content = encode('UTF-8', $message->{'content'}); my $content_type = $message->{'content_type'} || 'text/plain; charset="UTF-8"'; my $is_html = $content_type =~ m/html/io; + my $branch_email = undef; my $branch_replyto = undef; my $branch_returnpath = undef; + my $library; + if ($patron) { - my $library = $patron->library; + $library = $patron->library; $branch_email = $library->branchemail; $branch_replyto = $library->branchreplyto; $branch_returnpath = $library->branchreturnpath; } - my $email = Koha::Email->new(); - my %sendmail_params = $email->create_message_headers( + + my $email = Koha::Email->create( { to => $to_address, ( @@ -1314,29 +1343,66 @@ sub _send_message_by_email { ? ( bcc => C4::Context->preference('NoticeBcc') ) : () ), - from => $message->{'from_address'} || $branch_email, + from => $message->{'from_address'} || $branch_email, replyto => $message->{'reply_address'} || $branch_replyto, sender => $branch_returnpath, - subject => $subject, - message => $is_html ? _wrap_html( $content, $subject ) : $content, - contenttype => $content_type + subject => $subject } ); - $sendmail_params{'Auth'} = {user => $username, pass => $password, method => $method} if $username; + if ( $is_html ) { + $email->html_body( + _wrap_html( $content, $subject ) + ); + } + else { + $email->text_body( $content ); + } + + my $smtp_server; + if ( $library ) { + $smtp_server = Koha::SMTP::Servers->get_effective_server({ library => $library }); + } + else { + $smtp_server = Koha::SMTP::Servers->get_default; + } - _update_message_to_address($message->{'message_id'},$sendmail_params{To}) if !$message->{to_address} || $message->{to_address} ne $sendmail_params{To}; #if initial message address was empty, coming here means that a to address was found and queue should be updated; same if to address was overriden by create_message_headers + if ( $username ) { + $smtp_server->set( + { + sasl_username => $username, + sasl_password => $password, + } + ); + } - if ( Mail::Sendmail::sendmail( %sendmail_params ) ) { - _set_message_status( { message_id => $message->{'message_id'}, - status => 'sent' } ); +# if initial message address was empty, coming here means that a to address was found and +# queue should be updated; same if to address was overriden by create_message_headers + _update_message_to_address( $message->{'message_id'}, $email->email->header('To') ) + if !$message->{to_address} + || $message->{to_address} ne $email->email->header('To'); + + try { + Email::Sender::Simple::sendmail( $email, { transport => $smtp_server->transport } ); + + _set_message_status( + { + message_id => $message->{'message_id'}, + status => 'sent' + } + ); return 1; - } else { - _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); - carp $Mail::Sendmail::error; - return; } + catch { + _set_message_status( + { + message_id => $message->{'message_id'}, + status => 'failed' + } + ); + carp "$_"; + return; + }; } sub _wrap_html { diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 2389bf33d1..ddcb26d03f 100644 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -24,13 +24,14 @@ use Test::Warn; use MARC::Record; -my %mail; -my $module = new Test::MockModule('Mail::Sendmail'); -$module->mock( +my ( $email_object, $sendmail_params ); + +my $email_sender_module = Test::MockModule->new('Email::Sender::Simple'); +$email_sender_module->mock( 'sendmail', sub { + ( $email_object, $sendmail_params ) = @_; warn "Fake sendmail"; - %mail = @_; } ); @@ -444,8 +445,8 @@ warning_is { "Fake sendmail", "SendAlerts is using the mocked sendmail routine (orderacquisition)"; is($err, 1, "Successfully sent order."); -is($mail{'To'}, 'testemail@mydomain.com', "mailto correct in sent order"); -is($mail{'Message'}, 'my vendor|John Smith|Ordernumber ' . $ordernumber . ' (Silence in the library) (1 ordered)', 'Order notice text constructed successfully'); +is($email_object->email->header('To'), 'testemail@mydomain.com', "mailto correct in sent order"); +is($email_object->email->body, 'my vendor|John Smith|Ordernumber ' . $ordernumber . ' (Silence in the library) (1 ordered)', 'Order notice text constructed successfully'); $dbh->do(q{DELETE FROM letter WHERE code = 'TESTACQORDER';}); warning_like { @@ -462,8 +463,8 @@ warning_is { "SendAlerts is using the mocked sendmail routine"; is($err, 1, "Successfully sent claim"); -is($mail{'To'}, 'testemail@mydomain.com', "mailto correct in sent claim"); -is($mail{'Message'}, 'my vendor|John Smith|Ordernumber ' . $ordernumber . ' (Silence in the library) (1 ordered)', 'Claim notice text constructed successfully'); +is($email_object->email->header('To'), 'testemail@mydomain.com', "mailto correct in sent claim"); +is($email_object->email->body, 'my vendor|John Smith|Ordernumber ' . $ordernumber . ' (Silence in the library) (1 ordered)', 'Claim notice text constructed successfully'); } { @@ -496,14 +497,16 @@ my $borrowernumber = $patron->borrowernumber; my $subscription = Koha::Subscriptions->find( $subscriptionid ); $subscription->add_subscriber( $patron ); +t::lib::Mocks::mock_userenv({ branch => $library->{branchcode} }); my $err2; warning_is { $err2 = SendAlerts( 'issue', $serial->{serialid}, 'RLIST' ) } "Fake sendmail", "SendAlerts is using the mocked sendmail routine"; + is($err2, 1, "Successfully sent serial notification"); -is($mail{'To'}, 'john.smith@test.de', "mailto correct in sent serial notification"); -is($mail{'Message'}, 'Silence in the library,'.$subscriptionid.',No. 0', 'Serial notification text constructed successfully'); +is($email_object->email->header('To'), 'john.smith@test.de', "mailto correct in sent serial notification"); +is($email_object->email->body, 'Silence in the library,'.$subscriptionid.',No. 0', 'Serial notification text constructed successfully'); t::lib::Mocks::mock_preference( 'SendAllEmailsTo', 'robert.tables@mail.com' ); @@ -512,12 +515,12 @@ warning_is { $err3 = SendAlerts( 'issue', $serial->{serialid}, 'RLIST' ) } "Fake sendmail", "SendAlerts is using the mocked sendmail routine"; -is($mail{'To'}, 'robert.tables@mail.com', "mailto address overwritten by SendAllMailsTo preference"); +is($email_object->email->header('To'), 'robert.tables@mail.com', "mailto address overwritten by SendAllMailsTo preference"); } t::lib::Mocks::mock_preference( 'SendAllEmailsTo', '' ); subtest 'SendAlerts - claimissue' => sub { - plan tests => 8; + plan tests => 9; use C4::Serials; @@ -585,9 +588,14 @@ subtest 'SendAlerts - claimissue' => sub { $err = SendAlerts( 'claimissues', \@serialids , 'TESTSERIALCLAIM' ) } "Fake sendmail", "SendAlerts is using the mocked sendmail routine (claimissues)"; - is($err, 1, "Successfully sent claim"); - is($mail{'To'}, 'testemail@mydomain.com', "mailto correct in sent claim"); - is($mail{'Message'}, "$serialids[0]|2013-01-01|Silence in the library|xxxx-yyyy", 'Serial claim letter for 1 issue constructed successfully'); + is( $err, 1, "Successfully sent claim" ); + is( $email_object->email->header('To'), + 'testemail@mydomain.com', "mailto correct in sent claim" ); + is( + $email_object->email->body, + "$serialids[0]|2013-01-01|Silence in the library|xxxx-yyyy", + 'Serial claim letter for 1 issue constructed successfully' + ); } { @@ -597,8 +605,17 @@ subtest 'SendAlerts - claimissue' => sub { ($serials_count, @serials) = GetSerials($subscriptionid); push @serialids, ($serials[1]->{serialid}); - $err = SendAlerts( 'claimissues', \@serialids , 'TESTSERIALCLAIM' ); - is($mail{'Message'}, "$serialids[0]|2013-01-01|Silence in the library|xxxx-yyyy\n$serialids[1]|2013-01-01|Silence in the library|xxxx-yyyy", "Serial claim letter for 2 issues constructed successfully"); + warning_is { $err = SendAlerts( 'claimissues', \@serialids, 'TESTSERIALCLAIM' ); } + "Fake sendmail", + "SendAlerts is using the mocked sendmail routine (claimissues)"; + + is( + $email_object->email->body, + "$serialids[0]|2013-01-01|Silence in the library|xxxx-yyyy" + . $email_object->email->crlf + . "$serialids[1]|2013-01-01|Silence in the library|xxxx-yyyy", + "Serial claim letter for 2 issues constructed successfully" + ); $dbh->do(q{DELETE FROM letter WHERE code = 'TESTSERIALCLAIM';}); warning_like { @@ -718,7 +735,7 @@ subtest 'TranslateNotices' => sub { subtest 'SendQueuedMessages' => sub { - plan tests => 6; + plan tests => 9; t::lib::Mocks::mock_preference( 'SMSSendDriver', 'Email' ); t::lib::Mocks::mock_preference('EmailSMSSendDriverFromAddress', ''); @@ -735,7 +752,10 @@ subtest 'SendQueuedMessages' => sub { my $sms_pro = $builder->build_object({ class => 'Koha::SMS::Providers', value => { domain => 'kidclamp.rocks' } }); $patron->set( { smsalertnumber => '5555555555', sms_provider_id => $sms_pro->id() } )->store; $message_id = C4::Letters::EnqueueLetter($my_message); #using datas set around line 95 and forward - C4::Letters::SendQueuedMessages(); + + warning_is { C4::Letters::SendQueuedMessages(); } + "Fake sendmail", + "SendAlerts is using the mocked sendmail routine (claimissues)"; my $message = $schema->resultset('MessageQueue')->search({ borrowernumber => $borrowernumber, @@ -754,7 +774,9 @@ subtest 'SendQueuedMessages' => sub { t::lib::Mocks::mock_preference('EmailSMSSendDriverFromAddress', 'override@example.com'); $message_id = C4::Letters::EnqueueLetter($my_message); - C4::Letters::SendQueuedMessages(); + warning_is { C4::Letters::SendQueuedMessages(); } + "Fake sendmail", + "SendAlerts is using the mocked sendmail routine (claimissues)"; $message = $schema->resultset('MessageQueue')->search({ borrowernumber => $borrowernumber, @@ -777,7 +799,10 @@ subtest 'SendQueuedMessages' => sub { }); is ( $number_attempted, 0, 'There were no password reset messages for SendQueuedMessages to attempt.' ); - C4::Letters::SendQueuedMessages(); + warning_is { C4::Letters::SendQueuedMessages(); } + "Fake sendmail", + "SendAlerts is using the mocked sendmail routine (claimissues)"; + my $sms_message_address = $schema->resultset('MessageQueue')->search({ borrowernumber => $borrowernumber, status => 'sent' -- 2.28.0