Bug 40934: (follow-up) Fix false positive on --code/--exclude-code mutual exclusivity
The mutual-exclusivity check ran before the empty-element cleanup added for bug 37075, so a bare -c (no value) left an empty string in @letter_code, which the check saw as truthy and rejected even though -c had effectively not been used. Move the cleanup above the check. Also add the same mutual-exclusivity guard directly in C4::Letters::SendQueuedMessages, since letter_code and exclude_letter_code build the same search key and passing both would otherwise let one silently clobber the other in the hashref passed to ->search - only the CLI script enforced this until now, so any other caller could hit the silent-clobber bug. Adds regression coverage: unit tests for the new exception in SendQueuedMessages, and a script-level test reproducing the RM's reported false positive. Sponsored-by: OpenFifth <https://openfifth.co.uk/> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit is contained in:
Martin Renvoize
Pedro Amorim
parent
2a652a8d12
commit
167622c08d
|
|
@ -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} }
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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 <https://www.gnu.org/licenses>.
|
||||
|
||||
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)'
|
||||
);
|
||||
Loading…
Reference in New Issue