From efef24e504d56fbb36265cc19a0a5a18dfbc40af Mon Sep 17 00:00:00 2001 From: Lari Taskula 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 Signed-off-by: Josef Moravec --- 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