Attachment 113403 Details for Bug 22818 – Bug 22818: (QA follow-up) Email addressing corrections

[patch] Bug 22818: (QA follow-up) Email addressing corrections

Bug-22818-QA-follow-up-Email-addressing-correction.patch (text/plain), 12.85 KB, created by Andrew Isherwood on 2020-11-10 14:18:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Isherwood

Created: 2020-11-10 14:18:13 UTC

Size: 12.85 KB

patch

obsolete

>From 33491ad80f00ba33ea2421f1fcc465c6f71fce05 Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Thu, 9 Jul 2020 08:40:22 +0100
>Subject: [PATCH] Bug 22818: (QA follow-up) Email addressing corrections
>
>This patch updates the patchset to use recnetly introduced methods for
>obtaining the correct inbound and reply email addresses for branches.
>
>We also move get_staff_to_address into Koha::Library alongside
>inbound_email_address and rename it to inbound_ill_address.
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>
>Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
>---
> Koha/Illrequest.pm              | 61 +++++++-------------------
> Koha/Library.pm                 | 18 ++++++++
> t/db_dependent/Illrequests.t    | 76 ++++++---------------------------
> t/db_dependent/Koha/Libraries.t | 53 ++++++++++++++++++++++-
> 4 files changed, 99 insertions(+), 109 deletions(-)
>
>diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm
>index 61d1b43948..4a3eb566cd 100644
>--- a/Koha/Illrequest.pm
>+++ b/Koha/Illrequest.pm
>@@ -29,7 +29,6 @@ use C4::Letters;
> use C4::Members;
> use Koha::Database;
> use Koha::DateUtils qw/ dt_from_string /;
>-use Koha::Email;
> use Koha::Exceptions::Ill;
> use Koha::Illcomments;
> use Koha::Illrequestattributes;
>@@ -1329,8 +1328,8 @@ sub generic_confirm {
>             "No target email addresses found. Either select at least one partner or check your ILL partner library records.")
>           if ( !$to );
>         # Create the from, replyto and sender headers
>-        my $from = $branch->branchillemail || $branch->branchemail;
>-        my $replyto = $branch->branchreplyto || $from;
>+        my $from = $branch->branchemail;
>+        my $replyto = $branch->inbound_ill_address;
>         Koha::Exceptions::Ill::NoLibraryEmail->throw(
>             "Your library has no usable email address. Please set it.")
>           if ( !$from );
>@@ -1350,7 +1349,8 @@ sub generic_confirm {
>             borrowernumber         => $self->borrowernumber,
>             message_transport_type => 'email',
>             to_address             => $to,
>-            from_address           => $from
>+            from_address           => $from,
>+            reply_address          => $replyto
>         };
> 
>         if ($letter) {
>@@ -1386,45 +1386,6 @@ sub generic_confirm {
>     }
> }
> 
>-=head3 get_staff_to_address
>-
>-    my $email = $request->get_staff_to_address();
>-
>-Get the email address to which staff notices should be sent
>-
>-=cut
>-
>-sub get_staff_to_address {
>-    my ( $self ) = @_;
>-
>-    # The various places we can get an ILL staff email address from
>-    # (In order of preference)
>-    #
>-    # Dedicated branch address
>-    my $library = Koha::Libraries->find( $self->branchcode );
>-    my $branch_ill_to = $library->branchillemail;
>-    # General purpose ILL address from syspref
>-    my $syspref = C4::Context->preference("ILLDefaultStaffEmail");
>-    # Branch general email address
>-    my $branch_to = $library->branchemail;
>-    # Last resort
>-    my $koha_admin = C4::Context->preference('KohaAdminEmailAddress');
>-
>-    my $to;
>-    if ($branch_ill_to) {
>-        $to = $branch_ill_to;
>-    } elsif ($syspref) {
>-        $to = $syspref;
>-    } elsif ($branch_to) {
>-        $to = $branch_to;
>-    } elsif ($koha_admin) {
>-        $to = $koha_admin;
>-    }
>-
>-    # $to will not be defined if we didn't find a usable address
>-    return $to;
>-}
>-
> =head3 send_patron_notice
> 
>     my $result = $request->send_patron_notice($notice_code);
>@@ -1456,6 +1417,12 @@ sub send_patron_notice {
>     });
>     my @transports = keys %{ $borrower_preferences->{transports} };
> 
>+    # Notice should come from the library where the request was placed,
>+    # not the patrons home library
>+    my $branch = Koha::Libraries->find($self->branchcode);
>+    my $from_address = $branch->branchemail;
>+    my $reply_address = $branch->inbound_ill_address;
>+
>     # Send the notice to the patron via the chosen transport methods
>     # and record the results
>     my @success = ();
>@@ -1470,6 +1437,8 @@ sub send_patron_notice {
>                 letter                 => $letter,
>                 borrowernumber         => $self->borrowernumber,
>                 message_transport_type => $transport,
>+                from_address           => $from_address,
>+                reply_address          => $reply_address
>             });
>             if ($result) {
>                 push @success, $transport;
>@@ -1515,7 +1484,7 @@ sub send_staff_notice {
> 
>     # Get the staff notices that have been assigned for sending in
>     # the syspref
>-    my $staff_to_send = C4::Context->preference('ILLSendStaffNotices');
>+    my $staff_to_send = C4::Context->preference('ILLSendStaffNotices') // q{};
> 
>     # If it hasn't been enabled in the syspref, we don't want to send it
>     if ($staff_to_send !~ /\b$notice_code\b/) {
>@@ -1530,7 +1499,8 @@ sub send_staff_notice {
>     });
> 
>     # Try and get an address to which to send staff notices
>-    my $to_address = scalar $self->get_staff_to_address;
>+    my $branch = Koha::Libraries->find($self->branchcode);
>+    my $to_address = $branch->inbound_ill_address;
> 
>     my $params = {
>         letter                 => $letter,
>@@ -1540,7 +1510,6 @@ sub send_staff_notice {
> 
>     if ($to_address) {
>         $params->{to_address} = $to_address;
>-        $params->{from_address} = $to_address;
>     } else {
>         return {
>             error => 'notice_no_create'
>diff --git a/Koha/Library.pm b/Koha/Library.pm
>index dd5459f343..86291b0c7f 100644
>--- a/Koha/Library.pm
>+++ b/Koha/Library.pm
>@@ -134,6 +134,24 @@ sub inbound_email_address {
>       || undef;
> }
> 
>+=head3 inbound_ill_address
>+
>+  my $to_email = Koha::Library->inbound_ill_address;
>+
>+Returns an effective email address which should be accessible to librarians at the branch
>+for inter library loans communication.
>+
>+=cut
>+
>+sub inbound_ill_address {
>+    my ($self) = @_;
>+
>+    return
>+         $self->branchillemail
>+      || C4::Context->preference('ILLDefaultStaffEmail')
>+      || $self->inbound_email_address;
>+}
>+
> =head3 library_groups
> 
> Return the Library groups of this library
>diff --git a/t/db_dependent/Illrequests.t b/t/db_dependent/Illrequests.t
>index 4064d405d7..cc929759b6 100755
>--- a/t/db_dependent/Illrequests.t
>+++ b/t/db_dependent/Illrequests.t
>@@ -715,7 +715,7 @@ subtest 'Backend core methods' => sub {
> 
> subtest 'Helpers' => sub {
> 
>-    plan tests => 21;
>+    plan tests => 20;
> 
>     $schema->storage->txn_begin;
> 
>@@ -817,62 +817,6 @@ subtest 'Helpers' => sub {
>         "Correct error when missing type"
>     );
> 
>-    #get_staff_to_address
>-    # Mock a KohaAdminEmailAddress syspref
>-    t::lib::Mocks::mock_preference(
>-        'KohaAdminEmailAddress',
>-        'kohaadmin@nowhere.com'
>-    );
>-    # No branch addresses defined and no ILLDefaultStaffEmail, so should
>-    # fall back to Koha admin address
>-    my $email_kohaadmin = $illrq_obj->get_staff_to_address;
>-    ok(
>-        $email_kohaadmin eq 'kohaadmin@nowhere.com',
>-        'get_staff_to_address falls back to Koha admin in the absence of other alternatives'
>-    );
>-    # General branch address defined, should fall back to that
>-    $builder->delete({ source => 'Branch', records => $illbrn });
>-    $illbrn = $builder->build({
>-        source => 'Branch',
>-        value => {
>-            branchcode => 'HDE',
>-            branchemail => 'branch@nowhere.com',
>-            branchillemail => "",
>-            branchreplyto => ""
>-        }
>-    });
>-    my $email_gen_branch = $illrq_obj->get_staff_to_address;
>-    ok(
>-        $email_gen_branch eq 'branch@nowhere.com',
>-        'get_staff_to_address falls back to general branch address when defined'
>-    );
>-    # ILL staff syspref address defined, should fall back to that
>-    t::lib::Mocks::mock_preference(
>-        'ILLDefaultStaffEmail',
>-        'illstaff@nowhere.com'
>-    );
>-    my $email_syspref = $illrq_obj->get_staff_to_address;
>-    ok(
>-        $email_syspref eq 'illstaff@nowhere.com',
>-        'get_staff_to_address falls back to ILLDefaultStaffEmail when defined'
>-    );
>-    # Branch ILL address defined, should use that
>-    $builder->delete({ source => 'Branch', records => $illbrn });
>-    $illbrn = $builder->build({
>-        source => 'Branch',
>-        value => {
>-            branchcode => 'HDE',
>-            branchemail => 'branch@nowhere.com',
>-            branchillemail => 'branchill@nowhere.com',
>-            branchreplyto => ""
>-        }
>-    });
>-    my $email_branch = $illrq_obj->get_staff_to_address;
>-    ok(
>-        $email_branch eq 'branchill@nowhere.com',
>-        'get_staff_to_address uses branch ILL address when defined'
>-    );
>-
>     #send_staff_notice
>     # Specify that no staff notices should be send
>     t::lib::Mocks::mock_preference('ILLSendStaffNotices', '');
>@@ -883,21 +827,25 @@ subtest 'Helpers' => sub {
>         { error => 'notice_not_enabled' },
>         "Does not send notices that are not enabled"
>     );
>+    my $queue = $schema->resultset('MessageQueue')->search({
>+            letter_code => 'ILL_REQUEST_CANCEL'
>+        });
>+    is($queue->count, 0, "Notice is not queued");
>+
>     # Specify that the cancel notice can be sent
>     t::lib::Mocks::mock_preference('ILLSendStaffNotices', 'ILL_REQUEST_CANCEL');
>     my $return_staff_cancel = $illrq_obj->send_staff_notice(
>         'ILL_REQUEST_CANCEL'
>     );
>-    my $cancel = $schema->resultset('MessageQueue')->search({
>-            letter_code => 'ILL_REQUEST_CANCEL',
>-            message_transport_type => 'email',
>-            to_address => 'branchill@nowhere.com'
>-        })->next()->letter_code;
>     is_deeply(
>         $return_staff_cancel,
>         { success => 'notice_queued' },
>         "Correct return when staff notice created"
>     );
>+    $queue = $schema->resultset('MessageQueue')->search({
>+            letter_code => 'ILL_REQUEST_CANCEL'
>+        });
>+    is($queue->count, 1, "Notice queued as expected");
> 
>     my $return_staff_fail = $illrq_obj->send_staff_notice();
>     is_deeply(
>@@ -905,6 +853,10 @@ subtest 'Helpers' => sub {
>         { error => 'notice_no_type' },
>         "Correct error when missing type"
>     );
>+    $queue = $schema->resultset('MessageQueue')->search({
>+            letter_code => 'ILL_REQUEST_CANCEL'
>+        });
>+    is($queue->count, 1, "Notice is not queued");
> 
>     #get_notice
>     my $not = $illrq_obj->get_notice({
>diff --git a/t/db_dependent/Koha/Libraries.t b/t/db_dependent/Koha/Libraries.t
>index ff9f6a9f0a..b5e8d93b2a 100755
>--- a/t/db_dependent/Koha/Libraries.t
>+++ b/t/db_dependent/Koha/Libraries.t
>@@ -19,7 +19,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 9;
>+use Test::More tests => 10;
> 
> use C4::Biblio;
> use C4::Context;
>@@ -159,6 +159,57 @@ subtest '->inbound_email_address' => sub {
>     $schema->storage->txn_rollback;
> };
> 
>+subtest '->inbound_ill_address' => sub {
>+
>+    plan tests => 7;
>+
>+    $schema->storage->txn_begin;
>+
>+    my $library_1 = $builder->build_object(
>+        {
>+            class => 'Koha::Libraries',
>+            value => {
>+                branchemail   => 'from@mylibrary.com',
>+                branchreplyto => 'reply@mylibrary.com',
>+                branchillemail => 'ill@mylibrary.com'
>+            }
>+        }
>+    );
>+
>+    t::lib::Mocks::mock_preference( 'KohaAdminEmailAddress', 'admin@mylibrary.com' );
>+    t::lib::Mocks::mock_preference( 'ReplytoDefault', 'reply@mylibrary.com' );
>+    t::lib::Mocks::mock_preference( 'ILLDefaultStaffEmail', 'illdefault@mylibrary.com' );
>+
>+    is( $library_1->inbound_ill_address, $library_1->branchillemail,
>+       'If defined, use library branchillemail address');
>+
>+    $library_1->branchillemail(undef)->store();
>+    is( $library_1->inbound_ill_address, 'illdefault@mylibrary.com',
>+       'Fallback to ILLDefaultStaffEmail preference when branchillemail is undefined');
>+
>+    t::lib::Mocks::mock_preference( 'ILLDefaultStaffEmail', undef );
>+    is( $library_1->inbound_ill_address, $library_1->branchreplyto,
>+       'Fallback to library replyto address when ILLDefaultStaffEmail is undefined');
>+
>+    $library_1->branchreplyto(undef)->store();
>+    is( $library_1->inbound_ill_address, $library_1->branchemail,
>+       'Fallback to branches email address when branchreplyto is undefined');
>+
>+    $library_1->branchemail(undef)->store();
>+    is( $library_1->inbound_ill_address, 'reply@mylibrary.com',
>+       'Fallback to ReplytoDefault email address when branchreplyto and branchemail are undefined');
>+
>+    t::lib::Mocks::mock_preference( 'ReplytoDefault', '' );
>+    is( $library_1->inbound_ill_address, 'admin@mylibrary.com',
>+       'Fallback to KohaAdminEmailAddress email address when branchreplyto, branchemail and ReplytoDefault are undefined');
>+
>+    t::lib::Mocks::mock_preference( 'KohaAdminEmailAddress', '' );
>+    is( $library_1->inbound_ill_address, undef,
>+       'Return undef when  email address when branchreplyto, branchemail, ReplytoDefault and KohaAdminEmailAddress are undefined');
>+
>+    $schema->storage->txn_rollback;
>+};
>+
> subtest 'cash_registers' => sub {
>     plan tests => 3;
> 
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 22818: 89831 | 89832 | 89833 | 89834 | 89835 | 90063 | 90064 | 90065 | 90066 | 90067 | 90715 | 90716 | 90717 | 90718 | 90719 | 92898 | 92899 | 92900 | 92901 | 92902 | 100234 | 100235 | 100236 | 100237 | 100238 | 103268 | 103269 | 103270 | 103271 | 103272 | 106424 | 106425 | 106426 | 106427 | 106428 | 106675 | 106676 | 106677 | 106678 | 106679 | 106680 | 106681 | 106691 | 106693 | 106694 | 106704 | 106705 | 106706 | 106707 | 106708 | 106709 | 106710 | 106711 | 106712 | 106713 | 110196 | 110197 | 110198 | 110199 | 110201 | 110202 | 110203 | 110204 | 110205 | 110206 | 110207 | 110208 | 111837 | 111838 | 111839 | 111840 | 111841 | 111842 | 111843 | 111844 | 111845 | 111846 | 111847 | 111848 | 111874 | 111880 | 111881 | 111883 | 111884 | 111885 | 111886 | 111887 | 111888 | 111889 | 111890 | 111891 | 111892 | 111893 | 111897 | 111899 | 111900 | 111901 | 111902 | 111903 | 111904 | 111905 | 111906 | 111907 | 111908 | 111909 | 111910 | 111911 | 111912 | 111913 | 113398 | 113399 | 113400 | 113401 | 113402 | 113403 | 113404 | 113405 | 113406 | 113407 | 113408 | 113409 | 113410 | 113411 | 113412 | 113413 | 113463 | 113464 | 113465 | 113466 | 113467 | 113468 | 113469 | 113470 | 113471 | 113472 | 113473 | 113474 | 113475 | 113476 | 113477 | 113478 | 113479 | 113522 | 113549 | 113584