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.
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.
(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.
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.
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.
Created attachment 105983 [details] [review] Bug 25783: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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.
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>
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>
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.01
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)
Created attachment 106166 [details] [review] Bug 25783: Modify calls to AddReserve to match Koha 19.11.x AddReserve prototype
backported to 19.11.x for 19.11.10
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