From d76e23eea2694975c6c7d96a3651f51d32f0afe5 Mon Sep 17 00:00:00 2001 From: Benjamin Rokseth Date: Wed, 27 Apr 2016 13:47:04 +0000 Subject: [PATCH] Bug 16330: Add routes to add, update and delete patrons This patch adds support for add, edit and delete patrons via REST API. GET /api/v1/patrons Get patron list from params GET /api/v1/patrons/ Get single patron POST /api/v1/patrons Create a new patron PUT /api/v1/patrons/ Update data about patron DEL /api/v1/patrons/ Delete a patron Revised Test plan: 1) Apply this patch 2) Run tests perl t/db_dependent/api/v1/patrons.t 3) Add a user with proper rights to use the REST API 4) play with your favourite REST client (curl/httpie, etc.): Authenticate with the user created above and get a CGISESSION id. Use the CGISESSION to add, edit and delete patrons via the API. 5) Use PUT /patrons/ for a patron without borrowers flag. This should go into pending patron modification status and needs to be accepted by a librarian. Please note there is no validation of body input in PUT/POST other than branchcode,category,userid,cardnumber. Signed-off-by: Tomas Cohen Arazi Signed-off-by: Benjamin Rokseth Signed-off-by: Josef Moravec --- Koha/Exceptions.pm | 8 +- Koha/Exceptions/Category.pm | 17 + Koha/Exceptions/Library.pm | 17 + Koha/Exceptions/Patron.pm | 17 + Koha/Patron.pm | 127 ++++++ Koha/REST/V1/Patron.pm | 166 +++++++- api/v1/swagger/definitions/patron.json | 14 +- api/v1/swagger/paths/patrons.json | 708 ++++++++++++++++++++++++++++++++- t/db_dependent/Koha/Patrons.t | 94 +++++ t/db_dependent/api/v1/patrons.t | 494 ++++++++++++++++++----- 10 files changed, 1547 insertions(+), 115 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 9d7f397..f49a289 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -10,8 +10,8 @@ use Exception::Class ( }, 'Koha::Exceptions::BadParameter' => { isa => 'Koha::Exceptions::Exception', - description => 'Bad parameter was given', - fields => ["parameter"], + description => 'A bad parameter was given', + fields => ['parameter'], }, 'Koha::Exceptions::DuplicateObject' => { isa => 'Koha::Exceptions::Exception', @@ -29,6 +29,10 @@ use Exception::Class ( isa => 'Koha::Exceptions::Exception', description => 'A required parameter is missing' }, + 'Koha::Exceptions::NoChanges' => { + isa => 'Koha::Exceptions::Exception', + description => 'No changes were made', + }, 'Koha::Exceptions::WrongParameter' => { isa => 'Koha::Exceptions::Exception', description => 'One or more parameters are wrong', 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 d0be998..faf4efa 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 2017 # # This file is part of Koha. # @@ -30,6 +31,11 @@ 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; @@ -831,6 +837,18 @@ sub is_child { return $self->category->category_type eq 'C' ? 1 : 0; } +=head3 store + +=cut + +sub store { + my ($self) = @_; + + # $self->_validate(); + + return $self->SUPER::store(); +} + =head3 type =cut @@ -839,6 +857,115 @@ 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/Patron.pm b/Koha/REST/V1/Patron.pm index 8fbe458..763b325 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -19,15 +19,38 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; +use C4::Members qw( AddMember ModMember ); +use Koha::AuthUtils qw(hash_password); use Koha::Patrons; +use Koha::Patron::Categories; +use Koha::Patron::Modifications; +use Koha::Libraries; + +use Scalar::Util qw(blessed); +use Try::Tiny; sub list { my $c = shift->openapi->valid_input or return; - # FIXME The limited does not work here, the userenv is not set - my $patrons = Koha::Patrons->search_limited; - return $c->render(status => 200, openapi => $patrons); + my $args = $c->req->params->to_hash; + my $filter = {}; + for my $filter_param ( keys %$args ) { + $filter->{$filter_param} = { LIKE => $args->{$filter_param} . "%" }; + } + + return try { + my $patrons = Koha::Patrons->search_limited($filter); + return $c->render(status => 200, openapi => $patrons); + } + catch { + if ( $_->isa('DBIx::Class::Exception') ) { + return $c->render( status => 500, openapi => { error => $_->{msg} } ); + } + else { + return $c->render( status => 500, openapi => { error => "Something went wrong, check the logs." } ); + } + }; } sub get { @@ -35,6 +58,7 @@ sub get { my $borrowernumber = $c->validation->param('borrowernumber'); my $patron = Koha::Patrons->find($borrowernumber); + unless ($patron) { return $c->render(status => 404, openapi => { error => "Patron not found." }); } @@ -42,4 +66,140 @@ sub get { return $c->render(status => 200, openapi => $patron); } +sub add { + my ($c, $args, $cb) = @_; + + return try { + my $body = $c->req->json; + + Koha::Patron->new($body)->_validate; + # TODO: Use AddMember until it has been moved to Koha-namespace + my $borrowernumber = AddMember(%$body); + my $patron = Koha::Patrons->find($borrowernumber); + + return $c->$cb($patron, 201); + } + 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 => "Given branchcode does not exist" }, 400); + } + elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) { + return $c->$cb({ error => "Given categorycode does not exist"}, 400); + } + else { + return $c->$cb({ error => + "Something went wrong, check Koha logs for details."}, 500); + } + }; +} + +sub edit { + my ($c, $args, $cb) = @_; + + my $patron; + return try { + my $user = $c->stash('koha.user'); + $patron = Koha::Patrons->find($args->{borrowernumber}); + my $body = $c->req->json; + + $body->{borrowernumber} = $args->{borrowernumber}; + + if (!C4::Auth::haspermission($user->userid, { borrowers => 1 }) && + $user->borrowernumber == $patron->borrowernumber){ + if (C4::Context->preference('OPACPatronDetails')) { + $body = _delete_unmodifiable_parameters($body); + die unless $patron->set($body)->_validate; + my $m = Koha::Patron::Modification->new($body)->store(); + return $c->$cb({}, 202); + } else { + return $c->$cb({ error => "You need a permission to change" + ." Your personal details"}, 403); + } + } + else { + delete $body->{borrowernumber}; + die unless $patron->set($body)->_validate; + # TODO: Use ModMember until it has been moved to Koha-namespace + $body->{borrowernumber} = $args->{borrowernumber}; + die unless ModMember(%$body); + return $c->$cb($patron, 200); + } + } + catch { + unless ($patron) { + return $c->$cb({error => "Patron not found"}, 404); + } + 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 => "Given branchcode does not exist" }, 400); + } + elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) { + return $c->$cb({ error => "Given categorycode does not exist"}, 400); + } + elsif ($_->isa('Koha::Exceptions::MissingParameter')) { + return $c->$cb({error => "Missing mandatory parameter(s)", + parameters => $_->parameter }, 400); + } + elsif ($_->isa('Koha::Exceptions::BadParameter')) { + return $c->$cb({error => "Invalid parameter(s)", + parameters => $_->parameter }, 400); + } + elsif ($_->isa('Koha::Exceptions::NoChanges')) { + return $c->$cb({error => "No changes have been made"}, 204); + } + else { + return $c->$cb({ error => + "Something went wrong, check Koha logs for details."}, 500); + } + }; +} + +sub delete { + my ($c, $args, $cb) = @_; + + my $patron; + + return try { + $patron = Koha::Patrons->find($args->{borrowernumber}); + # check if loans, reservations, debarrment, etc. before deletion! + my $res = $patron->delete; + + return $c->$cb({}, 200); + } + 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); + } + }; +} + +sub _delete_unmodifiable_parameters { + my ($body) = @_; + + my %columns = map { $_ => 1 } Koha::Patron::Modifications->columns; + foreach my $param (keys %$body) { + unless (exists $columns{$param}) { + delete $body->{$param}; + } + } + return $body; +} + 1; diff --git a/api/v1/swagger/definitions/patron.json b/api/v1/swagger/definitions/patron.json index 2c6f9a6..948b956 100644 --- a/api/v1/swagger/definitions/patron.json +++ b/api/v1/swagger/definitions/patron.json @@ -121,6 +121,7 @@ }, "dateofbirth": { "type": ["string", "null"], + "format": "date", "description": "patron's date of birth" }, "branchcode": { @@ -133,10 +134,12 @@ }, "dateenrolled": { "type": ["string", "null"], + "format": "date", "description": "date the patron was added to Koha" }, "dateexpiry": { "type": ["string", "null"], + "format": "date", "description": "date the patron's card is set to expire" }, "date_renewed": { @@ -153,6 +156,7 @@ }, "debarred": { "type": ["string", "null"], + "format": "date", "description": "until this date the patron can only check-in" }, "debarredcomment": { @@ -272,15 +276,17 @@ "description": "produce a warning for this patron if this item has previously been checked out to this patron if 'yes', not if 'no', defer to category setting if 'inherit'" }, "updated_on": { - "type": ["string", "null"], + "type": "string", + "format": "date-time", "description": "time of last change could be useful for synchronization with external systems (among others)" }, "lastseen": { "type": ["string", "null"], + "format": "date-time", "description": "last time a patron has been seen (connected at the OPAC or staff interface)" }, "lang": { - "type": ["string", "null"], + "type": "string", "description": "lang to use to send notices to this patron" }, "login_attempts": { @@ -291,5 +297,7 @@ "type": ["string", "null"], "description": "persist OverDrive auth token" } - } + }, + "additionalProperties": false, + "required": ["surname", "address", "city", "branchcode", "categorycode"] } diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index 565d20c..56218a4 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -7,6 +7,517 @@ "produces": [ "application/json" ], + "parameters": [{ + "name": "borrowernumber", + "in": "query", + "description": "Case insensetive 'starts_with' search on borrowernumber", + "required": false, + "type": "string" + }, + { + "name": "cardnumber", + "in": "query", + "description": "Case insensetive 'starts_with' search on cardnumber", + "required": false, + "type": "string" + }, + { + "name": "surname", + "in": "query", + "description": "Case insensetive 'starts_with' search on surname", + "required": false, + "type": "string" + }, + { + "name": "firstname", + "in": "query", + "description": "Case insensetive 'starts_with' search on firstname", + "required": false, + "type": "string" + }, + { + "name": "title", + "in": "query", + "description": "Case insensetive 'starts_with' search on title", + "required": false, + "type": "string" + }, + { + "name": "othernames", + "in": "query", + "description": "Case insensetive 'starts_with' search on othernames", + "required": false, + "type": "string" + }, + { + "name": "initials", + "in": "query", + "description": "Case insensetive 'starts_with' search on initials", + "required": false, + "type": "string" + }, + { + "name": "streetnumber", + "in": "query", + "description": "Case insensetive 'starts_with' search on streetnumber", + "required": false, + "type": "string" + }, + { + "name": "streettype", + "in": "query", + "description": "Case insensetive 'starts_with' search on streettype", + "required": false, + "type": "string" + }, + { + "name": "address", + "in": "query", + "description": "Case insensetive 'starts_with' search on address", + "required": false, + "type": "string" + }, + { + "name": "address2", + "in": "query", + "description": "Case insensetive 'starts_with' search on address2", + "required": false, + "type": "string" + }, + { + "name": "city", + "in": "query", + "description": "Case insensetive 'starts_with' search on city", + "required": false, + "type": "string" + }, + { + "name": "state", + "in": "query", + "description": "Case insensetive 'starts_with' search on state", + "required": false, + "type": "string" + }, + { + "name": "zipcode", + "in": "query", + "description": "Case insensetive 'starts_with' search on zipcode", + "required": false, + "type": "string" + }, + { + "name": "country", + "in": "query", + "description": "Case insensetive 'starts_with' search on country", + "required": false, + "type": "string" + }, + { + "name": "email", + "in": "query", + "description": "Case insensetive 'starts_with' search on email", + "required": false, + "type": "string" + }, + { + "name": "phone", + "in": "query", + "description": "Case insensetive 'starts_with' search on phone", + "required": false, + "type": "string" + }, + { + "name": "mobile", + "in": "query", + "description": "Case insensetive 'starts_with' search on mobile", + "required": false, + "type": "string" + }, + { + "name": "fax", + "in": "query", + "description": "Case insensetive 'starts_with' search on fax", + "required": false, + "type": "string" + }, + { + "name": "emailpro", + "in": "query", + "description": "Case insensetive 'starts_with' search on emailpro", + "required": false, + "type": "string" + }, + { + "name": "phonepro", + "in": "query", + "description": "Case insensetive 'starts_with' search on phonepro", + "required": false, + "type": "string" + }, + { + "name": "B_streetnumber", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_streetnumber", + "required": false, + "type": "string" + }, + { + "name": "B_streettype", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_streettype", + "required": false, + "type": "string" + }, + { + "name": "B_address", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_address", + "required": false, + "type": "string" + }, + { + "name": "B_address2", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_address2", + "required": false, + "type": "string" + }, + { + "name": "B_city", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_city", + "required": false, + "type": "string" + }, + { + "name": "B_state", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_state", + "required": false, + "type": "string" + }, + { + "name": "B_zipcode", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_zipcode", + "required": false, + "type": "string" + }, + { + "name": "B_country", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_country", + "required": false, + "type": "string" + }, + { + "name": "B_email", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_email", + "required": false, + "type": "string" + }, + { + "name": "B_phone", + "in": "query", + "description": "Case insensetive 'starts_with' search on B_phone", + "required": false, + "type": "string" + }, + { + "name": "dateofbirth", + "in": "query", + "description": "Case insensetive 'starts_with' search on dateofbirth", + "required": false, + "type": "string" + }, + { + "name": "branchcode", + "in": "query", + "description": "Case insensetive 'starts_with' search on branchcode", + "required": false, + "type": "string" + }, + { + "name": "categorycode", + "in": "query", + "description": "Case insensetive 'starts_with' search on categorycode", + "required": false, + "type": "string" + }, + { + "name": "dateenrolled", + "in": "query", + "description": "Case insensetive 'starts_with' search on dateenrolled", + "required": false, + "type": "string" + }, + { + "name": "dateexpiry", + "in": "query", + "description": "Case insensetive 'starts_with' search on dateexpiry", + "required": false, + "type": "string" + }, + { + "name": "gonenoaddress", + "in": "query", + "description": "Case insensetive 'starts_with' search on gonenoaddress", + "required": false, + "type": "string" + }, + { + "name": "lost", + "in": "query", + "description": "Case insensetive 'starts_with' search on lost", + "required": false, + "type": "string" + }, + { + "name": "debarred", + "in": "query", + "description": "Case insensetive 'starts_with' search on debarred", + "required": false, + "type": "string" + }, + { + "name": "debarredcomment", + "in": "query", + "description": "Case insensetive 'starts_with' search on debarredcomment", + "required": false, + "type": "string" + }, + { + "name": "contactname", + "in": "query", + "description": "Case insensetive 'starts_with' search on contactname", + "required": false, + "type": "string" + }, + { + "name": "contactfirstname", + "in": "query", + "description": "Case insensetive 'starts_with' search on contactfirstname", + "required": false, + "type": "string" + }, + { + "name": "contacttitle", + "in": "query", + "description": "Case insensetive 'starts_with' search on contacttitle", + "required": false, + "type": "string" + }, + { + "name": "guarantorid", + "in": "query", + "description": "Case insensetive 'starts_with' search on guarantorid", + "required": false, + "type": "string" + }, + { + "name": "borrowernotes", + "in": "query", + "description": "Case insensetive 'starts_with' search on borrowernotes", + "required": false, + "type": "string" + }, + { + "name": "relationship", + "in": "query", + "description": "Case insensetive 'starts_with' search on relationship", + "required": false, + "type": "string" + }, + { + "name": "sex", + "in": "query", + "description": "Case insensetive 'starts_with' search on sex", + "required": false, + "type": "string" + }, + { + "name": "password", + "in": "query", + "description": "Case insensetive 'starts_with' search on password", + "required": false, + "type": "string" + }, + { + "name": "flags", + "in": "query", + "description": "Case insensetive 'starts_with' search on flags", + "required": false, + "type": "string" + }, + { + "name": "userid", + "in": "query", + "description": "Case insensetive 'starts_with' search on userid", + "required": false, + "type": "string" + }, + { + "name": "opacnote", + "in": "query", + "description": "Case insensetive 'starts_with' search on opacnote", + "required": false, + "type": "string" + }, + { + "name": "contactnote", + "in": "query", + "description": "Case insensetive 'starts_with' search on contactnote", + "required": false, + "type": "string" + }, + { + "name": "sort1", + "in": "query", + "description": "Case insensetive 'starts_with' search on sort1", + "required": false, + "type": "string" + }, + { + "name": "sort2", + "in": "query", + "description": "Case insensetive 'starts_with' search on sort2", + "required": false, + "type": "string" + }, + { + "name": "altcontactfirstname", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactfirstname", + "required": false, + "type": "string" + }, + { + "name": "altcontactsurname", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactsurname", + "required": false, + "type": "string" + }, + { + "name": "altcontactaddress1", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactaddress1", + "required": false, + "type": "string" + }, + { + "name": "altcontactaddress2", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactaddress2", + "required": false, + "type": "string" + }, + { + "name": "altcontactaddress3", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactaddress3", + "required": false, + "type": "string" + }, + { + "name": "altcontactstate", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactstate", + "required": false, + "type": "string" + }, + { + "name": "altcontactzipcode", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactzipcode", + "required": false, + "type": "string" + }, + { + "name": "altcontactcountry", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactcountry", + "required": false, + "type": "string" + }, + { + "name": "altcontactphone", + "in": "query", + "description": "Case insensetive 'starts_with' search on altcontactphone", + "required": false, + "type": "string" + }, + { + "name": "smsalertnumber", + "in": "query", + "description": "Case insensetive 'starts_with' search on smsalertnumber", + "required": false, + "type": "string" + }, + { + "name": "sms_provider_id", + "in": "query", + "description": "Case insensetive 'starts_with' search on sms_provider_id", + "required": false, + "type": "string" + }, + { + "name": "privacy", + "in": "query", + "description": "Case insensetive 'starts_with' search on privacy", + "required": false, + "type": "string" + }, + { + "name": "privacy_guarantor_checkouts", + "in": "query", + "description": "Case insensetive 'starts_with' search on privacy_guarantor_checkouts", + "required": false, + "type": "string" + }, + { + "name": "checkprevcheckout", + "in": "query", + "description": "Case insensetive 'starts_with' search on checkprevcheckout", + "required": false, + "type": "string" + }, + { + "name": "updated_on", + "in": "query", + "description": "Case insensetive 'starts_with' search on updated_on", + "required": false, + "type": "string" + }, + { + "name": "lastseen", + "in": "query", + "description": "Case insensetive 'starts_with' search on lastseen", + "required": false, + "type": "string" + }, + { + "name": "lang", + "in": "query", + "description": "Case insensetive 'starts_with' search on lang", + "required": false, + "type": "string" + }, + { + "name": "login_attempts", + "in": "query", + "description": "Case insensetive 'starts_with' search on login_attempts", + "required": false, + "type": "string" + }, + { + "name": "overdrive_auth_token", + "in": "query", + "description": "Case insensetive 'starts_with' search on overdrive_auth_token", + "required": false, + "type": "string" + }], "responses": { "200": { "description": "A list of patrons", @@ -34,12 +545,78 @@ "schema": { "$ref": "../definitions.json#/error" } + } + }, + "x-koha-authorization": { + "permissions": { + "borrowers": "1" + } + } + }, + "post": { + "operationId": "addPatron", + "tags": ["patrons"], + "parameters": [{ + "name": "body", + "in": "body", + "description": "A JSON object containing information about the new patron", + "required": true, + "schema": { + "$ref": "../definitions.json#/patron" + } + }], + "consumes": ["application/json"], + "produces": ["application/json"], + "responses": { + "201": { + "description": "A successfully created patron", + "schema": { + "items": { + "$ref": "../definitions.json#/patron" + } + } + }, + "400": { + "description": "Bad parameter", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "401": { + "description": "Authentication required", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "404": { + "description": "Resource not found", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "409": { + "description": "Conflict in creating resource", + "schema": { + "$ref": "../definitions.json#/error" + } }, "503": { "description": "Under maintenance", "schema": { "$ref": "../definitions.json#/error" } + }, + "500": { + "description": "Internal server error", + "schema": { + "$ref": "../definitions.json#/error" + } } }, "x-koha-authorization": { @@ -55,11 +632,10 @@ "operationId": "getPatron", "tags": ["patrons"], "parameters": [{ - "$ref": "../parameters.json#/borrowernumberPathParam" - } - ], + "$ref": "../parameters.json#/borrowernumberPathParam" + }], "produces": [ - "application/json" + "application/json" ], "responses": { "200": { @@ -106,6 +682,130 @@ "borrowers": "edit_borrowers" } } + }, + "put": { + "operationId": "editPatron", + "tags": ["patrons"], + "parameters": [ + { + "$ref": "../parameters.json#/borrowernumberPathParam" + }, + { + "name": "body", + "in": "body", + "description": "A JSON object containing new information about existing patron", + "required": true, + "schema": { + "$ref": "../definitions.json#/patron" + } + } + ], + "consumes": ["application/json"], + "produces": ["application/json"], + "responses": { + "200": { + "description": "A successfully updated patron", + "schema": { + "items": { + "$ref": "../definitions.json#/patron" + } + } + }, + "202": { + "description": "Accepted and waiting for librarian verification", + "schema": { + "type": "object" + } + }, + "204": { + "description": "No Content", + "schema": { + "type": "object" + } + }, + "400": { + "description": "Bad parameter", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "404": { + "description": "Resource not found", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "409": { + "description": "Conflict in updating resource", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "500": { + "description": "Internal server error", + "schema": { + "$ref": "../definitions.json#/error" + } + } + }, + "x-koha-authorization": { + "allow-owner": true, + "allow-guarantor": true, + "permissions": { + "borrowers": "1" + } + } + }, + "delete": { + "operationId": "deletePatron", + "tags": ["patrons"], + "parameters": [{ + "$ref": "../parameters.json#/borrowernumberPathParam" + }], + "produces": ["application/json"], + "responses": { + "200": { + "description": "Patron deleted successfully", + "schema": { + "type": "object" + } + }, + "400": { + "description": "Patron deletion failed", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "401": { + "description": "Authentication required", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "404": { + "description": "Patron not found", + "schema": { + "$ref": "../definitions.json#/error" + } + } + }, + "x-koha-authorization": { + "permissions": { + "borrowers": "1" + } + } } } } diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index b5a36a3..0b51a33 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1134,3 +1134,97 @@ 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 2166547..6389b5c 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -17,122 +17,410 @@ use Modern::Perl; -use Test::More tests => 21; +use Test::More tests => 5; use Test::Mojo; +use Test::Warn; + use t::lib::TestBuilder; use t::lib::Mocks; use C4::Auth; -use C4::Context; - +use Koha::Cities; use Koha::Database; -use Koha::Patron; my $schema = Koha::Database->new->schema; -my $builder = t::lib::TestBuilder->new(); - -$schema->storage->txn_begin; +my $builder = t::lib::TestBuilder->new; # FIXME: sessionStorage defaults to mysql, but it seems to break transaction handling # this affects the other REST api tests t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' ); -$ENV{REMOTE_ADDR} = '127.0.0.1'; -my $t = Test::Mojo->new('Koha::REST::V1'); +my $remote_address = '127.0.0.1'; +my $t = Test::Mojo->new('Koha::REST::V1'); -my $categorycode = $builder->build({ source => 'Category' })->{ categorycode }; -my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode }; -my $guarantor = $builder->build({ - source => 'Borrower', - value => { - branchcode => $branchcode, - categorycode => $categorycode, - flags => 0, - } -}); -my $borrower = $builder->build({ - source => 'Borrower', - value => { - branchcode => $branchcode, - categorycode => $categorycode, - flags => 0, - lost => 1, - guarantorid => $guarantor->{borrowernumber}, - } -}); - -$t->get_ok('/api/v1/patrons') - ->status_is(401); - -$t->get_ok("/api/v1/patrons/" . $borrower->{ borrowernumber }) - ->status_is(401); - -my $session = C4::Auth::get_session(''); -$session->param('number', $borrower->{ borrowernumber }); -$session->param('id', $borrower->{ userid }); -$session->param('ip', '127.0.0.1'); -$session->param('lasttime', time()); -$session->flush; - -my $session2 = C4::Auth::get_session(''); -$session2->param('number', $guarantor->{ borrowernumber }); -$session2->param('id', $guarantor->{ userid }); -$session2->param('ip', '127.0.0.1'); -$session2->param('lasttime', time()); -$session2->flush; - -my $tx = $t->ua->build_tx(GET => '/api/v1/patrons'); -$tx->req->cookies({name => 'CGISESSID', value => $session->id}); -$t->request_ok($tx) - ->status_is(403); - -$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . ($borrower->{ borrowernumber }-1)); -$tx->req->cookies({name => 'CGISESSID', value => $session->id}); -$t->request_ok($tx) - ->status_is(403) - ->json_is('/required_permissions', {"borrowers" => "edit_borrowers"}); - -# User without permissions, but is the owner of the object -$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{borrowernumber}); -$tx->req->cookies({name => 'CGISESSID', value => $session->id}); -$t->request_ok($tx) - ->status_is(200); - -# User without permissions, but is the guarantor of the owner of the object -$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{borrowernumber}); -$tx->req->cookies({name => 'CGISESSID', value => $session2->id}); -$t->request_ok($tx) - ->status_is(200) - ->json_is('/guarantorid', $guarantor->{borrowernumber}); - -my $loggedinuser = $builder->build({ - source => 'Borrower', - value => { +subtest 'list() tests' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + unauthorized_access_tests('GET', undef, undef); + + subtest 'librarian access tests' => sub { + plan tests => 8; + + my ($borrowernumber, $sessionid) = create_user_and_session({ + authorized => 1 }); + my $patron = Koha::Patrons->find($borrowernumber); + Koha::Patrons->search({ + borrowernumber => { '!=' => $borrowernumber}, + cardnumber => { LIKE => $patron->cardnumber . "%" } + })->delete; + Koha::Patrons->search({ + borrowernumber => { '!=' => $borrowernumber}, + address2 => { LIKE => $patron->address2 . "%" } + })->delete; + + my $tx = $t->ua->build_tx(GET => '/api/v1/patrons'); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $t->request_ok($tx) + ->status_is(200); + + $tx = $t->ua->build_tx(GET => '/api/v1/patrons?cardnumber='. + $patron->cardnumber); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $t->request_ok($tx) + ->status_is(200) + ->json_is('/0/cardnumber' => $patron->cardnumber); + + $tx = $t->ua->build_tx(GET => '/api/v1/patrons?address2='. + $patron->address2); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $t->request_ok($tx) + ->status_is(200) + ->json_is('/0/address2' => $patron->address2); + }; + + $schema->storage->txn_rollback; +}; + +subtest 'get() tests' => sub { + plan tests => 3; + + $schema->storage->txn_begin; + + unauthorized_access_tests('GET', -1, undef); + + subtest 'access own object tests' => sub { + plan tests => 4; + + my ($patronid, $patronsessionid) = create_user_and_session({ + authorized => 0 }); + + # Access patron's own data even though they have no borrowers flag + my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$patronid"); + $tx->req->cookies({name => 'CGISESSID', value => $patronsessionid}); + $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $t->request_ok($tx) + ->status_is(200); + + my $guarantee = $builder->build({ + source => 'Borrower', + value => { + guarantorid => $patronid, + } + }); + + # Access guarantee's data even though guarantor has no borrowers flag + my $guaranteenumber = $guarantee->{borrowernumber}; + $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$guaranteenumber"); + $tx->req->cookies({name => 'CGISESSID', value => $patronsessionid}); + $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $t->request_ok($tx) + ->status_is(200); + }; + + subtest 'librarian access tests' => sub { + plan tests => 5; + + my ($patron_id) = create_user_and_session({ + authorized => 0 }); + my $patron = Koha::Patrons->find($patron_id); + my ($borrowernumber, $sessionid) = create_user_and_session({ + authorized => 1 }); + my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$patron_id"); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(200) + ->json_is('/borrowernumber' => $patron_id) + ->json_is('/surname' => $patron->surname) + ->json_is('/lost' => Mojo::JSON->false ); + }; + + $schema->storage->txn_rollback; +}; + +subtest 'add() tests' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $categorycode = $builder->build({ source => 'Category' })->{categorycode}; + my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; + my $newpatron = { + address => 'Street', branchcode => $branchcode, + cardnumber => $branchcode.$categorycode, categorycode => $categorycode, - flags => 16 # borrowers flag - } -}); - -$session = C4::Auth::get_session(''); -$session->param('number', $loggedinuser->{ borrowernumber }); -$session->param('id', $loggedinuser->{ userid }); -$session->param('ip', '127.0.0.1'); -$session->param('lasttime', time()); -$session->flush; - -$tx = $t->ua->build_tx(GET => '/api/v1/patrons'); -$tx->req->cookies({name => 'CGISESSID', value => $session->id}); -$tx->req->env({REMOTE_ADDR => '127.0.0.1'}); -$t->request_ok($tx) - ->status_is(200); - -$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{ borrowernumber }); -$tx->req->cookies({name => 'CGISESSID', value => $session->id}); -$t->request_ok($tx) - ->status_is(200) - ->json_is('/borrowernumber' => $borrower->{ borrowernumber }) - ->json_is('/surname' => $borrower->{ surname }) - ->json_is('/lost' => Mojo::JSON->true ); - -$schema->storage->txn_rollback; + city => 'Joenzoo', + surname => "TestUser", + userid => $branchcode.$categorycode, + }; + + unauthorized_access_tests('POST', undef, $newpatron); + + subtest 'librarian access tests' => sub { + plan tests => 18; + + my ($borrowernumber, $sessionid) = create_user_and_session({ + authorized => 1 }); + + $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"); + $newpatron->{branchcode} = $branchcode; + + $newpatron->{categorycode} = "nonexistent"; # Test invalid patron category + $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 categorycode does not exist"); + $newpatron->{categorycode} = $categorycode; + + $newpatron->{falseproperty} = "Non existent property"; + $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); + delete $newpatron->{falseproperty}; + + $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(201, 'Patron created successfully') + ->json_has('/borrowernumber', 'got a borrowernumber') + ->json_is('/cardnumber', $newpatron->{ cardnumber }) + ->json_is('/surname' => $newpatron->{ surname }) + ->json_is('/firstname' => $newpatron->{ firstname }); + $newpatron->{borrowernumber} = $tx->res->json->{borrowernumber}; + + $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 } + } + ); + }; + + $schema->storage->txn_rollback; +}; + +subtest 'edit() tests' => sub { + plan tests => 3; + + $schema->storage->txn_begin; + + unauthorized_access_tests('PUT', 123, {email => 'nobody@example.com'}); + + subtest 'patron modifying own data' => sub { + plan tests => 7; + + my ($borrowernumber, $sessionid) = create_user_and_session({ + authorized => 0 }); + my $patron = Koha::Patrons->find($borrowernumber)->TO_JSON; + + t::lib::Mocks::mock_preference("OPACPatronDetails", 0); + my $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $patron->{borrowernumber} => json => $patron); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(403, 'OPACPatronDetails off - modifications not allowed.'); + + t::lib::Mocks::mock_preference("OPACPatronDetails", 1); + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $patron->{borrowernumber} => json => $patron); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(204, 'Updating myself with my current data'); + + $patron->{'firstname'} = "noob"; + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $patron->{borrowernumber} => json => $patron); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(202, 'Updating myself with my current data'); + + # Approve changes + Koha::Patron::Modifications->find({ + borrowernumber => $patron->{borrowernumber}, + firstname => "noob" + })->approve; + is(Koha::Patrons->find({ + borrowernumber => $patron->{borrowernumber}})->firstname, + "noob", "Changes approved"); + }; + + subtest 'librarian access tests' => sub { + plan tests => 20; + + t::lib::Mocks::mock_preference('minPasswordLength', 1); + my ($borrowernumber, $sessionid) = create_user_and_session({ + authorized => 1 }); + my ($borrowernumber2, undef) = create_user_and_session({ + authorized => 0 }); + my $patron = Koha::Patrons->find($borrowernumber2); + my $newpatron = Koha::Patrons->find($borrowernumber2)->TO_JSON; + + my $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/-1" => + json => $newpatron); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(404) + ->json_has('/error', 'Fails when trying to PUT nonexistent patron'); + + $newpatron->{categorycode} = 'nonexistent'; + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $newpatron->{borrowernumber} => json => $newpatron + ); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(400) + ->json_is('/error' => "Given categorycode does not exist"); + $newpatron->{categorycode} = $patron->categorycode; + + $newpatron->{branchcode} = 'nonexistent'; + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $newpatron->{borrowernumber} => json => $newpatron + ); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(400) + ->json_is('/error' => "Given branchcode does not exist"); + $newpatron->{branchcode} = $patron->branchcode; + + $newpatron->{falseproperty} = "Non existent property"; + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $newpatron->{borrowernumber} => json => $newpatron); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(400) + ->json_is('/errors/0/message' => + 'Properties not allowed: falseproperty.'); + delete $newpatron->{falseproperty}; + + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $borrowernumber => 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_has('/conflict', { + cardnumber => $newpatron->{ cardnumber }, + userid => $newpatron->{ userid } + } + ); + + $newpatron->{ cardnumber } = $borrowernumber.$borrowernumber2; + $newpatron->{ userid } = "user".$borrowernumber.$borrowernumber2; + $newpatron->{ surname } = "user".$borrowernumber.$borrowernumber2; + + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . + $newpatron->{borrowernumber} => json => $newpatron); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(200, 'Patron updated successfully') + ->json_has($newpatron); + is(Koha::Patrons->find($newpatron->{borrowernumber})->cardnumber, + $newpatron->{ cardnumber }, 'Patron is really updated!'); + }; + + $schema->storage->txn_rollback; +}; + +subtest 'delete() tests' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + unauthorized_access_tests('DELETE', 123, undef); + + subtest 'librarian access test' => sub { + plan tests => 4; + + my ($borrowernumber, $sessionid) = create_user_and_session({ + authorized => 1 }); + my ($borrowernumber2, $sessionid2) = create_user_and_session({ + authorized => 0 }); + + my $tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/-1"); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(404, 'Patron not found'); + + $tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/$borrowernumber2"); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(200, 'Patron deleted successfully'); + }; + + $schema->storage->txn_rollback; +}; + +# Centralized tests for 401s and 403s assuming the endpoint requires +# borrowers flag for access +sub unauthorized_access_tests { + my ($verb, $patronid, $json) = @_; + + my $endpoint = '/api/v1/patrons'; + $endpoint .= ($patronid) ? "/$patronid" : ''; + + subtest 'unauthorized access tests' => sub { + plan tests => 5; + + my $tx = $t->ua->build_tx($verb => $endpoint => json => $json); + $t->request_ok($tx) + ->status_is(401); + + my ($borrowernumber, $sessionid) = create_user_and_session({ + authorized => 0 }); + + $tx = $t->ua->build_tx($verb => $endpoint => json => $json); + $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $t->request_ok($tx) + ->status_is(403) + ->json_is('/required_permissions', {"borrowers" => "1"}); + }; +} + +sub create_user_and_session { + + my $args = shift; + my $flags = ( $args->{authorized} ) ? 16 : 0; + my $dbh = C4::Context->dbh; + + my $user = $builder->build( + { + source => 'Borrower', + value => { + flags => $flags, + gonenoaddress => 0, + lost => 0, + email => 'nobody@example.com', + emailpro => 'nobody@example.com', + B_email => 'nobody@example.com', + } + } + ); + + # Create a session for the authorized user + my $session = C4::Auth::get_session(''); + $session->param( 'number', $user->{borrowernumber} ); + $session->param( 'id', $user->{userid} ); + $session->param( 'ip', '127.0.0.1' ); + $session->param( 'lasttime', time() ); + $session->flush; + + return ( $user->{borrowernumber}, $session->id ); +} -- 2.1.4