[Bug 27834] New: CircControl syspref is used wrong in TooMany function C4::Circulation
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Bug ID: 27834 Summary: CircControl syspref is used wrong in TooMany function C4::Circulation Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: joonas.kylmala@helsinki.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com The CircControl syspref description says
Use the checkout and fines rules of PickupLibrary: the library you are logged in at. PatronLibrary: the library the patron is from. ItemHomeLibrary: the library the item is from.
This is mostly true but there is one exception in the TooMany function where it actually doesn't just change which libraries rules are used but how the the calculation is performed. With PatronLibrary set the behaviour is as expected by reading the syspref description. However with the setting PickupLibrary the behaviour changes now, instead of just using the rules of the pickup library we also only take into account the loans in the pickup library when calculating whether we are over the checkout limit or not. The same happens with ItemHomeLibrary where we instead calculate the patron's checkouts in the home library of the item being checked out. We should really try to fix this either by a) showing that ItemHomeLibrary and PickupLibrary is unacceptable and no one actually wants that, that would allow then making the behaviour similar as what it is to PatronLibrary (so just selecting the right circ rule instead of changing calculation method). b) Provide a new syspref for changing the calcutation method when PickupLibrary or ItemHomeLibrary CircControl values are used, and set it on upgrade enabled as default. Ps. svc/letters/preview might use CirControl wrongly as well, I'm not 100% I didn't quite understand but as for other usages of CircControl syspref I grepped where it was used and it correctly only changed the circ rules to be used instead of changing behaviour. So in addition to TooMany we might have to do some changes to svc/letters/preview if it turns out it changes behaviour as well. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #1 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- IMHO, easiest way would be to go with option b): b) Provide a new syspref for changing the calcutation method when PickupLibrary or ItemHomeLibrary CircControl values are used, and set it on upgrade enabled as default. We might miss some corner case even if we deem option a) safe and so if this new syspref introduced by option b) turns out unused for all users then we can very easily remove it in future and revert it if a corner case comes there. With option a) reverting is not as simple. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nugged@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Lisette Scheer <lisette.scheer@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette.scheer@bywatersolut | |ions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Lisette Scheer <lisette.scheer@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |anne-claire.bernaudin@univ- | |rennes1.fr --- Comment #2 from Lisette Scheer <lisette.scheer@bywatersolutions.com> --- *** Bug 21045 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emily.lamancusa@montgomeryc | |ountymd.gov --- Comment #3 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Still valid. It seems strange to me that only checkout limit behaves this way, but all other limits apply globally (if I am understanding correctly). That being said, I could see a consortium wanting to enforce checkout limits per branch, rather than total checkout limits, in a situation where the libraries are mostly independent but are sharing a Koha instance. I agree that adding a syspref would be a good fix - it's pretty straightforward, would preserve existing behavior for libraries that want it, and would allow CircControl's behavior to work the way the description makes it sound like it should for libraries who want that. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Ray Delahunty <r.delahunty@arts.ac.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |r.delahunty@arts.ac.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #4 from Ray Delahunty <r.delahunty@arts.ac.uk> --- I wish we had known in 2015 when we were selecting a new LMS that Koha circcontrol works this way. We set maximum loans globally (differing by patron group) rather than max loans being a multiplier of the number of libraries, and the only way we can do this is to set circcontrol to use the rules (and calendar!) from the library the patron belongs to. Our users move around the libraries and carry their rules with them and impose them on the libraries they visit(!). This means that hourly loans (like our laptops that loan for 5 hours) have never worked correctly coming up to library closing time and at weekends where some of our libraries are open, and some are not. This effects of this bug are now much more visible as it destroys the advantages offered in 24.11 with ConsiderLibraryHoursInCirulation and the addition of opening and closing times. Our support company does not see this as a bug which should be fixed as routine support and are suggesting we fund development to possibly add a system preference as Emily suggests. Without this fixed Koha circulation will never work correctly for hourly loan at our institution (or others that operate like our do). I have no idea if we can find development funds, but am about to find out. Depending on the work involved we may need help… in a joint funding situation. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 186652 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186652&action=edit Bug 27834: Add CheckoutLimitScope system preference This adds a new system preference CheckoutLimitScope to control how checkout limits are calculated when CircControl is set to PickupLibrary or ItemHomeLibrary. The preference has two options: - branch_specific: Count only checkouts matching the circulation rule context (traditional PickupLibrary/ItemHomeLibrary behavior) - all: Count all patron checkouts regardless of branch (PatronLibrary behavior) Sponsored-by: University of the Arts London -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 186653 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186653&action=edit Bug 27834: Fix CircControl behavior in TooMany function 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com | |, nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #7 from Ray Delahunty <r.delahunty@arts.ac.uk> --- I'm looking forward to configuring all this and testing the fix out but in my Open Fifth sandbox I cannot run the updatedatabase.pl script. Once our support staff run the script for me (assuming that can be done on a sandbox, and a KTD isn't needed) I hope to be able to see the new syspref and then I can get testing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 187296 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=187296&action=edit Bug 27834: Database update - Add CircControlCheckoutLimitScope system preference This patch adds the database components for the new CircControlCheckoutLimitScope system preference. The preference provides three explicit options for controlling how checkout limits (maxissueqty and patron_maxissueqty) are calculated: * 'all' (default) - Count all patron checkouts across all libraries * 'item' - Count only checkouts of items from the same library as the item being checked out (follows HomeOrHoldingBranch preference) * 'checkout' - Count only checkouts made at the same library as the current checkout This gives libraries explicit control over checkout limit scope, independent of the CircControl preference. Sponsored-by: University of the Arts London -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 187297 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=187297&action=edit Bug 27834: Use new CircControlCheckoutLimitScope system preference This patch updates the TooMany function to use the new CircControlCheckoutLimitScope system preference, providing explicit control over how checkout limits (maxissueqty and patron_maxissueqty) are calculated, independent of the CircControl preference. Previously, the TooMany function conflated two separate concerns: 1. CircControl - which circulation rules to apply 2. Checkout counting scope - which checkouts count toward limits This caused the checkout counting behavior to change based on CircControl, even when libraries wanted consistent limit enforcement regardless of which circulation rules applied. The new CircControlCheckoutLimitScope preference offers three options: * 'all' (default) - Count all patron checkouts across all libraries * 'item' - Count only checkouts of items from the same library as the item being checked out (follows HomeOrHoldingBranch preference) * 'checkout' - Count only checkouts made at the same library as the current checkout Implementation note: When scope is 'item', the system counts checkouts of items from the same library as the item being checked out (using the item's homebranch or holdingbranch). This applies regardless of whether the matching circulation rule is branch-specific or general (branch=*), ensuring consistent behavior across all rule types. This gives libraries explicit, flexible control over checkout limit scope while maintaining CircControl's intended purpose of rule selection. Test plan: 1. Apply patches and update database: perl installer/data/mysql/updatedatabase.pl 2. Verify new preference exists: Administration > System preferences > Circulation Search for "CircControlCheckoutLimitScope" Default value should be 'all' 3. 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 for both branches: * Branch1: maxissueqty = 1 for itemtype/category * Branch2: maxissueqty = 1 for itemtype/category 4. Test scope = 'checkout': a. Set CircControlCheckoutLimitScope = 'checkout' b. Log in as staff from Branch1 c. Check out item from Branch1 to patron → succeeds d. Try to check out another item from Branch1 → blocked (limit: 1) e. Log in as staff from Branch2 f. Check out item from Branch2 to same patron → succeeds g. Expected: Checkouts at different libraries don't count together 5. Test scope = 'all': a. Set CircControlCheckoutLimitScope = 'all' b. Return all items c. Log in as staff from Branch1 d. Check out item from Branch1 to patron → succeeds e. Log in as staff from Branch2 f. Try to check out item from Branch2 to same patron → blocked g. Expected: All patron checkouts count toward limit regardless of location 6. Test scope = 'item': a. Set CircControlCheckoutLimitScope = 'item' b. Set HomeOrHoldingBranch = 'homebranch' c. Return all items d. Check out item from Branch1 to patron → succeeds e. Try to check out another item from Branch1 → blocked (limit: 1) f. Check out item from Branch2 to same patron → succeeds g. Expected: Only items from same home library count together h. Note: When checking out an item, only checkouts of items from the same library as that item count toward the limit 7. Test scope applies to all rule types: a. With scope='item', test with a general rule (branch=*) b. Check out items from different branches c. Expected: Scope still applies - only items from the same branch as the item being checked out are counted 8. Test independence from CircControl: a. Test each scope with CircControl = 'PickupLibrary' b. Test each scope with CircControl = 'ItemHomeLibrary' c. Test each scope with CircControl = 'PatronLibrary' d. Expected: CircControlCheckoutLimitScope behavior is consistent regardless of CircControl setting 9. Run tests: prove t/db_dependent/Circulation/TooMany.t 10. Test with patron_maxissueqty: - Set up patron-level limits instead of item-type limits - Verify all three scopes work correctly - Test with both rule types active 11. Verify existing functionality: - Test on-site checkouts still work - Test item type hierarchies still work - Test unlimited ("") limits still work - prove t/db_dependent/Circulation/ Sponsored-by: University of the Arts London -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186652|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186653|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- OK.. so after a chat with Nick around this, I found I needed to think a little more deeply and hense the new patch. I'll get this onto a test system for you imminently Ray. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #187296|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189542 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189542&action=edit Bug 27834: Database update - Add CircControlCheckoutLimitScope system preference This patch adds the database components for the new CircControlCheckoutLimitScope system preference. The preference provides three explicit options for controlling how checkout limits (maxissueqty and patron_maxissueqty) are calculated: * 'all' (default) - Count all patron checkouts across all libraries * 'item' - Count only checkouts of items from the same library as the item being checked out (follows HomeOrHoldingBranch preference) * 'checkout' - Count only checkouts made at the same library as the current checkout This gives libraries explicit control over checkout limit scope, independent of the CircControl preference. Sponsored-by: University of the Arts London Signed-off-by: Ray Delahunty <lib-systems@arts.ac.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #187297|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189543&action=edit Bug 27834: Use new CircControlCheckoutLimitScope system preference This patch updates the TooMany function to use the new CircControlCheckoutLimitScope system preference, providing explicit control over how checkout limits (maxissueqty and patron_maxissueqty) are calculated, independent of the CircControl preference. Previously, the TooMany function conflated two separate concerns: 1. CircControl - which circulation rules to apply 2. Checkout counting scope - which checkouts count toward limits This caused the checkout counting behavior to change based on CircControl, even when libraries wanted consistent limit enforcement regardless of which circulation rules applied. The new CircControlCheckoutLimitScope preference offers three options: * 'all' (default) - Count all patron checkouts across all libraries * 'item' - Count only checkouts of items from the same library as the item being checked out (follows HomeOrHoldingBranch preference) * 'checkout' - Count only checkouts made at the same library as the current checkout Implementation note: When scope is 'item', the system counts checkouts of items from the same library as the item being checked out (using the item's homebranch or holdingbranch). This applies regardless of whether the matching circulation rule is branch-specific or general (branch=*), ensuring consistent behavior across all rule types. This gives libraries explicit, flexible control over checkout limit scope while maintaining CircControl's intended purpose of rule selection. Test plan: 1. Apply patches and update database: perl installer/data/mysql/updatedatabase.pl 2. Verify new preference exists: Administration > System preferences > Circulation Search for "CircControlCheckoutLimitScope" Default value should be 'all' 3. 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 for both branches: * Branch1: maxissueqty = 1 for itemtype/category * Branch2: maxissueqty = 1 for itemtype/category 4. Test scope = 'checkout': a. Set CircControlCheckoutLimitScope = 'checkout' b. Log in as staff from Branch1 c. Check out item from Branch1 to patron → succeeds d. Try to check out another item from Branch1 → blocked (limit: 1) e. Log in as staff from Branch2 f. Check out item from Branch2 to same patron → succeeds g. Expected: Checkouts at different libraries don't count together 5. Test scope = 'all': a. Set CircControlCheckoutLimitScope = 'all' b. Return all items c. Log in as staff from Branch1 d. Check out item from Branch1 to patron → succeeds e. Log in as staff from Branch2 f. Try to check out item from Branch2 to same patron → blocked g. Expected: All patron checkouts count toward limit regardless of location 6. Test scope = 'item': a. Set CircControlCheckoutLimitScope = 'item' b. Set HomeOrHoldingBranch = 'homebranch' c. Return all items d. Check out item from Branch1 to patron → succeeds e. Try to check out another item from Branch1 → blocked (limit: 1) f. Check out item from Branch2 to same patron → succeeds g. Expected: Only items from same home library count together h. Note: When checking out an item, only checkouts of items from the same library as that item count toward the limit 7. Test scope applies to all rule types: a. With scope='item', test with a general rule (branch=*) b. Check out items from different branches c. Expected: Scope still applies - only items from the same branch as the item being checked out are counted 8. Test independence from CircControl: a. Test each scope with CircControl = 'PickupLibrary' b. Test each scope with CircControl = 'ItemHomeLibrary' c. Test each scope with CircControl = 'PatronLibrary' d. Expected: CircControlCheckoutLimitScope behavior is consistent regardless of CircControl setting 9. Run tests: prove t/db_dependent/Circulation/TooMany.t 10. Test with patron_maxissueqty: - Set up patron-level limits instead of item-type limits - Verify all three scopes work correctly - Test with both rule types active 11. Verify existing functionality: - Test on-site checkouts still work - Test item type hierarchies still work - Test unlimited ("") limits still work - prove t/db_dependent/Circulation/ Sponsored-by: University of the Arts London Signed-off-by: Ray Delahunty <lib-systems@arts.ac.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED CC| |m.de.rooy@rijksmuseum.nl --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Joonas Kylmälä from comment #0)
a) showing that ItemHomeLibrary and PickupLibrary is unacceptable and no one actually wants that, that would allow then making the behaviour similar as what it is to PatronLibrary (so just selecting the right circ rule instead of changing calculation method).
ItemHomeLibrary is by far the most selected choice on HEA. 95% -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #15 from Ray Delahunty <raymund.delahunty@outlook.com> --- On our production server we are using PickupLibrary (rules, calendar and in 24.11, opening hours) to ensure hourly loans work in our multiple library university, with branches having different open hours (some 24/7, others 24/4 for example) and also different closed days. Now that we can rely on the due-back time we plan to use the patron emailer to warn patrons that their 5 hour loan laptops (in very high demand) are due back, before overdue fines are levied. Hourly loans appear to be working now, and maxloans is under control (at database level rather than per library)… which it wasn’t when we had to use the PatronLibrary option for circcontrol. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Reading the two comments of Ray, I understand the following: Your actual problem was: You needed the calendar and rules of the pickup lib (so normally CircControl should be Pickup library), but maxloans did not work as expected since it only considered checkouts at a specific pickup library and you wanted a global count. In order to achieve that, you switched to Patron lib but you got a different rule/calendar.. Choosing between Wrong or Wrong. Did you now backport the patches from this report already and are you using Pickup library with the new scope All? Please let me know if I misunderstood. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- INITIALIZING THE PREF + # Add new system preference + $dbh->do( + q{ + INSERT IGNORE INTO systempreferences (variable, value, options, explanation, type) VALUES + ('CircControlCheckoutLimitScope', 'all', This needs attention. Because by far most libraries are at ItemLibrary and their calculation now changes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- The following code changes are similar but not identical: PREVIOUS item library. Note here that the branchcode from the rule is either "item branch" or undefined ! $checkouts = $patron->checkouts->search( { "item.$branch_type" => $maxissueqty_rule->branchcode } ); CURRENT item scope. Note that the fallback to homebranch (from CircControlBranch) is eliminated here ! $checkouts = $patron->checkouts->search( { "item.$branch_type" => $item->$branch_type } ); PREVIOUS pickup library. Note here that the branchcode from the rule is either the userenv branch OR undefined ! $checkouts = $patron->checkouts->search( { 'me.branchcode' => $maxissueqty_rule->branchcode } ); CURRENT pickup scope where pickup_branch comes from userenv $checkouts = $patron->checkouts->search( { 'me.branchcode' => $pickup_branch } ); -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I have the feeling that the current TooMany is already buggy and we are now adding a pref that turns 3 combinations into 9 combinations. Where most are probably not very sensible. Wondering if we should not just add an option to go to the All scope instead of the ItemLibary scope or the PickupLibrary scope. That would turn 3 combinations into 5 btw. But note it still feels like some "emergency bandage"? Will still be adding an additional comment with more fundamental considerations. Moving this to FQA (could have been ID too) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #20 from Ray Delahunty <raymund.delahunty@outlook.com> --- Created attachment 189751 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189751&action=edit Beta / custom code added but working as expected Actually Marcel, the choice of Pickup library or Item Home Library was secondary to having circcontrol conduct corrected, so we could move away from what I describe in my (long!) attachment as the ‘ridiculous option’ (indeed, choosing between Wrong or Wrong) of Patron Library. But yes, the global count was what we needed. Our support company did backport the patches 10 days before I left UAL (and yes, that resulted in an ‘interesting’ last week before my retirement!) and UAL is using Pickup library with the new scope ‘All’ option. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- CURRENT ALGORITHM IN SHORT We missed POD for TooMany ;) First look for effective rule with branch, cat, itype. If that is a branch rule, use CircControl filter else look at all checkouts. If the rule has an item type, only consider those checkouts, else exclude item types with a rule on the SAME level (either global or branch). After that it looks at a second rule (GetBranchBorrowerCircRule/patron_maxissueqty) under Default checkout policy (global or by patron category but without item type). Checkouts are always filtered here by CircControl logic. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- TESTING SOME SAMPLE RULES A more fundamental question is how to interpret circ rules at global level (without branch) and branch level. To illustrate, an example with a few rules (branch, item type, patron category, max loans), ignoring paton category for simplicity: [GLOBAL] Undef, undef, undef, 4 [GLOBAL] Undef, BK, undef, 2 [BRANCH] CPL, BK, undef, 1 [BRANCH] MPL, undef, undef, 3 And no rules for a third branch RPL. No patron_maxissueqty defined at any level. What if CircControl == ItemLibrary? A patron may have 8 books: 1 BK from CPL, 3 from MPL, 2 others from CPL, 2 from RPL. But in another checkout order I just got 6 books. Very confusing! What if CircControl == PatronLibrary? A CPL patron may have 5 ( 1 BK, 4 others), a MPL patron 3, a RPL patron 6. What if CircControl == PickupLibrary? A patron may have 9 books: 1 BK and 4 others picked up at CPL, 3 others picked up at MPL, 1 BK picked up at RPL While testing this, I was sometimes surprised by the results. Showing that we ended up in a very non-intuitive setup? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QUESTIONS The longer I look at the code, the more I am wondering how we should use the second rule with patron_maxissueqty. The current code feels like a bug. If that rule (with or without a specific patron category) is on branch level, I can understand that we might want to apply CircControl logic. But if it is on global level, I am convinced that we should not (just as we do for the first rule btw). By removing the filter, we apply a global cap as any user will intuitively understand the rule. I am not sure about the reason why PatronLibrary uses all patron checkouts when a branch level rule is applied. It feels inconsistent at the least. It makes more sense to me to apply item.hold_or_holdingbranch eq patronbranch here. And on global level use all checkouts as is done already. Same for the second patron_maxissueqty rule with PatronLibrary: at branch level apply the same filter, at global level look at all checkouts. Another thing is the coupling of calendar and circ rules. The example of Ray seems to illustrate that you might want to set them differently. This surely should be on a new report. Opening up a new report 41280 too for looking at the first two of these points (not disturbing this one too much). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41280 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Found another bug in the handling of the patron_maxissueqty rule. Will add a patch on the new report 41280. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41290 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #24)
Found another bug in the handling of the patron_maxissueqty rule. Will add a patch on the new report 41280.
See bug 41290 too. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Leaving this now while waiting for feedback. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Irene <i.tortorella@arts.ac.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |i.tortorella@arts.ac.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #27 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Hi Marcel, Thank you for the detailed review. Let me address your concerns: Re: Comment 17 (Default value and backwards compatibility) You're right that defaulting to 'all' changes behaviour for existing installations. However, this was a deliberate choice for a few reasons: 1. I felt that 'all' provided the most predictable and consistent behaviour - it always counts all patron checkouts regardless of context 2. The old behaviour was actually inconsistent: it depended on whether the matched rule was branch-specific or general (branch=*), which was confusing 3. I also felt that 'all' was the safest default from a circulation policy perspective - it prevents patrons from exceeding intended limits by checking out at multiple branches That said, if we think backwards compatibility should take precedence, we could change this. I'd certainly at least mention the change in the release notes. Re: Comment 18 (Semantic change: using item branch vs rule branch) This change was deliberate and is core to the patch's purpose. The old code conflated two separate concerns: 1. Rule matching (CircControl) - which circulation rule applies 2. Checkout counting scope - which checkouts count toward limits The old behaviour had an unintended consequence: when a general rule (branch=*) matched, it would fall back to counting ALL checkouts, even when libraries wanted branch-specific limits. This is exactly the problem UAL encountered with their laptop loans. The new system provides consistent, predictable behaviour regardless of rule specificity: - scope='item' always counts checkouts of items from the same library as the current item - scope='checkout' always counts checkouts made at the same pickup library - scope='all' always counts all checkouts This is explicitly documented in the commit message: "This applies regardless of whether the matching circulation rule is branch-specific or general (branch=*), ensuring consistent behaviour across all rule types." See test plan point 7: "Test scope applies to all rule types: With scope='item', test with a general rule (branch=*). Expected: Scope still applies" Re: Comment 19 (9 combinations complexity) You're right that this creates 9 possible combinations. However, this was actually at Nick's suggestion. Initially, I had a simpler design that more tightly coupled the two preferences (essentially 3+2 options like you suggest). Nick requested that we keep CircControl and CircControlCheckoutLimitScope completely independent to allow for maximum flexibility. While this does create more combinations, it gives libraries explicit control over both dimensions independently, and I came round to liking the specifics of the preferences That said, I agree some combinations may not make practical sense. We could: 1. Document which combinations are recommended 2. Simplify the preference if the community feels the complexity isn't justified 3. Keep it flexible but provide clear guidance Re: Comments 21-23 (Design questions about global rules) Your questions about whether global-level rules should apply CircControl filtering at all are interesting and touch on deeper architectural issues. I agree that the interaction between branch-level and global rules can be non-intuitive. However, this patch was scoped to solve a specific problem: libraries need control over checkout counting scope independent of which rules apply. The broader questions you raise about rule hierarchy and global rule behaviour might be better addressed in separate bugs (as you've started with 41280 and 41290). I was aiming for a clear separation of concerns here. CircControl determines which rule applies; CircControlCheckoutLimitScope determines which checkouts count. This was a deliberate architectural choice to provide libraries with explicit, predictable control. Thankyou again for looking, I realise this one is a bit contentious and the code was a bit of a mess to work out frankly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Martin Renvoize (ashimema) from comment #27) Thx for your detailed explanation.
That said, if we think backwards compatibility should take precedence, we could change this. I'd certainly at least mention the change in the release notes.
If we go this way, I would say that it must be changed.
The old behaviour had an unintended consequence: when a general rule (branch=*) matched, it would fall back to counting ALL checkouts, even when libraries wanted branch-specific limits. This is exactly the problem UAL encountered with their laptop loans.
While thinking about this before, I concluded that the handling of the second rule ($branch_borrower_circ_rule) in TooMany is wrong. It should have the same global/branch scope as in handling the first rule. See also 41280 and earlier comments. In the UAL case this could probably solve their issue. Since they wanted PickupLibrary, a global $branch_borrower_circ_rule rule would count all checkouts instead of using a pickup library filter.
The new system provides consistent, predictable behaviour regardless of rule specificity: [..] This is explicitly documented in the commit message: "This applies regardless of whether the matching circulation rule is branch-specific or general (branch=*), ensuring consistent behaviour across all rule types."
I am not sure if we are actually bending the concept of global and library circ rules here. We now make it possible to create a library rule that we can apply to all checkouts by setting this new pref. And vice versa, a global rule that we only apply on branch level. It feels more consistent to apply global rules only globally and branch rules only on branch level. See also 41280 about PatronLibrary; which is just inconsistent.
You're right that this creates 9 possible combinations. However, this was actually at Nick's suggestion.
@Nick: Could you clarify in this context why we need this flexibility? To me it feels like sacrificing consistency on circ rules.
Your questions about whether global-level rules should apply CircControl filtering at all are interesting and touch on deeper architectural issues. I agree that the interaction between branch-level and global rules can be non-intuitive. However, this patch was scoped to solve a specific problem: libraries need control over checkout counting scope independent of which rules apply. The broader questions you raise about rule hierarchy and global rule behaviour might be better addressed in separate bugs (as you've started with 41280 and 41290).
I just fear that the current solution will not make things easier. Having the impression that addressing the current bugs/inconsistencies along the lines of 41280 will not bypass the idea behind our circulation rules. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #29 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #28)
(In reply to Martin Renvoize (ashimema) from comment #27)
You're right that this creates 9 possible combinations. However, this was actually at Nick's suggestion.
@Nick: Could you clarify in this context why we need this flexibility? To me it feels like sacrificing consistency on circ rules.
The original patches had: CheckoutLimitScope - options All / BranchSpecific if BranchSpecific was chosen, we then followed CircControl CircControl - options PickupLibrary, PatronLibrary, ItemHomeLibrary What I didn't like was the combo BranchSpecific+PickupLibrary - since 'PickupLibrary' didn't really read right My suggestion of a new pref does make 9 combinations, but I think it makes sense as these are two things being controlled: 1 - CircControl - decides which circ rule are we using - these patches don't change this, we will still use the same rule as before 2 - CheckoutLimitScope - decides how we count the patron's checkouts against the rule - this is what libraries don't like, that the choice in #1 decides #2 - this is why I think it should be independent -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Coming back here, trying to reload :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Nick Clemens (kidclamp) from comment #29)
2 - CheckoutLimitScope - decides how we count the patron's checkouts against the rule - this is what libraries don't like, that the choice in #1 decides #2 - this is why I think it should be independent
It still feels kind of wrong to me to solve it like that. We are actually creating an override for circulation rules that are specified for specific branches to apply them globally or vice versa. (On the alternative bug, I am correcting TooMany to apply branch rules at branch level and global rules at glubal level. Which feels to me more consistent.) Looking back to the actual cause of this report, the library chose PatronLibrary instead of PickupLibrary to get a global scope (but a wrong calendar along). But if we solved the bug in TooMany that applies a global rule on branch level (see 41280) then they could have defined a global rule (read: without a branch) for patron_maxissueqty and stick to PickupLibrary for the right calendar. Could you still have a look at 41280? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Mathieu Saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mathsabypro@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834 Kris <kbecker@jcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kbecker@jcls.org -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org