From a133230dacb5ebadd491d0c91d68a2b6d9365cb4 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 11 Jan 2019 13:23:14 -0300 Subject: [PATCH] Bug 16497: (follow-up) Adapt to existing guidelines and RFC This patch makes the original implementation match what is specified on the RFC [1]. The controller is updated, and so the tests. To test: - Apply this patches: - Run: $ kshell k$ prove t/db_dependent/api/v1/libraries.t => SUCCESS: Tests pass! [1] https://wiki.koha-community.org/wiki/Libraries_endpoint_RFC Signed-off-by: Tomas Cohen Arazi Signed-off-by: Josef Moravec Signed-off-by: Martin Renvoize Signed-off-by: Nick Clemens (cherry picked from commit 0718416ff1ca70a25bc12fa3b677fdeafd7854c8) Signed-off-by: Martin Renvoize --- Koha/REST/V1/Library.pm | 249 +++++++++++++----- api/v1/swagger/definitions/library.json | 54 ++-- api/v1/swagger/parameters.json | 4 +- api/v1/swagger/parameters/library.json | 6 +- api/v1/swagger/paths.json | 4 +- api/v1/swagger/paths/libraries.json | 325 +++++++++++++----------- api/v1/swagger/x-primitives.json | 2 +- t/db_dependent/api/v1/libraries.t | 193 ++++++-------- 8 files changed, 483 insertions(+), 354 deletions(-) diff --git a/Koha/REST/V1/Library.pm b/Koha/REST/V1/Library.pm index c0b92b2de9..0bf16ee60b 100644 --- a/Koha/REST/V1/Library.pm +++ b/Koha/REST/V1/Library.pm @@ -43,27 +43,22 @@ Controller function that handles listing Koha::Library objects sub list { my $c = shift->openapi->valid_input or return; - my $libraries; - my $filter; - my $args = $c->req->params->to_hash; - - for my $filter_param ( keys %$args ) { - $filter->{$filter_param} = { LIKE => $args->{$filter_param} . "%" }; - } - return try { - my $libraries = Koha::Libraries->search($filter); + my $libraries_set = Koha::Libraries->new; + my $libraries = $c->objects->search( $libraries_set, \&_to_model, \&_to_api ); return $c->render( status => 200, openapi => $libraries ); } 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."} ); + unless ( blessed $_ && $_->can('rethrow') ) { + return $c->render( + status => 500, + openapi => { error => "Something went wrong, check Koha logs for details." } + ); } + return $c->render( + status => 500, + openapi => { error => "$_" } + ); }; } @@ -76,14 +71,15 @@ Controller function that handles retrieving a single Koha::Library sub get { my $c = shift->openapi->valid_input or return; - my $branchcode = $c->validation->param('branchcode'); - my $library = Koha::Libraries->find({ branchcode => $branchcode }); + my $library_id = $c->validation->param('library_id'); + my $library = Koha::Libraries->find( $library_id ); + unless ($library) { return $c->render( status => 404, openapi => { error => "Library not found" } ); } - return $c->render( status => 200, openapi => $library ); + return $c->render( status => 200, openapi => _to_api( $library->TO_JSON ) ); } =head3 add @@ -96,23 +92,29 @@ sub add { my $c = shift->openapi->valid_input or return; return try { - if (Koha::Libraries->find($c->req->json->{branchcode})) { - return $c->render( status => 400, - openapi => { error => 'Library already exists' } ); - } - my $library = Koha::Library->new($c->validation->param('body'))->store; - my $branchcode = $library->branchcode; - $c->res->headers->location($c->req->url->to_string.'/'.$branchcode); - return $c->render( status => 201, openapi => $library); + my $library = Koha::Library->new( _to_model( $c->validation->param('body') ) ); + $library->store; + $c->res->headers->location( $c->req->url->to_string . '/' . $library->branchcode ); + return $c->render( status => 201, openapi => _to_api( $library->TO_JSON ) ); } catch { - if ( $_->isa('DBIx::Class::Exception') ) { - return $c->render( status => 500, - openapi => { error => $_->{msg} } ); + unless ( blessed $_ && $_->can('rethrow') ) { + return $c->render( + status => 500, + openapi => { error => "Something went wrong, check Koha logs for details." } + ); + } + if ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) { + return $c->render( + status => 409, + openapi => { error => $_->error, conflict => $_->duplicate_id } + ); } else { - return $c->render( status => 500, - openapi => { error => "Something went wrong, check the logs."} ); + return $c->render( + status => 500, + openapi => { error => "$_" } + ); } }; } @@ -126,25 +128,33 @@ Controller function that handles updating a Koha::Library object sub update { my $c = shift->openapi->valid_input or return; - my $library; + my $library = Koha::Libraries->find( $c->validation->param('library_id') ); + + if ( not defined $library ) { + return $c->render( + status => 404, + openapi => { error => "Library not found" } + ); + } + return try { - $library = Koha::Libraries->find($c->validation->param('branchcode')); - $library->set($c->validation->param('body'))->store; - return $c->render( status => 200, openapi => $library ); + my $params = $c->req->json; + $library->set( _to_model($params) ); + $library->store(); + return $c->render( status => 200, openapi => _to_api($library->TO_JSON) ); } catch { - if ( not defined $library ) { - return $c->render( status => 404, - openapi => { error => "Object not found" }); - } - elsif ( $_->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."} ); + unless ( blessed $_ && $_->can('rethrow') ) { + return $c->render( + status => 500, + openapi => { error => "Something went wrong, check Koha logs for details." } + ); } + + return $c->render( + status => 500, + openapi => { error => "$_" } + ); }; } @@ -155,27 +165,150 @@ Controller function that handles deleting a Koha::Library object =cut sub delete { + my $c = shift->openapi->valid_input or return; - my $library; + my $library = Koha::Libraries->find( $c->validation->param( 'library_id' ) ); + + if ( not defined $library ) { + return $c->render( status => 404, openapi => { error => "Library not found" } ); + } + return try { - $library = Koha::Libraries->find($c->validation->param('branchcode')); $library->delete; return $c->render( status => 204, openapi => ''); } catch { - if ( not defined $library ) { - return $c->render( status => 404, openapi => { error => "Object not found" } ); + unless ( blessed $_ && $_->can('rethrow') ) { + return $c->render( + status => 500, + openapi => { error => "Something went wrong, check Koha logs for details." } + ); } - elsif ( $_->isa('DBIx::Class::Exception') ) { - return $c->render( status => 500, - openapi => { error => $_->{msg} } ); + + return $c->render( + status => 500, + openapi => { error => "$_" } + ); + }; +} + +=head3 _to_api + +Helper function that maps a hashref of Koha::Library attributes into REST api +attribute names. + +=cut + +sub _to_api { + my $library = shift; + + # Rename attributes + foreach my $column ( keys %{ $Koha::REST::V1::Library::to_api_mapping } ) { + my $mapped_column = $Koha::REST::V1::Library::to_api_mapping->{$column}; + if ( exists $library->{ $column } + && defined $mapped_column ) + { + # key /= undef + $library->{ $mapped_column } = delete $library->{ $column }; } - else { - return $c->render( status => 500, - openapi => { error => "Something went wrong, check the logs."} ); + elsif ( exists $library->{ $column } + && !defined $mapped_column ) + { + # key == undef => to be deleted + delete $library->{ $column }; } - }; + } + + return $library; +} + +=head3 _to_model + +Helper function that maps REST api objects into Koha::Library +attribute names. + +=cut + +sub _to_model { + my $library = shift; + + foreach my $attribute ( keys %{ $Koha::REST::V1::Library::to_model_mapping } ) { + my $mapped_attribute = $Koha::REST::V1::Library::to_model_mapping->{$attribute}; + if ( exists $library->{ $attribute } + && defined $mapped_attribute ) + { + # key /= undef + $library->{ $mapped_attribute } = delete $library->{ $attribute }; + } + elsif ( exists $library->{ $attribute } + && !defined $mapped_attribute ) + { + # key == undef => to be deleted + delete $library->{ $attribute }; + } + } + + if ( exists $library->{pickup_location} ) { + $library->{pickup_location} = ( $library->{pickup_location} ) ? 1 : 0; + } + + return $library; } + +=head2 Global variables + +=head3 $to_api_mapping + +=cut + +our $to_api_mapping = { + branchcode => 'library_id', + branchname => 'name', + branchaddress1 => 'address1', + branchaddress2 => 'address2', + branchaddress3 => 'address3', + branchzip => 'postal_code', + branchcity => 'city', + branchstate => 'state', + branchcountry => 'country', + branchphone => 'phone', + branchfax => 'fax', + branchemail => 'email', + branchreplyto => 'reply_to_email', + branchreturnpath => 'return_path_email', + branchurl => 'url', + issuing => undef, + branchip => 'ip', + branchprinter => undef, + branchnotes => 'notes', + marcorgcode => 'marc_org_code', +}; + +=head3 $to_model_mapping + +=cut + +our $to_model_mapping = { + library_id => 'branchcode', + name => 'branchname', + address1 => 'branchaddress1', + address2 => 'branchaddress2', + address3 => 'branchaddress3', + postal_code => 'branchzip', + city => 'branchcity', + state => 'branchstate', + country => 'branchcountry', + phone => 'branchphone', + fax => 'branchfax', + email => 'branchemail', + reply_to_email => 'branchreplyto', + return_path_email => 'branchreturnpath', + url => 'branchurl', + ip => 'branchip', + notes => 'branchnotes', + marc_org_code => 'marcorgcode', +}; + 1; diff --git a/api/v1/swagger/definitions/library.json b/api/v1/swagger/definitions/library.json index b7f8aa1774..d31a83ff18 100644 --- a/api/v1/swagger/definitions/library.json +++ b/api/v1/swagger/definitions/library.json @@ -1,78 +1,70 @@ { "type": "object", "properties": { - "branchcode": { - "$ref": "../x-primitives.json#/branchcode" + "library_id": { + "$ref": "../x-primitives.json#/library_id" }, - "branchname": { + "name": { "type": "string", "description": "Printable name of library" }, - "branchaddress1": { + "address1": { "type": ["string", "null"], "description": "the first address line of the library" }, - "branchaddress2": { + "address2": { "type": ["string", "null"], "description": "the second address line of the library" }, - "branchaddress3": { + "address3": { "type": ["string", "null"], "description": "the third address line of the library" }, - "branchzip": { + "postal_code": { "type": ["string", "null"], - "description": "the zip or postal code of the library" + "description": "the postal code of the library" }, - "branchcity": { + "city": { "type": ["string", "null"], "description": "the city or province of the library" }, - "branchstate": { + "state": { "type": ["string", "null"], "description": "the reqional state of the library" }, - "branchcountry": { + "country": { "type": ["string", "null"], "description": "the county of the library" }, - "branchphone": { + "phone": { "type": ["string", "null"], "description": "the primary phone of the library" }, - "branchfax": { + "fax": { "type": ["string", "null"], "description": "the fax number of the library" }, - "branchemail": { + "email": { "type": ["string", "null"], "description": "the primary email address of the library" }, - "branchreplyto": { + "reply_to_email": { "type": ["string", "null"], "description": "the email to be used as a Reply-To" }, - "branchreturnpath": { + "return_path_email": { "type": ["string", "null"], "description": "the email to be used as Return-Path" }, - "branchurl": { + "url": { "type": ["string", "null"], "description": "the URL for your library or branch's website" }, - "issuing": { - "type": ["integer", "null"], - "description": "unused in Koha" - }, - "branchip": { + "ip": { "type": ["string", "null"], "description": "the IP address for your library or branch" }, - "branchprinter": { - "type": ["string", "null"], - "description": "unused in Koha" - }, - "branchnotes": { + "notes": { "type": ["string", "null"], "description": "notes related to your library or branch" }, @@ -84,11 +76,15 @@ "type": ["string", "null"], "description": "geolocation of your library" }, - "marcorgcode": { + "marc_org_code": { "type": [ "string", "null" ], "description": "MARC Organization Code, see http://www.loc.gov/marc/organizations/orgshome.html, when empty defaults to syspref MARCOrgCode" + }, + "pickup_location": { + "type": "boolean", + "description": "If the library can act as a pickup location" } }, "additionalProperties": false, - "required": ["branchcode", "branchname"] + "required": ["library_id", "name"] } diff --git a/api/v1/swagger/parameters.json b/api/v1/swagger/parameters.json index 1d5feba907..1e9bcbdaca 100644 --- a/api/v1/swagger/parameters.json +++ b/api/v1/swagger/parameters.json @@ -8,8 +8,8 @@ "city_id_pp": { "$ref": "parameters/city.json#/city_id_pp" }, - "branchcodePathParam": { - "$ref": "parameters/library.json#/branchcodePathParam" + "library_id_pp": { + "$ref": "parameters/library.json#/library_id_pp" }, "holdIdPathParam": { "$ref": "parameters/hold.json#/holdIdPathParam" diff --git a/api/v1/swagger/parameters/library.json b/api/v1/swagger/parameters/library.json index 366581e4e7..cb0da0b9ed 100644 --- a/api/v1/swagger/parameters/library.json +++ b/api/v1/swagger/parameters/library.json @@ -1,8 +1,8 @@ { - "branchcodePathParam": { - "name": "branchcode", + "library_id_pp": { + "name": "library_id", "in": "path", - "description": "Branch identifier code", + "description": "Internal library identifier", "required": true, "type": "string" } diff --git a/api/v1/swagger/paths.json b/api/v1/swagger/paths.json index 30cba9d85b..b7a8bd9bbe 100644 --- a/api/v1/swagger/paths.json +++ b/api/v1/swagger/paths.json @@ -23,8 +23,8 @@ "/libraries": { "$ref": "paths/libraries.json#/~1libraries" }, - "/libraries/{branchcode}": { - "$ref": "paths/libraries.json#/~1libraries~1{branchcode}" + "/libraries/{library_id}": { + "$ref": "paths/libraries.json#/~1libraries~1{library_id}" }, "/patrons": { "$ref": "paths/patrons.json#/~1patrons" diff --git a/api/v1/swagger/paths/libraries.json b/api/v1/swagger/paths/libraries.json index 1457788163..70249ed227 100644 --- a/api/v1/swagger/paths/libraries.json +++ b/api/v1/swagger/paths/libraries.json @@ -3,122 +3,130 @@ "get": { "x-mojo-to": "Library#list", "operationId": "listLibrary", - "tags": ["library"], - "parameters": [{ - "name": "branchname", - "in": "query", - "description": "Case insensitive 'starts-with' search on name", - "required": false, - "type": "string" - }, { - "name": "branchaddress1", - "in": "query", - "description": "Case insensitive 'starts-with' search on address1", - "required": false, - "type": "string" - }, { - "name": "branchaddress2", - "in": "query", - "description": "Case insensitive 'starts-with' search on address2", - "required": false, - "type": "string" - }, { - "name": "branchaddress3", - "in": "query", - "description": "Case insensitive 'starts-with' search on address3", - "required": false, - "type": "string" - }, { - "name": "branchzip", - "in": "query", - "description": "Case insensitive 'starts-with' search on zipcode", - "required": false, - "type": "string" - }, { - "name": "branchcity", - "in": "query", - "description": "Case insensitive 'starts-with' search on city", - "required": false, - "type": "string" - }, { - "name": "branchstate", - "in": "query", - "description": "Case insensitive 'starts-with' search on state", - "required": false, - "type": "string" - }, { - "name": "branchcountry", - "in": "query", - "description": "Case insensitive 'starts_with' search on country", - "required": false, - "type": "string" - }, { - "name": "branchphone", - "in": "query", - "description": "Case insensitive 'starts_with' search on phone number", - "required": false, - "type": "string" - }, { - "name": "branchfax", - "in": "query", - "description": "Case insensitive 'starts_with' search on fax number", - "required": false, - "type": "string" - }, { - "name": "branchemail", - "in": "query", - "description": "Case insensitive 'starts_with' search on email address", - "required": false, - "type": "string" - }, { - "name": "branchreplyto", - "in": "query", - "description": "Case insensitive 'starts_with' search on Reply-To email address", - "required": false, - "type": "string" - }, { - "name": "branchreturnpath", - "in": "query", - "description": "Case insensitive 'starts_with' search on Return-Path email address", - "required": false, - "type": "string" - }, { - "name": "branchurl", - "in": "query", - "description": "Case insensitive 'starts_with' search on website URL", - "required": false, - "type": "string" - }, { - "name": "issuing", - "in": "query", - "description": "Unused in Koha", - "required": false, - "type": "integer" - }, { - "name": "branchip", - "in": "query", - "description": "Case insensitive 'starts_with' search on IP address", - "required": false, - "type": "string" - }, { - "name": "branchprinter", - "in": "query", - "description": "Unused in Koha", - "required": false, - "type": "string" - }, { - "name": "branchnotes", - "in": "query", - "description": "Case insensitive 'starts_with' search on notes", - "required": false, - "type": "string" - }, { - "name": "opac_info", - "in": "query", - "description": "Case insensitive 'starts-with' search on OPAC info", - "required": false, - "type": "string" - }], + "tags": [ + "library" + ], + "parameters": [ + { + "name": "name", + "in": "query", + "description": "Case insensitive 'starts-with' search on name", + "required": false, + "type": "string" + }, + { + "name": "address1", + "in": "query", + "description": "Case insensitive 'starts-with' search on address1", + "required": false, + "type": "string" + }, + { + "name": "address2", + "in": "query", + "description": "Case insensitive 'starts-with' search on address2", + "required": false, + "type": "string" + }, + { + "name": "address3", + "in": "query", + "description": "Case insensitive 'starts-with' search on address3", + "required": false, + "type": "string" + }, + { + "name": "postal_code", + "in": "query", + "description": "Case insensitive 'starts-with' search on postal code", + "required": false, + "type": "string" + }, + { + "name": "city", + "in": "query", + "description": "Case insensitive 'starts-with' search on city", + "required": false, + "type": "string" + }, + { + "name": "state", + "in": "query", + "description": "Case insensitive 'starts-with' search on state", + "required": false, + "type": "string" + }, + { + "name": "country", + "in": "query", + "description": "Case insensitive 'starts_with' search on country", + "required": false, + "type": "string" + }, + { + "name": "phone", + "in": "query", + "description": "Case insensitive 'starts_with' search on phone number", + "required": false, + "type": "string" + }, + { + "name": "fax", + "in": "query", + "description": "Case insensitive 'starts_with' search on fax number", + "required": false, + "type": "string" + }, + { + "name": "email", + "in": "query", + "description": "Case insensitive 'starts_with' search on email address", + "required": false, + "type": "string" + }, + { + "name": "reply_to_email", + "in": "query", + "description": "Case insensitive 'starts_with' search on Reply-To email address", + "required": false, + "type": "string" + }, + { + "name": "return_path_email", + "in": "query", + "description": "Case insensitive 'starts_with' search on Return-Path email address", + "required": false, + "type": "string" + }, + { + "name": "url", + "in": "query", + "description": "Case insensitive 'starts_with' search on website URL", + "required": false, + "type": "string" + }, + { + "name": "ip", + "in": "query", + "description": "Case insensitive 'starts_with' search on IP address", + "required": false, + "type": "string" + }, + { + "name": "notes", + "in": "query", + "description": "Case insensitive 'starts_with' search on notes", + "required": false, + "type": "string" + }, + { + "name": "opac_info", + "in": "query", + "description": "Case insensitive 'starts-with' search on OPAC info", + "required": false, + "type": "string" + } + ], "produces": [ "application/json" ], @@ -149,16 +157,20 @@ "post": { "x-mojo-to": "Library#add", "operationId": "addLibrary", - "tags": ["library"], - "parameters": [{ - "name": "body", - "in": "body", - "description": "A JSON object containing informations about the new library", - "required": true, - "schema": { - "$ref": "../definitions.json#/library" + "tags": [ + "library" + ], + "parameters": [ + { + "name": "body", + "in": "body", + "description": "A JSON object containing informations about the new library", + "required": true, + "schema": { + "$ref": "../definitions.json#/library" + } } - }], + ], "produces": [ "application/json" ], @@ -187,6 +199,12 @@ "$ref": "../definitions.json#/error" } }, + "409": { + "description": "Conflict in creating resource", + "schema": { + "$ref": "../definitions.json#/error" + } + }, "500": { "description": "Internal error", "schema": { @@ -202,19 +220,21 @@ }, "x-koha-authorization": { "permissions": { - "parameters": "parameters_remaining_permissions" + "parameters": "manage_libraries" } } } }, - "/libraries/{branchcode}": { + "/libraries/{library_id}": { "get": { "x-mojo-to": "Library#get", "operationId": "getLibrary", - "tags": ["library"], + "tags": [ + "library" + ], "parameters": [ { - "$ref": "../parameters.json#/branchcodePathParam" + "$ref": "../parameters.json#/library_id_pp" } ], "produces": [ @@ -238,18 +258,23 @@ "put": { "x-mojo-to": "Library#update", "operationId": "updateLibrary", - "tags": ["library"], - "parameters": [{ - "$ref": "../parameters.json#/branchcodePathParam" - }, { - "name": "body", - "in": "body", - "description": "A JSON object containing information on the library", - "required": true, - "schema": { - "$ref": "../definitions.json#/library" + "tags": [ + "library" + ], + "parameters": [ + { + "$ref": "../parameters.json#/library_id_pp" + }, + { + "name": "body", + "in": "body", + "description": "A JSON object containing information on the library", + "required": true, + "schema": { + "$ref": "../definitions.json#/library" + } } - }], + ], "consumes": [ "application/json" ], @@ -302,24 +327,30 @@ }, "x-koha-authorization": { "permissions": { - "parameters": "parameters_remaining_permissions" + "parameters": "manage_libraries" } } }, "delete": { "x-mojo-to": "Library#delete", "operationId": "deleteLibrary", - "tags": ["library"], - "parameters": [{ - "$ref": "../parameters.json#/branchcodePathParam" - }], + "tags": [ + "library" + ], + "parameters": [ + { + "$ref": "../parameters.json#/library_id_pp" + } + ], "produces": [ "application/json" ], "responses": { "204": { "description": "Library deleted", - "schema": { "type": "string" } + "schema": { + "type": "string" + } }, "401": { "description": "Authentication required", @@ -354,7 +385,7 @@ }, "x-koha-authorization": { "permissions": { - "parameters": "parameters_remaining_permissions" + "parameters": "manage_libraries" } } } diff --git a/api/v1/swagger/x-primitives.json b/api/v1/swagger/x-primitives.json index 9b077d6f8f..b68632e153 100644 --- a/api/v1/swagger/x-primitives.json +++ b/api/v1/swagger/x-primitives.json @@ -7,7 +7,7 @@ "type": "integer", "description": "Internal patron identifier" }, - "branchcode": { + "library_id": { "type": "string", "description": "internally assigned library identifier", "maxLength": 10, diff --git a/t/db_dependent/api/v1/libraries.t b/t/db_dependent/api/v1/libraries.t index 7b59610c77..db76b6dbd0 100644 --- a/t/db_dependent/api/v1/libraries.t +++ b/t/db_dependent/api/v1/libraries.t @@ -44,13 +44,11 @@ subtest 'list() tests' => sub { $schema->storage->txn_begin; # Create test context - my $library = $builder->build( { source => 'Branch' } ); - my $another_library = { %$library }; # create a copy of $library but make - delete $another_library->{branchcode}; # sure branchcode will be regenerated - $another_library = $builder->build( - { source => 'Branch', value => $another_library } ); - my ( $borrowernumber, $session_id ) = - create_user_and_session( { authorized => 0 } ); + my $library = $builder->build_object({ class => 'Koha::Libraries' }); + my $another_library = $library->unblessed; # create a copy of $library but make + delete $another_library->{branchcode}; # sure branchcode will be regenerated + $another_library = $builder->build_object({ class => 'Koha::Libraries', value => $another_library }); + my ( $borrowernumber, $session_id ) = create_user_and_session( { authorized => 0 } ); ## Authorized user tests my $count_of_libraries = Koha::Libraries->search->count; @@ -59,23 +57,40 @@ subtest 'list() tests' => sub { $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx) - ->status_is(200) - ->json_has('/'.($count_of_libraries-1).'/branchcode') - ->json_hasnt('/'.($count_of_libraries).'/branchcode'); + ->status_is( 200, 'SWAGGER3.2.2' ) + ->json_has('/'.($count_of_libraries-1).'/library_id') + ->json_hasnt('/'.($count_of_libraries).'/library_id'); subtest 'query parameters' => sub { - my @fields = qw( - branchname branchaddress1 branchaddress2 branchaddress3 - branchzip branchcity branchstate branchcountry - branchphone branchfax branchemail branchreplyto - branchreturnpath branchurl issuing branchip - branchprinter branchnotes opac_info - ); - plan tests => scalar(@fields)*3; - - foreach my $field (@fields) { + + my $fields = { + name => 'branchname', + address1 => 'branchaddress1', + address2 => 'branchaddress2', + address3 => 'branchaddress3', + postal_code => 'branchzip', + city => 'branchcity', + state => 'branchstate', + country => 'branchcountry', + phone => 'branchphone', + fax => 'branchfax', + email => 'branchemail', + reply_to_email => 'branchreplyto', + return_path_email => 'branchreturnpath', + url => 'branchurl', + ip => 'branchip', + notes => 'branchnotes', + opac_info => 'opac_info', + }; + + my $size = keys %{$fields}; + + plan tests => $size * 3; + + foreach my $field ( keys %{$fields} ) { + my $model_field = $fields->{ $field }; $tx = $t->ua->build_tx( GET => - "/api/v1/libraries?$field=$library->{$field}" ); + "/api/v1/libraries?$field=" . $library->$model_field ); $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); my $result = @@ -102,18 +117,20 @@ subtest 'get() tests' => sub { $schema->storage->txn_begin; - my $library = $builder->build( { source => 'Branch' } ); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my ( $borrowernumber, $session_id ) = create_user_and_session( { authorized => 0 } ); - my $tx = $t->ua->build_tx( GET => "/api/v1/libraries/" . $library->{branchcode} ); + my $tx = $t->ua->build_tx( GET => "/api/v1/libraries/" . $library->branchcode ); $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx) - ->status_is(200) - ->json_is($library); + ->status_is( 200, 'SWAGGER3.2.2' ) + ->json_is( '' => Koha::REST::V1::Library::_to_api( $library->TO_JSON ), 'SWAGGER3.3.2' ); + + my $non_existent_code = $library->branchcode; + $library->delete; - my $non_existent_code = 'non_existent'.int(rand(10000)); $tx = $t->ua->build_tx( GET => "/api/v1/libraries/" . $non_existent_code ); $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); @@ -125,7 +142,8 @@ subtest 'get() tests' => sub { }; subtest 'add() tests' => sub { - plan tests => 31; + + plan tests => 17; $schema->storage->txn_begin; @@ -133,28 +151,10 @@ subtest 'add() tests' => sub { create_user_and_session( { authorized => 0 } ); my ( $authorized_borrowernumber, $authorized_session_id ) = create_user_and_session( { authorized => 1 } ); - my $library = { - branchcode => "LIBRARYBR1", - branchname => "Library Name", - branchaddress1 => "Library Address1", - branchaddress2 => "Library Address2", - branchaddress3 => "Library Address3", - branchzip => "Library Zipcode", - branchcity => "Library City", - branchstate => "Library State", - branchcountry => "Library Country", - branchphone => "Library Phone", - branchfax => "Library Fax", - branchemail => "Library Email", - branchreplyto => "Library Reply-To", - branchreturnpath => "Library Return-Path", - branchurl => "http://library.url", - issuing => undef, # unused in Koha - branchip => "127.0.0.1", - branchprinter => "Library Printer", # unused in Koha - branchnotes => "Library Notes", - opac_info => "

