Bug 21241 - Set suggestion notices message_transport_type to sms if syspref is enabled and patron has an smsalertnumber but no email address
Summary: Set suggestion notices message_transport_type to sms if syspref is enabled an...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 21776 21781 22908
  Show dependency treegraph
 
Reported: 2018-08-16 22:50 UTC by Alex Buckley
Modified: 2019-05-31 06:14 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
If the FallbackToSMSIfNoEmail syspref is enabled then when a borrower has no email address but does have a smsalertnumber then suggestion notice message_transport_type is set to sms.
Version(s) released in:


Attachments
Bug 21241 - Conditional to choose appropriate message_transport_type for suggestion notices (2.97 KB, patch)
2018-08-17 01:11 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21241 - Conditional to choose appropriate message_transport_type for suggestion notices (3.01 KB, patch)
2018-09-20 22:33 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21241: Conditional to choose appropriate message_transport_type for suggestion notices (3.06 KB, patch)
2018-10-30 15:08 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21241: (follow-up) Syspref to control fallback to SMS when no email is defined (13.87 KB, patch)
2018-11-06 06:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21241: (follow-up) Renamed system preference (6.83 KB, patch)
2018-11-06 23:04 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21241: Conditional to choose appropriate message_transport_type for suggestion notices (3.13 KB, patch)
2018-11-07 11:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21241: (follow-up) Syspref to control fallback to SMS when no email is defined (13.62 KB, patch)
2018-11-07 11:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21241: (follow-up) Code Golf (1.51 KB, patch)
2018-11-07 11:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21241: (follow-up) Made SQL change idempotent (3.26 KB, patch)
2018-12-27 01:39 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2018-08-16 22:50:38 UTC
Presently the message_transport_type of suggestion module notices is hardcoded to email in C4/Suggestions.pm.

If a patron does not have an email address set, but does have a smsalertnumber set then they will not receieve their suggestion notices. 

This patch introduces a conditional to C4/Suggestions.pm to check if no patron email exists AND a patron smsalertnumber does exist then the message_transport_type of the suggestion notice is set to 'sms'. Otherwise the message_transport_type is set to 'email'.
Comment 1 Alex Buckley 2018-08-17 01:11:56 UTC Comment hidden (obsolete)
Comment 2 Alex Buckley 2018-09-20 22:33:21 UTC
Created attachment 79189 [details] [review]
Bug 21241 - Conditional to choose appropriate message_transport_type for suggestion notices

Introducing a conditional into C4/Suggestions.pm to check if no patron
email exists but a smsalertnumber does exist then the suggestion notice
message_transport_type is set to 'sms', otherwise the suggestion
message_transport_type='email'.

This ensures patrons with no email address still receieve their
suggestion notices because at present the message_transport_type is
hardcoded to 'email'.

Test plan:
1. Chose a patron who has no email address set, but does have a
smsalertnumber set (this value is set in the Patron messaging
preferences section after the SMSSendDriver syspref is set)

2. Log into the OPAC with that user and submit a suggestion

3. In the staff client go to Acquisitions->Suggestions and tick the
suggestion and set its status to 'Accepted'

4. In the database query the message_queue and notice the
message_transport_type of the message is set to 'email' even though the
patron has no email address set.

5. Apply this patch, restart memcached and plack

6. Repeat steps 2,3 with the same patron and query the message_queue
table again and notice this time the message_transport_type = 'sms'

7. Repeat steps 2,3 with a patron with no smsalertnumber or email
address. Query the database and observe the
message_transport_type=email.

If no smsalertnumber is set then the message_transport_type is set to
'email' by default

Sponsored-By: Brimbank Library, Australia
Comment 3 Alex Buckley 2018-10-16 00:21:57 UTC
This is in use by Brimbank library so that acquisitions notices are not attempted to be sent via email to patrons with no defined email address. This would be a useful one to be tested and upstreamed.
Comment 4 Owen Leonard 2018-10-30 15:08:27 UTC
Created attachment 81625 [details] [review]
Bug 21241: Conditional to choose appropriate message_transport_type for suggestion notices

