From 4fc8fbb8be43e66ec211a1716d7561e3ed847973 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 3 Apr 2023 16:29:22 +0200 Subject: [PATCH] Bug 33360: Improving limit behavior in SendQueuedMessages Includes: [1] Do no longer use the limit in the sql selection, but apply the limit as a maximum for the number of sent messages. This is more practical in terms of not flooding your mail server (and the receiving ones). [2] Replace call of _get_unsent_messages by Koha objects search. [3] Do no longer report the number of messages seen, but report the number actually 'sent'. [4] If we lookup the to_address but we need to delay a message, save the email address for the next run. Also optimizing patron lookup in _send_message_by_email. [5} Add support for $where parameter in SendQueuedMessages. Used by process_message_queue.pl. [6] Handle scalar/array for letter_code and type parameter too. Test plan: [1] Adjust your domain limit settings in koha-conf. Use notices to three domains. Group A and B. A11h B/name>A C11h Replace A, B and C with your choice. Do not forget the belongs_to. Restart all. [2] Disable cron job for message queue. [3] Generate two notices for each domain A, B and C (in that order). Make sure that borrowers involved have correct address. [4] Run process_message_queue.pl -limit 1 [5] Check that one is sent for A, 5 pending. [6] Run process_message_queue.pl -limit 2 [7] Check that one is sent for C, 4 pending. [8] Run process_message_queue.pl (without limit). [9] Check that nothing is sent, 4 pending. [10] Check that message_queue.to_address is filled for those 4. Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize --- C4/Letters.pm | 75 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index f45771c932..9ee8e6df08 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -954,22 +954,22 @@ ENDSQL =head2 SendQueuedMessages ([$hashref]) my $sent = SendQueuedMessages({ - letter_code => $letter_code, + message_id => $id, borrowernumber => $who_letter_is_for, + letter_code => $letter_code, # can be scalar or arrayref + type => $type, # can be scalar or arrayref limit => 50, verbose => 1, - type => 'sms', + where => $where, }); -Sends all of the 'pending' items in the message queue, unless -parameters are passed. +Sends 'pending' messages from the queue, based on parameters. -The letter_code, borrowernumber and limit parameters are used -to build a parameter set for _get_unsent_messages, thus limiting -which pending messages will be processed. They are all optional. +The (optional) message_id, borrowernumber, letter_code, type and where +parameter are used to select which pending messages will be processed. The +limit parameter determines the volume of results, i.e. sent messages. -The verbose parameter can be used to generate debugging output. -It is also optional. +The optional verbose parameter can be used to generate debugging output. Returns number of messages sent. @@ -977,19 +977,26 @@ Returns number of messages sent. sub SendQueuedMessages { my $params = shift; + my $limit = $params->{limit}; + my $where = $params->{where}; + + my $count_messages = 0; + my $unsent_messages = Koha::Notice::Messages->search({ + status => 'pending', + $params->{message_id} ? ( message_id => $params->{message_id} ) : (), + $params->{borrowernumber} ? ( borrowernumber => $params->{borrowernumber} ) : (), + # Check for scalar or array in letter_code and type + ref($params->{letter_code}) && @{$params->{letter_code}} ? ( letter_code => $params->{letter_code} ) : (), + !ref($params->{letter_code}) && $params->{letter_code} ? ( letter_code => $params->{letter_code} ) : (), + ref($params->{type}) && @{$params->{type}} ? ( message_transport_type => $params->{type} ) : (), #TODO Existing inconsistency + !ref($params->{type}) && $params->{type} ? ( message_transport_type => $params->{type} ) : (), #TODO Existing inconsistency + }); + $unsent_messages = $unsent_messages->search( \$where ) if $where; - my $which_unsent_messages = { - 'message_id' => $params->{'message_id'}, - 'limit' => $params->{'limit'} // 0, - 'borrowernumber' => $params->{'borrowernumber'} // q{}, - 'letter_code' => $params->{'letter_code'} // q{}, - 'message_transport_type' => $params->{'type'} // q{}, - 'where' => $params->{'where'} // q{}, - }; - my $unsent_messages = _get_unsent_messages( $which_unsent_messages ); $domain_limits = Koha::Notice::Util->load_domain_limits; # (re)initialize per run - MESSAGE: foreach my $message ( @$unsent_messages ) { - my $message_object = Koha::Notice::Messages->find( $message->{message_id} ); + while( ( my $message_object = $unsent_messages->next ) && ( !$limit || $count_messages < $limit ) ) { + my $message = $message_object->unblessed; + # If this fails the database is unwritable and we won't manage to send a message that continues to be marked 'pending' $message_object->make_column_dirty('status'); return unless $message_object->store; @@ -1000,9 +1007,10 @@ sub SendQueuedMessages { $message->{'borrowernumber'} || 'Admin' ) if $params->{'verbose'}; # This is just begging for subclassing - next MESSAGE if ( lc($message->{'message_transport_type'}) eq 'rss' ); + next if ( lc($message->{'message_transport_type'}) eq 'rss' ); if ( lc( $message->{'message_transport_type'} ) eq 'email' ) { - _send_message_by_email( $message, $params->{'username'}, $params->{'password'}, $params->{'method'} ); + my $rv = _send_message_by_email( $message, $params->{'username'}, $params->{'password'}, $params->{'method'} ); + $count_messages++ if $rv; } elsif ( lc( $message->{'message_transport_type'} ) eq 'sms' ) { if ( C4::Context->preference('SMSSendDriver') eq 'Email' ) { @@ -1011,12 +1019,12 @@ sub SendQueuedMessages { unless ( $sms_provider ) { warn sprintf( "Patron %s has no sms provider id set!", $message->{'borrowernumber'} ) if $params->{'verbose'}; _set_message_status( { message_id => $message->{'message_id'}, status => 'failed' } ); - next MESSAGE; + next; } unless ( $patron->smsalertnumber ) { _set_message_status( { message_id => $message->{'message_id'}, status => 'failed' } ); warn sprintf( "No smsalertnumber found for patron %s!", $message->{'borrowernumber'} ) if $params->{'verbose'}; - next MESSAGE; + next; } $message->{to_address} = $patron->smsalertnumber; #Sometime this is set to email - sms should always use smsalertnumber $message->{to_address} .= '@' . $sms_provider->domain(); @@ -1029,13 +1037,15 @@ sub SendQueuedMessages { } _update_message_to_address($message->{'message_id'}, $message->{to_address}); - _send_message_by_email( $message, $params->{'username'}, $params->{'password'}, $params->{'method'} ); + my $rv = _send_message_by_email( $message, $params->{'username'}, $params->{'password'}, $params->{'method'} ); + $count_messages++ if $rv; } else { - _send_message_by_sms( $message ); + my $rv = _send_message_by_sms( $message ); + $count_messages++ if $rv; } } } - return scalar( @$unsent_messages ); + return $count_messages; } =head2 GetRSSMessages @@ -1307,9 +1317,10 @@ sub _send_message_by_email { my $message = shift or return; my ($username, $password, $method) = @_; - my $patron = Koha::Patrons->find( $message->{borrowernumber} ); + my $patron; my $to_address = $message->{'to_address'}; unless ($to_address) { + $patron = Koha::Patrons->find( $message->{borrowernumber} ); unless ($patron) { warn "FAIL: No 'to_address' and INVALID borrowernumber ($message->{borrowernumber})"; _set_message_status( @@ -1337,7 +1348,12 @@ sub _send_message_by_email { } # Skip this message if we exceed domain limits in this run - return if Koha::Notice::Util->exceeds_limit({ to => $to_address, limits => $domain_limits }); + if( Koha::Notice::Util->exceeds_limit({ to => $to_address, limits => $domain_limits }) ) { + # Save the to_address if you delay the message so that we dont need to look it up again + _update_message_to_address( $message->{'message_id'}, $to_address ) + if !$message->{to_address}; + return; + } my $subject = $message->{'subject'}; @@ -1350,6 +1366,7 @@ sub _send_message_by_email { my $branch_returnpath = undef; my $library; + $patron //= Koha::Patrons->find( $message->{borrowernumber} ); # we might already found him if ($patron) { $library = $patron->library; $branch_email = $library->from_email_address; -- 2.40.0