From 3e062f515a46e2bb86efe3faf275469fb4b3a7d8 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Wed, 31 May 2017 15:22:13 +0300 Subject: [PATCH] [SIGNED-OFF] Bug 17499: (follow-up) Bugfix for validation, 2 Remove duplicate validation for digest availability. This is already checked in $preference->validate(). Set values for current object before validating messaging transport types, not the other way around as it was before this patch. 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/* Signed-off-by: Josef Moravec --- Koha/Patron/Message/Preference.pm | 66 +++++++++++--------- t/db_dependent/Koha/Patron/Message/Preferences.t | 76 +++++++++++++++++++++--- 2 files changed, 105 insertions(+), 37 deletions(-) diff --git a/Koha/Patron/Message/Preference.pm b/Koha/Patron/Message/Preference.pm index a938446..6fb7fb8 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; @@ -228,12 +240,15 @@ Sets preference object values and additionally message_transport_types if given. sub set { my ($self, $params) = @_; - if ($params && $params->{'message_transport_types'}) { - $self->message_transport_types($params->{'message_transport_types'}); - delete $params->{'message_transport_types'}; + my $mtt = $params->{'message_transport_types'}; + delete $params->{'message_transport_types'}; + + $self->SUPER::set($params) if $params; + if ($mtt) { + $self->message_transport_types($mtt); } - return $self->SUPER::set($params) if $params; + return $self; } =head3 store @@ -354,7 +369,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 +392,19 @@ 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; + message_transport_type => $type + }); + unless ($transport) { + Koha::Exceptions::BadParameter->throw( + error => 'No transport configured for '.$self->message_name. + " transport type $type.", + parameter => 'message_transport_types' + ); + } + $self->{'_message_transport_types'}->{$type} + = $transport->letter_code; } } return $self; @@ -402,20 +425,7 @@ sub _validate_message_transport_types { })) { Koha::Exceptions::BadParameter->throw( error => "Message transport type '$type' does not exist", - 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', + parameter => 'message_transport_types', ); } } diff --git a/t/db_dependent/Koha/Patron/Message/Preferences.t b/t/db_dependent/Koha/Patron/Message/Preferences.t index 702936d..c38545e 100644 --- a/t/db_dependent/Koha/Patron/Message/Preferences.t +++ b/t/db_dependent/Koha/Patron/Message/Preferences.t @@ -24,6 +24,8 @@ use Test::More tests => 7; use t::lib::Mocks; use t::lib::TestBuilder; +use C4::Context; + use Koha::Notice::Templates; use Koha::Patron::Categories; use Koha::Patron::Message::Attributes; @@ -31,6 +33,9 @@ use Koha::Patron::Message::Transport::Types; use Koha::Patron::Message::Transports; use Koha::Patrons; +use File::Temp qw/tempfile/; +use Log::Log4perl; + my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; @@ -189,7 +194,7 @@ subtest 'Test Koha::Patron::Message::Preference->message_transport_types' => sub 'Method message_transport_types available'); subtest 'get message_transport_types' => sub { - plan tests => 4; + plan tests => 5; $schema->storage->txn_begin; @@ -229,6 +234,50 @@ subtest 'Test Koha::Patron::Message::Preference->message_transport_types' => sub ok(!$preference->message_transport_types->{'nonexistent'}, 'Didn\'t find nonexistent transport type.'); + subtest 'test logging of warnings by invalid message transport type' => sub { + plan tests => 2; + + my $log = mytempfile(); + my $conf = mytempfile( <<"HERE" +log4perl.logger.opac = WARN, OPAC +log4perl.appender.OPAC=Log::Log4perl::Appender::TestBuffer +log4perl.appender.OPAC.filename=$log +log4perl.appender.OPAC.mode=append +log4perl.appender.OPAC.layout=SimpleLayout +log4perl.logger.intranet = WARN, INTRANET +log4perl.appender.INTRANET=Log::Log4perl::Appender::TestBuffer +log4perl.appender.INTRANET.filename=$log +log4perl.appender.INTRANET.mode=append +log4perl.appender.INTRANET.layout=SimpleLayout +HERE + ); + t::lib::Mocks::mock_config('log4perl_conf', $conf); + my $appenders = Log::Log4perl->appenders; + my $appender = Log::Log4perl->appenders->{OPAC}; + + my $pref = Koha::Patron::Message::Preferences->find( + $preference->borrower_message_preference_id + ); + my $transports = $pref->message_transport_types; + is($appender, undef, 'Nothing in buffer yet'); + + my $mtt_new = build_a_test_transport_type(); + Koha::Patron::Message::Transport::Preference->new({ + borrower_message_preference_id => + $pref->borrower_message_preference_id, + message_transport_type => $mtt_new->message_transport_type, + })->store; + $pref = Koha::Patron::Message::Preferences->find( + $pref->borrower_message_preference_id + ); + $transports = $pref->message_transport_types; + $appender = Log::Log4perl->appenders->{OPAC}; + my $name = $pref->message_name; + my $tt = $mtt_new->message_transport_type; + like($appender->buffer, qr/WARN - $name has no transport with $tt/, + 'Logged invalid message transport type'); + }; + $schema->storage->txn_rollback; }; @@ -432,22 +481,24 @@ subtest 'Test adding a new preference with invalid parameters' => sub { 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.'); + is ($@->parameter, 'message_transport_types', 'The previous exception ' + .'tells us it was the message_transport_types.'); + + my $mtt_new = build_a_test_transport_type(); eval { Koha::Patron::Message::Preference->new({ borrowernumber => $patron->borrowernumber, message_attribute_id => $attribute->message_attribute_id, - message_transport_types => ['sms'], + message_transport_types => [$mtt_new->message_transport_type], 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.'); + .' => Koha::Exceptions::BadParameter'); + is ($@->parameter, 'message_transport_types', 'The previous exception ' + .'tells us it was the message_transport_types.'); + like ($@->error, qr/^No transport configured/, 'Exception is because of ' + .'given message_transport_type is not a valid option.'); eval { Koha::Patron::Message::Preference->new({ @@ -643,4 +694,11 @@ sub build_a_test_complete_preference { })->next, $mtt1, $mtt2); } +sub mytempfile { + my ( $fh, $fn ) = tempfile( SUFFIX => '.logger.test', UNLINK => 1 ); + print $fh $_[0]//''; + close $fh; + return $fn; +} + 1; -- 2.1.4