Bug 34596

Summary: Items in transit should not show up in the holds queue
Product: Koha Reporter: Laura Escamilla <Laura.escamilla>
Component: CirculationAssignee: Nick Clemens (kidclamp) <nick>
Status: Passed QA --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: andrew, blawlor, bowens, carthur, chanel.wheeler, gmcharlt, karie.straube, kyle, lucas, madamyk, martin.renvoize, mspinney, mteal, nick, rcoert, sally.healey
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34678
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30581
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 35092    
Attachments: Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn
Bug 34596: Unit tests
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn
Bug 34596: Unit tests
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn
Bug 34596: Unit tests
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn
Bug 34596: Unit tests

Description Laura Escamilla 2023-08-23 11:59:38 UTC
Libraries have reported that items are still showing up in their holds queue hours after they have been marked as "in transit". Only holds that are ready to be found should be showing up in the holds queue.
Comment 1 Laura Escamilla 2023-09-06 16:43:48 UTC

*** This bug has been marked as a duplicate of bug 32572 ***
Comment 2 Nick Clemens (kidclamp) 2023-09-13 17:07:17 UTC
This seems to be a consequence of two real time holds queue updates being triggered - once during the initial check in, before the transit is confirmed, and once after the transit/hold has been confirmed.

Currently if the two jobs attempt to update at the same time there is a collision, see bug 34596

We should try to reduce the build here to a single call, after the transfer is confirmed or rejected if possible
Comment 3 Katrin Fischer 2023-10-29 22:51:51 UTC
Any update on this one?
Comment 4 Chanel Wheeler 2024-10-09 21:09:12 UTC
We really need this as a busy consortium.
Comment 5 Nick Clemens (kidclamp) 2024-11-19 20:07:10 UTC
Created attachment 174811 [details] [review]
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored

This patch does two things:
1:
It prevents rebulding the real time holds queue during a checkin if a reserve was found.
The reserve will either:
a - be set to waiting if at correct location and no confirmation - this is fine because we will rebuild the queue when
    filling the hold
b - trigger a popup whihc will rebuild the queue when confirming the transfer, or be ignored

2:
It adds a form to ignoring a hold so that we can rebuild the holds queue if we decided not to use the current item
to fill the found hold

To test:
0 - Enable RealTimeHoldsQueue
1 - Place several holds for different libraries on a title
2 - Checkout a copy to a patron at current library (ignore current holds)
3 - Clean up DB so we can see outcomes from next steps:
   delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';
4 - Check in item, confirm hold
5 - Check background jobs, note two rebuilds of the holds queue
6 - Check item out to previous patron, repeat 3
7 - Check in item, ignore the hold
8 - Check background jobs, note only one rebuild
9 - Apply patch, restart all
10 - Checkout item, repeat 3
11 - Checkin item, confirm hold
12 - Check background jobs, onlt one rebuild of holds queue
13 - Checkout item, repeat 3
14 - Checkin item, ignore hold
15 - Check background jobs, only one rebuild of holds queue
Comment 6 Nick Clemens (kidclamp) 2024-11-19 20:08:29 UTC
This should have a unit test, but needs testing and confirmation of approach from QA before these will be attempted
Comment 7 Brendan Lawlor 2024-12-19 19:41:46 UTC
Created attachment 175804 [details] [review]
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored

This patch does two things:
1:
It prevents rebulding the real time holds queue during a checkin if a reserve was found.
The reserve will either:
a - be set to waiting if at correct location and no confirmation - this is fine because we will rebuild the queue when
    filling the hold
b - trigger a popup whihc will rebuild the queue when confirming the transfer, or be ignored

2:
It adds a form to ignoring a hold so that we can rebuild the holds queue if we decided not to use the current item
to fill the found hold

To test:
0 - Enable RealTimeHoldsQueue
1 - Place several holds for different libraries on a title
2 - Checkout a copy to a patron at current library (ignore current holds)
3 - Clean up DB so we can see outcomes from next steps:
   delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';
4 - Check in item, confirm hold
5 - Check background jobs, note two rebuilds of the holds queue
6 - Check item out to previous patron, repeat 3
7 - Check in item, ignore the hold
8 - Check background jobs, note only one rebuild
9 - Apply patch, restart all
10 - Checkout item, repeat 3
11 - Checkin item, confirm hold
12 - Check background jobs, onlt one rebuild of holds queue
13 - Checkout item, repeat 3
14 - Checkin item, ignore hold
15 - Check background jobs, only one rebuild of holds queue

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 8 Brendan Lawlor 2024-12-19 19:49:03 UTC
Test notes:
For step 3 I used ktd --dbshell to clean up the database
From the > prompt run:
  delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';

Then for steps 5, 8, 12 and 15 to check background jobs run:
select count(*) from background_jobs;

