Attachment 63811 Details for Bug 17499 – Bug 17499: (follow-up) Bugfix for validation

[patch] Bug 17499: (follow-up) Bugfix for validation

Bug-17499-follow-up-Bugfix-for-validation.patch (text/plain), 8.90 KB, created by Lari Taskula on 2017-05-29 12:40:24 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lari Taskula

Created: 2017-05-29 12:40:24 UTC

Size: 8.90 KB

patch

obsolete

>From 05cffbce4c4900fcfaaf80686a583b6eafadabc2 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] Bug 17499: (follow-up) Bugfix for validation
>
>- 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
>---
> 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.7.4

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