From 5a0ed4990c3ebfed91f159e9d1774b61b33bf125 Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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 --- 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 621ece76c8..5c4e717234 100644 --- a/Koha/Illrequest.pm +++ b/Koha/Illrequest.pm @@ -30,7 +30,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; @@ -1330,8 +1329,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 ); @@ -1351,7 +1350,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) { @@ -1387,45 +1387,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); @@ -1457,6 +1418,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 = (); @@ -1471,6 +1438,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; @@ -1516,7 +1485,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/) { @@ -1531,7 +1500,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, @@ -1541,7 +1511,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 8c0c8d176e..6ca3b083d3 100644 --- a/Koha/Library.pm +++ b/Koha/Library.pm @@ -84,6 +84,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 876cb78b3a..304bc6a5d4 100644 --- a/t/db_dependent/Illrequests.t +++ b/t/db_dependent/Illrequests.t @@ -713,7 +713,7 @@ subtest 'Backend core methods' => sub { subtest 'Helpers' => sub { - plan tests => 21; + plan tests => 20; $schema->storage->txn_begin; @@ -815,62 +815,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', ''); @@ -881,21 +825,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( @@ -903,6 +851,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 100644 --- 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