Bug 15986

Summary: Add a script for sending hold waiting reminder notices
Product: Koha Reporter: Nick Clemens <nick>
Component: Command-line UtilitiesAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, amy, andrewfh, axelle.clarisse, barbara.johnson, baselibrary.consortium, chris, david, fridolin.somers, george, jonathan.druart, katrin.fischer, kgnerre, kyle, m.de.rooy, martin.renvoize, nicole, robin, sally.healey, severine.queune, tomascohen, veron, victor
Version: masterKeywords: Manual
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch adds a new script (holds_reminder.pl) for sending holds reminder notices to patrons. The script is intended to be run as a cronjob and takes several parameters: days - after how many days waiting to send the notice lettercode - to specify which notice is used library - (repeatable) to specify which branches to send notices from holidays - to specify that holidays should not be counted as waiting days mtt - (message transport type) to specify which notice format to use, i.e. print, email, etc. date - to run the script as it would have been on a specific date n - no emails - test mode, report only, do not send notices
Version(s) released in:
21.05.00
Bug Depends on:    
Bug Blocks: 31011, 28153, 28198, 28581, 28797, 29704, 29801    
Attachments: Bug 15986 - Add a script for sending hold waiting reminder notices
Bug 15986 - Add a script for sending hold waiting reminder notices
Bug 15986 - Add a script for sending hold waiting reminder notices
[SIGNED-OFF] Bug 15986 - Add a script for sending hold waiting reminder notices
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Update for bug 24159
Bug 15986: Rename send_notice to queue_notice and update test count
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Update for bug 24159
Bug 15986: Rename send_notice to queue_notice and update test count
Bug 15986: Add a script for sending hold waiting reminder notices
Bug 15986: Update for bug 24159
Bug 15986: Rename send_notice to queue_notice and update test count
Bug 15986: (QA follow-up) Account for bugs 25333 and 25334
Bug 15986: (QA follow-up) Fix spelling mistake
Bug 15986: (follow-up) Fix usage statement
Bug 15986: (follow-up) Use inbound_email_address for library
Bug 15986: (follow-up) Add all holds per patron
Bug 15986: Add sample notice and use as default
Bug 15986: Add sample notice to translated notices
Bug 15986: (follow-up) Add atomic update
Bug 15986: Use KohaDates filter to correctly format the waitingdate
Bug 15986: Fix typo following vs follwing
Bug 15986: (follow-up) Switch nomail to confirm
Bug 15986: move holds_reminder.pl to misc/cronjobs/holds/
Bug 15986: Add missing notice template for it-IT
Bug 15986: Remove \r from HOLD_REMINDER in the yml

Description Nick Clemens 2016-03-04 20:33:46 UTC
Write a script that will take two parameters:

a) Length of time hold has been waiting ( in days )

b) Letter code of notice to be emailed

2) Script will take these two parameters and send the specified notice for each hold a patron has.
Comment 1 Nick Clemens 2016-07-12 17:28:42 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2016-07-13 00:21:13 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2016-07-18 03:24:51 UTC
Comment on attachment 53354 [details] [review]
Bug 15986 - Add a script for sending hold waiting reminder notices

Review of attachment 53354 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/holds_reminder.pl
@@ +1,4 @@
> +#!/usr/bin/perl
> +
> +# Copyright 2008 Liblime
> +# Copyright 2010 BibLibre

Can we please fix the copyright notices, this is a new script right?
Comment 4 Chris Cormack 2016-07-18 03:26:34 UTC
Just a general comment, we have a setting where borrowers can opt in/out of holds filled notices.
Should this script respect that preference, IE if they don't get a hold filled notice will they be happy getting one saying it's waiting. Annoying patrons is something libraries generally like to avoid :)

It should be easy enough to use the check we have for holds filled in circulation I hope
Comment 5 Nick Clemens 2016-07-21 16:40:42 UTC
Created attachment 53594 [details] [review]
Bug 15986 - Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.
Essentially it is a modification of the overdue_notices.pl script

To test:
1 - Create a hold reminder notice (Koha module: Holds)
2 - Set some waiting holds for patrons
3 - Run the script with various options and verify correct data is
    generated
