From bdf0199f531e329a9a947fc22672586681f54706 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 19 Dec 2017 14:46:45 -0300 Subject: [PATCH] Bug 16330: Remove validation code from Koha::Patron 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 --- Koha/Exceptions/Category.pm | 17 ------ Koha/Exceptions/Library.pm | 17 ------ Koha/Exceptions/Patron.pm | 17 ------ Koha/Patron.pm | 129 +--------------------------------------- Koha/REST/V1/Patrons.pm | 53 ++++++++--------- t/db_dependent/Koha/Patrons.t | 96 +----------------------------- t/db_dependent/api/v1/patrons.t | 59 +++++++++--------- 7 files changed, 58 insertions(+), 330 deletions(-) delete mode 100644 Koha/Exceptions/Category.pm delete mode 100644 Koha/Exceptions/Library.pm delete mode 100644 Koha/Exceptions/Patron.pm diff --git a/Koha/Exceptions/Category.pm b/Koha/Exceptions/Category.pm deleted file mode 100644 index 3487faa9c7..0000000000 --- a/Koha/Exceptions/Category.pm +++ /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 index 1aea6add13..0000000000 --- a/Koha/Exceptions/Library.pm +++ /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 index c409b4e3d1..0000000000 --- a/Koha/Exceptions/Patron.pm +++ /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; diff --git a/Koha/Patron.pm b/Koha/Patron.pm index f8da5d3606..414f18d870 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -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. # @@ -30,11 +29,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; @@ -676,19 +670,9 @@ sub account_locked { and $self->login_attempts >= $FailedLoginAttempts )? 1 : 0; } -=head3 store - -=cut - -sub store { - my ($self) = @_; - - # $self->_validate(); - - return $self->SUPER::store(); -} +=head2 Internal methods -=head3 type +=head3 _type =cut @@ -696,115 +680,6 @@ 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. - -=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; -} - =head1 AUTHOR Kyle M Hall diff --git a/Koha/REST/V1/Patrons.pm b/Koha/REST/V1/Patrons.pm index 7868e8d148..81e2642a17 100644 --- a/Koha/REST/V1/Patrons.pm +++ b/Koha/REST/V1/Patrons.pm @@ -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') ) { diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 263024d04b..6c0cb2e5a5 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 23; +use Test::More tests => 22; use Test::Warn; use Time::Fake; use DateTime; @@ -929,97 +929,3 @@ $retrieved_patron_1->delete; is( Koha::Patrons->search->count, $nb_of_patrons + 1, 'Delete should have deleted the patron' ); $schema->storage->txn_rollback; - -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; -}; diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index b52066d153..fb47af4c9a 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -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; -- 2.15.1