Bug 28553

Summary: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: arthur.suzuki, david, dcook, gmcharlt, kyle.m.hall, kyle, lucas, rachael.laritz, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30355
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.07, 21.11.14, 21.05.21
Bug Depends on: 18532    
Bug Blocks: 30355    
Attachments: Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them
Bug 28553: [QA followup] default to auto renew email when cron switched used

Description Andrew Fuerste-Henry 2021-06-10 19:05:37 UTC
Following bug 18532, one can set a patron's messaging prefs to deliver an auto-renew notice via SMS. However, the auto_renewals cron only knows how to generate email notices. We should not allow the selection of non-functional transport types.

We need to either teach the cron to send SMS notices or teach the messaging prefs to not allow you to select SMS.
Comment 1 Kyle M Hall 2022-07-18 16:46:26 UTC
Created attachment 137830 [details] [review]
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them

Following bug 18532, one can set a patron's messaging prefs to deliver an auto-renew notice via SMS.
However, the auto_renewals cron only knows how to generate email notices. We should not allow the selection of non-functional transport types.

Test Plan:
1) Set a patron to recieve auto-renewals via SMS, leave digest mode
   disabled. Ensure no other transports are enabled either.
2) Auto-renew some items
3) Note the SMS auto-renewal notice was generated!
Comment 2 Rachael 2022-08-02 16:06:23 UTC
Under NOTICES, nothing shows that an SMS was sent.
Comment 3 Nick Clemens 2022-08-11 13:09:00 UTC
Created attachment 139021 [details] [review]
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them

Following bug 18532, one can set a patron's messaging prefs to deliver an auto-renew notice via SMS.
However, the auto_renewals cron only knows how to generate email notices. We should not allow the selection of non-functional transport types.

Test Plan:
 1 - Set pref AutoRenewalNotices - 'according to patron messaging preferences'
 2 - Set circ rule with:
    Loan period: 5 days
    No renewal before: 5
    AutoRenewal: Yes
    Renewals allowed: 5
 3 - Set SMS Send driver to Email
 4 - Find a patron and set them to receive auto renewal notices via SMS and no other transport
 5 - Provide a proivder and sms number for the patron
 6 - Checkout an item to the patron, set due date to yesterday
 7 - Run auto renewals:
    perl misc/cronjobs/automatic_renewals.pl -v --send-notices -c
 8 - Item renewed, no message sent
 9 - Check in item, checkout again due yesterday
10 - Apply patch
11 - Browse to Tools->notices and slips
12 - Edit AUTO_RENEWALS and AUTO_RENEWALS_DIGEST to copy email template to sms and add a title
13 - perl misc/cronjobs/automatic_renewals.pl -v --send-notices -c
14 - Item renewed, confirm patron has a notice in their account
15 - Check the digest option, check in item, checkout again as due, repeat cronjob and note digest notic
16 - sign off

https://bugs.koha-community.org/show_bug.cgi?id=30355
Comment 4 David Nind 2022-09-11 20:34:48 UTC
Created attachment 140412 [details] [review]
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them

Following bug 18532, one can set a patron's messaging prefs to deliver an auto-renew notice via SMS.
However, the auto_renewals cron only knows how to generate email notices. We should not allow the selection of non-functional transport types.

Test Plan:
 1 - Set pref AutoRenewalNotices - 'according to patron messaging preferences'
 2 - Set circ rule with:
    Loan period: 5 days
    No renewal before: 5
    AutoRenewal: Yes
    Renewals allowed: 5
 3 - Set SMS Send driver to Email
 4 - Find a patron and set them to receive auto renewal notices via SMS and no other transport
 5 - Provide a proivder and sms number for the patron
 6 - Checkout an item to the patron, set due date to yesterday
 7 - Run auto renewals:
    perl misc/cronjobs/automatic_renewals.pl -v --send-notices -c
 8 - Item renewed, no message sent
 9 - Check in item, checkout again due yesterday
10 - Apply patch
11 - Browse to Tools->notices and slips
12 - Edit AUTO_RENEWALS and AUTO_RENEWALS_DIGEST to copy email template to sms and add a title
13 - perl misc/cronjobs/automatic_renewals.pl -v --send-notices -c
14 - Item renewed, confirm patron has a notice in their account
15 - Check the digest option, check in item, checkout again as due, repeat cronjob and note digest notic
16 - sign off

https://bugs.koha-community.org/show_bug.cgi?id=30355

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-09-11 20:45:16 UTC
Note that for the AUTO_RENEWALS notice, this message appears for the Email notice, before the last updated date:

  Template Toolkit error: undef error - : filter not found 

