Bug 22343: Make C4::Letters use the new SMTP server config
authorTomas Cohen Arazi <tomascohen@theke.io>
Thu, 6 Aug 2020 15:43:17 +0000 (12:43 -0300)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 2 Oct 2020 08:54:40 +0000 (10:54 +0200)
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 <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
C4/Letters.pm
t/db_dependent/Letters.t

index dbbb6fd..6981e95 100644 (file)
@@ -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 {
index 2389bf3..9a629bd 100755 (executable)
@@ -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'