|
Description
Jessie Zairo
2017-04-21 17:02:41 UTC
Comment hidden (obsolete)
Appears to be a configuration issue and not a bug. Oops, confirmed Created attachment 63487 [details] [review] Bug 18478 - Hold Notices sent via SMS gateway fail It seems that for HOLD notices we rely on C4::Letters::SendQueuedMessages to populate the correct address. The sms code in this sub, however, assumed the smsalertnumebr is populated. Ths patch changes C4::Reserves::_koha_notify_reserve to use C4::Message::enqueue for SMS and email messages in order to ensure the correct addrress is populated To test: 1 - Define a messaging prefs for a patron to recieve hold notices via SMS 2 - Ensure you have defined an SMS message for 'HOLD' letter 3 - Set an SMS alert number for patron 4 - Set the SMS::Send driver to 'Email' 5 - Fill a hold for the patron 6 - Check the db and note the address is null 7 - Apply patch 8 - Fill another hold 9 - Check db and note address is populated 10 - Repeat for email notices and see the same change Created attachment 63488 [details] [review] Bug 18478 - Unit tests To test: 1 - Apply this patch first 2 - Prove t/db_dependent/Reserves.t 3 - Last tests fail 4 - Apply other patch 5 - All tests should pass Created attachment 63504 [details] [review] Bug 18478 - Some notices sent via SMS gateway fail It seems that for HOLD and DUE (and maybe more) notices we rely on C4::Letters::SendQueuedMessages to populate the correct address. This patch adjust that subroutine to correctly populate the field and/or fail messages if no SMS provider available To test: 1 - Define a messaging prefs for a patron to recieve hold notices via SMS 2 - Ensure you have defined an SMS message for 'HOLD' letter 3 - Set an SMS alert number for patron 4 - Set the SMS::Send driver to 'Email' 5 - Fill a hold for the patron 6 - Check the db and note the address is null 7 - run process_message_queue.pl 8 - Check db - address is null and message pending 9 - Apply patch 10 - run process_message_queue 11 - Message to_address should be populated and message sent Created attachment 63505 [details] [review] Bug 18478 - Unit tests Created attachment 63506 [details] [review] Bug 18478 - Additional unit tests This method was not previously covered, we don't change it , but no reason to throw away these tests to ensure messages are created as expected To test: 1 - Apply this patch first 2 - Prove t/db_dependent/Reserves.t 3 - Last tests fail 4 - Apply other patch 5 - All tests should pass One test fails for me
prove t/db_dependent/Reserves.t
t/db_dependent/Reserves.t .. 55/72 No reserves HOLD letter transported by sms at /home/vagrant/kohaclone/C4/Letters.pm line 697.
Could not find a letter called 'HOLD' for sms in the 'reserves' module at /home/vagrant/kohaclone/C4/Reserves.pm line 1883.
# No tests run!
# Failed test 'No tests run for subtest "_koha_notify_reserve() tests"'
# at t/db_dependent/Reserves.t line 779.
Can't call method "to_address" on an undefined value at t/db_dependent/Reserves.t line 763.
This:
$message->{to_address} = $message->{to_address} ? $message->{to_address} : $member->{'smsalertnumber'};
is actually:
$message->{to_address} ||= $member->{smsalertnumber};
The call to _update_message_to_address just before _send_message_by_email should not be needed, it is already called from _send_message_by_email. Created attachment 63674 [details] [review] Bug 18478 - QA Followup Make sure to build necessary letters Created attachment 63675 [details] [review] Bug 18478 - QA Followup Make sure to build necessary letters Fix awkward construction (In reply to Jonathan Druart from comment #10) > The call to _update_message_to_address just before _send_message_by_email > should not be needed, it is already called from _send_message_by_email. I think it is only called there is the address was blank, we are fixing it before that call so need to update (In reply to Nick Clemens from comment #13) > (In reply to Jonathan Druart from comment #10) > > The call to _update_message_to_address just before _send_message_by_email > > should not be needed, it is already called from _send_message_by_email. > > I think it is only called there is the address was blank, we are fixing it > before that call so need to update Yes, thats what I understand too. Created attachment 63734 [details] [review] Bug 18478 - Some notices sent via SMS gateway fail It seems that for HOLD and DUE (and maybe more) notices we rely on C4::Letters::SendQueuedMessages to populate the correct address. This patch adjust that subroutine to correctly populate the field and/or fail messages if no SMS provider available To test: 1 - Define a messaging prefs for a patron to recieve hold notices via SMS 2 - Ensure you have defined an SMS message for 'HOLD' letter 3 - Set an SMS alert number for patron 4 - Set the SMS::Send driver to 'Email' 5 - Fill a hold for the patron 6 - Check the db and note the address is null 7 - run process_message_queue.pl 8 - Check db - address is null and message pending 9 - Apply patch 10 - run process_message_queue 11 - Message to_address should be populated and message sent Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63735 [details] [review] Bug 18478 - Unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63736 [details] [review] Bug 18478 - Additional unit tests This method was not previously covered, we don't change it , but no reason to throw away these tests to ensure messages are created as expected To test: 1 - Apply this patch first 2 - Prove t/db_dependent/Reserves.t 3 - Last tests fail 4 - Apply other patch 5 - All tests should pass Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63737 [details] [review] Bug 18478 - QA Followup Make sure to build necessary letters Fix awkward construction Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63738 [details] [review] Bug 18620: [QA Follow-up] Move the final rollback to the very end The new subtest in Reserves.t does not need its own transaction. Move the original rollback to the very end of the test. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 17.05, thanks Nick, Marcel! Is this needed in 16.11.x? (In reply to Katrin Fischer from comment #21) > Is this needed in 16.11.x? Yes, I believe so, the unit tests here caused the dependency on 18620 - but also depends on 9021 so problem exists in 16.11 Nick, could you provide rebased patches for 16.11.x please? Conflicts with the tests mostly. Created attachment 63983 [details] [review] [16.11.x Rebase] Bug 18478 - Some notices sent via SMS gateway fail It seems that for HOLD and DUE (and maybe more) notices we rely on C4::Letters::SendQueuedMessages to populate the correct address. This patch adjust that subroutine to correctly populate the field and/or fail messages if no SMS provider available To test: 1 - Define a messaging prefs for a patron to recieve hold notices via SMS 2 - Ensure you have defined an SMS message for 'HOLD' letter 3 - Set an SMS alert number for patron 4 - Set the SMS::Send driver to 'Email' 5 - Fill a hold for the patron 6 - Check the db and note the address is null 7 - run process_message_queue.pl 8 - Check db - address is null and message pending 9 - Apply patch 10 - run process_message_queue 11 - Message to_address should be populated and message sent Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63984 [details] [review] [16.11.x rebase] Bug 18478 - Unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63985 [details] [review] [16.11.x rebase] Bug 18478 - QA Followup Make sure to build necessary letters Fix awkward construction Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> rebased, removed changes to reserves.t as they had dependencies and removed use of build_object Thx Nick! These patches have been pushed to 16.11.x and will be in 16.11.09. Pushed to 16.05.x, for 16.05.15 release |