This message appears before the patch is applied - I'm not sure what is causing this, as it doesn't appear for a sample of other notices I checked.
Comment 6 David Cook 2022-09-13 06:31:09 UTC
(In reply to David Nind from comment #5)
> Note that for the AUTO_RENEWALS notice, this message appears for the Email
> notice, before the last updated date:
> 
>   Template Toolkit error: undef error - : filter not found 
> 
> This message appears before the patch is applied - I'm not sure what is
> causing this, as it doesn't appear for a sample of other notices I checked.

My guess would be due to the following line:
The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]
Comment 7 David Cook 2022-09-13 06:43:50 UTC
Comment on attachment 140412 [details] [review]
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them

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

The patch title and patch note don't accurately describe the patch. 

They should say something like the following:

Bug 28553: Add SMS notices for auto renew notices

This patch adds support for SMS notices to the
automatic_renewals.pl cronjob. Both digest and
non-digest messages are supported.

::: misc/cronjobs/automatic_renewals.pl
@@ +234,5 @@
>              );
> +        for my $issue ( @{ $report{$borrowernumber} } ) {
> +            my $item = $issue->item;
> +            # Force sending of email and only email if pref is set to "cron"
> +            my @transports = $send_notices_pref eq 'preferences' ? keys %{ $borrower_preferences->{'transports'} } : 'sms';

This is wrong. Instead of 'sms', it should be 'email'.
Comment 8 David Cook 2022-09-13 06:44:21 UTC
*** Bug 30355 has been marked as a duplicate of this bug. ***
Comment 9 David Cook 2022-09-13 06:46:16 UTC
(In reply to David Cook from comment #7)
> ::: misc/cronjobs/automatic_renewals.pl
> @@ +234,5 @@
> >              );
> > +        for my $issue ( @{ $report{$borrowernumber} } ) {
> > +            my $item = $issue->item;
> > +            # Force sending of email and only email if pref is set to "cron"
> > +            my @transports = $send_notices_pref eq 'preferences' ? keys %{ $borrower_preferences->{'transports'} } : 'sms';
> 
> This is wrong. Instead of 'sms', it should be 'email'.

I'll shortly attach a follow-up patch to fix this.
Comment 10 David Cook 2022-09-13 06:49:14 UTC
Created attachment 140510 [details] [review]
Bug 28553: Patrons can be set to receive auto_renew notices as SMS, but Koha does not generate them

Following bug 18532, one can set a patron's messaging prefs to deliver an auto-renew notice via SMS.
However, the auto_renewals cron only knows how to generate email notices. We should not allow the selection of non-functional transport types.

Test Plan:
 1 - Set pref AutoRenewalNotices - 'according to patron messaging preferences'
 2 - Set circ rule with:
    Loan period: 5 days
    No renewal before: 5
    AutoRenewal: Yes
    Renewals allowed: 5
 3 - Set SMS Send driver to Email
 4 - Find a patron and set them to receive auto renewal notices via SMS and no other transport
 5 - Provide a proivder and sms number for the patron
 6 - Checkout an item to the patron, set due date to yesterday
 7 - Run auto renewals:
    perl misc/cronjobs/automatic_renewals.pl -v --send-notices -c
 8 - Item renewed, no message sent
 9 - Check in item, checkout again due yesterday
10 - Apply patch
11 - Browse to Tools->notices and slips
12 - Edit AUTO_RENEWALS and AUTO_RENEWALS_DIGEST to copy email template to sms and add a title
13 - perl misc/cronjobs/automatic_renewals.pl -v --send-notices -c
14 - Item renewed, confirm patron has a notice in their account
15 - Check the digest option, check in item, checkout again as due, repeat cronjob and note digest notic
16 - sign off

https://bugs.koha-community.org/show_bug.cgi?id=30355

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 David Cook 2022-09-13 06:49:18 UTC
Created attachment 140511 [details] [review]
Bug 28553: [QA followup] default to auto renew email when cron switched used

This patch defaults to using "email" rather than "sms" notices
when AutoRenewalNotices is set to use the --send-notices cron switch

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 12 David Cook 2022-09-13 06:59:02 UTC
Despite my comments about the title/note on the first patch, which would probably ordinarily be a QA fail, I think that the patch is important enough to still Pass QA. 

Going to leave it up to the RM to decide if it needs to be rewritten, or to rewrite it, or to let it go. 

Thanks for the patch, Bywater folk!
Comment 13 Tomás Cohen Arazi 2022-09-16 13:34:19 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 14 Lucas Gass 2022-10-31 21:50:39 UTC
Backported to 22.05.x for 22.05.07 release
Comment 15 Arthur Suzuki 2022-11-14 15:53:33 UTC
applied to 21.11 for 21.11.14
Comment 16 Victor Grousset/tuxayo 2022-11-24 20:51:02 UTC
I guess this depends on bug 18532
Comment 17 Victor Grousset/tuxayo 2022-11-24 20:51:37 UTC
Backported: Pushed to 21.05.x branch for the upcoming release (21.05.21)
Nothing to document it seems, marking resolved.