https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27834
--- Comment #27 from Martin Renvoize (ashimema) <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