Introducing a conditional into C4/Suggestions.pm to check if no patron
email exists but a smsalertnumber does exist then the suggestion notice
message_transport_type is set to 'sms', otherwise the suggestion
message_transport_type='email'.

This ensures patrons with no email address still receieve their
suggestion notices because at present the message_transport_type is
hardcoded to 'email'.

Test plan:
1. Chose a patron who has no email address set, but does have a
smsalertnumber set (this value is set in the Patron messaging
preferences section after the SMSSendDriver syspref is set)

2. Log into the OPAC with that user and submit a suggestion

3. In the staff client go to Acquisitions->Suggestions and tick the
suggestion and set its status to 'Accepted'

4. In the database query the message_queue and notice the
message_transport_type of the message is set to 'email' even though the
patron has no email address set.

5. Apply this patch, restart memcached and plack

6. Repeat steps 2,3 with the same patron and query the message_queue
table again and notice this time the message_transport_type = 'sms'

7. Repeat steps 2,3 with a patron with no smsalertnumber or email
address. Query the database and observe the
message_transport_type=email.

If no smsalertnumber is set then the message_transport_type is set to
'email' by default

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Martin Renvoize 2018-10-31 12:58:57 UTC
SMS can be costly to libraries.. as such I feel this should be bound to a preference in case the library doesn't want to allow this type of message to be sent via SMS.

I'm also a bit concerned that it's a one-off.. I'm sure there are lots of cases where a fallback sequence could be prescribed and it would be nice to at least record bugs for each of those if not fixing them all at the same time.

