Attachment 63895 Details for Bug 17499 – Bug 17499: (follow-up) Bugfix for validation, 2

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

Bug-17499-follow-up-Bugfix-for-validation-2.patch (text/plain), 11.21 KB, created by Lari Taskula on 2017-06-01 11:56:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lari Taskula

Created: 2017-06-01 11:56:32 UTC

Size: 11.21 KB

patch

obsolete

>From 84b124c76092747f22fbb1fd218776770a5ce2a5 Mon Sep 17 00:00:00 2001
>From: Lari Taskula <lari.taskula@jns.fi>
>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().
>
>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/*
>---
> 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.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