From 2183c7a175f1ac3648b7963a5d18dce0cd2936f9 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 Signed-off-by: Josef Moravec Signed-off-by: Jonathan Druart --- Koha/Exceptions/Category.pm | 17 ----- Koha/Exceptions/Library.pm | 17 ----- Koha/Exceptions/Patron.pm | 17 ----- Koha/Patron.pm | 119 +------------------------------- Koha/REST/V1/Patrons.pm | 53 +++++++------- t/db_dependent/Koha/Patrons.t | 94 ------------------------- t/db_dependent/api/v1/patrons.t | 59 ++++++++-------- 7 files changed, 58 insertions(+), 318 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 faf4efaa23..43f4265bcc 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. # @@ -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 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 01a1bd987c..c494241097 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -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; -}; diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index a23b47be10..a482d2df0b 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.39.5