Attachment 92181 Details for Bug 14570 – Bug 14570: Add error handling to Koha::Patron::Relationship->store

[patch] Bug 14570: Add error handling to Koha::Patron::Relationship->store

Bug-14570-Add-error-handling-to-KohaPatronRelation.patch (text/plain), 14.69 KB, created by Tomás Cohen Arazi (tcohen) on 2019-08-13 17:13:23 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2019-08-13 17:13:23 UTC

Size: 14.69 KB

patch

obsolete

>From c9a5c50379570de8862c1e5b29337f979b92f21a Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>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 <tomascohen@theke.io>
>---
> 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 <http://www.gnu.org/licenses>.
>+
>+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 <http://www.gnu.org/licenses>.
>+
>+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.22.0

Actions: View | Diff | Splinter Review

Attachments on bug 14570: 52400 | 52401 | 52402 | 52403 | 52406 | 52407 | 58146 | 58147 | 58159 | 58202 | 58391 | 58399 | 58412 | 58688 | 58689 | 58739 | 58762 | 58763 | 58764 | 58834 | 58835 | 58836 | 58837 | 58838 | 58839 | 58949 | 58950 | 58951 | 58952 | 58953 | 58954 | 58972 | 58973 | 58974 | 58975 | 58976 | 58977 | 58978 | 58979 | 58980 | 62450 | 62451 | 62452 | 64124 | 64125 | 64126 | 71160 | 71161 | 71162 | 71163 | 73283 | 73284 | 73583 | 73584 | 74950 | 74951 | 74952 | 74953 | 74954 | 74976 | 74977 | 74978 | 74979 | 77029 | 77030 | 77031 | 77032 | 77033 | 77034 | 77040 | 77041 | 77042 | 77043 | 77044 | 77045 | 77046 | 77047 | 77048 | 77049 | 77067 | 77068 | 77069 | 77070 | 77071 | 77072 | 77073 | 77074 | 77075 | 77076 | 77077 | 77078 | 77290 | 77291 | 77346 | 77347 | 77348 | 80842 | 80849 | 81932 | 81933 | 85411 | 85412 | 85431 | 89889 | 89890 | 89891 | 89892 | 89909 | 89910 | 89911 | 89912 | 89921 | 89922 | 89929 | 90048 | 91231 | 91439 | 91440 | 91441 | 91442 | 91443 | 91444 | 91445 | 91694 | 91695 | 91696 | 91697 | 91698 | 91699 | 91700 | 92157 | 92158 | 92161 | 92162 | 92163 | 92180 | 92181 | 92182 | 92183 | 92232 | 92233 | 92234 | 92235 | 92277 | 92278 | 92279 | 92280 | 92281 | 92282 | 92283 | 92284 | 92285 | 92286 | 92287 | 92288 | 92289 | 92290 | 92291 | 92292 | 92298 | 92299 | 92300 | 92301 | 92302 | 92303 | 92304 | 92305 | 92306 | 92307 | 92308 | 92309 | 92310 | 92311 | 92312 | 92313 | 92314 | 92315 | 92316 | 92317 | 92318 | 92319 | 92320 | 92321 | 92322 | 92323 | 92324 | 92325 | 92346 | 92347 | 92348 | 92349 | 92350