From b14ed3b5345f83192ed3d04643124b60daa46254 Mon Sep 17 00:00:00 2001
From: Lari Taskula <lari.taskula@hypernova.fi>
Date: Wed, 18 Nov 2020 02:15:23 +0000
Subject: [PATCH] Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland
---
 Koha/Patron/Message/Preference.pm             | 95 +++++++++++--------
 .../Koha/Patron/Message/Preferences.t         | 82 +++++++++++++++-
 2 files changed, 133 insertions(+), 44 deletions(-)

diff --git a/Koha/Patron/Message/Preference.pm b/Koha/Patron/Message/Preference.pm
index a3ffffa695..4bcf6e7dd5 100644
--- a/Koha/Patron/Message/Preference.pm
+++ b/Koha/Patron/Message/Preference.pm
@@ -229,6 +229,34 @@ sub message_transport_types {
     }
 }
 
+=head3 mtt_deliverable
+
+$preference->mtt_deliverable('sms'[, $borrowernumer]);
+
+Returns true if given message transport type can be used to deliver message to
+patron.
+
+By default, uses the borrowernumber bound to C<$preference>, but this may be
+overridden by providing optional C<$borrowernumber> parameter.
+
+=cut
+
+sub mtt_deliverable {
+    my ( $self, $mtt, $borrowernumber ) = @_;
+
+    $borrowernumber //= $self->borrowernumber;
+
+    return 0 unless ($borrowernumber);
+
+    my $patron = Koha::Patrons->find($self->borrowernumber);
+
+    return (( $mtt eq 'email' and $patron->notice_email_address ) # No email address
+         or ( $mtt eq 'sms'   and $patron->smsalertnumber ) # No SMS number
+         or ( $mtt eq 'itiva' and C4::Context->preference('TalkingTechItivaPhoneNotification') ) # Notice is handled by TalkingTech_itiva_outbound.pl
+         or ( $mtt eq 'phone' and $patron->phone )) # No phone number to call
+    ? 1 : 0;
+}
+
 =head3 set
 
 $preference->set({
@@ -407,48 +435,31 @@ sub _set_message_transport_types {
                 );
             }
             if (defined $self->borrowernumber) {
-                my $patron = Koha::Patrons->find($self->borrowernumber);
-                if ($type eq 'email') {
-                    if ( !$patron->email )
-                    {
-                        Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired->throw(
-                            error => 'Patron has not set email address, '.
-                                     'cannot use email as message transport',
-                            message_name => $self->message_name,
-                            borrowernumber => $self->borrowernumber,
-                        );
-                    }
-                }
-                elsif ($type eq 'itiva') {
-                    if (!C4::Context->preference('TalkingTechItivaPhoneNotification')) {
-                        Koha::Exceptions::Patron::Message::Preference::TalkingTechItivaPhoneNotificationRequired->throw(
-                            error => 'System preference TalkingTechItivaPhoneNotification disabled'.
-                                     'cannot use itiva as message transport',
-                            message_name => $self->message_name,
-                            borrowernumber => $self->borrowernumber,
-                        );
-                    }
-                }
-                elsif ($type eq 'phone') {
-                    if ( !$patron->phone ) {
-                        Koha::Exceptions::Patron::Message::Preference::PhoneNumberRequired->throw(
-                            error => 'Patron has not set phone number'.
-                                     'cannot use phone as message transport',
-                            message_name => $self->message_name,
-                            borrowernumber => $self->borrowernumber,
-                        );
-                    }
-
-                }
-                elsif ($type eq 'sms') {
-                    if ( !$patron->smsalertnumber ){
-                        Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired->throw(
-                            error => 'Patron has not set SMS number'.
-                                     'cannot use sms as message transport',
-                            message_name => $self->message_name,
-                            borrowernumber => $self->borrowernumber,
-                        );
-                    }
+                if ( ! $self->mtt_deliverable($type) ) {
+                    Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired->throw(
+                        error => 'Patron has not set email address, '.
+                                  'cannot use email as message transport',
+                        message_name => $self->message_name,
+                        borrowernumber => $self->borrowernumber,
+                    ) if $type eq 'email';
+                    Koha::Exceptions::Patron::Message::Preference::TalkingTechItivaPhoneNotificationRequired->throw(
+                        error => 'System preference TalkingTechItivaPhoneNotification disabled'.
+                                 'cannot use itiva as message transport',
+                        message_name => $self->message_name,
+                        borrowernumber => $self->borrowernumber,
+                    ) if $type eq 'itiva';
+                    Koha::Exceptions::Patron::Message::Preference::PhoneNumberRequired->throw(
+                        error => 'Patron has not set phone number'.
+                                 'cannot use phone as message transport',
+                        message_name => $self->message_name,
+                        borrowernumber => $self->borrowernumber,
+                    ) if $type eq 'phone';
+                    Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired->throw(
+                        error => 'Patron has not set SMS number'.
+                                 'cannot use sms as message transport',
+                        message_name => $self->message_name,
+                        borrowernumber => $self->borrowernumber,
+                    ) if $type eq 'sms';
                 }
             }
             $self->{'_message_transport_types'}->{$type}
diff --git a/t/db_dependent/Koha/Patron/Message/Preferences.t b/t/db_dependent/Koha/Patron/Message/Preferences.t
index 75cdb600e5..a402ae9c3c 100644
--- a/t/db_dependent/Koha/Patron/Message/Preferences.t
+++ b/t/db_dependent/Koha/Patron/Message/Preferences.t
@@ -19,7 +19,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 7;
+use Test::More tests => 8;
 
 use t::lib::Mocks;
 use t::lib::TestBuilder;
@@ -373,7 +373,7 @@ HERE
             is_digest              => 1
         })->store;
         t::lib::Mocks::mock_preference('TalkingTechItivaPhoneNotification', 0);
