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: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
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
Modified: 2024-02-07 22:14 UTC (History)
5 users (show)

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


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 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 :)