From c99e814a6fd5b9e05deaa0adc4fbe8e37aef58fd Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 23 Oct 2017 15:34:01 -0300 Subject: [PATCH] Bug 7317: (followup) Migrate endpoint to OpenAPI This patch moves the current endpoint implementation from Swagger2 to the OpenAPI plugin. It also takes advantage of the overloaded Koha::Illrequest::TO_JSON method which has now the option to embed what's needed for the REST api. The path spec is adjusted to fit OpenAPI, and some minor fixes are applied: - Missing 'metadata' query param - 'ill' permissions should be required instead of 'borrowers' - Full test coverage To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Magnus Enger Signed-off-by: Tomas Cohen Arazi Signed-off-by: Benjamin Rokseth Signed-off-by: Jonathan Druart --- Koha/REST/V1/Illrequests.pm | 50 +++--------- api/v1/swagger/paths/illrequests.json | 41 ++++++++-- t/db_dependent/api/v1/illrequests.t | 113 +++++++++++++++----------- 3 files changed, 112 insertions(+), 92 deletions(-) diff --git a/Koha/REST/V1/Illrequests.pm b/Koha/REST/V1/Illrequests.pm index 0807eb1bf6..947fb7972f 100644 --- a/Koha/REST/V1/Illrequests.pm +++ b/Koha/REST/V1/Illrequests.pm @@ -20,13 +20,13 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; use Koha::Illrequests; -use Koha::Library; +use Koha::Libraries; sub list { - my ($c, $args, $cb) = @_; + my $c = shift->openapi->valid_input or return; + my $args = $c->req->params->to_hash // {}; my $filter; - $args //= {}; my $output = []; # Create a hash where all keys are embedded values @@ -44,42 +44,16 @@ sub list { my $requests = Koha::Illrequests->search($filter); - while (my $request = $requests->next) { - my $unblessed = $request->unblessed; - # Add the request's id_prefix - $unblessed->{id_prefix} = $request->id_prefix; - # Augment the request response with patron details - # if appropriate - if (defined $embed{patron}) { - my $patron = $request->patron; - $unblessed->{patron} = { - firstname => $patron->firstname, - surname => $patron->surname, - cardnumber => $patron->cardnumber - }; - } - # Augment the request response with metadata details - # if appropriate - if (defined $embed{metadata}) { - $unblessed->{metadata} = $request->metadata; - } - # Augment the request response with status details - # if appropriate - if (defined $embed{capabilities}) { - $unblessed->{capabilities} = $request->capabilities; - } - # Augment the request response with branch details - # if appropriate - if (defined $embed{branch}) { - $unblessed->{branch} = Koha::Libraries->find( - $request->branchcode - )->unblessed; - } - push @{$output}, $unblessed + if ( scalar (keys %embed) ) + { + # Need to embed stuff + my @results = map { $_->TO_JSON(\%embed) } $requests->as_list; + return $c->render( status => 200, openapi => \@results ); + } + else + { + return $c->render( status => 200, openapi => $requests ); } - - return $c->$cb( $output, 200 ); - } 1; diff --git a/api/v1/swagger/paths/illrequests.json b/api/v1/swagger/paths/illrequests.json index ddafd80216..5372cb9d09 100644 --- a/api/v1/swagger/paths/illrequests.json +++ b/api/v1/swagger/paths/illrequests.json @@ -1,8 +1,8 @@ { "/illrequests": { "get": { - "x-mojo-controller": "Koha::REST::V1::Illrequests", - "operationId": "list", + "x-mojo-to": "Illrequests#list", + "operationId": "listIllrequests", "tags": ["illrequests"], "parameters": [{ "name": "embed", @@ -16,7 +16,8 @@ "enum": [ "patron", "branch", - "capabilities" + "capabilities", + "metadata" ] } }, { @@ -85,12 +86,42 @@ ], "responses": { "200": { - "description": "OK" + "description": "A list of ILL requests" + }, + "401": { + "description": "Authentication required", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "404": { + "description": "ILL requests not found", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "500": { + "description": "Internal server error", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "503": { + "description": "Under maintenance", + "schema": { + "$ref": "../definitions.json#/error" + } } }, "x-koha-authorization": { "permissions": { - "borrowers": "1" + "ill": "1" } } } diff --git a/t/db_dependent/api/v1/illrequests.t b/t/db_dependent/api/v1/illrequests.t index fb306ba144..cd1b04a1d6 100644 --- a/t/db_dependent/api/v1/illrequests.t +++ b/t/db_dependent/api/v1/illrequests.t @@ -18,6 +18,7 @@ use Modern::Perl; use Test::More tests => 1; +use Test::MockModule; use Test::Mojo; use Test::Warn; @@ -30,8 +31,6 @@ use Koha::Illrequests; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; -# FIXME: sessionStorage defaults to mysql, but it seems to break transaction handling -# this affects the other REST api tests t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' ); my $remote_address = '127.0.0.1'; @@ -39,13 +38,19 @@ my $t = Test::Mojo->new('Koha::REST::V1'); subtest 'list() tests' => sub { - plan tests => 6; + plan tests => 15; + + my $illreqmodule = Test::MockModule->new('Koha::Illrequest'); + # Mock ->capabilities + $illreqmodule->mock( 'capabilities', sub { return 'capable'; } ); + # Mock ->metadata + $illreqmodule->mock( 'metadata', sub { return 'metawhat?'; } ); $schema->storage->txn_begin; Koha::Illrequests->search->delete; - my ( $borrowernumber, $session_id ) = - create_user_and_session( { authorized => 1 } ); + # ill => 22 (userflags.sql) + my ( $borrowernumber, $session_id ) = create_user_and_session({ authorized => 22 }); ## Authorized user tests # No requests, so empty array should be returned @@ -54,40 +59,56 @@ subtest 'list() tests' => sub { $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx)->status_is(200)->json_is( [] ); -# my $city_country = 'France'; -# my $city = $builder->build( -# { source => 'City', value => { city_country => $city_country } } ); -# -# # One city created, should get returned -# $tx = $t->ua->build_tx( GET => '/api/v1/cities' ); -# $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); -# $tx->req->env( { REMOTE_ADDR => $remote_address } ); -# $t->request_ok($tx)->status_is(200)->json_is( [$city] ); -# -# my $another_city = $builder->build( -# { source => 'City', value => { city_country => $city_country } } ); -# my $city_with_another_country = $builder->build( { source => 'City' } ); -# -# # Two cities created, they should both be returned -# $tx = $t->ua->build_tx( GET => '/api/v1/cities' ); -# $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); -# $tx->req->env( { REMOTE_ADDR => $remote_address } ); -# $t->request_ok($tx)->status_is(200) -# ->json_is( [ $city, $another_city, $city_with_another_country ] ); -# -# # Filtering works, two cities sharing city_country -# $tx = -# $t->ua->build_tx( GET => "/api/v1/cities?city_country=" . $city_country ); -# $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); -# $tx->req->env( { REMOTE_ADDR => $remote_address } ); -# my $result = -# $t->request_ok($tx)->status_is(200)->json_is( [ $city, $another_city ] ); -# -# $tx = $t->ua->build_tx( -# GET => "/api/v1/cities?city_name=" . $city->{city_name} ); -# $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); -# $tx->req->env( { REMOTE_ADDR => $remote_address } ); -# $t->request_ok($tx)->status_is(200)->json_is( [$city] ); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + + # Create an ILL request + my $illrequest = $builder->build_object( + { + class => 'Koha::Illrequests', + value => { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber + } + } + ); + + # One illrequest created, should get returned + $tx = $t->ua->build_tx( GET => '/api/v1/illrequests' ); + $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + $t->request_ok($tx)->status_is(200)->json_is( [ $illrequest->TO_JSON ] ); + + # One illrequest created, returned with augmented data + $tx = $t->ua->build_tx( GET => + '/api/v1/illrequests?embed=patron,branch,capabilities,metadata' ); + $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + $t->request_ok($tx)->status_is(200)->json_is( + [ + $illrequest->TO_JSON( + { patron => 1, branch => 1, capabilities => 1, metadata => 1 } + ) + ] + ); + + # Create another ILL request + my $illrequest2 = $builder->build_object( + { + class => 'Koha::Illrequests', + value => { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber + } + } + ); + + # Two illrequest created, should get returned + $tx = $t->ua->build_tx( GET => '/api/v1/illrequests' ); + $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + $t->request_ok($tx)->status_is(200) + ->json_is( [ $illrequest->TO_JSON, $illrequest2->TO_JSON ] ); # Warn on unsupported query parameter $tx = $t->ua->build_tx( GET => '/api/v1/illrequests?request_blah=blah' ); @@ -102,9 +123,10 @@ subtest 'list() tests' => sub { sub create_user_and_session { - my $args = shift; - my $flags = ( $args->{authorized} ) ? $args->{authorized} : 0; - my $dbh = C4::Context->dbh; + my $args = shift; + my $dbh = C4::Context->dbh; + + my $flags = ( $args->{authorized} ) ? 2**$args->{authorized} : 0; my $user = $builder->build( { @@ -123,13 +145,6 @@ sub create_user_and_session { $session->param( 'lasttime', time() ); $session->flush; - if ( $args->{authorized} ) { - $dbh->do( " - INSERT INTO user_permissions (borrowernumber,module_bit,code) - VALUES (?,3,'parameters_remaining_permissions')", undef, - $user->{borrowernumber} ); - } - return ( $user->{borrowernumber}, $session->id ); } -- 2.39.5