Bugzilla – Attachment 149648 Details for
Bug 33360
SendQueuedMessages: Improve limit behavior and add domain limits
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 33360: Improving limit behavior in SendQueuedMessages
Bug-33360-Improving-limit-behavior-in-SendQueuedMe.patch (text/plain), 9.58 KB, created by
Martin Renvoize (ashimema)
on 2023-04-14 09:05:56 UTC
(
hide
)
Description:
Bug 33360: Improving limit behavior in SendQueuedMessages
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2023-04-14 09:05:56 UTC
Size:
9.58 KB
patch
obsolete
>From 4fc8fbb8be43e66ec211a1716d7561e3ed847973 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >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. > <message_domain_limits> > <domain><name>A</name><limit>1</limit><unit>1h</unit></domain> > <domain><name>B/name><belongs_to>A</belongs_to></domain> > <domain><name>C</name><limit>1</limit><unit>1h</unit></domain> > </message_domain_limits> > 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 <m.de.rooy@rijksmuseum.nl> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 33360
:
149256
|
149257
|
149258
|
149546
|
149547
|
149548
|
149549
|
149550
|
149551
|
149552
|
149645
|
149646
|
149647
|
149648
|
149649
|
149650
|
149651
|
149662
|
149663
|
149664
|
149665
|
149666
|
149667
|
149668