4 - Verify that with 'optout' switch set only patrons who have email
hold filled notices specified will recieve a reminder

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)
Comment 6 Nick Clemens 2016-07-21 16:47:17 UTC
(In reply to Chris Cormack from comment #4)
> Just a general comment, we have a setting where borrowers can opt in/out of
> holds filled notices.
> Should this script respect that preference, IE if they don't get a hold
> filled notice will they be happy getting one saying it's waiting. Annoying
> patrons is something libraries generally like to avoid :)

Good point, I added it as a switch, some libraries charge fines for holds not picked up etc, so they may want to force the notification, I figure this way makes it flexible
Comment 7 Chris Cormack 2016-07-21 21:31:25 UTC
Yep sounds good, I'll give it a test when I'm back at work
Comment 8 Kyle M Hall 2016-09-20 16:19:01 UTC
Created attachment 55723 [details] [review]
[SIGNED-OFF] Bug 15986 - Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.
Essentially it is a modification of the overdue_notices.pl script

To test:
1 - Create a hold reminder notice (Koha module: Holds)
2 - Set some waiting holds for patrons
3 - Run the script with various options and verify correct data is
    generated
4 - Verify that with 'optout' switch set only patrons who have email
hold filled notices specified will recieve a reminder

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)

Signed-off-by: Robert <rhilliker@hotchkiss.org>
Comment 9 Katrin Fischer 2016-11-01 23:02:59 UTC
I love the amount of documentation for this script!

I didn't get to test this yet, but while reading through it I wondered:
There is a lot of work done in order to allow exporting in different formats. I was wondering if it might be better to leave the job of creating a printable version to gather_print_notices.pl working towards a more unified workflow for all the printed letters.
Comment 10 Marcel de Rooy 2016-11-02 15:36:55 UTC
I like the feature, but would love to see less (new) code in cron scripts and more code in our modules. With advantages of testing etc.
Comment 11 Sally 2017-01-16 14:13:59 UTC
This is a really exciting feature.

I know it's been sent back to discussion from being signed off - what's the next step for moving it forward?  We think this would make a fantastic addition to Koha.
Comment 12 Chris Cormack 2017-01-17 04:09:38 UTC
(In reply to Sally Healey from comment #11)
> This is a really exciting feature.
> 
> I know it's been sent back to discussion from being signed off - what's the
> next step for moving it forward?  We think this would make a fantastic
> addition to Koha.

To move it forward it needs comment 10 addressed, and a lot of the code moved out of the .pl file into a module, with unit tests written. 
This shouldn't be a big job, but will mean way less chance of breakage/maintenance in the future.
As well as a signoff from outside of the sponsoring library.

I think comment 9 could be a future enhancement. But mostly comment 10 (moving the code to a module and writing tests) and an independent sign off are what are need
Comment 13 Katrin Fischer 2017-01-17 23:38:25 UTC
I think if we do #9 later it will introduce a change in workflow - if we did it from the beginning, we could safe people from having to change their setups. But I am not blocking this, just explaining my point of view maybe also for future enh.
Comment 14 Nick Clemens 2019-07-25 13:58:31 UTC
Created attachment 91767 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAYS) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 4
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)
Comment 15 Nick Clemens 2019-07-25 14:00:10 UTC
(In reply to Chris Cormack from comment #12)
> I think comment 9 could be a future enhancement. But mostly comment 10
> (moving the code to a module and writing tests) and an independent sign off
> are what are need

Code for sending moved to Koha::Patron::send_notice
Comment 16 Nick Clemens 2019-07-25 14:25:52 UTC
(In reply to Katrin Fischer from comment #13)
> I think if we do #9 later it will introduce a change in workflow - if we did
> it from the beginning, we could safe people from having to change their
> setups. But I am not blocking this, just explaining my point of view maybe
> also for future enh.

Removed the option to print out notices, now fallback to print if no email/sms
Comment 17 Nick Clemens 2019-07-25 14:40:24 UTC
Created attachment 91768 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAYS) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 4
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)
Comment 18 David Nind 2020-02-17 08:23:15 UTC
Patch no longer applies
Comment 19 Nick Clemens 2020-03-02 14:21:19 UTC
Created attachment 99919 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAYS) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 4
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)
Comment 20 Jonathan Druart 2020-03-02 14:57:04 UTC
Is the test_mode parameter of the method really useful?
Comment 21 Nick Clemens 2020-04-07 14:35:21 UTC
(In reply to Jonathan Druart from comment #20)
> Is the test_mode parameter of the method really useful?

Yes.
Comment 22 David Nind 2020-04-08 20:23:30 UTC
Things move too fast 8-) - patch no longer applies:

