Bug 16330: Remove validation code from Koha::Patron
authorTomas Cohen Arazi <tomascohen@theke.io>
Tue, 19 Dec 2017 17:46:45 +0000 (14:46 -0300)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Thu, 29 Mar 2018 14:42:07 +0000 (11:42 -0300)
This patch removes previously added validation code from Koha::Patron
as we will rely on the DB structure and relationships to catch the same
problems. This is implemented on bug 19828.

This patch also adapts the API controller class to expect this behaviour
change from Koha::Patron. The expected exceptions are adjusted, and some
minor changes take place. The API tests are adjusted as well.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests should still pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Koha/Exceptions/Category.pm [deleted file]
Koha/Exceptions/Library.pm [deleted file]
Koha/Exceptions/Patron.pm [deleted file]
Koha/Patron.pm
Koha/REST/V1/Patrons.pm
t/db_dependent/Koha/Patrons.t
t/db_dependent/api/v1/patrons.t

diff --git a/Koha/Exceptions/Category.pm b/Koha/Exceptions/Category.pm
deleted file mode 100644 (file)
index 3487faa..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-package Koha::Exceptions::Category;
-
-use Modern::Perl;
-
-use Exception::Class (
-
-    'Koha::Exceptions::Category' => {
-        description => 'Something went wrong!',
-    },
-    'Koha::Exceptions::Category::CategorycodeNotFound' => {
-        isa => 'Koha::Exceptions::Category',
-        description => "Category does not exist",
-        fields => ["categorycode"],
-    },
-);
-
-1;
diff --git a/Koha/Exceptions/Library.pm b/Koha/Exceptions/Library.pm
deleted file mode 100644 (file)
index 1aea6ad..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-package Koha::Exceptions::Library;
-
-use Modern::Perl;
-
-use Exception::Class (
-
-    'Koha::Exceptions::Library' => {
-        description => 'Something went wrong!',
-    },
-    'Koha::Exceptions::Library::BranchcodeNotFound' => {
-        isa => 'Koha::Exceptions::Library',
-        description => "Library does not exist",
-        fields => ["branchcode"],
-    },
-);
-
-1;
diff --git a/Koha/Exceptions/Patron.pm b/Koha/Exceptions/Patron.pm
deleted file mode 100644 (file)
index c409b4e..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-package Koha::Exceptions::Patron;
-
-use Modern::Perl;
-
-use Exception::Class (
-
-    'Koha::Exceptions::Patron' => {
-        description => 'Something went wrong!',
-    },
-    'Koha::Exceptions::Patron::DuplicateObject' => {
-        isa => 'Koha::Exceptions::Patron',
-        description => "Patron cardnumber and userid must be unique",
-        fields => ["conflict"],
-    },
-);
-
-1;
index faf4efa..43f4265 100644 (file)
@@ -2,7 +2,6 @@ package Koha::Patron;
 
 # Copyright ByWater Solutions 2014
 # Copyright PTFS Europe 2016
-# Copyright Koha-Suomi Oy 2017
 #
 # This file is part of Koha.
 #
@@ -31,11 +30,6 @@ use Koha::Database;
 use Koha::DateUtils;
 use Koha::Holds;
 use Koha::Old::Checkouts;
-use Koha::Exceptions;
-use Koha::Exceptions::Category;
-use Koha::Exceptions::Library;
-use Koha::Exceptions::Patron;
-use Koha::Libraries;
 use Koha::Patron::Categories;
 use Koha::Patron::HouseboundProfile;
 use Koha::Patron::HouseboundRole;
@@ -849,121 +843,14 @@ sub store {
     return $self->SUPER::store();
 }
 
-=head3 type
-
-=cut
-
-sub _type {
-    return 'Borrower';
-}
-
 =head2 Internal methods
 
-=head3 _check_branchcode
-
-Checks the existence of patron's branchcode and throws
-Koha::Exceptions::Library::BranchcodeNotFound if branchcode is not found.
-
-=cut
-
-sub _check_branchcode {
-    my ($self) = @_;
-
-    return unless $self->branchcode;
-    unless (Koha::Libraries->find($self->branchcode)) {
-        Koha::Exceptions::Library::BranchcodeNotFound->throw(
-            error => "Library does not exist",
-            branchcode => $self->branchcode,
-        );
-    }
-    return 1;
-}
-
-=head3 _check_categorycode
-
-Checks the existence of patron's categorycode and throws
-Koha::Exceptions::Category::CategorycodeNotFound if categorycode is not found.
+=head3 _type
 
 =cut
 
