@@ -, +, @@ - Run: $ kshell k$ prove t/db_dependent/Koha/Patrons.t k$ prove t/db_dependent/api/v1/patrons.t --- 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 | 96 +------------------------------- t/db_dependent/api/v1/patrons.t | 59 ++++++++++---------- 7 files changed, 59 insertions(+), 319 deletions(-) delete mode 100644 Koha/Exceptions/Category.pm delete mode 100644 Koha/Exceptions/Library.pm delete mode 100644 Koha/Exceptions/Patron.pm --- a/Koha/Exceptions/Category.pm +++ a/Koha/Exceptions/Category.pm @@ -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; --- a/Koha/Exceptions/Library.pm +++ a/Koha/Exceptions/Library.pm @@ -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; --- a/Koha/Exceptions/Patron.pm +++ a/Koha/Exceptions/Patron.pm @@ -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; --- a/Koha/Patron.pm +++ a/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; @@ -824,121 +818,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 --- a/Koha/REST/V1/Patrons.pm +++ a/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') ) { --- a/t/db_dependent/Koha/Patrons.t +++ a/t/db_dependent/Koha/Patrons.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 25; +use Test::More tests => 24; use Test::Warn; use Time::Fake; use DateTime; @@ -1071,97 +1071,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; -}; --- a/t/db_dependent/api/v1/patrons.t +++ a/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; --