root@5e1d1195a414:koha(bz15986)$ git bz apply 15986
Bug 15986 - Add a script for sending hold waiting reminder notices

99919 - Bug 15986: Add a script for sending hold waiting reminder notices

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 15986: Add a script for sending hold waiting reminder notices
Using index info to reconstruct a base tree...
M	C4/Letters.pm
M	Koha/Patron.pm
M	t/db_dependent/Koha/Patrons.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Patrons.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Patrons.t
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
Auto-merging C4/Letters.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 15986: Add a script for sending hold waiting reminder notices

Also noticed that there is a holds subfolder in misc/cronjobs - should the script go in that?
Comment 23 Nick Clemens 2020-04-10 13:15:09 UTC
Created attachment 102723 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAYS) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 4
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)
Comment 24 David Nind 2020-04-14 11:17:02 UTC
For step 17, 19, 21, and 24 I'm getting an error message when running the script:

perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
branch CPL passed on parameter
Can't locate object method "in_units" via package "-3" (perhaps you forgot to load "-3"?) at /kohadevbox/koha/Koha/Calendar.pm line 193.
Comment 25 David Nind 2020-04-14 11:22:09 UTC
Also, in step 11 '..INTERVAL 3 DAYS)..' should be '..INTERVAL 3 DAY)..' (that is, DAYS should be DAY).
Comment 26 Nick Clemens 2020-04-14 11:55:15 UTC
Created attachment 102926 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAY) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 4
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)
Comment 27 Nick Clemens 2020-04-14 11:55:44 UTC
(In reply to David Nind from comment #24)
> For step 17, 19, 21, and 24 I'm getting an error message when running the
> script:
> 
> perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
> -holidays -days 3
> branch CPL passed on parameter
> Can't locate object method "in_units" via package "-3" (perhaps you forgot
> to load "-3"?) at /kohadevbox/koha/Koha/Calendar.pm line 193.

Fixed, thanks David!
Comment 28 David Nind 2020-04-15 17:53:59 UTC
Thanks Nick!

I can now get most things to work, except for steps 17 and 21 - not sure what I'm doing wrong here.

There are reserves table entries: current date = 2020-04-15, waitingdate for items = 2020-04-12, holiday date set (day = 14, month =4, yeare = 20204

Step 17:

perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 4
branch CPL passed on parameter
No reserves found for CPL

Step 21:

perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 4 -mtt sms
No reserves found for CPL
Message 'HOLDREMINDER' content not found for FFL
Message 'HOLDREMINDER' content not found for FPL
...
Comment 29 Martin Renvoize 2020-04-20 13:20:04 UTC
Comment on attachment 102926 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

Review of attachment 102926 [details] [review]:
-----------------------------------------------------------------

::: Koha/Patron.pm
@@ +1695,4 @@
>      };
>  }
>  
> +=head3 send_notice

Hmm, feels like this should be 'queue_notice' rather than 'send_notice'.. that paves the way for a later 'send_notice' which sends immediately vs waiting in the queue.
Comment 30 Katrin Fischer 2020-04-20 16:47:36 UTC
(In reply to Martin Renvoize from comment #29)
> Comment on attachment 102926 [details] [review] [review]
> Bug 15986: Add a script for sending hold waiting reminder notices
> 
> Review of attachment 102926 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Patron.pm
> @@ +1695,4 @@
> >      };
> >  }
> >  
> > +=head3 send_notice
> 
> Hmm, feels like this should be 'queue_notice' rather than 'send_notice'..
> that paves the way for a later 'send_notice' which sends immediately vs
> waiting in the queue.

We already have a way to send directly without waiting through the queue - it's used by the self registration and password reset emails: bug 21390

