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'.
Created attachment 77960 [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
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
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.
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>
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?
(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.
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
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
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.
Maybe FallbackToSMSIfNoEmail ?
Created attachment 82014 [details] [review] Bug 21241: (follow-up) Renamed system preference Sponsored-By: Brimbank Library, Australia
Thanks Katrin great suggestion I have attached the follow-up changing the system preference name to 'FallbackToSMSIfNoEmail'. Ready to test.
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>
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>
Created attachment 82020 [details] [review] Bug 21241: (follow-up) Code Golf Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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
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';
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
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
Looks solid to me.. go for it Nick.. my PQA still stands :)
Awesome work all! Pushed to master for 19.05
Thanks Martin, Nick, Katrin and Owen!
Enhancement, will not be backported to 18.11.x series.