@@ -, +, @@ once per patron Set circ rules 'On shelf holds allowed' to 'If any unavailable' --- C4/Circulation.pm | 51 ++++++--- t/db_dependent/Circulation.t | 211 +++++++++++++++++++++++++++++------ 2 files changed, 211 insertions(+), 51 deletions(-) --- a/C4/Circulation.pm +++ a/C4/Circulation.pm @@ -27,7 +27,7 @@ use Encode; use Koha::DateUtils qw( dt_from_string output_pref ); use C4::Context; use C4::Stats qw( UpdateStats ); -use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ); +use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ItemsAnyAvailableAndNotRestricted ); use C4::Biblio qw( UpdateTotalIssues ); use C4::Items qw( ModItemTransfer ModDateLastSeen CartToShelf ); use C4::Accounts; @@ -2835,37 +2835,56 @@ sub CanBookBeRenewed { else { # Get all other items that could possibly fill reserves + # FIXME We could join reserves (or more tables) here to eliminate some checks later my $items = Koha::Items->search({ biblionumber => $resrec->{biblionumber}, onloan => undef, notforloan => 0, -not => { itemnumber => $itemnumber } }); + my $item_count = $items->count(); # Get all other reserves that could have been filled by this item my @borrowernumbers = map { $_->{borrowernumber} } @$possible_reserves; + # Note: fetching the patrons in this manner means that a patron with 2 holds will + # not block renewal if one reserve can be satisfied i.e. each patron is checked once my $patrons = Koha::Patrons->search({ borrowernumber => { -in => \@borrowernumbers } }); - - # If the count of the union of the lists of reservable items for each borrower - # is equal or greater than the number of borrowers, we know that all reserves - # can be filled with available items. We can get the union of the sets simply - # by pushing all the elements onto an array and removing the duplicates. - my @reservable; - ITEM: while ( my $item = $items->next ) { - next if IsItemOnHoldAndFound( $item->itemnumber ); - while ( my $patron = $patrons->next ) { - next unless IsAvailableForItemLevelRequest($item, $patron); - next unless CanItemBeReserved($patron->borrowernumber,$item->itemnumber,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; - push @reservable, $item->itemnumber; - if (@reservable >= @borrowernumbers) { + my $patron_count = $patrons->count(); + + return ( 0, "on_reserve" ) if ($patron_count > $item_count); + # We cannot possibly fill all reserves if we don't have enough items + + # If we can fill each hold that has been found with the available items on the record + # then the patron can renew. If we cannot, they cannot renew. + # FIXME This code does not check whether the item we are renewing can fill + # any of the existing reserves. + my $reservable = 0; + my %matched_items; + my $seen = 0; + PATRON: while ( my $patron = $patrons->next ) { + # If there is a reserve that cannot be filled we are done + return ( 0, "on_reserve" ) if ( $seen > $reservable ); + my $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron }); + while ( my $other_item = $items->next ) { + next if defined $matched_items{$other_item->itemnumber}; + next if IsItemOnHoldAndFound( $other_item->itemnumber ); + next unless IsAvailableForItemLevelRequest($other_item, $patron, undef, $items_any_available); + next unless CanItemBeReserved($patron->borrowernumber,$other_item->itemnumber,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; + # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' + # CanItemBeReserved checks 'rules' and 'policies' which means + # items will fill holds at checkin that are rejected here + $reservable++; + if ($reservable >= $patron_count) { $resfound = 0; - last ITEM; + last PATRON; } + $matched_items{$other_item->itemnumber} = 1; last; } - $patrons->reset; + $items->reset; + $seen++; } } } --- a/t/db_dependent/Circulation.t +++ a/t/db_dependent/Circulation.t @@ -1556,16 +1556,30 @@ subtest "Bug 13841 - Do not create new 0 amount fines" => sub { }; subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { - $dbh->do('DELETE FROM issues'); - $dbh->do('DELETE FROM items'); - $dbh->do('DELETE FROM circulation_rules'); + plan tests => 13; + my $biblio = $builder->build_sample_biblio(); + my $item_1 = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + } + ); + my $item_2= $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + itype => $item_1->effective_itemtype, + } + ); + Koha::CirculationRules->set_rules( { categorycode => undef, - itemtype => undef, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { reservesallowed => 25, + holds_per_record => 25, issuelength => 14, lengthunit => 'days', renewalsallowed => 1, @@ -1578,23 +1592,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { } } ); - my $biblio = $builder->build_sample_biblio(); - my $item_1 = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library2->{branchcode}, - itype => $itemtype, - } - ); - - my $item_2= $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library2->{branchcode}, - itype => $itemtype, - } - ); my $borrowernumber1 = Koha::Patron->new({ firstname => 'Kyle', @@ -1608,6 +1606,22 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { categorycode => $patron_category->{categorycode}, branchcode => $library2->{branchcode}, })->store->borrowernumber; + my $patron_category_2 = $builder->build( + { + source => 'Category', + value => { + category_type => 'P', + enrolmentfee => 0, + BlockExpiredPatronOpacActions => -1, # Pick the pref value + } + } + ); + my $borrowernumber3 = Koha::Patron->new({ + firstname => 'Carnegie', + surname => 'Hall', + categorycode => $patron_category_2->{categorycode}, + branchcode => $library2->{branchcode}, + })->store->borrowernumber; my $borrower1 = Koha::Patrons->find( $borrowernumber1 )->unblessed; my $borrower2 = Koha::Patrons->find( $borrowernumber2 )->unblessed; @@ -1629,7 +1643,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { Koha::CirculationRules->set_rules( { categorycode => undef, - itemtype => undef, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { onshelfholds => 0, @@ -1640,53 +1654,121 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); is( $renewokay, 0, 'Bug 14337 - Verify the borrower cannot renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfholds are disabled' ); + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + is( $renewokay, 0, 'Bug 14337 - Verify the borrower cannot renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled and onshelfholds is disabled' ); + Koha::CirculationRules->set_rules( { categorycode => undef, - itemtype => undef, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { - onshelfholds => 0, + onshelfholds => 1, } } ); + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 0 ); + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + is( $renewokay, 0, 'Bug 14337 - Verify the borrower cannot renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is disabled and onshelfhold is enabled' ); + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); - is( $renewokay, 0, 'Bug 14337 - Verify the borrower cannot renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled and onshelfholds is disabled' ); + is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled' ); + + AddReserve( + { + branchcode => $library2->{branchcode}, + borrowernumber => $borrowernumber3, + biblionumber => $biblio->biblionumber, + priority => 1, + } + ); + + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + is( $renewokay, 0, 'Verify the borrower cannot renew with 2 holds on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and one other item on record' ); + + my $item_3= $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + itype => $item_1->effective_itemtype, + } + ); + + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + is( $renewokay, 1, 'Verify the borrower cannot renew with 2 holds on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and two other items on record' ); Koha::CirculationRules->set_rules( { - categorycode => undef, - itemtype => undef, + categorycode => $patron_category_2->{categorycode}, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { - onshelfholds => 1, + reservesallowed => 0, } } ); - t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 0 ); + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); - is( $renewokay, 0, 'Bug 14337 - Verify the borrower cannot renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is disabled and onshelfhold is enabled' ); + is( $renewokay, 0, 'Verify the borrower cannot renew with 2 holds on the record, but only one of those holds can be filled when AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and two other items on record' ); Koha::CirculationRules->set_rules( { - categorycode => undef, - itemtype => undef, + categorycode => $patron_category_2->{categorycode}, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { - onshelfholds => 1, + reservesallowed => 25, } } ); - t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); - ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); - is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled' ); # Setting item not checked out to be not for loan but holdable $item_2->notforloan(-1)->store; ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); is( $renewokay, 0, 'Bug 14337 - Verify the borrower can not renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled but the only available item is notforloan' ); + + my $mock_circ = Test::MockModule->new("C4::Circulation"); + $mock_circ->mock( CanItemBeReserved => sub { + warn "Checked"; + return { status => 'no' } + } ); + + $item_2->notforloan(0)->store; + $item_3->delete(); + # Two items total, one item available, one issued, two holds on record + + warnings_are{ + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + } [], "CanItemBeReserved not called when there are more possible holds than available items"; + is( $renewokay, 0, 'Borrower cannot renew when there are more holds than available items' ); + + $item_3 = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + itype => $item_1->effective_itemtype, + } + ); + + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => $item_1->effective_itemtype, + branchcode => undef, + rules => { + reservesallowed => 0, + } + } + ); + + warnings_are{ + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + } ["Checked","Checked"], "CanItemBeReserved only called once per available item if it returns a negative result for all items for a borrower"; + is( $renewokay, 0, 'Borrower cannot renew when there are more holds than available items' ); + }; { @@ -1771,6 +1853,25 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub { holdingbranch => $holdingbranch->{branchcode}, } ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => $item->effective_itemtype, + branchcode => undef, + rules => { + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + maxissueqty => 20 + } + } + ); set_userenv($holdingbranch); @@ -1924,6 +2025,26 @@ subtest 'CanBookBeIssued + Koha::Patron->is_debarred|has_overdues' => sub { library => $library->{branchcode}, } ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => $library->{branchcode}, + rules => { + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + maxissueqty => 20 + } + } + ); + my ( $error, $question, $alerts ); @@ -2121,6 +2242,26 @@ subtest 'CanBookBeIssued + AllowMultipleIssuesOnABiblio' => sub { } ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => $library->{branchcode}, + rules => { + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + maxissueqty => 20 + } + } + ); + my ( $error, $question, $alerts ); my $issue = AddIssue( $patron->unblessed, $item_1->barcode, dt_from_string->add( days => 1 ) ); --