Bug 41917 - Parent itemtype fallback missing for bookable flag and circulation rules
Summary: Parent itemtype fallback missing for bookable flag and circulation rules
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Paul Derscheid
QA Contact: Testopia
URL:
Keywords:
Depends on: 29002
Blocks:
  Show dependency treegraph
 
Reported: 2026-02-24 15:57 UTC by Paul Derscheid
Modified: 2026-02-25 07:32 UTC (History)
6 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: Sponsored
Comma delimited list of Sponsors: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/>
Crowdfunding goal: 0
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function: Bookings


Attachments
Bug 41917: Add parent itemtype fallback to Item::effective_bookable (5.56 KB, patch)
2026-02-24 16:23 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 41917: Add parent itemtype fallback to Item::effective_bookable (5.73 KB, patch)
2026-02-24 17:10 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 41917: Add parent itemtype fallback to Items::filter_by_bookable (4.48 KB, patch)
2026-02-24 17:10 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 41917: Add parent itemtype fallback to CirculationRules::get_effective_rule (6.25 KB, patch)
2026-02-24 17:10 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 41917: (QA follow-up) Fix effective_bookable item-level override (4.81 KB, patch)
2026-02-24 19:23 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: (QA follow-up) Derive parent pool rule from child rule in TooMany (3.00 KB, patch)
2026-02-25 07:27 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: (QA follow-up) Avoid extra SELECT in get_effective_rule fallback (1.47 KB, patch)
2026-02-25 07:27 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: Add parent itemtype fallback to Item::effective_bookable (5.79 KB, patch)
2026-02-25 07:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: Add parent itemtype fallback to Items::filter_by_bookable (4.54 KB, patch)
2026-02-25 07:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: Add parent itemtype fallback to CirculationRules::get_effective_rule (6.31 KB, patch)
2026-02-25 07:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: (QA follow-up) Fix effective_bookable item-level override (4.88 KB, patch)
2026-02-25 07:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: (QA follow-up) Derive parent pool rule from child rule in TooMany (3.06 KB, patch)
2026-02-25 07:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41917: (QA follow-up) Avoid extra SELECT in get_effective_rule fallback (1.53 KB, patch)
2026-02-25 07:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Derscheid 2026-02-24 15:57:09 UTC
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.
Comment 1 Paul Derscheid 2026-02-24 16:23:55 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
Comment 2 Paul Derscheid 2026-02-24 17:10:30 UTC
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/>
Comment 3 Paul Derscheid 2026-02-24 17:10:32 UTC
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/>
Comment 4 Paul Derscheid 2026-02-24 17:10:34 UTC
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/>
Comment 5 Martin Renvoize (ashimema) 2026-02-24 19:19:35 UTC
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.
Comment 6 Martin Renvoize (ashimema) 2026-02-24 19:23:59 UTC
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.
Comment 7 Martin Renvoize (ashimema) 2026-02-25 07:27:26 UTC
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.
Comment 8 Martin Renvoize (ashimema) 2026-02-25 07:27:27 UTC
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.
Comment 9 Martin Renvoize (ashimema) 2026-02-25 07:28:54 UTC
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.
Comment 10 Martin Renvoize (ashimema) 2026-02-25 07:32:05 UTC
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>
Comment 11 Martin Renvoize (ashimema) 2026-02-25 07:32:06 UTC
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>
Comment 12 Martin Renvoize (ashimema) 2026-02-25 07:32:08 UTC
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>
Comment 13 Martin Renvoize (ashimema) 2026-02-25 07:32:09 UTC
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>
Comment 14 Martin Renvoize (ashimema) 2026-02-25 07:32:10 UTC
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>
Comment 15 Martin Renvoize (ashimema) 2026-02-25 07:32:12 UTC
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>