Bug 18478 - Some notices sent via SMS gateway fail
Summary: Some notices sent via SMS gateway fail
Status: Pushed to Stable
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: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 18620
Blocks: 18990
  Show dependency treegraph
 
Reported: 2017-04-21 17:02 UTC by Jessie Zairo
Modified: 2017-08-21 15:02 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18478 - Hold Notices sent via SMS gateway fail (2.11 KB, patch)
2017-05-16 15:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18478 - Unit tests (3.08 KB, patch)
2017-05-16 15:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18478 - Some notices sent via SMS gateway fail (2.47 KB, patch)
2017-05-17 17:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18478 - Unit tests (1.62 KB, patch)
2017-05-17 17:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18478 - Additional unit tests (3.27 KB, patch)
2017-05-17 17:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18478 - QA Followup (1.48 KB, patch)
2017-05-23 20:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18478 - QA Followup (2.37 KB, patch)
2017-05-23 20:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18478 - Some notices sent via SMS gateway fail (2.56 KB, patch)
2017-05-26 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18478 - Unit tests (1.72 KB, patch)
2017-05-26 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18478 - Additional unit tests (3.37 KB, patch)
2017-05-26 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18478 - QA Followup (2.47 KB, patch)
2017-05-26 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18620: [QA Follow-up] Move the final rollback to the very end (1.65 KB, patch)
2017-05-26 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[16.11.x Rebase] Bug 18478 - Some notices sent via SMS gateway fail (2.54 KB, patch)
2017-06-05 18:45 UTC, Nick Clemens
Details | Diff | Splinter Review
[16.11.x rebase] Bug 18478 - Unit tests (2.14 KB, patch)
2017-06-05 18:45 UTC, Nick Clemens
Details | Diff | Splinter Review
[16.11.x rebase] Bug 18478 - QA Followup (1.24 KB, patch)
2017-06-05 18:45 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Zairo 2017-04-21 17:02:41 UTC Comment hidden (obsolete)
Comment 1 Kyle M Hall 2017-05-10 17:36:12 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2017-05-10 17:55:57 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2017-05-16 15:02:33 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2017-05-16 15:02:36 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2017-05-17 17:02:15 UTC
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
Comment 6 Nick Clemens 2017-05-17 17:02:19 UTC
Created attachment 63505 [details] [review]
Bug 18478 - Unit tests
Comment 7 Nick Clemens 2017-05-17 17:02:22 UTC
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
Comment 8 Jonathan Druart 2017-05-18 14:57:57 UTC
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.
Comment 9 Jonathan Druart 2017-05-18 15:05:55 UTC
This:

  $message->{to_address} = $message->{to_address} ? $message->{to_address} : $member->{'smsalertnumber'};

is actually:

  $message->{to_address} ||= $member->{smsalertnumber};
Comment 10 Jonathan Druart 2017-05-18 15:07:39 UTC
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.
Comment 11 Nick Clemens 2017-05-23 20:11:56 UTC
Created attachment 63674 [details] [review]
Bug 18478 - QA Followup

Make sure to build necessary letters
Comment 12 Nick Clemens 2017-05-23 20:14:32 UTC
Created attachment 63675 [details] [review]
Bug 18478 - QA Followup

Make sure to build necessary letters
Fix awkward construction
Comment 13 Nick Clemens 2017-05-23 20:15:20 UTC
(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
Comment 14 Marcel de Rooy 2017-05-26 08:59:53 UTC
(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.
Comment 15 Marcel de Rooy 2017-05-26 09:34:11 UTC
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>
Comment 16 Marcel de Rooy 2017-05-26 09:34:16 UTC
Created attachment 63735 [details] [review]
Bug 18478 - Unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2017-05-26 09:34:20 UTC
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>
Comment 18 Marcel de Rooy 2017-05-26 09:34:24 UTC
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>
Comment 19 Marcel de Rooy 2017-05-26 09:34:28 UTC
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>
Comment 20 Kyle M Hall 2017-05-29 02:18:03 UTC
Pushed to master for 17.05, thanks Nick, Marcel!
Comment 21 Katrin Fischer 2017-06-05 12:35:28 UTC
Is this needed in 16.11.x?
Comment 22 Nick Clemens 2017-06-05 12:41:18 UTC
(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
Comment 23 Katrin Fischer 2017-06-05 13:36:13 UTC
Nick, could you provide rebased patches for 16.11.x please? Conflicts with the tests mostly.
Comment 24 Nick Clemens 2017-06-05 18:45:01 UTC
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>
Comment 25 Nick Clemens 2017-06-05 18:45:05 UTC
Created attachment 63984 [details] [review]
[16.11.x rebase] Bug 18478 - Unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Nick Clemens 2017-06-05 18:45:08 UTC
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>
Comment 27 Nick Clemens 2017-06-05 18:45:51 UTC
rebased, removed changes to reserves.t as they had dependencies and removed use of build_object
Comment 28 Katrin Fischer 2017-06-05 20:18:56 UTC
Thx Nick!

These patches have been pushed to 16.11.x and will be in 16.11.09.
Comment 29 Mason James 2017-07-31 11:13:11 UTC
Pushed to 16.05.x, for 16.05.15 release