+            my $num_letters_attempted = C4::Letters::SendQueuedMessages( {
+                    letter_code => 'OPAC_REG_VERIFY'
+                    } );
Comment 31 Andrew Fuerste-Henry 2020-05-18 12:25:23 UTC
I'm also getting stuck at step 17. I have my reserves with waiting date set to curdate-3. They properly trigger a notice when not worrying about holidays. But once I set the -holidays flag in the command I cannot get notices to generate, no matter how many days I give it.
Comment 32 Nick Clemens 2020-07-16 16:54:10 UTC
Created attachment 106962 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAY) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 2
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)
Comment 33 Nick Clemens 2020-07-16 16:54:13 UTC
Created attachment 106963 [details] [review]
Bug 15986: Update for bug 24159

The job is concerned with holds waiting and takes days, I think using days_mode 'Calendar'
makes sense as we are not calculating due dates
Comment 34 Nick Clemens 2020-07-16 16:54:17 UTC
Created attachment 106964 [details] [review]
Bug 15986: Rename send_notice to queue_notice and update test count
Comment 35 Nick Clemens 2020-07-16 17:04:13 UTC
Adding a holiday means the holds are waiting 1-less day, not 1 more :-)

Fixed the math in the test plan :-)
Comment 36 Séverine Queune 2020-11-03 17:34:10 UTC
This patch works fine !
But I won't sign it because of 2 points : 
- I can't test the sms part, I don't know how it works and how to use the feature
- I've got a question about the content of the message : when there several holds, only the informations about the oldest reserve (for what I saw) are displayed. Fields coming from the items or biblio tables can't be used. I know it's happening for some other notices so don't Failed the patch because of this point, but I regret I couldn't use the <<count>> or <<items.content>> datas as it is recommended for some other notices, I think it could be useful for patron to have some details about the waiting holds.
Comment 37 Andrew Fuerste-Henry 2020-11-04 14:49:08 UTC
Created attachment 112999 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAY) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 2
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)

Signed-off-by: Kim Gnerre <kgnerre@hotchkiss.org>
Comment 38 Andrew Fuerste-Henry 2020-11-04 14:49:11 UTC
Created attachment 113000 [details] [review]
Bug 15986: Update for bug 24159

The job is concerned with holds waiting and takes days, I think using days_mode 'Calendar'
makes sense as we are not calculating due dates

Signed-off-by: Kim Gnerre <kgnerre@hotchkiss.org>
Comment 39 Andrew Fuerste-Henry 2020-11-04 14:49:15 UTC
Created attachment 113001 [details] [review]
Bug 15986: Rename send_notice to queue_notice and update test count

