From d46b6d96ac0b87e966861c3c224214a5810e9eec Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 25 Mar 2024 16:42:18 +0100 Subject: [PATCH] Bug 35353: Add REST API endpoint to retrieve old holds Same as checkout but for holds, we need to provide a way to retrieve old holds for a patron. Test plan: Create some holds for a patron, cancel and fulfill some, then use the REST API endpoint with the new 'old' flag set to 1 /api/v1/patrons/42/holds?old=1 Signed-off-by: Matt Blenkinsop Signed-off-by: Nick Clemens Signed-off-by: Katrin Fischer (cherry picked from commit 7a32231a52d9d1d5007ad574b8e3b48fd4c99878) Signed-off-by: Fridolin Somers --- Koha/Hold.pm | 16 ++++++++++-- Koha/Old/Hold.pm | 26 +++++++++++++++++++ Koha/REST/V1/Holds.pm | 14 ++++++++--- Koha/REST/V1/Patrons/Holds.pm | 18 ++++++++------ api/v1/swagger/definitions/hold.yaml | 15 +++++++++++ api/v1/swagger/paths/holds.yaml | 7 ++++++ api/v1/swagger/paths/patrons_holds.yaml | 9 +++++++ t/db_dependent/Koha/Hold.t | 19 +++++++++++++- t/db_dependent/Koha/Old/Hold.t | 27 +++++++++++++++++++- t/db_dependent/api/v1/patrons_holds.t | 33 ++++++++++++++++++++----- 10 files changed, 163 insertions(+), 21 deletions(-) diff --git a/Koha/Hold.pm b/Koha/Hold.pm index a74ed95b58..5c6939d446 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -574,13 +574,25 @@ sub item_group { =head3 branch -Returns the related Koha::Library object for this Hold +Returns the related Koha::Library object for this hold + +DEPRECATED =cut sub branch { + return shift->pickup_library(@_); +} + +=head3 pickup_library + +Returns the related Koha::Library object for this hold + +=cut + +sub pickup_library { my ($self) = @_; - my $rs = $self->_result->branchcode; + my $rs = $self->_result->pickup_library; return Koha::Library->_new_from_dbic($rs); } diff --git a/Koha/Old/Hold.pm b/Koha/Old/Hold.pm index 91f2adb39c..8f97163c6e 100644 --- a/Koha/Old/Hold.pm +++ b/Koha/Old/Hold.pm @@ -47,6 +47,32 @@ sub biblio { return Koha::Biblio->_new_from_dbic($rs); } +=head3 item + +Returns the related Koha::Item object for this old Hold + +=cut + +sub item { + my ($self) = @_; + my $rs = $self->_result->itemnumber; + return unless $rs; + return Koha::Item->_new_from_dbic($rs); +} + +=head3 pickup_library + +Returns the related Koha::Biblio object for this old hold + +=cut + +sub pickup_library { + my ($self) = @_; + my $rs = $self->_result->pickup_library; + return unless $rs; + return Koha::Library->_new_from_dbic($rs); +} + =head3 anonymize $old_hold->anonymize(); diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm index e34d23199b..d5069bc9f5 100644 --- a/Koha/REST/V1/Holds.pm +++ b/Koha/REST/V1/Holds.pm @@ -26,6 +26,7 @@ use C4::Reserves; use Koha::Items; use Koha::Patrons; use Koha::Holds; +use Koha::Old::Holds; use Koha::DateUtils qw( dt_from_string ); use List::MoreUtils qw( any ); @@ -44,11 +45,18 @@ Method that handles listing Koha::Hold objects sub list { my $c = shift->openapi->valid_input or return; + my $old = $c->param('old'); + $c->req->params->remove('old'); + return try { - my $holds = $c->objects->search( Koha::Holds->new ); + my $holds_set = + $old + ? Koha::Old::Holds->new + : Koha::Holds->new; + + my $holds = $c->objects->search($holds_set); return $c->render( status => 200, openapi => $holds ); - } - catch { + } catch { $c->unhandled_exception($_); }; } diff --git a/Koha/REST/V1/Patrons/Holds.pm b/Koha/REST/V1/Patrons/Holds.pm index 88eea15b53..d3aa97ca91 100644 --- a/Koha/REST/V1/Patrons/Holds.pm +++ b/Koha/REST/V1/Patrons/Holds.pm @@ -49,16 +49,18 @@ sub list { ); } - return try { + my $old = $c->param('old'); + $c->req->params->remove('old'); - my $holds = $c->objects->search( $patron->holds ); + return try { + my $holds_set = + $old + ? $patron->old_holds + : $patron->holds; - return $c->render( - status => 200, - openapi => $holds - ); - } - catch { + my $holds = $c->objects->search( $holds_set ); + return $c->render( status => 200, openapi => $holds ); + } catch { $c->unhandled_exception($_); }; } diff --git a/api/v1/swagger/definitions/hold.yaml b/api/v1/swagger/definitions/hold.yaml index f97c3e216b..ae2c055a30 100644 --- a/api/v1/swagger/definitions/hold.yaml +++ b/api/v1/swagger/definitions/hold.yaml @@ -108,5 +108,20 @@ properties: - boolean - "null" description: Cancellation requests count for the hold (x-koha-embed) + biblio: + type: + - object + - "null" + description: Bibliographic record + item: + type: + - object + - "null" + description: The item + pickup_library: + type: + - object + - "null" + description: Pickup library additionalProperties: false diff --git a/api/v1/swagger/paths/holds.yaml b/api/v1/swagger/paths/holds.yaml index 7a1055e391..6458ff72ef 100644 --- a/api/v1/swagger/paths/holds.yaml +++ b/api/v1/swagger/paths/holds.yaml @@ -88,6 +88,11 @@ - $ref: "../swagger.yaml#/parameters/q_param" - $ref: "../swagger.yaml#/parameters/q_body" - $ref: "../swagger.yaml#/parameters/request_id_header" + - name: old + in: query + description: By default, current holds are returned, when this is true then + old holds are returned as result. + type: boolean - name: x-koha-embed in: header required: false @@ -97,6 +102,8 @@ type: string enum: - cancellation_requested + - biblio + - pickup_library collectionFormat: csv produces: - application/json diff --git a/api/v1/swagger/paths/patrons_holds.yaml b/api/v1/swagger/paths/patrons_holds.yaml index 7204be98cf..c8a4750b91 100644 --- a/api/v1/swagger/paths/patrons_holds.yaml +++ b/api/v1/swagger/paths/patrons_holds.yaml @@ -15,6 +15,11 @@ - $ref: "../swagger.yaml#/parameters/q_param" - $ref: "../swagger.yaml#/parameters/q_body" - $ref: "../swagger.yaml#/parameters/request_id_header" + - name: old + in: query + description: By default, current holds are returned, when this is true then + old holds are returned as result. + type: boolean - name: x-koha-embed in: header required: false @@ -24,6 +29,10 @@ type: string enum: - cancellation_requested + - biblio + - item + - pickup_library + - pickup_library.branchname collectionFormat: csv produces: - application/json diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index aa7f677f64..99dae133a2 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 13; +use Test::More tests => 14; use Test::Exception; use Test::MockModule; @@ -87,6 +87,23 @@ subtest 'biblio() tests' => sub { $schema->storage->txn_rollback; }; +subtest 'pickup_library/branch tests' => sub { + + plan tests => 1; + + $schema->storage->txn_begin; + + my $hold = $builder->build_object( + { + class => 'Koha::Holds', + } + ); + + is( ref( $hold->pickup_library ), 'Koha::Library', '->pickup_library should return a Koha::Library object' ); + + $schema->storage->txn_rollback; +}; + subtest 'fill() tests' => sub { plan tests => 14; diff --git a/t/db_dependent/Koha/Old/Hold.t b/t/db_dependent/Koha/Old/Hold.t index d62a3136fc..3b52b5ee27 100755 --- a/t/db_dependent/Koha/Old/Hold.t +++ b/t/db_dependent/Koha/Old/Hold.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 2; +use Test::More tests => 3; use Test::Exception; use Koha::Database; @@ -118,3 +118,28 @@ subtest 'biblio() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'item/pickup_library() tests' => sub { + + plan tests => 4; + + $schema->storage->txn_begin; + + my $old_hold = $builder->build_object( + { + class => 'Koha::Old::Holds', + } + ); + is( ref( $old_hold->item ), 'Koha::Item', '->item should return a Koha::Item object' ); + is( ref( $old_hold->pickup_library ), 'Koha::Library', '->pickup_library should return a Koha::Library object' ); + + $old_hold->item->delete; + $old_hold->pickup_library->delete; + + $old_hold = $old_hold->get_from_storage; # Be on the safe side + + is( $old_hold->item, undef, 'Item has been deleted, ->item should return undef' ); + is( $old_hold->pickup_library, undef, 'Library has been deleted, ->pickup_library should return undef' ); + + $schema->storage->txn_rollback; +}; diff --git a/t/db_dependent/api/v1/patrons_holds.t b/t/db_dependent/api/v1/patrons_holds.t index 98cffc375b..ffe6f56f0d 100755 --- a/t/db_dependent/api/v1/patrons_holds.t +++ b/t/db_dependent/api/v1/patrons_holds.t @@ -35,7 +35,7 @@ t::lib::Mocks::mock_preference( 'RESTBasicAuth', 1 ); subtest 'list() tests' => sub { - plan tests => 9; + plan tests => 18; $schema->storage->txn_begin; @@ -51,12 +51,33 @@ subtest 'list() tests' => sub { ->status_is( 200, 'SWAGGER3.2.2' ) ->json_is( [] ); - my $hold_1 = $builder->build_object({ class => 'Koha::Holds', value => { borrowernumber => $patron->id } }); - my $hold_2 = $builder->build_object({ class => 'Koha::Holds', value => { borrowernumber => $patron->id } }); + my $hold_1 = $builder->build_object( { class => 'Koha::Holds', value => { borrowernumber => $patron->id } } ); + my $hold_2 = $builder->build_object( { class => 'Koha::Holds', value => { borrowernumber => $patron->id } } ); + my $hold_3 = $builder->build_object( { class => 'Koha::Holds', value => { borrowernumber => $patron->id } } ); - $t->get_ok("//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?_order_by=+me.hold_id') - ->status_is( 200, 'SWAGGER3.2.2' ) - ->json_is( '' => [ $hold_1->to_api, $hold_2->to_api ], 'Holds retrieved' ); + $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?_order_by=+me.hold_id' ) + ->status_is( 200, 'SWAGGER3.2.2' ) + ->json_is( '' => [ $hold_1->to_api, $hold_2->to_api, $hold_3->to_api ], 'Holds retrieved' ); + + $hold_1->fill; + $hold_3->fill; + + $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?_order_by=+me.hold_id' ) + ->status_is( 200, 'SWAGGER3.2.2' )->json_is( '' => [ $hold_2->to_api ], 'Only current holds retrieved' ); + + $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?old=1&_order_by=+me.hold_id' ) + ->status_is( 200, 'SWAGGER3.2.2' ) + ->json_is( '' => [ $hold_1->to_api, $hold_3->to_api ], 'Only old holds retrieved' ); + + my $old_hold_1 = Koha::Old::Holds->find( $hold_1->id ); + $old_hold_1->item->delete; + $old_hold_1->pickup_library->delete; + + $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?old=1&_order_by=+me.hold_id' ) + ->status_is( 200, 'SWAGGER3.2.2' )->json_is( + '' => [ $old_hold_1->get_from_storage->to_api, $hold_3->to_api ], + 'Old holds even after item and library removed' + ); my $non_existent_patron = $builder->build_object({ class => 'Koha::Patrons' }); my $non_existent_patron_id = $non_existent_patron->id; -- 2.39.5