From 1da98a38bd6d0233fe9f4b972842f2cab14c39c9 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::Exception::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. There are multiple other reasons of failure where Exceptions will come in handy. For example the SMS Gateway provider may return some errors at request, and with this patch we will be able to handle them better. Below is a short example for making your SMS::Send driver throw an exception in case of a connection failure in SMS/Send/MyDriver/Driver.pm. _______________________________________________________________ use Koha::Exception::ConnectionFailed; 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::Exception::ConnectionFailed->throw(error => "Connection failed"); } #.....your implementation..... } _______________________________________________________________ prerequisites: -2. Set system preference SMSSendDriver to Example::ExceptionExample -1. Enable system preference EnhancedMessagingPreferences To test: 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. Apply patch 5. Run misc/cronjob/process_message_queue.pl 6. Observe that your message is still in pending status You can also test it with your own implementation of SMSSendDriver. What you need to do is follow the example mentioned earlier to make send_sms() subroutine throw Koha::Exception::ConnectionFailed in case of a connection failure. --- C4/Letters.pm | 68 +++++++++++++++++++++------- C4/SMS.pm | 22 +++++---- SMS/Send/Example/ExceptionExample.pm | 87 ++++++++++++++++++++++++++++++++++++ t/db_dependent/Letters.t | 14 +++++- 4 files changed, 166 insertions(+), 25 deletions(-) create mode 100644 SMS/Send/Example/ExceptionExample.pm diff --git a/C4/Letters.pm b/C4/Letters.pm index a037cda..8dda63b 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::Exception::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/SMS/Send/Example/ExceptionExample.pm b/SMS/Send/Example/ExceptionExample.pm new file mode 100644 index 0000000..34b0849 --- /dev/null +++ b/SMS/Send/Example/ExceptionExample.pm @@ -0,0 +1,87 @@ +package SMS::Send::Example::ExceptionExample; + +=pod + +=head1 NAME + +SMS::Send::Example::ExceptionExample + +=head1 SYNOPSIS + + use Try::Tiny; + use Scalar::Util qw( blessed ); + + # Create a testing sender + my $send = SMS::Send->new( 'Example::ExceptionExample' ); + + # Send a message + try { + $send->send_sms( + text => 'Hi there', + to => '+61 (4) 1234 5678', + ); + } catch { + if (blessed($_) && $_->can('rethrow')){ + # handle exception + } else { + die $_; + } + } + +=head1 DESCRIPTION + + This SMS::Send module provides an example for how + to throw Koha::Exceptions in case of an error. + + Exceptions will be caught outside this module by + try-catch block. + +=cut + +use strict; +use SMS::Send::Driver (); +use Koha::Exception::ConnectionFailed; + +use vars qw{$VERSION @ISA}; +BEGIN { + $VERSION = '0.06'; + @ISA = 'SMS::Send::Driver'; +} + + + + + +##################################################################### +# Constructor + +sub new { + my $class = shift; + + my $self = bless {}, $class; + + $self->{_login} = "ned"; + $self->{_password} = "flanders"; + + return $self; +} + +sub send_sms { + # ... + # ... our imaginary cURL implementation of sending sms messages to gateway + # $curl = sendMessageWithcURL("http://url.com/send", { + # destination => $params->{to}, + # text => $params->{text} + # }); + # my $errorCode = $curl->{'retcode'}; + # Using cURL, our request produced error code 6, CURLE_COULDNT_RESOLVE_HOST + my $errorCode = 6; + + if ($errorCode == 6) { + Koha::Exception::ConnectionFailed->throw(error => "Connection failed"); + } + + return 1; +} + +1; diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 91111ac..2b2d7e4 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; @@ -153,6 +153,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', 'Example::ExceptionExample'); +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