From 86213906ddfa6cfd147f0e3ff76af6a52f7612d1 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 21 Aug 2014 13:15:36 +0200 Subject: [PATCH] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The Koha::Patron::notice_email_address subroutine has been renamed to Koha::Patron::notice_email_addresses (plural). It only gets the patron emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1. Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2. Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3. Verify the email is correctly sent to the message_queue. 4. Fill the pref with email and emailpro (for instance) 5. Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6. You can also verify that only 1 notice is generated if the emailpro is empty. Important note for testers: Pending messages must be removed each time you change the value of AutoEmailPrimaryAddress. When there are already existing pending messages for a borrower, Koha tries to update them rather than enqueuing new ones, but it will not update the recipient's email address, nor add/remove messages depending on the new value of AutoEmailPrimaryAddress; not sure if this is a bug or not. --- C4/Circulation.pm | 10 +- C4/Letters.pm | 146 ++++++++++-------- C4/Message.pm | 45 +----- C4/Reserves.pm | 14 +- Koha/Patron.pm | 25 +-- Koha/Patron/Password/Recovery.pm | 9 +- circ/pendingreserves.pl | 3 +- ...ge-autoemailprimaryddress-syspref-type.sql | 1 + installer/data/mysql/mandatory/sysprefs.sql | 2 +- .../en/modules/admin/preferences/patrons.pref | 2 +- members/memberentry.pl | 20 +-- .../notice_unprocessed_suggestions.pl | 4 +- misc/cronjobs/overdue_notices.pl | 8 +- .../thirdparty/TalkingTech_itiva_outbound.pl | 5 +- opac/opac-memberentry.pl | 10 +- opac/opac-shareshelf.pl | 7 +- suggestion/suggestion.pl | 4 +- t/db_dependent/Koha/Patrons.t | 11 +- t/db_dependent/Letters.t | 105 +++++++++++-- 19 files changed, 253 insertions(+), 178 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_12802-change-autoemailprimaryddress-syspref-type.sql diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 03fe5c78f2..3734517d11 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3600,13 +3600,15 @@ sub SendCirculationAlert { C4::Context->dbh->do(q|LOCK TABLE message_queue READ|) unless $do_not_lock; C4::Context->dbh->do(q|LOCK TABLE message_queue WRITE|) unless $do_not_lock; - my $message = C4::Message->find_last_message($borrower, $type, $mtt); - unless ( $message ) { + my @messages = C4::Message->find_last_messages($borrower, $type, $mtt); + unless ( @messages ) { C4::Context->dbh->do(q|UNLOCK TABLES|) unless $do_not_lock; C4::Message->enqueue($letter, $borrower, $mtt); } else { - $message->append($letter); - $message->update; + foreach my $message (@messages) { + $message->append($letter); + $message->update; + } } C4::Context->dbh->do(q|UNLOCK TABLES|) unless $do_not_lock; } diff --git a/C4/Letters.pm b/C4/Letters.pm index 28b90fded0..b82fc2f237 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -34,7 +34,7 @@ use C4::SMS; use C4::Debug; use Koha::DateUtils; use Koha::SMS::Providers; - +use Koha::Database; use Koha::Email; use Koha::Notice::Messages; use Koha::Notice::Templates; @@ -920,14 +920,17 @@ sub _parseletter { =head2 EnqueueLetter - my $success = EnqueueLetter( { letter => $letter, - borrowernumber => '12', message_transport_type => 'email' } ) + my @message_ids = EnqueueLetter({ + letter => $letter, + borrowernumber => '12', + message_transport_type => 'email' + }); places a letter in the message_queue database table, which will eventually get processed (sent) by the process_message_queue.pl cronjob when it calls SendQueuedMessages. -return message_id on success +return the list of message ids =cut @@ -935,7 +938,6 @@ sub EnqueueLetter { my $params = shift or return; return unless exists $params->{'letter'}; -# return unless exists $params->{'borrowernumber'}; return unless exists $params->{'message_transport_type'}; my $content = $params->{letter}->{content}; @@ -955,29 +957,67 @@ sub EnqueueLetter { ); } - my $dbh = C4::Context->dbh(); - my $statement = << 'ENDSQL'; -INSERT INTO message_queue -( borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, to_address, from_address, reply_address, content_type ) -VALUES -( ?, ?, ?, ?, ?, ?, ?, NOW(), ?, ?, ?, ? ) -ENDSQL - - my $sth = $dbh->prepare($statement); - my $result = $sth->execute( - $params->{'borrowernumber'}, # borrowernumber - $params->{'letter'}->{'title'}, # subject - $params->{'letter'}->{'content'}, # content - $params->{'letter'}->{'metadata'} || '', # metadata - $params->{'letter'}->{'code'} || '', # letter_code - $params->{'message_transport_type'}, # message_transport_type - 'pending', # status - $params->{'to_address'}, # to_address - $params->{'from_address'}, # from_address - $params->{'reply_address'}, # reply_address - $params->{'letter'}->{'content-type'}, # content_type - ); - return $dbh->last_insert_id(undef,undef,'message_queue', undef); + my @to_addresses; + if ( $params->{to_address} ) { + @to_addresses = ( $params->{to_address} ); + } elsif ( $params->{borrowernumber} and $params->{message_transport_type} eq 'email' ) { + my $patron = Koha::Patrons->find($params->{borrowernumber}); + if ($patron) { + @to_addresses = @{ $patron->notice_email_addresses }; + } + } + + my $from_address; + if ( $params->{from_address} ) { + $from_address = $params->{from_address}; + } elsif ( $params->{message_transport_type} eq 'email' ) { + if ( $params->{borrowernumber} ) { + my $rs = Koha::Database->new->schema->resultset('Borrower')->search({ borrowernumber => $params->{borrowernumber} }, {join => 'branchcode'}); + $from_address = $rs->next->branchcode->branchemail; + } + + $from_address ||= C4::Context->preference('KohaAdminEmailAddress'); + } + + my @message_ids; + + my $rs = Koha::Database->new->schema->resultset('MessageQueue'); + if ($params->{message_transport_type} eq 'email') { + for my $to_address ( @to_addresses ) { + my $message = $rs->create({ + borrowernumber => $params->{borrowernumber}, + subject => $params->{letter}{title}, + content => $params->{letter}{content}, + metadata => ( $params->{letter}{metadata} || '' ), + letter_code => ( $params->{letter}{code} || ''), + message_transport_type => $params->{message_transport_type}, + status => 'pending', + time_queued => \'NOW()', + to_address => $to_address, + from_address => $from_address, + reply_address => $params->{'reply_address'}, + content_type => $params->{letter}{'content-type'}, + }); + push @message_ids, $message->id(); + } + } else { + my $message = $rs->create({ + borrowernumber => $params->{borrowernumber}, + subject => $params->{letter}{title}, + content => $params->{letter}{content}, + metadata => ( $params->{letter}{metadata} || '' ), + letter_code => ( $params->{letter}{code} || ''), + message_transport_type => $params->{message_transport_type}, + status => 'pending', + time_queued => \'NOW()', + from_address => $from_address, + reply_address => $params->{'reply_address'}, + content_type => $params->{letter}{'content-type'}, + }); + push @message_ids, $message->id(); + } + + return @message_ids; } =head2 SendQueuedMessages ([$hashref]) @@ -1116,31 +1156,15 @@ Return is an arrayref of hashes, each has represents a message in the message qu sub GetQueuedMessages { my $params = shift; - my $dbh = C4::Context->dbh(); - my $statement = << 'ENDSQL'; -SELECT message_id, borrowernumber, subject, content, message_transport_type, status, time_queued, updated_on -FROM message_queue -ENDSQL - - my @query_params; - my @whereclauses; - if ( exists $params->{'borrowernumber'} ) { - push @whereclauses, ' borrowernumber = ? '; - push @query_params, $params->{'borrowernumber'}; - } + my (%cond, %attrs); - if ( @whereclauses ) { - $statement .= ' WHERE ' . join( 'AND', @whereclauses ); - } + $cond{borrowernumber} = $params->{borrowernumber} if $params->{borrowernumber}; - if ( defined $params->{'limit'} ) { - $statement .= ' LIMIT ? '; - push @query_params, $params->{'limit'}; - } + $attrs{rows} = $params->{limit} if $params->{limit}; - my $sth = $dbh->prepare( $statement ); - my $result = $sth->execute( @query_params ); - return $sth->fetchall_arrayref({}); + my $rs = Koha::Database->new->schema->resultset('MessageQueue')->search(\%cond, \%attrs); + $rs->result_class('DBIx::Class::ResultClass::HashRefInflator'); + return [ $rs->all ]; } =head2 GetMessageTransportTypes @@ -1324,21 +1348,13 @@ sub _send_message_by_email { my $patron = Koha::Patrons->find( $message->{borrowernumber} ); my $to_address = $message->{'to_address'}; - unless ($to_address) { - unless ($patron) { - warn "FAIL: No 'to_address' and INVALID borrowernumber ($message->{borrowernumber})"; - _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); - return; - } - $to_address = $patron->notice_email_address; - unless ($to_address) { - # warn "FAIL: No 'to_address' and no email for " . ($member->{surname} ||'') . ", borrowernumber ($message->{borrowernumber})"; - # warning too verbose for this more common case? - _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); - return; - } + unless ($patron and $to_address) { + warn "FAIL: No 'to_address' and INVALID borrowernumber ($message->{borrowernumber})" unless $patron; + _set_message_status({ + message_id => $message->{message_id}, + status => 'failed', + }); + return; } my $subject = $message->{'subject'}; diff --git a/C4/Message.pm b/C4/Message.pm index 659ff96aeb..7f5f717d32 100644 --- a/C4/Message.pm +++ b/C4/Message.pm @@ -54,7 +54,7 @@ How to update a borrower's last checkout message: use C4::Message; my $borrower = { borrowernumber => 1 }; - my $message = C4::Message->find_last_message($borrower, 'CHECKOUT', 'email'); + my $message = C4::Message->find_last_messages($borrower, 'CHECKOUT', 'email'); $message->append("you also checked out some other book...."); $message->update; @@ -111,17 +111,17 @@ sub find { } } -=head3 C4::Message->find_last_message($borrower, $letter_code, $transport) +=head3 C4::Message->find_last_messages($borrower, $letter_code, $transport) This method is used to get the borrower's most recent, pending, check-in or -checkout message. (This makes it possible to add more information to the +checkout messages. (This makes it possible to add more information to the message before it gets sent out.) =cut -# C4::Message->find_last_message($borrower, $letter_code, $transport) -# -- get the borrower's most recent pending checkin or checkout notification -sub find_last_message { +# C4::Message->find_last_messages($borrower, $letter_code, $transport) +# -- get the borrower's most recent pending checkin or checkout notifications +sub find_last_messages { my ($class, $borrower, $letter_code, $transport) = @_; # $type is the message_transport_type $transport ||= 'email'; @@ -140,11 +140,8 @@ sub find_last_message { $letter_code, $transport, ); - if (@$msgs) { - return $class->new($msgs->[0]); - } else { - return; - } + + return map { $class->new($_) } @$msgs; } @@ -159,7 +156,6 @@ the message. sub enqueue { my ($class, $letter, $borrower, $transport) = @_; my $metadata = _metadata($letter); - my $to_address = _to_address($borrower, $transport); # Same as render_metadata my $format ||= sub { $_[0] || "" }; @@ -171,34 +167,9 @@ sub enqueue { letter => $letter, borrowernumber => $borrower->{borrowernumber}, message_transport_type => $transport, - to_address => $to_address, }); } -# based on message $transport, pick an appropriate address to send to -sub _to_address { - my ($borrower, $transport) = @_; - my $address; - if ($transport eq 'email') { - $address = $borrower->{email} - || $borrower->{emailpro} - || $borrower->{B_email}; - } elsif ($transport eq 'sms') { - $address = $borrower->{smsalertnumber} - || $borrower->{phone} - || $borrower->{phonepro} - || $borrower->{B_phone}; - } else { - warn "'$transport' is an unknown message transport."; - } - if (not defined $address) { - warn "An appropriate $transport address " - . "for borrower $borrower->{userid} " - . "could not be found."; - } - return $address; -} - # _metadata($letter) -- return the letter split into head/body/footer sub _metadata { my ($letter) = @_; diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 54ccc7457d..b86e455c04 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -721,12 +721,13 @@ SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>? Takes an itemnumber and returns the status of the reserve placed on it. If several reserves exist, the reserve with the lower priority is given. -=cut - ## FIXME: I don't think this does what it thinks it does. ## It only ever checks the first reserve result, even though ## multiple reserves for that bib can have the itemnumber set ## the sub is only used once in the codebase. + +=cut + sub GetReserveStatus { my ($itemnumber) = @_; @@ -1828,9 +1829,6 @@ sub _koha_notify_reserve { my $patron = Koha::Patrons->find( $borrowernumber ); - # Try to get the borrower's email address - my $to_address = $patron->notice_email_address; - my $messagingprefs = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $borrowernumber, message_name => 'Hold_Filled' @@ -1869,14 +1867,16 @@ sub _koha_notify_reserve { C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $borrowernumber, - from_address => $admin_email_address, message_transport_type => $mtt, } ); }; + # Try to get the borrower's email addresses + my $to_addresses = $patron->notice_email_addresses; + while ( my ( $mtt, $letter_code ) = each %{ $messagingprefs->{transports} } ) { next if ( - ( $mtt eq 'email' and not $to_address ) # No email address + ( $mtt eq 'email' and not @$to_addresses ) # No email address or ( $mtt eq 'sms' and not $patron->smsalertnumber ) # No SMS number or ( $mtt eq 'itiva' and C4::Context->preference('TalkingTechItivaPhoneNotification') ) # Notice is handled by TalkingTech_itiva_outbound.pl or ( $mtt eq 'phone' and not $patron->phone ) # No phone number to call diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 8c1592c3f2..36fd464f74 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1227,25 +1227,30 @@ sub return_claims { return Koha::Checkouts::ReturnClaims->_new_from_dbic( $return_claims ); } -=head3 notice_email_address +=head3 notice_email_addresses - my $email = $patron->notice_email_address; + my $emails = $patron->notice_email_addresses; -Return the email address of patron used for notices. -Returns the empty string if no email address. +Return an arrayref containing the email addresses of patron used for notices. =cut -sub notice_email_address{ +sub notice_email_addresses { my ( $self ) = @_; - my $which_address = C4::Context->preference("AutoEmailPrimaryAddress"); - # if syspref is set to 'first valid' (value == OFF), look up email address - if ( $which_address eq 'OFF' ) { - return $self->first_valid_email_address; + my $which_addresses = C4::Context->preference("AutoEmailPrimaryAddress"); + my @email_addresses; + for my $email_address_field (split /,/, $which_addresses) { + # if syspref is set to 'first valid' (value == OFF), look up email address + if ( $email_address_field eq 'OFF' ) { + return [ $self->first_valid_email_address ]; + } + + my $email_address = $self->$email_address_field; + push @email_addresses, $email_address if $email_address; } - return $self->$which_address || ''; + return \@email_addresses; } =head3 first_valid_email_address diff --git a/Koha/Patron/Password/Recovery.pm b/Koha/Patron/Password/Recovery.pm index c2e2d27930..bceb720599 100644 --- a/Koha/Patron/Password/Recovery.pm +++ b/Koha/Patron/Password/Recovery.pm @@ -153,7 +153,7 @@ sub SendPasswordRecoveryEmail { my $library = $borrower->library; my $kohaEmail = $library->branchemail || C4::Context->preference('KohaAdminEmailAddress'); # send from patron's branch or Koha Admin - my $message_id = C4::Letters::EnqueueLetter( + my @message_ids = C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $borrower->borrowernumber, @@ -163,8 +163,11 @@ sub SendPasswordRecoveryEmail { } ); - my $num_letters_attempted = - C4::Letters::SendQueuedMessages( { message_id => $message_id } ); + my $num_letters_attempted = 0; + foreach my $message_id (@message_ids) { + $num_letters_attempted += + C4::Letters::SendQueuedMessages( { message_id => $message_id } ); + } return ($num_letters_attempted > 0); } diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl index 995d63bd9c..3ca40cf6c1 100755 --- a/circ/pendingreserves.pl +++ b/circ/pendingreserves.pl @@ -95,7 +95,8 @@ if ( $op eq 'cancel_reserve' and $reserve_id ) { from_address => $admin_email_address, } ); - unless ( $patron->notice_email_address ) { + my $email_addresses = $patron->notice_email_addresses; + unless ( @$email_addresses ) { push @messages, {type => 'alert', code => 'no_email_address', }; } push @messages, { type => 'message', code => 'letter_enqueued' }; diff --git a/installer/data/mysql/atomicupdate/bug_12802-change-autoemailprimaryddress-syspref-type.sql b/installer/data/mysql/atomicupdate/bug_12802-change-autoemailprimaryddress-syspref-type.sql new file mode 100644 index 0000000000..6a7cb9146d --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_12802-change-autoemailprimaryddress-syspref-type.sql @@ -0,0 +1 @@ +UPDATE systempreferences SET type='multiple' WHERE variable='AutoEmailPrimaryAddress'; \ No newline at end of file diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 1034313061..fe61a3d059 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -72,7 +72,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('AutoCreateAuthorities','0',NULL,'Automatically create authorities that do not exist when cataloging records.','YesNo'), ('AutoCreditNumber', '', '', 'Automatically generate a number for account credits', 'Choice'), ('AutoEmailOpacUser','0',NULL,'Sends notification emails containing new account details to patrons - when account is created.','YesNo'), -('AutoEmailPrimaryAddress','OFF','email|emailpro|B_email|cardnumber|OFF','Defines the default email address where \'Account Details\' emails are sent.','Choice'), +('AutoEmailPrimaryAddress','OFF','email|emailpro|B_email|cardnumber|OFF','Defines the default email address where \'Account Details\' emails are sent.','multiple'), ('AutoShareWithMana','subscription','','defines datas automatically shared with mana','multiple'), ('AutoLocation','0',NULL,'If ON, IP authentication is enabled, blocking access to the staff interface from unauthorized IP addresses','YesNo'), ('AutomaticItemReturn','1',NULL,'If ON, Koha will automatically set up a transfer of this item to its homebranch','YesNo'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref index e599773fa2..9d749cb871 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref @@ -151,7 +151,7 @@ Patrons: - "Use" - pref: AutoEmailPrimaryAddress default: "OFF" - choices: + multiple: email: home emailpro: work B_email: alternate diff --git a/members/memberentry.pl b/members/memberentry.pl index e6be7854a0..6b6a6dde7e 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -466,19 +466,15 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ # If 'AutoEmailOpacUser' syspref is on, email user their account details from the 'notice' that matches the user's branchcode. if ( C4::Context->preference("AutoEmailOpacUser") == 1 && $newdata{'userid'} && $newdata{'password'}) { #look for defined primary email address, if blank - attempt to use borr.email and borr.emailpro instead - my $emailaddr; - if (C4::Context->preference("AutoEmailPrimaryAddress") ne 'OFF' && - $newdata{C4::Context->preference("AutoEmailPrimaryAddress")} =~ /\w\@\w/ ) { - $emailaddr = $newdata{C4::Context->preference("AutoEmailPrimaryAddress")} - } - elsif ($newdata{email} =~ /\w\@\w/) { - $emailaddr = $newdata{email} + my @email_addresses; + for my $field ( split /,/, C4::Context->preference("AutoEmailPrimaryAddress") ) { + push @email_addresses, newdata{$field} if $newdata{$field} =~ /\w@\w/ } - elsif ($newdata{emailpro} =~ /\w\@\w/) { - $emailaddr = $newdata{emailpro} - } - elsif ($newdata{B_email} =~ /\w\@\w/) { - $emailaddr = $newdata{B_email} + my $emailaddr = join ", ", @email_addresses; + unless ( $emailaddr ) { + for my $field ( qw( email emailpro B_email ) ) { + $emailaddr = $newdata{$field} if $newdata{$field} =~ m|\w\@\w|; + } } # if we manage to find a valid email address, send notice if ($emailaddr) { diff --git a/misc/cronjobs/notice_unprocessed_suggestions.pl b/misc/cronjobs/notice_unprocessed_suggestions.pl index 055315c9dd..70733c37bf 100755 --- a/misc/cronjobs/notice_unprocessed_suggestions.pl +++ b/misc/cronjobs/notice_unprocessed_suggestions.pl @@ -47,12 +47,12 @@ for my $number_of_days (@days) { my $budget = C4::Budgets::GetBudget( $suggestion->{budgetid} ); my $patron = Koha::Patrons->find( $budget->{budget_owner_id} ); - my $email_address = $patron->notice_email_address; + my $email_addresses = $patron->notice_email_addresses; my $library = $patron->library; my $admin_email_address = $library->branchemail || C4::Context->preference('KohaAdminEmailAddress'); - if ($email_address) { + if (@$email_addresses) { say "Patron " . $patron->borrowernumber . " is going to be notified" if $verbose; my $letter = C4::Letters::GetPreparedLetter( module => 'suggestions', diff --git a/misc/cronjobs/overdue_notices.pl b/misc/cronjobs/overdue_notices.pl index bf1a370a11..8edb2ac364 100755 --- a/misc/cronjobs/overdue_notices.pl +++ b/misc/cronjobs/overdue_notices.pl @@ -612,7 +612,6 @@ END_SQL my $patron = Koha::Patrons->find( $borrowernumber ); @emails_to_use = (); - my $notice_email = $patron->notice_email_address; unless ($nomail) { if (@emails) { foreach (@emails) { @@ -620,7 +619,8 @@ END_SQL } } else { - push @emails_to_use, $notice_email if ($notice_email); + my $notice_emails = $patron->notice_email_addresses; + push @emails_to_use, @$notice_emails; } } @@ -762,7 +762,7 @@ END_SQL letternumber => $i, postcode => $data->{'zipcode'}, country => $data->{'country'}, - email => $notice_email, + email => ( scalar( @emails_to_use ) ? $emails_to_use[0] : undef ), itemcount => $itemcount, titles => $titles, outputformat => defined $csvfilename ? 'csv' : defined $htmlfilename ? 'html' : defined $text_filename ? 'text' : '', @@ -781,7 +781,7 @@ END_SQL city => $data->{'city'}, postcode => $data->{'zipcode'}, country => $data->{'country'}, - email => $notice_email, + email => ( scalar( @emails_to_use ) ? $emails_to_use[0] : undef ), itemcount => $itemcount, titles => $titles, outputformat => defined $csvfilename ? 'csv' : defined $htmlfilename ? 'html' : defined $text_filename ? 'text' : '', diff --git a/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl b/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl index ae6a4f4e00..a85800bf40 100755 --- a/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl +++ b/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl @@ -131,7 +131,10 @@ foreach my $type (@types) { my $patrons; foreach my $issues (@loop) { $patrons->{$issues->{borrowernumber}} ||= Koha::Patrons->find( $issues->{borrowernumber} ) if $skip_patrons_with_email; - next if $skip_patrons_with_email && $patrons->{$issues->{borrowernumber}}->notice_email_address; + if ($skip_patrons_with_email) { + my $email_addresses = $patrons->{$issues->{borrowernumber}}->notice_email_addresses; + next if @$email_addresses; + } my $date_dt = dt_from_string ( $issues->{'date_due'} ); my $due_date = output_pref( { dt => $date_dt, dateonly => 1, dateformat =>'metric' } ); diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index dbfd6f83dd..cc2f7b3932 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -197,16 +197,16 @@ if ( $action eq 'create' ) { }, ); - my $message_id = C4::Letters::EnqueueLetter( + my @message_ids = C4::Letters::EnqueueLetter( { + borrowernumber => $borrower{borrowernumber}, letter => $letter, message_transport_type => 'email', - to_address => $borrower{'email'}, - from_address => - C4::Context->preference('KohaAdminEmailAddress'), } ); - C4::Letters::SendQueuedMessages({ message_id => $message_id }); + for my $message_id (@message_ids) { + C4::Letters::SendQueuedMessages({ message_id => $message_id }); + } } else { ( $template, $borrowernumber, $cookie ) = get_template_and_user( diff --git a/opac/opac-shareshelf.pl b/opac/opac-shareshelf.pl index 7eecf6c45f..4bf167b634 100755 --- a/opac/opac-shareshelf.pl +++ b/opac/opac-shareshelf.pl @@ -165,7 +165,7 @@ sub notify_owner { my $patron = Koha::Patrons->find( $param->{owner} ); return unless $patron; - my $toaddr = $patron->notice_email_address or return; + return unless @{ $patron->notice_email_addresses }; #prepare letter my $letter = C4::Letters::GetPreparedLetter( @@ -180,10 +180,9 @@ sub notify_owner { #send letter to queue C4::Letters::EnqueueLetter( { + borrowernumber => $param->{owner}, letter => $letter, message_transport_type => 'email', - from_address => C4::Context->preference('KohaAdminEmailAddress'), - to_address => $toaddr, } ); } @@ -209,7 +208,6 @@ sub process_addrlist { sub send_invitekey { my ($param) = @_; - my $fromaddr = C4::Context->preference('KohaAdminEmailAddress'); my $url = C4::Context->preference('OPACBaseURL') . "/cgi-bin/koha/opac-shareshelf.pl?shelfnumber=" @@ -251,7 +249,6 @@ sub send_invitekey { { letter => $letter, message_transport_type => 'email', - from_address => $fromaddr, to_address => $a, } ); diff --git a/suggestion/suggestion.pl b/suggestion/suggestion.pl index 64b4106b2d..1919b72d16 100755 --- a/suggestion/suggestion.pl +++ b/suggestion/suggestion.pl @@ -186,8 +186,8 @@ if ( $op =~ /save/i ) { if ( $notify ) { my $patron = Koha::Patrons->find( $suggestion_only->{managedby} ); - my $email_address = $patron->notice_email_address; - if ($patron->notice_email_address) { + my $email_addresses = $patron->notice_email_addresses; + if (@$email_addresses) { my $library = $patron->library; my $admin_email_address = $library->branchemail || C4::Context->preference('KohaAdminEmailAddress'); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index c2824082e7..beae86a99e 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1007,16 +1007,19 @@ subtest 'holds and old_holds' => sub { $patron->delete; }; -subtest 'notice_email_address' => sub { - plan tests => 2; +subtest 'notice_email_addresses' => sub { + plan tests => 3; my $patron = $builder->build_object({ class => 'Koha::Patrons' }); t::lib::Mocks::mock_preference( 'AutoEmailPrimaryAddress', 'OFF' ); - is ($patron->notice_email_address, $patron->email, "Koha::Patron->notice_email_address returns correct value when AutoEmailPrimaryAddress is off"); + is_deeply ($patron->notice_email_addresses, [$patron->email], "Koha::Patron->notice_email_addresses returns correct value when AutoEmailPrimaryAddress is off"); t::lib::Mocks::mock_preference( 'AutoEmailPrimaryAddress', 'emailpro' ); - is ($patron->notice_email_address, $patron->emailpro, "Koha::Patron->notice_email_address returns correct value when AutoEmailPrimaryAddress is emailpro"); + is_deeply ($patron->notice_email_addresses, [$patron->emailpro], "Koha::Patron->notice_email_addresses returns correct value when AutoEmailPrimaryAddress is emailpro"); + + t::lib::Mocks::mock_preference( 'AutoEmailPrimaryAddress', 'email,emailpro' ); + is_deeply ($patron->notice_email_addresses, [$patron->email, $patron->emailpro], "Koha::Patron->notice_email_addresses returns correct value when AutoEmailPrimaryAddress is email,emailpro"); $patron->delete; }; diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 3ba3361d28..4738eba7cf 100755 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -112,8 +112,7 @@ my $my_message = { to_address => undef, from_address => 'from@example.com', }; -my $message_id = C4::Letters::EnqueueLetter($my_message); -is( $message_id, undef, 'EnqueueLetter without the letter argument returns undef' ); +is( C4::Letters::EnqueueLetter($my_message), undef, 'EnqueueLetter without the letter argument returns undef' ); delete $my_message->{message_transport_type}; $my_message->{letter} = { @@ -124,13 +123,12 @@ $my_message->{letter} = { content_type => 'text/plain', }; -$message_id = C4::Letters::EnqueueLetter($my_message); -is( $message_id, undef, 'EnqueueLetter without the message type argument argument returns undef' ); +my @message_ids = C4::Letters::EnqueueLetter($my_message); +is( scalar @message_ids, 0, 'EnqueueLetter without the message type argument argument returns an empty list' ); $my_message->{message_transport_type} = 'sms'; -$message_id = C4::Letters::EnqueueLetter($my_message); -ok(defined $message_id && $message_id > 0, 'new message successfully queued'); - +@message_ids = C4::Letters::EnqueueLetter($my_message); +is(scalar @message_ids, 1, 'new message successfully queued'); # GetQueuedMessages my $messages = C4::Letters::GetQueuedMessages(); @@ -138,7 +136,6 @@ is( @$messages, 1, 'GetQueuedMessages without argument returns all the entries' $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber }); is( @$messages, 1, 'one message stored for the borrower' ); -is( $messages->[0]->{message_id}, $message_id, 'EnqueueLetter returns the message id correctly' ); is( $messages->[0]->{borrowernumber}, $borrowernumber, 'EnqueueLetter stores the borrower number correctly' ); is( $messages->[0]->{subject}, $my_message->{letter}->{title}, 'EnqueueLetter stores the subject correctly' ); is( $messages->[0]->{content}, $my_message->{letter}->{content}, 'EnqueueLetter stores the content correctly' ); @@ -820,8 +817,8 @@ subtest 'SendQueuedMessages' => sub { is( $@, '', 'SendQueuedMessages should not explode if the patron does not have a sms provider set' ); 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 + $patron->set( { smsalertnumber => '5555555555', sms_provider_id => $sms_pro->id(), email => '', emailpro => '' } )->store; + C4::Letters::EnqueueLetter($my_message); #using datas set around line 95 and forward warning_like { C4::Letters::SendQueuedMessages(); } qr|Fake send_or_die|, @@ -843,7 +840,7 @@ subtest 'SendQueuedMessages' => sub { t::lib::Mocks::mock_preference('EmailSMSSendDriverFromAddress', 'override@example.com'); - $message_id = C4::Letters::EnqueueLetter($my_message); + C4::Letters::EnqueueLetter($my_message); warning_like { C4::Letters::SendQueuedMessages(); } qr|Fake send_or_die|, "SendAlerts is using the mocked send_or_die routine (claimissues)"; @@ -861,7 +858,7 @@ subtest 'SendQueuedMessages' => sub { $schema->resultset('MessageQueue')->search({borrowernumber => $borrowernumber,status => 'sent'})->delete(); #clear borrower queue $my_message->{to_address} = 'fixme@kidclamp.iswrong'; - $message_id = C4::Letters::EnqueueLetter($my_message); + C4::Letters::EnqueueLetter($my_message); my $number_attempted = C4::Letters::SendQueuedMessages({ borrowernumber => -1, # -1 still triggers the borrowernumber condition @@ -875,6 +872,7 @@ subtest 'SendQueuedMessages' => sub { my $sms_message_address = $schema->resultset('MessageQueue')->search({ borrowernumber => $borrowernumber, + message_transport_type => 'sms', status => 'sent' })->next()->to_address(); is( $sms_message_address, '5555555555@kidclamp.rocks', 'SendQueuedMessages populates the to address correctly for SMS by email when to_address is set incorrectly' ); @@ -989,7 +987,7 @@ subtest 'Test message_id parameter for SendQueuedMessages' => sub { 'message_transport_type' => 'email', 'from_address' => 'root@localhost' # invalid KohaAdminEmailAddress }; - my $message_id = C4::Letters::EnqueueLetter($my_message); + my ($message_id) = C4::Letters::EnqueueLetter($my_message); throws_ok { C4::Letters::SendQueuedMessages( { message_id => $message_id } ); } 'Koha::Exceptions::BadParameter', @@ -999,10 +997,89 @@ subtest 'Test message_id parameter for SendQueuedMessages' => sub { is( $message_1->{status}, 'pending', 'Invalid KohaAdminEmailAddress => status pending' ); $my_message->{from_address} = 'root@example.org'; # valid KohaAdminEmailAddress - $message_id = C4::Letters::EnqueueLetter($my_message); + ($message_id) = C4::Letters::EnqueueLetter($my_message); C4::Letters::SendQueuedMessages( { message_id => $message_id } ); $message_1 = C4::Letters::GetMessage($message_1->{message_id}); my $message_2 = C4::Letters::GetMessage($message_id); is( $message_1->{status}, 'pending', 'Message 1 status is unchanged' ); # Must be 'failed' is( $message_2->{status}, 'sent', 'Valid KohaAdminEmailAddress => status sent' ); }; + +subtest 'EnqueueLetter and AutoEmailPrimaryAddress' => sub { + plan tests => 19; + + my ( $email, $emailpro ) = ( 'email@example.org', 'emailpro@example.org' ); + my $borrowernumber = Koha::Patron->new({ + firstname => 'Jane', + surname => 'Smith', + categorycode => $patron_category, + branchcode => $library->{branchcode}, + dateofbirth => $date, + smsalertnumber => undef, + email => $email, + emailpro => $emailpro, + })->store->borrowernumber; + + my $my_message = { + borrowernumber => $borrowernumber, + message_transport_type => 'email', + to_address => 'to@example.com', + from_address => 'from@example.com', + letter => { + content => 'a message', + title => 'message title', + metadata => 'metadata', + code => 'TEST_MESSAGE', + content_type => 'text/plain', + }, + }; + + $dbh->do(q|DELETE FROM message_queue|); + t::lib::Mocks::mock_preference('AutoEmailPrimaryAddress', 'email'); + @message_ids = C4::Letters::EnqueueLetter($my_message); + is(scalar @message_ids, 1, 'message successfully queued' ); + $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber }); + is( scalar( @$messages ), 1, 'The message has been queued for the expected patron' ); + is( $messages->[0]{to_address}, 'to@example.com', 'AutoEmailPrimaryAddress=email, EnqueueLetter used the to_address given in parameter' ); + + $dbh->do(q|DELETE FROM message_queue|); + delete $my_message->{to_address}; + @message_ids = C4::Letters::EnqueueLetter($my_message); + is(scalar @message_ids, 1, 'message successfully queued' ); + $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber }); + is( scalar( @$messages ), 1, 'The message has been queued for the expected patron' ); + is( $messages->[0]{to_address}, $email, 'AutoEmailPrimaryAddress=email, EnqueueLetter used the patron email address if no to_address is given in parameter' ); + + $dbh->do(q|DELETE FROM message_queue|); + t::lib::Mocks::mock_preference('AutoEmailPrimaryAddress', 'emailpro'); + @message_ids = C4::Letters::EnqueueLetter($my_message); + is( scalar @message_ids, 1, 'message successfully queued' ); + $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber }); + is( scalar( @$messages ), 1, 'The message has been queued for the expected patron' ); + is( $messages->[0]{to_address}, $emailpro, 'AutoEmailPrimaryAddress=emailpro, EnqueueLetter used the patron emailpro address if no to_address is given in parameter' ); + + $dbh->do(q|DELETE FROM message_queue|); + t::lib::Mocks::mock_preference('AutoEmailPrimaryAddress', 'OFF'); + @message_ids = C4::Letters::EnqueueLetter($my_message); + is( scalar @message_ids, 1, 'message successfully queued' ); + $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber }); + is( scalar( @$messages ), 1, 'The message has been queued for the expected patron' ); + is( $messages->[0]{to_address}, $email, 'AutoEmailPrimaryAddress=OFF, EnqueueLetter used the patron email address if no to_address is given in parameter' ); + + $dbh->do(q|DELETE FROM message_queue|); + t::lib::Mocks::mock_preference('AutoEmailPrimaryAddress', 'email,OFF,emailpro'); # This is no consistent. OFF should be alone. + @message_ids = C4::Letters::EnqueueLetter($my_message); + is( scalar @message_ids, 1, 'message successfully queued' ); + $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber }); + is( scalar( @$messages ), 1, 'The message has been queued for the expected patron' ); + is( $messages->[0]{to_address}, $email, 'AutoEmailPrimaryAddress=email,OFF,emailpro, EnqueueLetter used the patron email address if no to_address is given in parameter' ); + + $dbh->do(q|DELETE FROM message_queue|); + t::lib::Mocks::mock_preference('AutoEmailPrimaryAddress', 'email,emailpro'); + @message_ids = C4::Letters::EnqueueLetter($my_message); + is( scalar @message_ids, 2, 'messages successfully queued' ); + $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber }); + is( scalar( @$messages ), 2, 'The messages have been queued for the expected patron' ); + is( $messages->[0]{to_address}, $email, 'AutoEmailPrimaryAddress=email,emailpro, EnqueueLetter used the patron email address for the first letter' ); + is( $messages->[1]{to_address}, $emailpro, 'AutoEmailPrimaryAddress=email,emailpro, EnqueueLetter used the patron emailpro address for the second letter' ); +}; -- 2.20.1