Library OPAC info

", - }; + + my $library_obj = $builder->build_object({ class => 'Koha::Libraries' }); + my $library = Koha::REST::V1::Library::_to_api( $library_obj->TO_JSON ); + $library_obj->delete; # Unauthorized attempt to write my $tx = $t->ua->build_tx( POST => "/api/v1/libraries" => json => $library ); @@ -189,30 +189,15 @@ subtest 'add() tests' => sub { $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - my $branchcode = $t->request_ok($tx) - ->status_is(201) - ->json_is( '/branchname' => $library->{branchname} ) - ->json_is( '/branchaddress1' => $library->{branchaddress1} ) - ->json_is( '/branchaddress2' => $library->{branchaddress2} ) - ->json_is( '/branchaddress3' => $library->{branchaddress3} ) - ->json_is( '/branchzip' => $library->{branchzip} ) - ->json_is( '/branchcity' => $library->{branchcity} ) - ->json_is( '/branchstate' => $library->{branchstate} ) - ->json_is( '/branchcountry' => $library->{branchcountry} ) - ->json_is( '/branchphone' => $library->{branchphone} ) - ->json_is( '/branchfax' => $library->{branchfax} ) - ->json_is( '/branchemail' => $library->{branchemail} ) - ->json_is( '/branchreplyto' => $library->{branchreplyto} ) - ->json_is( '/branchreturnpath' => $library->{branchreturnpath} ) - ->json_is( '/branchurl' => $library->{branchurl} ) - ->json_is( '/branchip' => $library->{branchip} ) - ->json_is( '/branchnotes' => $library->{branchnotes} ) - ->json_is( '/opac_info' => $library->{opac_info} ) - ->header_is(Location => "/api/v1/libraries/$library->{branchcode}") - ->tx->res->json->{branchcode}; + $t->request_ok($tx) + ->status_is( 201, 'SWAGGER3.2.1' ) + ->json_is( '' => $library, 'SWAGGER3.3.1' ) + ->header_is( Location => '/api/v1/libraries/' . $library->{library_id}, 'SWAGGER3.4.1' ); + # save the library_id + my $library_id = $library->{library_id}; # Authorized attempt to create with null id - $library->{branchcode} = undef; + $library->{library_id} = undef; $tx = $t->ua->build_tx( POST => "/api/v1/libraries" => json => $library ); $tx->req->cookies( @@ -223,15 +208,19 @@ subtest 'add() tests' => sub { ->json_has('/errors'); # Authorized attempt to create with existing id - $library->{branchcode} = $branchcode; + $library->{library_id} = $library_id; $tx = $t->ua->build_tx( POST => "/api/v1/libraries" => json => $library ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - $t->request_ok($tx) - ->status_is(400) - ->json_is('/error' => 'Library already exists'); + + warning_like { + $t->request_ok($tx) + ->status_is(409) + ->json_has( '/error' => "Fails when trying to add an existing library_id") + ->json_is( '/conflict', 'PRIMARY' ); } # WTF + qr/^DBD::mysql::st execute failed: Duplicate entry '(.*)' for key 'PRIMARY'/; $schema->storage->txn_rollback; }; @@ -246,10 +235,11 @@ subtest 'update() tests' => sub { my ( $authorized_borrowernumber, $authorized_session_id ) = create_user_and_session( { authorized => 1 } ); - my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; + my $library = $builder->build_object({ class => 'Koha::Libraries' }); + my $library_id = $library->branchcode; # Unauthorized attempt to update - my $tx = $t->ua->build_tx( PUT => "/api/v1/libraries/$branchcode" + my $tx = $t->ua->build_tx( PUT => "/api/v1/libraries/$library_id" => json => { branchname => 'New unauthorized name change' } ); $tx->req->cookies( { name => 'CGISESSID', value => $unauthorized_session_id } ); @@ -259,10 +249,10 @@ subtest 'update() tests' => sub { # Attempt partial update on a PUT my $library_with_missing_field = { - branchaddress1 => "New library address", + address1 => "New library address", }; - $tx = $t->ua->build_tx( PUT => "/api/v1/libraries/$branchcode" => + $tx = $t->ua->build_tx( PUT => "/api/v1/libraries/$library_id" => json => $library_with_missing_field ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); @@ -270,48 +260,27 @@ subtest 'update() tests' => sub { $t->request_ok($tx) ->status_is(400) ->json_has( "/errors" => - [ { message => "Missing property.", path => "/body/branchaddress2" } ] + [ { message => "Missing property.", path => "/body/address2" } ] ); - # Full object update on PUT - my $library_with_updated_field = { - branchcode => "LIBRARYBR2", - branchname => "Library Name", - branchaddress1 => "Library Address1", - branchaddress2 => "Library Address2", - branchaddress3 => "Library Address3", - branchzip => "Library Zipcode", - branchcity => "Library City", - branchstate => "Library State", - branchcountry => "Library Country", - branchphone => "Library Phone", - branchfax => "Library Fax", - branchemail => "Library Email", - branchreplyto => "Library Reply-To", - branchreturnpath => "Library Return-Path", - branchurl => "http://library.url", - issuing => undef, # unused in Koha - branchip => "127.0.0.1", - branchprinter => "Library Printer", # unused in Koha - branchnotes => "Library Notes", - opac_info => "

