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.
Created attachment 47654 [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
Created attachment 47655 [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
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
(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!
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.
Created attachment 49089 [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
*** Bug 15914 has been marked as a duplicate of this bug. ***
Patch does not apply.
Created attachment 51595 [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
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
All the unit tests pass for me. Are you running them on a stock sample database?
Re-run under kohadevbox; all tests pass.
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
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?
*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.
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
(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.
(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?
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
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.
(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
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>
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>
Code looks good, fixes the problem for me and I cannot replicate Liz's issue. Passing QA
Pushed to Master - Should be in the November 16.11 Release. Thanks!
Pushed in 16.05. Will be in 16.05.05.