From 1ef1092a137152b1dfe193793ec73a957331d852 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>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@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.17.1