and I verified the count of background_jobs is 1 after the patch
Comment 9 Martin Renvoize (ashimema) 2025-01-07 15:58:05 UTC
Fancy having a go that those unit tests Nick?
Comment 10 Nick Clemens (kidclamp) 2025-02-03 15:52:20 UTC
(In reply to Martin Renvoize (ashimema) from comment #9)
> Fancy having a go that those unit tests Nick?

I haven't written the tests because I wonder if we should go about this in a different way.

The current patches have AddReturn check the context of the call essentially and ignore the specific problematic case.

I think it might be better to remove the queue rebuild from AddReturn and call it explicitly where needed from the scripts that call AddReturn.

It will be more code, but should be more obvious. Thoughts?
Comment 11 Andrew Fuerste-Henry 2025-09-05 17:00:48 UTC
(In reply to Nick Clemens (kidclamp) from comment #10)
> (In reply to Martin Renvoize (ashimema) from comment #9)
> > Fancy having a go that those unit tests Nick?
> 
> I haven't written the tests because I wonder if we should go about this in a
> different way.
> 
> The current patches have AddReturn check the context of the call essentially
> and ignore the specific problematic case.
> 
> I think it might be better to remove the queue rebuild from AddReturn and
> call it explicitly where needed from the scripts that call AddReturn.
> 
> It will be more code, but should be more obvious. Thoughts?

We'd love to see this move forward. Martin, do you have thoughts on Nick's questions?
Comment 12 Nick Clemens (kidclamp) 2025-09-18 20:23:29 UTC
Created attachment 186592 [details] [review]
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored

This patch does two things:
1:
It prevents rebulding the real time holds queue during a checkin if a reserve was found.
The reserve will either:
a - be set to waiting if at correct location and no confirmation - this is fine because we will rebuild the queue when
    filling the hold
b - trigger a popup whihc will rebuild the queue when confirming the transfer, or be ignored

2:
It adds a form to ignoring a hold so that we can rebuild the holds queue if we decided not to use the current item
to fill the found hold

To test:
0 - Enable RealTimeHoldsQueue
1 - Place several holds for different libraries on a title
2 - Checkout a copy to a patron at current library (ignore current holds)
3 - Clean up DB so we can see outcomes from next steps:
   delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';
4 - Check in item, confirm hold
5 - Check background jobs, note two rebuilds of the holds queue
6 - Check item out to previous patron, repeat 3
7 - Check in item, ignore the hold
8 - Check background jobs, note only one rebuild
9 - Apply patch, restart all
10 - Checkout item, repeat 3
11 - Checkin item, confirm hold
12 - Check background jobs, onlt one rebuild of holds queue
13 - Checkout item, repeat 3
14 - Checkin item, ignore hold
15 - Check background jobs, only one rebuild of holds queue

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 13 Nick Clemens (kidclamp) 2025-09-18 20:23:32 UTC
Created attachment 186593 [details] [review]
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn
Comment 14 Nick Clemens (kidclamp) 2025-09-18 20:23:35 UTC
Created attachment 186594 [details] [review]
Bug 34596: Unit tests

Writing these I found that transferbook was generating two queue rebuilds - fixed that
Comment 15 Nick Clemens (kidclamp) 2025-09-18 20:25:20 UTC
Created attachment 186595 [details] [review]
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored

This patch does two things:
1:
It prevents rebulding the real time holds queue during a checkin if a reserve was found.
The reserve will either:
a - be set to waiting if at correct location and no confirmation - this is fine because we will rebuild the queue when
    filling the hold
b - trigger a popup whihc will rebuild the queue when confirming the transfer, or be ignored

2:
It adds a form to ignoring a hold so that we can rebuild the holds queue if we decided not to use the current item
to fill the found hold

To test:
0 - Enable RealTimeHoldsQueue
1 - Place several holds for different libraries on a title
2 - Checkout a copy to a patron at current library (ignore current holds)
3 - Clean up DB so we can see outcomes from next steps:
   delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';
4 - Check in item, confirm hold
5 - Check background jobs, note two rebuilds of the holds queue
6 - Check item out to previous patron, repeat 3
7 - Check in item, ignore the hold
8 - Check background jobs, note only one rebuild
9 - Apply patch, restart all
10 - Checkout item, repeat 3
11 - Checkin item, confirm hold
12 - Check background jobs, onlt one rebuild of holds queue
13 - Checkout item, repeat 3
14 - Checkin item, ignore hold
15 - Check background jobs, only one rebuild of holds queue

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 16 Nick Clemens (kidclamp) 2025-09-18 20:25:23 UTC
Created attachment 186596 [details] [review]
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn

Existing calls to AddReturn:

C4/Circulation.pm
 - transferbook - calls ModDateLastSeen, queue is rebuilt
 - AddIssue - rebuild not needed, book is going out again immediately

C4/SIP/ILS/Transaction/Checkin.pm
 - do_checkin - added

Koha/Checkouts.pm
 - automatic_checkin - added

circ/circulation.pl
 - called in stats checkout, not needed

circ/returns.pl
 - added

opac/sci/sci-main.pl
- added

svc/checkin
 - added

svc/recall
 - wasn't actually called, removed the use statement

tools/inventory.pl
 - added
Comment 17 Nick Clemens (kidclamp) 2025-09-18 20:25:26 UTC
Created attachment 186597 [details] [review]
Bug 34596: Unit tests

Writing these I found that transferbook was generating two queue rebuilds - fixed that
Comment 18 Martin Renvoize (ashimema) 2025-09-24 16:00:34 UTC
Created attachment 186897 [details] [review]
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored

This patch does two things:
1:
It prevents rebulding the real time holds queue during a checkin if a reserve was found.
The reserve will either:
a - be set to waiting if at correct location and no confirmation - this is fine because we will rebuild the queue when
    filling the hold
b - trigger a popup whihc will rebuild the queue when confirming the transfer, or be ignored

2:
It adds a form to ignoring a hold so that we can rebuild the holds queue if we decided not to use the current item
to fill the found hold

To test:
0 - Enable RealTimeHoldsQueue
1 - Place several holds for different libraries on a title
2 - Checkout a copy to a patron at current library (ignore current holds)
3 - Clean up DB so we can see outcomes from next steps:
   delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';
4 - Check in item, confirm hold
5 - Check background jobs, note two rebuilds of the holds queue
6 - Check item out to previous patron, repeat 3
7 - Check in item, ignore the hold
8 - Check background jobs, note only one rebuild
9 - Apply patch, restart all
10 - Checkout item, repeat 3
11 - Checkin item, confirm hold
12 - Check background jobs, onlt one rebuild of holds queue
13 - Checkout item, repeat 3
14 - Checkin item, ignore hold
15 - Check background jobs, only one rebuild of holds queue

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 19 Martin Renvoize (ashimema) 2025-09-24 16:00:36 UTC
Created attachment 186898 [details] [review]
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn

Existing calls to AddReturn:

C4/Circulation.pm
 - transferbook - calls ModDateLastSeen, queue is rebuilt
 - AddIssue - rebuild not needed, book is going out again immediately

C4/SIP/ILS/Transaction/Checkin.pm
 - do_checkin - added

Koha/Checkouts.pm
 - automatic_checkin - added

circ/circulation.pl
 - called in stats checkout, not needed

circ/returns.pl
 - added

opac/sci/sci-main.pl
- added

svc/checkin
 - added

svc/recall
 - wasn't actually called, removed the use statement

tools/inventory.pl
 - added

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 20 Martin Renvoize (ashimema) 2025-09-24 16:00:39 UTC
Created attachment 186899 [details] [review]
Bug 34596: Unit tests

Writing these I found that transferbook was generating two queue rebuilds - fixed that

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 21 Martin Renvoize (ashimema) 2025-09-24 16:01:22 UTC
ack.. transferbook.. I tried to get rid of that years ago :(

Anywho.. I'm happy with the work here.. took me a while to wrap my head around but I believe it's all OK

Passing QA
Comment 22 Nick Clemens (kidclamp) 2025-09-30 12:15:25 UTC
Created attachment 187090 [details] [review]
Bug 34596: Do not build holds queue when checkin reserve found but do if hold ignored

This patch does two things:
1:
It prevents rebulding the real time holds queue during a checkin if a reserve was found.
The reserve will either:
a - be set to waiting if at correct location and no confirmation - this is fine because we will rebuild the queue when
    filling the hold
b - trigger a popup whihc will rebuild the queue when confirming the transfer, or be ignored

2:
It adds a form to ignoring a hold so that we can rebuild the holds queue if we decided not to use the current item
to fill the found hold

To test:
0 - Enable RealTimeHoldsQueue
1 - Place several holds for different libraries on a title
2 - Checkout a copy to a patron at current library (ignore current holds)
3 - Clean up DB so we can see outcomes from next steps:
   delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';
4 - Check in item, confirm hold
5 - Check background jobs, note two rebuilds of the holds queue
6 - Check item out to previous patron, repeat 3
7 - Check in item, ignore the hold
8 - Check background jobs, note only one rebuild
9 - Apply patch, restart all
10 - Checkout item, repeat 3
11 - Checkin item, confirm hold
12 - Check background jobs, onlt one rebuild of holds queue
13 - Checkout item, repeat 3
14 - Checkin item, ignore hold
15 - Check background jobs, only one rebuild of holds queue

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 23 Nick Clemens (kidclamp) 2025-09-30 12:15:33 UTC
Created attachment 187091 [details] [review]
Bug 34596: Add rebuild queue where needed for existing calls to AddReturn

Existing calls to AddReturn:

C4/Circulation.pm
 - transferbook - calls ModDateLastSeen, queue is rebuilt
 - AddIssue - rebuild not needed, book is going out again immediately

C4/SIP/ILS/Transaction/Checkin.pm
 - do_checkin - added

Koha/Checkouts.pm
 - automatic_checkin - added

circ/circulation.pl
 - called in stats checkout, not needed

circ/returns.pl
 - added

opac/sci/sci-main.pl
- added

svc/checkin
 - added

svc/recall
 - wasn't actually called, removed the use statement

tools/inventory.pl
 - added

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 24 Nick Clemens (kidclamp) 2025-09-30 12:15:42 UTC
Created attachment 187092 [details] [review]
Bug 34596: Unit tests

Writing these I found that transferbook was generating two queue rebuilds - fixed that

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>