After bug 24488 was merged Holds to Pull list (/cgi-bin/koha/circ/pendingreserves.pl) doesn't respect anymore the 'holdallowed' circulation rules (we bisected it to this change). If we look at the patch "Bug 24488: Show correct first patron details on Holds to pull" (2285c2d6575) then we can notice that previously the items that were shown in the list must had holdallowed != 0: > LEFT JOIN circulation_rules ON ( items.itype=circulation_rules.itemtype AND rule_name = 'holdallowed' AND circulation_rules.branchcode IS NULL AND circulation_rules.categorycode IS NULL ) And > AND ( circulation_rules.rule_value IS NULL OR circulation_rules.rule_value != 0 ) Now it seems like similar circ rule check is missing in the newly rewritten Holds To Pull implementation. I will continue investigating deeper this issue and update on progress.
Created attachment 116940 [details] [review] Bug 27718: Add tests
Created attachment 116941 [details] [review] Bug 27718: (bug 24488 follow-up) Hide items not holdable After bug 24488 was merged Holds to Pull list (/cgi-bin/koha/circ/pendingreserves.pl) doesn't respect anymore the 'holdallowed' circulation rules (we bisected it to this change). If we look at the patch "Bug 24488: Show correct first patron details on Holds to pull" (2285c2d6575) then we can notice that previously the items that were shown in the list must had holdallowed != 0: Now it seems like similar circ rule check is missing in the newly rewritten Holds To Pull implementation. (description copied from comment 0) Test plan: Have several pending holds Mark some item type as not holdable in the circulation rules Confirm that the non holdable items are no longer listed on the "Holds to pull" view
The tests patch doesn't apply anymore.
Created attachment 117198 [details] [review] Bug 27718: Add tests
Created attachment 117199 [details] [review] Bug 27718: (bug 24488 follow-up) Hide items not holdable After bug 24488 was merged Holds to Pull list (/cgi-bin/koha/circ/pendingreserves.pl) doesn't respect anymore the 'holdallowed' circulation rules (we bisected it to this change). If we look at the patch "Bug 24488: Show correct first patron details on Holds to pull" (2285c2d6575) then we can notice that previously the items that were shown in the list must had holdallowed != 0: Now it seems like similar circ rule check is missing in the newly rewritten Holds To Pull implementation. (description copied from comment 0) Test plan: Have several pending holds Mark some item type as not holdable in the circulation rules Confirm that the non holdable items are no longer listed on the "Holds to pull" view
circ/pendingreserves.pl 197: my $all_items; 198: foreach my $item ( $holds->get_items_that_can_fill ) { 199: push @{$all_items->{$item->biblionumber}}, $item; 200: } 201: Software error: The method Koha::Items->biblionumber is not covered by tests! Trace begun at .../Koha/Objects.pm line 592 Koha::Objects::AUTOLOAD('Koha::Items=HASH(0x55dd70618e38)') called at .../circ/pendingreserves.pl line 199 I just researched, it's because of missing return and then DBIXc doesn't sense "wantarray", I assume. - my $items = Koha::Items->search( + return Koha::Items->search( worked. Let me Sign-it-off patched :) ...
Created attachment 117234 [details] [review] Bug 27718: Add tests Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 117235 [details] [review] Bug 27718: (bug 24488 follow-up) Hide items not holdable After bug 24488 was merged Holds to Pull list (/cgi-bin/koha/circ/pendingreserves.pl) doesn't respect anymore the 'holdallowed' circulation rules (we bisected it to this change). If we look at the patch "Bug 24488: Show correct first patron details on Holds to pull" (2285c2d6575) then we can notice that previously the items that were shown in the list must had holdallowed != 0: Now it seems like similar circ rule check is missing in the newly rewritten Holds To Pull implementation. (description copied from comment 0) Test plan: Have several pending holds Mark some item type as not holdable in the circulation rules Confirm that the non holdable items are no longer listed on the "Holds to pull" view Signed-off-by: Andrew Nugged <nugged@gmail.com>
Jonathan QA Note by the way: this in tests: $items = $holds->get_items_that_can_fill; but this in the main code we have: foreach my $item ( $holds->get_items_that_can_fill ) is that was intended for the subroutine to sense "wantarray" (so it works in for), or we should just change that for to smth: foreach my $item ( @{$holds->get_items_that_can_fill} ) ?
Andrew, could you add a note before your sign-off that you modified the patch or alternatively send the fix as separate patch. That way the attribution is clear.
Re-doing... moment.
Created attachment 117241 [details] [review] Bug 27718: Add tests Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 117242 [details] [review] Bug 27718: (bug 24488 follow-up) Hide items not holdable After bug 24488 was merged Holds to Pull list (/cgi-bin/koha/circ/pendingreserves.pl) doesn't respect anymore the 'holdallowed' circulation rules (we bisected it to this change). If we look at the patch "Bug 24488: Show correct first patron details on Holds to pull" (2285c2d6575) then we can notice that previously the items that were shown in the list must had holdallowed != 0: Now it seems like similar circ rule check is missing in the newly rewritten Holds To Pull implementation. (description copied from comment 0) Test plan: Have several pending holds Mark some item type as not holdable in the circulation rules Confirm that the non holdable items are no longer listed on the "Holds to pull" view Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 117243 [details] [review] Bug 27718: (follow-up) add missing return Missing return doesn't allowed for "wantarray" state to DBIXc so sub returned scalar in any context. Also blank line to match with coding style removed. Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 117291 [details] [review] Bug 27718: Add tests Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117292 [details] [review] Bug 27718: (bug 24488 follow-up) Hide items not holdable After bug 24488 was merged Holds to Pull list (/cgi-bin/koha/circ/pendingreserves.pl) doesn't respect anymore the 'holdallowed' circulation rules (we bisected it to this change). If we look at the patch "Bug 24488: Show correct first patron details on Holds to pull" (2285c2d6575) then we can notice that previously the items that were shown in the list must had holdallowed != 0: Now it seems like similar circ rule check is missing in the newly rewritten Holds To Pull implementation. (description copied from comment 0) Test plan: Have several pending holds Mark some item type as not holdable in the circulation rules Confirm that the non holdable items are no longer listed on the "Holds to pull" view Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117293 [details] [review] Bug 27718: (follow-up) add missing return Missing return doesn't allowed for "wantarray" state to DBIXc so sub returned scalar in any context. Also blank line to match with coding style removed. Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Fix works as advertised and no issues with code spotted. Passing QA.
Thanks Andrew for the follow-up!
Pushed to master for 21.05, thanks to everybody involved!
Depends on Bug 24488 not in 20.11.x