Bug 25783

Summary: Holds Queue treating item-level holds as bib-level
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: critical    
Priority: P5 - low CC: aleisha, gmcharlt, jonathan.druart, kyle.m.hall, sbcornell, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25786
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00 20.05.01, 19.11.10
Bug Depends on:    
Bug Blocks: 25786    
Attachments: Bug 25783: Holds Queue treating item-level holds as bib-level
Bug 25783: Holds Queue treating item-level holds as bib-level
Bug 25783: Holds Queue treating item-level holds as bib-level
Bug 25783: Regression tests
Bug 25783: Holds Queue treating item-level holds as bib-level
Bug 25783: Regression tests
Bug 25783: Holds Queue treating item-level holds as bib-level
Bug 25783: Modify calls to AddReserve to match Koha 19.11.x AddReserve prototype

Description Andrew Fuerste-Henry 2020-06-17 13:27:59 UTC
We've only seen this on 19.11 and have not been able to recreate it on a test system, but we're seeing instances where a patron has an item-level hold but the holds queue treats it as a bib-level hold.
Comment 1 Kyle M Hall 2020-06-17 14:17:39 UTC
Created attachment 105973 [details] [review]
Bug 25783: Holds Queue treating item-level holds as bib-level

I discovered two issues. First, the holds queue builder does not honor
the new item_level_hold flag. Instead, it only item_level_request if
in the loop dealing with item level holds. This is incorrect. Item level
holds may be trapped in the local holds priority loop as well. It's
trivial to just pass though the correct item/biblio level hold flag.

In addition, in bug 23934 I removed the line of code that prevented
local holds priority from working for item level holds. This change
itself was good and correct. However, the code in that loop does not
check to ensure the item matches the requested item! That means it can
match an item level hold with the wrong item!

