Bug 30324 - Parent and child itemtype checkout limits not enforced as expected
Summary: Parent and child itemtype checkout limits not enforced as expected
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 20 votes (vote)
Assignee: Nick Clemens
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on: 21946
Blocks: 35776
  Show dependency treegraph
 
Reported: 2022-03-21 15:25 UTC by Andrew Fuerste-Henry
Modified: 2024-02-16 18:06 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30324: Unit tests (5.25 KB, patch)
2024-01-11 16:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30324: Don't use default rule if we have a parent rule (2.29 KB, patch)
2024-01-11 16:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30324: Unit tests (5.30 KB, patch)
2024-01-14 21:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 30324: Don't use default rule if we have a parent rule (2.34 KB, patch)
2024-01-14 21:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 30324: Don't use default rule if we have a parent rule (2.39 KB, patch)
2024-01-18 22:40 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-03-21 15:25:51 UTC
When creating rules or parent and child item types, checkout limits are enforced confusingly.

To recreate:
1: have an itype BK
2: create an itype BK2, set BK as the parent to BK2
3: create a circ rule for All Patrons, item type BK (displays as "Books (All)" in the circ rules interface), setting total checkouts to 1
4: do not create any rule specifically for the BK2 itype
5: have some BK items and some BK2 items
6: check a BK item out to a patron
7: check a BK2 item out to the same patron, observe you are not blocked from doing so
8: try to check out a second BK2 item to the same patron, get message "Too many checked out. 1 checked out, only 1 are allowed."
9: try to check out a second BK item to the same patrons, get message "Too many checked out. 2 checked out, only 1 are allowed."

10: return both items
11: check a BK2 item out to your patron
12: try to check a BK item out to your patron, get message "Too many checked out. 1 checked out, only 1 are allowed."

We're somehow not enforcing parent/child rules reciprocally. That forces me to explicitly define my child checkout limit, defeating some of the point of the enhancement from bug 21946.
Comment 1 Nick Clemens 2024-01-11 16:28:54 UTC
Created attachment 160877 [details] [review]
Bug 30324: Unit tests
Comment 2 Nick Clemens 2024-01-11 16:28:57 UTC
Created attachment 160878 [details] [review]
Bug 30324: Don't use default rule if we have a parent rule

Currenty, if we find only a defult rule for a checkout we are ignoring the parent rule.
We should not use the default if there is a parent rule

To test:
 1: have an itype BK
 2: create an itype BK2, set BK as the parent to BK2
 3: create a circ rule for All Patrons, item type BK (displays as "Books (All)" in the circ rules interface), setting total checkouts to 1
 4: do not create any rule specifically for the BK2 itype
 5: have some BK items and some BK2 items
 6: check a BK item out to a patron
 7: check a BK2 item out to the same patron, observe you are not blocked from doing so
 8: try to check out a second BK2 item to the same patron, get message "Too many checked out. 1 checked out, only 1 are allowed."
 9: try to check out a second BK item to the same patrons, get message "Too many checked out. 2 checked out, only 1 are allowed."
10: return both items
11: check a BK2 item out to your patron
12: try to check a BK item out to your patron, get message "Too many checked out. 1 checked out, only 1 are allowed."
13: Apply patch
14: Repeat 7, you are now blocked
15: Other results should be the same
Comment 3 David Nind 2024-01-14 21:35:29 UTC
Created attachment 161030 [details] [review]
Bug 30324: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2024-01-14 21:35:32 UTC
Created attachment 161031 [details] [review]
Bug 30324: Don't use default rule if we have a parent rule

Currenty, if we find only a defult rule for a checkout we are ignoring the parent rule.
We should not use the default if there is a parent rule

To test:
 1: have an itype BK
 2: create an itype BK2, set BK as the parent to BK2
 3: create a circ rule for All Patrons, item type BK (displays as "Books (All)" in the circ rules interface), setting total checkouts to 1
 4: do not create any rule specifically for the BK2 itype
 5: have some BK items and some BK2 items
 6: check a BK item out to a patron
 7: check a BK2 item out to the same patron, observe you are not blocked from doing so
 8: try to check out a second BK2 item to the same patron, get message "Too many checked out. 1 checked out, only 1 are allowed."
 9: try to check out a second BK item to the same patrons, get message "Too many checked out. 2 checked out, only 1 are allowed."
