From efef24e504d56fbb36265cc19a0a5a18dfbc40af Mon Sep 17 00:00:00 2001
From: Lari Taskula <lari.taskula@jns.fi>
Date: Mon, 29 May 2017 15:02:07 +0300
Subject: [PATCH] [SIGNED-OFF] Bug 17499: (follow-up) Bugfix for validation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Bugfix for days_in_advance and wants_digest validation. Have to check definedness
  of value instead the value itself.
- Fixes broken tests caused by above update.
- Adding a missing unit test for wants_digest validation
- Minor edits to exception messages.
- Throwing exception for invalid message_attribute_id. Incl. unit test

Tested again, prove -v t/db_dependent/Koha/Patron/Message/Preferences.t turns green
prove -v t/db_dependent/Koha/Patron/Message/* turns green
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
---
 Koha/Patron/Message/Preference.pm                  | 25 ++++++----
 t/db_dependent/Koha/Patron/Message/Preferences.t   | 55 ++++++++++++++++++++--
 .../Koha/Patron/Message/Transport/Preferences.t    |  7 +++
 3 files changed, 73 insertions(+), 14 deletions(-)

diff --git a/Koha/Patron/Message/Preference.pm b/Koha/Patron/Message/Preference.pm
index 1df44af..a938446 100644
--- a/Koha/Patron/Message/Preference.pm
+++ b/Koha/Patron/Message/Preference.pm
@@ -325,35 +325,40 @@ sub validate {
         }
     }
 
-    my $attr;
-    if ($self->days_in_advance || $self->wants_digest) {
-        $attr = Koha::Patron::Message::Attributes->find(
-            $self->message_attribute_id
+    my $attr = Koha::Patron::Message::Attributes->find(
+        $self->message_attribute_id
+    );
+    unless ($attr) {
+        Koha::Exceptions::BadParameter->throw(
+            error => 'Message attribute with id '.$self->message_attribute_id
+            .' not found',
+            parameter => 'message_attribute_id'
         );
     }
-    if ($self->days_in_advance) {
+    if (defined $self->days_in_advance) {
         if ($attr && $attr->takes_days == 0) {
             Koha::Exceptions::BadParameter->throw(
                 error => 'days_in_advance cannot be defined for '.
-                $attr->message_name . ' .',
+                $attr->message_name . '.',
                 parameter => 'days_in_advance',
             );
         }
         elsif ($self->days_in_advance < 0 || $self->days_in_advance > 30) {
             Koha::Exceptions::BadParameter->throw(
                 error => 'days_in_advance has to be a value between 0-30 for '.
-                $attr->message_name . ' .',
+                $attr->message_name . '.',
                 parameter => 'days_in_advance',
             );
         }
     }
-    if ($self->wants_digest) {
+    if (defined $self->wants_digest) {
         my $transports = Koha::Patron::Message::Transports->search({
             message_attribute_id => $self->message_attribute_id,
-            is_digest            => 1,
+            is_digest            => $self->wants_digest,
         });
         Koha::Exceptions::BadParameter->throw(
-            error => 'Digest not available for '.$attr->message_name.' .',
+            error => (!$self->wants_digest ? 'No d' : 'D').'igest not available '.
+            'for '.$attr->message_name.'.',
             parameter => 'wants_digest',
         ) if $transports->count == 0;
     }
diff --git a/t/db_dependent/Koha/Patron/Message/Preferences.t b/t/db_dependent/Koha/Patron/Message/Preferences.t
index 8f5626a..702936d 100644
--- a/t/db_dependent/Koha/Patron/Message/Preferences.t
+++ b/t/db_dependent/Koha/Patron/Message/Preferences.t
@@ -47,6 +47,15 @@ subtest 'Test Koha::Patron::Message::Preferences' => sub {
     $schema->storage->txn_begin;
 
     my $attribute = build_a_test_attribute();
+    my $letter = build_a_test_letter();
+    my $mtt = build_a_test_transport_type();
+    Koha::Patron::Message::Transport->new({
+        message_attribute_id   => $attribute->message_attribute_id,
+        message_transport_type => $mtt->message_transport_type,
+        is_digest              => 0,
+        letter_module          => $letter->module,
+        letter_code            => $letter->code,
+    })->store;
 
     subtest 'Test for a patron' => sub {
         plan tests => 3;
@@ -369,7 +378,7 @@ subtest 'Test adding a new preference with invalid parameters' => sub {
     };
 
     subtest 'Bad parameter' => sub {
-        plan tests => 13;
+        plan tests => 19;
 
         $schema->storage->txn_begin;
 
@@ -417,7 +426,7 @@ subtest 'Test adding a new preference with invalid parameters' => sub {
             .' was the days_in_advance.');
 
         eval { Koha::Patron::Message::Preference->new({
-                borrowernumber => $patron->{'borrowernumber'},
+                borrowernumber => $patron->borrowernumber,
                 message_transport_types => ['nonexistent']
             })->store };
         is (ref $@, 'Koha::Exceptions::BadParameter',
@@ -427,7 +436,7 @@ subtest 'Test adding a new preference with invalid parameters' => sub {
             .' was the message_transport_type.');
         eval {
             Koha::Patron::Message::Preference->new({
-                borrowernumber => $patron->{'borrowernumber'},
+                borrowernumber => $patron->borrowernumber,
                 message_attribute_id => $attribute->message_attribute_id,
                 message_transport_types => ['sms'],
                 wants_digest => 1,
@@ -440,6 +449,35 @@ subtest 'Test adding a new preference with invalid parameters' => sub {
         like ($@->error, qr/^Message transport option/, 'Exception s because of given'
             .' message_transport_type is not a valid option.');
 
+        eval {
+            Koha::Patron::Message::Preference->new({
+                borrowernumber => $patron->borrowernumber,
+                message_attribute_id => $attribute->message_attribute_id,
+                message_transport_types => [],
+                wants_digest => 1,
+            })->store };
+        is (ref $@, 'Koha::Exceptions::BadParameter',
+            'Adding a message preference with invalid message_transport_type'
+            .' => Koha::Exceptions::BadParameter');
+        is ($@->parameter, 'wants_digest', 'The previous exception tells us it'
+            .' was the wants_digest');
+        like ($@->error, qr/^Digest not available/, 'Exception s because of given'
+            .' digest is not available for this transport.');
+
+        eval {
+            Koha::Patron::Message::Preference->new({
+                borrowernumber => $patron->borrowernumber,
+                message_attribute_id => -1,
+                message_transport_types => [],
+            })->store };
+        is (ref $@, 'Koha::Exceptions::BadParameter',
+            'Adding a message preference with invalid message_transport_type'
+            .' => Koha::Exceptions::BadParameter');
+        is ($@->parameter, 'message_attribute_id', 'The previous exception tells'
+            .' us it was the message_attribute_id');
+        like ($@->error, qr/^Message attribute with id -1 not found/, 'Exception '
+            .' is because of given message attribute id is not found.');
+
         $schema->storage->txn_rollback;
     };
 
@@ -449,6 +487,15 @@ subtest 'Test adding a new preference with invalid parameters' => sub {
         $schema->storage->txn_begin;
 
         my $attribute = build_a_test_attribute();
+        my $letter = build_a_test_letter();
+        my $mtt = build_a_test_transport_type();
+        Koha::Patron::Message::Transport->new({
+            message_attribute_id   => $attribute->message_attribute_id,
+            message_transport_type => $mtt->message_transport_type,
+            is_digest              => 0,
+            letter_module          => $letter->module,
+            letter_code            => $letter->code,
+        })->store;
         my $patron    = build_a_test_patron();
         my $preference = Koha::Patron::Message::Preference->new({
             borrowernumber => $patron->borrowernumber,
@@ -570,7 +617,7 @@ sub build_a_test_category_preference {
         message_attribute_id => $attr->message_attribute_id,
         wants_digest         => $params->{digest} ? 1 : 0,
         days_in_advance      => $params->{days_in_advance}
-                                 ? $params->{days_in_advance} : 0,
+                                 ? $params->{days_in_advance} : undef,
     })->store;
 
     Koha::Patron::Message::Transport::Preference->new({
diff --git a/t/db_dependent/Koha/Patron/Message/Transport/Preferences.t b/t/db_dependent/Koha/Patron/Message/Transport/Preferences.t
index b77415d..0cdf809 100644
--- a/t/db_dependent/Koha/Patron/Message/Transport/Preferences.t
+++ b/t/db_dependent/Koha/Patron/Message/Transport/Preferences.t
@@ -52,6 +52,13 @@ subtest 'Test Koha::Patron::Message::Transport::Preferences' => sub {
     my $letter    = build_a_test_letter({
         mtt => $mtt->message_transport_type
     });
+    Koha::Patron::Message::Transport->new({
+        message_attribute_id   => $attribute->message_attribute_id,
+        message_transport_type => $mtt->message_transport_type,
+        is_digest              => 0,
+        letter_module          => $letter->module,
+        letter_code            => $letter->code,
+    })->store;
 
     subtest 'For a patron' => sub {
         my $patron    = build_a_test_patron();
-- 
2.1.4