Bug 27265

Summary: process_message_queue.pl cron should be able to take multiple types as a parameter
Product: Koha Reporter: Nick Clemens <nick>
Component: NoticesAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, arthur.suzuki, caroline.cyr-la-rose, david, dcook, kyle, lucas, m.de.rooy, sally.healey, tomascohen, wainuiwitikapark
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27096
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Caroline Cyr La Rose Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/679
Text to go in the release notes:
This patch adds the ability to specify several types or letter codes when running the process_message_queue script. This allows libraries to consolidate calls when some message types or letter codes are scheduled differently than others
Version(s) released in:
22.11.00, 22.05.8, 21.11.14
Bug Depends on: 25097    
Bug Blocks:    
Attachments: Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: (follow-up) Use existing parameter
Bug 27265: Enable message queue cron to accept multiple codes and types
Bug 27265: (follow-up) Use existing parameter
Bug 27265: (QA follow-up) Typo on L99 cron script

Description Nick Clemens 2020-12-17 17:39:23 UTC
It would be nice to be able to combine several types in a single run, but exclude others, without having to have multiple cron lines
Comment 1 Kyle M Hall 2020-12-18 13:53:27 UTC
Created attachment 114522 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 2 Kyle M Hall 2020-12-18 14:00:50 UTC
Created attachment 114523 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 3 Kyle M Hall 2020-12-18 14:02:08 UTC
Created attachment 114525 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 4 Andrew Fuerste-Henry 2020-12-23 14:58:24 UTC
This isn't working for me. Here's what I see:
1- have a patron with email enabled for CHECKOUT, CHECKIN notices and an email address on their account
2- check item out to patron, check item in again
3- confirm on patron notices page that they have pending CHECKOUT and CHECKIN notices
4- perl misc/cronjobs/process_message_queue.pl -c CHECKOUT -c CHECKIN
5- notices are not sent
6- perl misc/cronjobs/process_message_queue.pl -c CHECKOUT
7- one notice is sent
Comment 5 Nick Clemens 2021-01-20 12:51:41 UTC
Created attachment 115430 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 6 David Nind 2021-06-05 20:43:16 UTC
Patch no longer applies 8-(..

root@kohadevbox:koha(bz27265)$ git bz apply 27265

Bug 27265 - Process message queue cron should be able to take multiple types as a parameter

115430 - Bug 27265: Enable message queue cron to accept multiple codes and types

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 27265: Enable message queue cron to accept multiple codes and types
Using index info to reconstruct a base tree...
M	C4/Letters.pm
M	misc/cronjobs/process_message_queue.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/cronjobs/process_message_queue.pl
Auto-merging C4/Letters.pm
CONFLICT (content): Merge conflict in C4/Letters.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 27265: Enable message queue cron to accept multiple codes and types
Comment 7 Kyle M Hall 2021-06-08 11:14:19 UTC
Created attachment 121717 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 8 David Nind 2021-12-08 03:40:47 UTC
I get this message when running the script after the patch is applied (koha-testing-docker):

root@kohadevbox:koha(bz27265)$ perl misc/cronjobs/process_message_queue.pl -c HOLD
Global symbol "$type" requires explicit package name (did you forget to declare "my $type"?) at misc/cronjobs/process_message_queue.pl line 89.
Global symbol "$letter_code" requires explicit package name (did you forget to declare "my $letter_code"?) at misc/cronjobs/process_message_queue.pl line 90.
Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) at misc/cronjobs/process_message_queue.pl line 96, near "};"
Type of arg 1 to Try::Tiny::try must be block or sub {} (not reference constructor) at misc/cronjobs/process_message_queue.pl line 96, near "};"
Execution of misc/cronjobs/process_message_queue.pl aborted due to compilation errors.

It worked with a single letter type before the patch was applied.
Comment 9 David Cook 2021-12-08 04:48:55 UTC
I was going to write a very different comment here based on what I read in the "Text to go in the release notes", but after re-reading the comments and code, I realized I totally misunderstood this change.

You might consider changing the phrasing to something like "This patch allows one to specify several types or letter codes, instead of only one type or letter code, when running the process_message_queue script. This allows libraries to consolidate calls when some messages types or letter codes are scheduled differently than others."

