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.
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!
Under NOTICES, nothing shows that an SMS was sent.
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
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>
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.
(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 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'.
*** Bug 30355 has been marked as a duplicate of this bug. ***
(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.
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>
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>
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!
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for 22.05.07 release
applied to 21.11 for 21.11.14
I guess this depends on bug 18532
Backported: Pushed to 21.05.x branch for the upcoming release (21.05.21) Nothing to document it seems, marking resolved.