Bug 14514

Summary: LocalHoldsPriority and the HoldsQueue conflict with each other
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: aleisha, barton, brendan, cbrannon, frederic, gmcharlt, gwilliams, jsasse, lisettepalouse+koha, liz, martin.renvoize, srdjan
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16671    
Bug Blocks:    
Attachments: Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other
Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other
Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other
Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other
[SIGNED-OFF] Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other
Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other
[PASSED QA] Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other

Description Kyle M Hall 2015-07-09 18:32:37 UTC
It appears that the LocalHoldsPriority feature and the Holds Queue are fundamentally at odds with each other.

The problem appears to be that both are attempting to choose the best way to fill holds. When you are using the holds queue and you check in an item that has been selected by the holds queue builder, that part of Koha where the LocalHoldsPriority feature lives doesn't get to see all the holds in order to pick the best one. Instead only the hold selected by the holds queue builder is returned so to the LocalHoldsPriority feature, that is only one hold to pick from!

The solution *may* be to add the similar logic that appears C4::Reserves::CheckReserves for handling local hold priority to C4::HoldsQueue::MapItemsToHoldRequests at the very beginning before it even attempts to match item level requests.
Comment 1 Kyle M Hall 2016-02-04 19:45:07 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-02-04 20:17:03 UTC Comment hidden (obsolete)
Comment 3 Aleisha Amohia 2016-02-05 00:16:55 UTC
t/db_dependent/HoldsQueue.t .. 1/27 
#   Failed test 'Holds queue giving priority to patron who's home library matches item's home library'
#   at t/db_dependent/HoldsQueue.t line 333.
#          got: 'cdoBnBl'
#     expected: 'pkSI5p'

#   Failed test 'Holds queue giving priority to patron who's home library matches item's holding library'
#   at t/db_dependent/HoldsQueue.t line 350.
#          got: 'cdoBnBl'
#     expected: 'pkSI5p'

#   Failed test 'Holds queue giving priority to patron who's home library matches item's holding library'
#   at t/db_dependent/HoldsQueue.t line 367.
#          got: 'cdoBnBl'
#     expected: 'pkSI5p'

#   Failed test 'Holds queue giving priority to patron who's home library matches item's holding library'
#   at t/db_dependent/HoldsQueue.t line 384.
#          got: 'cdoBnBl'
#     expected: 'pkSI5p'

#   Failed test 'Bug 15062 - Holds queue with Transport Cost Matrix will transfer item even if transfers disabled'
#   at t/db_dependent/HoldsQueue.t line 492.
#          got: '1'
#     expected: '0'
# Looks like you failed 5 tests of 27.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 5 (wstat 1280, 0x500)
Failed 5/27 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 1280 Tests: 27 Failed: 5)
  Failed tests:  22-25, 27
  Non-zero exit status: 5
Files=1, Tests=27,  1 wallclock secs ( 0.03 usr  0.01 sys +  1.13 cusr  0.09 csys =  1.26 CPU)
Result: FAIL
Comment 4 Kyle M Hall 2016-02-05 16:29:37 UTC
(In reply to Aleisha Amohia from comment #3)
Aleisha, all the tests pass on my test server. Can you try running the tests on a clean installation of Koha with just the default data installed? If they pass for you that way, it means I need to fix my db assumptions for my unit tests.

Thanks!
Comment 5 Srdjan Jankovic 2016-03-11 01:39:15 UTC
Applying: Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other
Using index info to reconstruct a base tree...
M       C4/HoldsQueue.pm
M       t/db_dependent/HoldsQueue.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/HoldsQueue.t
CONFLICT (content): Merge conflict in t/db_dependent/HoldsQueue.t
Auto-merging C4/HoldsQueue.pm
Failed to merge in the changes.
Comment 6 Kyle M Hall 2016-03-14 12:08:57 UTC Comment hidden (obsolete)
Comment 7 Christopher Brannon 2016-03-24 21:52:41 UTC
*** Bug 15914 has been marked as a duplicate of this bug. ***
Comment 8 Barton Chittenden 2016-05-13 20:44:59 UTC
Patch does not apply.
Comment 9 Kyle M Hall 2016-05-18 15:49:37 UTC Comment hidden (obsolete)
Comment 10 Barton Chittenden 2016-05-18 20:36:24 UTC
t/db_dependent/HoldsQueue.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 11/42 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 2816 Tests: 42 Failed: 11)
  Failed tests:  22, 24-28, 32-34, 36, 40
  Non-zero exit status: 11
Files=1, Tests=42,  3 wallclock secs ( 0.03 usr  0.00 sys +  2.02 cusr  0.12 csys =  2.17 CPU)
Result: FAIL
Comment 11 Kyle M Hall 2016-06-13 18:33:32 UTC
All the unit tests pass for me. Are you running them on a stock sample database?
Comment 12 Barton Chittenden 2016-06-15 16:28:42 UTC
Re-run under kohadevbox; all tests pass.
Comment 13 Kyle M Hall 2016-06-16 17:25:38 UTC
Test Plan:
1) Create a number of holds on a record such that the item will be trapped for the last patron on the queue
2) Check in the item, note the hold is trapped for the priority patron
3) Run the holds queue builder
4) Check in the item, note the hold is now trapped for someone else
5) Apply this patch
6) Rerun the holds queue builder
7) Check in the item, note the hold is now trapped for the priority patron again
Comment 14 Liz Rea 2016-07-22 04:20:52 UTC
Should we expect that this local hold that has priority for a branch turn up on the hold queue? Or do we only care about what happens when an item is checked in?

My instinct says that a local hold priority item should show up on the hold queue for the prioritised library/item/borrower

