Bugzilla – Attachment 84592 Details for
Bug 16497
Add routes for library retrieval, update and deletion
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 16497: (follow-up) Adapt to existing guidelines and RFC
Bug-16497-follow-up-Adapt-to-existing-guidelines-a.patch (text/plain), 42.54 KB, created by
Martin Renvoize (ashimema)
on 2019-01-31 15:58:47 UTC
(
hide
)
Description:
Bug 16497: (follow-up) Adapt to existing guidelines and RFC
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2019-01-31 15:58:47 UTC
Size:
42.54 KB
patch
obsolete
>From 26280d82e2ad3530cab7b303f0430a743148db0b Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> > >Signed-off-by: Josef Moravec <josef.moravec@gmail.com> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > 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 077ebdd2b1..c4104cb9b3 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 => "<p>Library OPAC info</p>", >- }; >+ >+ 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 => "<p>Library OPAC info</p>", >- }; >+ 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.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 16497
:
51454
|
52905
|
53818
|
53819
|
54943
|
60801
|
60802
|
61164
|
61165
|
63160
|
63161
|
66837
|
66838
|
68746
|
68747
|
68748
|
68838
|
68839
|
68840
|
68841
|
69508
|
83913
|
83914
|
83915
|
83916
|
83917
|
83943
|
83945
|
83946
|
83947
|
83948
|
83949
|
84198
|
84588
|
84589
|
84590
|
84591
| 84592 |
84593
|
84734
|
84769