From 3539aa2b3ebb314141874b459ec5564261a444b8 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Tue, 18 Oct 2016 20:07:42 +0300 Subject: [PATCH] Bug 16330: (follow-up) Move validation logic from controller to Koha::Patron This patch moves validation logic from controller into Koha::Patron object and takes advantage of Koha::Exceptions for throwing proper exceptions and catching them inside controller in order to return correct error messages. To test: 1. Apply this patch 2. Run tests perl t/db_dependent/api/v1/patrons.t 3. Follow the original test plan and observe it still applying --- Koha/Exceptions.pm | 4 ++ Koha/Exceptions/Category.pm | 17 +++++ Koha/Exceptions/Library.pm | 17 +++++ Koha/Exceptions/Patron.pm | 17 +++++ Koha/Patron.pm | 59 +++++++++++++++++ Koha/REST/V1/Patron.pm | 136 +++++++++++++++------------------------- t/db_dependent/Patrons.t | 30 ++++++++- t/db_dependent/api/v1/patrons.t | 6 +- 8 files changed, 197 insertions(+), 89 deletions(-) create mode 100644 Koha/Exceptions/Category.pm create mode 100644 Koha/Exceptions/Library.pm create mode 100644 Koha/Exceptions/Patron.pm diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm index dd8647b..fb671fd 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -21,6 +21,10 @@ use Exception::Class ( isa => 'Koha::Exceptions::Exception', description => 'A required parameter is missing' }, + 'Koha::Exceptions::UnblessedReference' => { + isa => 'Koha::Exceptions::Exception', + description => 'Calling unblessed reference' + }, # Virtualshelves exceptions 'Koha::Exceptions::Virtualshelves::DuplicateObject' => { isa => 'Koha::Exceptions::DuplicateObject', diff --git a/Koha/Exceptions/Category.pm b/Koha/Exceptions/Category.pm new file mode 100644 index 0000000..3487faa --- /dev/null +++ b/Koha/Exceptions/Category.pm @@ -0,0 +1,17 @@ +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 new file mode 100644 index 0000000..1aea6ad --- /dev/null +++ b/Koha/Exceptions/Library.pm @@ -0,0 +1,17 @@ +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 new file mode 100644 index 0000000..c409b4e --- /dev/null +++ b/Koha/Exceptions/Patron.pm @@ -0,0 +1,17 @@ +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 b5449a3..adf59e6 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -2,6 +2,7 @@ package Koha::Patron; # Copyright ByWater Solutions 2014 # Copyright PTFS Europe 2016 +# Copyright Koha-Suomi Oy 2016 # # This file is part of Koha. # @@ -26,7 +27,12 @@ use C4::Context; use C4::Log; use Koha::Database; use Koha::DateUtils; +use Koha::Exceptions; +use Koha::Exceptions::Category; +use Koha::Exceptions::Library; +use Koha::Exceptions::Patron; use Koha::Issues; +use Koha::Libraries; use Koha::OldIssues; use Koha::Patron::Categories; use Koha::Patron::Images; @@ -267,6 +273,59 @@ sub track_login { $self->lastseen( dt_from_string() )->store; } +=head3 validate + +my $patron = $patron->new($body)->validate->store; + $patron = $patron->set($body)->validate->store; + +Validates Koha::Patron object and throws Koha::Exceptions if validation issues +are found. + +Throws Koha::Exceptions::UnblessedReference if validate() is called + on an unblessed reference. +Throws Koha::Exceptions::Patron::DuplicateObject if trying to add a duplicate. +Throws Koha::Exceptions::Library::BranchcodeNotFound if branchcode is not found. +Throws Koha::Exceptions::Category::CategorycodeNotFound if categorycode is not + found. + +=cut + +sub validate { + my ($self) = @_; + + Koha::Exceptions::UnblessedReference->throw( + error => 'validate() must be called on a blessed Koha::Patron object' + ) unless $self && ref($self) eq "Koha::Patron"; + + # patron cardnumber and/or userid unique? + if ($self->cardnumber || $self->userid) { + my $patron = Koha::Patrons->find({cardnumber => $self->cardnumber, userid => $self->userid}); + if ($patron && (!$self->in_storage || $self->in_storage + && $self->borrowernumber ne $patron->borrowernumber)) { + Koha::Exceptions::Patron::DuplicateObject->throw( + error => "Patron cardnumber and userid must be unique", + conflict => { cardnumber => $patron->cardnumber, userid => $patron->userid } + ); + } + } + + my $branch = Koha::Libraries->find({branchcode => $self->branchcode }); + unless ($branch) { + Koha::Exceptions::Library::BranchcodeNotFound->throw( + error => "Library does not exist", + branchcode => $self->branchcode, + ); + } + my $category = Koha::Patron::Categories->find({ categorycode => $self->categorycode }); + unless ($category) { + Koha::Exceptions::Category::CategorycodeNotFound->throw( + error => "Patron category does not exist", + categorycode => $self->categorycode, + ); + } + return $self; +} + =head3 type =cut diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm index 1e2f4f8..67755ac 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -23,6 +23,9 @@ use Koha::Patrons; use Koha::Patron::Categories; use Koha::Libraries; +use Scalar::Util qw(blessed); +use Try::Tiny; + sub list { my ($c, $args, $cb) = @_; @@ -55,104 +58,69 @@ sub get { sub add { my ($c, $args, $cb) = @_; - my $body = $c->req->json; + try { + my $body = $c->req->json; - # patron cardnumber and/or userid unique? - if ($body->{cardnumber} || $body->{userid}) { - my $patron = Koha::Patrons->find({cardnumber => $body->{cardnumber}, userid => $body->{userid} }); - if ($patron) { - return $c->$cb({ - error => "Patron cardnumber and userid must be unique", - conflict => { cardnumber => $patron->cardnumber, userid => $patron->userid } - }, 409); - } - } + if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given - my $branch = Koha::Libraries->find({branchcode => $body->{branchcode} }); - unless ($branch) { - return $c->$cb({error => "Library with branchcode \"" . $body->{branchcode} . "\" does not exist"}, 404); + my $patron = Koha::Patron->new($body)->validate->store; + return $c->$cb($patron->unblessed, 201); } - my $category = Koha::Patron::Categories->find({ categorycode => $body->{categorycode} }); - unless ($category) { - return $c->$cb({error => "Patron category \"" . $body->{categorycode} . "\" does not exist"}, 404); - } - # All OK - save new patron - - if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given - - my $patron = eval { - Koha::Patron->new($body)->store; + catch { + unless (blessed $_ && $_->can('rethrow')) { + return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500); + } + if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) { + return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409); + } + elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) { + return $c->$cb({ error => "Library with branchcode \"".$_->branchcode."\" does not exist" }, 404); + } + elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) { + return $c->$cb({error => "Patron category \"".$_->categorycode."\" does not exist"}, 404); + } + else { + return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500); + } }; - - unless ($patron) { - return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500); - } - - return $c->$cb($patron->unblessed, 201); } sub edit { my ($c, $args, $cb) = @_; - my $patron = Koha::Patrons->find($args->{borrowernumber}); - unless ($patron) { - return $c->$cb({error => "Patron not found"}, 404); - } + my $patron; + try { + $patron = Koha::Patrons->find($args->{borrowernumber}); + my $body = $c->req->json; - my $body = $c->req->json; - - # Can we change userid and/or cardnumber? in that case check that they are altered first - if ($body->{cardnumber} || $body->{userid}) { - if ( ($body->{cardnumber} && $body->{cardnumber} ne $patron->cardnumber) || ($body->{userid} && $body->{userid} ne $patron->userid) ) { - my $conflictingPatron = Koha::Patrons->find({cardnumber => $body->{cardnumber}, userid => $body->{userid} }); - if ($conflictingPatron) { - return $c->$cb({ - error => "Patron cardnumber and userid must be unique", - conflict => { cardnumber => $conflictingPatron->cardnumber, userid => $conflictingPatron->userid } - }, 409); - } - } - } + if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given - if ($body->{branchcode}) { - my $branch = Koha::Libraries->find({branchcode => $body->{branchcode} }); - unless ($branch) { - return $c->$cb({error => "Library with branchcode \"" . $body->{branchcode} . "\" does not exist"}, 404); - } - } + die unless $patron->set($body)->validate; - if ($body->{categorycode}) { - my $category = Koha::Patron::Categories->find({ categorycode => $body->{categorycode} }); - unless ($category) { - return $c->$cb({error => "Patron category \"" . $body->{categorycode} . "\" does not exist"}, 404); - } + return $c->$cb({}, 204) unless $patron->is_changed; # No Content = No changes made + $patron->store; + return $c->$cb($patron->unblessed, 200); } - # ALL OK - Update patron - # Perhaps limit/validate what should be updated here? flags, et.al. - if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given - - my $updatedpatron = eval { - $patron->set($body); - }; - - if ($updatedpatron) { - if ($updatedpatron->is_changed) { - - my $res = eval { - $updatedpatron->store; - }; - - unless ($res) { - return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500); - } - return $c->$cb($res->unblessed, 200); - - } else { - return $c->$cb({}, 204); # No Content = No changes made + catch { + unless ($patron) { + return $c->$cb({error => "Patron not found"}, 404); } - } else { - return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500); - } + unless (blessed $_ && $_->can('rethrow')) { + return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500); + } + if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) { + return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409); + } + elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) { + return $c->$cb({ error => "Library with branchcode \"".$_->branchcode."\" does not exist" }, 404); + } + elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) { + return $c->$cb({error => "Patron category \"".$_->categorycode."\" does not exist"}, 404); + } + else { + return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500); + } + }; } sub delete { diff --git a/t/db_dependent/Patrons.t b/t/db_dependent/Patrons.t index 24707df..0dddbbd 100755 --- a/t/db_dependent/Patrons.t +++ b/t/db_dependent/Patrons.t @@ -17,8 +17,10 @@ use Modern::Perl; -use Test::More tests => 17; +use Test::More tests => 22; use Test::Warn; +use Test::Fatal; +use t::lib::TestBuilder; use C4::Context; use Koha::Database; @@ -36,6 +38,7 @@ $dbh->{RaiseError} = 1; $dbh->do("DELETE FROM issues"); $dbh->do("DELETE FROM borrowers"); +my $builder = t::lib::TestBuilder->new(); my $categorycode = Koha::Database->new()->schema()->resultset('Category')->first() ->categorycode(); @@ -46,7 +49,7 @@ my $b1 = Koha::Patron->new( { surname => 'Test 1', branchcode => $branchcode, - categorycode => $categorycode + categorycode => $categorycode, } ); $b1->store(); @@ -98,6 +101,29 @@ is( $b->surname(), 'Test 3', "Next returns third patron" ); $b = $patrons->next(); is( $b, undef, "Next returns undef" ); +is($b1->validate, $b1, "Patron b1 validates"); +isa_ok(exception { $b1->set({ categorycode => "nonexistent" })->validate }, + "Koha::Exceptions::Category::CategorycodeNotFound", + "Invalid categorycode"); +isa_ok(exception { $b1->set({ branchcode => "nonexistent" })->validate }, + "Koha::Exceptions::Library::BranchcodeNotFound", + "Invalid branchcode"); + +# test validation for duplicate cardnumber & userid +my $patron = $builder->build({ + source => 'Borrower', + value => { + branchcode => $branchcode, + categorycode => $categorycode, + flags => 0, + } +}); +my $duplicate = $patron; +isa_ok(exception { Koha::Patron->new($duplicate)->validate }, + "Koha::Exceptions::Patron::DuplicateObject", + "Duplicate patron"); + + # Test Reset and iteration in concert $patrons->reset(); foreach my $b ( $patrons->as_list() ) { diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index f03f36d..f1d8f84 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -186,7 +186,7 @@ $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(500) - ->json_is('/error' => "Something went wrong, check Koha logs for details"); + ->json_is('/error' => "Something went wrong, check Koha logs for details."); delete $newpatron->{ falseproperty }; $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron); @@ -213,7 +213,7 @@ $t->request_ok($tx) ->status_is(404) ->json_has('/error', 'Fails when trying to PUT nonexistent patron'); -$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => {categorycode => "nonexistent"}); +$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => {branchcode => $branchcode, categorycode => "nonexistent"}); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(404) @@ -230,7 +230,7 @@ $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(500) - ->json_is('/error' => "Something went wrong, check Koha logs for details"); + ->json_is('/error' => "Something went wrong, check Koha logs for details."); delete $newpatron->{ falseproperty }; $newpatron->{ cardnumber } = $patron-> { cardnumber }; -- 1.9.1