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 <tomascohen@theke.io>

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/Koha/REST/V1/Illrequests.pm b/Koha/REST/V1/Illrequests.pm
index 0807eb1..947fb79 100644 (file)
--- 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 ddafd80..5372cb9 100644 (file)
--- 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 fb306ba..cd1b04a 100644 (file)
--- 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 );
 }