Bug 27718 - Holds to Pull list doesn't respect holdallowed circulation rule anymore
Summary: Holds to Pull list doesn't respect holdallowed circulation rule anymore
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 24488 13640 27131
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-17 13:44 UTC by Joonas Kylmälä
Modified: 2021-03-04 09:31 UTC (History)
4 users (show)

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


Attachments
Bug 27718: Add tests (3.12 KB, patch)
2021-02-17 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27718: (bug 24488 follow-up) Hide items not holdable (1.96 KB, patch)
2021-02-17 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27718: Add tests (3.13 KB, patch)
2021-02-23 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27718: (bug 24488 follow-up) Hide items not holdable (1.96 KB, patch)
2021-02-23 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27718: Add tests (3.18 KB, patch)
2021-02-24 04:18 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 27718: (bug 24488 follow-up) Hide items not holdable (1.90 KB, patch)
2021-02-24 04:18 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 27718: Add tests (3.18 KB, patch)
2021-02-24 09:49 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 27718: (bug 24488 follow-up) Hide items not holdable (2.00 KB, patch)
2021-02-24 09:49 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 27718: (follow-up) add missing return (1.02 KB, patch)
2021-02-24 09:50 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 27718: Add tests (3.33 KB, patch)
2021-02-25 08:56 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27718: (bug 24488 follow-up) Hide items not holdable (2.15 KB, patch)
2021-02-25 08:56 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27718: (follow-up) add missing return (1.16 KB, patch)
2021-02-25 08:56 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 Andrew 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 Andrew Nugged 2021-02-24 04:18:28 UTC Comment hidden (obsolete)
Comment 8 Andrew Nugged 2021-02-24 04:18:31 UTC Comment hidden (obsolete)
Comment 9 Andrew 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 Andrew Nugged 2021-02-24 09:34:20 UTC Comment hidden (obsolete)
Comment 12 Andrew 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 Andrew 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 Andrew 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