-sub _check_categorycode {
-    my ($self) = @_;
-
-    return unless $self->categorycode;
-    unless (Koha::Patron::Categories->find($self->categorycode)) {
-        Koha::Exceptions::Category::CategorycodeNotFound->throw(
-            error => "Patron category does not exist",
-            categorycode => $self->categorycode,
-        );
-    }
-    return 1;
-}
-
-=head3 _check_uniqueness
-
-Checks patron's cardnumber and userid for uniqueness and throws
-Koha::Exceptions::Patron::DuplicateObject if conflicting with another patron.
-
-=cut
-
-sub _check_uniqueness {
-    my ($self) = @_;
-
-    my $select = {};
-    $select->{cardnumber} = $self->cardnumber if $self->cardnumber;
-    $select->{userid} = $self->userid if $self->userid;
-
-    return unless keys %$select;
-
-    # Find conflicting patrons
-    my $patrons = Koha::Patrons->search({
-        '-or' => $select
-    });
-
-    if ($patrons->count) {
-        my $conflict = {};
-        foreach my $patron ($patrons->as_list) {
-            # New patron $self: a conflicting patron $patron found.
-            # Updating patron $self: first make sure conflicting patron $patron is
-            #                        not this patron $self.
-            if (!$self->in_storage || $self->in_storage &&
-            $self->borrowernumber != $patron->borrowernumber) {
-                # Populate conflict information to exception
-                if ($patron->cardnumber && $self->cardnumber &&
-                    $patron->cardnumber eq $self->cardnumber)
-                {
-                    $conflict->{cardnumber} = $self->cardnumber;
-                }
-                if ($patron->userid && $self->userid &&
-                    $patron->userid eq $self->userid)
-                {
-                    $conflict->{userid} = $self->userid;
-                }
-            }
-        }
-
-        Koha::Exceptions::Patron::DuplicateObject->throw(
-            error => "Patron data conflicts with another patron",
-            conflict => $conflict
-        ) if keys %$conflict;
-    }
-    return 1;
-}
-
-=head3 _validate
-
-Performs a set of validations on this object and throws Koha::Exceptions if errors
-are found.
-
-=cut
-
-sub _validate {
-    my ($self) = @_;
-
-    $self->_check_branchcode;
-    $self->_check_categorycode;
-    $self->_check_uniqueness;
-    return $self;
+sub _type {
+    return 'Borrower';
 }
 
 =head1 AUTHOR
index 7868e8d..81e2642 100644 (file)
@@ -90,9 +90,8 @@ sub add {
     my $c = shift->openapi->valid_input or return;
 
     return try {
-        my $body = $c->validation->param('body');
 
-        Koha::Patron->new($body)->_validate;
+        my $body = _to_model($c->validation->param('body'));
 
         # TODO: Use AddMember until it has been moved to Koha-namespace
         my $borrowernumber = AddMember(%$body);
@@ -109,22 +108,22 @@ sub add {
                 }
             );
         }
-        if ( $_->isa('Koha::Exceptions::Patron::DuplicateObject') ) {
+        if ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) {
             return $c->render(
                 status  => 409,
-                openapi => { error => $_->error, conflict => $_->conflict }
+                openapi => { error => $_->error, conflict => $_->duplicate_id }
             );
         }
-        elsif ( $_->isa('Koha::Exceptions::Library::BranchcodeNotFound') ) {
+        elsif ( $_->isa('Koha::Exceptions::Object::FKConstraint') ) {
             return $c->render(
                 status  => 400,
-                openapi => { error => "Given branchcode does not exist" }
+                openapi => { error => "Given " . $_->broken_fk . " does not exist" }
             );
         }
-        elsif ( $_->isa('Koha::Exceptions::Category::CategorycodeNotFound') ) {
+        elsif ( $_->isa('Koha::Exceptions::BadParameter') ) {
             return $c->render(
                 status  => 400,
-                openapi => { error => "Given categorycode does not exist" }
+                openapi => { error => "Given " . $_->parameter . " does not exist" }
             );
         }
         else {
@@ -147,18 +146,26 @@ Controller function that handles updating a Koha::Patron object
 sub update {
     my $c = shift->openapi->valid_input or return;
 
-    my $patron = Koha::Patrons->find( $c->validation->param('borrowernumber') );
+    my $patron_id = $c->validation->param('borrowernumber');
+    my $patron    = Koha::Patrons->find( $patron_id );
 
-    return try {
-        my $body = $c->validation->param('body');
+    unless ($patron) {
+         return $c->render(
+             status  => 404,
+             openapi => { error => "Patron not found" }
+         );
+     }
 
-        $patron->set( _to_model($body) )->_validate;
+    return try {
+        my $body = _to_model($c->validation->param('body'));
 
         ## TODO: Use ModMember until it has been moved to Koha-namespace
         # Add borrowernumber to $body, as required by ModMember
-        $body->{borrowernumber} = $patron->borrowernumber;
+        $body->{borrowernumber} = $patron_id;
 
         if ( ModMember(%$body) ) {
+            # Fetch the updated Koha::Patron object
+            $patron->discard_changes;
             return $c->render( status => 200, openapi => $patron );
         }
         else {
@@ -171,12 +178,6 @@ sub update {
         }
     }
     catch {
-        unless ($patron) {
-            return $c->render(
-                status  => 404,
-                openapi => { error => "Patron not found" }
-            );
-        }
         unless ( blessed $_ && $_->can('rethrow') ) {
             return $c->render(
                 status  => 500,
@@ -185,22 +186,16 @@ sub update {
                 }
             );
         }
-        if ( $_->isa('Koha::Exceptions::Patron::DuplicateObject') ) {
+        if ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) {
             return $c->render(
                 status  => 409,
-                openapi => { error => $_->error, conflict => $_->conflict }
-            );
-        }
-        elsif ( $_->isa('Koha::Exceptions::Library::BranchcodeNotFound') ) {
-            return $c->render(
-                status  => 400,
-                openapi => { error => "Given branchcode does not exist" }
+                openapi => { error => $_->error, conflict => $_->duplicate_id }
             );
         }
-        elsif ( $_->isa('Koha::Exceptions::Category::CategorycodeNotFound') ) {
+        elsif ( $_->isa('Koha::Exceptions::Object::FKConstraint') ) {
             return $c->render(
                 status  => 400,
-                openapi => { error => "Given categorycode does not exist" }
+                openapi => { error => "Given " . $_->broken_fk . " does not exist" }
             );
         }
         elsif ( $_->isa('Koha::Exceptions::MissingParameter') ) {
index 01a1bd9..c494241 100644 (file)
@@ -1153,97 +1153,3 @@ sub set_logged_in_user {
         '',                      ''
     );
 }
-
-subtest '_validate() tests' => sub {
-    plan tests => 4;
-
-    $schema->storage->txn_begin;
-
-    Koha::Patrons->delete;
-
-    my $categorycode = $builder->build({ source => 'Category' })->{categorycode};
-    my $branchcode = $builder->build({ source => 'Branch' })->{branchcode};
-    my $patron = $builder->build({
-        source => 'Borrower',
-        value => {
-            branchcode   => $branchcode,
-            cardnumber   => 'conflict',
-            categorycode => $categorycode,
-        }
-    });
-
-    ok(Koha::Patron->new({
-        surname      => 'Store test',
-        branchcode   => $branchcode,
-        categorycode => $categorycode
-    })->_validate->store, 'Stored a patron');
-
-    subtest '_check_categorycode' => sub {
-        plan tests => 2;
-
-        my $conflicting = $builder->build({
-            source => 'Borrower',
-            value => {
-                branchcode   => $branchcode,
-                categorycode => 'nonexistent',
-            }
-        });
-        delete $conflicting->{borrowernumber};
-
-        eval { Koha::Patron->new($conflicting)->_validate };
-
-        isa_ok($@, "Koha::Exceptions::Category::CategorycodeNotFound");
-        is($@->{categorycode}, $conflicting->{categorycode},
-           'Exception describes non-existent categorycode');
-    };
-
-    subtest '_check_categorycode' => sub {
-        plan tests => 2;
-
-        my $conflicting = $builder->build({
-            source => 'Borrower',
-            value => {
-                branchcode   => 'nonexistent',
-                categorycode => $categorycode,
-            }
-        });
-        delete $conflicting->{borrowernumber};
-
-        eval { Koha::Patron->new($conflicting)->_validate };
-
-        isa_ok($@, "Koha::Exceptions::Library::BranchcodeNotFound");
-        is($@->{branchcode}, $conflicting->{branchcode},
-           'Exception describes non-existent branchcode');
-    };
-
-    subtest '_check_uniqueness() tests' => sub {
-        plan tests => 4;
-
-        my $conflicting = $builder->build({
-            source => 'Borrower',
-            value => {
-                branchcode   => $branchcode,
-                categorycode => $categorycode,
-            }
-        });
-        delete $conflicting->{borrowernumber};
-        $conflicting->{cardnumber} = 'conflict';
-        $conflicting->{userid} = $patron->{userid};
-
-        eval { Koha::Patron->new($conflicting)->_validate };
-
-        isa_ok($@, "Koha::Exceptions::Patron::DuplicateObject");
-        is($@->{conflict}->{cardnumber}, $conflicting->{cardnumber},
-           'Exception describes conflicting cardnumber');
-        is($@->{conflict}->{userid}, $conflicting->{userid},
-           'Exception describes conflicting userid');
-
-        $conflicting->{cardnumber} = 'notconflicting';
-        $conflicting->{userid}     = 'notconflicting';
-
-        ok(Koha::Patron->new($conflicting)->_validate->store, 'After modifying'
-           .' cardnumber and userid to not conflict with others, no exception.');
-    };
-
-    $schema->storage->txn_rollback;
-};
index a23b47b..a482d2d 100644 (file)
@@ -19,6 +19,7 @@ use Modern::Perl;
 
 use Test::More tests => 5;
 use Test::Mojo;
+use Test::Warn;
 
 use t::lib::TestBuilder;
 use t::lib::Mocks;
@@ -160,7 +161,7 @@ subtest 'add() tests' => sub {
     unauthorized_access_tests('POST', undef, $newpatron);
 
     subtest 'librarian access tests' => sub {
-        plan tests => 18;
+        plan tests => 20;
 
         my ($borrowernumber, $sessionid) = create_user_and_session({
             authorized => 1 });
@@ -168,9 +169,12 @@ subtest 'add() tests' => sub {
         $newpatron->{branchcode} = "nonexistent"; # Test invalid branchcode
         my $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron );
         $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
-        $t->request_ok($tx)
-          ->status_is(400)
-          ->json_is('/error' => "Given branchcode does not exist");
+        warning_like {
+            $t->request_ok($tx)
+              ->status_is(400)
+              ->json_is('/error' => "Given branchcode does not exist"); }
+            qr/^DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails/;
+
         $newpatron->{branchcode} = $branchcode;
 
         $newpatron->{categorycode} = "nonexistent"; # Test invalid patron category
@@ -200,15 +204,12 @@ subtest 'add() tests' => sub {
 
         $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron);
         $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
-        $t->request_ok($tx)
-          ->status_is(409)
-          ->json_has('/error', 'Fails when trying to POST duplicate'.
-                     ' cardnumber or userid')
-          ->json_has('/conflict', {
-                        userid => $newpatron->{ userid },
-                        cardnumber => $newpatron->{ cardnumber }
-                    }
-            );
+        warning_like {
+            $t->request_ok($tx)
+              ->status_is(409)
+              ->json_has( '/error', 'Fails when trying to POST duplicate cardnumber' )
+              ->json_has( '/conflict', 'cardnumber' ); }
+            qr/^DBD::mysql::st execute failed: Duplicate entry '(.*?)' for key 'cardnumber'/;
     };
 
     $schema->storage->txn_rollback;
@@ -222,7 +223,7 @@ subtest 'update() tests' => sub {
     unauthorized_access_tests('PUT', 123, {email => 'nobody@example.com'});
 
     subtest 'librarian access tests' => sub {
-        plan tests => 20;
+        plan tests => 23;
 
         t::lib::Mocks::mock_preference('minPasswordLength', 1);
         my ($borrowernumber, $sessionid) = create_user_and_session({ authorized => 1 });
@@ -243,17 +244,21 @@ subtest 'update() tests' => sub {
         $newpatron->{categorycode} = 'nonexistent';
         $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron );
         $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
-        $t->request_ok($tx)
-          ->status_is(400)
-          ->json_is('/error' => "Given categorycode does not exist");
+        warning_like {
+            $t->request_ok($tx)
+              ->status_is(400)
+              ->json_is('/error' => "Given categorycode does not exist"); }
+            qr/^DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails/;
         $newpatron->{categorycode} = $patron_2->categorycode;
 
         $newpatron->{branchcode} = 'nonexistent';
         $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron );
         $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
-        $t->request_ok($tx)
-          ->status_is(400)
-          ->json_is('/error' => "Given branchcode does not exist");
+        warning_like {
+            $t->request_ok($tx)
+              ->status_is(400)
+              ->json_is('/error' => "Given branchcode does not exist"); }
+            qr/^DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails/;
         $newpatron->{branchcode} = $patron_2->branchcode;
 
         $newpatron->{falseproperty} = "Non existent property";
@@ -271,14 +276,12 @@ subtest 'update() tests' => sub {
 
         $tx = $t->ua->build_tx( PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron );
         $tx->req->cookies({ name => 'CGISESSID', value => $sessionid });
-        $t->request_ok($tx)->status_is(409)
-          ->json_has( '/error' => "Fails when trying to update to an existing cardnumber or userid")
-          ->json_is(  '/conflict',
-                        {
-                            cardnumber => $newpatron->{cardnumber},
-                            userid     => $newpatron->{userid}
-                        }
-          );
+        warning_like {
+            $t->request_ok($tx)
+              ->status_is(409)
+              ->json_has( '/error' => "Fails when trying to update to an existing cardnumber or userid")
+              ->json_is(  '/conflict', 'cardnumber' ); }
+            qr/^DBD::mysql::st execute failed: Duplicate entry '(.*?)' for key 'cardnumber'/;
 
         $newpatron->{ cardnumber } = $borrowernumber.$borrowernumber2;
         $newpatron->{ userid } = "user".$borrowernumber.$borrowernumber2;