From 540136bb470ec66bff45881e97f51897bee731d8 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 12 Aug 2019 16:13:25 -0300 Subject: [PATCH] Bug 14570: Add error handling to Koha::Patron::Relationship->store This patch adds checks on the values for the 'relationship'. This is done to avoid future problems when migrating relationships from the plain text syspref into (why not) a proper table. And to preserve consistency. There's also catching on possible broken constraints and throwing a new exception Tests are added for both the new exceptions and the changes to Koha::Patron::Relationship. To test: - Apply this patches - Run: $ kshell k$ prove t/Koha/Exceptions.t \ t/db_dependent/Koha/Patron.t \ t/db_dependent/Koha/Patron/Relationship.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi Signed-off-by: Kyle M Hall Signed-off-by: Agustin Moyano Signed-off-by: Liz Rea Signed-off-by: Tomas Cohen Arazi Signed-off-by: Martin Renvoize --- Koha/Exceptions/Patron/Relationship.pm | 89 +++++++++++++++++ Koha/Patron/Relationship.pm | 37 +++++++- t/Koha/Exceptions.t | 48 +++++++++- t/db_dependent/Koha/Patron.t | 74 +++++++++++++++ t/db_dependent/Koha/Patron/Relationship.t | 111 ++++++++++++++++++++++ 5 files changed, 357 insertions(+), 2 deletions(-) create mode 100644 Koha/Exceptions/Patron/Relationship.pm create mode 100644 t/db_dependent/Koha/Patron.t create mode 100644 t/db_dependent/Koha/Patron/Relationship.t diff --git a/Koha/Exceptions/Patron/Relationship.pm b/Koha/Exceptions/Patron/Relationship.pm new file mode 100644 index 0000000000..d8a79d0810 --- /dev/null +++ b/Koha/Exceptions/Patron/Relationship.pm @@ -0,0 +1,89 @@ +package Koha::Exceptions::Patron::Relationship; + +# 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, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Patron::Relationship' => { + description => 'Something went wrong!', + }, + 'Koha::Exceptions::Patron::Relationship::DuplicateRelationship' => { + isa => 'Koha::Exceptions::Patron::Relationship', + description => 'There can only be one relationship between patrons in a direction', + fields => [ 'guarantor_id', 'guarantee_id' ] + }, + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship' => { + isa => 'Koha::Exceptions::Patron::Relationship', + description => 'The specified relationship is invalid', + fields => ['relationship','no_relationship'] + } +); + +sub full_message { + my $self = shift; + + my $msg = $self->message; + + unless ( $msg) { + if ( $self->isa('Koha::Exceptions::Patron::Relationship::InvalidRelationship') ) { + if ( $self->no_relationship ) { + $msg = sprintf( "No relationship passed." ); + } + else { + $msg = sprintf("Invalid relationship passed, '%s' is not defined.", $self->relationship ); + } + } + elsif ( $self->isa('Koha::Exceptions::Patron::Relationship::DuplicateRelationship') ) { + $msg + = sprintf( + "There already exists a relationship for the same guarantor (%s) and guarantee (%s) combination", + $self->guarantor_id, $self->guarantee_id ); + } + } + + return $msg; +} + +=head1 NAME + +Koha::Exceptions::Patron::Relationship - Base class for patron relatioship exceptions + +=head1 Exceptions + +=head2 Koha::Exceptions::Patron::Relationship + +Generic Patron exception + +=head2 Koha::Exceptions::Patron::Relationship::DuplicateRelationship + +Exception to be used when more than one relationship is requested for a +pair of patrons in the same direction. + +=head2 Koha::Exceptions::Patron::Relationship::InvalidRelationship + +Exception to be used when an invalid relationship is passed. + +=head1 Class methods + +=head2 full_message + +Overloaded method for exception stringifying. + +=cut + +1; diff --git a/Koha/Patron/Relationship.pm b/Koha/Patron/Relationship.pm index 24994aa2b7..f3e3e65345 100644 --- a/Koha/Patron/Relationship.pm +++ b/Koha/Patron/Relationship.pm @@ -18,8 +18,11 @@ package Koha::Patron::Relationship; use Modern::Perl; use Carp; +use List::MoreUtils qw( any ); +use Try::Tiny; use Koha::Database; +use Koha::Exceptions::Patron::Relationship; use base qw(Koha::Object); @@ -32,10 +35,42 @@ and provides a way to access those relationships. =head1 API -=head2 Class Methods +=head2 Class methods =cut +=head3 store + +Overloaded method that makes some checks before storing on the DB + +=cut + +sub store { + my ( $self ) = @_; + + my @valid_relationships = split /,|\|/, C4::Context->preference('borrowerRelationship'); + + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( + no_relationship => 1 ) + unless defined $self->relationship; + + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( + relationship => $self->relationship ) + unless any { $_ eq $self->relationship } @valid_relationships; + + return try { + $self->SUPER::store; + } + catch { + if ( ref($_) eq 'Koha::Exceptions::Object::DuplicateID' ) { + Koha::Exceptions::Patron::Relationship::DuplicateRelationship->throw( + guarantee_id => $self->guarantee_id, + guarantor_id => $self->guarantor_id + ); + } + }; +} + =head3 guarantor Returns the Koha::Patron object for the guarantor, if there is one diff --git a/t/Koha/Exceptions.t b/t/Koha/Exceptions.t index a5efae2791..a5a504e6fd 100644 --- a/t/Koha/Exceptions.t +++ b/t/Koha/Exceptions.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 4; +use Test::More tests => 5; use Test::MockObject; use Test::Exception; @@ -128,3 +128,49 @@ subtest 'Koha::Exceptions::Metadata tests' => sub { 'Exception is thrown :-D'; is( "$@", 'Manual message exception', 'Exception not stringified if manually passed' ); }; + +subtest 'Koha::Exceptions::Patron::Relationship tests' => sub { + + plan tests => 9; + + use_ok('Koha::Exceptions::Patron::Relationship'); + + throws_ok + { Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( no_relationship => 1 ); } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception is thrown :-D'; + + # stringify the exception + is( "$@", 'No relationship passed.', 'Exception stringified correctly' ); + + throws_ok + { Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( relationship => 'some' ); } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception is thrown :-D'; + + # stringify the exception + is( "$@", "Invalid relationship passed, 'some' is not defined.", 'Exception stringified correctly' ); + + my $guarantor_id = 1; + my $guarantee_id = 2; + + throws_ok { + Koha::Exceptions::Patron::Relationship::DuplicateRelationship->throw( + guarantor_id => $guarantor_id, + guarantee_id => $guarantee_id + ); + } + 'Koha::Exceptions::Patron::Relationship::DuplicateRelationship', 'Exception is thrown :-D'; + + # stringify the exception + is( "$@", + "There already exists a relationship for the same guarantor ($guarantor_id) and guarantee ($guarantee_id) combination", + 'Exception stringified correctly' + ); + + throws_ok + { Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( "Manual message exception" ) } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception is thrown :-D'; + is( "$@", 'Manual message exception', 'Exception not stringified if manually passed' ); +}; diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t new file mode 100644 index 0000000000..76bbed8b84 --- /dev/null +++ b/t/db_dependent/Koha/Patron.t @@ -0,0 +1,74 @@ +#!/usr/bin/perl + +# Copyright 2019 Koha Development team +# +# 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 Test::More tests => 1; +use Test::Exception; + +use Koha::Database; +use Koha::Patrons; +use Koha::Patron::Relationships; + +use t::lib::TestBuilder; +use t::lib::Mocks; + +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +subtest 'add_guarantor() tests' => sub { + + plan tests => 6; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'borrowerRelationship', 'father1|father2' ); + + my $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); + my $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); + + throws_ok + { $patron_1->add_guarantor({ guarantor_id => $patron_2->borrowernumber }); } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception is thrown as no relationship passed'; + + is( $patron_1->guarantee_relationships->count, 0, 'No guarantors added' ); + + throws_ok + { $patron_1->add_guarantor({ guarantor_id => $patron_2->borrowernumber, relationship => 'father' }); } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception is thrown as a wrong relationship was passed'; + + is( $patron_1->guarantee_relationships->count, 0, 'No guarantors added' ); + + $patron_1->add_guarantor({ guarantor_id => $patron_2->borrowernumber, relationship => 'father1' }); + + my $guarantors = $patron_1->guarantor_relationships; + + is( $guarantors->count, 1, 'No guarantors added' ); + + $SIG{__WARN__} = sub {}; # FIXME: PrintError = 0 not working! + + throws_ok + { $patron_1->add_guarantor({ guarantor_id => $patron_2->borrowernumber, relationship => 'father2' }); } + 'Koha::Exceptions::Patron::Relationship::DuplicateRelationship', + 'Exception is thrown for duplicated relationship'; + + $schema->storage->txn_rollback; +}; diff --git a/t/db_dependent/Koha/Patron/Relationship.t b/t/db_dependent/Koha/Patron/Relationship.t new file mode 100644 index 0000000000..47b51f7190 --- /dev/null +++ b/t/db_dependent/Koha/Patron/Relationship.t @@ -0,0 +1,111 @@ +#!/usr/bin/perl + +# Copyright 2019 Koha Development team +# +# 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 Test::More tests => 1; +use Test::Exception; + +use Koha::Database; +use Koha::Patrons; +use Koha::Patron::Relationships; + +use t::lib::TestBuilder; +use t::lib::Mocks; + +my $schema = Koha::Database->new->schema; +my $dbh = $schema->storage->dbh; +my $builder = t::lib::TestBuilder->new; + +subtest 'store() tests' => sub { + + plan tests => 9; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'borrowerRelationship', 'father1|father2' ); + + my $patron_1 = $builder->build_object( { class => 'Koha::Patrons' } ); + my $patron_2 = $builder->build_object( { class => 'Koha::Patrons' } ); + + my $relationship_1 = Koha::Patron::Relationship->new( + { guarantor_id => $patron_2->borrowernumber, + guarantee_id => $patron_1->borrowernumber + } + ); + + throws_ok + { $relationship_1->store; } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception is thrown as no relationship passed'; + + is( "$@", "No relationship passed.", 'Exception stringified correctly' ); + + is( Koha::Patron::Relationships->search( { guarantee_id => $patron_1->borrowernumber } )->count, + 0, + 'No guarantors added' + ); + + my $relationship = 'father'; + + throws_ok + { $relationship_1->relationship($relationship)->store; } + 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', + 'Exception is thrown as a wrong relationship was passed'; + + is( "$@", "Invalid relationship passed, '$relationship' is not defined.", 'Exception stringified correctly' ); + + is( Koha::Patron::Relationships->search( { guarantee_id => $patron_1->borrowernumber } )->count, + 0, + 'No guarantors added' + ); + + $relationship_1->relationship('father1')->store; + + is( Koha::Patron::Relationships->search( { guarantee_id => $patron_1->borrowernumber } )->count, + 1, + 'Guarantor added' + ); + + my $relationship_2 = Koha::Patron::Relationship->new( + { guarantor_id => $patron_2->borrowernumber, + guarantee_id => $patron_1->borrowernumber, + relationship => 'father2' + } + ); + + $SIG{__WARN__} = sub {}; # FIXME: PrintError = 0 not working! + + throws_ok + { $relationship_2->store; } + 'Koha::Exceptions::Patron::Relationship::DuplicateRelationship', + 'Exception is thrown for duplicated relationship'; + + is( "$@", + "There already exists a relationship for the same guarantor (" + . $patron_2->borrowernumber + . ") and guarantee (" + . $patron_1->borrowernumber + . ") combination", + 'Exception stringified correctly' + ); + + + $schema->storage->txn_rollback; +}; -- 2.39.5