-        $patron->set({ email => '', phone => '', smsalertnumber => '' })->store;
+        $patron->set({ email => '', emailpro => '', B_email => '', phone => '', smsalertnumber => '' })->store;
         eval {
             $preference->message_transport_types('email')->store;
         };
@@ -473,6 +473,84 @@ subtest 'Test Koha::Patron::Message::Preference->message_name' => sub {
     $schema->storage->txn_rollback;
 };
 
+subtest 'Test Koha::Patron::Message::Preference->mtt_deliverable' => sub {
+    plan tests => 10;
+
+    $schema->storage->txn_begin;
+
+    my $patron      = $builder->build_object({ class => 'Koha::Patrons' });
+    my ($preference, $mtt1, $mtt2) = build_a_test_complete_preference({
+        patron => $patron
+    });
+
+    # Test email and smsalertnumber validation
+    eval { Koha::Patron::Message::Transport::Types->new({
+            message_transport_type => 'email'
+        })->store };
+    eval { Koha::Patron::Message::Transport::Types->new({
+            message_transport_type => 'sms'
+        })->store };
+    eval { Koha::Patron::Message::Transport::Types->new({
+            message_transport_type => 'phone'
+        })->store };
+    eval { Koha::Patron::Message::Transport::Types->new({
+            message_transport_type => 'itiva'
+        })->store };
+    Koha::Patron::Message::Transport->new({
+        message_attribute_id   => $preference->message_attribute_id,
+        message_transport_type => 'email',
+        is_digest              => 1
+    })->store;
+    Koha::Patron::Message::Transport->new({
+        message_attribute_id   => $preference->message_attribute_id,
+        message_transport_type => 'sms',
+        is_digest              => 1
+    })->store;
+    Koha::Patron::Message::Transport->new({
+        message_attribute_id   => $preference->message_attribute_id,
+        message_transport_type => 'phone',
+        is_digest              => 1
+    })->store;
+    Koha::Patron::Message::Transport->new({
+        message_attribute_id   => $preference->message_attribute_id,
+        message_transport_type => 'itiva',
+        is_digest              => 1
+    })->store;
+    $patron->set({
+        email => 'nobody@koha-community.org',
+        emailpro => 'nobody@koha-community.org',
+        B_email => 'nobody@koha-community.org',
+        smsalertnumber => '123',
+        phone => '123',
+    })->store;
+    t::lib::Mocks::mock_preference('TalkingTechItivaPhoneNotification', 1);
+
+    is($preference->mtt_deliverable('email'), 1, 'mtt_deliverable - email');
+    is($preference->mtt_deliverable('itiva'), 1, 'mtt_deliverable - itiva');
+    is($preference->mtt_deliverable('phone'), 1, 'mtt_deliverable - phone');
+    is($preference->mtt_deliverable('sms'), 1, 'mtt_deliverable - sms');
+
+    $patron->set({email => '',})->store;
+    is($preference->mtt_deliverable('email'), 1, 'mtt_deliverable - emailpro');
+
+    $patron->set({emailpro => '',})->store;
+    is($preference->mtt_deliverable('email'), 1, 'mtt_deliverable - B_email');
+
+    $patron->set({B_email => '',})->store;
+    is($preference->mtt_deliverable('email'), 0, 'mtt_deliverable - email false');
+
+    t::lib::Mocks::mock_preference('TalkingTechItivaPhoneNotification', 0);
+    is($preference->mtt_deliverable('itiva'), 0, 'mtt_deliverable - itiva false');
+
+    $patron->set({phone => '',})->store;
+    is($preference->mtt_deliverable('phone'), 0, 'mtt_deliverable - phone false');
+
+    $patron->set({smsalertnumber => '',})->store;
+    is($preference->mtt_deliverable('sms'), 0, 'mtt_deliverable - smsalertnumber false');
+
+    $schema->storage->txn_rollback;
+};
+
 subtest 'Test adding a new preference with invalid parameters' => sub {
     plan tests => 4;
 
-- 
2.25.1