When an item is selected for a hold via the holds queue, it is removed from the queue if checked out to the selected patron. If staff allow another patron to checkout the book, however, the item is not removed from the queue, and staff spend time trying to find an item that is not available. To recreate: 1 - Place a next available hold on a title 2 - Run the holds queue to generate an entry for this 3 - Go to Circulation->Holds queue 4 - Verify you see an item selected 5 - Copy the barcode 6 - Check the item out to a different patron 7 - Confirm the checkout 8 - Reload the holds queue, the item is still there
Created attachment 160851 [details] [review] Bug 35357: Add unit test
Created attachment 160852 [details] [review] Bug 35357: Remove item from holds queue when it is checked out Test Plan: 1) Place a hold on an item 2) Build the holds queue 3) Check out the item to a different patron than the one targeted in the holds queue 4) Verify the holds queue viewer still shows that item and patron 5) Apply this patch 6) Repeat stepts 1 through 3 7) Verify the holds queue viewer no longer shows that patron and item!
Created attachment 161009 [details] [review] Bug 35357: Add unit test Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 161010 [details] [review] Bug 35357: Remove item from holds queue when it is checked out Test Plan: 1) Place a hold on an item 2) Build the holds queue 3) Check out the item to a different patron than the one targeted in the holds queue 4) Verify the holds queue viewer still shows that item and patron 5) Apply this patch 6) Repeat stepts 1 through 3 7) Verify the holds queue viewer no longer shows that patron and item! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
> 7) Verify the holds queue viewer no longer shows that patron and item! The next run of misc/cronjobs/holds/build_holds_queue.pl brings back that patron and item in the Holds queue page. So back with the initial issue :( ---- QA script unhappy: WARN t/db_dependent/HoldsQueue.t WARN tidiness The file is less tidy than before (bad/messy lines before: 484, now: 488) ---- minor finding: The test uses a record level hold instead of an item level one. It works because there is one item for the record. Otherwise reproducing the bug would depend on which item is picked. IIUC it has to be the one suggested by the hold queue to get the issue. So maybe a comment is needed to say that the test tests the case where the checked out item is the one suggested by the hold queue. Or make it do an item level hold like in the test plan.
While testing, found this that is not due to this patch but related: When canceling a hold, it stays in the hold queue. Is that bad? It stays until the next queue rebuild (15min is said to be the usual delay)
(In reply to Victor Grousset/tuxayo from comment #6) > While testing, found this that is not due to this patch but related: > > When canceling a hold, it stays in the hold queue. Is that bad? It stays > until the next queue rebuild (15min is said to be the usual delay) Please keep in mind that build_holdsqueue is no longer the only use case. We have RTHoldsqueue without people running the script or only running it nightly now.
(In reply to Victor Grousset/tuxayo from comment #6) > While testing, found this that is not due to this patch but related: > > When canceling a hold, it stays in the hold queue. Is that bad? It stays > until the next queue rebuild (15min is said to be the usual delay) See Bug 35997 for the fix for that!
Created attachment 161789 [details] [review] Bug 35357: Add unit test Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 161790 [details] [review] Bug 35357: Remove item from holds queue when it is checked out Test Plan: 1) Place a hold on an item 2) Build the holds queue 3) Check out the item to a different patron than the one targeted in the holds queue 4) Verify the holds queue viewer still shows that item and patron 5) Apply this patch 6) Repeat stepts 1 through 3 7) Verify the holds queue viewer no longer shows that patron and item! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 161791 [details] [review] Bug 35357: Enhance unit tests
(In reply to Victor Grousset/tuxayo from comment #5) > > 7) Verify the holds queue viewer no longer shows that patron and item! > > The next run of misc/cronjobs/holds/build_holds_queue.pl brings back that > patron and item in the Holds queue page. So back with the initial issue :( I am unable to recreate that. If you can please submit a new bug report as that would be a different bug! > QA script unhappy: > WARN t/db_dependent/HoldsQueue.t > WARN tidiness > The file is less tidy than before (bad/messy lines before: 484, now: 488) Fixed! > minor finding: > The test uses a record level hold instead of an item level one. It works > because there is one item for the record. Otherwise reproducing the bug > would depend on which item is picked. IIUC it has to be the one suggested by > the hold queue to get the issue. > So maybe a comment is needed to say that the test tests the case where the > checked out item is the one suggested by the hold queue. Or make it do an > item level hold like in the test plan. That is correct, it is always the one suggested by the holds queue. I have enhanced the unit tests to prove this is the case.
Created attachment 161792 [details] [review] Bug 35357: Enhance unit tests
(In reply to Katrin Fischer from comment #7) > (In reply to Victor Grousset/tuxayo from comment #6) > > While testing, found this that is not due to this patch but related: > > > > When canceling a hold, it stays in the hold queue. Is that bad? It stays > > until the next queue rebuild (15min is said to be the usual delay) > > Please keep in mind that build_holdsqueue is no longer the only use case. We > have RTHoldsqueue without people running the script or only running it > nightly now. Indeed thanks for pointing this out. (confirmed that the RTHQ doesn't suffer from the bug here) --- Thanks Kyle for the followups, I'll looks at them asap and again at the issue at step 7.
Created attachment 161849 [details] [review] Bug 35357: Add unit test Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 161850 [details] [review] Bug 35357: Remove item from holds queue when it is checked out Test Plan: 1) Place a hold on an item 2) Build the holds queue 3) Check out the item to a different patron than the one targeted in the holds queue 4) Verify the holds queue viewer still shows that item and patron 5) Apply this patch 6) Repeat stepts 1 through 3 7) Verify the holds queue viewer no longer shows that patron and item! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 161851 [details] [review] Bug 35357: Enhance unit tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
(In reply to Kyle M Hall from comment #12) > (In reply to Victor Grousset/tuxayo from comment #5) > > > 7) Verify the holds queue viewer no longer shows that patron and item! > > > > The next run of misc/cronjobs/holds/build_holds_queue.pl brings back that > > patron and item in the Holds queue page. So back with the initial issue :( > > I am unable to recreate that. If you can please submit a new bug report as > that would be a different bug! I can't create either, my bad, I might have checked in the item and forgot when testing various stuff. So the reappearance in the queue would be expected there. Thanks for the followup that fixes the rest. Works, makes sense, QA script happy, code looks good, passing QA :)
Just wondering about the direct SQL in this patch. We do have HoldsQueueItems.pm - should we not be able to use a delete?
Pushed for 24.05! Well done everyone, thank you!
(In reply to Katrin Fischer from comment #19) > Just wondering about the direct SQL in this patch. > We do have HoldsQueueItems.pm - should we not be able to use a delete? Indeed it seems so , thanks for seeing that. @Kyle: up for a follow up patch? I can open the ticket.
(In reply to Victor Grousset/tuxayo from comment #21) > (In reply to Katrin Fischer from comment #19) > > Just wondering about the direct SQL in this patch. > > We do have HoldsQueueItems.pm - should we not be able to use a delete? > > Indeed it seems so , thanks for seeing that. > > @Kyle: up for a follow up patch? I can open the ticket. Using DBIC to find and delete the lines in the two tables will result in 4 queries compared to the one query we have now.
(In reply to Kyle M Hall from comment #22) > (In reply to Victor Grousset/tuxayo from comment #21) > > (In reply to Katrin Fischer from comment #19) > > > Just wondering about the direct SQL in this patch. > > > We do have HoldsQueueItems.pm - should we not be able to use a delete? > > > > Indeed it seems so , thanks for seeing that. > > > > @Kyle: up for a follow up patch? I can open the ticket. > > Using DBIC to find and delete the lines in the two tables will result in 4 > queries compared to the one query we have now. Hi Kyle, can you explain a bit more about those extra queries if you can? I'd like to understand the issue better and learn.
Seeing comments, I wait for backport
(In reply to Kyle M Hall from comment #22) > Using DBIC to find and delete the lines in the two tables will result in 4 > queries compared to the one query we have now. The manual query should still do in practice two lookups via itemnumber. And then two deletes. But maybe for simple queries the overhead of doing individual queries is larger than the work done by the simple queries themselves. I don't know enough about real world DBMS behavior. --- (In reply to Katrin Fischer from comment #23) > Hi Kyle, can you explain a bit more about those extra queries if you can? > I'd like to understand the issue better and learn. IIUC, we don't have the ORM objects on hand so we have to do a find first. Then do a delete on the object. So 4 queries. (maybe Kyle had something else in mind) --- (In reply to Fridolin Somers from comment #24) > Seeing comments, I wait for backport No worries, you can backport.
Pushed to 23.11.x for 23.11.04
> (In reply to Katrin Fischer from comment #23) > > Hi Kyle, can you explain a bit more about those extra queries if you can? > > I'd like to understand the issue better and learn. > > IIUC, we don't have the ORM objects on hand so we have to do a find first. > Then do a delete on the object. So 4 queries. (maybe Kyle had something else > in mind) This exactly what I meant!
Thanks for explaining :)
Tests fail on 23.05.x: # Failed test 'No tests run for subtest "Remove item from holds queue on checkout"' # at /kohadevbox/koha/t/db_dependent/HoldsQueue.t line 2160. Can't locate object method "search" via package "Koha::Hold::HoldsQueueItems" (perhaps you forgot to load "Koha::Hold::HoldsQueueItems"?) at /kohadevbox/koha/t/db_dependent/HoldsQueue.t line 2135. # Looks like your test exited with 11 just after 62.
I am trying to absorb this to my 23.11.x for reproduction. It dies 1757:1 error Syntax: Global symbol "$dbh" requires explicit package name (did you forget to declare "my $dbh"?)
@Andrii Where is the error? In C4/Circulation.pm in AddIssue() ? --- @Lucas HoldsQueueItems didn't exist back then. If someone wants this for 23.05.x they will need to get a backport that use the appropriate way at the time to access tmp_holdsqueue
owshi... sorry, forgot file name. Yes, C4/Circulation.pm 1757 yntax: Global symbol "$dbh" requires explicit package name (did you forget to declare "my $dbh"?)
This is weird, 23.11.x has $dbh declared at the beginning of the function. https://gitlab.com/koha-community/Koha/-/blob/23.11.x/C4/Circulation.pm#L1548
IGNORE MY REPORT please. (In reply to Victor Grousset/tuxayo from comment #33) > This is weird, 23.11.x has $dbh declared at the beginning of the function. > https://gitlab.com/koha-community/Koha/-/blob/23.11.x/C4/Circulation.pm#L1548 I double-checked, right, it is there. It was another codebase, where "dangling" $dbh-es (unused) were removed (and without code this $dbh was unused but declared here), now it's used, and all ok. so, whatever it is - it is all correct, false alamf from my side.