Created attachment 103525 [details] [review]
Bug 21781: Fallback to SMS if no email address where email address hardcoded

Extend use of FallbackToSMSIfNoEmail syspref to fallback to SMS if no
patron email address outside of it's current usage sending suggestion
notices.

Test plan for C4/Acquisitions.pm:
1. Input a value into SMSSendDriver syspref
2. Create a patron with sms (under Member messaging preferences) and no email address
3. In Acquistions module make currency, active budget, fund, vendor
4. Create a basket
5. Add to basket. Click to add 'From a new (empty) record'
6. Under 'Patrons' header add a user and search for your patron (added
in #1)
7. Submit new order
8. Receive the order:
'Close this basket'
'Receive shipment'
Submit invoice page
Select 'Receive' link on right hand side of page
Tick checkbox to add 1 quantity of item and Save
9. Revisit your patrons page and in the 'Notices' tab you'll see a
'Order received' notice with type of 'email'
11. Apply patch and restart plack
12. Repeat steps 4-9 and observe the 'Order received' notice has a type
of SMS

Test plan for amendment to circ/pendingreserves.pl (patch has been
applied in 'Test plan for C4/Acquidsitions.pm):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set CanMarkHoldsToPullAsLost  syspref to 'Allow to mark items as lost
and notify the patron'
3. Using the patron from test plan for C4/Acquisitions.pm (i.e. sms alert
number and no email) place a hold on an item currently available
4. Visit Circulation > Holds to pull
5. Select 'Mark lost and notify patron'
6. Return to patron page look in 'Notices' tab and observe 'Hold has
been cancelled' notice has type of 'email'
7. Change 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 3,4,5,6
and observe the new 'Hold has been cancelled' notice has type 'sms'

Test plan for amendment to misc/cronjobs/automatic_renewals.pl (patch
has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Enable automatic renewals in Administration > Circulation and fine
rules
3. Checkout an item to our patron and set due date in the past
4. cd into misc/cronjobs
5. Jump into koha-shell and run automatic_renewals.pl cronjob
sudo koha-shell <instancename>
./automatic_renewals.pl --confirm --send-notices
6. Observe in patron records 'Notices' tab there is a 'Auto renewals'
notice with type 'email'
7. Set 'FallbackToSMSIfNoEmail' syspref to 'Enable' and repeat steps
3,4,5,6 and observe the new 'Auto renewals' notice is type 'sms'

Test plan for amendment to misc/cronjobs/membership_expiry.pl (patch has
been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'MembershipExpiryDaysNotice' to '1'
3. Edit our patrons record setting 'Expiry date' to 1 day in future
4. cd to misc/cronjobs directory
5. Jump into koha-shell and run membership_expiry.pl cronjob:
sudo koha-shell <instancename>
./membership_expiry.pl -c
6. Visit the patron record's 'Notices' tab and observe 'Account
expiration' notice with type 'email'
7. Change 'FallbackToSMSIfNoEmail' syspref to 'Enable'
8. Reset patrons epxiration date to 1 day in future
9. Repeat step 4,5,6 and observe new 'Account expiration' notice is type
'sms'

Test plan for opac/opac-shareshelf.pl (patch has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'OpacAllowSharingPrivateLists' syspref to 'Allow'
3. Ensure OPACBaseURL is set
4. In OPAC log in as the patron with set smsalertnumber and no email
5. Create a private list, select 'Share' link, input email
address and submit
6. Back in staff client make a new patron record
7. Return to OPAC and login as patron from step #5
8. Query database:
select content from message_queue where letter_code='SHARE_INVITE';
9. Click on the link in the message_queue notice
10. OPAC loads
11. Query database again:
select to_address, message_transport_type from
message_queue where letter_code='SHARE_ACCEPT';
12. Observe there is no 'Share on list <listname> accepted' notice
13. Set 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 4,5,7,8,
9,10,11,12 and observe there is a 'Share on list <listname> accepted'
notice with to_address = smsalertnumber and message_transport_type is 'sms'

14. Set 'FallbackToSMSIfNoEmail' to 'disable' and edit the patron with
smsalertnumber set and no email setting an email address and repeat
steps 4,5,7,8,9,10,11,12 and observe there is a 'Share on list
<listname> accepted' notice with the to_address of the email you set and
message_transport_type of 'email'

Sponsored-by: Catalyst IT

Created attachment 103526 [details] [review]
Bug 21781: Fallback to SMS if no email address where email address hardcoded

Extend use of FallbackToSMSIfNoEmail syspref to fallback to SMS if no
patron email address outside of it's current usage sending suggestion
notices.

Test plan for C4/Acquisition.pm:
1. Input a value into SMSSendDriver syspref
2. Create a patron with sms (under Member messaging preferences) and no email address
3. In Acquistions module make currency, active budget, fund, vendor
4. Create a basket
5. Add to basket. Click to add 'From a new (empty) record'
6. Under 'Patrons' header add a user and search for your patron (added
in #1)
7. Submit new order
8. Receive the order:
'Close this basket'
'Receive shipment'
Submit invoice page
Select 'Receive' link on right hand side of page
Tick checkbox to add 1 quantity of item and Save
9. Revisit your patrons page and in the 'Notices' tab you'll see a
'Order received' notice with type of 'email'
11. Apply patch and restart plack
12. Repeat steps 4-9 and observe the 'Order received' notice has a type
of SMS

Test plan for amendment to circ/pendingreserves.pl (patch has been
applied in 'Test plan for C4/Acquisition.pm):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set CanMarkHoldsToPullAsLost  syspref to 'Allow to mark items as lost
and notify the patron'
3. Using the patron from test plan for C4/Acquisitions.pm (i.e. sms alert
number and no email) place a hold on an item currently available
4. Visit Circulation > Holds to pull
5. Select 'Mark lost and notify patron'
6. Return to patron page look in 'Notices' tab and observe 'Hold has
been cancelled' notice has type of 'email'
7. Change 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 3,4,5,6
and observe the new 'Hold has been cancelled' notice has type 'sms'

Test plan for amendment to misc/cronjobs/automatic_renewals.pl (patch
has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Enable automatic renewals in Administration > Circulation and fine
rules
3. Checkout an item to our patron and set due date in the past
4. cd into misc/cronjobs
5. Jump into koha-shell and run automatic_renewals.pl cronjob
sudo koha-shell <instancename>
./automatic_renewals.pl --confirm --send-notices
6. Observe in patron records 'Notices' tab there is a 'Auto renewals'
notice with type 'email'
7. Set 'FallbackToSMSIfNoEmail' syspref to 'Enable' and repeat steps
3,4,5,6 and observe the new 'Auto renewals' notice is type 'sms'

Test plan for amendment to misc/cronjobs/membership_expiry.pl (patch has
been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'MembershipExpiryDaysNotice' to '1'
3. Edit our patrons record setting 'Expiry date' to 1 day in future
4. cd to misc/cronjobs directory
5. Jump into koha-shell and run membership_expiry.pl cronjob:
sudo koha-shell <instancename>
./membership_expiry.pl -c
6. Visit the patron record's 'Notices' tab and observe 'Account
expiration' notice with type 'email'
7. Change 'FallbackToSMSIfNoEmail' syspref to 'Enable'
8. Reset patrons epxiration date to 1 day in future
9. Repeat step 4,5,6 and observe new 'Account expiration' notice is type
'sms'

Test plan for opac/opac-shareshelf.pl (patch has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'OpacAllowSharingPrivateLists' syspref to 'Allow'
3. Ensure OPACBaseURL is set
4. In OPAC log in as the patron with set smsalertnumber and no email
5. Create a private list, select 'Share' link, input email
address and submit
6. Back in staff client make a new patron record
7. Return to OPAC and login as patron from step #5
8. Query database:
select content from message_queue where letter_code='SHARE_INVITE';
9. Click on the link in the message_queue notice
10. OPAC loads
11. Query database again:
select to_address, message_transport_type from
message_queue where letter_code='SHARE_ACCEPT';
12. Observe there is no 'Share on list <listname> accepted' notice
13. Set 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 4,5,7,8,
9,10,11,12 and observe there is a 'Share on list <listname> accepted'
notice with to_address = smsalertnumber and message_transport_type is 'sms'

14. Set 'FallbackToSMSIfNoEmail' to 'disable' and edit the patron with
smsalertnumber set and no email setting an email address and repeat
steps 4,5,7,8,9,10,11,12 and observe there is a 'Share on list
<listname> accepted' notice with the to_address of the email you set and
message_transport_type of 'email'

Sponsored-by: Catalyst IT

Hi all, 

I've attached a patch which extends the use of FallbackToSMSIfNoEmail syspref to the following files which Martin noted:

* opac/opac-shareshelf.pl
* misc/cronjobs/membership_expiry.pl
* misc/cronjobs/automatic_renewals.pl
* circ/pendingreserves.pl
* C4/Acquisition.pm

I've included an individual test plan for each of these files to test the changes. The individual test plans are sequential though, so you can start testing 'Test plan for C4/Acquisition.pm' where you apply the patch and then you keep it applied through all the other test plans. 

The following files I have not included for the following rationale, very happy to discuss if you disagree :)

* opac/svc/checkout_notes -> Sends 'CHECKOUT_NOTE' notice to library inbound email address if AllowCheckoutNotes
* opac/opac-issue-note.pl -> ditto
* opac/opac-memberentry.pl -> Sends 'OPAC_REG_VERIFY' notice to entered email address, defining sms address should be on separate bug report
* opac/opac-shareshelf.pl -> One of 2 hardcoded message_transport_types I have made. Regarding unchanged one this is unchanged as 'SHARE_INVITE' notice sent to inputted sharee email address.
Defining sharee sms number should be different bug report
* circ/article-request-slip.pl -> Hardcoded message_transport_type of 'print' on master
* C4/Reserves.pm -> Sends 'HOLDPLACED' notice to library inbound email address if 'emailLibrarianWhenHoldIsPlaced' syspref is enabled
* misc/cronjobs/stockrotation.pl -> Sends 'SR_SLIP' stock rotation report notice to library inbound email address or KohaAdminEmailAddress 
* Koha/ArticleRequests.pm -> No longer contains notice being queued
* Koha/Patron/Password/Recovery.pm -> Hardcoded message_transport_type for 'PASSWORD_RESET' is requiring email from OpacResetPassword syspref
*  Koha/Account.pm -> Requires email to be hardcoded as only run if UseEmailReceipts is enabled

Thanks,
Alex

*** Bug 21776 has been marked as a duplicate of this bug. ***

Created attachment 105328 [details] [review]
Bug 21781: Fallback to SMS if no email address where email address hardcoded

Extend use of FallbackToSMSIfNoEmail syspref to fallback to SMS if no
patron email address outside of it's current usage sending suggestion
notices.

Test plan for C4/Acquisition.pm:
1. Input a value into SMSSendDriver syspref
2. Create a patron with sms (under Member messaging preferences) and no email address
3. In Acquistions module make currency, active budget, fund, vendor
4. Create a basket
5. Add to basket. Click to add 'From a new (empty) record'
6. Under 'Patrons' header add a user and search for your patron (added
in #1)
7. Submit new order
8. Receive the order:
'Close this basket'
'Receive shipment'
Submit invoice page
Select 'Receive' link on right hand side of page
Tick checkbox to add 1 quantity of item and Save
9. Revisit your patrons page and in the 'Notices' tab you'll see a
'Order received' notice with type of 'email'
11. Apply patch and restart plack
12. Repeat steps 4-9 and observe the 'Order received' notice has a type
of SMS

Test plan for amendment to circ/pendingreserves.pl (patch has been
applied in 'Test plan for C4/Acquisition.pm):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set CanMarkHoldsToPullAsLost  syspref to 'Allow to mark items as lost
and notify the patron'
3. Using the patron from test plan for C4/Acquisitions.pm (i.e. sms alert
number and no email) place a hold on an item currently available
4. Visit Circulation > Holds to pull
5. Select 'Mark lost and notify patron'
6. Return to patron page look in 'Notices' tab and observe 'Hold has
been cancelled' notice has type of 'email'
7. Change 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 3,4,5,6
and observe the new 'Hold has been cancelled' notice has type 'sms'

Test plan for amendment to misc/cronjobs/automatic_renewals.pl (patch
has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Enable automatic renewals in Administration > Circulation and fine
rules
3. Checkout an item to our patron and set due date in the past
4. cd into misc/cronjobs
5. Jump into koha-shell and run automatic_renewals.pl cronjob
sudo koha-shell <instancename>
./automatic_renewals.pl --confirm --send-notices
6. Observe in patron records 'Notices' tab there is a 'Auto renewals'
notice with type 'email'
7. Set 'FallbackToSMSIfNoEmail' syspref to 'Enable' and repeat steps
3,4,5,6 and observe the new 'Auto renewals' notice is type 'sms'

Test plan for amendment to misc/cronjobs/membership_expiry.pl (patch has
been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'MembershipExpiryDaysNotice' to '1'
3. Edit our patrons record setting 'Expiry date' to 1 day in future
4. cd to misc/cronjobs directory
5. Jump into koha-shell and run membership_expiry.pl cronjob:
sudo koha-shell <instancename>
./membership_expiry.pl -c
6. Visit the patron record's 'Notices' tab and observe 'Account
expiration' notice with type 'email'
7. Change 'FallbackToSMSIfNoEmail' syspref to 'Enable'
8. Reset patrons epxiration date to 1 day in future
9. Repeat step 4,5,6 and observe new 'Account expiration' notice is type
'sms'

Test plan for opac/opac-shareshelf.pl (patch has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'OpacAllowSharingPrivateLists' syspref to 'Allow'
3. Ensure OPACBaseURL is set
4. In OPAC log in as the patron with set smsalertnumber and no email
5. Create a private list, select 'Share' link, input email
address and submit
6. Back in staff client make a new patron record
7. Return to OPAC and login as patron from step #5
8. Query database:
select content from message_queue where letter_code='SHARE_INVITE';
9. Click on the link in the message_queue notice
10. OPAC loads
11. Query database again:
select to_address, message_transport_type from
message_queue where letter_code='SHARE_ACCEPT';
12. Observe there is no 'Share on list <listname> accepted' notice
13. Set 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 4,5,7,8,
9,10,11,12 and observe there is a 'Share on list <listname> accepted'
notice with to_address = smsalertnumber and message_transport_type is 'sms'

14. Set 'FallbackToSMSIfNoEmail' to 'disable' and edit the patron with
smsalertnumber set and no email setting an email address and repeat
steps 4,5,7,8,9,10,11,12 and observe there is a 'Share on list
<listname> accepted' notice with the to_address of the email you set and
message_transport_type of 'email'

Sponsored-by: Catalyst IT

Rebased against master.

Created attachment 109559 [details] [review]
Bug 21781: Fallback to SMS if no email address where email address hardcoded

Extend use of FallbackToSMSIfNoEmail syspref to fallback to SMS if no
patron email address outside of it's current usage sending suggestion
notices.

Test plan for C4/Acquisition.pm:
1. Input a value into SMSSendDriver syspref
2. Create a patron with sms (under Member messaging preferences) and no email address
3. In Acquistions module make currency, active budget, fund, vendor
4. Create a basket
5. Add to basket. Click to add 'From a new (empty) record'
6. Under 'Patrons' header add a user and search for your patron (added
in #1)
7. Submit new order
8. Receive the order:
'Close this basket'
'Receive shipment'
Submit invoice page
Select 'Receive' link on right hand side of page
Tick checkbox to add 1 quantity of item and Save
9. Revisit your patrons page and in the 'Notices' tab you'll see a
'Order received' notice with type of 'email'
11. Apply patch and restart plack
12. Repeat steps 4-9 and observe the 'Order received' notice has a type
of SMS

Test plan for amendment to circ/pendingreserves.pl (patch has been
applied in 'Test plan for C4/Acquisition.pm):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set CanMarkHoldsToPullAsLost  syspref to 'Allow to mark items as lost
and notify the patron'
3. Using the patron from test plan for C4/Acquisitions.pm (i.e. sms alert
number and no email) place a hold on an item currently available
4. Visit Circulation > Holds to pull
5. Select 'Mark lost and notify patron'
6. Return to patron page look in 'Notices' tab and observe 'Hold has
been cancelled' notice has type of 'email'
7. Change 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 3,4,5,6
and observe the new 'Hold has been cancelled' notice has type 'sms'

Test plan for amendment to misc/cronjobs/automatic_renewals.pl (patch
has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Enable automatic renewals in Administration > Circulation and fine
rules
3. Checkout an item to our patron and set due date in the past
4. cd into misc/cronjobs
5. Jump into koha-shell and run automatic_renewals.pl cronjob
sudo koha-shell <instancename>
./automatic_renewals.pl --confirm --send-notices
6. Observe in patron records 'Notices' tab there is a 'Auto renewals'
notice with type 'email'
7. Set 'FallbackToSMSIfNoEmail' syspref to 'Enable' and repeat steps
3,4,5,6 and observe the new 'Auto renewals' notice is type 'sms'

Test plan for amendment to misc/cronjobs/membership_expiry.pl (patch has
been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'MembershipExpiryDaysNotice' to '1'
3. Edit our patrons record setting 'Expiry date' to 1 day in future
4. cd to misc/cronjobs directory
5. Jump into koha-shell and run membership_expiry.pl cronjob:
sudo koha-shell <instancename>
./membership_expiry.pl -c
6. Visit the patron record's 'Notices' tab and observe 'Account
expiration' notice with type 'email'
7. Change 'FallbackToSMSIfNoEmail' syspref to 'Enable'
8. Reset patrons epxiration date to 1 day in future
9. Repeat step 4,5,6 and observe new 'Account expiration' notice is type
'sms'

Test plan for opac/opac-shareshelf.pl (patch has been applied already):
1. Set 'FallbackToSMSIfNoEmail' syspref to 'Disable'
2. Set 'OpacAllowSharingPrivateLists' syspref to 'Allow'
3. Ensure OPACBaseURL is set
4. In OPAC log in as the patron with set smsalertnumber and no email
5. Create a private list, select 'Share' link, input email
address and submit
6. Back in staff client make a new patron record
7. Return to OPAC and login as patron from step #5
8. Query database:
select content from message_queue where letter_code='SHARE_INVITE';
9. Click on the link in the message_queue notice
10. OPAC loads
11. Query database again:
select to_address, message_transport_type from
message_queue where letter_code='SHARE_ACCEPT';
12. Observe there is no 'Share on list <listname> accepted' notice
13. Set 'FallbackToSMSIfNoEmail' to 'Enable' and repeat steps 4,5,7,8,
9,10,11,12 and observe there is a 'Share on list <listname> accepted'
notice with to_address = smsalertnumber and message_transport_type is 'sms'

14. Set 'FallbackToSMSIfNoEmail' to 'disable' and edit the patron with
smsalertnumber set and no email setting an email address and repeat
steps 4,5,7,8,9,10,11,12 and observe there is a 'Share on list
<listname> accepted' notice with the to_address of the email you set and
message_transport_type of 'email'

Sponsored-by: Catalyst IT

Rebased against master

Patches still apply cleanly on master.

This may need updating for the new `queue_message` method added with bug 15986.

I like the idea of a migration to using 'queue_message' for all message queuing operations and thus centralising the fallback system for message transport types.

I got a couple of small merge errors here. I managed to apply the patches, but something in pendingreseves is still mad. I get this error when I try to go to Holds to Pull as part of your Test plan for amendment to circ/pendingreserves.pl:


Could not compile /kohadevbox/koha/circ/pendingreserves.pl: Global symbol "$admin_email_address" requires explicit package name (did you forget to declare "my $admin_email_address"?) at /kohadevbox/koha/circ/pendingreserves.pl line 95.
BEGIN not safe after errors--compilation aborted at /kohadevbox/koha/circ/pendingreserves.pl line 315.

Also running into issues with auto-renew notices, probably due to changes made in bug 18532. That added messaging preferences for auto-renew notices. Might that mean we should remove them from this bug? Since now patrons/libraries are more explicitly choosing how they want auto-renews sent?

With the patch applied right now, I have FallbackToSMSIfNoEmail turned off, my patron has an SMS number but no email address, and their messaging prefs say to send auto-renew as an email only. When I run the cron, my patron gets an SMS. So that's not right.

For what it's worth, the acq and membership expiry notices worked as described. I didn't test the list share.

Created attachment 124868 [details] [review]
Bug 21781 - message_transport_type should allow fallbacks

Created attachment 124869 [details] [review]
Bug 21781 - message_transport_type should allow fallbacks

has anyone tested the latest patches here? setting this to needs signoff so we can make sure the patches still don't apply.

(In reply to Liz Rea from comment #15)
> has anyone tested the latest patches here? setting this to needs signoff so
> we can make sure the patches still don't apply.

The patch no longer applies 8-(...

git bz apply 21781

Bug 21781 - message_transport_type should allow fallbacks

124869 - Bug 21781 - message_transport_type should allow fallbacks

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 21781 - message_transport_type should allow fallbacks
Using index info to reconstruct a base tree...
M	C4/Acquisition.pm
M	C4/Suggestions.pm
M	Koha/Patron.pm
M	circ/pendingreserves.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
M	misc/cronjobs/membership_expiry.pl
M	opac/opac-shareshelf.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-shareshelf.pl
Auto-merging misc/cronjobs/membership_expiry.pl
CONFLICT (content): Merge conflict in misc/cronjobs/membership_expiry.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging circ/pendingreserves.pl
Auto-merging Koha/Patron.pm
Auto-merging C4/Suggestions.pm
Auto-merging C4/Acquisition.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 21781 - message_transport_type should allow fallbacks

Created attachment 168199 [details] [review]
Bug 21781 - message_transport_type should allow fallbacks

Created attachment 168200 [details] [review]
Bug 21781: Apply message_transport_type fallbacks to more locations

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 168201 [details] [review]
Bug 21781: (follow-up) Use SMS templates

We shouldn't only be changing the message transport type for the enqueue
function but also for the call to GetPreparedLetter.. otherwise we'll
just be putting what may be an innapropriately formatted for email
notice into an SMS message.

This patch updates all the occurences touched by the patchset to use the
SMS notice template when we're falling back to SMS for transport medium.

We also drop the fallback for the expiry notice, as a bug has since
introduced functionality to send to both email and sms for that notice.

I still think the long term goal should be to further improve Koha::Patron->queue_notice and then use that wherever we can.. but this is an OK intermediate step in my opinion.

Martin, do you think it would be a good idea to have some tests to cover these changes?