Bugzilla – Attachment 62615 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.00 KB, created by
Lari Taskula
on 2017-04-24 16:32:19 UTC
(
hide
)
Description:
Bug 14791: Resend failed notices - Add Koha::Exceptions to SMS::Send drivers
Filename:
MIME Type:
Creator:
Lari Taskula
Created:
2017-04-24 16:32:19 UTC
Size:
12.00 KB
patch
obsolete
>From 1da98a38bd6d0233fe9f4b972842f2cab14c39c9 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 | 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 <http://www.gnu.org/licenses>. > > 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
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