[Bug 27718] New: Holds to Pull list doesn't respect holdallowed circulation rule anymore
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Bug ID: 27718 Summary: Holds to Pull list doesn't respect holdallowed circulation rule anymore Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: joonas.kylmala@helsinki.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, nugged@gmail.com Depends on: 24488 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24488 [Bug 24488] Holds to Pull sometimes shows the wrong 'first patron' details -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |13640 CC| |jonathan.druart@bugs.koha-c | |ommunity.org Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13640 [Bug 13640] Holds To Pull List includes items unreserveable items -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |jonathan.druart@bugs.koha-c |ity.org |ommunity.org -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 116940 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116940&action=edit Bug 27718: Add tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 116941 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116941&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |27131 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 [Bug 27131] Move code from circ/pendingreserves.pl to modules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Bug 27718 depends on bug 27131, which changed state. Bug 27131 Summary: Move code from circ/pendingreserves.pl to modules https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- The tests patch doesn't apply anymore. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116940|0 |1 is obsolete| | Attachment #116941|0 |1 is obsolete| | --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 117198 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117198&action=edit Bug 27718: Add tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 117199 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117199&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #6 from Andrew Nugged <nugged@gmail.com> --- 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 :) ... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117198|0 |1 is obsolete| | Attachment #117199|0 |1 is obsolete| | --- Comment #7 from Andrew Nugged <nugged@gmail.com> --- Created attachment 117234 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117234&action=edit Bug 27718: Add tests Signed-off-by: Andrew Nugged <nugged@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #8 from Andrew Nugged <nugged@gmail.com> --- Created attachment 117235 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117235&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #9 from 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} ) ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #11 from Andrew Nugged <nugged@gmail.com> --- Re-doing... moment. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117234|0 |1 is obsolete| | Attachment #117235|0 |1 is obsolete| | --- Comment #12 from Andrew Nugged <nugged@gmail.com> --- Created attachment 117241 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117241&action=edit Bug 27718: Add tests Signed-off-by: Andrew Nugged <nugged@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #13 from Andrew Nugged <nugged@gmail.com> --- Created attachment 117242 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117242&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #14 from Andrew Nugged <nugged@gmail.com> --- Created attachment 117243 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117243&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117241|0 |1 is obsolete| | Attachment #117242|0 |1 is obsolete| | Attachment #117243|0 |1 is obsolete| | --- Comment #15 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117291 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117291&action=edit Bug 27718: Add tests Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #16 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117292 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117292&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #17 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117293 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117293&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #18 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Fix works as advertised and no issues with code spotted. Passing QA. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Thanks Andrew for the follow-up! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27718 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED CC| |fridolin.somers@biblibre.co | |m --- Comment #21 from Fridolin Somers <fridolin.somers@biblibre.com> --- Depends on Bug 24488 not in 20.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org