Bug 24359 - Remove items from Holds Queue when checked in
Summary: Remove items from Holds Queue when checked in
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Agustín Moyano
QA Contact: Kyle M Hall (khall)
URL:
Keywords:
Depends on: 27068
Blocks:
  Show dependency tree / graph
 
Reported: 2020-01-06 22:34 UTC by Joy Nelson
Modified: 2023-08-31 15:30 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development makes Koha trigger an update on the holds queue when items are checked in. This way, the holds queue will be updated faster than the default 1 hour frequency (cronjob). Note: this doesn't trigger the more expensive task of recalculating the whole queue, which remains a cronjob-based task.
Version(s) released in:
21.05.00,20.11.04
Circulation function:

Attachments
Bug 24359: Add test (2.80 KB, patch)
2020-10-28 14:05 UTC, Agustín Moyano 		Details | Diff | Splinter Review
Bug 24359: Remove hold from holds queue when captured (1.83 KB, patch)
2020-10-28 14:05 UTC, Agustín Moyano 		Details | Diff | Splinter Review
Bug 24359: Add test (2.86 KB, patch)
2020-12-15 09:18 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 24359: Remove hold from holds queue when captured (1.89 KB, patch)
2020-12-15 09:19 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 24359: Add test (2.82 KB, patch)
2021-03-01 18:47 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 24359: Remove hold from holds queue when captured (1.95 KB, patch)
2021-03-01 18:47 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 24359: (QA follow-up) Fix tests (3.35 KB, patch)
2021-03-01 18:47 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 24359: Add test (2.89 KB, patch)
2021-03-01 22:57 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 24359: Remove hold from holds queue when captured (2.02 KB, patch)
2021-03-01 22:57 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 24359: (QA follow-up) Fix tests (3.42 KB, patch)
2021-03-01 22:57 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joy Nelson 2020-01-06 22:34:20 UTC 
Currently, if an item is checked in and the hold is triggered/captured, the item will not come off the holds queue until the the cronjob runs again.  This causes some confusion for multibranch systems when other branches attempt to fill holds that have already been filled.

Proposal: Remove bib/items from the Holds Queue after checkin and before the next holds queue cron runs to update the holds queue.
Comment 1 AspenCat Team 2020-05-18 20:54:17 UTC 
This would be very helpful to AspenCat.
Comment 2 Sally 2020-08-14 08:14:38 UTC 
This would be a fantastic addition to Koha.
Comment 3 Agustín Moyano 2020-10-28 14:05:09 UTC 
Created attachment 112641 [details] [review]
Bug 24359: Add test
Comment 4 Agustín Moyano 2020-10-28 14:05:12 UTC 
Created attachment 112642 [details] [review]
Bug 24359: Remove hold from holds queue when captured

This patch removes a hold from holds queue when captured by check in.

To test:

1. check out an item from a book with multiple items for patron_1
2. place a biblio level hold for patron_2
3. perl misc/cronjobs/holds/build_holds_queue.pl
CHECK => holds queue shows the placed hold
4. check in the item from step 1 and confirm hold for patron_2
SUCCESS => hold for patron_2 is no longer in holds queue
5. prove t/db_dependent/HoldsQueue.t
Comment 5 Kelly McElligott 2020-12-10 01:03:28 UTC 
Hi,
I have gone through your test plan and at step 4- after checking the item in and confirming the hold, the item does not get removed from the Holds Queue, instead, the hold stays on the Holds queue, but the item also shows waiting for the patron.
Comment 6 Martin Renvoize (ashimema) 2020-12-15 09:18:59 UTC 
Created attachment 114405 [details] [review]
Bug 24359: Add test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2020-12-15 09:19:03 UTC 
Created attachment 114406 [details] [review]
Bug 24359: Remove hold from holds queue when captured

This patch removes a hold from holds queue when captured by check in.

To test:

1. check out an item from a book with multiple items for patron_1
2. place a biblio level hold for patron_2
3. perl misc/cronjobs/holds/build_holds_queue.pl
CHECK => holds queue shows the placed hold
4. check in the item from step 1 and confirm hold for patron_2
SUCCESS => hold for patron_2 is no longer in holds queue
5. prove t/db_dependent/HoldsQueue.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2020-12-15 09:19:20 UTC 
I like this, all works as expected, signing off.
Comment 9 Kyle M Hall (khall) 2021-02-24 20:14:20 UTC 
Unit tests are failing:

