From a74d4ab26215f0c5b6837774b63c0406f1ec20e5 Mon Sep 17 00:00:00 2001 From: Petro Vashchuk Date: Fri, 14 Jul 2023 18:04:34 +0300 Subject: [PATCH] Bug 34281: Add logic to CanBookBeRenewed function to check if item can be transferred To reproduce: 1. Find or create a biblio that has two items of the same itemtype, one on branch A and another one on branch B 2. Checkout the item on branch A 3. Make a hold on that same branch 4. Checkout the item on branch B 5. Go to the sysprefs and enable UseBranchTransferLimits, after which go to the checkin and transfer policy page and disallow the transfer 6. Try to renew the item on branch B, the koha won't let you renew it 7. Apply the patch 8. Repeat step 6 but this time you should be able to renew an item Also run the tests prior to applying the patch and then after. --- C4/Circulation.pm | 13 ++++- t/db_dependent/Circulation.t | 96 +++++++++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 0f1b6914aa..81f16826db 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3047,7 +3047,9 @@ sub CanBookBeRenewed { notforloan => 0, -not => { itemnumber => $item->itemnumber } })->as_list; - return ( 0, "on_reserve" ) if @possible_holds && (scalar @other_items < scalar @possible_holds); + unless ( C4::Context->preference("UseBranchTransferLimits") ) { + return ( 0, "on_reserve" ) if @possible_holds && (scalar @other_items < scalar @possible_holds); + } my %matched_items; foreach my $possible_hold (@possible_holds) { @@ -3059,6 +3061,15 @@ sub CanBookBeRenewed { }); # FIXME: We are not checking whether the item we are renewing can fill the hold + if ( C4::Context->preference("UseBranchTransferLimits") ) { + if ( C4::Context->preference("item-level_itypes") && C4::Context->preference("BranchTransferLimitsType") eq 'itemtype' ) { + if ( ! IsBranchTransferAllowed( $possible_hold->branchcode, $item->homebranch, $item->itype ) ) { + next; + } + } elsif ( ! IsBranchTransferAllowed( $possible_hold->branchcode, $item->homebranch, $item->ccode ) ) { + next; + } + } foreach my $other_item (@other_items) { next if defined $matched_items{$other_item->itemnumber}; diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 1b625f90e4..032a85acc0 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,7 +18,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 67; +use Test::More tests => 68; use Test::Exception; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -290,6 +290,9 @@ Koha::CirculationRules->set_rules( subtest "CanBookBeRenewed AllowRenewalIfOtherItemsAvailable multiple borrowers and items tests" => sub { plan tests => 7; + my $schema = Koha::Database->schema; + $schema->storage->txn_begin; + #Can only reserve from home branch Koha::CirculationRules->set_rule( { @@ -377,6 +380,97 @@ subtest "CanBookBeRenewed AllowRenewalIfOtherItemsAvailable multiple borrowers a ( $renewokay, $error ) = CanBookBeRenewed($patron, $issue); is( $renewokay, 0, 'Cannot renew when there is an item specific hold'); is( $error, 'on_reserve', 'Cannot renew, only this item can fill the reserve'); + $schema->storage->txn_rollback; +}; + +subtest "CanBookBeRenewed AllowRenewalIfOtherItemsAvailable multiple branches (transfers are limited)" => sub { + plan tests => 10; + + my $schema = Koha::Database->schema; + $schema->storage->txn_begin; + + # Patrons from three different branches + my $patron_borrower_1 = $builder->build_object({ class => 'Koha::Patrons' }); + my $patron_borrower_2 = $builder->build_object({ class => 'Koha::Patrons' }); + my $patron_hold = $builder->build_object({ class => 'Koha::Patrons' }); + my $biblio = $builder->build_sample_biblio(); + + my $branch1 = $patron_borrower_1->branchcode; + my $branch2 = $patron_borrower_2->branchcode; + + # Item at each patron branch + my $item_1 = $builder->build_sample_item({ + biblionumber => $biblio->biblionumber, + homebranch => $branch1 + }); + my $item_2 = $builder->build_sample_item({ + biblionumber => $biblio->biblionumber, + homebranch => $branch2, + itype => $item_1->effective_itemtype + }); + + # checkout item_1 + my $issue1 = AddIssue( $patron_borrower_1->unblessed, $item_1->barcode); + my $datedue1 = dt_from_string( $issue1->date_due() ); + is (defined $issue1->date_due(), 1, "Item 1 checked out, due date: " . $issue1->date_due() ); + + # checkout item_2 + my $issue2 = AddIssue( $patron_borrower_2->unblessed, $item_2->barcode); + my $datedue2 = dt_from_string( $issue2->date_due() ); + is (defined $issue2->date_due(), 1, "Item 2 checked out, due date: " . $issue2->date_due() ); + + AddReserve( + { + branchcode => $branch1, + borrowernumber => $patron_hold->borrowernumber, + biblionumber => $biblio->biblionumber, + priority => 1, + reservation_date => dt_from_string(), + expiration_date => undef, + itemnumber => $item_1->itemnumber, + found => undef, + } + ); + + my $AllowRenewalIfOtherItemsAvailable = C4::Context->preference('AllowRenewalIfOtherItemsAvailable'); + my $UseBranchTransferLimits = C4::Context->preference('UseBranchTransferLimits'); + my $BranchTransferLimitsType = C4::Context->preference('BranchTransferLimitsType'); + + warn $UseBranchTransferLimits; + + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); + + my ( $renewokay, $error ) = CanBookBeRenewed($patron_borrower_1->borrowernumber, $item_1->itemnumber); + is( $renewokay, 0, 'Cannot renew item1, no other items available for hold'); + is( $error, 'on_reserve', 'Cannot renew, reserved (returned error is on_reserve)'); + + t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '1' ); + t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', 'itemtype' ); + + ( $renewokay, $error ) = CanBookBeRenewed($patron_borrower_2->borrowernumber, $item_2->itemnumber); + is( $renewokay, 0, 'Cannot renew item2, no other items available for hold, transfers are not permitted'); + is( $error, 'on_reserve', 'Cannot renew, reserved (returned error is on_reserve)'); + + my $limit = Koha::Item::Transfer::Limit->new( + { + toBranch => $branch1, + fromBranch => $branch2, + itemtype => $item_1->effective_itemtype, + })->store(); + + ( $renewokay, $error ) = CanBookBeRenewed($patron_borrower_1->borrowernumber, $item_1->itemnumber); + is( $renewokay, 0, 'Cannot renew item1, no other items available for hold'); + is( $error, 'on_reserve', 'Cannot renew, reserved (returned error is on_reserve)'); + + ( $renewokay, $error ) = CanBookBeRenewed($patron_borrower_2->borrowernumber, $item_2->itemnumber); + is( $renewokay, 1, 'Can renew item2, it\'s on on different branch than hold, transfers are permitted between those two branches'); + is( $error, undef, 'Can renew, no holds on the same branch'); + + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', $AllowRenewalIfOtherItemsAvailable ); + t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', $UseBranchTransferLimits ); + t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', $BranchTransferLimitsType ); + + $schema->storage->txn_rollback; }; subtest "GetIssuingCharges tests" => sub { -- 2.41.0