diff --git a/C4/Letters.pm b/C4/Letters.pm index ff17637ef6..f871c34503 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -1032,6 +1032,7 @@ sub EnqueueLetter { message_id => $id, borrowernumber => $who_letter_is_for, letter_code => $letter_code, # can be scalar or arrayref + exclude_letter_code => $exclude_letter_code, # can be scalar or arrayref type => $type, # can be scalar or arrayref limit => 50, verbose => 1, @@ -1044,6 +1045,10 @@ 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 optional exclude_letter_code parameter processes all pending messages +except those with the given letter code(s). It cannot be combined with +letter_code; passing both raises a Koha::Exceptions::BadParameter exception. + The optional verbose parameter can be used to generate debugging output. Returns number of messages sent. @@ -1058,6 +1063,18 @@ sub SendQueuedMessages { Koha::Exceptions::BadParameter->throw("Parameter message_id cannot be empty if passed.") if ( exists( $params->{message_id} ) && !$params->{message_id} ); + my $has_letter_code = + ref( $params->{letter_code} ) ? scalar @{ $params->{letter_code} } : $params->{letter_code}; + my $has_exclude_letter_code = + ref( $params->{exclude_letter_code} ) + ? scalar @{ $params->{exclude_letter_code} } + : $params->{exclude_letter_code}; + + # letter_code and exclude_letter_code build the same search key ('letter_code'); passing both + # would let one silently clobber the other in the hash literal built below. + Koha::Exceptions::BadParameter->throw("Parameters letter_code and exclude_letter_code are mutually exclusive.") + if ( $has_letter_code && $has_exclude_letter_code ); + if ( C4::Context->config("enable_plugins") ) { my @plugins = Koha::Plugins->new->GetPlugins( { @@ -1084,16 +1101,16 @@ sub SendQueuedMessages { $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} ) : (), - - # Check for scalar or array in exclude_letter_code - ref( $params->{exclude_letter_code} ) - && @{ $params->{exclude_letter_code} } ? ( letter_code => { '-not_in' => $params->{exclude_letter_code} } ) : (), - !ref( $params->{exclude_letter_code} ) - && $params->{exclude_letter_code} ? ( letter_code => { '!=' => $params->{exclude_letter_code} } ) : (), + # letter_code and exclude_letter_code can each be scalar or arrayref; guaranteed + # mutually exclusive above, so they never both contribute a letter_code key here. + $has_letter_code ? ( letter_code => $params->{letter_code} ) : (), + $has_exclude_letter_code + ? ( + letter_code => ref( $params->{exclude_letter_code} ) + ? { '-not_in' => $params->{exclude_letter_code} } + : { '!=' => $params->{exclude_letter_code} } + ) + : (), ref( $params->{type} ) && @{ $params->{type} } diff --git a/misc/cronjobs/process_message_queue.pl b/misc/cronjobs/process_message_queue.pl index 3d5ca291be..f28b1c81bc 100755 --- a/misc/cronjobs/process_message_queue.pl +++ b/misc/cronjobs/process_message_queue.pl @@ -75,6 +75,12 @@ ENDUSAGE die $usage if $help; +# Remove empty elements, see bug 37075. -c/-x take an optional value ('c|code:s'), so a bare +# flag with no value still leaves an empty string in the array; strip those before the +# mutual-exclusivity check below, otherwise a bare -c would falsely trigger it (bug 40934). +@letter_code = grep { $_ ne q{} } @letter_code; +@exclude_letter_code = grep { $_ ne q{} } @exclude_letter_code; + if ( @letter_code && @exclude_letter_code ) { die "The options --code and --exclude-code are mutually exclusive.\nUse one or the other.\n" . $usage; } @@ -91,10 +97,6 @@ try { exit; }; -# Remove empty elements, see bug 37075 -@letter_code = grep { $_ ne q{} } @letter_code; -@exclude_letter_code = grep { $_ ne q{} } @exclude_letter_code; - C4::Letters::SendQueuedMessages( { verbose => $verbose, diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index ec35595de1..f5946523cb 100755 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -1826,7 +1826,7 @@ subtest 'Virtual method ->strftime in notices' => sub { }; subtest 'Test exclude_letter_code parameter for SendQueuedMessages' => sub { - plan tests => 10; + plan tests => 12; my $dbh = C4::Context->dbh; @@ -1947,6 +1947,34 @@ subtest 'Test exclude_letter_code parameter for SendQueuedMessages' => sub { is( Koha::Notice::Messages->find( $id[2] )->status, 'sent', 'ACQ_NOTIF message processed' ); is( Koha::Notice::Messages->find( $id[3] )->status, 'pending', 'TEST_MESSAGE message excluded, still pending' ); + + # Reset messages to pending + Koha::Notice::Messages->find( $id[2] )->update( { status => 'pending' } ); + Koha::Notice::Messages->find( $id[3] )->update( { status => 'pending' } ); + + throws_ok { + C4::Letters::SendQueuedMessages( + { + letter_code => ['ACQ_NOTIF'], + exclude_letter_code => ['DUEDGST'], + type => 'sms', + } + ); + } + 'Koha::Exceptions::BadParameter', + 'SendQueuedMessages dies when letter_code and exclude_letter_code are both passed'; + + throws_ok { + C4::Letters::SendQueuedMessages( + { + letter_code => 'ACQ_NOTIF', + exclude_letter_code => 'DUEDGST', + type => 'sms', + } + ); + } + 'Koha::Exceptions::BadParameter', + 'SendQueuedMessages dies when scalar letter_code and exclude_letter_code are both passed'; }; $schema->storage->txn_rollback; diff --git a/t/db_dependent/cronjobs/process_message_queue.pl.t b/t/db_dependent/cronjobs/process_message_queue.pl.t new file mode 100755 index 0000000000..6498d6c7a5 --- /dev/null +++ b/t/db_dependent/cronjobs/process_message_queue.pl.t @@ -0,0 +1,45 @@ +#!/usr/bin/perl +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use FindBin qw($Bin); +use Test::More tests => 3; +use Test::NoWarnings; + +# Regression tests for bug 40934: the --code/--exclude-code mutual-exclusivity +# check must only fire when both options are given an actual value, not when +# one is passed bare (GetOptions' optional-value ':s' type leaves an empty +# string in the array in that case). + +my $script = "$Bin/../../../misc/cronjobs/process_message_queue.pl"; + +# --where "1=0" keeps this a safe no-op: no message ever matches, so nothing +# is ever sent even though the script runs for real against the test database. +my $safe_run = "$^X $script --where '1=0'"; + +my $output = `$safe_run --code FOO --exclude-code BAR 2>&1`; +like( + $output, qr/mutually exclusive/i, + '--code and --exclude-code together are still rejected as mutually exclusive' +); + +$output = `$safe_run -c --exclude-code DUEDGST 2>&1`; +unlike( + $output, qr/mutually exclusive/i, + 'bare -c with --exclude-code is not falsely rejected as mutually exclusive (bug 40934)' +);