From 82d993e89b51c875687d63e0c27ecad8a91e5189 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Tue, 8 Sep 2015 14:15:22 +0300 Subject: [PATCH] Bug 14791: Automatically resend SMS notices that failed due to network problems Sometimes notices keep failing due to various reasons. One common problem is network connection failures. Because of this, the notices go into 'failed' state. This is very problematic, because we have to not only monitor the failed messages but also resend them manually. The purpose of this patch is to enable more automated way of handling delivery failures. This patch enables us to handle exceptions in SMS messaging. The main idea is to throw an exception from SMS::Send driver in case of a failure. The exception will be caught by C4::SMS and from here it will be forwarded to C4::Letters, where instead of automatically setting the message into 'failed' status, we now can decide what to do with the message. As an example I have caught Koha::Exceptions::ConnectionFailed in C4::Letters::_send_message_by_sms(). When we catch the said exception, we simply leave the message in 'pending' status. This way it will be resent whenever process_message_queue.pl is executed again. sub send_sms { #.....your implementation..... # Throw an exception in case of a connection error # $connError can be for example: ($curl->{'retcode'} == 6) # cURL code 6: CURLE_COULDNT_RESOLVE_HOST if ($connError){ Koha::Exceptions::ConnectionFailed->throw(error => "Connection failed"); } } To test: 1. prove t/db_dependent/Letters.t Also testable with your own implementation of SMSSendDriver. 1. Have/create some pending sms messages into message_queue 2. Go to Patrons -> Notices 3. Observe that the your message is in pending status 4. Run misc/cronjob/process_message_queue.pl 5. Observe that the message is no longer pending 6. Add Koha::Exceptions::ConnectionFailed->throw(error => "Connection failed"); to your SMS::Send driver send_sms. 7. Set your message back to pending status 8. Run misc/cronjob/process_message_queue.pl 9. Observe that your message is back in pending status 10. Observe it also has the following delivery note: "Connection failed. Attempting to resend." Sponsored-by: Hypernova Oy --- C4/Letters.pm | 42 ++++++++++++++++++++++++++++++++++------ C4/SMS.pm | 21 ++++++++++++-------- Koha/Exceptions.pm | 4 ++++ t/db_dependent/Letters.t | 32 +++++++++++++++++++++++++++++- 4 files changed, 84 insertions(+), 15 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index fc3173a2d0..99b082ce8b 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -26,6 +26,8 @@ use Encode; use Carp; use Template; use Module::Load::Conditional qw(can_load); +use Scalar::Util qw ( blessed ); +use Try::Tiny; use C4::Members; use C4::Members::Attributes qw(GetBorrowerAttributes); @@ -1392,12 +1394,40 @@ sub _send_message_by_sms { return; } - my $success = C4::SMS->send_sms( { destination => $patron->smsalertnumber, - message => $message->{'content'}, - } ); - _set_message_status( { message_id => $message->{'message_id'}, - status => ($success ? 'sent' : 'failed'), - delivery_note => ($success ? '' : 'No notes from SMS driver') } ); + my $success; + try { + $success = C4::SMS->send_sms( + { destination => $patron->smsalertnumber, + message => $message->{'content'}, + } ); + _set_message_status( { message_id => $message->{'message_id'}, + status => ($success ? 'sent' : 'failed'), + delivery_note => ($success ? '' : 'No notes from SMS driver') } ); + + } catch { + if (blessed($_)){ + if ($_->isa('Koha::Exceptions::ConnectionFailed')){ + # Keep the message in pending status but + # add a delivery note explaining what happened + _set_message_status ( { message_id => $message->{'message_id'}, + status => 'pending', + delivery_note => 'Connection failed. Attempting to resend.' } ); + } + else { + # failsafe: if we catch and unknown exception, set message status to failed + _set_message_status( { message_id => $message->{'message_id'}, + status => 'failed', + delivery_note => ref($_) } ); + $_->rethrow(); + } + } + else { + _set_message_status( { message_id => $message->{'message_id'}, + status => 'failed', + delivery_note => 'Unknown exception.' } ); + die $_; + } + }; return $success; } diff --git a/C4/SMS.pm b/C4/SMS.pm index eff72b358c..580ddbbb56 100644 --- a/C4/SMS.pm +++ b/C4/SMS.pm @@ -56,6 +56,8 @@ use warnings; use C4::Context; use File::Spec; +use Try::Tiny; +use Scalar::Util qw ( blessed ); =head1 METHODS @@ -107,7 +109,9 @@ sub send_sms { %args = map { q{_} . $_ => $conf->{$_} } keys %$conf; } - eval { + #We might die because SMS::Send $driver is not defined or the sms-number has a bad format + #Catch those errors and fail the sms-sending gracefully. + try { # Create a sender $sender = SMS::Send->new( $driver, @@ -121,15 +125,16 @@ sub send_sms { to => $params->{destination}, text => $params->{message}, ); + return $sent; + } catch { + if (blessed($_) && $_->can('rethrow')) { + $_->rethrow(); + } + else { + die $_; + } }; - #We might die because SMS::Send $driver is not defined or the sms-number has a bad format - #Catch those errors and fail the sms-sending gracefully. - if ($@) { - warn $@; - return; - } - # warn 'failure' unless $sent; return $sent; } diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm index 72a56cc3af..e2f5b6bfd2 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -22,6 +22,10 @@ use Exception::Class ( isa => 'Koha::Exceptions::Exception', description => 'The default value cannot be deleted' }, + 'Koha::Exceptions::ConnectionFailed' => { + isa => 'Koha::Exceptions::Exception', + description => 'Connecting to host failed', + }, 'Koha::Exceptions::MissingParameter' => { isa => 'Koha::Exceptions::Exception', description => 'A required parameter is missing' diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index cf0a342012..0f09f61028 100644 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -18,11 +18,12 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 69; +use Test::More tests => 70; use Test::MockModule; use Test::Warn; use MARC::Record; +use Module::Load::Conditional qw/check_install/; my %mail; my $module = new Test::MockModule('Mail::Sendmail'); @@ -160,6 +161,35 @@ is( $resent, undef, 'ResendMessage should return undef if not message_id given' is($messages->[0]->{delivery_note}, 'Missing SMS number', 'Delivery note for no smsalertnumber correctly set'); +subtest 'Test SMS::Send driver connectivity exception' => sub { + if ( check_install( module => 'SMS::Send::Test') ) { + plan tests => 3; + } else { + plan skip_all => 'SMS::Send not installed'; + } + + + my $sms_send_module = new Test::MockModule('SMS::Send::Test'); + $sms_send_module->mock( + 'send_sms', + sub { + warn "Fake SMS driver"; + Koha::Exceptions::ConnectionFailed->throw(error => "Connection failed"); + } + ); + t::lib::Mocks::mock_preference('SMSSendDriver', 'Test'); + Koha::Patrons->find($borrowernumber)->set({ smsalertnumber => "+1234567890" })->store; + warning_is { $messages_processed = C4::Letters::SendQueuedMessages(); } + "Fake SMS driver", + "SMS sent using the mocked SMS::Send driver subroutine send_sms"; + $messages = C4::Letters::GetQueuedMessages(); + is( $messages->[0]->{status}, 'pending', + 'Message is still pending after SendQueuedMessages() because of network failure (bug 14791)' ); + is( $messages->[0]->{delivery_note}, 'Connection failed. Attempting to resend.', + 'Message has correct delivery note about resending' ); +}; + + # GetLetters my $letters = C4::Letters::GetLetters(); -- 2.17.1