Koha-community/Koha
13
7
Fork 0
Code Issues Releases Wiki Activity

Bug 36307: SMS::Send driver errors are not captured and stored

Browse source 
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 <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
(cherry picked from commit 8acdc1e8e9)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
This commit is contained in:
Kyle Hall 2024-03-13 13:32:17 -04:00 committed by Fridolin Somers
parent cd8bee46d9
commit 8c53454ab7
4 changed files with 45 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
C4/Letters.pm
View file

@ -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',
}
);
}

12
C4/SMS.pm
View file

@ -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;

4
koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
View file

@ -107,9 +107,11 @@
[% ELSIF ( QUEUED_MESSAGE.failure_code == 'MISSING_SMS' ) %]<span class="clearfix">Missing SMS number</span>
[% ELSIF ( QUEUED_MESSAGE.failure_code == 'DUPLICATE_MESSAGE' ) %]<span class="clearfix">Message is duplicate</span>
[% ELSIF ( QUEUED_MESSAGE.failure_code == 'NO_NOTES' ) %]<span class="clearfix">No notes from SMS driver</span>
[% ELSIF ( QUEUED_MESSAGE.failure_code == 'SMS_SEND_DRIVER_MISSING' ) %]<span class="clearfix">The SMS driver could not be loaded</span>
[% ELSIF ( QUEUED_MESSAGE.failure_code == 'SENDMAIL' ) %]<span class="clearfix">Unhandled email failure, check the logs for further details</span>
[% ELSIF ( QUEUED_MESSAGE.failure_code == "UNKNOWN_ERROR" ) %]<span class="clearfix">Unknown error</span>
[% ELSE %]<span class="clearfix">Error occurred while sending email</span>
[% ELSE %]
<span class="clearfix">Message failed to send with the following error: [% QUEUED_MESSAGE.failure_code | html %]</span>
[% END %]
[% END %]
[% IF ( QUEUED_MESSAGE.status == 'sent' ) %]

56
t/SMS.t
View file

@ -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, $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 = C4::SMS->send_sms({
destination => '+33123456789',
message => 'my message',
driver => 'Test',
});
( $send_sms, $error ) = C4::SMS->send_sms(
{
destination => '+33123456789',
message => 'my message',
driver => 'Test',
}
);
is( $send_sms, 1, 'send_sms returns 1' );