From f32b9bfeac66a79b4fda1a1d6cd7edc105af6ffc Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Tue, 8 Sep 2015 14:15:22 +0300 Subject: [PATCH] Bug 14791: Resend failed notices - Add Koha::Exceptions to SMS::Send drivers Sometimes notices keep failing due to various reasons. One common problem is network connection failures. Because of this, the notices go into 'failed' status without another attempt for sending. 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 move us into 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 do as we wish with various exceptions. 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." --- C4/Letters.pm | 68 ++++++++++++++++++++++++++++++++++++------------ C4/SMS.pm | 22 ++++++++++------ Koha/Exceptions.pm | 4 +++ t/db_dependent/Letters.t | 23 +++++++++++++++- 4 files changed, 92 insertions(+), 25 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index 340c99c..8826f4e 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -38,6 +38,9 @@ use Koha::SMS::Providers; use Koha::Email; use Koha::DateUtils qw( format_sqldatetime dt_from_string ); +use Scalar::Util qw ( blessed ); +use Try::Tiny; + use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); BEGIN { @@ -1044,18 +1047,23 @@ sub SendQueuedMessages { if $params->{'verbose'} or $debug; # This is just begging for subclassing next MESSAGE if ( lc($message->{'message_transport_type'}) eq 'rss' ); - if ( lc( $message->{'message_transport_type'} ) eq 'email' ) { - _send_message_by_email( $message, $params->{'username'}, $params->{'password'}, $params->{'method'} ); - } - elsif ( lc( $message->{'message_transport_type'} ) eq 'sms' ) { - if ( C4::Context->preference('SMSSendDriver') eq 'Email' ) { - my $member = C4::Members::GetMember( 'borrowernumber' => $message->{'borrowernumber'} ); - my $sms_provider = Koha::SMS::Providers->find( $member->{'sms_provider_id'} ); - $message->{to_address} .= '@' . $sms_provider->domain(); + eval { + if ( lc( $message->{'message_transport_type'} ) eq 'email' ) { _send_message_by_email( $message, $params->{'username'}, $params->{'password'}, $params->{'method'} ); - } else { - _send_message_by_sms( $message ); } + elsif ( lc( $message->{'message_transport_type'} ) eq 'sms' ) { + if ( C4::Context->preference('SMSSendDriver') eq 'Email' ) { + my $member = C4::Members::GetMember( 'borrowernumber' => $message->{'borrowernumber'} ); + my $sms_provider = Koha::SMS::Providers->find( $member->{'sms_provider_id'} ); + $message->{to_address} .= '@' . $sms_provider->domain(); + _send_message_by_email( $message, $params->{'username'}, $params->{'password'}, $params->{'method'} ); + } else { + _send_message_by_sms( $message ); + } + } + }; + if ($@) { + warn $@; } } return scalar( @$unsent_messages ); @@ -1416,12 +1424,40 @@ sub _send_message_by_sms { return; } - my $success = C4::SMS->send_sms( { destination => $member->{'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 => $member->{'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 => 'Unknown exception.' } ); + $_->rethrow(); + } + } + else { + # failsafe + _set_message_status( { message_id => $message->{'message_id'}, + status => 'failed', + delivery_note => 'Unknown non-blessed exception.' } ); + die $_; + } + }; return $success; } diff --git a/C4/SMS.pm b/C4/SMS.pm index c603117..a428d94 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 @@ -105,7 +107,10 @@ 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, @@ -119,15 +124,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 60b9e6a..56762f5 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -8,6 +8,10 @@ use Exception::Class ( 'Koha::Exceptions::Exception' => { description => 'Something went wrong!', }, + 'Koha::Exceptions::ConnectionFailed' => { + isa => 'Koha::Exceptions::Exception', + description => 'Connecting to host failed', + }, 'Koha::Exceptions::DuplicateObject' => { isa => 'Koha::Exceptions::Exception', description => 'Same object already exists', diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 91111ac..78a64ac 100644 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -18,7 +18,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 82; +use Test::More tests => 85; use Test::MockModule; use Test::Warn; @@ -34,6 +34,15 @@ $module->mock( } ); +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"); + } +); + use_ok('C4::Context'); use_ok('C4::Members'); use_ok('C4::Acquisition'); @@ -153,6 +162,18 @@ 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'); +# Test connectivity Exception (Bug 14791) +t::lib::Mocks::mock_preference('SMSSendDriver', 'Test'); +ModMember(borrowernumber => $borrowernumber, smsalertnumber => "+1234567890"); +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.7.4