From 0bfea3d8f465859f2d40e6fda87a84a5e5603ce8 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 29 Aug 2025 13:55:40 +0100 Subject: [PATCH] Bug 27834: Fix CircControl behavior in TooMany function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch fixes the TooMany function in C4::Circulation to separate circulation rule selection from checkout counting behavior, addressing the mismatch between CircControl's documented purpose and actual behavior. Previously, CircControl incorrectly controlled both: 1. Which circulation rules to apply (intended behavior) 2. How checkout limits are calculated (unintended side effect) This caused inconsistent behavior where checkout limits would change based on the user's location or item's library, even when the same circulation rules applied. This patch: - Maintains CircControl's intended purpose (rule selection only) - Uses the new CheckoutLimitScope preference to control checkout counting - Preserves backward compatibility with existing behavior Test plan: 1. Apply patches and restart services 2. Run: perl installer/data/mysql/updatedatabase.pl 3. Verify the new CheckoutLimitScope preference exists in system preferences 4. Set up test environment: - Create two branches (Branch1, Branch2) - Create a patron category and item type - Create a patron in Branch1 - Create items from both branches - Set circulation rules: * Branch1: maxissueqty = 1 for the itemtype/category * Branch2: maxissueqty = 1 for the itemtype/category 5. Test PickupLibrary with branch_specific scope (default): a. Set CircControl = PickupLibrary b. Set CheckoutLimitScope = branch_specific c. Login as staff from Branch1, issue item from Branch1 to patron d. Try to checkout another item from Branch1 → should be blocked (limit reached) e. Login as staff from Branch2, try to checkout item from Branch2 to same patron f. Expected: Checkout allowed (no checkouts from Branch2 pickup location) 6. Test PickupLibrary with all scope: a. Keep CircControl = PickupLibrary b. Set CheckoutLimitScope = all c. From Branch2, try to checkout item to same patron d. Expected: Checkout blocked (patron already has 1 checkout total) 7. Test ItemHomeLibrary with branch_specific scope: a. Set CircControl = ItemHomeLibrary b. Set CheckoutLimitScope = branch_specific c. Reset patron (return all items) d. Checkout item from Branch1 to patron e. Try to checkout another item from Branch1 → should be blocked f. Try to checkout item from Branch2 → should be allowed g. Expected: Only items from same home library count toward limit 8. Test ItemHomeLibrary with all scope: a. Keep CircControl = ItemHomeLibrary b. Set CheckoutLimitScope = all c. Try to checkout item from Branch2 to same patron d. Expected: Checkout blocked (patron already has 1 checkout total) 9. Test PatronLibrary behavior unchanged: a. Set CircControl = PatronLibrary b. CheckoutLimitScope should be ignored c. Expected: Always counts all patron checkouts regardless of preference 10. Run tests: prove t/db_dependent/Circulation/TooMany.t 11. Test with patron-level limits: - Set up patron_maxissueqty limits at branch level - Verify same behavior patterns apply for patron-level limits - Test both CheckoutLimitScope settings 12. Verify existing functionality: - Test on-site checkouts still work correctly - Test item type hierarchies still work - Test unlimited ("") limits still work - Run broader circulation tests: prove t/db_dependent/Circulation/ Sponsored-by: University of the Arts London --- C4/Circulation.pm | 52 ++++++++---- t/db_dependent/Circulation/TooMany.t | 122 ++++++++++++++++++++++++++- 2 files changed, 157 insertions(+), 17 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index e03a5e27fb7..91b68f2bf6c 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -483,16 +483,26 @@ sub TooMany { my $checkouts; if ( $maxissueqty_rule->branchcode ) { - if ( C4::Context->preference('CircControl') eq 'PickupLibrary' ) { - $checkouts = $patron->checkouts->search( { 'me.branchcode' => $maxissueqty_rule->branchcode } ); - } elsif ( C4::Context->preference('CircControl') eq 'PatronLibrary' ) { - $checkouts = $patron->checkouts; # if branch is the patron's home branch, then count all loans by patron + my $checkout_limit_scope = C4::Context->preference('CheckoutLimitScope') || 'branch_specific'; + my $circ_control = C4::Context->preference('CircControl'); + + if ( $checkout_limit_scope eq 'branch_specific' + && ( $circ_control eq 'PickupLibrary' || $circ_control eq 'ItemHomeLibrary' ) ) + { + # Use branch-specific counting for PickupLibrary and ItemHomeLibrary + if ( $circ_control eq 'PickupLibrary' ) { + $checkouts = $patron->checkouts->search( { 'me.branchcode' => $maxissueqty_rule->branchcode } ); + } else { + my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; + $checkouts = $patron->checkouts->search( { "item.$branch_type" => $maxissueqty_rule->branchcode } ); + } } else { - my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; - $checkouts = $patron->checkouts->search( { "item.$branch_type" => $maxissueqty_rule->branchcode } ); + + # Default: count all patron checkouts (PatronLibrary behavior) + $checkouts = $patron->checkouts; } } else { - $checkouts = $patron->checkouts; # if rule is not branch specific then count all loans by patron + $checkouts = $patron->checkouts; # if rule is not branch specific then count all loans by patron } $checkouts = $checkouts->search( undef, { prefetch => 'item' } ); @@ -590,16 +600,26 @@ sub TooMany { and $branch_borrower_circ_rule->{patron_maxissueqty} ne '' ) { my $checkouts; - if ( C4::Context->preference('CircControl') eq 'PickupLibrary' ) { - $checkouts = $patron->checkouts->search( { 'me.branchcode' => $branch } ); - } elsif ( C4::Context->preference('CircControl') eq 'PatronLibrary' ) { - $checkouts = $patron->checkouts; # if branch is the patron's home branch, then count all loans by patron + my $checkout_limit_scope = C4::Context->preference('CheckoutLimitScope') || 'branch_specific'; + my $circ_control = C4::Context->preference('CircControl'); + + if ( $checkout_limit_scope eq 'branch_specific' + && ( $circ_control eq 'PickupLibrary' || $circ_control eq 'ItemHomeLibrary' ) ) + { + # Use branch-specific counting for PickupLibrary and ItemHomeLibrary + if ( $circ_control eq 'PickupLibrary' ) { + $checkouts = $patron->checkouts->search( { 'me.branchcode' => $branch } ); + } else { + my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; + $checkouts = $patron->checkouts->search( + { "item.$branch_type" => $branch }, + { prefetch => 'item' } + ); + } } else { - my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; - $checkouts = $patron->checkouts->search( - { "item.$branch_type" => $branch }, - { prefetch => 'item' } - ); + + # Default: count all patron checkouts (PatronLibrary behavior) + $checkouts = $patron->checkouts; } my $checkout_count = $checkouts->count; diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t index 700ca24f09f..6f5f6bbfa37 100755 --- a/t/db_dependent/Circulation/TooMany.t +++ b/t/db_dependent/Circulation/TooMany.t @@ -16,7 +16,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 12; +use Test::More tests => 13; use C4::Context; use C4::Members; @@ -1225,6 +1225,126 @@ subtest 'HomeOrHoldingBranch is used' => sub { teardown(); }; +subtest 'CheckoutLimitScope system preference controls checkout counting' => sub { + plan tests => 12; + + t::lib::Mocks::mock_preference( 'item-level_itypes', 1 ); + + my $branch1 = $builder->build( { source => 'Branch' } ); + my $branch2 = $builder->build( { source => 'Branch' } ); + my $category = $builder->build( { source => 'Category' } ); + my $itemtype = $builder->build( { source => 'Itemtype', value => { notforloan => 0 } } ); + + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + categorycode => $category->{categorycode}, + branchcode => $branch1->{branchcode}, + } + } + ); + + # Create items from different branches + my $item_branch1 = $builder->build_sample_item( + { + homebranch => $branch1->{branchcode}, + holdingbranch => $branch1->{branchcode}, + itype => $itemtype->{itemtype} + } + ); + + my $item_branch2 = $builder->build_sample_item( + { + homebranch => $branch2->{branchcode}, + holdingbranch => $branch2->{branchcode}, + itype => $itemtype->{itemtype} + } + ); + + # Set circulation rules for both branches + Koha::CirculationRules->set_rules( + { + branchcode => $branch1->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => $itemtype->{itemtype}, + rules => { + maxissueqty => 1, + } + } + ); + + Koha::CirculationRules->set_rules( + { + branchcode => $branch2->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => $itemtype->{itemtype}, + rules => { + maxissueqty => 1, + } + } + ); + + # Issue an item from branch1 + t::lib::Mocks::mock_userenv( { branchcode => $branch1->{branchcode} } ); + my $issue = C4::Circulation::AddIssue( $patron, $item_branch1->barcode, dt_from_string() ); + ok( $issue, 'Item from branch1 issued successfully' ); + + # Test PickupLibrary with CheckoutLimitScope = 'branch_specific' (default) + t::lib::Mocks::mock_preference( 'CircControl', 'PickupLibrary' ); + t::lib::Mocks::mock_preference( 'CheckoutLimitScope', 'branch_specific' ); + + # From branch1 - should count checkout made at branch1 + my $data = C4::Circulation::TooMany( $patron, $item_branch1 ); + ok( $data, 'TooMany should return limit exceeded when checking from same pickup library' ); + is( $data->{count}, 1, 'Should count checkout made at this pickup library' ); + is( $data->{reason}, 'TOO_MANY_CHECKOUTS', 'Correct reason returned' ); + + # From branch2 - should not count checkout made at branch1 + t::lib::Mocks::mock_userenv( { branchcode => $branch2->{branchcode} } ); + $data = C4::Circulation::TooMany( $patron, $item_branch2 ); + ok( !$data, 'TooMany should allow checkout when no checkouts from this pickup library' ); + + # Test PickupLibrary with CheckoutLimitScope = 'all' + t::lib::Mocks::mock_preference( 'CheckoutLimitScope', 'all' ); + + # Should count all patron checkouts regardless of pickup library + $data = C4::Circulation::TooMany( $patron, $item_branch2 ); + ok( $data, 'TooMany should return limit exceeded when counting all checkouts' ); + is( $data->{count}, 1, 'Should count all patron checkouts regardless of branch' ); + + # Test ItemHomeLibrary with CheckoutLimitScope = 'all' + t::lib::Mocks::mock_preference( 'CircControl', 'ItemHomeLibrary' ); + t::lib::Mocks::mock_preference( 'CheckoutLimitScope', 'all' ); + + # Should count all patron checkouts regardless of item home library + $data = C4::Circulation::TooMany( $patron, $item_branch2 ); + ok( $data, 'TooMany should return limit exceeded' ); + is( $data->{count}, 1, 'Should count all patron checkouts' ); + + # Test ItemHomeLibrary with CheckoutLimitScope = 'branch_specific' + t::lib::Mocks::mock_preference( 'CheckoutLimitScope', 'branch_specific' ); + + # Item from branch2 - should not count checkout of item from branch1 + $data = C4::Circulation::TooMany( $patron, $item_branch2 ); + ok( !$data, 'TooMany should allow checkout when no items from this home library checked out' ); + + # Item from branch1 - should count checkout of item from branch1 + my $item_branch1_2 = $builder->build_sample_item( + { + homebranch => $branch1->{branchcode}, + holdingbranch => $branch1->{branchcode}, + itype => $itemtype->{itemtype} + } + ); + + $data = C4::Circulation::TooMany( $patron, $item_branch1_2 ); + ok( $data, 'TooMany should return limit exceeded when item from same home library already checked out' ); + is( $data->{count}, 1, 'Should count checkout of item from same home library' ); + + teardown(); +}; + $schema->storage->txn_rollback; sub teardown { -- 2.51.0