[Bug 41917] New: Parent itemtype fallback missing for bookable flag and circulation rules
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Bug ID: 41917 Summary: Parent itemtype fallback missing for bookable flag and circulation rules Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: paul.derscheid@lmscloud.de QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle@bywatersolutions.com When item types are organized in a parent-child hierarchy, child item types do not inherit settings from their parent. This means that every child item type needs its own bookable flag and its own set of circulation rules, even when the parent already defines them. The expected lookup order should be: 1. Item-level / child itemtype-specific value 2. Parent itemtype value 3. Global default Currently step 2 is skipped entirely, so the system jumps straight from a missing child-specific value to the global default. This affects Item::effective_bookable, Items::filter_by_bookable, and CirculationRules::get_effective_rule. -- 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=41917 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk Circulation| |Bookings function| | Depends on| |29002 Assignee|koha-bugs@lists.koha-commun |paul.derscheid@lmscloud.de |ity.org | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29002 [Bug 29002] Add ability to book items ahead of time -- 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=41917 --- Comment #1 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 193776 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193776&action=edit Bug 41917: Add parent itemtype fallback to Item::effective_bookable - Check parent itemtype bookable flag when child itemtype is not bookable - Item-level and child-level bookable values still take priority To test: - Run t/db_dependent/Koha/Item.t - Verify effective_bookable falls back to parent itemtype when child is not bookable - Verify child-level and item-level bookable take priority - Verify orphan itemtypes without a parent return 0 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Paul Derscheid <paul.derscheid@lmscloud.de> 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=41917 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193776|0 |1 is obsolete| | --- Comment #2 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 193778 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193778&action=edit Bug 41917: Add parent itemtype fallback to Item::effective_bookable - Check parent itemtype bookable flag when child itemtype is not bookable - Item-level and child-level bookable values still take priority To test: - Run t/db_dependent/Koha/Item.t - Verify effective_bookable falls back to parent itemtype when child is not bookable - Verify child-level and item-level bookable take priority - Verify orphan itemtypes without a parent return 0 Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #3 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 193779 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193779&action=edit Bug 41917: Add parent itemtype fallback to Items::filter_by_bookable - Include children of bookable parent itemtypes in the bookable filter - Add explicit Koha::ItemTypes import To test: - Run t/db_dependent/Koha/Items.t - Verify filter_by_bookable includes items whose child itemtype has a bookable parent (both itype modes) - Verify items with explicit bookable=0 are excluded even when parent is bookable Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #4 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 193780 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193780&action=edit Bug 41917: Add parent itemtype fallback to CirculationRules::get_effective_rule - When only a global rule is found, check for a parent itemtype-specific rule - Two-query strategy preserves existing priority ordering To test: - Run t/db_dependent/Koha/CirculationRules.t - Verify child itemtype falls back to parent-specific rule over the global default - Verify child-specific rules take priority over parent rules - Verify orphan itemtypes still fall back to global - Verify branch-specific parent rules take priority Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Hmm, there's certainly at least one QA Fail here.. but I'll add a follow-up for it. [Item level bookable = 0 now get's hard overridden by parent itemtype.. this would be a regression] But the bigger question regards the third commit and the change to get_effective_rule (Koha/CirculationRules.pm:298). There's lots of callers to this method and your changing the return so the regression scope it large. This change makes all circulation rules silently inherit from parent itemtype when no child-specific rule is found. This is a broad behavioural change, not scoped to bookings. A key concern is the interaction with C4/Circulation.pm::TooMany (lines 441–585), which already has its own explicit parent-type handling for maxissueqty. After the patch: - $maxissueqty_rule = get_effective_rule(itemtype => $child_type) can now return the parent's specific rule instead of the global rule. - This means $maxissueqty_rule->itemtype is no longer NULL (it's the parent type). - Line 575: if ($maxissueqty_rule->rule_value < $parent_maxissueqty_rule->rule_value && defined($maxissueqty_rule->itemtype)) — with the patch, both $maxissueqty_rule and $parent_maxissueqty_rule now hold the same parent rule. rule_value < rule_value is never true, so this branch is silently dead. This effectively changes checkout limit enforcement for child itemtypes. Tracing through with the existing TooMany.t scenario (branch=1, parent=2 rule), the tests appear to still pass — the counting and limit logic happen to converge on the same result. However, this relies on coincidental symmetry, not a deliberate test of the new code path. The TooMany.t parent-type subtest (t/db_dependent/Circulation/TooMany.t:864) should be run explicitly against this branch to verify, and ideally a test should be added for the affected $rule_itemtype path. Other callers that will silently change behaviour for child itemtypes include: - returnbranch — affects where items are routed on return - fine / lengthunit — affects fine calculation - opacitemholds — affects hold permissions Libraries with parent/child itemtype hierarchies and any parent-specific rules could see unexpected changes to all of these — not just bookings. So, I suppose the question is.. Was this a deliberate choice of the original implimentation or an oversight.. and in either case, is it likely to be in practical use anywhere in such a way that this would cause an active regression. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193799&action=edit Bug 41917: (QA follow-up) Fix effective_bookable item-level override The previous implementation used the // (defined-or) operator then checked !$bookable, which could not distinguish between an explicit item-level bookable=0 and an inherited itemtype bookable=0. Both triggered the parent itemtype fallback, allowing a bookable parent to override an item explicitly marked as not bookable. Fix by returning item->bookable immediately when it is defined (0 or 1), so the parent fallback is only reached when item->bookable is NULL. Also caches $self->itemtype in a local variable, consistent with the effective_not_for_loan_status pattern. Adds a missing test: item bookable=0 with a bookable parent itemtype should still return 0. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aude.charillon@openfifth.co | |.uk, | |jonathan.druart@gmail.com, | |katrin.fischer@bsz-bw.de, | |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=41917 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193827 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193827&action=edit Bug 41917: (QA follow-up) Derive parent pool rule from child rule in TooMany Now that CirculationRules::get_effective_rule performs parent itemtype fallback internally, a child item with no specific maxissueqty rule will already receive the parent rule back from that call. Deriving $parent_maxissueqty_rule from $maxissueqty_rule in that case avoids a redundant second get_effective_rule call for the parent itemtype. When the child does have its own specific rule the parent pool limit is still fetched explicitly, preserving the existing behaviour for that path. Also moves the $maxissueqty_rule lookup before the parent rule determination so the result is available for the comparison. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193828 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193828&action=edit Bug 41917: (QA follow-up) Avoid extra SELECT in get_effective_rule fallback The parent itemtype fallback in get_effective_rule called $itemtype_obj->parent, which uses the DBIx::Class belongs_to relationship accessor and issues a second SELECT to load the full parent row. Only the parent's itemtype code is needed for the subsequent search, so replace it with $itemtype_obj->parent_type which goes through Koha::Object's AUTOLOAD and reads the already- fetched column value directly with no extra query. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- After a bit of a merry-go-round here, I've come to the conclusion we're fixing a bug in the existing get_effective_rule logic. The itemtypes page clearly states the expectation with the following hint under the 'parent type select'
Defining a parent type will apply checkout limits for all children as described on the circulation rules page.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch Sponsorship status|--- |Sponsored Status|Needs Signoff |Signed Off Comma delimited| |Büchereizentrale list of Sponsors| |Schleswig-Holstein | |<https://www.bz-sh.de/> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193778|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=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193779|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=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193780|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=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193799|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=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193827|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=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193828|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=41917 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193829 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193829&action=edit Bug 41917: Add parent itemtype fallback to Item::effective_bookable - Check parent itemtype bookable flag when child itemtype is not bookable - Item-level and child-level bookable values still take priority To test: - Run t/db_dependent/Koha/Item.t - Verify effective_bookable falls back to parent itemtype when child is not bookable - Verify child-level and item-level bookable take priority - Verify orphan itemtypes without a parent return 0 Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193830 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193830&action=edit Bug 41917: Add parent itemtype fallback to Items::filter_by_bookable - Include children of bookable parent itemtypes in the bookable filter - Add explicit Koha::ItemTypes import To test: - Run t/db_dependent/Koha/Items.t - Verify filter_by_bookable includes items whose child itemtype has a bookable parent (both itype modes) - Verify items with explicit bookable=0 are excluded even when parent is bookable Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193831 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193831&action=edit Bug 41917: Add parent itemtype fallback to CirculationRules::get_effective_rule - When only a global rule is found, check for a parent itemtype-specific rule - Two-query strategy preserves existing priority ordering To test: - Run t/db_dependent/Koha/CirculationRules.t - Verify child itemtype falls back to parent-specific rule over the global default - Verify child-specific rules take priority over parent rules - Verify orphan itemtypes still fall back to global - Verify branch-specific parent rules take priority Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193832 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193832&action=edit Bug 41917: (QA follow-up) Fix effective_bookable item-level override The previous implementation used the // (defined-or) operator then checked !$bookable, which could not distinguish between an explicit item-level bookable=0 and an inherited itemtype bookable=0. Both triggered the parent itemtype fallback, allowing a bookable parent to override an item explicitly marked as not bookable. Fix by returning item->bookable immediately when it is defined (0 or 1), so the parent fallback is only reached when item->bookable is NULL. Also caches $self->itemtype in a local variable, consistent with the effective_not_for_loan_status pattern. Adds a missing test: item bookable=0 with a bookable parent itemtype should still return 0. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193833 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193833&action=edit Bug 41917: (QA follow-up) Derive parent pool rule from child rule in TooMany Now that CirculationRules::get_effective_rule performs parent itemtype fallback internally, a child item with no specific maxissueqty rule will already receive the parent rule back from that call. Deriving $parent_maxissueqty_rule from $maxissueqty_rule in that case avoids a redundant second get_effective_rule call for the parent itemtype. When the child does have its own specific rule the parent pool limit is still fetched explicitly, preserving the existing behaviour for that path. Also moves the $maxissueqty_rule lookup before the parent rule determination so the result is available for the comparison. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193834 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193834&action=edit Bug 41917: (QA follow-up) Avoid extra SELECT in get_effective_rule fallback The parent itemtype fallback in get_effective_rule called $itemtype_obj->parent, which uses the DBIx::Class belongs_to relationship accessor and issues a second SELECT to load the full parent row. Only the parent's itemtype code is needed for the subsequent search, so replace it with $itemtype_obj->parent_type which goes through Koha::Object's AUTOLOAD and reads the already- fetched column value directly with no extra query. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41917 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|martin.renvoize@openfifth.c |testopia@bugs.koha-communit |o.uk |y.org -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org