Bug 35357 - Item not removed from holds queue when checked out to a different patron
Summary: Item not removed from holds queue when checked out to a different patron
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-11-17 03:31 UTC by Nick Clemens (kidclamp)
Modified: 2024-04-03 16:53 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04


Attachments
Bug 35357: Add unit test (2.42 KB, patch)
2024-01-11 14:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35357: Remove item from holds queue when it is checked out (1.32 KB, patch)
2024-01-11 14:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35357: Add unit test (2.47 KB, patch)
2024-01-13 14:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35357: Remove item from holds queue when it is checked out (1.36 KB, patch)
2024-01-13 14:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35357: Add unit test (2.47 KB, patch)
2024-02-06 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35357: Remove item from holds queue when it is checked out (2.81 KB, patch)
2024-02-06 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35357: Enhance unit tests (2.23 KB, patch)
2024-02-06 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35357: Enhance unit tests (2.37 KB, patch)
2024-02-06 17:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35357: Add unit test (2.52 KB, patch)
2024-02-07 21:21 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35357: Remove item from holds queue when it is checked out (2.86 KB, patch)
2024-02-07 21:21 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35357: Enhance unit tests (2.42 KB, patch)
2024-02-07 21:21 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-11-17 03:31:23 UTC
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
Comment 1 Kyle M Hall 2024-01-11 14:36:48 UTC
Created attachment 160851 [details] [review]
Bug 35357: Add unit test
Comment 2 Kyle M Hall 2024-01-11 14:37:12 UTC
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!
Comment 3 Owen Leonard 2024-01-13 14:24:36 UTC
Created attachment 161009 [details] [review]
Bug 35357: Add unit test

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Owen Leonard 2024-01-13 14:24:38 UTC
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>
Comment 5 Victor Grousset/tuxayo 2024-02-05 11:52:34 UTC
> 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.
Comment 6 Victor Grousset/tuxayo 2024-02-05 12:21:23 UTC
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)
Comment 7 Katrin Fischer 2024-02-06 12:43:17 UTC
(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.
Comment 8 Kyle M Hall 2024-02-06 16:52:44 UTC
(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!
Comment 9 Kyle M Hall 2024-02-06 17:12:32 UTC
Created attachment 161789 [details] [review]
Bug 35357: Add unit test

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Kyle M Hall 2024-02-06 17:12:40 UTC
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>
Comment 11 Kyle M Hall 2024-02-06 17:12:42 UTC
Created attachment 161791 [details] [review]
Bug 35357: Enhance unit tests
Comment 12 Kyle M Hall 2024-02-06 17:13:43 UTC
(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.
Comment 13 Kyle M Hall 2024-02-06 17:15:24 UTC
Created attachment 161792 [details] [review]
Bug 35357: Enhance unit tests
Comment 14 Victor Grousset/tuxayo 2024-02-07 02:02:05 UTC
(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.
Comment 15 Victor Grousset/tuxayo 2024-02-07 21:21:46 UTC
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>
Comment 16 Victor Grousset/tuxayo 2024-02-07 21:21:49 UTC
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>
Comment 17 Victor Grousset/tuxayo 2024-02-07 21:21:52 UTC
Created attachment 161851 [details] [review]
Bug 35357: Enhance unit tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Victor Grousset/tuxayo 2024-02-07 22:14:12 UTC
(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 :)
Comment 19 Katrin Fischer 2024-03-07 16:28:25 UTC
Just wondering about the direct SQL in this patch. 
We do have HoldsQueueItems.pm - should we not be able to use a delete?
Comment 20 Katrin Fischer 2024-03-07 16:37:12 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 21 Victor Grousset/tuxayo 2024-03-07 21:14:17 UTC
(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.
Comment 22 Kyle M Hall 2024-03-08 11:46:36 UTC
(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.
Comment 23 Katrin Fischer 2024-03-08 16:38:03 UTC
(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.
Comment 24 Fridolin Somers 2024-03-11 10:27:16 UTC
Seeing comments, I wait for backport
Comment 25 Victor Grousset/tuxayo 2024-03-11 20:21:31 UTC
(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.
Comment 26 Fridolin Somers 2024-03-12 10:41:04 UTC
Pushed to 23.11.x for 23.11.04
Comment 27 Kyle M Hall 2024-03-12 15:31:39 UTC
> (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!
Comment 28 Katrin Fischer 2024-03-12 20:53:02 UTC
Thanks for explaining :)
Comment 29 Lucas Gass 2024-03-19 20:53:08 UTC
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.
Comment 30 Andrii Nugged 2024-03-22 17:21:48 UTC
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"?)
Comment 31 Victor Grousset/tuxayo 2024-03-23 04:20:10 UTC
@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
Comment 32 Andrii Nugged 2024-03-27 13:28:52 UTC
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"?)
Comment 33 Victor Grousset/tuxayo 2024-03-28 18:30:07 UTC
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
Comment 34 Andrii Nugged 2024-04-03 16:53:42 UTC
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.