Signed-off-by: Kim Gnerre <kgnerre@hotchkiss.org>
Comment 40 Nick Clemens 2020-11-04 15:24:03 UTC
(In reply to Séverine Queune from comment #36)
> This patch works fine !
> But I won't sign it because of 2 points : 
> - I can't test the sms part, I don't know how it works and how to use the
> feature
> - I've got a question about the content of the message : when there several
> holds, only the informations about the oldest reserve (for what I saw) are
> displayed. Fields coming from the items or biblio tables can't be used. I
> know it's happening for some other notices so don't Failed the patch because
> of this point, but I regret I couldn't use the <<count>> or
> <<items.content>> datas as it is recommended for some other notices, I think
> it could be useful for patron to have some details about the waiting holds.

- sms part works exactly like email up to the sending of the message, so testing is the same, just need to confirm it makes it into the queue

- the notice supports template toolkit, so you can get other holds information like:
Dear [% borrower.firstname %] [% borrower.surname %],

Your hold for [% hold.biblio.title %] is waiting since [% hold.waitingdate %]

[% IF borrower.holds.count > 0 %]
  You also have other holds:
  [% FOREACH hold IN borrower.holds %]
    [% hold.biblio.title %] : [% hold.found %]
  [% END %]
[% END %]

That could be cleaned up and made fancier, but just to give an idea
Comment 41 Séverine Queune 2020-11-04 15:28:01 UTC
(In reply to Nick Clemens from comment #40)
> (In reply to Séverine Queune from comment #36)
> > This patch works fine !
> > But I won't sign it because of 2 points : 
> > - I can't test the sms part, I don't know how it works and how to use the
> > feature
> > - I've got a question about the content of the message : when there several
> > holds, only the informations about the oldest reserve (for what I saw) are
> > displayed. Fields coming from the items or biblio tables can't be used. I
> > know it's happening for some other notices so don't Failed the patch because
> > of this point, but I regret I couldn't use the <<count>> or
> > <<items.content>> datas as it is recommended for some other notices, I think
> > it could be useful for patron to have some details about the waiting holds.
> 
> - sms part works exactly like email up to the sending of the message, so
> testing is the same, just need to confirm it makes it into the queue
> 
> - the notice supports template toolkit, so you can get other holds
> information like:
> Dear [% borrower.firstname %] [% borrower.surname %],
> 
> Your hold for [% hold.biblio.title %] is waiting since [% hold.waitingdate %]
> 
> [% IF borrower.holds.count > 0 %]
>   You also have other holds:
>   [% FOREACH hold IN borrower.holds %]
>     [% hold.biblio.title %] : [% hold.found %]
>   [% END %]
> [% END %]
> 
> That could be cleaned up and made fancier, but just to give an idea

Thanks Nick !
I completely forgot we could use template toolkit
#ashamed... 
Do you need a second sign off for that one ?
Comment 42 Victor Grousset/tuxayo 2020-12-29 15:23:50 UTC
Can't QA this one but got a question about -holidays, why is it better to not take them into account by default? I don't have much functional experience so I might miss something obvious ^^
Comment 43 Andrew Fuerste-Henry 2020-12-29 15:29:13 UTC
The various sysprefs that determine whether or not to consult the calendar for various tasks are inconsistent in a new install -- some are on and some are off. However, ExcludeHolidaysFromMaxPickUpDelay defaults to off. That's not exactly the same as the --holiday flag here, but seems close enough that I'm happy to make this new feature keep the same default choice.
Comment 44 Victor Grousset/tuxayo 2020-12-29 21:08:34 UTC
> The various sysprefs that determine whether or not to consult the calendar for various tasks are inconsistent in a new install

:o

> However, ExcludeHolidaysFromMaxPickUpDelay defaults to off. That's not exactly the same as the --holiday flag here, but seems close enough that I'm happy to make this new feature keep the same default choice.

Thanks for the analysis. Is there an obviously less surprising default choice there? Or does it depend too much?
Comment 45 Andrew Fuerste-Henry 2020-12-29 21:21:29 UTC
I don't really get the sense one option is more standard or more likely to be correct for any given library -- it's just one of those things one ends up asking every new library about. Same with UseDaysMode, finesCalendar, OverdueNoticeCalendar, etc.
Comment 46 Victor Grousset/tuxayo 2021-01-01 17:59:25 UTC
Good, there doesn't seem to be a bad choice.
Thanks for the info Andrew, that was my 2 cents, I'll leave the rest to more knowledgeable people ^^"
Comment 47 Martin Renvoize 2021-03-24 11:46:15 UTC
Created attachment 118733 [details] [review]
Bug 15986: Add a script for sending hold waiting reminder notices

This patch adds a script for sending holds reminder notice to patrons.

We add a 'send_notice' routine to Koha::Patrons - this will either send using the patron's
email prefs, or allow forcing of a single method via the cron

To test:
 1 - Create an email hold reminder notice for a single library (Koha module: Holds, code HOLDREMINDER, branch: CPL)
 2 - Set some waiting holds today for patrons at CPL, ensure those patrons have 'email' as the transport for hold filled notices
 3 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL
 4 - You should see the patrons here would have received emails
 5 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL
 6 - You should see the emails that were sent
 7 - Check the patron notices tab to confirm
 8 - Note a ptron with two holds waiting receives only one notice
 9 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
10 - No notices are sent
11 - Adjust the waiting date for the holds:
    UPDATE reserves SET waitingdate=DATE_SUB(CURDATE(), INTERVAL 3 DAY) WHERE waitingdate = CURDATE();
12 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -li CPL -days 3
13 - Confirm the holds are now reminded
14 - Set yesterday as a holiday for CPL
15 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 3
16 - Notices should not be sent
17 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -li CPL -holidays -days 2
18 - Notices should be sent again
19 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2
20 - Should get feedback that notice was not found for other libraries
21 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2 -mtt sms
22 - Notice is not found
23 - Add the notice for sms
24 - perl misc/cronjobs/holds_reminder.pl -v -lettercode HOLDREMINDER -n -holidays -days 2 -mtt sms
25 - The notice should be sent
26 - Check patrons messaging tab to confirm
27 - prove -v t/db_dependent/Koha/Patrons.t

Sponsored by: The Hotchkiss School (http://www.hotchkiss.org/)

Signed-off-by: Kim Gnerre <kgnerre@hotchkiss.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2021-03-24 11:46:20 UTC
Created attachment 118734 [details] [review]
Bug 15986: Update for bug 24159

The job is concerned with holds waiting and takes days, I think using days_mode 'Calendar'
makes sense as we are not calculating due dates

Signed-off-by: Kim Gnerre <kgnerre@hotchkiss.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2021-03-24 11:46:24 UTC
Created attachment 118735 [details] [review]
Bug 15986: Rename send_notice to queue_notice and update test count

Signed-off-by: Kim Gnerre <kgnerre@hotchkiss.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2021-03-24 11:46:29 UTC
Created attachment 118736 [details] [review]
Bug 15986: (QA follow-up) Account for bugs 25333 and 25334

Bug 25333 changed the existing 'phone' message transport type to 'itiva' and
bug 25334 added a new 'phone' message transport type.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2021-03-24 11:48:19 UTC
Whilst I feel we could continue to iterate on this and make improvements, I think this enhancement has waited long enough and those iterations can come in the future as the queue_notice method starts to get more widely adopted.

It all works as expected, contained tests for the new Patron method and causes no regressions.

Passing QA
Comment 52 Martin Renvoize 2021-03-24 11:49:49 UTC
Created attachment 118737 [details] [review]
Bug 15986: (QA follow-up) Fix spelling mistake

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Nick Clemens 2021-04-07 11:42:55 UTC
Created attachment 119277 [details] [review]
Bug 15986: (follow-up) Fix usage statement
Comment 54 Jonathan Druart 2021-04-07 13:38:48 UTC
1.
1824     my $admin_email_address = $library->{branchemail} || C4::Context->preference('KohaAdminEmailAddress');

Shouldn't it be $library->inbound_email_address instead?

2.
1844         next if ($mtt eq 'phone' and C4::Context->preference('TalkingTechItivaPhoneNotification') );

There are now 2 mtt, phone and itiva. Here you need to compare with 'itiva', and add a new condition for 'phone'.

3. Shouldn't we rename "send_notice" to "enqueue_notice" as we don't actually send it right away?

4. Why don't we insert a sample notice template and use it as the default for the script?

5.
+    # If passed message transports we force use those, otherwise we will use the patrons preferences
+    # for the 'Hold_Filled' notice
+    my $sending_params = @mtts ? { message_transports => \@mtts } : { message_name => "Hold_Filled" };

We should add that to the POD

6.
+        # Skip if we already dealt with this borrower
+        next if ( $done{$patron->borrowernumber} );

What if a patron has more than one hold? We won't be able to send a notice like "You have X holds waiting"

7. (not blocker) please don't forget to perltidy new files and blocks of code.
Comment 55 Nick Clemens 2021-04-07 15:02:04 UTC
Created attachment 119308 [details] [review]
Bug 15986: (follow-up) Use inbound_email_address for library
Comment 56 Nick Clemens 2021-04-07 15:05:00 UTC
(In reply to Jonathan Druart from comment #54)
> 1. Shouldn't it be $library->inbound_email_address instead?

Done

> 2.There are now 2 mtt, phone and itiva. Here you need to compare with 'itiva',
> and add a new condition for 'phone'. 
> 3. Shouldn't we rename "send_notice" to "enqueue_notice" as we don't
> actually send it right away?
> 5. # If passed message transports we force use those, otherwise we will
> use the patrons preferences

These are all done in followups



> 4. Why don't we insert a sample notice template and use it as the default
> for the script?

This can be done on a new bug 

> 6. What if a patron has more than one hold? We won't be able to send a notice
> like "You have X holds waiting"

We can access all of their holds in template toolkit

[% FOREACH hold IN hold.patron.holds %]
...
[% END %]
Comment 57 Martin Renvoize 2021-04-07 17:20:48 UTC
(In reply to Nick Clemens from comment #55)
> Created attachment 119308 [details] [review] [review]
> Bug 15986: (follow-up) Use inbound_email_address for library

Damn, Shame on me for missing that one..

I agree the others should already be fixed by my followups.

A sample notice would be nice, I agree here with Jonathan really... Especially given the chance sample here where you iterate through holds to construct a digest.
Comment 58 Jonathan Druart 2021-04-08 09:33:17 UTC
(In reply to Martin Renvoize from comment #57)
> (In reply to Nick Clemens from comment #55)
> > Created attachment 119308 [details] [review] [review] [review]
> > Bug 15986: (follow-up) Use inbound_email_address for library
> 
> Damn, Shame on me for missing that one..
> 
> I agree the others should already be fixed by my followups.

Sorry about that, I usually review all the patches squashed, it seems that I forgot to squash those before review!

> A sample notice would be nice, I agree here with Jonathan really...
> Especially given the chance sample here where you iterate through holds to
> construct a digest.

And shouldn't we actually reflect that in the script? We should loop over the patrons, not the holds.
Comment 59 Nick Clemens 2021-04-08 14:56:19 UTC
Created attachment 119327 [details] [review]
Bug 15986: (follow-up) Add all holds per patron

This changes the code to loop through all the holds and group by patron,
we then send the holds to the letter using the 'loops' option
Comment 60 Nick Clemens 2021-04-08 14:56:24 UTC
Created attachment 119328 [details] [review]
Bug 15986: Add sample notice and use as default
Comment 61 Nick Clemens 2021-04-08 14:56:28 UTC
Created attachment 119329 [details] [review]
Bug 15986: Add sample notice to translated notices
Comment 62 Nick Clemens 2021-04-09 14:15:49 UTC
Created attachment 119383 [details] [review]
Bug 15986: (follow-up) Add atomic update
Comment 63 Jonathan Druart 2021-04-09 14:18:33 UTC
Created attachment 119384 [details] [review]
Bug 15986: Use KohaDates filter to correctly format the waitingdate
Comment 64 Jonathan Druart 2021-04-09 14:32:32 UTC
Created attachment 119386 [details] [review]
Bug 15986: Fix typo following vs follwing
Comment 65 Jonathan Druart 2021-04-09 14:39:44 UTC
Few more questions, (sorry!)

1. 
 58    -n                             No email will be sent
shouldn't be replaced with a --confirm flag?

2. Shouldn't we add a new message preference to let patrons a choice to opt-out if they wish? That could be done on a separate bug report but needed for 21.05
Comment 66 Nick Clemens 2021-04-15 12:55:22 UTC
Created attachment 119624 [details] [review]
Bug 15986: (follow-up) Switch nomail to confirm
Comment 67 Jonathan Druart 2021-04-16 12:34:21 UTC
Created attachment 119725 [details] [review]
Bug 15986: move holds_reminder.pl to misc/cronjobs/holds/
Comment 68 Jonathan Druart 2021-04-16 13:12:01 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 69 Jonathan Druart 2021-04-16 14:08:02 UTC
Created attachment 119748 [details] [review]
Bug 15986: Add missing notice template for it-IT
Comment 70 Jonathan Druart 2021-04-16 14:08:49 UTC
Follow-up pushed to master.
Comment 71 Fridolin Somers 2021-04-22 14:32:01 UTC
Enhancement not pushed to 20.11.x
Comment 72 Jonathan Druart 2021-04-27 13:29:42 UTC
Created attachment 120224 [details] [review]
Bug 15986: Remove \r from HOLD_REMINDER in the yml

To prevent the following errors on `gulp po:update`

misc/translator/Koha-installer.pot:952: warning: internationalized messages should not contain the '\r' escape sequence
Comment 73 Jonathan Druart 2021-04-27 14:25:38 UTC
Follow-up pushed to master.
Comment 74 Fridolin Somers 2021-04-30 08:31:07 UTC
Enhancement not pushed to 20.11.x