From b80dafc210fa4798d81adbb751ae2b8eeddecfa4 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 20 Apr 2026 17:50:43 +0100 Subject: [PATCH] Bug 23909: Apply hold policy to OPAC REST self-checkout The OPAC REST self-checkout path (guarded by OpacTrustedCheckout) is functionally SCO-over-API, so it should honour the same hold policy as the SCO web interface. Previously _check_availability unconditionally promoted RESERVED, RESERVE_WAITING, TRANSFERRED and PROCESSING from the confirmation hash to the blockers hash for any public caller. This meant the OPAC REST endpoint always refused held items regardless of the librarian's AllowItemsOnHoldCheckoutSCO configuration. With this patch the promotion is gated on AllowItemsOnHoldCheckoutSCO: * 0 - Promote all four hold confirmations to blockers (previous behaviour for pending holds) * 1 - Promote only waiting/transit/processing; silently allow checkout of items with a pending hold * 2 - Promote none; silently allow checkout of items with any hold Remaining hold confirmations are stripped from the response so the caller does not have to resolve a confirmation token for a policy the admin has already granted. Test plan: 1. prove t/db_dependent/api/v1/checkouts.t 2. Enable OpacTrustedCheckout. POST an item with a pending hold for another patron to /api/v1/public/patrons/X/checkouts: - AllowItemsOnHoldCheckoutSCO=0: 403 Checkout not authorized - AllowItemsOnHoldCheckoutSCO=1: 201 Created 3. Place a waiting hold for another patron on an item. Repeat: - AllowItemsOnHoldCheckoutSCO=1: 403 - AllowItemsOnHoldCheckoutSCO=2: 201 Signed-off-by: Kristi Krueger Signed-off-by: Andrew Fuerste Henry Signed-off-by: Lucas Gass --- Koha/REST/V1/Checkouts.pm | 20 +++++++++-- t/db_dependent/api/v1/checkouts.t | 55 ++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/Koha/REST/V1/Checkouts.pm b/Koha/REST/V1/Checkouts.pm index ab99808d49..522ff799a8 100644 --- a/Koha/REST/V1/Checkouts.pm +++ b/Koha/REST/V1/Checkouts.pm @@ -120,8 +120,16 @@ sub _check_availability { if ( $c->stash('is_public') ) { # Upgrade some confirmations to blockers - my @should_block = - qw/TOO_MANY ISSUED_TO_ANOTHER RESERVED RESERVE_WAITING TRANSFERRED PROCESSING AGE_RESTRICTION/; + my @should_block = qw/TOO_MANY ISSUED_TO_ANOTHER AGE_RESTRICTION/; + + # Hold-related confirmations are gated by AllowItemsOnHoldCheckoutSCO: + # 0 = block pending and waiting holds (default) + # 1 = allow pending holds, block waiting/transit/processing holds + # 2 = allow pending and waiting/transit/processing holds + my $hold_pref = C4::Context->preference('AllowItemsOnHoldCheckoutSCO') || 0; + push @should_block, 'RESERVED' if $hold_pref < 1; + push @should_block, qw/RESERVE_WAITING TRANSFERRED PROCESSING/ if $hold_pref < 2; + for my $block (@should_block) { if ( exists( $confirmation->{$block} ) ) { $impossible->{$block} = $confirmation->{$block}; @@ -129,9 +137,15 @@ sub _check_availability { } } + # Drop the remaining hold confirmations so the caller does not have + # to resolve a confirmation token for a policy we've already permitted + for my $allowed (qw/RESERVED RESERVE_WAITING TRANSFERRED PROCESSING/) { + delete $confirmation->{$allowed}; + } + # Remove any non-public info that's returned by CanBookBeIssued my @restricted_keys = - qw/issued_borrowernumber issued_cardnumber issued_firstname issued_surname resborrowernumber resbranchcode rescardnumber reserve_id resfirstname resreservedate ressurname item_notforloan/; + qw/issued_borrowernumber issued_cardnumber issued_firstname issued_surname resborrowernumber resbranchcode rescardnumber reserve_id resfirstname resreservedate reswaitingdate ressurname item_notforloan/; for my $key (@restricted_keys) { delete $confirmation->{$key}; delete $impossible->{$key}; diff --git a/t/db_dependent/api/v1/checkouts.t b/t/db_dependent/api/v1/checkouts.t index adf7068ba1..a672f9e9f8 100755 --- a/t/db_dependent/api/v1/checkouts.t +++ b/t/db_dependent/api/v1/checkouts.t @@ -380,7 +380,7 @@ subtest 'get_availability' => sub { %needsconfirmation = (); subtest 'public availability' => sub { - plan tests => 22; + plan tests => 40; # Authentication required $t->get_ok("/api/v1/public/checkouts/availability?item_id=$item1_id&patron_id=$patron_id")->status_is(401); @@ -411,6 +411,59 @@ subtest 'get_availability' => sub { ->json_has('/confirmation_token'); %needsconfirmation = (); + # Hold confirmations are gated by AllowItemsOnHoldCheckoutSCO. + # Because the mocked CanBookBeIssued returns the hashes by reference + # and _check_availability mutates them, we reset both hashes before + # every request. + my $reset_hold_hashes = sub { + %issuingimpossible = (); + %needsconfirmation = ( + RESERVED => 1, + RESERVE_WAITING => 1, + TRANSFERRED => 1, + PROCESSING => 1 + ); + }; + + $reset_hold_hashes->(); + t::lib::Mocks::mock_preference( 'AllowItemsOnHoldCheckoutSCO', 0 ); + $t->get_ok( + "//$unauth_userid:$unauth_password@/api/v1/public/checkouts/availability?item_id=$item1_id&patron_id=$patron_id" + )->status_is(200)->json_is( + '/blockers' => { + RESERVED => 1, + RESERVE_WAITING => 1, + TRANSFERRED => 1, + PROCESSING => 1 + } + )->json_is( '/confirms' => {} )->json_is( '/warnings' => {} )->json_has('/confirmation_token'); + + $reset_hold_hashes->(); + t::lib::Mocks::mock_preference( 'AllowItemsOnHoldCheckoutSCO', 1 ); + $t->get_ok( + "//$unauth_userid:$unauth_password@/api/v1/public/checkouts/availability?item_id=$item1_id&patron_id=$patron_id" + )->status_is(200)->json_is( + '/blockers' => { + RESERVE_WAITING => 1, + TRANSFERRED => 1, + PROCESSING => 1 + } + )->json_is( '/confirms' => {} )->json_is( '/warnings' => {} )->json_has('/confirmation_token'); + + $reset_hold_hashes->(); + t::lib::Mocks::mock_preference( 'AllowItemsOnHoldCheckoutSCO', 2 ); + $t->get_ok( + "//$unauth_userid:$unauth_password@/api/v1/public/checkouts/availability?item_id=$item1_id&patron_id=$patron_id" + ) + ->status_is(200) + ->json_is( '/blockers' => {} ) + ->json_is( '/confirms' => {} ) + ->json_is( '/warnings' => {} ) + ->json_has('/confirmation_token'); + + %issuingimpossible = (); + %needsconfirmation = (); + # Remove personal information from public endpoint %issuingimpossible = ( issued_borrowernumber => 'private',