From 90b477f8008c96d519d7f865689f3eb3f67d1b49 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Fri, 15 Mar 2024 13:45:58 +0000 Subject: [PATCH] Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed Before this patch we get all holds on a record and see if we can fill them with available items. This means we check to fill holds that the item in questoion may not be able to fill, especially in the case where no holds are allowed on the item type, this is wrong To test: 1 - Find or create a biblio with two items of different item types 2 - Make sure one item type allows holds, and the other has: "Default holds policy by item type" Set to "No holds allowed" 3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 4 - Check out the unholdable item to a patron 5 - Set a hold for a different patron on the next available item 6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page) 7 - Checkout the other item to a third patron 8 - Confirm the first item can still be renewed 9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow" 10 - Confirm the item cannot be renewed now 11 - Apply patch, restart all 12 - Confirm the item can be renewed 13 - Set the item type to a type that allows holds 14 - Confirm the item can no longer be renewed 15 - Restore the item type 16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 17 - Confirm the item can be renewed 18 - Check in the item from the third patron 19 - Confirm the item can still be renewed 20 - prove -v t/db_dependent/Circulation.t - test still pass --- C4/Circulation.pm | 23 ++++++----------------- t/db_dependent/Circulation.t | 7 ++----- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 13d1758f493..b56fac2fbdb 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3069,18 +3069,10 @@ sub CanBookBeRenewed { return ( 0, "on_reserve" ) if ( $item->current_holds->search( { non_priority => 0 } )->count ); - my $fillable_holds = Koha::Holds->search( - { - biblionumber => $item->biblionumber, - non_priority => 0, - found => undef, - reservedate => { '<=' => \'NOW()' }, - suspend => 0 - } - ); - if ( $fillable_holds->count ) { + + my ($status, $matched_reserve, $fillable_holds) = CheckReserves($item); + if ( $fillable_holds ) { if ( C4::Context->preference('AllowRenewalIfOtherItemsAvailable') ) { - my @possible_holds = $fillable_holds->as_list; # Get all other items that could possibly fill reserves # FIXME We could join reserves (or more tables) here to eliminate some checks later @@ -3090,14 +3082,12 @@ sub CanBookBeRenewed { notforloan => 0, -not => { itemnumber => $item->itemnumber } })->as_list; - return ( 0, "on_reserve" ) if @possible_holds && (scalar @other_items < scalar @possible_holds); + return ( 0, "on_reserve" ) if @{$fillable_holds} && (scalar @other_items < scalar @{$fillable_holds} ); my %matched_items; - foreach my $possible_hold (@possible_holds) { + foreach my $possible_hold ( @{$fillable_holds} ) { my $fillable = 0; - my $patron_with_reserve = Koha::Patrons->find($possible_hold->borrowernumber); - - # FIXME: We are not checking whether the item we are renewing can fill the hold + my $patron_with_reserve = Koha::Patrons->find($possible_hold->{borrowernumber}); foreach my $other_item (@other_items) { next if defined $matched_items{$other_item->itemnumber}; @@ -3115,7 +3105,6 @@ sub CanBookBeRenewed { } } else { - my ($status, $matched_reserve, $possible_reserves) = CheckReserves($item); return ( 0, "on_reserve" ) if $status; } } diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 8dd246578f2..e13985f8ffd 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -4421,12 +4421,9 @@ subtest 'ItemsDeniedRenewal rules are checked' => sub { } ); - my $allow_book = $builder->build_object({ class => 'Koha::Items', value => { + my $allow_book = $builder->build_sample_item({ homebranch => $idr_lib->branchcode, - withdrawn => 0, - itype => 'NOHIDE', - location => 'NOPROC' - } + holdingbranch => $idr_lib->branchcode, }); my $idr_borrower = $builder->build_object({ class => 'Koha::Patrons', value=> { -- 2.30.2