Bugzilla – Attachment 181435 Details for
Bug 38340
Translatability - Standard form include files are not translatable
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 38340: Rewrite API controllers
Bug-38340-Rewrite-API-controllers.patch (text/plain), 13.44 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-04-24 11:38:24 UTC
(
hide
)
Description:
Bug 38340: Rewrite API controllers
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-04-24 11:38:24 UTC
Size:
13.44 KB
patch
obsolete
>From 9465afd3b3c12aa1ec5b4007609b416421d34c57 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> >--- > 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
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 38340
:
177749
|
177750
|
178057
|
178058
|
178079
|
178080
|
178414
|
178415
|
178416
|
178418
|
178420
|
178421
|
178422
|
180066
|
180067
|
180068
|
181215
|
181216
|
181217
|
181294
|
181391
|
181392
|
181435
|
181541
|
181542
|
181543
|
181544
|
181549