From c1d06e8773bc39c643689df8ae04cd8dc5a9492c Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Wed, 31 May 2017 15:22:13 +0300 Subject: [PATCH] Bug 17499: (follow-up) Bugfix for validation, 2 Remove duplicate validation for digest availability. This is already checked in $preference->validate(). Also prevents a crash when patron has messaging transport type selected for a message for which it should not be available. This patch logs it with as a warning, and only throws an exception when attempting to set it. To test: 1. prove t/db_dependent/Koha/Patron/Message/* --- Koha/Patron/Message/Preference.pm | 52 ++++++++++++++---------- t/db_dependent/Koha/Patron/Message/Preferences.t | 16 +------- 2 files changed, 31 insertions(+), 37 deletions(-) diff --git a/Koha/Patron/Message/Preference.pm b/Koha/Patron/Message/Preference.pm index a938446..03a91a4 100644 --- a/Koha/Patron/Message/Preference.pm +++ b/Koha/Patron/Message/Preference.pm @@ -198,11 +198,23 @@ sub message_transport_types { if ($self->{'_message_transport_types'}) { return $self->{'_message_transport_types'}; } - map { $self->{'_message_transport_types'}->{$_->message_transport_type} = - Koha::Patron::Message::Transports->find({ - message_attribute_id => $self->message_attribute_id, - message_transport_type => $_->message_transport_type, - is_digest => $self->wants_digest})->letter_code } + map { + my $transport = Koha::Patron::Message::Transports->find({ + message_attribute_id => $self->message_attribute_id, + message_transport_type => $_->message_transport_type, + is_digest => $self->wants_digest + }); + unless ($transport) { + my $logger = Koha::Logger->get; + $logger->warn( + $self->message_name . ' has no transport with '. + $_->message_transport_type . ' (digest: '. + ($self->wants_digest ? 'yes':'no').').' + ); + } + $self->{'_message_transport_types'}->{$_->message_transport_type} + = $transport ? $transport->letter_code : ' '; + } Koha::Patron::Message::Transport::Preferences->search({ borrower_message_preference_id => $self->borrower_message_preference_id, })->as_list; @@ -354,7 +366,7 @@ sub validate { if (defined $self->wants_digest) { my $transports = Koha::Patron::Message::Transports->search({ message_attribute_id => $self->message_attribute_id, - is_digest => $self->wants_digest, + is_digest => $self->wants_digest ? 1 : 0, }); Koha::Exceptions::BadParameter->throw( error => (!$self->wants_digest ? 'No d' : 'D').'igest not available '. @@ -377,11 +389,20 @@ sub _set_message_transport_types { $self->_validate_message_transport_types({ message_transport_types => $types }); foreach my $type (@$types) { unless (exists $self->{'_message_transport_types'}->{$type}) { - $self->{'_message_transport_types'}->{$type} = - Koha::Patron::Message::Transports->find({ + my $transport = Koha::Patron::Message::Transports->find({ message_attribute_id => $self->message_attribute_id, message_transport_type => $type, - is_digest => $self->wants_digest})->letter_code; + is_digest => $self->wants_digest + }); + unless ($transport) { + Koha::Exceptions::ObjectNotFound->throw( + error => 'No transport configured for '. + $type . ' (digest: '. + ($self->wants_digest ? 'yes':'no').').' + ); + } + $self->{'_message_transport_types'}->{$type} + = $transport->letter_code; } } return $self; @@ -405,19 +426,6 @@ sub _validate_message_transport_types { parameter => 'message_transport_type', ); } - my $tmp = (ref($self) eq __PACKAGE__) ? $self->unblessed : $params; - unless (Koha::Patron::Message::Transports->find({ - message_transport_type => $type, - message_attribute_id => $tmp->{'message_attribute_id'}, - is_digest => $tmp->{'wants_digest'}, - })) { - Koha::Exceptions::BadParameter->throw( - error => "Message transport option '$type' (". - ($tmp->{'wants_digest'} ? 'digest':'no digest').") for ". - $self->message_name . 'does not exist', - parameter => 'message_transport_type', - ); - } } return $types; } diff --git a/t/db_dependent/Koha/Patron/Message/Preferences.t b/t/db_dependent/Koha/Patron/Message/Preferences.t index 702936d..0e212a1 100644 --- a/t/db_dependent/Koha/Patron/Message/Preferences.t +++ b/t/db_dependent/Koha/Patron/Message/Preferences.t @@ -378,7 +378,7 @@ subtest 'Test adding a new preference with invalid parameters' => sub { }; subtest 'Bad parameter' => sub { - plan tests => 19; + plan tests => 16; $schema->storage->txn_begin; @@ -434,20 +434,6 @@ subtest 'Test adding a new preference with invalid parameters' => sub { .' => Koha::Exceptions::BadParameter'); is ($@->parameter, 'message_transport_type', 'The previous exception tells us it' .' was the message_transport_type.'); - eval { - Koha::Patron::Message::Preference->new({ - borrowernumber => $patron->borrowernumber, - message_attribute_id => $attribute->message_attribute_id, - message_transport_types => ['sms'], - wants_digest => 1, - })->store }; - is (ref $@, 'Koha::Exceptions::BadParameter', - 'Adding a message preference with invalid message_transport_type' - .' => Koha::Exceptions::BadParameter'); - is ($@->parameter, 'message_transport_type', 'The previous exception tells us it' - .' was the message_transport_type.'); - 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({ -- 2.7.4