Bugzilla – Attachment 42698 Details for
Bug 14791
Automatically attempt to resend failed notices
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14791: Resend failed notices - Add Koha::Exceptions to SMS::Send drivers
Bug-14791-Resend-failed-notices---Add-KohaExceptio.patch (text/plain), 12.14 KB, created by
Lari Taskula
on 2015-09-18 10:41:58 UTC
(
hide
)
Description:
Bug 14791: Resend failed notices - Add Koha::Exceptions to SMS::Send drivers
Filename:
MIME Type:
Creator:
Lari Taskula
Created:
2015-09-18 10:41:58 UTC
Size:
12.14 KB
patch
obsolete
>From 27b803096c8cc9c05c0ec98b49e34697f7256baf Mon Sep 17 00:00:00 2001 >From: Lari Taskula <larit@student.uef.fi> >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 | 58 +++++++++++++++++++----- > C4/SMS.pm | 23 ++++++---- > SMS/Send/Example/ExceptionExample.pm | 87 ++++++++++++++++++++++++++++++++++++ > t/db_dependent/Letters.t | 24 +++++++++- > 4 files changed, 171 insertions(+), 21 deletions(-) > create mode 100644 SMS/Send/Example/ExceptionExample.pm > >diff --git a/C4/Letters.pm b/C4/Letters.pm >index 08e73e6..50a4e1c 100644 >--- a/C4/Letters.pm >+++ b/C4/Letters.pm >@@ -35,6 +35,8 @@ use Date::Calc qw( Add_Delta_Days ); > use Encode; > use Carp; > use Koha::Email; >+use Scalar::Util qw( blessed ); >+use Try::Tiny; > > use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); > >@@ -948,6 +950,7 @@ sub SendQueuedMessages { > > my $unsent_messages = _get_unsent_messages(); > MESSAGE: foreach my $message ( @$unsent_messages ) { >+ > # warn Data::Dumper->Dump( [ $message ], [ 'message' ] ); > warn sprintf( 'sending %s message to patron: %s', > $message->{'message_transport_type'}, >@@ -955,11 +958,16 @@ 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' ) { >- _send_message_by_sms( $message ); >+ eval { >+ 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' ) { >+ _send_message_by_sms( $message ); >+ } >+ }; >+ if ($@) { >+ warn $@; > } > } > return scalar( @$unsent_messages ); >@@ -1314,12 +1322,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 1cd67b9..a4ee9c7 100644 >--- a/C4/SMS.pm >+++ b/C4/SMS.pm >@@ -36,6 +36,8 @@ use strict; > use warnings; > > use C4::Context; >+use Try::Tiny; >+use Scalar::Util qw ( blessed ); > > use vars qw( $VERSION ); > >@@ -77,7 +79,10 @@ sub send_sms { > # warn "using driver: $driver to send message to $params->{'destination'}"; > > my ($sent, $sender); >- 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, > _login => C4::Context->preference('SMSSendUsername'), >@@ -88,14 +93,16 @@ sub send_sms { > $sent = $sender->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 d3ef012..ad02c04 100644 >--- a/t/db_dependent/Letters.t >+++ b/t/db_dependent/Letters.t >@@ -18,7 +18,7 @@ > # along with Koha; if not, see <http://www.gnu.org/licenses>. > > use Modern::Perl; >-use Test::More tests => 71; >+use Test::More tests => 74; > use Test::MockModule; > use Test::Warn; > >@@ -33,7 +33,6 @@ $module->mock( > %mail = @_; > } > ); >- > use_ok('C4::Context'); > use_ok('C4::Members'); > use_ok('C4::Branch'); >@@ -46,6 +45,16 @@ use Koha::DateUtils qw( dt_from_string output_pref ); > use Koha::Acquisition::Order; > use Koha::Acquisition::Bookseller; > use Koha::Database; >+use Koha::Exception::ConnectionFailed; >+ >+my $c4sms = new Test::MockModule('C4::SMS'); >+$c4sms->mock( >+ 'driver' => >+ sub { >+ warn "Fake SMS driver"; >+ return "Example::ExceptionExample"; >+ } >+); > > my $dbh = C4::Context->dbh; > >@@ -149,6 +158,17 @@ is( $resent, undef, 'ResendMessage should return undef if not message_id given' > # Delivery notes > is($messages->[0]->{delivery_note}, 'Missing SMS number', 'Delivery note for no smsalertnumber correctly set'); > >+# Test connectivity Exception (Bug 14791) >+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(); > is( @$letters, 0, 'GetLetters returns the correct number of letters' ); >-- >1.9.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 14791
:
42463
|
42465
|
42467
|
42468
|
42697
|
42698
|
62615
|
62622
|
87527