Attachment 164463 Details for Bug 36307 – Bug 36307: SMS::Send driver errors are not captured and stored

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

Bug-36307-SMSSend-driver-errors-are-not-captured-a.patch (text/plain), 7.46 KB, created by Marcel de Rooy on 2024-04-05 09:52:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2024-04-05 09:52:00 UTC

Size: 7.46 KB

patch

obsolete

>From 4c5e71c29b3aa6bfa885b02572bd444935dc3541 Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Wed, 13 Mar 2024 13:32:17 -0400
>Subject: [PATCH] Bug 36307: SMS::Send driver errors are not captured and
> stored
>Content-Type: text/plain; charset=utf-8
>
>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>
>---
> 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 e300c00d51..7a509e646b 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.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 36307: 163116 | 163156 | 164463