I do not know how to write a reproducable test plan for these issues.
Comment 2 Kyle M Hall 2020-06-17 15:18:05 UTC
(In reply to Kyle M Hall from comment #1)
> Created attachment 105973 [details] [review] [review]
> Bug 25783: Holds Queue treating item-level holds as bib-level
> 
> I discovered two issues. First, the holds queue builder does not honor
> the new item_level_hold flag. Instead, it only item_level_request if
> in the loop dealing with item level holds. This is incorrect. Item level
> holds may be trapped in the local holds priority loop as well. It's
> trivial to just pass though the correct item/biblio level hold flag.
> 
> In addition, in bug 23934 I removed the line of code that prevented
> local holds priority from working for item level holds. This change
> itself was good and correct. However, the code in that loop does not
> check to ensure the item matches the requested item! That means it can
> match an item level hold with the wrong item!
> 
> I do not know how to write a reproducable test plan for these issues.

I'm going to split the second issue out into it's own bug.
Comment 3 Kyle M Hall 2020-06-17 15:42:07 UTC
Created attachment 105974 [details] [review]
Bug 25783: Holds Queue treating item-level holds as bib-level

The holds queue builder does not honor
the new item_level_hold flag. Instead, it only item_level_request if
in the loop dealing with item level holds. This is incorrect. Item level
holds may be trapped in the local holds priority loop as well. It's
trivial to just pass though the correct item/biblio level hold flag.

I do not know how to write a reproducable test plan for these issues.
Comment 4 Kyle M Hall 2020-06-17 18:43:04 UTC
Created attachment 105982 [details] [review]
Bug 25783: Holds Queue treating item-level holds as bib-level

The holds queue builder does not honor
the new item_level_hold flag. Instead, it only item_level_request if
in the loop dealing with item level holds. This is incorrect. Item level
holds may be trapped in the local holds priority loop as well. It's
trivial to just pass though the correct item/biblio level hold flag.

I do not know how to write a reproducable test plan for these issues.
Comment 5 Tomás Cohen Arazi 2020-06-17 20:23:15 UTC
Created attachment 105983 [details] [review]
Bug 25783: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2020-06-17 20:23:22 UTC
Created attachment 105984 [details] [review]
Bug 25783: Holds Queue treating item-level holds as bib-level

The holds queue builder does not honor
the new item_level_hold flag. Instead, it only item_level_request if
in the loop dealing with item level holds. This is incorrect. Item level
holds may be trapped in the local holds priority loop as well. It's
trivial to just pass though the correct item/biblio level hold flag.

I do not know how to write a reproducable test plan for these issues.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2020-06-17 20:28:23 UTC
I wrote regression tests on top of master and the problem is there. Added my SO as  the patch clearly solves the issue. But I'm not sure why we don't reproduce it in master through the UI.
Comment 8 Andrew Fuerste-Henry 2020-06-18 01:41:30 UTC
Created attachment 105992 [details] [review]
Bug 25783: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kim Peine <kim@williston.lib.vt.us>
Comment 9 Andrew Fuerste-Henry 2020-06-18 01:41:33 UTC
Created attachment 105993 [details] [review]
Bug 25783: Holds Queue treating item-level holds as bib-level

The holds queue builder does not honor
the new item_level_hold flag. Instead, it only item_level_request if
in the loop dealing with item level holds. This is incorrect. Item level
holds may be trapped in the local holds priority loop as well. It's
trivial to just pass though the correct item/biblio level hold flag.

I do not know how to write a reproducable test plan for these issues.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kim Peine <kim@williston.lib.vt.us>
Comment 10 Jonathan Druart 2020-06-18 15:00:04 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Jonathan Druart 2020-06-18 15:09:33 UTC
backported to 20.05.x for 20.05.01
Comment 12 Aleisha Amohia 2020-06-22 04:50:02 UTC
tests do not pass on 19.11.x

$ prove t/db_dependent/HoldsQueue.t 
t/db_dependent/HoldsQueue.t .. 49/52     # No tests run!

#   Failed test 'No tests run for subtest "Item level holds info is preserved (Bug 25738)"'
#   at t/db_dependent/HoldsQueue.t line 1086.
DBIx::Class::Row::store_column(): No such column 'itemnumber' on Koha::Schema::Result::Branch at /home/vagrant/kohaclone/Koha/Object.pm line 77
# Looks like your test exited with 255 just after 51.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/52 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 65280 Tests: 51 Failed: 1)
  Failed test:  51
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 52 tests but ran 51.
Files=1, Tests=51,  6 wallclock secs ( 0.01 usr  0.03 sys +  4.32 cusr  1.08 csys =  5.44 CPU)
Result: FAIL

please rebase!

also, is the name of the subtest incorrect? should it be "Item level holds info is preserved (Bug 25783)" instead? (fixed bug number)
Comment 13 Kyle M Hall 2020-06-22 13:26:33 UTC
Created attachment 106166 [details] [review]
Bug 25783: Modify calls to AddReserve to match Koha 19.11.x AddReserve prototype
Comment 14 Aleisha Amohia 2020-09-07 01:58:59 UTC
backported to 19.11.x for 19.11.10
Comment 15 Victor Grousset/tuxayo 2020-09-20 15:33:45 UTC
Can't backport to 19.05. Test failing. Not sure if it's just a column name that changed or if it's more complex than that. 

kohadev-koha@c57830f10a86:/kohadevbox/koha$ prove t/db_dependent/HoldsQueue.t
t/db_dependent/HoldsQueue.t .. 1/49 DBD::mysql::st execute failed: Unknown column 'item_level_hold' in 'field list' [for Statement "SELECT biblionumber, borrowernumber, itemnumber, priority, reserves.branchcode,
                                reservedate, reservenotes, borrowers.branchcode AS borrowerbranch, itemtype, item_level_hold
                         FROM reserves
                         JOIN borrowers USING (borrowernumber)
                         WHERE biblionumber = ?
                         AND found IS NULL
                         AND priority > 0
                         AND reservedate <= CURRENT_DATE()
                         AND suspend = 0
                         ORDER BY priority" with ParamValues: 0=440] at /kohadevbox/koha/C4/HoldsQueue.pm line 292.
Can't use an undefined value as an ARRAY reference at /usr/lib/x86_64-linux-gnu/perl5/5.24/DBI.pm line 2088.
# Looks like your test exited with 11 just after 2.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 47/49 subtests