Example, Patron from springfield has a biblio level reserve for a book that has a springfield copy, the springfield copy is available, should this turn up on the hold queue for springfield? 

Currently the reserve doesn't show up on any hold queue. Is that correct behaviour? How would the library know the reserve exists in this case without scanning it to trap it?
Comment 15 Liz Rea 2016-07-22 04:23:32 UTC
*with this patch, the reserve doesn't show up on any hold queue. Without it, as you noted, it does show up on the wrong priority library's hold queue.
Comment 16 Barton Chittenden 2016-07-28 16:04:45 UTC
Created attachment 53787 [details] [review]
[SIGNED-OFF] Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other

It appears that the LocalHoldsPriority feature and the Holds Queue are
fundamentally at odds with each other.

The problem appears to be that both are attempting to choose the best
way to fill holds. When you are using the holds queue and you check in
an item that has been selected by the holds queue builder, that part of
Koha where the LocalHoldsPriority feature lives doesn't get to see all
the holds in order to pick the best one. Instead only the hold selected
by the holds queue builder is returned so to the LocalHoldsPriority
feature, that is only one hold to pick from!

Test Plan:
1) Apply this patch
2) prove t/db_dependent/HoldsQueue.t
3) All tests should pass

Signed-off-by: Barton Chittenden barton@bywatersolutions.com
Comment 17 Kyle M Hall 2016-07-28 17:41:26 UTC
(In reply to Liz Rea from comment #14)
> Should we expect that this local hold that has priority for a branch turn up
> on the hold queue? Or do we only care about what happens when an item is
> checked in?
> 
> My instinct says that a local hold priority item should show up on the hold
> queue for the prioritised library/item/borrower
> 
> Example, Patron from springfield has a biblio level reserve for a book that
> has a springfield copy, the springfield copy is available, should this turn
> up on the hold queue for springfield? 
> 
> Currently the reserve doesn't show up on any hold queue. Is that correct
> behaviour? How would the library know the reserve exists in this case
> without scanning it to trap it?

Yes, the hold should show up in the holds queue. That's all this patch is supposed to do is to select the same patron for the holds queue that would be selected if the item in question was not already in the holds queue. The problem right now is the holds queue is choosing the wrong patron.
Comment 18 Kyle M Hall 2016-07-28 17:42:37 UTC
(In reply to Liz Rea from comment #15)
> *with this patch, the reserve doesn't show up on any hold queue. Without it,
> as you noted, it does show up on the wrong priority library's hold queue.

Do you have Local Holds Priority enabled? Can you try again with it switched both on and off?
Comment 19 Joel Sasse 2016-08-26 17:57:26 UTC
Liz,

Would you please respond to Kyle's questions in the previous comment? I am anxious to test this patch, sign off on, and get it into Koha as soon as possible.

Thanks,
Joel
Comment 20 Liz Rea 2016-09-09 03:29:09 UTC
Yes, I have the syspref turned on. I have just tested this again with the same results: 

The book is captured for the correct patron on check in, it does not turn up on a hold queue for any library. This seems wrong.
Comment 21 Kyle M Hall 2016-09-09 13:45:58 UTC
(In reply to Liz Rea from comment #20)
> Yes, I have the syspref turned on. I have just tested this again with the
> same results: 
> 
> The book is captured for the correct patron on check in, it does not turn up
> on a hold queue for any library. This seems wrong.

Liz can you make a screencast? I don't see how this is even possible!

For what it's worth, here is a video demo I just made: https://www.youtube.com/watch?v=d947TgBENwM
Comment 22 Danielle Elder 2016-10-11 07:51:00 UTC
Created attachment 56126 [details] [review]
Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other

It appears that the LocalHoldsPriority feature and the Holds Queue are
fundamentally at odds with each other.

The problem appears to be that both are attempting to choose the best
way to fill holds. When you are using the holds queue and you check in
an item that has been selected by the holds queue builder, that part of
Koha where the LocalHoldsPriority feature lives doesn't get to see all
the holds in order to pick the best one. Instead only the hold selected
by the holds queue builder is returned so to the LocalHoldsPriority
feature, that is only one hold to pick from!

Test Plan:
1) Apply this patch
2) prove t/db_dependent/HoldsQueue.t
3) All tests should pass

Signed-off-by: Barton Chittenden barton@bywatersolutions.com
Signed-off-by: Dani Elder <dani@bywatersolutions.com>
Comment 23 Martin Renvoize 2016-10-14 07:53:17 UTC
Created attachment 56456 [details] [review]
[PASSED QA] Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other

It appears that the LocalHoldsPriority feature and the Holds Queue are
fundamentally at odds with each other.

The problem appears to be that both are attempting to choose the best
way to fill holds. When you are using the holds queue and you check in
an item that has been selected by the holds queue builder, that part of
Koha where the LocalHoldsPriority feature lives doesn't get to see all
the holds in order to pick the best one. Instead only the hold selected
by the holds queue builder is returned so to the LocalHoldsPriority
feature, that is only one hold to pick from!

Test Plan:
1) Apply this patch
2) prove t/db_dependent/HoldsQueue.t
3) All tests should pass

Signed-off-by: Barton Chittenden barton@bywatersolutions.com
Signed-off-by: Dani Elder <dani@bywatersolutions.com>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2016-10-14 07:54:31 UTC
Code looks good, fixes the problem for me and I cannot replicate Liz's issue. Passing QA
Comment 25 Brendan Gallagher 2016-10-14 09:01:24 UTC
Pushed to Master - Should be in the November 16.11 Release.  Thanks!
Comment 26 Frédéric Demians 2016-10-22 12:24:31 UTC
Pushed in 16.05. Will be in 16.05.05.