Bug 23411 - SMS messages sent as print should not fall back to 'email'
Summary: SMS messages sent as print should not fall back to 'email'
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 15967
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-01 14:43 UTC by Nick Clemens
Modified: 2020-04-19 23:58 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10


Attachments
Bug 23411 - SMS messages should not fall back to 'email' (1.95 KB, patch)
2019-08-01 15:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23411 - SMS messages should not fall back to 'email' (2.02 KB, patch)
2020-02-24 12:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23411: Do not fall back to 'email' for SMS messages (2.09 KB, patch)
2020-03-20 12:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23411: Do not fall back to 'email' for SMS messages (2.16 KB, patch)
2020-03-20 12:29 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-08-01 14:43:02 UTC
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
Comment 1 Nick Clemens 2019-08-01 15:02:40 UTC
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
Comment 2 Martin Renvoize 2020-02-24 12:49:09 UTC
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>
Comment 3 Jonathan Druart 2020-02-24 14:38:16 UTC
Nick, the test plan is considering SMS, but the code is for any MTT.

What about the test plan from bug 15967?
Comment 4 Nick Clemens 2020-02-24 15:01:12 UTC
(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.
Comment 5 Jonathan Druart 2020-03-18 08:48:16 UTC
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?
Comment 6 Nick Clemens 2020-03-18 10:28:10 UTC
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"
Comment 7 Jonathan Druart 2020-03-20 12:28:48 UTC
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>
Comment 8 Jonathan Druart 2020-03-20 12:29:16 UTC
I still have the feeling that something is not 100% correct here, but cannot find what, so passing QA.
Comment 9 Kyle M Hall 2020-03-20 12:29:50 UTC
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>
Comment 10 Martin Renvoize 2020-03-20 15:43:22 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 11 Joy Nelson 2020-04-03 21:04:13 UTC
backported to 19.11.x for 19.11.05
Comment 12 Lucas Gass 2020-04-13 19:26:10 UTC
backported to 19.05.x for 19.05.10
Comment 13 Hayley Mapley 2020-04-19 23:58:15 UTC
Minor/normal will not be backported to 18.11.x series