Attachment 119489 Details for Bug 17499 – Bug 17499: (follow-up) Contact information vs. mtt validation

[patch] Bug 17499: (follow-up) Contact information vs. mtt validation

Bug-17499-follow-up-Contact-information-vs-mtt-val.patch (text/plain), 10.78 KB, created by Michal Denar on 2021-04-12 19:51:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Michal Denar

Created: 2021-04-12 19:51:46 UTC

Size: 10.78 KB

patch

obsolete

>From f0e0388f273d0b5e20f5bf145df8721a3bb0677d 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
>
>Signed-off-by: Michal Denar <black23@gmail.com>
>---
> Koha/Patron/Message/Preference.pm                | 95 +++++++++++++-----------
> t/db_dependent/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.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 17499: 63427 | 63428 | 63429 | 63430 | 63431 | 63432 | 63433 | 63434 | 63786 | 63787 | 63788 | 63789 | 63790 | 63791 | 63792 | 63793 | 63801 | 63802 | 63803 | 63804 | 63805 | 63806 | 63807 | 63808 | 63809 | 63810 | 63811 | 63812 | 63878 | 63895 | 64035 | 64036 | 64037 | 64038 | 64039 | 64040 | 64041 | 64042 | 64043 | 64044 | 64795 | 66686 | 69434 | 69435 | 69458 | 91864 | 93863 | 93865 | 93872 | 94748 | 99479 | 99634 | 99668 | 99669 | 99670 | 103441 | 103442 | 103443 | 106653 | 106654 | 106655 | 106656 | 106657 | 107612 | 107613 | 107614 | 107615 | 107616 | 113010 | 113011 | 113127 | 113129 | 113686 | 113687 | 113765 | 113766 | 113767 | 113768 | 118452 | 118453 | 119470 | 119471 | 119472 | 119473 | 119474 | 119475 | 119476 | 119477 | 119478 | 119479 | 119480 | 119481 | 119482 | 119483 | 119484 | 119485 | 119486 | 119487 | 119488 | 119489 | 119490 | 119491 | 141408 | 141409 | 141410 | 141411 | 141412 | 141413 | 141414 | 141415 | 141416 | 141417 | 141418 | 146529 | 146530 | 146531 | 146532 | 146533 | 146534 | 146535 | 146536 | 146537 | 146538 | 146539 | 146540 | 151900 | 151901 | 151902 | 151903 | 151904 | 151905 | 151906 | 151907 | 151908 | 151909 | 151910 | 151911 | 151937 | 151938 | 151939 | 151940 | 151941 | 151942 | 151943 | 151944 | 151945 | 151946 | 151947 | 151948 | 155365 | 155366 | 155367 | 155368 | 155369 | 155370 | 155371 | 155372 | 155373 | 155374 | 155375 | 155376