I thought this patch added the ability to specify types or letter codes in general, and I was going to comment on that ability, but that ability was added in a previous ticket...
Comment 10 Nick Clemens 2022-02-10 20:54:47 UTC
Created attachment 130476 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 11 Andrew Fuerste-Henry 2022-03-01 13:52:23 UTC
I'm getting this error when trying to run process_message_queue with this patch:

Global symbol "$letter_code" requires explicit package name (did you forget to declare "my $letter_code"?) at misc/cronjobs/process_message_queue.pl line 84.
Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) at misc/cronjobs/process_message_queue.pl line 90, near "};"
Type of arg 1 to Try::Tiny::try must be block or sub {} (not reference constructor) at misc/cronjobs/process_message_queue.pl line 90, near "};"
Execution of misc/cronjobs/process_message_queue.pl aborted due to compilation errors.
Comment 12 Nick Clemens 2022-03-07 13:43:35 UTC
Created attachment 131434 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 13 Andrew Fuerste-Henry 2022-08-05 17:54:02 UTC
Created attachment 138712 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits
Comment 14 Marcel de Rooy 2022-09-30 09:02:26 UTC
andrewfh@dubcolib.org	2022-08-05 17:53:58 UTC	Status	Needs Signoff	Signed Off

Normally we would also expect your signoff line?
Comment 15 Marcel de Rooy 2022-09-30 09:12:05 UTC
At scope borders:

sub SendQueuedMessages 
    my $which_unsent_messages  = {
        'message_id'     => $params->{'message_id'},
        'limit'          => $params->{'limit'} // 0,
        'borrowernumber' => $params->{'borrowernumber'} // q{},
        'letter_code'    => $params->{'letter_code'} // q{},
        'type'           => $params->{'type'} // q{},
 my $unsent_messages = _get_unsent_messages( $which_unsent_messages );

The POD of the following sub does not mention the undocumented 'type'
sub _get_unsent_messages {

        if ( $params->{'message_transport_type'} ) {
            $statement .= ' AND mq.message_transport_type = ? ';
            push @query_params, $params->{'message_transport_type'};
        }
AND
        if ( $params->{'type'} ) {
            my @types = ref $params->{'type'} eq "ARRAY" ? @{$params->{'type'}} : $params->{'type'};
            if ( @types ) {
                my $q = join( ",", ("?") x @types );
                $statement .= " AND message_transport_type IN ( $q ) ";
                push @query_params, @types;
            }
        }

We could better use one parameter and use the one mentioned in POD.
Comment 16 Nick Clemens 2022-09-30 12:34:20 UTC
Created attachment 141129 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 17 Nick Clemens 2022-09-30 12:34:24 UTC
Created attachment 141130 [details] [review]
Bug 27265: (follow-up) Use existing parameter
Comment 18 Andrew Fuerste-Henry 2022-09-30 13:13:48 UTC
(In reply to Marcel de Rooy from comment #14)
> andrewfh@dubcolib.org	2022-08-05 17:53:58 UTC	Status	Needs Signoff	Signed Off
> 
> Normally we would also expect your signoff line?

Oh! I'm not sure how that happened; thanks for catching it.
Comment 19 Marcel de Rooy 2022-10-17 13:10:10 UTC
Created attachment 141966 [details] [review]
Bug 27265: Enable message queue cron to accept multiple codes and types

It would be nice to be able to combine several types in a single run,
but exclude others, without having to have multiple cron lines

Test Plan:
1) Apply this patch
2) Run process_message_queue.pl with a single -c parameter
3) Note behavior is unchanged
4) Run process_message_queue.pl with multiple -c parameters
5) Note all the codes specified are processed
6) Repeat 2-5 with -t for type limits

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2022-10-17 13:10:14 UTC
Created attachment 141967 [details] [review]
Bug 27265: (follow-up) Use existing parameter

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2022-10-17 13:10:18 UTC
Created attachment 141968 [details] [review]
Bug 27265: (QA follow-up) Typo on L99 cron script

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Tomás Cohen Arazi 2022-10-18 00:15:17 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 23 Lucas Gass 2022-12-21 21:02:45 UTC
Backported to 22.05.x for upcoming 22.05.08
Comment 24 Arthur Suzuki 2022-12-22 18:40:12 UTC
applied to 21.11.x for 21.11.14 (backported from 22.05.x)
Comment 25 wainuiwitikapark 2023-03-15 01:20:42 UTC
Not backported to 21.05.x