Finally.. an alternative could be to shift this notice type into the advanced messaging preferences perhaps?
Comment 6 Katrin Fischer 2018-10-31 16:52:54 UTC
(In reply to Martin Renvoize from comment #5)
> SMS can be costly to libraries.. as such I feel this should be bound to a
> preference in case the library doesn't want to allow this type of message to
> be sent via SMS.
> 
> I'm also a bit concerned that it's a one-off.. I'm sure there are lots of
> cases where a fallback sequence could be prescribed and it would be nice to
> at least record bugs for each of those if not fixing them all at the same
> time.
> 
> Finally.. an alternative could be to shift this notice type into the
> advanced messaging preferences perhaps?

+1 I also think an automatic fallback here could be problematic.
Comment 7 Alex Buckley 2018-11-06 06:08:25 UTC
Created attachment 81975 [details] [review]
Bug 21241: (follow-up) Syspref to control fallback to SMS when no email is defined

This patch adds a new system preference (MessageTransportTypeFallbackToSMSIfNoPatronEmail)
which if enabled Koha will send suggestion notices by SMS if a borrower
has a defined SMSalertnumber and no email.

The use of the syspref prevents automatic fallback to sending suggestion notices as SMS when there's no defined email.

Test plan:
1. Chose a patron who has no email address set, but does have a
smsalertnumber set (this value is set in the Patron messaging
preferences section after the SMSSendDriver syspref is set)

2. Log into the OPAC with that user and submit a suggestion

3. In the staff client go to Acquisitions->Suggestions and tick the
suggestion and set its status to 'Accepted'

4. In the database query the message_queue and notice the
message_transport_type of the message is set to 'email' even though the
patron has no email address set.

5. Apply patches, restart memcached and plack

6. Check the 'MessageTransportTypeFallbackToSMSIfNoPatronEmail' syspref
is disabled

7. Repeat steps 2,3 and observe in the message_queue
table the message_transport_type = 'email'

    i.e. If the syspref is disabled then the message is still sent by email
    to borrowers with defined smsalertnumber and no email address

8. Enable the 'MessageTransportTypeFallbackToSMSIfNoPatronEmail' syspref
and repeat steps 2,3 and notice the
message_transport_type = 'sms'

    i.e. If the syspref is enabled then the message is sent by sms to
    borrowers with defined smsalertnumber and no email address

9. Repeat steps 2,3 with a patron with an email
address and no smsalertnumber trying with the 'MessageTransportTypeFallbackToSMSIfNoPatronEmail' syspref
enabled and disabled and notice in both cases the
message_transport_type = email.

    i.e. If a borrower has an email address defined the suggestion
    notice will always be sent via email

10. Repeat steps 2,3 with a patron with no email or smsalertnumber trying with the 'MessageTransportTypeFallbackToSMSIfNoPatronEmail' syspref enabled and disabled and notice in both cases the message_transport_type = email

    i.e. If the borrower has no smsalertnumber and no email defined then the
    suggestion notice will be sent by 'email'

11. Run t/db_dependent/Suggestions.t

Sponsored-By: Brimbank Libraries, Australia
Comment 8 Alex Buckley 2018-11-06 06:11:25 UTC
Hi Martin and Katrin,

I have finished and attached a follow-up patch which introduces a new syspref to control if notices (in this case suggestion notices) are sent by sms if the borrower does not have an email set. ex

i.e. it replaces the automatic fallback from my first patch. 

Can you please have a test and let me know if this is what you had in mind or if you would like further changes?

I can also go through and make bug reports for all the other instances where the message_transport_type of 'email' is hardcoded, so they can also use this syspref fallback as well if you'd like me to.

Cheers,
Alex
Comment 9 Alex Buckley 2018-11-06 21:12:50 UTC
I realize the syspref name 'MessageTransportTypeFallbackToSMSIfNoPatronEmail' is pretty long, if you would like me to shorten it please let me know the name you would prefer and I can quickly write and attach a follow-up patch changing the name.
Comment 10 Katrin Fischer 2018-11-06 22:43:34 UTC
Maybe FallbackToSMSIfNoEmail ?
Comment 11 Alex Buckley 2018-11-06 23:04:13 UTC
Created attachment 82014 [details] [review]
Bug 21241: (follow-up) Renamed system preference

Sponsored-By: Brimbank Library, Australia
Comment 12 Alex Buckley 2018-11-06 23:05:12 UTC
Thanks Katrin great suggestion I have attached the follow-up changing the system preference name to 'FallbackToSMSIfNoEmail'.

Ready to test.
Comment 13 Martin Renvoize 2018-11-07 11:45:46 UTC
Created attachment 82018 [details] [review]
Bug 21241: Conditional to choose appropriate message_transport_type for suggestion notices

Introducing a conditional into C4/Suggestions.pm to check if no patron
email exists but a smsalertnumber does exist then the suggestion notice
message_transport_type is set to 'sms', otherwise the suggestion
message_transport_type='email'.

This ensures patrons with no email address still receieve their
suggestion notices because at present the message_transport_type is
hardcoded to 'email'.

Test plan:
1. Chose a patron who has no email address set, but does have a
smsalertnumber set (this value is set in the Patron messaging
preferences section after the SMSSendDriver syspref is set)

2. Log into the OPAC with that user and submit a suggestion

3. In the staff client go to Acquisitions->Suggestions and tick the
suggestion and set its status to 'Accepted'

4. In the database query the message_queue and notice the
message_transport_type of the message is set to 'email' even though the
patron has no email address set.

5. Apply this patch, restart memcached and plack

6. Repeat steps 2,3 with the same patron and query the message_queue
table again and notice this time the message_transport_type = 'sms'

7. Repeat steps 2,3 with a patron with no smsalertnumber or email
address. Query the database and observe the
message_transport_type=email.

If no smsalertnumber is set then the message_transport_type is set to
'email' by default

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2018-11-07 11:45:49 UTC
Created attachment 82019 [details] [review]
Bug 21241: (follow-up) Syspref to control fallback to SMS when no email is defined

This patch adds a new system preference (FallbackToSMSIfNoEmail)
which if enabled Koha will send suggestion notices by SMS if a borrower
has a defined SMSalertnumber and no email.

The use of the syspref prevents automatic fallback to sending suggestion notices as SMS when there's no defined email.

Test plan:
1. Chose a patron who has no email address set, but does have a
smsalertnumber set (this value is set in the Patron messaging
preferences section after the SMSSendDriver syspref is set)

2. Log into the OPAC with that user and submit a suggestion

3. In the staff client go to Acquisitions->Suggestions and tick the
suggestion and set its status to 'Accepted'

4. In the database query the message_queue and notice the
message_transport_type of the message is set to 'email' even though the
patron has no email address set.

5. Apply patches, restart memcached and plack

6. Check the 'FallbackToSMSIfNoEmail' syspref
is disabled

7. Repeat steps 2,3 and observe in the message_queue
table the message_transport_type = 'email'

    i.e. If the syspref is disabled then the message is still sent by email
    to borrowers with defined smsalertnumber and no email address

8. Enable the 'FallbackToSMSIfNoEmail' syspref
and repeat steps 2,3 and notice the
message_transport_type = 'sms'

    i.e. If the syspref is enabled then the message is sent by sms to
    borrowers with defined smsalertnumber and no email address

9. Repeat steps 2,3 with a patron with an email
address and no smsalertnumber trying with the 'FallbackToSMSIfNoEmail' syspref
enabled and disabled and notice in both cases the
message_transport_type = email.

    i.e. If a borrower has an email address defined the suggestion
    notice will always be sent via email

10. Repeat steps 2,3 with a patron with no email or smsalertnumber trying with the 'FallbackToSMSIfNoEmail' syspref enabled and disabled and notice in both cases the message_transport_type = email

    i.e. If the borrower has no smsalertnumber and no email defined then the
    suggestion notice will be sent by 'email'

11. Run t/db_dependent/Suggestions.t

Sponsored-By: Brimbank Libraries, Australia
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 21241: (follow-up) Renamed system preference

Sponsored-By: Brimbank Library, Australia
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2018-11-07 11:45:53 UTC
Created attachment 82020 [details] [review]
Bug 21241: (follow-up) Code Golf

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2018-11-07 11:54:22 UTC
I've squashed the syspref rename and done a little code golf on the C4::Suggestions change to try and clarify the intention a little.

QA Scripts are passing, tests are present and passing.. I'm happy to PQA on this.  I'll add a bug series for extending it's use.. be nice to continue on this during the next cycle.. I still feel the optimum approach may be to utilise the advanced messaging preferences a bit more deeply (and perhaps have a syspref for specifying a fallback order rather than just an email -> sms hard coded) but for now I think this solves a problem and shouldn't be held up further.

Passing QA
Comment 17 Nick Clemens 2018-12-26 20:49:55 UTC
Please make the statement idempotent (INSERT IGNORE)
Add the pref to sysprefs.sql for new installs
I think the conditional can be further simplified:

-        my $transport = 'email';
-        if (C4::Context->preference("FallbackToSMSIfNoEmail")) {
-            $transport = ($patron->smsalertnumber) && (!$patron->email) ? 'sms' : 'email';
-        }
+        my $transport = (C4::Context->preference("FallbackToSMSIfNoEmail")) && ($patron->smsalertnumber) && (!$patron->email) ? 'sms' : 'email';
Comment 18 Alex Buckley 2018-12-27 01:39:26 UTC
Created attachment 83517 [details] [review]
Bug 21241: (follow-up) Made SQL change idempotent

Also added the new pref to sysprefs.sql

Also simplified the conditional statement.

Sponsored-By: Brimbank Library, Australia
Comment 19 Alex Buckley 2018-12-27 01:41:24 UTC
Hi Nick, 

I have made those changes in a commit I have just attached to the bug report. 

I will set the status back to 'Passed QA' - not sure if that is the correct status it should be at now. 

Can you please retest?

Thanks, 
Alex
Comment 20 Martin Renvoize 2019-01-09 11:53:04 UTC
Looks solid to me.. go for it Nick.. my PQA still stands :)
Comment 21 Nick Clemens 2019-01-28 14:13:15 UTC
Awesome work all!

Pushed to master for 19.05
Comment 22 Alex Buckley 2019-01-28 20:03:13 UTC
Thanks Martin, Nick, Katrin and Owen!
Comment 23 Martin Renvoize 2019-02-01 12:30:34 UTC
Enhancement, will not be backported to 18.11.x series.