|
Description
Paul Derscheid
2026-02-24 15:57:09 UTC
Created attachment 193776 [details] [review] 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 Created attachment 193778 [details] [review] 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/> Created attachment 193779 [details] [review] 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/> Created attachment 193780 [details] [review] 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/> 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. Created attachment 193799 [details] [review] 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. Created attachment 193827 [details] [review] 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. Created attachment 193828 [details] [review] 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. 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.
Created attachment 193829 [details] [review] 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> Created attachment 193830 [details] [review] 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> Created attachment 193831 [details] [review] 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> Created attachment 193832 [details] [review] 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> Created attachment 193833 [details] [review] 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> Created attachment 193834 [details] [review] 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> |