Bugzilla – Attachment 70038 Details for
Bug 16330
Add routes to add, update and delete patrons
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 16330: Remove validation code from Koha::Patron
Bug-16330-Remove-validation-code-from-KohaPatron.patch (text/plain), 20.52 KB, created by
Tomás Cohen Arazi (tcohen)
on 2017-12-21 14:43:54 UTC
(
hide
)
Description:
Bug 16330: Remove validation code from Koha::Patron
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2017-12-21 14:43:54 UTC
Size:
20.52 KB
patch
obsolete
>From bdf0199f531e329a9a947fc22672586681f54706 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> >--- > 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 <kyle@bywatersolutions.com> >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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=70038">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 16330
:
50803
|
50889
|
51066
|
51131
|
51132
|
53216
|
53217
|
53820
|
54942
|
55147
|
56666
|
56667
|
56668
|
63646
|
63647
|
63649
|
63650
|
69546
|
69547
|
69548
|
69549
|
69550
|
69551
|
69752
|
69762
|
69763
|
69764
|
69765
|
69766
|
69767
|
69768
|
69787
|
70034
|
70035
|
70036
|
70037
|
70038
|
71667
|
71668
|
71669
|
71670
|
71671
|
71920
|
71921
|
71922
|
71923
|
71924
|
71925
|
71933
|
71934
|
71935
|
71936
|
71937
|
71938
|
73434