root@kohadevbox:koha(bug24359-qa)$ prove t/db_dependent/HoldsQueue.t 
t/db_dependent/HoldsQueue.t .. 56/57 
    #   Failed test 'Holds queue has one element'
    #   at t/db_dependent/HoldsQueue.t line 1712.
    #          got: '2'
    #     expected: '1'

    #   Failed test 'Holds queue has no elements, even when queue was not rebuilt'
    #   at t/db_dependent/HoldsQueue.t line 1722.
    #          got: '1'
    #     expected: '0'
t/db_dependent/HoldsQueue.t .. 57/57     # Looks like you failed 2 tests of 2.

#   Failed test 'Remove holds on check-in match'
#   at t/db_dependent/HoldsQueue.t line 1726.
# Looks like you failed 1 test of 57.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/57 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 256 Tests: 57 Failed: 1)
  Failed test:  57
  Non-zero exit status: 1
Files=1, Tests=57, 10 wallclock secs ( 0.03 usr  0.00 sys +  5.82 cusr  0.97 csys =  6.82 CPU)
Result: FAIL
Comment 10 Tomás Cohen Arazi (tcohen) 2021-03-01 18:47:27 UTC 
Created attachment 117462 [details] [review]
Bug 24359: Add test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi (tcohen) 2021-03-01 18:47:31 UTC 
Created attachment 117463 [details] [review]
Bug 24359: Remove hold from holds queue when captured

This patch removes a hold from holds queue when captured by check in.

To test:

1. check out an item from a book with multiple items for patron_1
2. place a biblio level hold for patron_2
3. perl misc/cronjobs/holds/build_holds_queue.pl
CHECK => holds queue shows the placed hold
4. check in the item from step 1 and confirm hold for patron_2
SUCCESS => hold for patron_2 is no longer in holds queue
5. prove t/db_dependent/HoldsQueue.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi (tcohen) 2021-03-01 18:47:36 UTC 
Created attachment 117464 [details] [review]
Bug 24359: (QA follow-up) Fix tests

This patch fixes a rebase issue, and also makes sure the tests do not
rely on any prior data. The query on tmp_holdsqueue is done against the
actual reserve_id so no need to delete all the data from the
tmp_holdsqueue and hold_fill_targets table.

I also noticed the previous subtest wasn't run inside a transaction even
if the code was designed to not depend on prior data as well.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/HoldsQueue.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi (tcohen) 2021-03-01 18:49:21 UTC 
I fixed a rebase issue. I also made the tests not reset the used tables.
Comment 14 Kyle M Hall (khall) 2021-03-01 22:57:06 UTC 
Created attachment 117465 [details] [review]
Bug 24359: Add test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall (khall) 2021-03-01 22:57:16 UTC 
Created attachment 117466 [details] [review]
Bug 24359: Remove hold from holds queue when captured

This patch removes a hold from holds queue when captured by check in.

To test:

1. check out an item from a book with multiple items for patron_1
2. place a biblio level hold for patron_2
3. perl misc/cronjobs/holds/build_holds_queue.pl
CHECK => holds queue shows the placed hold
4. check in the item from step 1 and confirm hold for patron_2
SUCCESS => hold for patron_2 is no longer in holds queue
5. prove t/db_dependent/HoldsQueue.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall (khall) 2021-03-01 22:57:19 UTC 
Created attachment 117467 [details] [review]
Bug 24359: (QA follow-up) Fix tests

This patch fixes a rebase issue, and also makes sure the tests do not
rely on any prior data. The query on tmp_holdsqueue is done against the
actual reserve_id so no need to delete all the data from the
tmp_holdsqueue and hold_fill_targets table.

I also noticed the previous subtest wasn't run inside a transaction even
if the code was designed to not depend on prior data as well.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/HoldsQueue.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Jonathan Druart 2021-03-09 10:02:34 UTC 
Pushed to master for 21.05, thanks to everybody involved!
Comment 18 Fridolin Somers 2021-03-12 08:48:49 UTC 
Small enhancement, I choose to backport.

Pushed to 20.11.x for 20.11.04
Comment 19 Katrin Fischer 2021-03-13 13:36:50 UTC 
While this is great, I wonder if it's not too much of a change in workflow for stable?
Comment 20 Andrew Fuerste-Henry 2021-03-14 20:02:51 UTC 
Not backporting to 20.05
Comment 21 Katrin Fischer 2021-03-15 14:48:58 UTC 
(In reply to Katrin Fischer from comment #19)
> While this is great, I wonder if it's not too much of a change in workflow
> for stable?

Discussed on IRC and deemed a good change, but release notes should be added!