From 9465afd3b3c12aa1ec5b4007609b416421d34c57 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 23 Apr 2025 12:09:36 -0300 Subject: [PATCH] Bug 38340: Rewrite API controllers This patch splits the controller in two: * A controller for staff users use * An unprivileged access controller The public one makes use of the state-of-the-art in Koha for public endpoints, using the `$c->auth->public` validation helper and leaving the (newly added) public path spec with no special permissions requirements. The staff-oriented endpoint gets the right permissions added (at least those used in the main `/ill/requests` endpoint. TODO: Tests need to be split for both controllers, and adjusted. TODO: The OPAC changes need to be redone using the public endpoint. TODO: The OPAC table should use kohaTable. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/api/v1/ill_requests.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi --- Koha/REST/V1/ILL/Requests.pm | 44 ++++++--- api/v1/swagger/paths/patrons.yaml | 4 +- api/v1/swagger/paths/public_patrons.yaml | 65 ++++++++++++- api/v1/swagger/swagger.yaml | 4 +- t/db_dependent/api/v1/ill_requests.t | 115 ++++++++++++++++++----- 5 files changed, 189 insertions(+), 43 deletions(-) diff --git a/Koha/REST/V1/ILL/Requests.pm b/Koha/REST/V1/ILL/Requests.pm index bb8eecb15e1..3f4652790ae 100644 --- a/Koha/REST/V1/ILL/Requests.pm +++ b/Koha/REST/V1/ILL/Requests.pm @@ -60,32 +60,48 @@ sub list { =head3 patron_list -Controller function that handles listing Koha::ILL::Request objects for a given patron. - -The patron must match the requesting user unless the requesting user is a superlibrarian. - -This is a public route, so some request details are omitted. +Controller function that returns a patron's list of ILL requests. =cut sub patron_list { - my $c = shift->openapi->valid_input or return; - my $user = $c->stash('koha.user'); - $c->stash( is_public => 1 ); + my $c = shift->openapi->valid_input or return; + + my $patron = Koha::Patrons->find( $c->param('patron_id') ); + + return $c->render_resource_not_found('Patron') + unless $patron; + + return try { - if ( $user->borrowernumber != $c->param('patron_id') and !$user->is_superlibrarian ) { return $c->render( - status => 403, - openapi => { error => "Cannot lookup ILL requests for other users" } + status => 200, + openapi => $c->objects->search( $patron->ill_requests ), ); - } + } catch { + $c->unhandled_exception($_); + }; +} + +=head3 public_patron_list + +Controller function that returns a patron's list of ILL requests for unprivileged +access. + +=cut + +sub public_patron_list { + my $c = shift->openapi->valid_input or return; return try { - my $reqs = $c->objects->search( Koha::ILL::Requests->search( { borrowernumber => $c->param('patron_id') } ) ); + + $c->auth->public( $c->param('patron_id') ); + + my $patron = $c->stash('koha.user'); return $c->render( status => 200, - openapi => $reqs, + openapi => $c->objects->search( $patron->ill_requests() ), ); } catch { $c->unhandled_exception($_); diff --git a/api/v1/swagger/paths/patrons.yaml b/api/v1/swagger/paths/patrons.yaml index dca5ce64bed..65ac96a2ced 100644 --- a/api/v1/swagger/paths/patrons.yaml +++ b/api/v1/swagger/paths/patrons.yaml @@ -667,7 +667,6 @@ x-mojo-to: ILL::Requests#patron_list operationId: getPatronIllRequests tags: - - patrons - ill_requests summary: Get patron's ILL requests parameters: @@ -727,4 +726,5 @@ schema: $ref: "../swagger.yaml#/definitions/error" x-koha-authorization: - permissions: {} + permissions: + ill: "1" diff --git a/api/v1/swagger/paths/public_patrons.yaml b/api/v1/swagger/paths/public_patrons.yaml index 7952eabfaad..81aee733f06 100644 --- a/api/v1/swagger/paths/public_patrons.yaml +++ b/api/v1/swagger/paths/public_patrons.yaml @@ -277,4 +277,67 @@ "503": description: Under maintenance schema: - $ref: "../swagger.yaml#/definitions/error" \ No newline at end of file + $ref: "../swagger.yaml#/definitions/error" +"/public/patrons/{patron_id}/ill/requests": + get: + x-mojo-to: ILL::Requests#public_patron_list + operationId: getPublicPatronIllRequests + tags: + - ill_requests + summary: Get patron's ILL requests + parameters: + - $ref: "../swagger.yaml#/parameters/patron_id_pp" + - $ref: "../swagger.yaml#/parameters/page" + - $ref: "../swagger.yaml#/parameters/per_page" + - $ref: "../swagger.yaml#/parameters/match" + - $ref: "../swagger.yaml#/parameters/order_by" + - $ref: "../swagger.yaml#/parameters/q_param" + - $ref: "../swagger.yaml#/parameters/q_body" + - $ref: "../swagger.yaml#/parameters/request_id_header" + - name: x-koha-embed + in: header + required: false + description: Embed list sent as a request header + type: array + items: + type: string + enum: + - +strings + - extended_attributes + collectionFormat: csv + produces: + - application/json + responses: + "200": + description: A list of parton's ILL requests + schema: + type: array + items: + $ref: "../swagger.yaml#/definitions/ill_request" + "400": + description: Bad request + schema: + $ref: "../swagger.yaml#/definitions/error" + "401": + description: Authentication required + schema: + $ref: "../swagger.yaml#/definitions/error" + "403": + description: Access forbidden + schema: + $ref: "../swagger.yaml#/definitions/error" + "404": + description: Patron not found + schema: + $ref: "../swagger.yaml#/definitions/error" + "500": + description: | + Internal server error. Possible `error_code` attribute values: + + * `internal_server_error` + schema: + $ref: "../swagger.yaml#/definitions/error" + "503": + description: Under maintenance + schema: + $ref: "../swagger.yaml#/definitions/error" diff --git a/api/v1/swagger/swagger.yaml b/api/v1/swagger/swagger.yaml index 18e275965d5..e583215a8be 100644 --- a/api/v1/swagger/swagger.yaml +++ b/api/v1/swagger/swagger.yaml @@ -531,6 +531,8 @@ paths: $ref: "./paths/public_patrons.yaml#/~1public~1patrons~1{patron_id}~1guarantors~1can_see_checkouts" "/public/patrons/{patron_id}/holds/{hold_id}": $ref: "./paths/public_patrons.yaml#/~1public~1patrons~1{patron_id}~1holds~1{hold_id}" + "/public/patrons/{patron_id}/ill/requests": + $ref: "./paths/public_patrons.yaml#/~1public~1patrons~1{patron_id}~1ill~1requests" "/public/patrons/{patron_id}/password": $ref: "./paths/public_patrons.yaml#/~1public~1patrons~1{patron_id}~1password" "/public/tickets": @@ -1295,4 +1297,4 @@ tags: x-displayName: Two factor authentication - description: "Manage vendors for the acquisitions module\n" name: vendors - x-displayName: Vendors \ No newline at end of file + x-displayName: Vendors diff --git a/t/db_dependent/api/v1/ill_requests.t b/t/db_dependent/api/v1/ill_requests.t index 2015d8b0453..d44c6ccad79 100755 --- a/t/db_dependent/api/v1/ill_requests.t +++ b/t/db_dependent/api/v1/ill_requests.t @@ -18,7 +18,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 4; +use Test::More tests => 5; use Test::MockModule; use Test::MockObject; @@ -264,9 +264,11 @@ subtest 'list() tests' => sub { $schema->storage->txn_rollback; }; -subtest 'patron_list() tests' => sub { +subtest 'public_patron_list() tests' => sub { + + plan tests => 13; - plan tests => 15; + $schema->storage->txn_begin; # Mock ILLBackend (as object) my $backend = Test::MockObject->new; @@ -283,36 +285,36 @@ subtest 'patron_list() tests' => sub { sub { my $self = shift; $self->{_my_backend} = $backend; return $self } ); - $schema->storage->txn_begin; - - Koha::ILL::Requests->search->delete; + my $password = 'thePassword123'; - my $requester = $builder->build_object( + my $patron_1 = $builder->build_object( { class => 'Koha::Patrons', value => { flags => 0 } } ); - my $password = 'thePassword123'; - $requester->set_password( { password => $password, skip_validation => 1 } ); - my $requester_userid = $requester->userid; - my $requester_number = $requester->borrowernumber; + my $patron_1_userid = $patron_1->userid; + my $patron_1_id = $patron_1->id; + + $patron_1->set_password( { password => $password, skip_validation => 1 } ); - my $observer = $builder->build_object( + my $patron_2 = $builder->build_object( { class => 'Koha::Patrons', value => { flags => 0 } } ); - $observer->set_password( { password => $password, skip_validation => 1 } ); - my $observer_userid = $observer->userid; - my $observer_number = $observer->borrowernumber; + $patron_2->set_password( { password => $password, skip_validation => 1 } ); + + my $patron_2_userid = $patron_2->userid; + my $patron_2_id = $patron_2->id; # No requests yet, expect empty for both - $t->get_ok("//$requester_userid:$password@/api/v1/patrons/$requester_number/ill/requests")->status_is(200) + $t->get_ok("//$patron_1_userid:$password@/api/v1/public/patrons/$patron_1_id/ill/requests")->status_is(200) ->json_is( [] ); - $t->get_ok("//$observer_userid:$password@/api/v1/patrons/$observer_number/ill/requests")->status_is(200) + + $t->get_ok("//$patron_2_userid:$password@/api/v1/public/patrons/$patron_2_id/ill/requests")->status_is(200) ->json_is( [] ); for ( 0 .. 25 ) { @@ -320,7 +322,7 @@ subtest 'patron_list() tests' => sub { { class => 'Koha::ILL::Requests', value => { - borrowernumber => $requester_number, + borrowernumber => $patron_1_id, batch_id => undef, status => 'NEW', backend => $backend->name, @@ -331,20 +333,83 @@ subtest 'patron_list() tests' => sub { } # Other user should not see anything - $t->get_ok("//$observer_userid:$password@/api/v1/patrons/$observer_number/ill/requests")->status_is(200) + $t->get_ok("//$patron_2_userid:$password@/api/v1/public/patrons/$patron_2_id/ill/requests")->status_is(200) ->json_is( [] ); # Staff notes hidden in the public API - $t->get_ok("//$requester_userid:$password@/api/v1/patrons/$requester_number/ill/requests")->status_is(200) - ->json_is( + $t->get_ok("//$patron_1_userid:$password@/api/v1/public/patrons/$patron_1_id/ill/requests?_per_page=-1") + ->status_is(200)->json_is( '/0/staff_notes' => undef, ); - # Not all requests get returned, pagination works - $t->get_ok("//$requester_userid:$password@/api/v1/patrons/$requester_number/ill/requests")->status_is(200) - ->json_is( - '/21' => undef, + my $reqs = $t->tx->res->json; + is( scalar @{$reqs}, 26, 'All ILL requests returned for the patron' ); + + $schema->storage->txn_rollback; +}; + +subtest 'patron_list() tests' => sub { + + plan tests => 7; + + $schema->storage->txn_begin; + + # Mock ILLBackend (as object) + my $backend = Test::MockObject->new; + $backend->set_isa('Koha::Illbackends::Mock'); + $backend->set_always( 'name', 'Mock' ); + $backend->mock( + 'status_graph', sub { }, + ); + + # Mock Koha::ILL::Request::load_backend (to load Mocked Backend) + my $illreqmodule = Test::MockModule->new('Koha::ILL::Request'); + $illreqmodule->mock( + 'load_backend', + sub { my $self = shift; $self->{_my_backend} = $backend; return $self } + ); + + my $password = 'thePassword123'; + + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 2**22 } # 22 => ill + } + ); + + my $user_id = $patron->userid; + my $patron_id = $patron->id; + + $patron->set_password( { password => $password, skip_validation => 1 } ); + + # No requests yet, expect empty for both + $t->get_ok("//$user_id:$password@/api/v1/patrons/$patron_id/ill/requests")->status_is(200)->json_is( [] ); + + my $secret_notes = 'secret staff notes'; + + for ( 0 .. 25 ) { + $builder->build_object( + { + class => 'Koha::ILL::Requests', + value => { + borrowernumber => $patron_id, + batch_id => undef, + status => 'NEW', + backend => $backend->name, + notesstaff => $secret_notes, + } + } ); + } + + # Staff notes available in the staff endpoint + $t->get_ok("//$user_id:$password@/api/v1/patrons/$patron_id/ill/requests?_per_page=-1")->status_is(200)->json_is( + '/0/staff_notes' => $secret_notes, + ); + + my $reqs = $t->tx->res->json; + is( scalar @{$reqs}, 26, 'All ILL requests returned for the patron' ); $schema->storage->txn_rollback; }; -- 2.49.0