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.
Created attachment 160877 [details] [review] Bug 30324: Unit tests
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
Created attachment 161030 [details] [review] Bug 30324: Unit tests Signed-off-by: David Nind <david@davidnind.com>
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>
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>
> 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! :)
> #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 :)
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.
(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
Created attachment 165663 [details] [review] Bug 30324: Unit tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 165664 [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>
Created attachment 165665 [details] [review] Bug 30324: (follow-up) Remove stray comments
Created attachment 165704 [details] [review] Bug 30324: Unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 165705 [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> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 165706 [details] [review] Bug 30324: (follow-up) Remove stray comments Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Works, makes sense, QA script happy, code looks good, passing QA :)
Always so happy about the accompanying unit tests, Nick. You are a star :)
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.06
Doesn't apply to 23.05.x, no backport.