From a0e5f4e0d97eec2c13fb2825869fdacc259b01f2 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Wed, 4 Nov 2020 15:19:22 +0000 Subject: [PATCH] Bug 17499: (follow-up) More explicit exceptions Adds and throws more detailed exceptions. This is useful for APIs and generating translatable errors in GUI. In short, replaces Koha::Exceptions::BadParameter with: Koha::Exceptions::Patron::NotFound Koha::Exceptions::Patron::Category Koha::Exceptions::Patron::Category::NotFound Koha::Exceptions::Patron::Message::Preference Koha::Exceptions::Patron::Message::Preference::AttributeNotFound Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable Koha::Exceptions::Patron::Message::Preference::DigestRequired Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired Koha::Exceptions::Patron::Message::Preference::NoTransportType Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired To test: 1. prove t/db_dependent/Koha/Patron/Message/Preferences.t Sponsored-by: The National Library of Finland Signed-off-by: Michal Denar Signed-off-by: Sam Lau Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- Koha/Exceptions/Patron.pm | 6 +- Koha/Exceptions/Patron/Category.pm | 30 +++++ Koha/Exceptions/Patron/Message/Preference.pm | 67 ++++++++++ Koha/Exceptions/Patron/Message/Transport.pm | 32 +++++ Koha/Patron/Message/Preference.pm | 76 +++++------ .../Koha/Patron/Message/Preferences.t | 118 +++++++++--------- 6 files changed, 231 insertions(+), 98 deletions(-) create mode 100644 Koha/Exceptions/Patron/Category.pm create mode 100644 Koha/Exceptions/Patron/Message/Preference.pm create mode 100644 Koha/Exceptions/Patron/Message/Transport.pm diff --git a/Koha/Exceptions/Patron.pm b/Koha/Exceptions/Patron.pm index 6b607a23f1..7df1702945 100644 --- a/Koha/Exceptions/Patron.pm +++ b/Koha/Exceptions/Patron.pm @@ -32,7 +32,11 @@ use Exception::Class ( isa => 'Koha::Exceptions::Patron', description => "Mandatory extended attribute missing", fields => ['type'] - } + }, + 'Koha::Exceptions::Patron::NotFound' => { + isa => 'Koha::Exceptions::Patron', + description => "Patron not found" + }, ); sub full_message { diff --git a/Koha/Exceptions/Patron/Category.pm b/Koha/Exceptions/Patron/Category.pm new file mode 100644 index 0000000000..3eac4d2ba2 --- /dev/null +++ b/Koha/Exceptions/Patron/Category.pm @@ -0,0 +1,30 @@ +package Koha::Exceptions::Patron::Category; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Exception::Class ( + 'Koha::Exceptions::Patron::Category' => { + description => "Something went wrong!" + }, + 'Koha::Exceptions::Patron::Category::NotFound' => { + isa => 'Koha::Exceptions::Patron::Category', + description => "Patron category not found" + }, +); + +1; diff --git a/Koha/Exceptions/Patron/Message/Preference.pm b/Koha/Exceptions/Patron/Message/Preference.pm new file mode 100644 index 0000000000..23f78029f5 --- /dev/null +++ b/Koha/Exceptions/Patron/Message/Preference.pm @@ -0,0 +1,67 @@ +package Koha::Exceptions::Patron::Message::Preference; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Patron::Message::Preference' => { + description => 'Something went wrong' + }, + 'Koha::Exceptions::Patron::Message::Preference::AttributeNotFound' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Message attribute not found", + fields => ['message_attribute_id'] + }, + 'Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Days in advance is out of range", + fields => ['min','max', 'message_name'] + }, + 'Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Days in advance cannot be selected for this preference", + fields => ['message_name'] + }, + 'Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Digest is not available for this message type", + fields => ['message_name'] + }, + 'Koha::Exceptions::Patron::Message::Preference::DigestRequired' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Digest must be selected for this message type", + fields => ['message_name'] + }, + 'Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Patron has no email address, cannot use email transport type.", + fields => ['message_name', 'borrowernumber' ] + }, + 'Koha::Exceptions::Patron::Message::Preference::NoTransportType' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Transport type not available for this message.", + fields => ['message_name', 'transport_type'] + }, + 'Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired' => { + isa => 'Koha::Exceptions::Patron::Message::Preference', + description => "Patron has no SMS number, cannot use sms transport type.", + fields => ['message_name', 'borrowernumber' ] + }, +); + +1; diff --git a/Koha/Exceptions/Patron/Message/Transport.pm b/Koha/Exceptions/Patron/Message/Transport.pm new file mode 100644 index 0000000000..6a7e00a96e --- /dev/null +++ b/Koha/Exceptions/Patron/Message/Transport.pm @@ -0,0 +1,32 @@ +package Koha::Exceptions::Patron::Message::Transport; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Patron::Message::Transport' => { + description => 'Something went wrong' + }, + 'Koha::Exceptions::Patron::Message::Transport::TypeNotFound' => { + isa => 'Koha::Exceptions::Patron::Message::Transport', + description => "Transport type does not exist.", + fields => ['transport_type'] + }, +); + +1; diff --git a/Koha/Patron/Message/Preference.pm b/Koha/Patron/Message/Preference.pm index 1a1b540582..553085fafe 100644 --- a/Koha/Patron/Message/Preference.pm +++ b/Koha/Patron/Message/Preference.pm @@ -21,6 +21,9 @@ use Modern::Perl; use Koha::Database; use Koha::Exceptions; +use Koha::Exceptions::Patron::Message::Preference; +use Koha::Exceptions::Patron::Message::Transport; +use Koha::Exceptions::Patron::Category; use Koha::Patron::Categories; use Koha::Patron::Message::Attributes; use Koha::Patron::Message::Preferences; @@ -284,16 +287,7 @@ sub store { Makes a basic validation for object. -Throws following exceptions regarding parameters. -- Koha::Exceptions::MissingParameter -- Koha::Exceptions::TooManyParameters -- Koha::Exceptions::BadParameter - -See $_->parameter to identify the parameter causing the exception. - -Throws Koha::Exceptions::DuplicateObject if this preference already exists. - -Returns Koha::Patron::Message::Preference object. +Returns Koha::Patron::Message::Preference object, or throws and exception. =cut @@ -311,15 +305,13 @@ sub validate { ); } if ($self->borrowernumber) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::NotFound->throw( error => 'Patron not found.', - parameter => 'borrowernumber', ) unless Koha::Patrons->find($self->borrowernumber); } if ($self->categorycode) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::Category::NotFound->throw( error => 'Category not found.', - parameter => 'categorycode', ) unless Koha::Patron::Categories->find($self->categorycode); } @@ -341,25 +333,27 @@ sub validate { $self->message_attribute_id ); unless ($attr) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::Message::Preference::AttributeNotFound->throw( error => 'Message attribute with id '.$self->message_attribute_id .' not found', - parameter => 'message_attribute_id' + message_attribute_id => $self->message_attribute_id, ); } if (defined $self->days_in_advance) { if ($attr && $attr->takes_days == 0) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable->throw( error => 'days_in_advance cannot be defined for '. $attr->message_name . '.', - parameter => 'days_in_advance', + message_name => $attr->message_name, ); } elsif ($self->days_in_advance < 0 || $self->days_in_advance > 30) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange->throw( error => 'days_in_advance has to be a value between 0-30 for '. $attr->message_name . '.', - parameter => 'days_in_advance', + message_name => $attr->message_name, + min => 0, + max => 30, ); } } @@ -368,12 +362,19 @@ sub validate { message_attribute_id => $self->message_attribute_id, is_digest => $self->wants_digest ? 1 : 0, }); - Koha::Exceptions::BadParameter->throw( - error => (!$self->wants_digest ? 'Digest must be selected' - : 'Digest cannot be selected') - . ' for '.$attr->message_name.'.', - parameter => 'wants_digest', - ) if $transports->count == 0; + unless ($transports->count) { + if (!$self->wants_digest) { + Koha::Exceptions::Patron::Message::Preference::DigestRequired->throw( + error => 'Digest must be selected for '.$attr->message_name.'.', + message_name => $attr->message_name + ); + } else { + Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable->throw( + error => 'Digest cannot be selected for '.$attr->message_name.'.', + message_name => $attr->message_name + ); + } + } } return $self; @@ -395,10 +396,11 @@ sub _set_message_transport_types { message_transport_type => $type }); unless ($transport) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::Message::Preference::NoTransportType->throw( error => 'No transport configured for '.$self->message_name. " transport type $type.", - parameter => 'message_transport_types' + message_name => $self->message_name, + transport_type => $type, ); } if (defined $self->borrowernumber) { @@ -406,19 +408,21 @@ sub _set_message_transport_types { if ($type eq 'email') { if ( !$patron->email ) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired->throw( error => 'Patron has not set email address, '. 'cannot use email as message transport', - parameter => 'message_transport_types' + message_name => $self->message_name, + borrowernumber => $self->borrowernumber, ); } } elsif ($type eq 'sms') { if ( !$patron->smsalertnumber ){ - Koha::Exceptions::BadParameter->throw( - error => 'Patron has not set sms number, '. - 'cannot set sms as message transport', - parameter => 'message_transport_types' + Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired->throw( + error => 'Patron has not set SMS number'. + 'cannot use sms as message transport', + message_name => $self->message_name, + borrowernumber => $self->borrowernumber, ); } } @@ -443,9 +447,9 @@ sub _validate_message_transport_types { unless (Koha::Patron::Message::Transport::Types->find({ message_transport_type => $type })) { - Koha::Exceptions::BadParameter->throw( + Koha::Exceptions::Patron::Message::Transport::TypeNotFound->throw( error => "Message transport type '$type' does not exist", - parameter => 'message_transport_types', + transport_type => $type, ); } } diff --git a/t/db_dependent/Koha/Patron/Message/Preferences.t b/t/db_dependent/Koha/Patron/Message/Preferences.t index 89273ea234..7bc08103a6 100644 --- a/t/db_dependent/Koha/Patron/Message/Preferences.t +++ b/t/db_dependent/Koha/Patron/Message/Preferences.t @@ -360,23 +360,23 @@ HERE eval { $preference->message_transport_types('email')->store; }; - is (ref $@, 'Koha::Exceptions::BadParameter', + is (ref $@, 'Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired', 'Adding a message preference with invalid message_transport_type' - .' => Koha::Exceptions::BadParameter'); - is ($@->parameter, 'message_transport_types', 'The previous exception ' - .' tells us it was the message_transport_types'); - like ($@->error, qr/^Patron has not set email address/, 'Exception ' - .' is because of patron has not set email address.'); + .' => Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired'); + is ($@->message_name, $preference->message_name, 'The previous exception ' + .' tells us it which message name it was.'); + is ($@->borrowernumber, $patron->borrowernumber, 'The previous exception ' + .' tells us which patron it was.'); eval { $preference->message_transport_types('sms')->store; }; - is (ref $@, 'Koha::Exceptions::BadParameter', + is (ref $@, 'Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired', 'Adding a message preference with invalid message_transport_type' - .' => Koha::Exceptions::BadParameter'); - is ($@->parameter, 'message_transport_types', 'The previous exception ' - .' tells us it was the message_transport_types'); - like ($@->error, qr/^Patron has not set sms number/, 'Exception ' - .' is because of patron has not set sms number.'); + .' => Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired'); + is ($@->message_name, $preference->message_name, 'The previous exception ' + .' tells us it which message name it was.'); + is ($@->borrowernumber, $patron->borrowernumber, 'The previous exception ' + .' tells us which patron it was.'); $schema->storage->txn_rollback; }; @@ -466,27 +466,23 @@ subtest 'Test adding a new preference with invalid parameters' => sub { }; subtest 'Bad parameter' => sub { - plan tests => 22; + plan tests => 18; $schema->storage->txn_begin; eval { Koha::Patron::Message::Preference->new({ borrowernumber => -999, })->store }; - is(ref $@, 'Koha::Exceptions::BadParameter', + is(ref $@, 'Koha::Exceptions::Patron::NotFound', 'Adding a message preference with invalid borrowernumber' - .' => Koha::Exceptions::BadParameter'); - is ($@->parameter, 'borrowernumber', 'The previous exception tells us it' - .' was the borrowernumber.'); + .' => Koha::Exceptions::Patron::NotFound'); eval { Koha::Patron::Message::Preference->new({ categorycode => 'nonexistent', })->store }; - is(ref $@, 'Koha::Exceptions::BadParameter', + is(ref $@, 'Koha::Exceptions::Patron::Category::NotFound', 'Adding a message preference with invalid categorycode' - .' => Koha::Exceptions::BadParameter'); - is($@->parameter, 'categorycode', 'The previous exception tells us it' - .' was the categorycode.'); + .' => Koha::Exceptions::Patron::Category::NotFound'); my $attribute = build_a_test_attribute({ takes_days => 0 }); my $patron = $builder->build_object({ class => 'Koha::Patrons' }); @@ -495,33 +491,40 @@ subtest 'Test adding a new preference with invalid parameters' => sub { message_attribute_id => $attribute->message_attribute_id, days_in_advance => 10, })->store }; - is(ref $@, 'Koha::Exceptions::BadParameter', + is(ref $@, 'Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable', 'Adding a message preference with days in advance option when not' - .' available => Koha::Exceptions::BadParameter'); - is($@->parameter, 'days_in_advance', 'The previous exception tells us it' - .' was the days_in_advance.'); + .' available => Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable'); $attribute->set({ takes_days => 1 })->store; + eval { Koha::Patron::Message::Preference->new({ + borrowernumber => $patron->borrowernumber, + message_attribute_id => $attribute->message_attribute_id, + days_in_advance => -1, + })->store }; + is(ref $@, 'Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange', + 'Adding a message preference with days in advance option is out of range' + .' => Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange'); + is ($@->min, 0, 'The previous exception min value is 0.'); + eval { Koha::Patron::Message::Preference->new({ borrowernumber => $patron->borrowernumber, message_attribute_id => $attribute->message_attribute_id, days_in_advance => 31, })->store }; - is(ref $@, 'Koha::Exceptions::BadParameter', - 'Adding a message preference with days in advance option too large' - .' => Koha::Exceptions::BadParameter'); - is($@->parameter, 'days_in_advance', 'The previous exception tells us it' - .' was the days_in_advance.'); + is(ref $@, 'Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange', + 'Adding a message preference with days in advance option is out of range' + .' => Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange'); + is ($@->max, 30, 'The previous exception max value is 30.'); eval { Koha::Patron::Message::Preference->new({ borrowernumber => $patron->borrowernumber, message_transport_types => ['nonexistent'] })->store }; - is (ref $@, 'Koha::Exceptions::BadParameter', + is (ref $@, 'Koha::Exceptions::Patron::Message::Transport::TypeNotFound', 'Adding a message preference with invalid message_transport_type' - .' => Koha::Exceptions::BadParameter'); - is ($@->parameter, 'message_transport_types', 'The previous exception ' - .'tells us it was the message_transport_types.'); + .' => Koha::Exceptions::Patron::Message::Transport::TypeNotFound'); + is ($@->transport_type, 'nonexistent', 'The previous exception ' + .'tells us it which transport type it was.'); my $mtt_new = $builder->build_object({ class => 'Koha::Patron::Message::Transport::Types' }); eval { @@ -531,13 +534,13 @@ subtest 'Test adding a new preference with invalid parameters' => sub { message_transport_types => [$mtt_new->message_transport_type], wants_digest => 1, })->store }; - is (ref $@, 'Koha::Exceptions::BadParameter', + is (ref $@, 'Koha::Exceptions::Patron::Message::Preference::NoTransportType', 'Adding a message preference with invalid message_transport_type' - .' => 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.'); + .' => Koha::Exceptions::Patron::Message::Preference::NoTransportType'); + is ($@->message_name, $attribute->message_name, 'The previous exception ' + .'tells us which message it was.'); + is ($@->transport_type, $mtt_new->message_transport_type, 'The previous exception ' + .'tells us which message transport type it was.'); eval { Koha::Patron::Message::Preference->new({ @@ -546,13 +549,11 @@ subtest 'Test adding a new preference with invalid parameters' => sub { 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 cannot be selected/, 'Exception s because of' - .' given digest is not available for this transport.'); + is (ref $@, 'Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable', + 'Adding a message preference that has no digest setting' + .' => Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable'); + is ($@->message_name, $attribute->message_name, 'The previous exception tells us' + .' which message it was'); eval { Koha::Patron::Message::Preference->new({ @@ -561,27 +562,22 @@ subtest 'Test adding a new preference with invalid parameters' => sub { message_transport_types => [], wants_digest => 0, })->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 must be selected/, 'Exception s because of' - .' digest has to be on for this transport.'); - + is (ref $@, 'Koha::Exceptions::Patron::Message::Preference::DigestRequired', + 'Adding a message preference, that requires digest, with no digest' + .' => Koha::Exceptions::Patron::Message::Preference::DigestRequired'); + is ($@->message_name, $attribute->message_name, 'The previous exception tells us' + .' which message it was'); eval { Koha::Patron::Message::Preference->new({ borrowernumber => $patron->borrowernumber, message_attribute_id => -1, message_transport_types => [], })->store }; - is (ref $@, 'Koha::Exceptions::BadParameter', + is (ref $@, 'Koha::Exceptions::Patron::Message::Preference::AttributeNotFound', '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.'); + .' => KKoha::Exceptions::Patron::Message::Preference::AttributeNotFound'); + is ($@->message_attribute_id, -1, 'The previous exception tells' + .' us which message attribute id it was.'); $schema->storage->txn_rollback; }; -- 2.39.5