From ceb3ba1fd62ebd2437aff27edf0881118daa80ba Mon Sep 17 00:00:00 2001 From: Arthur Suzuki Date: Thu, 4 Sep 2025 08:49:38 +0000 Subject: [PATCH] Bug 32702: (QA follow-up) remove IsAvailableForItemLevelRequest and move checks to CanItemBeReserved --- C4/Circulation.pm | 10 +- C4/ILSDI/Services.pm | 10 +- C4/Reserves.pm | 51 -------- Koha/Item.pm | 5 +- opac/opac-ISBDdetail.pl | 9 +- opac/opac-MARCdetail.pl | 9 +- opac/opac-detail.pl | 6 +- opac/opac-reserve.pl | 6 +- reserve/request.pl | 13 +- t/db_dependent/Holds.t | 2 +- .../Holds/DisallowHoldIfItemsAvailable.t | 118 ++++++++++-------- t/db_dependent/Reserves.t | 68 +++------- 12 files changed, 102 insertions(+), 205 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 59e12c72cd0..271a93baaa2 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -25,11 +25,10 @@ use Encode; use Try::Tiny; use C4::Context; -use C4::Stats qw( UpdateStats ); -use C4::Reserves - qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority IsAvailableForItemLevelRequest ); -use C4::Biblio qw( UpdateTotalIssues ); -use C4::Items qw( ModItemTransfer ModDateLastSeen CartToShelf ); +use C4::Stats qw( UpdateStats ); +use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority ); +use C4::Biblio qw( UpdateTotalIssues ); +use C4::Items qw( ModItemTransfer ModDateLastSeen CartToShelf ); use C4::Accounts; use C4::ItemCirculationAlertPreference; use C4::Message; @@ -3267,7 +3266,6 @@ sub CanBookBeRenewed { foreach my $other_item (@other_items) { next if defined $matched_items{ $other_item->itemnumber }; next if $other_item->holds->filter_by_found->count; - next unless IsAvailableForItemLevelRequest( $other_item, $patron_with_reserve, undef ); next unless CanItemBeReserved( $patron_with_reserve, $other_item, undef, diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 4aa7a3d9324..41efaaa91ef 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -24,7 +24,7 @@ use C4::Members; use C4::Items qw( get_hostitemnumbers_of ); use C4::Circulation qw( CanBookBeRenewed barcodedecode CanBookBeIssued AddRenewal ); use C4::Accounts; -use C4::Reserves qw( CanBookBeReserved IsAvailableForItemLevelRequest CalculatePriority AddReserve CanItemBeReserved ); +use C4::Reserves qw( CanBookBeReserved CalculatePriority AddReserve CanItemBeReserved ); use C4::Context; use C4::Auth; use CGI qw ( -utf8 ); @@ -642,12 +642,10 @@ sub GetServices { my @availablefor; # Reserve level management - my $biblionumber = $item->biblionumber; - my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber ); - if ( $canbookbereserved->{status} eq 'OK' ) { + my $biblionumber = $item->biblionumber; + if ( CanBookBeReserved( $borrower, $biblionumber )->{status} eq 'OK' ) { push @availablefor, 'title level hold'; - my $canitembereserved = IsAvailableForItemLevelRequest( $item, $patron ); - if ($canitembereserved) { + if ( CanItemBeReserved( $patron, $item )->{status} eq 'OK' ) { push @availablefor, 'item level hold'; } } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 8ea531d04a7..982d59c05dd 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -123,7 +123,6 @@ BEGIN { AutoUnsuspendReserves - IsAvailableForItemLevelRequest ItemsAnyAvailableAndNotRestricted AlterPriority @@ -1366,56 +1365,6 @@ sub ModReserveMinusPriority { _FixPriority( { reserve_id => $reserve_id, rank => '0' } ); } -=head2 IsAvailableForItemLevelRequest - - my $is_available = IsAvailableForItemLevelRequest( $item_record, $borrower_record, $pickup_branchcode ); - -Checks whether a given item record is available for an -item-level hold request. An item is available if - -* it is not lost AND -* it is not damaged AND -* it is not withdrawn AND -* a waiting or in transit reserve is placed on -* does not have a not for loan value > 0 - -Need to check the issuingrules onshelfholds column, -if this is set items on the shelf can be placed on hold - -Note that IsAvailableForItemLevelRequest() does not -check if the staff operator is authorized to place -a request on the item - in particular, -this routine does not check IndependentBranches -and canreservefromotherbranches. - -Note also that this subroutine does not checks smart -rules limits for item by reservesallowed/holds_per_record -values, this complemented in calling code with calls and -checks with CanItemBeReserved or CanBookBeReserved. - -=cut - -sub IsAvailableForItemLevelRequest { - my $item = shift; - my $patron = shift; - my $pickup_branchcode = shift; - - if ($pickup_branchcode) { - my $destination = Koha::Libraries->find($pickup_branchcode); - return 0 unless $destination; - return 0 unless $destination->pickup_location; - return 0 unless $item->can_be_transferred( { to => $destination } ); - my $reserves_control_branch = Koha::Policy::Holds->holds_control_library( $item, $patron ); - my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->itype ); - my $home_library = Koha::Libraries->find( { branchcode => $item->homebranch } ); - return 0 - unless $branchitemrule->{hold_fulfillment_policy} ne 'holdgroup' - || $home_library->validate_hold_sibling( { branchcode => $pickup_branchcode } ); - } - - return IsOnShelfHoldsPolicySatisfied( { item => $item, patron => $patron } ); -} - =head2 ItemsAnyAvailableAndNotRestricted ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron }); diff --git a/Koha/Item.pm b/Koha/Item.pm index df6a4a52d9d..205d5742b50 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -411,7 +411,10 @@ Returns the itemtype for the item based on whether item level itemtypes are set sub effective_itemtype { my ($self) = @_; - + if ( C4::Context->preference('item-level_itypes') ) { + return undef + unless defined $self->itype; + } return $self->_result()->effective_itemtype(); } diff --git a/opac/opac-ISBDdetail.pl b/opac/opac-ISBDdetail.pl index 4bf0bc08cb0..7eb05963b42 100755 --- a/opac/opac-ISBDdetail.pl +++ b/opac/opac-ISBDdetail.pl @@ -51,7 +51,7 @@ use C4::Biblio qw( TransformMarcToKoha ); use C4::Record qw( marc2cites ); -use C4::Reserves qw( IsAvailableForItemLevelRequest ); +use C4::Reserves qw( CanBookBeReserved ); use C4::Serials qw( CountSubscriptionFromBiblionumber SearchSubscriptions GetLatestSerials ); use C4::Koha qw( GetNormalizedEAN @@ -214,12 +214,7 @@ my $holdable_items = $biblio->items->filter_by_for_hold->count; # If we don't have a patron, then holdable items determines holdability my $can_holds_be_placed = $patron ? 0 : $holdable_items; if ($patron) { - my $items = $biblio->items; - while ( my $item = $items->next ) { - $can_holds_be_placed = $can_holds_be_placed - || ( IsAvailableForItemLevelRequest( $item, $patron, undef ) - && CanItemBeReserved( $patron, $item, undef ) ); - } + $can_holds_be_placed = CanBookBeReserved( $patron->id, $biblio->id ); } $template->param( diff --git a/opac/opac-MARCdetail.pl b/opac/opac-MARCdetail.pl index 9c9d0f59c73..43ef286aeeb 100755 --- a/opac/opac-MARCdetail.pl +++ b/opac/opac-MARCdetail.pl @@ -57,7 +57,7 @@ use C4::Biblio qw( TransformMarcToKoha ); use C4::Record qw( marc2cites ); -use C4::Reserves qw( IsAvailableForItemLevelRequest ); +use C4::Reserves qw( CanBookBeReserved ); use C4::Members; use C4::Koha qw( GetNormalizedISBN ); use List::MoreUtils qw( uniq ); @@ -175,12 +175,7 @@ my $holdable_items = $biblio->items->filter_by_for_hold->count; my $can_holds_be_placed = $patron ? 0 : $holdable_items; if ($patron) { - $items->reset; - while ( my $item = $items->next ) { - $can_holds_be_placed = $can_holds_be_placed - || ( IsAvailableForItemLevelRequest( $item, $patron, undef ) - && CanItemBeReserved( $patron, $item, undef ) ); - } + $can_holds_be_placed = CanBookBeReserved( $patron->id, $biblio->id ); } $template->param( ReservableItems => $can_holds_be_placed ); diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index 851c9c101b2..05620202e12 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -59,7 +59,7 @@ use C4::External::Syndetics qw( use C4::Members; use C4::XSLT qw( XSLTParse4Display ); use C4::ShelfBrowser qw( GetNearbyItems ); -use C4::Reserves qw( GetReserveStatus IsAvailableForItemLevelRequest ); +use C4::Reserves qw( GetReserveStatus CanItemBeReserved ); use C4::Charset qw( SetUTF8Flag ); use MARC::Field; use List::MoreUtils qw( any ); @@ -747,9 +747,7 @@ if ( not $viewallitems and $items->count > $max_items_to_display ) { # We only need to check if we haven't determined holds can be placed # and if we don't have patron, we have already decided $can_holds_be_placed = $can_holds_be_placed - || ( $patron - && IsAvailableForItemLevelRequest( $item, $patron, undef ) - && CanItemBeReserved( $patron, $item, undef ) ); + || ( $patron && CanItemBeReserved( $patron, $item, undef ) ); # get collection code description, too my $ccode = $item->ccode; diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 6db213f2985..38161bb58c0 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -24,7 +24,7 @@ use CGI qw ( -utf8 ); use C4::Auth qw( get_template_and_user ); use C4::Koha qw( getitemtypeimagelocation getitemtypeimagesrc ); use C4::Circulation qw( GetBranchItemRule ); -use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve IsAvailableForItemLevelRequest GetReserveFee ); +use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve GetReserveFee ); use C4::Biblio qw( GetBiblioData GetFrameworkCode ); use C4::Output qw( output_html_with_http_headers ); use C4::Context; @@ -418,7 +418,6 @@ foreach my $biblioNum (@biblionumbers) { my $numCopiesOPACAvailable = 0; # iterating through all items first to check if any of them available - # to pass this value further inside down to IsAvailableForItemLevelRequest to # it's complicated logic to analyse. # (before this loop was inside that sub loop so it was O(n^2) ) foreach my $item ( @{ $biblioData->{items} } ) { @@ -461,8 +460,7 @@ foreach my $biblioNum (@biblionumbers) { my $branch = Koha::Policy::Holds->holds_control_library( $item, $patron ); - my $policy_holdallowed = CanItemBeReserved( $patron, $item, undef, { get_from_cache => 1 } )->{status} eq 'OK' - && IsAvailableForItemLevelRequest( $item, $patron, undef ); + my $policy_holdallowed = CanItemBeReserved( $patron, $item, undef, { get_from_cache => 1 } )->{status} eq 'OK'; if ($policy_holdallowed) { my $opac_hold_policy = diff --git a/reserve/request.pl b/reserve/request.pl index a553070e4b3..24806dee42d 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -33,7 +33,7 @@ use Date::Calc qw( Date_to_Days ); use C4::Output qw( output_html_with_http_headers ); use C4::Auth qw( get_template_and_user ); use C4::Reserves - qw( AlterPriority ToggleLowestPriority CanBookBeReserved GetMaxPatronHoldsForRecord CanItemBeReserved IsAvailableForItemLevelRequest ); + qw( AlterPriority ToggleLowestPriority CanBookBeReserved GetMaxPatronHoldsForRecord CanItemBeReserved ); use C4::Items qw( get_hostitemnumbers_of ); use C4::Koha qw( getitemtypeimagelocation ); use C4::Serials qw( CountSubscriptionFromBiblionumber ); @@ -395,7 +395,6 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) my $valid_items = 0; # iterating through all items first to check if any of them available - # to pass this value further inside down to IsAvailableForItemLevelRequest to # it's complicated logic to analyse. # (before this loop was inside that sub loop so it was O(n^2) ) @@ -507,15 +506,9 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) $default_pickup_branch = C4::Context->userenv->{branch}; } - if ( - !$item->{cantreserve} + if ( !$item->{cantreserve} && !$exceeded_maxreserves - && $can_item_be_reserved eq 'OK' - - # items_any_available defined outside of the current loop, - # so we avoiding loop inside IsAvailableForItemLevelRequest: - && IsAvailableForItemLevelRequest( $item_object, $patron, undef ) - ) + && $can_item_be_reserved eq 'OK' ) { $valid_items = 1; } diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 483f0f443c7..9ffe9064e8d 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -743,7 +743,7 @@ subtest 'CanItemBeReserved' => sub { is( CanItemBeReserved( $patrons[0], $item )->{status}, 'notforloan', - "Item with no notforloan itemtype cannot be reserved" + "Item with notforloan status cannot be reserved" ); $itemtype->notforloan(0)->store(); diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index 5617e38b93c..3f032d24f0c 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -16,7 +16,7 @@ use t::lib::TestBuilder; use t::lib::Mocks; BEGIN { - use_ok( 'C4::Reserves', qw( ItemsAnyAvailableAndNotRestricted IsAvailableForItemLevelRequest ) ); + use_ok( 'C4::Reserves', qw( ItemsAnyAvailableAndNotRestricted CanItemBeReserved ) ); } my $schema = Koha::Database->schema; @@ -35,13 +35,16 @@ my $itemtype = $builder->build( } )->{itemtype}; -t::lib::Mocks::mock_userenv( { branchcode => $library1->{branchcode} } ); +my $library_A = $library1->{branchcode}; +my $library_B = $library2->{branchcode}; + +t::lib::Mocks::mock_userenv( { branchcode => $library_A } ); my $patron1 = $builder->build_object( { class => 'Koha::Patrons', value => { - branchcode => $library1->{branchcode}, + branchcode => $library_A, dateexpiry => '3000-01-01', } } @@ -52,7 +55,7 @@ my $patron2 = $builder->build_object( { class => 'Koha::Patrons', value => { - branchcode => $library1->{branchcode}, + branchcode => $library_A, dateexpiry => '3000-01-01', } } @@ -62,15 +65,12 @@ my $patron3 = $builder->build_object( { class => 'Koha::Patrons', value => { - branchcode => $library2->{branchcode}, + branchcode => $library_B, dateexpiry => '3000-01-01', } } ); -my $library_A = $library1->{branchcode}; -my $library_B = $library2->{branchcode}; - my $biblio = $builder->build_sample_biblio( { itemtype => $itemtype } ); my $biblionumber = $biblio->biblionumber; my $item1 = $builder->build_sample_item( @@ -111,16 +111,16 @@ my $is; $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 } ); is( $is, 1, "Items availability: both of 2 items are available" ); -$is = IsAvailableForItemLevelRequest( $item1, $patron1 ); -is( $is, 0, "Item cannot be held, 2 items available" ); +$is = CanItemBeReserved( $patron1, $item1 ); +is( $is->{status}, "onShelfHoldsNotAllowed", "Item cannot be held, 2 items available" ); my $issue1 = AddIssue( $patron2, $item1->barcode ); $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 } ); is( $is, 1, "Items availability: one item is available" ); -$is = IsAvailableForItemLevelRequest( $item1, $patron1 ); -is( $is, 0, "Item cannot be held, 1 item available" ); +$is = CanItemBeReserved( $patron1, $item1 ); +is( $is->{status}, "onShelfHoldsNotAllowed", "Item cannot be held, 1 item available" ); AddIssue( $patron2, $item2->barcode ); $cache->flush(); @@ -128,13 +128,13 @@ $cache->flush(); $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 } ); is( $is, 0, "Items availability: none of items are available" ); -$is = IsAvailableForItemLevelRequest( $item1, $patron1 ); -is( $is, 1, "Item can be held, no items available" ); +$is = CanItemBeReserved( $patron1, $item1 ); +is( $is->{status}, "OK", "Item can be held, no items available" ); AddReturn( $item1->barcode ); { # Remove the issue for the first patron, and modify the branch for item1 - subtest 'IsAvailableForItemLevelRequest behaviours depending on ReservesControlBranch + holdallowed' => sub { + subtest 'CanItemBeReserved behaviours depending on ReservesControlBranch + holdallowed' => sub { plan tests => 2; my $hold_allowed_from_home_library = 'from_home_library'; @@ -163,15 +163,15 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from home + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 1, + $is->{status}, "cannotReserveFromOtherBranches", "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, " - . "One item is available at different library, not holdable = none available => the hold is allowed at item level" + . "One item is available at different library, not holdable = none available => the hold is not allowed at item level" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron2 ); + $is = CanItemBeReserved( $patron2, $item1 ); is( - $is, 1, + $is->{status}, "cannotReserveFromOtherBranches", "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, " . "One item is available at home library, holdable = one available => the hold is not allowed at item level" ); @@ -187,9 +187,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from any library for library B + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, " . "One item is available at different library, holdable = one available => the hold is not allowed at item level" ); @@ -204,11 +204,11 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from any library for library B + ReservesControlBranch=PatronLibrary + one item is available at different library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 1, + $is->{status}, "cannotReserveFromOtherBranches", "Hold allowed from home library + ReservesControlBranch=PatronLibrary, " - . "One item is available at different library, not holdable = none available => the hold is allowed at item level" + . "One item is available at different library, not holdable = none available => the hold is not allowed at item level" ); #Adding a rule for the patron's home library affects the availability for an item from another library because ReservesControlBranch is set to PatronLibrary @@ -222,9 +222,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from any library for library A + ReservesControlBranch=PatronLibrary + one item is available at different library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from home library + ReservesControlBranch=PatronLibrary, " . "One item is available at different library, holdable = one available => the hold is not allowed at item level" ); @@ -243,9 +243,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from any library + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, " . "One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" ); @@ -260,9 +260,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from any library + ReservesControlBranch=PatronLibrary + one item is available at different library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from any library + ReservesControlBranch=PatronLibrary, " . "One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" ); @@ -294,9 +294,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from home library + ReservesControlBranch=ItemHomeLibrary + one item is available at home library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, " . "One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); @@ -310,9 +310,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from home library + ReservesControlBranch=PatronLibrary + one item is available at home library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from home library + ReservesControlBranch=PatronLibrary, " . "One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); @@ -330,9 +330,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from any library + ReservesControlBranch=ItemHomeLibrary + one item is available at home library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, " . "One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); @@ -346,9 +346,9 @@ AddReturn( $item1->barcode ); "Items availability: hold allowed from any library + ReservesControlBranch=PatronLibrary + one item is available at home library" ); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); + $is = CanItemBeReserved( $patron1, $item1 ); is( - $is, 0, + $is->{status}, "onShelfHoldsNotAllowed", "Hold allowed from any library + ReservesControlBranch=PatronLibrary, " . "One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); @@ -381,8 +381,9 @@ Koha::CirculationRules->set_rules( itemtype => $itemtype2, branchcode => undef, rules => { - maxissueqty => 99, - onshelfholds => 0, + reservesallowed => 99, + maxissueqty => 99, + onshelfholds => 0, } } ); @@ -391,8 +392,8 @@ $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron ; # patron1 in library A, library A 1 item is( $is, 1, "Items availability: 1 item is available, 1 item held in T" ); -$is = IsAvailableForItemLevelRequest( $item3, $patron1 ); -is( $is, 1, "Item can be held, items in transit are not available" ); +$is = CanItemBeReserved( $patron1, $item3 ); +is( $is->{status}, "OK", "Item can be held, items in transit are not available" ); subtest 'Check holds availability with different item types' => sub { plan tests => 6; @@ -459,11 +460,17 @@ subtest 'Check holds availability with different item types' => sub { "Items availability: 2 items, one allowed by smart rule but not checked out, another one not allowed to be held by smart rule" ); - $is = IsAvailableForItemLevelRequest( $item4, $patron1 ); - is( $is, 0, "Item4 cannot be requested to hold: 2 items, Item4 available, Item5 restricted" ); + $is = CanItemBeReserved( $patron1, $item4 ); + is( + $is->{status}, "onShelfHoldsNotAllowed", + "Item4 cannot be requested to hold: 2 items, Item4 available, Item5 restricted" + ); - $is = IsAvailableForItemLevelRequest( $item5, $patron1 ); - is( $is, 0, "Item5 cannot be requested to hold: 2 items, Item4 available, Item5 restricted" ); + $is = CanItemBeReserved( $patron1, $item5 ); + is( + $is->{status}, "noReservesAllowed", + "Item5 cannot be requested to hold: 2 items, Item4 available, Item5 restricted" + ); AddIssue( $patron2, $item4->barcode ); $cache->flush(); @@ -474,13 +481,14 @@ subtest 'Check holds availability with different item types' => sub { "Items availability: 2 items, one allowed by smart rule and checked out, another one not allowed to be held by smart rule" ); - $is = IsAvailableForItemLevelRequest( $item4, $patron1 ); - is( $is, 1, "Item4 can be requested to hold, 2 items, Item4 checked out, Item5 restricted" ); + $is = CanItemBeReserved( $patron1, $item4 ); + is( $is->{status}, "OK", "Item4 can be requested to hold, 2 items, Item4 checked out, Item5 restricted" ); - $is = IsAvailableForItemLevelRequest( $item5, $patron1 ); - - # Note: read IsAvailableForItemLevelRequest sub description about CanItemBeReserved/CanBookBeReserved: - is( $is, 1, "Item5 can be requested to hold, 2 items, Item4 checked out, Item5 restricted" ); + $is = CanItemBeReserved( $patron1, $item5 ); + is( + $is->{status}, "noReservesAllowed", + "Item5 can be requested to hold, 2 items, Item4 checked out, Item5 restricted" + ); }; subtest 'Check item checkout availability with ordered item' => sub { @@ -549,8 +557,8 @@ subtest 'Check item availability for hold with ordered item' => sub { ); $cache->flush(); - $is = IsAvailableForItemLevelRequest( $item1, $patron1 ); - is( $is, 1, "Ordered items are available for hold" ); + $is = CanItemBeReserved( $patron1, $item1 ); + is( $is->{status}, "OK", "Ordered items are available for hold" ); }; # Cleanup diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 52409d9246d..403f03a3739 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 71; +use Test::More tests => 70; use Test::NoWarnings; use Test::MockModule; use Test::Warn; @@ -34,7 +34,7 @@ use C4::Biblio qw( GetMarcFromKohaField ModBiblio ); use C4::HoldsQueue; use C4::Members; use C4::Reserves - qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanBookBeReserved IsAvailableForItemLevelRequest MoveReserve ChargeReserveFee CanItemBeReserved MergeHolds ); + qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanBookBeReserved MoveReserve ChargeReserveFee CanItemBeReserved MergeHolds ); use Koha::ActionLogs; use Koha::Biblios; use Koha::Caches; @@ -659,9 +659,18 @@ is( ####### EO Bug 13113 <<< #### -ok( C4::Reserves::IsAvailableForItemLevelRequest( $item, $patron ), "Reserving a book on item level" ); - -my $pickup_branch = $builder->build( { source => 'Branch' } )->{branchcode}; +is( C4::Reserves::CanItemBeReserved( $patron, $item )->{status}, "OK", "Reserving a book on item level" ); +my $branch = $builder->build_object( + { + class => 'Koha::Libraries', + value => { + branchemail => 'branch@e.mail', + branchreplyto => 'branch@reply.to', + pickup_location => 1 + } + } +); +my $pickup_branch = $branch->branchcode; t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '1' ); t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', 'itemtype' ); my $limit = Koha::Item::Transfer::Limit->new( @@ -672,7 +681,7 @@ my $limit = Koha::Item::Transfer::Limit->new( } )->store(); is( - C4::Reserves::IsAvailableForItemLevelRequest( $item, $patron, $pickup_branch ), 0, + C4::Reserves::CanItemBeReserved( $patron, $item, $pickup_branch )->{status}, "cannotBeTransferred", "Item level request not available due to transfer limit" ); t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '0' ); @@ -1534,53 +1543,6 @@ sub place_item_hold { # we reached the finish $schema->storage->txn_rollback(); -subtest 'IsAvailableForItemLevelRequest() tests' => sub { - - plan tests => 2; - - $schema->storage->txn_begin; - - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - - my $item_type = undef; - - my $item_mock = Test::MockModule->new('Koha::Item'); - $item_mock->mock( 'effective_itemtype', sub { return $item_type; } ); - - my $item = $builder->build_sample_item; - - # Weird use case to highlight issue - $item_type = '0'; - Koha::ItemTypes->search( { itemtype => $item_type } )->delete; - my $itemtype = $builder->build_object( - { - class => 'Koha::ItemTypes', - value => { itemtype => $item_type } - } - ); - ok( - C4::Reserves::IsAvailableForItemLevelRequest( $item, $patron ), - "Item not available for item-level hold because no effective item type" - ); - - Koha::CirculationRules->set_rules( - { - categorycode => '*', - itemtype => '*', - branchcode => '*', - rules => { - onshelfholds => 0, - } - } - ); - my $item_1 = $builder->build_sample_item( { notforloan => -1 } ); - ok( - C4::Reserves::IsAvailableForItemLevelRequest( $item_1, $patron ), - "We can placing hold on item with negative not for loan values when 'On shelf holds allowed' is set to 'If any unavailable'" - ); - $schema->storage->txn_rollback; -}; - subtest 'AddReserve() tests' => sub { plan tests => 2; -- 2.39.5