Bug 25783 - Holds Queue treating item-level holds as bib-level
Summary: Holds Queue treating item-level holds as bib-level
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 25786
  Show dependency treegraph
 
Reported: 2020-06-17 13:27 UTC by Andrew Fuerste-Henry
Modified: 2020-09-20 15:33 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
20.11.00 20.05.01, 19.11.10


Attachments
Bug 25783: Holds Queue treating item-level holds as bib-level (3.72 KB, patch)
2020-06-17 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25783: Holds Queue treating item-level holds as bib-level (3.38 KB, patch)
2020-06-17 15:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25783: Holds Queue treating item-level holds as bib-level (2.93 KB, patch)
2020-06-17 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25783: Regression tests (3.08 KB, patch)
2020-06-17 20:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25783: Holds Queue treating item-level holds as bib-level (2.97 KB, patch)
2020-06-17 20:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25783: Regression tests (3.14 KB, patch)
2020-06-18 01:41 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25783: Holds Queue treating item-level holds as bib-level (3.02 KB, patch)
2020-06-18 01:41 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25783: Modify calls to AddReserve to match Koha 19.11.x AddReserve prototype (1.54 KB, patch)
2020-06-22 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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