Bug 29346 - Real-time holds queue update
Summary: Real-time holds queue update
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Nick Clemens
URL:
Keywords: release-notes-needed
: 26003 29130 (view as bug list)
Depends on: 27783 30181
Blocks: 34822 35819 30710 31948
  Show dependency treegraph
 
Reported: 2021-10-28 11:04 UTC by Nick Clemens
Modified: 2024-01-16 14:55 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Enabled by default, this feature can be opted out using the *RealTimeHoldsQueue* system preference.
Version(s) released in:
22.05.00


Attachments
Bug 29346: Refactor loop code into a subroutine (6.00 KB, patch)
2022-02-24 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add holds queue update background job (6.18 KB, patch)
2022-02-24 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Refactor loop code into a subroutine (6.00 KB, patch)
2022-03-16 20:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add holds queue update background job (5.96 KB, patch)
2022-03-16 20:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Hold actions triggers (7.89 KB, patch)
2022-03-16 20:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Circulation actions triggers (4.18 KB, patch)
2022-03-16 21:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Refactor loop code into a subroutine (6.00 KB, patch)
2022-03-23 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add holds queue update background job (6.07 KB, patch)
2022-03-23 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Hold actions triggers (8.86 KB, patch)
2022-03-23 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Circulation actions triggers (4.18 KB, patch)
2022-03-23 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Biblio actions triggers (5.09 KB, patch)
2022-03-23 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Item action trigger (3.13 KB, patch)
2022-03-23 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Refactor loop code into a subroutine (6.06 KB, patch)
2022-04-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: Add holds queue update background job (6.14 KB, patch)
2022-04-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: Hold actions triggers (8.91 KB, patch)
2022-04-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: Circulation actions triggers (4.25 KB, patch)
2022-04-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: Biblio actions triggers (5.15 KB, patch)
2022-04-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: Item action trigger (3.19 KB, patch)
2022-04-11 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix typo (957 bytes, patch)
2022-04-11 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix import issue in HoldsQueue (1.24 KB, patch)
2022-04-11 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: FIXME: Highlight double enqueue (1.20 KB, patch)
2022-04-11 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: FIXME: Highlight double enqueue (1.70 KB, patch)
2022-04-12 08:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29346: Refactor loop code into a subroutine (6.12 KB, patch)
2022-04-26 19:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add holds queue update background job (6.19 KB, patch)
2022-04-26 19:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Hold actions triggers (8.97 KB, patch)
2022-04-26 19:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Circulation actions triggers (4.31 KB, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Biblio actions triggers (5.21 KB, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Item action trigger (3.25 KB, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix typo (1014 bytes, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix import issue in HoldsQueue (1.30 KB, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Highlight double enqueue (1.75 KB, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Avoid duplicate actions tests (6.78 KB, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add more fine-grained control of holds queue updates (9.53 KB, patch)
2022-04-26 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29077: (follow-up) Use another syntax (1.18 KB, patch)
2022-04-27 07:01 UTC, David Nind
Details | Diff | Splinter Review
Bug 29346: (QA follow-up) Rebuild queue when adjusting priorities of holds (1.06 KB, patch)
2022-04-28 12:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix typo 'availabe' (1.02 KB, patch)
2022-04-28 12:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Refactor loop code into a subroutine (6.12 KB, patch)
2022-04-28 22:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add holds queue update background job (6.19 KB, patch)
2022-04-28 22:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Hold actions triggers (8.97 KB, patch)
2022-04-28 22:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Circulation actions triggers (4.30 KB, patch)
2022-04-28 22:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Biblio actions triggers (5.20 KB, patch)
2022-04-28 22:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Item action trigger (3.25 KB, patch)
2022-04-28 22:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix typo (1012 bytes, patch)
2022-04-28 22:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Highlight double enqueue (1.75 KB, patch)
2022-04-28 22:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Avoid duplicate actions tests (6.78 KB, patch)
2022-04-28 22:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add more fine-grained control of holds queue updates (9.58 KB, patch)
2022-04-28 22:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: (QA follow-up) Rebuild queue when adjusting priorities of holds (1.11 KB, patch)
2022-04-28 22:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix typo 'availabe' (1.08 KB, patch)
2022-04-28 22:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Add better reporting (5.18 KB, patch)
2022-04-28 22:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Remove unused circular dependency on C4::Search (690 bytes, patch)
2022-04-28 22:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29346: Refactor loop code into a subroutine (6.17 KB, patch)
2022-04-29 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Add holds queue update background job (6.25 KB, patch)
2022-04-29 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Hold actions triggers (9.02 KB, patch)
2022-04-29 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Circulation actions triggers (4.36 KB, patch)
2022-04-29 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Biblio actions triggers (5.26 KB, patch)
2022-04-29 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Item action trigger (3.30 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix typo (1.04 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Highlight double enqueue (1.81 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Avoid duplicate actions tests (6.83 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Add more fine-grained control of holds queue updates (9.64 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: (QA follow-up) Rebuild queue when adjusting priorities of holds (1.17 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: (follow-up) Fix typo 'availabe' (1.13 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Add better reporting (5.24 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Remove unused circular dependency on C4::Search (747 bytes, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: Use fully qualified names for C4:Circulation routines in C4::HoldsQueue (1.54 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29346: (follow-up) Build holds queue when AlterPriority or RevertWaitingStatus is called (5.29 KB, patch)
2022-04-29 01:03 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-10-28 11:04:07 UTC
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
Comment 1 Nick Clemens 2021-10-28 11:04:25 UTC
*** Bug 29130 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2022-02-24 19:57:30 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2022-02-24 19:57:35 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2022-02-24 19:58:16 UTC
Submitting early to get feedback from interested community members.
Comment 5 Tomás Cohen Arazi 2022-03-16 02:42:10 UTC
Thanks for the enthusiasm everyone. Moving forward!
Comment 6 Tomás Cohen Arazi 2022-03-16 20:43:10 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2022-03-16 20:43:15 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2022-03-16 20:43:20 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2022-03-16 21:09:07 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2022-03-23 19:36:05 UTC
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
Comment 11 Tomás Cohen Arazi 2022-03-23 19:36:10 UTC
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
Comment 12 Tomás Cohen Arazi 2022-03-23 19:36:15 UTC
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
Comment 13 Tomás Cohen Arazi 2022-03-23 19:36:20 UTC
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
Comment 14 Tomás Cohen Arazi 2022-03-23 19:36:25 UTC
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>
Comment 15 Tomás Cohen Arazi 2022-03-23 19:36:30 UTC
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
Comment 16 Martin Renvoize 2022-04-01 07:39:17 UTC
*** Bug 26003 has been marked as a duplicate of this bug. ***
Comment 17 Owen Leonard 2022-04-04 14:21:07 UTC
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.
Comment 18 Tomás Cohen Arazi 2022-04-04 14:45:33 UTC
(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
Comment 19 Tomás Cohen Arazi 2022-04-04 17:53:00 UTC
(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
Comment 20 Martin Renvoize 2022-04-11 16:22:37 UTC
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>
Comment 21 Martin Renvoize 2022-04-11 16:22:42 UTC
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>
Comment 22 Martin Renvoize 2022-04-11 16:22:46 UTC
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>
Comment 23 Martin Renvoize 2022-04-11 16:22:51 UTC
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>
Comment 24 Martin Renvoize 2022-04-11 16:22:57 UTC
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>
Comment 25 Martin Renvoize 2022-04-11 16:23:01 UTC
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>
Comment 26 Martin Renvoize 2022-04-11 16:23:06 UTC
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>
Comment 27 Martin Renvoize 2022-04-11 16:23:11 UTC
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.
Comment 28 Martin Renvoize 2022-04-11 16:23:15 UTC
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...
Comment 29 Martin Renvoize 2022-04-11 16:25:49 UTC
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.
Comment 30 Martin Renvoize 2022-04-12 08:21:25 UTC
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...
Comment 31 Martin Renvoize 2022-04-12 08:22:34 UTC
Adding caller in the test here is pretty useful for debugging.. you can now see which module is calling the enqueue each time :)
Comment 32 Martin Renvoize 2022-04-12 10:34:58 UTC
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.
Comment 33 David Nind 2022-04-19 18:52:53 UTC
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
Comment 34 Tomás Cohen Arazi 2022-04-22 11:26:40 UTC
(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!
Comment 35 Tomás Cohen Arazi 2022-04-26 19:38:47 UTC
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>
Comment 36 Tomás Cohen Arazi 2022-04-26 19:38:52 UTC
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>
Comment 37 Tomás Cohen Arazi 2022-04-26 19:38:58 UTC
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>
Comment 38 Tomás Cohen Arazi 2022-04-26 19:39:05 UTC
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>
Comment 39 Tomás Cohen Arazi 2022-04-26 19:39:11 UTC
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>
Comment 40 Tomás Cohen Arazi 2022-04-26 19:39:17 UTC
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>
Comment 41 Tomás Cohen Arazi 2022-04-26 19:39:23 UTC
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>
Comment 42 Tomás Cohen Arazi 2022-04-26 19:39:29 UTC
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>
Comment 43 Tomás Cohen Arazi 2022-04-26 19:39:34 UTC
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>
Comment 44 Tomás Cohen Arazi 2022-04-26 19:39:40 UTC
Created attachment 134000 [details] [review]
Bug 29346: Avoid duplicate actions tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Tomás Cohen Arazi 2022-04-26 19:39:46 UTC
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
Comment 46 David Nind 2022-04-27 07:01:08 UTC Comment hidden (obsolete)
Comment 47 Nick Clemens 2022-04-28 12:13:22 UTC
Created attachment 134225 [details] [review]
Bug 29346: (QA follow-up) Rebuild queue when adjusting priorities of holds
Comment 48 Nick Clemens 2022-04-28 12:13:27 UTC
Created attachment 134227 [details] [review]
Bug 29346: (follow-up) Fix typo 'availabe'
Comment 49 Nick Clemens 2022-04-28 12:15:19 UTC
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
Comment 50 Tomás Cohen Arazi 2022-04-28 19:06:21 UTC
Sorry, it seems I didn't confirm the upload on my desktop computer. Will do once I get back home.
Comment 51 Tomás Cohen Arazi 2022-04-28 22:55:08 UTC
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>
Comment 52 Tomás Cohen Arazi 2022-04-28 22:55:16 UTC
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>
Comment 53 Tomás Cohen Arazi 2022-04-28 22:55:25 UTC
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>
Comment 54 Tomás Cohen Arazi 2022-04-28 22:55:35 UTC
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>
Comment 55 Tomás Cohen Arazi 2022-04-28 22:55:45 UTC
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>
Comment 56 Tomás Cohen Arazi 2022-04-28 22:55:56 UTC
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>
Comment 57 Tomás Cohen Arazi 2022-04-28 22:56:07 UTC
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>
Comment 58 Tomás Cohen Arazi 2022-04-28 22:56:26 UTC
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>
Comment 59 Tomás Cohen Arazi 2022-04-28 22:56:41 UTC
Created attachment 134317 [details] [review]
Bug 29346: Avoid duplicate actions tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 60 Tomás Cohen Arazi 2022-04-28 22:56:50 UTC
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>
Comment 61 Tomás Cohen Arazi 2022-04-28 22:57:02 UTC
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>
Comment 62 Tomás Cohen Arazi 2022-04-28 22:57:13 UTC
Created attachment 134320 [details] [review]
Bug 29346: (follow-up) Fix typo 'availabe'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Tomás Cohen Arazi 2022-04-28 22:57:24 UTC
Created attachment 134321 [details] [review]
Bug 29346: Add better reporting

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Tomás Cohen Arazi 2022-04-28 22:57:35 UTC
Created attachment 134322 [details] [review]
Bug 29346: Remove unused circular dependency on C4::Search

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 65 Nick Clemens 2022-04-29 01:02:36 UTC
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>
Comment 66 Nick Clemens 2022-04-29 01:02:41 UTC
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>
Comment 67 Nick Clemens 2022-04-29 01:02:46 UTC
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>
Comment 68 Nick Clemens 2022-04-29 01:02:51 UTC
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>
Comment 69 Nick Clemens 2022-04-29 01:02:56 UTC
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>
Comment 70 Nick Clemens 2022-04-29 01:03:01 UTC
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>
Comment 71 Nick Clemens 2022-04-29 01:03:06 UTC
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>
Comment 72 Nick Clemens 2022-04-29 01:03:11 UTC
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>
Comment 73 Nick Clemens 2022-04-29 01:03:15 UTC
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>
Comment 74 Nick Clemens 2022-04-29 01:03:20 UTC
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>
Comment 75 Nick Clemens 2022-04-29 01:03:25 UTC
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>
Comment 76 Nick Clemens 2022-04-29 01:03:30 UTC
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>
Comment 77 Nick Clemens 2022-04-29 01:03:35 UTC
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>
Comment 78 Nick Clemens 2022-04-29 01:03:40 UTC
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>
Comment 79 Nick Clemens 2022-04-29 01:03:45 UTC
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>
Comment 80 Nick Clemens 2022-04-29 01:03:50 UTC
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>
Comment 81 Fridolin Somers 2022-05-05 21:20:38 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 82 Jonathan Druart 2022-05-06 11:59:47 UTC
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.
Comment 83 Nick Clemens 2022-05-06 12:29:08 UTC
(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.
Comment 84 Jonathan Druart 2022-05-06 13:24:01 UTC
(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