From cb68b3bced03cf9c2cf04217c0ba19b12dada254 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 13 Sep 2021 10:18:46 +0100 Subject: [PATCH] Bug 28803: (follow-up) Error details improvement This patch updates the exceptions thrown by Koha::Email to include the parameter that failed email validation and then updates the failure code to include this parameter and finally display this field in the template. --- C4/Letters.pm | 24 +++++++++++++------ Koha/Email.pm | 18 +++++++++----- .../prog/en/modules/members/notices.tt | 2 +- t/db_dependent/Letters.t | 10 ++++---- 4 files changed, 35 insertions(+), 19 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index 07a47380b1..a02c0d51a9 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -1406,13 +1406,23 @@ sub _send_message_by_email { ); } catch { - _set_message_status( - { - message_id => $message->{'message_id'}, - status => 'failed', - failure_code => 'INVALID_EMAIL' - } - ); + if ( ref($_) eq 'Koha::Exceptions::BadParameter' ) { + _set_message_status( + { + message_id => $message->{'message_id'}, + status => 'failed', + failure_code => "INVALID_EMAIL:".$_->parameter + } + ); + } else { + _set_message_status( + { + message_id => $message->{'message_id'}, + status => 'failed', + failure_code => 'UNKNOWN_ERROR' + } + ); + } return 0; }; return unless $email; diff --git a/Koha/Email.pm b/Koha/Email.pm index 4806126217..cac63763ab 100644 --- a/Koha/Email.pm +++ b/Koha/Email.pm @@ -77,8 +77,10 @@ sub create { my $args = {}; $args->{from} = $params->{from} || C4::Context->preference('KohaAdminEmailAddress'); - Koha::Exceptions::BadParameter->throw("Invalid 'from' parameter: ".$args->{from}) - unless Koha::Email->is_valid($args->{from}); # from is mandatory + Koha::Exceptions::BadParameter->throw( + error => "Invalid 'from' parameter: " . $args->{from}, + parameter => 'from' + ) unless Koha::Email->is_valid( $args->{from} ); # from is mandatory $args->{subject} = $params->{subject} // ''; @@ -89,8 +91,10 @@ sub create { $args->{to} = $params->{to}; } - Koha::Exceptions::BadParameter->throw("Invalid 'to' parameter: ".$args->{to}) - unless Koha::Email->is_valid($args->{to}); # to is mandatory + Koha::Exceptions::BadParameter->throw( + error => "Invalid 'to' parameter: " . $args->{to}, + parameter => 'to' + ) unless Koha::Email->is_valid( $args->{to} ); # to is mandatory my $addresses = {}; $addresses->{reply_to} = $params->{reply_to}; @@ -110,9 +114,11 @@ sub create { foreach my $address ( keys %{$addresses} ) { Koha::Exceptions::BadParameter->throw( - "Invalid '$address' parameter: " . $addresses->{$address} ) + error => "Invalid '$address' parameter: " . $addresses->{$address}, + parameter => $address + ) if $addresses->{$address} - and !Koha::Email->is_valid($addresses->{$address}); + and !Koha::Email->is_valid( $addresses->{$address} ); } $args->{cc} = $addresses->{cc} 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 5503654b66..f9d504226e 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt @@ -95,7 +95,7 @@ [% IF ( QUEUED_MESSAGE.failure_code ) %] [% IF ( QUEUED_MESSAGE.failure_code == "INVALID_BORNUMBER" ) %]Invalid borrowernumber [% borrowernumber | html %] [% ELSIF ( QUEUED_MESSAGE.failure_code == 'NO_EMAIL' ) %]Unable to find an email address for this borrower - [% ELSIF ( QUEUED_MESSAGE.failure_code == "INVALID_EMAIL" ) %]Invalid email address found [% borrowernumber | html %] + [% ELSIF (matches = QUEUED_MESSAGE.failure_code.match('INVALID_EMAIL:(\w+)') ) %]Invalid [% matches.0 | html %] email address found [% borrowernumber | html %] [% ELSIF ( QUEUED_MESSAGE.failure_code == 'NO_FROM' ) %]Missing from email address [% ELSIF ( QUEUED_MESSAGE.failure_code == 'MISSING_SMS' ) %]Missing SMS number [% ELSIF ( QUEUED_MESSAGE.failure_code == 'DUPLICATE_MESSAGE' ) %]Message is duplicate diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index f6621d2bb6..9a37118503 100755 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -949,16 +949,16 @@ subtest 'Test message_id parameter for SendQueuedMessages' => sub { 'borrowernumber' => $borrowernumber, 'to_address' => 'to@example.org', 'message_transport_type' => 'email', - 'from_address' => 'root@localhost.' # invalid KohaAdminEmailAddress + 'from_address' => '@example.com' # invalid from_address }; my $message_id = C4::Letters::EnqueueLetter($my_message); my $processed = C4::Letters::SendQueuedMessages( { message_id => $message_id } ); is( $processed, 1, 'Processed 1 message when one message_id passed' ); my $message_1 = C4::Letters::GetMessage($message_id); - is( $message_1->{status}, 'failed', 'Invalid KohaAdminEmailAddress => status failed' ); - is( $message_1->{failure_code}, 'INVALID_EMAIL', 'Failure code set correctly for invalid email parameter'); + is( $message_1->{status}, 'failed', 'Invalid from_address => status failed' ); + is( $message_1->{failure_code}, 'INVALID_EMAIL:from', 'Failure code set correctly for invalid email parameter'); - $my_message->{from_address} = 'root@example.org'; # valid KohaAdminEmailAddress + $my_message->{from_address} = 'root@example.org'; # valid from_address $message_id = C4::Letters::EnqueueLetter($my_message); warning_like { C4::Letters::SendQueuedMessages( { message_id => $message_id } ); } qr|Fake send_or_die|, @@ -966,5 +966,5 @@ subtest 'Test message_id parameter for SendQueuedMessages' => sub { $message_1 = C4::Letters::GetMessage($message_1->{message_id}); my $message_2 = C4::Letters::GetMessage($message_id); is( $message_1->{status}, 'failed', 'Message 1 status is unchanged' ); - is( $message_2->{status}, 'sent', 'Valid KohaAdminEmailAddress => status sent' ); + is( $message_2->{status}, 'sent', 'Valid from_address => status sent' ); }; -- 2.20.1