Bug 15967 ensured that an email notice that falls back to print when the patron has no email will send a print notice using the email template if there is no print template. When SMS and fallback were added this means that an SMS message where there is no SMS template will use the email template - it shouldn't as SMS messages need to be shorter, we should instead fail sending SMS if no SMS template defined
Created attachment 91907 [details] [review] Bug 23411 - SMS messages should not fall back to 'email' To test: 1 - Have a patron wiht an sms number, but no email 2 - Enable sms by setting SMSDriver syspref to Email 3 - Set Notice triggers to send an overdue via sms and email 4 - Ensure the selected notice is defined only for 'email' 5 - Checkout an overdue to the patron above You can set a specify a due date in the past 6 - perl misc/cronjobs/overdue_notices.pl -t 7 - Check the patron notices tab, they have a print notice and an SMS notice pending 8 - The SMS notice used the 'email' template 9 - Apply patch 10 - sudo koha-mysql kohadev 11 - DELETE * FROM message_queue WHERE borrowernumber={borrower as above}; 12 - Run the cron again 13 - Patron should have a print notice queued, no sms 14 - Define an SMS notice 15 - delete the message queue 16 - Run again 17 - The patron should have a print and an SMS
Created attachment 99511 [details] [review] Bug 23411 - SMS messages should not fall back to 'email' To test: 1 - Have a patron wiht an sms number, but no email 2 - Enable sms by setting SMSDriver syspref to Email 3 - Set Notice triggers to send an overdue via sms and email 4 - Ensure the selected notice is defined only for 'email' 5 - Checkout an overdue to the patron above You can set a specify a due date in the past 6 - perl misc/cronjobs/overdue_notices.pl -t 7 - Check the patron notices tab, they have a print notice and an SMS notice pending 8 - The SMS notice used the 'email' template 9 - Apply patch 10 - sudo koha-mysql kohadev 11 - DELETE * FROM message_queue WHERE borrowernumber={borrower as above}; 12 - Run the cron again 13 - Patron should have a print notice queued, no sms 14 - Define an SMS notice 15 - delete the message queue 16 - Run again 17 - The patron should have a print and an SMS Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nick, the test plan is considering SMS, but the code is for any MTT. What about the test plan from bug 15967?
(In reply to Jonathan Druart from comment #3) > Nick, the test plan is considering SMS, but the code is for any MTT. The only time $effective_mtt and $mtt should be different are for email/sms - for email nothing will change, only SMS is affected. Future types could be affected by this code, however, I think it is the correct behaviour > > What about the test plan from bug 15967? This does not invalidate that - there we fellback to email if we were sending a print but didn't have a print notice. We continue to do that, but only change falling back to SMS when sending SMS via print if there is no print notice. The issue was that sometimes we would send an email notice via SMS if we didn't have an SMS notice defined - that causes problems and can generate multiple messages etc. If sending SMS and no SMS notice is defined, then we should not send.
Sorry but I am not convince by the code. If what we want is to not send a SMS notice if there is not SMS template, why are not we written that explicitly in the code?
This patch makes a very small change in the case that we cannot send our notice directly because the patron doesn't have a valid address to send to. Current logic: If we don't have an email address, send a print notice - if we can't find a print template use the email template If we don't have an SMS number, send a print notice - if we can't find a print template use the email template New logic: If we don't have an email address, send a print notice - if we can't find a print template use the email template If we don't have an SMS number, send a print notice - if we can't find a print template use the SMS template (In reply to Jonathan Druart from comment #5) > If what we want is to not send a SMS notice if there is not SMS template, > why are not we written that explicitly in the code? If I set a notice to have a print version and no email version, and I trigger it as an email, no message is sent. We get an error: No circulation CHECKOUT letter transported by email at /kohadevbox/koha/C4/Letters.pm line 608. Why do we not do the same for SMS? This code is not changing that anyways, it simply changes that if we try to send an SMS but cannot and are going to send a 'print' then we should use the SMS template if there is no print template. This affects the @output_chunks of the code - the notice that gets sent to libraries saying "These messages were not sent directly to the patron"
Created attachment 101091 [details] [review] Bug 23411: Do not fall back to 'email' for SMS messages To test: 1 - Have a patron wiht an sms number, but no email 2 - Enable sms by setting SMSDriver syspref to Email 3 - Set Notice triggers to send an overdue via sms and email 4 - Ensure the selected notice is defined only for 'email' 5 - Checkout an overdue to the patron above You can set a specify a due date in the past 6 - perl misc/cronjobs/overdue_notices.pl -t 7 - Check the patron notices tab, they have a print notice and an SMS notice pending 8 - The SMS notice used the 'email' template 9 - Apply patch 10 - sudo koha-mysql kohadev 11 - DELETE * FROM message_queue WHERE borrowernumber={borrower as above}; 12 - Run the cron again 13 - Patron should have a print notice queued, no sms 14 - Define an SMS notice 15 - delete the message queue 16 - Run again 17 - The patron should have a print and an SMS Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I still have the feeling that something is not 100% correct here, but cannot find what, so passing QA.
Created attachment 101092 [details] [review] Bug 23411: Do not fall back to 'email' for SMS messages To test: 1 - Have a patron wiht an sms number, but no email 2 - Enable sms by setting SMSDriver syspref to Email 3 - Set Notice triggers to send an overdue via sms and email 4 - Ensure the selected notice is defined only for 'email' 5 - Checkout an overdue to the patron above You can set a specify a due date in the past 6 - perl misc/cronjobs/overdue_notices.pl -t 7 - Check the patron notices tab, they have a print notice and an SMS notice pending 8 - The SMS notice used the 'email' template 9 - Apply patch 10 - sudo koha-mysql kohadev 11 - DELETE * FROM message_queue WHERE borrowernumber={borrower as above}; 12 - Run the cron again 13 - Patron should have a print notice queued, no sms 14 - Define an SMS notice 15 - delete the message queue 16 - Run again 17 - The patron should have a print and an SMS Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x for 19.11.05
backported to 19.05.x for 19.05.10
Minor/normal will not be backported to 18.11.x series