From b7fb10757b5ca876e8636a209adabbfcd551799e Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 8 Dec 2017 09:35:45 -0300 Subject: [PATCH] Bug 19784: Adapt /v1/patrons to new naming guidelines This patch introduces two functions to the patrons endpoint: - _to_api - _to_model This are in charge of field mappings in order to comply with the guidelines. Koha::REST::V1:Auth is adjusted to handle 'patron_id' as well. To test: - Apply the patches - Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off :-D --- Koha/REST/V1/Auth.pm | 8 ++-- Koha/REST/V1/Patrons.pm | 76 +++++++++++++++++++++++++--------- api/v1/swagger/definitions/patron.json | 14 +++---- api/v1/swagger/parameters.json | 8 ++-- api/v1/swagger/parameters/patron.json | 10 ++--- api/v1/swagger/paths.json | 4 +- api/v1/swagger/paths/patrons.json | 20 ++++----- api/v1/swagger/x-primitives.json | 4 +- 8 files changed, 91 insertions(+), 53 deletions(-) diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm index 77864eb26d..e859dc0f73 100644 --- a/Koha/REST/V1/Auth.pm +++ b/Koha/REST/V1/Auth.pm @@ -254,7 +254,7 @@ sub check_object_ownership { my $parameters = { accountlines_id => \&_object_ownership_by_accountlines_id, - borrowernumber => \&_object_ownership_by_borrowernumber, + patron_id => \&_object_ownership_by_patron_id, checkout_id => \&_object_ownership_by_checkout_id, reserve_id => \&_object_ownership_by_reserve_id, }; @@ -296,10 +296,10 @@ Compares C<$borrowernumber> to currently logged in C<$user>. =cut -sub _object_ownership_by_borrowernumber { - my ($c, $user, $borrowernumber) = @_; +sub _object_ownership_by_patron_id { + my ($c, $user, $patron_id) = @_; - return $user->borrowernumber == $borrowernumber; + return $user->borrowernumber == $patron_id; } =head3 _object_ownership_by_checkout_id diff --git a/Koha/REST/V1/Patrons.pm b/Koha/REST/V1/Patrons.pm index 7868e8d148..7cd05b8b0a 100644 --- a/Koha/REST/V1/Patrons.pm +++ b/Koha/REST/V1/Patrons.pm @@ -44,7 +44,8 @@ sub list { return try { my $patrons_set = Koha::Patrons->new; - my @patrons = $c->objects->search( $patrons_set )->as_list; + my @patrons = $c->objects->search( $patrons_set, \&_to_model )->as_list; + @patrons = map { _to_api($_->TO_JSON) } @patrons; return $c->render( status => 200, openapi => \@patrons ); } catch { @@ -70,14 +71,14 @@ Controller function that handles retrieving a single Koha::Patron object sub get { my $c = shift->openapi->valid_input or return; - my $borrowernumber = $c->validation->param('borrowernumber'); - my $patron = Koha::Patrons->find($borrowernumber); + my $patron_id = $c->validation->param('patron_id'); + my $patron = Koha::Patrons->find($patron_id); unless ($patron) { - return $c->render(status => 404, openapi => { error => "Patron not found." }); + return $c->render( status => 404, openapi => { error => "Patron not found." } ); } - return $c->render(status => 200, openapi => $patron); + return $c->render( status => 200, openapi => _to_api($patron->TO_JSON) ); } =head3 add @@ -92,11 +93,11 @@ sub add { return try { my $body = $c->validation->param('body'); - Koha::Patron->new($body)->_validate; + Koha::Patron->new(_to_model($body))->_validate; # TODO: Use AddMember until it has been moved to Koha-namespace - my $borrowernumber = AddMember(%$body); - my $patron = Koha::Patrons->find($borrowernumber); + my $patron_id = AddMember( %{ _to_model($body) } ); + my $patron = _to_api(Koha::Patrons->find( $patron_id )->TO_JSON); return $c->render( status => 201, openapi => $patron ); } @@ -118,13 +119,13 @@ sub add { elsif ( $_->isa('Koha::Exceptions::Library::BranchcodeNotFound') ) { return $c->render( status => 400, - openapi => { error => "Given branchcode does not exist" } + openapi => { error => "Given library_id does not exist" } ); } elsif ( $_->isa('Koha::Exceptions::Category::CategorycodeNotFound') ) { return $c->render( status => 400, - openapi => { error => "Given categorycode does not exist" } + openapi => { error => "Given category_id does not exist" } ); } else { @@ -147,7 +148,7 @@ 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 = Koha::Patrons->find( $c->validation->param('patron_id') ); return try { my $body = $c->validation->param('body'); @@ -158,7 +159,7 @@ sub update { # Add borrowernumber to $body, as required by ModMember $body->{borrowernumber} = $patron->borrowernumber; - if ( ModMember(%$body) ) { + if ( ModMember( %{ _to_model($body) } ) ) { return $c->render( status => 200, openapi => $patron ); } else { @@ -194,13 +195,13 @@ sub update { elsif ( $_->isa('Koha::Exceptions::Library::BranchcodeNotFound') ) { return $c->render( status => 400, - openapi => { error => "Given branchcode does not exist" } + openapi => { error => "Given library_id does not exist" } ); } elsif ( $_->isa('Koha::Exceptions::Category::CategorycodeNotFound') ) { return $c->render( status => 400, - openapi => { error => "Given categorycode does not exist" } + openapi => { error => "Given category_id does not exist" } ); } elsif ( $_->isa('Koha::Exceptions::MissingParameter') ) { @@ -251,7 +252,7 @@ sub delete { my $patron; return try { - $patron = Koha::Patrons->find( $c->validation->param('borrowernumber') ); + $patron = Koha::Patrons->find( $c->validation->param('patron_id') ); # check if loans, reservations, debarrment, etc. before deletion! my $res = $patron->delete; @@ -276,6 +277,31 @@ sub delete { }; } +=head3 _to_api + +Helper function that maps Koha::Patron objects into REST api +attribute names. + +=cut + +sub _to_api { + my $patron = shift; + + my $mapping = { + borrowernumber => 'patron_id', + branchcode => 'library_id', + categorycode => 'category_id' + }; + + foreach my $key ( keys %{ $mapping } ) { + if ( exists $patron->{ $key } ) { + $patron->{ $mapping->{$key} } = delete $patron->{ $key }; + } + } + + return $patron; +} + =head3 _to_model Helper function that maps REST api objects into Koha::Patron @@ -284,12 +310,24 @@ attribute names. =cut sub _to_model { - my $params = shift; + my $patron = shift; + + my $mapping = { + category_id => 'categorycode', + library_id => 'branchcode', + patron_id => 'borrowernumber' + }; + + foreach my $key ( keys %{ $mapping } ) { + if ( exists $patron->{ $key } ) { + $patron->{ $mapping->{$key} } = delete $patron->{ $key }; + } + } - $params->{lost} = ($params->{lost}) ? 1 : 0; - $params->{gonenoaddress} = ($params->{gonenoaddress}) ? 1 : 0; + $patron->{lost} = ($patron->{lost}) ? 1 : 0; + $patron->{gonenoaddress} = ($patron->{gonenoaddress}) ? 1 : 0; - return $params; + return $patron; } 1; diff --git a/api/v1/swagger/definitions/patron.json b/api/v1/swagger/definitions/patron.json index 40b0e51f20..48ebaf5d32 100644 --- a/api/v1/swagger/definitions/patron.json +++ b/api/v1/swagger/definitions/patron.json @@ -1,8 +1,8 @@ { "type": "object", "properties": { - "borrowernumber": { - "$ref": "../x-primitives.json#/borrowernumber" + "patron_id": { + "$ref": "../x-primitives.json#/patron_id" }, "cardnumber": { "$ref": "../x-primitives.json#/cardnumber" @@ -124,13 +124,13 @@ "format": "date", "description": "patron's date of birth" }, - "branchcode": { + "library_id": { "type": "string", - "description": "code of patron's home branch" + "description": "Internal identifier for the patron's home library" }, - "categorycode": { + "category_id": { "type": "string", - "description": "code of patron's category" + "description": "Internal identifier for the patron's category" }, "dateenrolled": { "type": ["string", "null"], @@ -299,5 +299,5 @@ } }, "additionalProperties": false, - "required": ["surname", "address", "city", "branchcode", "categorycode"] + "required": ["surname", "address", "city", "library_id", "category_id"] } diff --git a/api/v1/swagger/parameters.json b/api/v1/swagger/parameters.json index 1da158c3f4..0f2b5893af 100644 --- a/api/v1/swagger/parameters.json +++ b/api/v1/swagger/parameters.json @@ -1,9 +1,9 @@ { - "borrowernumberPathParam": { - "$ref": "parameters/patron.json#/borrowernumberPathParam" + "patron_id_pp": { + "$ref": "parameters/patron.json#/patron_id_pp" }, - "borrowernumberQueryParam": { - "$ref": "parameters/patron.json#/borrowernumberQueryParam" + "patron_id_qp": { + "$ref": "parameters/patron.json#/patron_id_qp" }, "cityidPathParam": { "$ref": "parameters/city.json#/cityidPathParam" diff --git a/api/v1/swagger/parameters/patron.json b/api/v1/swagger/parameters/patron.json index 409d0b2c35..d2114ba36f 100644 --- a/api/v1/swagger/parameters/patron.json +++ b/api/v1/swagger/parameters/patron.json @@ -1,15 +1,15 @@ { - "borrowernumberPathParam": { - "name": "borrowernumber", + "patron_id_pp": { + "name": "patron_id", "in": "path", "description": "Internal patron identifier", "required": true, "type": "integer" }, - "borrowernumberQueryParam": { - "name": "borrowernumber", + "patron_id_qp": { + "name": "patron_id", "in": "query", - "description": "Internal borrower identifier", + "description": "Internal patron identifier", "type": "integer" } } diff --git a/api/v1/swagger/paths.json b/api/v1/swagger/paths.json index 9c3a6d2efd..9fd947f02d 100644 --- a/api/v1/swagger/paths.json +++ b/api/v1/swagger/paths.json @@ -20,8 +20,8 @@ "/patrons": { "$ref": "paths/patrons.json#/~1patrons" }, - "/patrons/{borrowernumber}": { - "$ref": "paths/patrons.json#/~1patrons~1{borrowernumber}" + "/patrons/{patron_id}": { + "$ref": "paths/patrons.json#/~1patrons~1{patron_id}" }, "/illrequests": { "$ref": "paths/illrequests.json#/~1illrequests" diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index 9790fdd080..c113b80c84 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -8,9 +8,9 @@ "application/json" ], "parameters": [{ - "name": "borrowernumber", + "name": "patron_id", "in": "query", - "description": "Case insensitive search on borrowernumber", + "description": "Search on patron_id", "required": false, "type": "string" }, { @@ -200,15 +200,15 @@ "required": false, "type": "string" }, { - "name": "branchcode", + "name": "library_id", "in": "query", - "description": "Case insensitive search on branchcode", + "description": "Case insensitive search on library_id", "required": false, "type": "string" }, { - "name": "categorycode", + "name": "category_id", "in": "query", - "description": "Case insensitive search on categorycode", + "description": "Case insensitive search on category_id", "required": false, "type": "string" }, { @@ -563,13 +563,13 @@ } } }, - "/patrons/{borrowernumber}": { + "/patrons/{patron_id}": { "get": { "x-mojo-to": "Patrons#get", "operationId": "getPatron", "tags": ["patrons"], "parameters": [{ - "$ref": "../parameters.json#/borrowernumberPathParam" + "$ref": "../parameters.json#/patron_id_pp" }], "produces": [ "application/json" @@ -626,7 +626,7 @@ "tags": ["patrons"], "parameters": [ { - "$ref": "../parameters.json#/borrowernumberPathParam" + "$ref": "../parameters.json#/patron_id_pp" }, { "name": "body", @@ -703,7 +703,7 @@ "operationId": "deletePatron", "tags": ["patrons"], "parameters": [{ - "$ref": "../parameters.json#/borrowernumberPathParam" + "$ref": "../parameters.json#/patron_id_pp" }], "produces": ["application/json"], "responses": { diff --git a/api/v1/swagger/x-primitives.json b/api/v1/swagger/x-primitives.json index 12e5e9de39..94961cb0c9 100644 --- a/api/v1/swagger/x-primitives.json +++ b/api/v1/swagger/x-primitives.json @@ -3,9 +3,9 @@ "type": "integer", "description": "internally assigned biblio identifier" }, - "borrowernumber": { + "patron_id": { "type": "integer", - "description": "internally assigned user identifier" + "description": "Internal patron identifier" }, "cardnumber": { "type": ["string", "null"], -- 2.15.1