From 915c3c9b990dbc53c59bfee243b179e9f683e8e3 Mon Sep 17 00:00:00 2001 From: Benjamin Rokseth Date: Wed, 27 Apr 2016 13:47:04 +0000 Subject: [PATCH] Bug 16330: REST API: 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 (on top of dependent bug 14868) 2) Run minifySwagger to create swagger.min.json and restart Plack 3) Run tests perl t/db_dependent/api/v1/patrons.t 4) Add a user with proper rights to use the REST API 5) 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. Please note there is no validation of body input in PUT/POST other than branchcode,category,userid,cardnumber. Signed-off-by: Bernardo Gonzalez Kriegel On top of 15126 Tested using postman, first try with the api GET, POST, PUT and DELETE works, i.e. list, add, update, search and delete. Small errors fixed in followup. --- Koha/REST/V1/Patron.pm | 143 ++++++++++++++++++++++++++++++-- api/v1/swagger/paths/patrons.json | 169 +++++++++++++++++++++++++++++++++++++- t/db_dependent/api/v1/patrons.t | 168 +++++++++++++++++++++++++++++++++---- 3 files changed, 454 insertions(+), 26 deletions(-) diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm index a66dbb9..7c26184 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -19,15 +19,25 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; -use C4::Auth qw( haspermission ); +use Koha::AuthUtils qw(hash_password); use Koha::Patrons; +use Koha::Patron::Categories; +use Koha::Libraries; sub list { my ($c, $args, $cb) = @_; - my $user = $c->stash('koha.user'); - - my $patrons = Koha::Patrons->search; + my $params = $c->req->query_params->to_hash; + my $patrons; + if (keys %$params) { + my @valid_params = Koha::Patrons->columns; + foreach my $key (keys %$params) { + delete $params->{$key} unless grep { $key eq $_ } @valid_params; + } + $patrons = Koha::Patrons->search($params); + } else { + $patrons = Koha::Patrons->search; + } $c->$cb($patrons->unblessed, 200); } @@ -35,8 +45,6 @@ sub list { sub get { my ($c, $args, $cb) = @_; - my $user = $c->stash('koha.user'); - my $patron = Koha::Patrons->find($args->{borrowernumber}); unless ($patron) { return $c->$cb({error => "Patron not found"}, 404); @@ -45,4 +53,127 @@ sub get { return $c->$cb($patron->unblessed, 200); } +sub add { + my ($c, $args, $cb) = @_; + + 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); + } + } + + my $branch = Koha::Libraries->find({branchcode => $body->{branchcode} }); + unless ($branch) { + return $c->$cb({error => "Library with branchcode \"" . $body->{branchcode} . "\" does not exist"}, 404); + } + 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; + }; + + 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 $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->{branchcode}) { + my $branch = Koha::Libraries->find({branchcode => $body->{branchcode} }); + unless ($branch) { + return $c->$cb({error => "Library with branchcode \"" . $body->{branchcode} . "\" does not exist"}, 404); + } + } + + 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); + } + } + # 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 + } + } else { + return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500); + } +} + +sub delete { + my ($c, $args, $cb) = @_; + + my $patron = Koha::Patrons->find($args->{borrowernumber}); + unless ($patron) { + return $c->$cb({error => "Patron not found"}, 404); + } + + # check if loans, reservations, debarrment, etc. before deletion! + my $res = $patron->delete; + + if ($res eq '1') { + return $c->$cb({}, 200); + } elsif ($res eq '-1') { + return $c->$cb({}, 404); + } else { + return $c->$cb({}, 400); + } +} + 1; diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index 67632e1..39f340a 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -28,6 +28,60 @@ "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" + } + } + }, + "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" + } + }, + "500": { + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } + } + }, + "x-koha-authorization": { + "permissions": { + "borrowers": "1" + } + } } }, "/patrons/{borrowernumber}": { @@ -35,11 +89,10 @@ "operationId": "getPatron", "tags": ["patrons"], "parameters": [{ - "$ref": "../parameters.json#/borrowernumberPathParam" - } - ], + "$ref": "../parameters.json#/borrowernumberPathParam" + }], "produces": [ - "application/json" + "application/json" ], "responses": { "200": { @@ -68,6 +121,114 @@ "borrowers": "1" } } + }, + "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" + } + } + }, + "204": { + "description": "No Content", + "schema": { + "type": "object" + } + }, + "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 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" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "404": { + "description": "Patron not found", + "schema": { + "$ref": "../definitions.json#/error" + } + } + }, + "x-koha-authorization": { + "allow-owner": true, + "allow-guarantor": true, + "permissions": { + "borrowers": "1" + } + } } } } diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index f4b9410..716a2cd 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 20; +use Test::More tests => 74; use Test::Mojo; use t::lib::TestBuilder; @@ -25,17 +25,21 @@ use C4::Auth; use C4::Context; use Koha::Database; -use Koha::Patron; -my $builder = t::lib::TestBuilder->new(); +BEGIN { + use_ok('Koha::Object'); + use_ok('Koha::Patron'); +} +my $builder = t::lib::TestBuilder->new(); my $dbh = C4::Context->dbh; -$dbh->{AutoCommit} = 0; -$dbh->{RaiseError} = 1; +my $schema = Koha::Database->schema; $ENV{REMOTE_ADDR} = '127.0.0.1'; my $t = Test::Mojo->new('Koha::REST::V1'); +$schema->storage->txn_begin; + my $categorycode = $builder->build({ source => 'Category' })->{ categorycode }; my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode }; my $guarantor = $builder->build({ @@ -46,7 +50,7 @@ my $guarantor = $builder->build({ flags => 0, } }); -my $borrower = $builder->build({ +my $patron = $builder->build({ source => 'Borrower', value => { branchcode => $branchcode, @@ -59,12 +63,12 @@ my $borrower = $builder->build({ $t->get_ok('/api/v1/patrons') ->status_is(401); -$t->get_ok("/api/v1/patrons/" . $borrower->{ borrowernumber }) +$t->get_ok("/api/v1/patrons/" . $patron->{ borrowernumber }) ->status_is(401); my $session = C4::Auth::get_session(''); -$session->param('number', $borrower->{ borrowernumber }); -$session->param('id', $borrower->{ userid }); +$session->param('number', $patron->{ borrowernumber }); +$session->param('id', $patron->{ userid }); $session->param('ip', '127.0.0.1'); $session->param('lasttime', time()); $session->flush; @@ -81,20 +85,20 @@ $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 = $t->ua->build_tx(GET => "/api/v1/patrons/" . ($patron->{ borrowernumber }-1)); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(403) ->json_is('/required_permissions', {"borrowers" => "1"}); # User without permissions, but is the owner of the object -$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{borrowernumber}); +$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $patron->{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 = $t->ua->build_tx(GET => "/api/v1/patrons/" . $patron->{borrowernumber}); $tx->req->cookies({name => 'CGISESSID', value => $session2->id}); $t->request_ok($tx) ->status_is(200) @@ -121,12 +125,144 @@ $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); $t->request_ok($tx) ->status_is(200); +ok(@{$tx->res->json} >= 1, 'Json response lists all when no params given'); -$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{ borrowernumber }); +$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $patron->{ 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('/borrowernumber' => $patron->{ borrowernumber }) + ->json_is('/surname' => $patron->{ surname }); + +$tx = $t->ua->build_tx(GET => '/api/v1/patrons' => form => {surname => 'nonexistent'}); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$tx->req->env({REMOTE_ADDR => '127.0.0.1'}); +$t->request_ok($tx) + ->status_is(200); +ok(@{$tx->res->json} == 0, "Json response yields no results when params doesn't match"); + +$tx = $t->ua->build_tx(GET => '/api/v1/patrons' => form => {surname => $patron->{ surname }}); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$tx->req->env({REMOTE_ADDR => '127.0.0.1'}); +$t->request_ok($tx) + ->status_is(200) + ->json_has($patron); +ok(@{$tx->res->json} == 1, 'Json response yields expected results when params match'); + +### POST /api/v1/patrons + +my $newpatron = { + branchcode => $branchcode, + categorycode => $categorycode, + surname => "TestUser", + cardnumber => "123456", + userid => "testuser" +}; + +$newpatron->{ branchcode } = "nonexistent"; # Test invalid branchcode +$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(404) + ->json_is('/error' => "Library with branchcode \"nonexistent\" 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 => $session->id}); +$t->request_ok($tx) + ->status_is(404) + ->json_is('/error' => "Patron category \"nonexistent\" 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 => $session->id}); +$t->request_ok($tx) + ->status_is(500) + ->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); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$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 => $session->id}); +$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 } }); + +### PUT /api/v1/patrons +$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/0" => json => {}); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$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->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(404) + ->json_is('/error' => "Patron category \"nonexistent\" does not exist"); + +$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => {branchcode => "nonexistent"}); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(404) + ->json_is('/error' => "Library with branchcode \"nonexistent\" does not exist"); + +$newpatron->{ falseproperty } = "Non existent property"; +$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => 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"); +delete $newpatron->{ falseproperty }; + +$newpatron->{ cardnumber } = $patron-> { cardnumber }; +$newpatron->{ userid } = $patron-> { userid }; +$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => $newpatron); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$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 => $patron->{ cardnumber }, userid => $patron->{ userid } }); + +$newpatron->{ cardnumber } = "123456"; +$newpatron->{ userid } = "testuser"; +$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => $newpatron); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(204, 'No changes - patron NOT updated'); + +$newpatron->{ cardnumber } = "234567"; +$newpatron->{ userid } = "updatedtestuser"; +$newpatron->{ surname } = "UpdatedTestUser"; + +$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => $newpatron); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(200, 'Patron updated successfully') + ->json_has($newpatron); + +### DELETE /api/v1/patrons + +$tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/0"); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(404, 'Patron not found'); + +$tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/" . $newpatron->{ borrowernumber }); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(200, 'Patron deleted successfully'); -$dbh->rollback; +$schema->storage->txn_rollback; -- 2.1.4