From 8acdc1e8e9b88436204f6d16c9d5abbc775d8943 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Wed, 13 Mar 2024 13:32:17 -0400 Subject: [PATCH] Bug 36307: SMS::Send driver errors are not captured and stored If an SMS::Send driver succeeds, it returns a value that evaluates to true. Every driver I've inspected uses croak when it encounters a failure state. When an SMS message fails to send, code hard codes the failure code to NO_NOTES (No notes from SMS driver). We should store the real error in `failure_code` and display that if the failure code doesn't match a known failure code. Test Plan: 1) Apply this patch 2) Set SMSSendDriver to any value 3) Generate a pending sms message 4) Run the following query: update message_queue set status = 'failed', failure_code = "This is a test"; 5) View the patron's messages, note the delivery note contains the contents of the failure code 6) Run the following query: update message_queue set status = 'failed', failure_code = "SMS_SEND_DRIVER_MISSING"; 7) Reload the patron's messages, not the delivery note is now "The SMS driver could not be loaded". Signed-off-by: David Nind Signed-off-by: Marcel de Rooy Signed-off-by: Katrin Fischer --- C4/Letters.pm | 4 +- C4/SMS.pm | 12 ++-- .../prog/en/modules/members/notices.tt | 4 +- t/SMS.t | 58 +++++++++++-------- 4 files changed, 46 insertions(+), 32 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index d8dcdba7d2..70927cee61 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -1598,7 +1598,7 @@ sub _send_message_by_sms { return; } - my $success = C4::SMS->send_sms( + my ( $success, $error ) = C4::SMS->send_sms( { destination => $patron->smsalertnumber, message => $message->{'content'}, @@ -1619,7 +1619,7 @@ sub _send_message_by_sms { { message_id => $message->{'message_id'}, status => 'failed', - failure_code => 'NO_NOTES' + failure_code => $error || 'NO_NOTES', } ); } diff --git a/C4/SMS.pm b/C4/SMS.pm index ce3358f66a..cb65263f98 100644 --- a/C4/SMS.pm +++ b/C4/SMS.pm @@ -25,8 +25,12 @@ C4::SMS - send SMS messages =head1 SYNOPSIS -my $success = C4::SMS->send_sms({ message => 'This is my text message', - destination => '212-555-1212' }); +my ( $success, $error ) = C4::SMS->send_sms( + { + message => 'This is my text message', + destination => '212-555-1212' + } +); =head1 DESCRIPTION @@ -87,7 +91,7 @@ sub send_sms { # This allows the user to override the driver. See SMS::Send::Test my $driver = exists $params->{'driver'} ? $params->{'driver'} : $self->driver(); - return unless $driver; + return ( undef, 'SMS_SEND_DRIVER_MISSING' ) unless $driver; my ($sent, $sender); @@ -127,7 +131,7 @@ sub send_sms { #Catch those errors and fail the sms-sending gracefully. if ($@) { warn $@; - return; + return ( undef, $@ ); } # warn 'failure' unless $sent; return $sent; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt index 7b2efaa703..d9e5e569e7 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt @@ -108,9 +108,11 @@ [% ELSIF ( QUEUED_MESSAGE.failure_code == 'MISSING_SMS' ) %]Missing SMS number [% ELSIF ( QUEUED_MESSAGE.failure_code == 'DUPLICATE_MESSAGE' ) %]Message is duplicate [% ELSIF ( QUEUED_MESSAGE.failure_code == 'NO_NOTES' ) %]No notes from SMS driver + [% ELSIF ( QUEUED_MESSAGE.failure_code == 'SMS_SEND_DRIVER_MISSING' ) %]The SMS driver could not be loaded [% ELSIF ( QUEUED_MESSAGE.failure_code == 'SENDMAIL' ) %]Unhandled email failure, check the logs for further details [% ELSIF ( QUEUED_MESSAGE.failure_code == "UNKNOWN_ERROR" ) %]Unknown error - [% ELSE %]Error occurred while sending email + [% ELSE %] + Message failed to send with the following error: [% QUEUED_MESSAGE.failure_code | html %] [% END %] [% END %] [% IF ( QUEUED_MESSAGE.status == 'sent' ) %] diff --git a/t/SMS.t b/t/SMS.t index bf8de65152..763251a713 100755 --- a/t/SMS.t +++ b/t/SMS.t @@ -19,44 +19,52 @@ use Modern::Perl; use t::lib::Mocks; -use Test::More tests => 7; +use Test::More tests => 8; BEGIN { - use_ok('C4::SMS', qw( driver send_sms )); + use_ok( 'C4::SMS', qw( driver send_sms ) ); } - my $driver = 'my mock driver'; -t::lib::Mocks::mock_preference('SMSSendDriver', $driver); +t::lib::Mocks::mock_preference( 'SMSSendDriver', $driver ); is( C4::SMS->driver(), $driver, 'driver returns the SMSSendDriver correctly' ); -t::lib::Mocks::mock_preference('SMSSendUsername', 'username'); -t::lib::Mocks::mock_preference('SMSSendPassword', 'pwd'); +t::lib::Mocks::mock_preference( 'SMSSendUsername', 'username' ); +t::lib::Mocks::mock_preference( 'SMSSendPassword', 'pwd' ); -my $send_sms = C4::SMS->send_sms(); +my ( $send_sms, $error ) = C4::SMS->send_sms(); is( $send_sms, undef, 'send_sms without arguments returns undef' ); -$send_sms = C4::SMS->send_sms({ - destination => 'my destination', -}); +($send_sms) = C4::SMS->send_sms( + { + destination => 'my destination', + } +); is( $send_sms, undef, 'send_sms without message returns undef' ); -$send_sms = C4::SMS->send_sms({ - message => 'my message', -}); +($send_sms) = C4::SMS->send_sms( + { + message => 'my message', + } +); is( $send_sms, undef, 'send_sms without destination returns undef' ); -$send_sms = C4::SMS->send_sms({ - destination => 'my destination', - message => 'my message', - driver => '', -}); -is( $send_sms, undef, 'send_sms with an undef driver returns undef' ); - -$send_sms = C4::SMS->send_sms({ - destination => '+33123456789', - message => 'my message', - driver => 'Test', -}); +( $send_sms, $error ) = C4::SMS->send_sms( + { + destination => 'my destination', + message => 'my message', + driver => '', + } +); +is( $send_sms, undef, 'send_sms with an undef driver returns undef' ); +is( $error, 'SMS_SEND_DRIVER_MISSING', 'Error code returned is SMS_SEND_DRIVER_MISSING' ); + +( $send_sms, $error ) = C4::SMS->send_sms( + { + destination => '+33123456789', + message => 'my message', + driver => 'Test', + } +); is( $send_sms, 1, 'send_sms returns 1' ); -- 2.39.5