Bugzilla – Attachment 186653 Details for
Bug 27834
CircControl syspref is used wrong in TooMany function C4::Circulation
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 27834: Fix CircControl behavior in TooMany function
Bug-27834-Fix-CircControl-behavior-in-TooMany-func.patch (text/plain), 13.34 KB, created by
Martin Renvoize (ashimema)
on 2025-09-22 06:51:05 UTC
(
hide
)
Description:
Bug 27834: Fix CircControl behavior in TooMany function
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-09-22 06:51:05 UTC
Size:
13.34 KB
patch
obsolete
>From 0bfea3d8f465859f2d40e6fda87a84a5e5603ce8 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 27834
:
186652
|
186653
|
187296
|
187297