Attachment 108139 Details for Bug 22343 – Bug 22343: Make C4::Letters use the new SMTP server config

[patch] Bug 22343: Make C4::Letters use the new SMTP server config

Bug-22343-Make-C4Letters-use-the-new-SMTP-server-c.patch (text/plain), 19.62 KB, created by Martin Renvoize (ashimema) on 2020-08-12 16:08:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2020-08-12 16:08:00 UTC

Size: 19.62 KB

patch

obsolete

>From da677ac85550c4f9401513331936034b1bc432dc Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>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: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> 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.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 22343: 107598 | 107599 | 107600 | 107601 | 107602 | 107603 | 107604 | 107605 | 107987 | 107988 | 107989 | 107990 | 107991 | 107992 | 107993 | 107994 | 107995 | 107996 | 107997 | 107998 | 107999 | 108009 | 108107 | 108108 | 108109 | 108110 | 108111 | 108112 | 108113 | 108114 | 108115 | 108116 | 108117 | 108118 | 108119 | 108131 | 108132 | 108133 | 108134 | 108135 | 108136 | 108137 | 108138 | 108139 | 108140 | 108141 | 108142 | 108143 | 108529 | 108537 | 108538 | 108539 | 108540 | 108541 | 108542 | 108543 | 108544 | 108545 | 108546 | 108547 | 108548 | 108549 | 108609 | 108638 | 108642 | 108643 | 108644 | 108645 | 108646 | 108647 | 108648 | 108649 | 108650 | 108651 | 108652 | 108653 | 108654 | 108655 | 108656 | 108715 | 108716 | 109003 | 109004 | 109005 | 109006 | 109007 | 109008 | 109009 | 109010 | 109011 | 109012 | 109013 | 109014 | 109015 | 109016 | 109017 | 109018 | 109155 | 109156 | 109157 | 109158 | 109159 | 109160 | 109161 | 109162 | 109163 | 109164 | 109165 | 109166 | 109167 | 109168 | 109169 | 109170 | 109601 | 109602 | 109603 | 109604 | 109605 | 109606 | 109607 | 109608 | 109609 | 109610 | 109611 | 109612 | 109613 | 109614 | 109615 | 109616 | 109723 | 109724 | 109725 | 109726 | 109727 | 109728 | 109729 | 109730 | 109731 | 109732 | 109733 | 109734 | 109735 | 109736 | 109737 | 109738 | 109739 | 109740 | 109787 | 109788 | 111048 | 111087 | 111088 | 111089 | 111090 | 111154 | 111165 | 111166 | 111175 | 111176 | 111213 | 111215 | 111216 | 111442 | 111482 | 113240 | 113827 | 113847