Bug 27718

Summary: Holds to Pull list doesn't respect holdallowed circulation rule anymore
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, nugged
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on: 13640, 24488, 27131    
Bug Blocks:    
Attachments: Bug 27718: Add tests
Bug 27718: (bug 24488 follow-up) Hide items not holdable
Bug 27718: Add tests
Bug 27718: (bug 24488 follow-up) Hide items not holdable
Bug 27718: Add tests
Bug 27718: (bug 24488 follow-up) Hide items not holdable
Bug 27718: Add tests
Bug 27718: (bug 24488 follow-up) Hide items not holdable
Bug 27718: (follow-up) add missing return
Bug 27718: Add tests
Bug 27718: (bug 24488 follow-up) Hide items not holdable
Bug 27718: (follow-up) add missing return

Description Joonas Kylmälä 2021-02-17 13:44:14 UTC
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.
Comment 1 Jonathan Druart 2021-02-17 15:13:56 UTC
Created attachment 116940 [details] [review]
Bug 27718: Add tests
Comment 2 Jonathan Druart 2021-02-17 15:13:59 UTC
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
Comment 3 Joonas Kylmälä 2021-02-23 11:20:58 UTC
The tests patch doesn't apply anymore.
Comment 4 Jonathan Druart 2021-02-23 12:04:10 UTC
Created attachment 117198 [details] [review]
Bug 27718: Add tests
Comment 5 Jonathan Druart 2021-02-23 12:04:14 UTC
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
Comment 6 Andrii Nugged 2021-02-24 04:03:21 UTC
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 :) ...
Comment 7 Andrii Nugged 2021-02-24 04:18:28 UTC Comment hidden (obsolete)
Comment 8 Andrii Nugged 2021-02-24 04:18:31 UTC Comment hidden (obsolete)
Comment 9 Andrii Nugged 2021-02-24 04:21:06 UTC
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} )

?
Comment 10 Joonas Kylmälä 2021-02-24 07:48:54 UTC
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.
Comment 11 Andrii Nugged 2021-02-24 09:34:20 UTC Comment hidden (obsolete)
Comment 12 Andrii Nugged 2021-02-24 09:49:56 UTC
Created attachment 117241 [details] [review]
Bug 27718: Add tests

Signed-off-by: Andrew Nugged <nugged@gmail.com>
Comment 13 Andrii Nugged 2021-02-24 09:49:59 UTC
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>
Comment 14 Andrii Nugged 2021-02-24 09:50:02 UTC
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>
Comment 15 Joonas Kylmälä 2021-02-25 08:56:45 UTC
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>
Comment 16 Joonas Kylmälä 2021-02-25 08:56:48 UTC
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>
Comment 17 Joonas Kylmälä 2021-02-25 08:56:51 UTC
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>
Comment 18 Joonas Kylmälä 2021-02-25 08:57:39 UTC
Fix works as advertised and no issues with code spotted. Passing QA.
Comment 19 Jonathan Druart 2021-03-01 10:20:17 UTC
Thanks Andrew for the follow-up!
Comment 20 Jonathan Druart 2021-03-01 10:29:09 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 21 Fridolin Somers 2021-03-04 09:31:50 UTC
Depends on Bug 24488 not in 20.11.x