From 3a80954aa3538785af4064f5aa0961c28a9b0685 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 Signed-off-by: Kyle M Hall Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- C4/Letters.pm | 200 ++++++++++++++++++++++++--------------- t/db_dependent/Letters.t | 107 ++++++++++++++------- 2 files changed, 199 insertions(+), 108 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index dbbb6fdf9f..6981e95732 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -20,13 +20,14 @@ package C4::Letters; use Modern::Perl; use MIME::Lite; -use Mail::Sendmail; use Date::Calc qw( Add_Delta_Days ); 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 +40,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); @@ -337,28 +339,31 @@ sub SendAlerts { want_librarian => 1, ) or return; - # ... then send mail - my $message = Koha::Email->new(); - my %mail = $message->create_message_headers( + # FIXME: This 'default' behaviour should be moved to Koha::Email + 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"', + to => $email, + from => $library->branchemail, + reply_to => $library->branchreplyto, + sender => $library->branchreturnpath, + subject => "" . $letter->{title}, } ); - unless( Mail::Sendmail::sendmail(%mail) ) { - carp $Mail::Sendmail::error; - return { error => $Mail::Sendmail::error }; + + if ( $letter->{is_html} ) { + $mail->html_body( _wrap_html( $letter->{content}, "" . $letter->{title} ) ); + } + else { + $mail->text_body( $letter->{content} ); } + + try { + $mail->send_or_die({ transport => $library->smtp_server->transport }); + } + catch { + carp "$_"; + return { error => "$_" }; + }; } } elsif ( $type eq 'claimacquisition' or $type eq 'claimissues' or $type eq 'orderacquisition' ) { @@ -477,8 +482,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,27 +491,30 @@ 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"', + subject => "" . $letter->{title}, } ); - unless ( Mail::Sendmail::sendmail(%mail) ) { - carp $Mail::Sendmail::error; - return { error => $Mail::Sendmail::error }; + if ( $letter->{is_html} ) { + $mail->html_body( _wrap_html( $letter->{content}, "" . $letter->{title} ) ); } + else { + $mail->text_body( "" . $letter->{content} ); + } + + try { + $mail->send_or_die({ transport => $library->smtp_server->transport }); + } + catch { + carp "$_"; + return { error => "$_" }; + }; logaction( "ACQUISITION", @@ -523,41 +530,46 @@ 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( - { - to => $externalid->{'emailaddr'}, - 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"', + try { + + # FIXME: This 'default' behaviour should be moved to Koha::Email + my $mail = Koha::Email->create( + { + to => $externalid->{'emailaddr'}, + from => $library->branchemail, + reply_to => $library->branchreplyto, + sender => $library->branchreturnpath, + subject => "" . $letter->{'title'}, + } + ); + + if ( $letter->{is_html} ) { + $mail->html_body( _wrap_html( $letter->{content}, "" . $letter->{title} ) ); } - ); - unless( Mail::Sendmail::sendmail(%mail) ) { - carp $Mail::Sendmail::error; - return { error => $Mail::Sendmail::error }; + else { + $mail->text_body( $letter->{content} ); + } + + $mail->send_or_die({ transport => $library->smtp_server->transport }); } + catch { + carp "$_"; + return { error => "$_" }; + }; } # If we come here, return an OK status @@ -1296,17 +1308,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 +1329,66 @@ sub _send_message_by_email { ? ( bcc => C4::Context->preference('NoticeBcc') ) : () ), - 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 + from => $message->{'from_address'} || $branch_email, + reply_to => $message->{'reply_address'} || $branch_replyto, + sender => $branch_returnpath, + subject => "" . $message->{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 ); + } - _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 + my $smtp_server; + if ( $library ) { + $smtp_server = $library->smtp_server; + } + else { + $smtp_server = Koha::SMTP::Servers->get_default; + } - if ( Mail::Sendmail::sendmail( %sendmail_params ) ) { - _set_message_status( { message_id => $message->{'message_id'}, - status => 'sent' } ); + if ( $username ) { + $smtp_server->set( + { + sasl_username => $username, + sasl_password => $password, + } + ); + } + +# 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->send_or_die({ 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..9a629bdfbf 100755 --- 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( - 'sendmail', +my ( $email_object, $sendmail_params ); + +my $email_sender_module = Test::MockModule->new('Email::Stuffer'); +$email_sender_module->mock( + 'send_or_die', sub { - warn "Fake sendmail"; - %mail = @_; + ( $email_object, $sendmail_params ) = @_; + warn "Fake send_or_die"; } ); @@ -62,6 +63,11 @@ $dbh->do(q|DELETE FROM message_transport_types|); my $library = $builder->build({ source => 'Branch', + value => { + branchemail => 'branchemail@address.com', + branchreplyto => 'branchreplyto@address.com', + branchreturnpath => 'branchreturnpath@address.com', + } }); my $patron_category = $builder->build({ source => 'Category' })->{categorycode}; my $date = dt_from_string; @@ -407,7 +413,16 @@ if (C4::Context->preference('marcflavour') eq 'UNIMARC') { ); } -my $logged_in_user = $builder->build_object({ class => 'Koha::Patrons', value => { branchcode => $library->{branchcode} }}); +my $logged_in_user = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + branchcode => $library->{branchcode}, + email => 'some@email.com' + } + } +); + t::lib::Mocks::mock_userenv({ patron => $logged_in_user }); ($biblionumber, $biblioitemnumber) = AddBiblio($bib, ''); @@ -439,13 +454,13 @@ t::lib::Mocks::mock_preference( 'LetterLog', 'on' ); t::lib::Mocks::mock_preference( 'KohaAdminEmailAddress', 'library@domain.com' ); { -warning_is { +warning_like { $err = SendAlerts( 'orderacquisition', $basketno , 'TESTACQORDER' ) } - "Fake sendmail", + qr|Fake send_or_die|, "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 { @@ -456,14 +471,14 @@ is($err->{'error'}, 'no_letter', "No TESTACQORDER letter was defined."); } { -warning_is { +warning_like { $err = SendAlerts( 'claimacquisition', [ $ordernumber ], 'TESTACQCLAIM' ) } - "Fake sendmail", + qr|Fake send_or_die|, "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,28 +511,30 @@ 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 { +warning_like { $err2 = SendAlerts( 'issue', $serial->{serialid}, 'RLIST' ) } - "Fake sendmail", + qr|Fake send_or_die|, "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' ); my $err3; -warning_is { +warning_like { $err3 = SendAlerts( 'issue', $serial->{serialid}, 'RLIST' ) } - "Fake sendmail", + qr|Fake send_or_die|, "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; @@ -581,13 +598,18 @@ subtest 'SendAlerts - claimissue' => sub { t::lib::Mocks::mock_preference( 'KohaAdminEmailAddress', 'library@domain.com' ); { - warning_is { + warning_like { $err = SendAlerts( 'claimissues', \@serialids , 'TESTSERIALCLAIM' ) } - "Fake sendmail", + qr|Fake send_or_die|, "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 +619,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_like { $err = SendAlerts( 'claimissues', \@serialids, 'TESTSERIALCLAIM' ); } + qr|Fake send_or_die|, + "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 +749,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 +766,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_like { C4::Letters::SendQueuedMessages(); } + qr|Fake send_or_die|, + "SendAlerts is using the mocked sendmail routine (claimissues)"; my $message = $schema->resultset('MessageQueue')->search({ borrowernumber => $borrowernumber, @@ -754,7 +788,9 @@ subtest 'SendQueuedMessages' => sub { t::lib::Mocks::mock_preference('EmailSMSSendDriverFromAddress', 'override@example.com'); $message_id = C4::Letters::EnqueueLetter($my_message); - C4::Letters::SendQueuedMessages(); + warning_like { C4::Letters::SendQueuedMessages(); } + qr|Fake send_or_die|, + "SendAlerts is using the mocked sendmail routine (claimissues)"; $message = $schema->resultset('MessageQueue')->search({ borrowernumber => $borrowernumber, @@ -777,7 +813,10 @@ subtest 'SendQueuedMessages' => sub { }); is ( $number_attempted, 0, 'There were no password reset messages for SendQueuedMessages to attempt.' ); - C4::Letters::SendQueuedMessages(); + warning_like { C4::Letters::SendQueuedMessages(); } + qr|Fake send_or_die|, + "SendAlerts is using the mocked sendmail routine (claimissues)"; + my $sms_message_address = $schema->resultset('MessageQueue')->search({ borrowernumber => $borrowernumber, status => 'sent' -- 2.39.5