10: return both items
11: check a BK2 item out to your patron
12: try to check a BK item out to your patron, get message "Too many checked out. 1 checked out, only 1 are allowed."
13: Apply patch
14: Repeat 7, you are now blocked
15: Other results should be the same

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Biblibre Sandboxes 2024-01-18 22:40:22 UTC
Created attachment 161161 [details] [review]
Bug 30324: Don't use default rule if we have a parent rule

Currenty, if we find only a defult rule for a checkout we are ignoring the parent rule.
We should not use the default if there is a parent rule

To test:
 1: have an itype BK
 2: create an itype BK2, set BK as the parent to BK2
 3: create a circ rule for All Patrons, item type BK (displays as "Books (All)" in the circ rules interface), setting total checkouts to 1
 4: do not create any rule specifically for the BK2 itype
 5: have some BK items and some BK2 items
 6: check a BK item out to a patron
 7: check a BK2 item out to the same patron, observe you are not blocked from doing so
 8: try to check out a second BK2 item to the same patron, get message "Too many checked out. 1 checked out, only 1 are allowed."
 9: try to check out a second BK item to the same patrons, get message "Too many checked out. 2 checked out, only 1 are allowed."
10: return both items
11: check a BK2 item out to your patron
12: try to check a BK item out to your patron, get message "Too many checked out. 1 checked out, only 1 are allowed."
13: Apply patch
14: Repeat 7, you are now blocked
15: Other results should be the same

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Michael Adamyk <madamyk@ckls.org>
Comment 6 Victor Grousset/tuxayo 2024-02-06 23:17:08 UTC
> 15: Other results should be the same

9 isn't the same. And it shouldn't be the same. Because 7 failed, it's not "2 checked out" but "1 checked out". So what happens makes sense.

Thanks for the test plan with all those combination. It's great! :)
Comment 7 Victor Grousset/tuxayo 2024-02-07 01:18:11 UTC
> #Patron has 1 checkout of parent itemtype {{{{ child itype1

What does {{{{ means?
My guess is that «{{{{ child itype1» isn't meant to be here. And it's just some mistyping in vim ^^

+#    my $circ_rule_object =
+#      Koha::CirculationRules->find( $parent_iq_rule->{id} );
+#    $circ_rule_object->rule_value(4)->store();

---

There is a probably forgotten comment:

+#    my $circ_rule_object =
+#      Koha::CirculationRules->find( $parent_iq_rule->{id} );
+#    $circ_rule_object->rule_value(4)->store();

Otherwise, all seems to work as it should and code looks good :)
Comment 8 Catrina Berka 2024-02-16 17:46:23 UTC
This bug is also affecting hold limits. For example, I have two child item types (Audio book and new books) under a parent Book. In my circulation rules, I set Holds allowed (total) for audio books and new books to 1 each and then set my Book rule to 4. I place a hold on an audio book. It successfully prevents me from placing a second hold. I do the same with new books - first hold successfully placed, second prevented. When I attempt to place a first hold on a Book,  I get the error Cannot place hold - Too many holds: can only place a maximum of 2 total holds. Koha is only adding the holds allowed from the children item types, not considering the parent or default limits.
Comment 9 Nick Clemens 2024-02-16 18:06:40 UTC
(In reply to Catrina Berka from comment #8)
> This bug is also affecting hold limits. For example, I have two child item
> types (Audio book and new books) under a parent Book. In my circulation
> rules, I set Holds allowed (total) for audio books and new books to 1 each
> and then set my Book rule to 4. I place a hold on an audio book. It
> successfully prevents me from placing a second hold. I do the same with new
> books - first hold successfully placed, second prevented. When I attempt to
> place a first hold on a Book,  I get the error Cannot place hold - Too many
> holds: can only place a maximum of 2 total holds. Koha is only adding the
> holds allowed from the children item types, not considering the parent or
> default limits.

Parent/child types don't currently affect holds - see bug 35704