Bugzilla – Attachment 87527 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: Automatically resend SMS notices that failed due to network problems
Bug-14791-Automatically-resend-SMS-notices-that-fa.patch (text/plain), 8.62 KB, created by
Lari Taskula
on 2019-04-08 15:16:15 UTC
(
hide
)
Description:
Bug 14791: Automatically resend SMS notices that failed due to network problems
Filename:
MIME Type:
Creator:
Lari Taskula
Created:
2019-04-08 15:16:15 UTC
Size:
8.62 KB
patch
obsolete
>From 82d993e89b51c875687d63e0c27ecad8a91e5189 Mon Sep 17 00:00:00 2001 >From: Lari Taskula <lari.taskula@hypernova.fi> >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 <http://www.gnu.org/licenses>. > > 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
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