Currently the holds queue deletes all currently selected items, gets a list of bibs with holds and loops through the bibs, building the queue for each. If the queue could be run a for a single biblionumber it would allow: - Deleting only holds on that bib: - this would remove the need for bug 29130 - could be run in background when a hold is placed/updated - fulfilled holds are already removed from queue - so we shouldn't need to clear any bibs that aren't updated Ultimately this would remove the need for the build_holds_queue cronjob
*** Bug 29130 has been marked as a duplicate of this bug. ***
Created attachment 131088 [details] [review] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries
Created attachment 131089 [details] [review] Bug 29346: Add holds queue update background job This patch adds a background job that takes care of updating the holds queue rows for a given list of biblios. It is designed to be used for real-time batch updating the holds queue when required. Tasks would be added by the relevant methods under the right use cases. Sponsored-by: Montgomery County Public Libraries
Submitting early to get feedback from interested community members.
Thanks for the enthusiasm everyone. Moving forward!
Created attachment 131811 [details] [review] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries
Created attachment 131812 [details] [review] Bug 29346: Add holds queue update background job This patch adds a background job that takes care of updating the holds queue rows for a given list of biblios. It is designed to be used for real-time batch updating the holds queue when required. Tasks would be added by the relevant methods under the right use cases. Sponsored-by: Montgomery County Public Libraries
Created attachment 131813 [details] [review] Bug 29346: Hold actions triggers This patch makes several holds related actions schedule the background job for real-time update of the holds queue. This actions are: - place (C4::Reserves::AddReserve) - fill (Koha::Hold->fill) - cancel (Koha::Hold->cancel) - suspend (Koha::Hold->suspend) - resume (Koha::Hold->resume) To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t \ t/db_dependent/Reserves.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Sponsored-by: Montgomery County Public Libraries
Created attachment 131815 [details] [review] Bug 29346: Circulation actions triggers This patch introduces triggers for real-time updating the holds queue at check out and check in. The following high-level methods are involved: - C4::Circulation::AddIssue - C4::Circulation::AddReturn To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Circulation_holdsqueue.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Note: I put the tests on a separate file because the other one was too big already. Sponsored-by: Montgomery County Public Libraries
Created attachment 132078 [details] [review] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries
Created attachment 132079 [details] [review] Bug 29346: Add holds queue update background job This patch adds a background job that takes care of updating the holds queue rows for a given list of biblios. It is designed to be used for real-time batch updating the holds queue when required. Tasks would be added by the relevant methods under the right use cases. Sponsored-by: Montgomery County Public Libraries
Created attachment 132080 [details] [review] Bug 29346: Hold actions triggers This patch makes several holds related actions schedule the background job for real-time update of the holds queue. This actions are: - place (C4::Reserves::AddReserve) - fill (Koha::Hold->fill) - cancel (Koha::Hold->cancel) - suspend (Koha::Hold->suspend) - resume (Koha::Hold->resume) The cancel() action is added a *skip_holds_queue* parameter to skip triggering the background job entirely. It targets cases like C4::Biblio::DelBiblio in which all biblio holds are cancelled in a loop. In that case, we just want to cancel them and let a single backgroung job take care of the holds queue, once the biblio is deleted. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t \ t/db_dependent/Reserves.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Sponsored-by: Montgomery County Public Libraries holds
Created attachment 132081 [details] [review] Bug 29346: Circulation actions triggers This patch introduces triggers for real-time updating the holds queue at check out and check in. The following high-level methods are involved: - C4::Circulation::AddIssue - C4::Circulation::AddReturn To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Circulation_holdsqueue.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Note: I put the tests on a separate file because the other one was too big already. Sponsored-by: Montgomery County Public Libraries
Created attachment 132082 [details] [review] Bug 29346: Biblio actions triggers This patch adds the trigger for the holds queue update on teh following methods: - C4::Biblio::DelBiblio - C4::Biblio::ModBiblio The ModBiblio use case could be improved by checking if itemtype is one of the updated attributes... but it felt there was no way to do it without some overhead. So I leave it as-is. It also mocks the ->enqueue method in the tests that call DelBiblio and ModBiblio to avoid breakages [1] Tests are added to check the trigger is called. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! It means the trigger is triggered :-D 3. Sign off :-D [1] This breakages could be avoided if we solve Koha/BackgrounJob.pm:101 FIXME. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 132083 [details] [review] Bug 29346: Item action trigger This patch makes the following actions trigger a holds queue rebuild for the related biblio: - Adding an item - Updating an item - Deleting an item To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => SUCCESS: Tests pass! Background job scheduled 3. Sign off :-D
*** Bug 26003 has been marked as a duplicate of this bug. ***
I'm getting an error from the test: t/db_dependent/HoldsQueue.t .. 1/58 No property queue for Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue# Looks like your test exited with 11 just after 2. t/db_dependent/HoldsQueue.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 56/58 subtests Test Summary Report ------------------- t/db_dependent/HoldsQueue.t (Wstat: 2816 Tests: 2 Failed: 0) Non-zero exit status: 11 Parse errors: Bad plan. You planned 58 tests but ran 2.
(In reply to Owen Leonard from comment #17) > I'm getting an error from the test: > > t/db_dependent/HoldsQueue.t .. 1/58 No property queue for > Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue# Looks like your test > exited with 11 just after 2. > t/db_dependent/HoldsQueue.t .. Dubious, test returned 11 (wstat 2816, 0xb00) > Failed 56/58 subtests > > Test Summary Report > ------------------- > t/db_dependent/HoldsQueue.t (Wstat: 2816 Tests: 2 Failed: 0) > Non-zero exit status: 11 > Parse errors: Bad plan. You planned 58 tests but ran 2. You need to run dbic on one of the dependencies
(In reply to Tomás Cohen Arazi from comment #18) > (In reply to Owen Leonard from comment #17) > > I'm getting an error from the test: > > > > t/db_dependent/HoldsQueue.t .. 1/58 No property queue for > > Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue# Looks like your test > > exited with 11 just after 2. > > t/db_dependent/HoldsQueue.t .. Dubious, test returned 11 (wstat 2816, 0xb00) > > Failed 56/58 subtests > > > > Test Summary Report > > ------------------- > > t/db_dependent/HoldsQueue.t (Wstat: 2816 Tests: 2 Failed: 0) > > Non-zero exit status: 11 > > Parse errors: Bad plan. You planned 58 tests but ran 2. > > You need to run dbic on one of the dependencies I've added a [DO NOT PUSH] patch on bug 27783 for that. Please retry :-D
Created attachment 133183 [details] [review] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133184 [details] [review] Bug 29346: Add holds queue update background job This patch adds a background job that takes care of updating the holds queue rows for a given list of biblios. It is designed to be used for real-time batch updating the holds queue when required. Tasks would be added by the relevant methods under the right use cases. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133185 [details] [review] Bug 29346: Hold actions triggers This patch makes several holds related actions schedule the background job for real-time update of the holds queue. This actions are: - place (C4::Reserves::AddReserve) - fill (Koha::Hold->fill) - cancel (Koha::Hold->cancel) - suspend (Koha::Hold->suspend) - resume (Koha::Hold->resume) The cancel() action is added a *skip_holds_queue* parameter to skip triggering the background job entirely. It targets cases like C4::Biblio::DelBiblio in which all biblio holds are cancelled in a loop. In that case, we just want to cancel them and let a single backgroung job take care of the holds queue, once the biblio is deleted. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t \ t/db_dependent/Reserves.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133186 [details] [review] Bug 29346: Circulation actions triggers This patch introduces triggers for real-time updating the holds queue at check out and check in. The following high-level methods are involved: - C4::Circulation::AddIssue - C4::Circulation::AddReturn To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Circulation_holdsqueue.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Note: I put the tests on a separate file because the other one was too big already. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133187 [details] [review] Bug 29346: Biblio actions triggers This patch adds the trigger for the holds queue update on teh following methods: - C4::Biblio::DelBiblio - C4::Biblio::ModBiblio The ModBiblio use case could be improved by checking if itemtype is one of the updated attributes... but it felt there was no way to do it without some overhead. So I leave it as-is. It also mocks the ->enqueue method in the tests that call DelBiblio and ModBiblio to avoid breakages [1] Tests are added to check the trigger is called. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! It means the trigger is triggered :-D 3. Sign off :-D [1] This breakages could be avoided if we solve Koha/BackgrounJob.pm:101 FIXME. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133188 [details] [review] Bug 29346: Item action trigger This patch makes the following actions trigger a holds queue rebuild for the related biblio: - Adding an item - Updating an item - Deleting an item To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => SUCCESS: Tests pass! Background job scheduled 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133189 [details] [review] Bug 29346: (follow-up) Fix typo We set biblio_ids in enqueue then referred to biblio_id in process ;) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133190 [details] [review] Bug 29346: (follow-up) Fix import issue in HoldsQueue For some reason `t/db_dependent/HoldsQueue..t` starts failing for me without this patch.. using the full module path resolves the issue but I'd love to understand why/how this creeps in.
Created attachment 133191 [details] [review] Bug 29346: FIXME: Highlight double enqueue This patch highlights a possible issue with the triggers.. though it may not actually matter in reality. I appear to already see test failures before this patch with a double enqueue of the rebuild for both AddIssue and AddReturn.. I couldn't spot what was causing that but whilst digging I did find another case where it could happen as highlighted here...
This is cool.. and mostly appears to work in testing. I've added a few tiny follow-ups to fix things I found whilst running the tests.. not idea where the imports get a bit confused and require that second fix though. As for the third patch, it highlights a case I think we may need to deal with.. we're already getting double enqueued on AddIssue and AddReturn (I reckon the test passed before we added the next patches that add triggers to ModBiblio and ModItem).. they now double trigger for these cases.. or even triple trigger in certainly preference setups.
Created attachment 133205 [details] [review] Bug 29346: FIXME: Highlight double enqueue This patch highlights a possible issue with the triggers.. though it may not actually matter in reality. I appear to already see test failures before this patch with a double enqueue of the rebuild for both AddIssue and AddReturn.. I couldn't spot what was causing that but whilst digging I did find another case where it could happen as highlighted here...
Adding caller in the test here is pretty useful for debugging.. you can now see which module is calling the enqueue each time :)
Adding discussion from Slack.. I don't think we need the trigger in DelBiblio.. but we likely do need to think about preventing Item level triggers for batch operations and then triggering at the end of said batch operations (like BatchDeleteBiblio). I think we can drop the triggers in circ (but keep the tests to ensure the triggers do actually still get triggered once).. circ operations affect items and as such trigger the task there.
All tests pass except this one: prove t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation_holdsqueue.t .. Use of uninitialized value in addition (+) at /kohadevbox/koha/C4/Biblio.pm line 3107. # Looks like you planned 2 tests but ran 5. t/db_dependent/Circulation_holdsqueue.t .. 1/1 # Failed test 'AddIssue() and AddReturn() real-time holds queue tests' # at t/db_dependent/Circulation_holdsqueue.t line 66. # Looks like you failed 1 test of 1. t/db_dependent/Circulation_holdsqueue.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/1 subtests Test Summary Report ------------------- t/db_dependent/Circulation_holdsqueue.t (Wstat: 256 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 1 Files=1, Tests=1, 2 wallclock secs ( 0.01 usr 0.01 sys + 1.98 cusr 0.11 csys = 2.11 CPU) Result: FAIL
(In reply to David Nind from comment #33) > All tests pass except this one: > > prove t/db_dependent/Circulation_holdsqueue.t Hi, David. Thanks for testing! Martin highlighted with this the fact that some calls (circulation) trigger item/biblio updates and thus duplicate actions. I have worked on chasing those cases and will wrap the code and new tests on monday. Have a great weekend!
Created attachment 133991 [details] [review] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133992 [details] [review] Bug 29346: Add holds queue update background job This patch adds a background job that takes care of updating the holds queue rows for a given list of biblios. It is designed to be used for real-time batch updating the holds queue when required. Tasks would be added by the relevant methods under the right use cases. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133993 [details] [review] Bug 29346: Hold actions triggers This patch makes several holds related actions schedule the background job for real-time update of the holds queue. This actions are: - place (C4::Reserves::AddReserve) - fill (Koha::Hold->fill) - cancel (Koha::Hold->cancel) - suspend (Koha::Hold->suspend) - resume (Koha::Hold->resume) The cancel() action is added a *skip_holds_queue* parameter to skip triggering the background job entirely. It targets cases like C4::Biblio::DelBiblio in which all biblio holds are cancelled in a loop. In that case, we just want to cancel them and let a single backgroung job take care of the holds queue, once the biblio is deleted. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t \ t/db_dependent/Reserves.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133994 [details] [review] Bug 29346: Circulation actions triggers This patch introduces triggers for real-time updating the holds queue at check out and check in. The following high-level methods are involved: - C4::Circulation::AddIssue - C4::Circulation::AddReturn To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Circulation_holdsqueue.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Note: I put the tests on a separate file because the other one was too big already. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133995 [details] [review] Bug 29346: Biblio actions triggers This patch adds the trigger for the holds queue update on teh following methods: - C4::Biblio::DelBiblio - C4::Biblio::ModBiblio The ModBiblio use case could be improved by checking if itemtype is one of the updated attributes... but it felt there was no way to do it without some overhead. So I leave it as-is. It also mocks the ->enqueue method in the tests that call DelBiblio and ModBiblio to avoid breakages [1] Tests are added to check the trigger is called. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! It means the trigger is triggered :-D 3. Sign off :-D [1] This breakages could be avoided if we solve Koha/BackgrounJob.pm:101 FIXME. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133996 [details] [review] Bug 29346: Item action trigger This patch makes the following actions trigger a holds queue rebuild for the related biblio: - Adding an item - Updating an item - Deleting an item To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => SUCCESS: Tests pass! Background job scheduled 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133997 [details] [review] Bug 29346: (follow-up) Fix typo We set biblio_ids in enqueue then referred to biblio_id in process ;) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133998 [details] [review] Bug 29346: (follow-up) Fix import issue in HoldsQueue For some reason `t/db_dependent/HoldsQueue..t` starts failing for me without this patch.. using the full module path resolves the issue but I'd love to understand why/how this creeps in. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 133999 [details] [review] Bug 29346: Highlight double enqueue This patch highlights a possible issue with the triggers.. though it may not actually matter in reality. I appear to already see test failures before this patch with a double enqueue of the rebuild for both AddIssue and AddReturn.. I couldn't spot what was causing that but whilst digging I did find another case where it could happen as highlighted here... Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134000 [details] [review] Bug 29346: Avoid duplicate actions tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134001 [details] [review] Bug 29346: Add more fine-grained control of holds queue updates This patch deals with the fact that high-level circualtion methods like `AddIssue`, `AddReturn` and `ModDateLastSeen` all eventually call lower-level methods like ModBiblio, Koha::Item->store of UpdateTotalIssues which are expected to trigger holds queue updates (for the object CRUD operations use cases). As the circulation methods need to trigger holds queue update as well, duplicate updates were being requested which is suboptimal, of course. In order to prevent this, and because circulation methdos could trigger holds queue updates several times, actually, I added a new parameter *skip_holds_queue* to the low-level methods, so when they are called from circulation, the trigger is skipped and we have greater control on when and how holds queue updates are scheduled. This patch introduces the `skip_holds_queue` parameter to the following methods: * C4::Biblio::ModBiblio * C4::Biblio::UpdateTotalIssues * Koha::Item->store Calls to those methods from the following methods will include the new parameter, and thus duplicated holds queue updates avoided: * C4::Circulation::AddIssue * C4::Circulation::AddReturn * C4::Items::ModDateLastSeen Tests are added, to verify that the (mocked) BatchUpdateBiblioHoldsQueue task is only scheduled once when they are called. To test: 1. Apply up to the previous patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t \ t/db_dependent/Biblio_holdsqueue.t \ t/db_dependent/Circulation_holdsqueue.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D
Created attachment 134010 [details] [review] Bug 29077: (follow-up) Use another syntax Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 134225 [details] [review] Bug 29346: (QA follow-up) Rebuild queue when adjusting priorities of holds
Created attachment 134227 [details] [review] Bug 29346: (follow-up) Fix typo 'availabe'
The queue works, test pass, but when I go to the background jobs page: 173 Started 0 / 1 Unknown job type 'update_holds_queue_for_biblios' 04/28/2022 12:08 04/28/2022 12:08 04/28/2022 12:08 And if I click view: Template process failed: file error - background_jobs/update_holds_queue_for_biblios.inc: not found at /kohadevbox/koha/C4/Templates.pm line 127 Also, we should update the queue when we 'Revert waiting status' on a hold
Sorry, it seems I didn't confirm the upload on my desktop computer. Will do once I get back home.
Created attachment 134309 [details] [review] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134310 [details] [review] Bug 29346: Add holds queue update background job This patch adds a background job that takes care of updating the holds queue rows for a given list of biblios. It is designed to be used for real-time batch updating the holds queue when required. Tasks would be added by the relevant methods under the right use cases. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134311 [details] [review] Bug 29346: Hold actions triggers This patch makes several holds related actions schedule the background job for real-time update of the holds queue. This actions are: - place (C4::Reserves::AddReserve) - fill (Koha::Hold->fill) - cancel (Koha::Hold->cancel) - suspend (Koha::Hold->suspend) - resume (Koha::Hold->resume) The cancel() action is added a *skip_holds_queue* parameter to skip triggering the background job entirely. It targets cases like C4::Biblio::DelBiblio in which all biblio holds are cancelled in a loop. In that case, we just want to cancel them and let a single backgroung job take care of the holds queue, once the biblio is deleted. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t \ t/db_dependent/Reserves.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134312 [details] [review] Bug 29346: Circulation actions triggers This patch introduces triggers for real-time updating the holds queue at check out and check in. The following high-level methods are involved: - C4::Circulation::AddIssue - C4::Circulation::AddReturn To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Circulation_holdsqueue.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Note: I put the tests on a separate file because the other one was too big already. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134313 [details] [review] Bug 29346: Biblio actions triggers This patch adds the trigger for the holds queue update on teh following methods: - C4::Biblio::DelBiblio - C4::Biblio::ModBiblio The ModBiblio use case could be improved by checking if itemtype is one of the updated attributes... but it felt there was no way to do it without some overhead. So I leave it as-is. It also mocks the ->enqueue method in the tests that call DelBiblio and ModBiblio to avoid breakages [1] Tests are added to check the trigger is called. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! It means the trigger is triggered :-D 3. Sign off :-D [1] This breakages could be avoided if we solve Koha/BackgrounJob.pm:101 FIXME. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134314 [details] [review] Bug 29346: Item action trigger This patch makes the following actions trigger a holds queue rebuild for the related biblio: - Adding an item - Updating an item - Deleting an item To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => SUCCESS: Tests pass! Background job scheduled 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134315 [details] [review] Bug 29346: (follow-up) Fix typo We set biblio_ids in enqueue then referred to biblio_id in process ;) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134316 [details] [review] Bug 29346: Highlight double enqueue This patch highlights a possible issue with the triggers.. though it may not actually matter in reality. I appear to already see test failures before this patch with a double enqueue of the rebuild for both AddIssue and AddReturn.. I couldn't spot what was causing that but whilst digging I did find another case where it could happen as highlighted here... Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134317 [details] [review] Bug 29346: Avoid duplicate actions tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134318 [details] [review] Bug 29346: Add more fine-grained control of holds queue updates This patch deals with the fact that high-level circualtion methods like `AddIssue`, `AddReturn` and `ModDateLastSeen` all eventually call lower-level methods like ModBiblio, Koha::Item->store of UpdateTotalIssues which are expected to trigger holds queue updates (for the object CRUD operations use cases). As the circulation methods need to trigger holds queue update as well, duplicate updates were being requested which is suboptimal, of course. In order to prevent this, and because circulation methdos could trigger holds queue updates several times, actually, I added a new parameter *skip_holds_queue* to the low-level methods, so when they are called from circulation, the trigger is skipped and we have greater control on when and how holds queue updates are scheduled. This patch introduces the `skip_holds_queue` parameter to the following methods: * C4::Biblio::ModBiblio * C4::Biblio::UpdateTotalIssues * Koha::Item->store Calls to those methods from the following methods will include the new parameter, and thus duplicated holds queue updates avoided: * C4::Circulation::AddIssue * C4::Circulation::AddReturn * C4::Items::ModDateLastSeen Tests are added, to verify that the (mocked) BatchUpdateBiblioHoldsQueue task is only scheduled once when they are called. To test: 1. Apply up to the previous patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t \ t/db_dependent/Biblio_holdsqueue.t \ t/db_dependent/Circulation_holdsqueue.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134319 [details] [review] Bug 29346: (QA follow-up) Rebuild queue when adjusting priorities of holds Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134320 [details] [review] Bug 29346: (follow-up) Fix typo 'availabe' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134321 [details] [review] Bug 29346: Add better reporting Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134322 [details] [review] Bug 29346: Remove unused circular dependency on C4::Search Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134328 [details] [review] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134329 [details] [review] Bug 29346: Add holds queue update background job This patch adds a background job that takes care of updating the holds queue rows for a given list of biblios. It is designed to be used for real-time batch updating the holds queue when required. Tasks would be added by the relevant methods under the right use cases. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134330 [details] [review] Bug 29346: Hold actions triggers This patch makes several holds related actions schedule the background job for real-time update of the holds queue. This actions are: - place (C4::Reserves::AddReserve) - fill (Koha::Hold->fill) - cancel (Koha::Hold->cancel) - suspend (Koha::Hold->suspend) - resume (Koha::Hold->resume) The cancel() action is added a *skip_holds_queue* parameter to skip triggering the background job entirely. It targets cases like C4::Biblio::DelBiblio in which all biblio holds are cancelled in a loop. In that case, we just want to cancel them and let a single backgroung job take care of the holds queue, once the biblio is deleted. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t \ t/db_dependent/Reserves.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134331 [details] [review] Bug 29346: Circulation actions triggers This patch introduces triggers for real-time updating the holds queue at check out and check in. The following high-level methods are involved: - C4::Circulation::AddIssue - C4::Circulation::AddReturn To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Circulation_holdsqueue.t => SUCCESS: Tests pass! Triggers are triggered 3. Sign off :-D Note: I put the tests on a separate file because the other one was too big already. Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134332 [details] [review] Bug 29346: Biblio actions triggers This patch adds the trigger for the holds queue update on teh following methods: - C4::Biblio::DelBiblio - C4::Biblio::ModBiblio The ModBiblio use case could be improved by checking if itemtype is one of the updated attributes... but it felt there was no way to do it without some overhead. So I leave it as-is. It also mocks the ->enqueue method in the tests that call DelBiblio and ModBiblio to avoid breakages [1] Tests are added to check the trigger is called. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! It means the trigger is triggered :-D 3. Sign off :-D [1] This breakages could be avoided if we solve Koha/BackgrounJob.pm:101 FIXME. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134333 [details] [review] Bug 29346: Item action trigger This patch makes the following actions trigger a holds queue rebuild for the related biblio: - Adding an item - Updating an item - Deleting an item To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => SUCCESS: Tests pass! Background job scheduled 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134334 [details] [review] Bug 29346: (follow-up) Fix typo We set biblio_ids in enqueue then referred to biblio_id in process ;) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134335 [details] [review] Bug 29346: Highlight double enqueue This patch highlights a possible issue with the triggers.. though it may not actually matter in reality. I appear to already see test failures before this patch with a double enqueue of the rebuild for both AddIssue and AddReturn.. I couldn't spot what was causing that but whilst digging I did find another case where it could happen as highlighted here... Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134336 [details] [review] Bug 29346: Avoid duplicate actions tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134337 [details] [review] Bug 29346: Add more fine-grained control of holds queue updates This patch deals with the fact that high-level circualtion methods like `AddIssue`, `AddReturn` and `ModDateLastSeen` all eventually call lower-level methods like ModBiblio, Koha::Item->store of UpdateTotalIssues which are expected to trigger holds queue updates (for the object CRUD operations use cases). As the circulation methods need to trigger holds queue update as well, duplicate updates were being requested which is suboptimal, of course. In order to prevent this, and because circulation methdos could trigger holds queue updates several times, actually, I added a new parameter *skip_holds_queue* to the low-level methods, so when they are called from circulation, the trigger is skipped and we have greater control on when and how holds queue updates are scheduled. This patch introduces the `skip_holds_queue` parameter to the following methods: * C4::Biblio::ModBiblio * C4::Biblio::UpdateTotalIssues * Koha::Item->store Calls to those methods from the following methods will include the new parameter, and thus duplicated holds queue updates avoided: * C4::Circulation::AddIssue * C4::Circulation::AddReturn * C4::Items::ModDateLastSeen Tests are added, to verify that the (mocked) BatchUpdateBiblioHoldsQueue task is only scheduled once when they are called. To test: 1. Apply up to the previous patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t \ t/db_dependent/Biblio_holdsqueue.t \ t/db_dependent/Circulation_holdsqueue.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134338 [details] [review] Bug 29346: (QA follow-up) Rebuild queue when adjusting priorities of holds Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134339 [details] [review] Bug 29346: (follow-up) Fix typo 'availabe' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134340 [details] [review] Bug 29346: Add better reporting Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134341 [details] [review] Bug 29346: Remove unused circular dependency on C4::Search Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134342 [details] [review] Bug 29346: Use fully qualified names for C4:Circulation routines in C4::HoldsQueue I suppose this is similar to circular dependency on other patch HoldsQueue uses Circulation uses BatchUpdateBiblioHoldsQueueuse HoldsQueue Without this the background job builds the queue, but reports failure: Holds queue for biblio The Jacobite clans of the Great Glen, 1650-1784 /. An error occurred (Undefined subroutine &C4::HoldsQueue::GetTransfers called at /kohadevbox/koha/C4/HoldsQueue.pm line 351. ) Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 134343 [details] [review] Bug 29346: (follow-up) Build holds queue when AlterPriority or RevertWaitingStatus is called To test: 1 - Place 3 holds on a bib with a single item 2 - Confirm bib shows in holds queue 3 - Check in item and cnofirm hold 4 - Bib is no longer in queue 5 - Revert the waiting status 6 - The hold is in the queue again 7 - Move top hold to bottom 8 - Confirm queue selects hold for new top priorty patron Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
There is something wrong with the update_holds_queue_for_biblios task. Try this, in fresh ktd delete itemnumbers 958,960 in a batch and check "delete the biblio if no item remain". You will see 3 update_holds_queue_for_biblios new tasks. I think we need to revisit this enhancement for the batch tools.
(In reply to Jonathan Druart from comment #82) > There is something wrong with the update_holds_queue_for_biblios task. Try > this, in fresh ktd delete itemnumbers 958,960 in a batch and check "delete > the biblio if no item remain". You will see 3 update_holds_queue_for_biblios > new tasks. > > I think we need to revisit this enhancement for the batch tools. This makes sense, one update each when we remove the items, then a third update when we delete one of the biblios Ideally we would only update once, but the code handles item deletion, then biblio deletion, so would need a refactoring to support this.
(In reply to Nick Clemens from comment #83) > (In reply to Jonathan Druart from comment #82) > > There is something wrong with the update_holds_queue_for_biblios task. Try > > this, in fresh ktd delete itemnumbers 958,960 in a batch and check "delete > > the biblio if no item remain". You will see 3 update_holds_queue_for_biblios > > new tasks. > > > > I think we need to revisit this enhancement for the batch tools. > > This makes sense, one update each when we remove the items, then a third > update when we delete one of the biblios data: {"biblio_ids":[429],"messages":[{"type":"success","biblio_id":429,"code":"holds_queue_updated"}],"report":{"total_biblios":1,"total_success":1}} data: {"biblio_ids":[430],"messages":[{"code":"holds_queue_updated","biblio_id":430,"type":"success"}],"report":{"total_success":1,"total_biblios":1}} data: {"messages":[{"biblio_id":430,"type":"success","code":"holds_queue_updated"}],"biblio_ids":[430],"report":{"total_biblios":1,"total_success":1}} Yes, it's what it does. But it does not make sense, it's a bug. With 1 batchmod of 2 items, we end up with: 1 task for the main one 3 holds_queue_updated 1 indexing