Library OPAC info

", - }; + my $deleted_library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library_with_updated_field = Koha::REST::V1::Library::_to_api( $deleted_library->TO_JSON ); + $library_with_updated_field->{library_id} = $library_id; + $deleted_library->delete; - $tx = $t->ua->build_tx( - PUT => "/api/v1/libraries/$branchcode" => json => $library_with_updated_field ); - $tx->req->cookies( - { name => 'CGISESSID', value => $authorized_session_id } ); + $tx = $t->ua->build_tx( PUT => "/api/v1/libraries/$library_id" => json => $library_with_updated_field ); + $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx) - ->status_is(200) - ->json_is( '/branchname' => 'Library Name' ); + ->status_is(200, 'SWAGGER3.2.1') + ->json_is( '' => $library_with_updated_field, 'SWAGGER3.3.3' ); # Authorized attempt to write invalid data my $library_with_invalid_field = { %$library_with_updated_field }; $library_with_invalid_field->{'branchinvalid'} = 'Library invalid'; $tx = $t->ua->build_tx( - PUT => "/api/v1/libraries/$branchcode" => json => $library_with_invalid_field ); + PUT => "/api/v1/libraries/$library_id" => json => $library_with_invalid_field ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); @@ -364,8 +333,8 @@ subtest 'delete() tests' => sub { { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx) - ->status_is(204) - ->content_is(''); + ->status_is(204, 'SWAGGER3.2.4') + ->content_is('', 'SWAGGER3.3.4'); $tx = $t->ua->build_tx( DELETE => "/api/v1/libraries/$branchcode" ); $tx->req->cookies( -- 2.39.5