From dfbadc70d4d6b117b9287beda8b2b46dced6b432 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 17 Jan 2018 14:03:28 -0300 Subject: [PATCH] Bug 20004: Adapt /v1/cities to new naming guidelines This patch implements the changes required by the cities endpoint RFC [1]. It uses the objects.search helper, and relies on bug 19686. To test: - Apply the patches - Compare the spec with the RFC (api/v1/swagger/definitions/city.json) => SUCCESS: It makes sense - Run: $ kshell k$ prove t/db_dependent/api/v1/cities.t => Tests pass! - Sign off :-D Signed-off-by: Claire Gravely Signed-off-by: Kyle M Hall Signed-off-by: Jonathan Druart --- Koha/REST/V1/Cities.pm | 149 ++++++++++++++++++++------- api/v1/swagger/definitions/city.json | 16 +-- api/v1/swagger/parameters.json | 4 +- api/v1/swagger/parameters/city.json | 6 +- api/v1/swagger/paths.json | 4 +- api/v1/swagger/paths/cities.json | 26 ++--- api/v1/swagger/x-primitives.json | 2 +- 7 files changed, 142 insertions(+), 65 deletions(-) diff --git a/Koha/REST/V1/Cities.pm b/Koha/REST/V1/Cities.pm index 3003e3aefe..3c7c5e7c4c 100644 --- a/Koha/REST/V1/Cities.pm +++ b/Koha/REST/V1/Cities.pm @@ -19,7 +19,6 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; -use Koha::City; use Koha::Cities; use Try::Tiny; @@ -27,16 +26,9 @@ use Try::Tiny; sub list { my $c = shift->openapi->valid_input or return; - my $cities; - my $filter; - my $args = $c->req->params->to_hash; - - for my $filter_param ( keys %$args ) { - $filter->{$filter_param} = { LIKE => $args->{$filter_param} . "%" }; - } - return try { - $cities = Koha::Cities->search($filter); + my $cities_set = Koha::Cities->new; + my $cities = $c->objects->search( $cities_set, \&_to_model, \&_to_api ); return $c->render( status => 200, openapi => $cities ); } catch { @@ -55,32 +47,35 @@ sub list { sub get { my $c = shift->openapi->valid_input or return; - my $city = Koha::Cities->find( $c->validation->param('cityid') ); + my $city = Koha::Cities->find( $c->validation->param('city_id') ); unless ($city) { return $c->render( status => 404, openapi => { error => "City not found" } ); } - return $c->render( status => 200, openapi => $city ); + return $c->render( status => 200, openapi => _to_api($city->TO_JSON) ); } sub add { my $c = shift->openapi->valid_input or return; - my $city = Koha::City->new( $c->validation->param('body') ); - return try { + my $city = Koha::City->new( _to_model( $c->validation->param('body') ) ); $city->store; - return $c->render( status => 200, openapi => $city ); + return $c->render( status => 200, openapi => _to_api($city->TO_JSON) ); } catch { if ( $_->isa('DBIx::Class::Exception') ) { - return $c->render( status => 500, - openapi => { error => $_->{msg} } ); + return $c->render( + status => 500, + openapi => { error => $_->{msg} } + ); } else { - return $c->render( status => 500, - openapi => { error => "Something went wrong, check the logs."} ); + return $c->render( + status => 500, + openapi => { error => "Something went wrong, check the logs." } + ); } }; } @@ -88,21 +83,21 @@ sub add { sub update { my $c = shift->openapi->valid_input or return; - my $city; + my $city = Koha::Cities->find( $c->validation->param('city_id') ); + + if ( not defined $city ) { + return $c->render( status => 404, + openapi => { error => "Object not found" } ); + } return try { - $city = Koha::Cities->find( $c->validation->param('cityid') ); my $params = $c->req->json; - $city->set( $params ); + $city->set( _to_model($params) ); $city->store(); - return $c->render( status => 200, openapi => $city ); + return $c->render( status => 200, openapi => _to_api($city->TO_JSON) ); } catch { - if ( not defined $city ) { - return $c->render( status => 404, - openapi => { error => "Object not found" } ); - } - elsif ( $_->isa('Koha::Exceptions::Object') ) { + if ( $_->isa('Koha::Exceptions::Object') ) { return $c->render( status => 500, openapi => { error => $_->message } ); } @@ -111,25 +106,23 @@ sub update { openapi => { error => "Something went wrong, check the logs."} ); } }; - } sub delete { my $c = shift->openapi->valid_input or return; - my $city; + my $city = Koha::Cities->find( $c->validation->param('city_id') ); + if ( not defined $city ) { + return $c->render( status => 404, + openapi => { error => "Object not found" } ); + } return try { - $city = Koha::Cities->find( $c->validation->param('cityid') ); $city->delete; return $c->render( status => 200, openapi => "" ); } catch { - if ( not defined $city ) { - return $c->render( status => 404, - openapi => { error => "Object not found" } ); - } - elsif ( $_->isa('DBIx::Class::Exception') ) { + if ( $_->isa('DBIx::Class::Exception') ) { return $c->render( status => 500, openapi => { error => $_->{msg} } ); } @@ -138,7 +131,91 @@ sub delete { openapi => { error => "Something went wrong, check the logs."} ); } }; +} + +=head3 _to_api + +Helper function that maps a hashref of Koha::City attributes into REST api +attribute names. + +=cut + +sub _to_api { + my $city = shift; + # Rename attributes + foreach my $column ( keys %{ $Koha::REST::V1::Cities::to_api_mapping } ) { + my $mapped_column = $Koha::REST::V1::Cities::to_api_mapping->{$column}; + if ( exists $city->{ $column } + && defined $mapped_column ) + { + # key /= undef + $city->{ $mapped_column } = delete $city->{ $column }; + } + elsif ( exists $city->{ $column } + && !defined $mapped_column ) + { + # key == undef => to be deleted + delete $city->{ $column }; + } + } + + return $city; } +=head3 _to_model + +Helper function that maps REST api objects into Koha::Cities +attribute names. + +=cut + +sub _to_model { + my $city = shift; + + foreach my $attribute ( keys %{ $Koha::REST::V1::Cities::to_model_mapping } ) { + my $mapped_attribute = $Koha::REST::V1::Cities::to_model_mapping->{$attribute}; + if ( exists $city->{ $attribute } + && defined $mapped_attribute ) + { + # key /= undef + $city->{ $mapped_attribute } = delete $city->{ $attribute }; + } + elsif ( exists $city->{ $attribute } + && !defined $mapped_attribute ) + { + # key == undef => to be deleted + delete $city->{ $attribute }; + } + } + + return $city; +} + +=head2 Global variables + +=head3 $to_api_mapping + +=cut + +our $to_api_mapping = { + cityid => 'city_id', + city_country => 'country', + city_name => 'name', + city_state => 'state', + city_zipcode => 'postal_code' +}; + +=head3 $to_model_mapping + +=cut + +our $to_model_mapping = { + city_id => 'cityid', + country => 'city_country', + name => 'city_name', + postal_code => 'city_zipcode', + state => 'city_state' +}; + 1; diff --git a/api/v1/swagger/definitions/city.json b/api/v1/swagger/definitions/city.json index f322136fd9..8643e98c14 100644 --- a/api/v1/swagger/definitions/city.json +++ b/api/v1/swagger/definitions/city.json @@ -1,26 +1,26 @@ { "type": "object", "properties": { - "cityid": { - "$ref": "../x-primitives.json#/cityid" + "city_id": { + "$ref": "../x-primitives.json#/city_id" }, - "city_name": { + "name": { "description": "city name", "type": "string" }, - "city_state": { + "state": { "description": "city state", "type": ["string", "null"] }, - "city_zipcode": { - "description": "city zipcode", + "postal_code": { + "description": "city postal code", "type": ["string", "null"] }, - "city_country": { + "country": { "description": "city country", "type": ["string", "null"] } }, "additionalProperties": false, - "required": ["city_name", "city_state", "city_zipcode", "city_country"] + "required": ["name", "state", "postal_code", "country"] } diff --git a/api/v1/swagger/parameters.json b/api/v1/swagger/parameters.json index 1da158c3f4..584a0c2112 100644 --- a/api/v1/swagger/parameters.json +++ b/api/v1/swagger/parameters.json @@ -5,8 +5,8 @@ "borrowernumberQueryParam": { "$ref": "parameters/patron.json#/borrowernumberQueryParam" }, - "cityidPathParam": { - "$ref": "parameters/city.json#/cityidPathParam" + "city_id_pp": { + "$ref": "parameters/city.json#/city_id_pp" }, "holdIdPathParam": { "$ref": "parameters/hold.json#/holdIdPathParam" diff --git a/api/v1/swagger/parameters/city.json b/api/v1/swagger/parameters/city.json index c35df74b9f..e971f90160 100644 --- a/api/v1/swagger/parameters/city.json +++ b/api/v1/swagger/parameters/city.json @@ -1,8 +1,8 @@ { - "cityidPathParam": { - "name": "cityid", + "city_id_pp": { + "name": "city_id", "in": "path", - "description": "City id", + "description": "City internal identifier", "required": true, "type": "integer" } diff --git a/api/v1/swagger/paths.json b/api/v1/swagger/paths.json index 9c3a6d2efd..df4f55a999 100644 --- a/api/v1/swagger/paths.json +++ b/api/v1/swagger/paths.json @@ -8,8 +8,8 @@ "/cities": { "$ref": "paths/cities.json#/~1cities" }, - "/cities/{cityid}": { - "$ref": "paths/cities.json#/~1cities~1{cityid}" + "/cities/{city_id}": { + "$ref": "paths/cities.json#/~1cities~1{city_id}" }, "/holds": { "$ref": "paths/holds.json#/~1holds" diff --git a/api/v1/swagger/paths/cities.json b/api/v1/swagger/paths/cities.json index 2fe9f49666..cb51dcba2a 100644 --- a/api/v1/swagger/paths/cities.json +++ b/api/v1/swagger/paths/cities.json @@ -8,27 +8,27 @@ "application/json" ], "parameters": [{ - "name": "city_name", + "name": "name", "in": "query", - "description": "Case insensative 'starts-with' search on city name", + "description": "Case insensative search on city name", "required": false, "type": "string" }, { - "name": "city_state", + "name": "state", "in": "query", - "description": "Case insensative 'starts-with' search on city state", + "description": "Case insensative search on city state", "required": false, "type": "string" }, { - "name": "city_country", + "name": "country", "in": "query", - "description": "Case insensative 'starts_with' search on city country", + "description": "Case insensative search on city country", "required": false, "type": "string" }, { - "name": "city_zipcode", + "name": "postal_code", "in": "query", - "description": "Case Insensative 'starts-with' search on zipcode", + "description": "Case Insensative search on city postal code", "required": false, "type": "string" }], @@ -117,13 +117,13 @@ } } }, - "/cities/{cityid}": { + "/cities/{city_id}": { "get": { "x-mojo-to": "Cities#get", "operationId": "getCity", "tags": ["cities"], "parameters": [{ - "$ref": "../parameters.json#/cityidPathParam" + "$ref": "../parameters.json#/city_id_pp" }], "produces": [ "application/json" @@ -160,11 +160,11 @@ "operationId": "updateCity", "tags": ["cities"], "parameters": [{ - "$ref": "../parameters.json#/cityidPathParam" + "$ref": "../parameters.json#/city_id_pp" }, { "name": "body", "in": "body", - "description": "A JSON object containing informations about the new hold", + "description": "A city object", "required": true, "schema": { "$ref": "../definitions.json#/city" @@ -222,7 +222,7 @@ "operationId": "deleteCity", "tags": ["cities"], "parameters": [{ - "$ref": "../parameters.json#/cityidPathParam" + "$ref": "../parameters.json#/city_id_pp" }], "produces": [ "application/json" diff --git a/api/v1/swagger/x-primitives.json b/api/v1/swagger/x-primitives.json index 12e5e9de39..5fa58badaa 100644 --- a/api/v1/swagger/x-primitives.json +++ b/api/v1/swagger/x-primitives.json @@ -11,7 +11,7 @@ "type": ["string", "null"], "description": "library assigned user identifier" }, - "cityid": { + "city_id": { "type": "integer", "description": "internally assigned city identifier", "readOnly": true -- 2.39.5