Bug 32565 - Holds placed when all libraries are closed do not get added to holds queue if HoldsQueueSkipClosed and RealTimeHoldsQueue are enabled
Summary: Holds placed when all libraries are closed do not get added to holds queue if...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal with 10 votes (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
: 34676 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-01-04 15:32 UTC by Emily Lamancusa
Modified: 2024-05-08 18:30 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch set adds a new option "--unallocated" to the build_holds_queue.pl cronjob. This option prevents deletion of the existing queue, and looks for new unassigned holds that may be mapped to available items. There are two intended uses for the option, depending on whether the 'RealTimeHoldsQueue' (RTHQ) system preference is enabled or not. Without RTHQ libraries who want a more static holds queue during the day can run an hourly 'unallocated' cronjob. This will add new holds to the queue as they come in, but allow libraries longer to fill the holds in their existing queue before they move on. The recommendation would then be a nightly full run to rebuild the queue entirely. With RTHQ, libraries could run a nightly 'unallocated' cron to select holds for libraries that were not open on the previous day, and to select holds that have been unsuspended by another cronjob. Current setups will continue to function as before with no change, but libraries may wish to review their options after this upgrade.
Version(s) released in:
24.05.00


Attachments
Bug 32565: WIP (7.23 KB, patch)
2023-03-02 17:03 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32565: WIP (7.29 KB, patch)
2024-01-22 10:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: WIP - Minor fixes to have WIP working (1.45 KB, patch)
2024-01-22 10:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: Add unallocated option to holds queue (9.89 KB, patch)
2024-01-25 12:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32565: Unit tests (3.57 KB, patch)
2024-01-25 12:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32565: Add unallocated option to holds queue (9.95 KB, patch)
2024-01-31 12:07 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: Unit tests (3.63 KB, patch)
2024-01-31 12:07 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: QA Follow-up: Fix tests (2.30 KB, patch)
2024-02-19 12:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: Remove unallocated param from cron script (3.33 KB, patch)
2024-02-20 14:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: Add unallocated option to holds queue (9.95 KB, patch)
2024-04-02 14:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: Unit tests (3.53 KB, patch)
2024-04-02 14:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: QA Follow-up: Fix tests (2.36 KB, patch)
2024-04-02 14:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32565: Add unallocated option to holds queue (9.95 KB, patch)
2024-04-19 11:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32565: Unit tests (3.53 KB, patch)
2024-04-19 11:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32565: (QA follow-up) Fix tests (2.36 KB, patch)
2024-04-19 11:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32565: (follow-up) Tidy (6.32 KB, patch)
2024-04-19 11:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32565: Add unallocated option to holds queue (9.95 KB, patch)
2024-04-29 18:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32565: Unit tests (3.53 KB, patch)
2024-04-29 18:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32565: (QA follow-up) Fix tests (2.36 KB, patch)
2024-04-29 18:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32565: (follow-up) Tidy (6.32 KB, patch)
2024-04-29 18:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa 2023-01-04 15:32:05 UTC
If HoldsQueueSkipClosed is enabled and a hold is placed on a day when all libraries are closed (e.g. a holiday), that hold doesn't get added to any holds queue, because there are no valid queues to add it to at the time. This did not matter with the build-holds-queue cronjob, because the job would just add it to the queue later; however, if RealTimeHoldsQueue is also enabled, that hold will not get added to the holds queue when the libraries reopen unless something triggers a holds queue update on that biblio record, potentially leading to holds stuck in limbo.

The exception is a hold that has copies available at the pickup location - those types of holds will still get added to the holds queue even if all libraries are closed.

To recreate:
1. Enable RealTimeHoldsQueue and set HoldsQueueSkipClosed to satisfy holds from libraries only when they are open
2. Add a holiday to the calendar for all libraries for today
3. Place a biblio-level hold on a biblio record and set the pickup location to a library that has available copies
4. Verify that that hold got added to the holds queue
5. Place a bilbio-level hold on a biblio record where there are no other holds and copies are available at another location, but not the pickup location
6. Verify that that hold was not added to any holds queue
7. Remove the holiday (or wait for the next day) and verify that the hold from step 5 is still not on the holds queue

This could be solved by adding a check to the real-time holds queue to check whether all libraries are closed, and ignore HoldsQueueSkipClosed when that is the case.
Comment 1 Nick Clemens (kidclamp) 2023-01-04 17:37:24 UTC
I think the solution to this one may be server configuration - there is a need to run the holds queue cron daily/nightly to check for holds that may not have had an open library when placed
Comment 2 Katrin Fischer 2023-01-04 18:04:31 UTC
Could the script be modified to only check for missing holds that haven't been allocated yet? Otherwise it will delete the whole table, mess with all the timestamps etc which would be a real set back now that we have RT.
Comment 3 Emily Lamancusa 2023-01-04 18:29:35 UTC
(In reply to Katrin Fischer from comment #2)
> Could the script be modified to only check for missing holds that haven't
> been allocated yet? Otherwise it will delete the whole table, mess with all
> the timestamps etc which would be a real set back now that we have RT.

I second this - in addition to the reasons Katrin mentioned, we've found (at least in testing) that the real-time holds queue also distributes the holds more equitably among our libraries than the cron job, so we'd like to preserve that!
Running a modified holds queue cron would also be more in keeping with the goals of Bug 31995.
Comment 4 Daniel Gaghan 2023-02-14 17:50:58 UTC
We saw this recently at PCCLD, so +1 to fixing this.
Comment 5 Nick Clemens (kidclamp) 2023-03-02 17:03:54 UTC
Created attachment 147659 [details] [review]
Bug 32565: WIP

Add an unallocated option to CreateQueue and pass through as needed
Avoid deletion of the tmp_holdsqueue, and only check holds
and items that are not currently matched

A future hold with a higher priority will still fail here - because the
item may already be assigned, but on next change to the biblio it would
be corrected

This will need unit tests
Comment 6 Nick Clemens (kidclamp) 2023-03-02 17:32:01 UTC
Not taking this one right now, just had an idea to test, so if someone else wants to pickup unit tests please do
Comment 7 Pedro Amorim 2024-01-22 10:49:16 UTC
Created attachment 161231 [details] [review]
Bug 32565: WIP

Add an unallocated option to CreateQueue and pass through as needed
Avoid deletion of the tmp_holdsqueue, and only check holds
and items that are not currently matched

A future hold with a higher priority will still fail here - because the
item may already be assigned, but on next change to the biblio it would
be corrected

This will need unit tests
Comment 8 Pedro Amorim 2024-01-22 10:49:19 UTC
Created attachment 161232 [details] [review]
Bug 32565: WIP - Minor fixes to have WIP working
Comment 9 Pedro Amorim 2024-01-22 11:21:27 UTC
This is my understanding of the suggested solution:
In order to address the problem reported in the original post, one would have to configure the cronjob entry "build_holds_queue.pl --unallocated" on top of the sys pref configurations stated in the original post (HoldsQueueSkipClosed = "open" and RealTimeHoldsQueue = "enabled").
In other words, the patch alone does not prevent the issue, it aims to fix the issue after it has occurred, when the cron runs by allocating the unallocated holds. Please correct me if I'm wrong.

Observations with the patch:
- My understanding is that RealTimeHoldsQueue and the build_holds_queue.pl cronjob are/were designed to be mutually exclusive, or at least for the most part, i.e. either use one of the other (?).
This patch enforces the relationship between the syspref+cronjob, i.e. one would be expected to use both RealTimeHoldsQueue and the build_holds_queue.pl cronjob (at least when HoldsQueueSkipClosed is set to "open").
I think this is confusing. I think we should have one way or the other but, honestly/ideally, just one standard way of doing this.

- With all of the above said, and having tested the patch, I don't think it does what it's designed to do. To recreate, clean k-t-d:
1) Apply both patches
2) Enable RealTimeHoldsQueue and set HoldsQueueSkipClosed to "open"
3) Add a holiday to the calendar for all libraries for today, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Holiday repeated every same day of the week"
-- Click "Copy to all libraries". Hit "Save.
4) Place a biblio-level hold on a biblio record and set the pickup location to a library that has available copies, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=76&borrowernumber=51
-- Click the first "Place hold" button to place the biblio-level hold.
5) Verify that that hold got added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
6) Place a biblio-level hold on a biblio record where there are no other holds and copies are available at another location, but not the pickup location, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=437&borrowernumber=51
-- On the "pickup at" dropdown, pick something else other than "Centerville", e.g. "Fairfield".
-- Click the first "Place hold" button to place the biblio-level hold.
7) Check the holds queue again, notice that this 2nd hold was not added to the queue:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
8) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
9) Notice nothing changed in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
10) Remove the holiday we created previously, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Delete this holiday"
-- Click "Copy to all libraries". Hit "Save.
11) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
12) Notice nothing changed in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1

At this point I would have expected the unallocated hold to have been placed, but it isn't.

13) Lastly, run the cron script without 'unallocated':
perl misc/cronjobs/holds/build_holds_queue.pl --force
14) Notice both holds are now in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1

My understanding is that the downside of running the cron with --force while having the syspref configured as above, is that the whole queue is rebuilt and some items previously allocated may be swapped around (?), and this is why having the cron with --force plus the sysprefs is not recommended. Correct me if I'm wrong please.

I don't have a proposed fix of my own as I don't think I understand any of this well enough to provide one efficiently.
Comment 10 Katrin Fischer 2024-01-22 21:13:39 UTC
Trying to help untangle this, in hope of not making it worse :)

Problem: When all libraries owning a requested item are closed, we are not able to allocate a hold.

Past:
In the past and without RTHoldsQueue the holdsqueue was rebuilt completely. As a result, the unallocated holds would get allocated.

Present:
We now have RTHoldsQueue and try to avoid running the build_holdsqueue additionally, so we need a way to deal with the unallocated holds without still rebuilding the table regularly.

I think having a switch on the build_holdsqueue that does prevent it deleting the table and makes it only add the so far unallocated would be OK.

What could be alternatives?
* Having a separate script: not ideal really as it would be too similar really.
* Have some other way of triggering re-allocation - I don't think we have an option there apart from cron?
Comment 11 Pedro Amorim 2024-01-23 09:52:26 UTC
(In reply to Katrin Fischer from comment #10)
> * Have some other way of triggering re-allocation - I don't think we have an
> option there apart from cron?

Discussed this with Martin, an option would perhaps have it be a "scheduled job" (think background jobs, but scheduled to run at a specific given time instead of whenever possible), but I don't think we're there yet.

Brainstorm idea, but if a hold is placed today and no library is open today, why not have RTHoldsQueue check tomorrow? and the day after that, and so on, until it finds a suitable one? (within a sensible limit) That's what would happen anyway through the cron, right? Maybe we'd have to add a way of flagging the hold that it can't be satisfied immediately as the library is closed but it could show the hold as "pre-allocated". 
This may not be a great idea, but let me know what the problems are with it as I lack the hold queues workflow knowledge required to see it.
Comment 12 Katrin Fischer 2024-01-23 22:34:26 UTC
Pre-allocation will require new logic that we currently not have and has some issues that we could encounter.

Allocation right now can be random (StaticHoldsQueueWeight), static (RandomizeHoldsQueueWeight) or driven by the transport cost matrix (UseTransportCostMatrix). 
Only "open" libraries will be looked at with HoldsQueueSkipClosed.

So we have a complex system.

When pre-allocating, we could do open libraries first, but when all are closed, should we take the duration of the closing into account? It could be just a holiday closure or it could be library that is closed for months. What if the calendar entries were changed after pre-allocating (closing period is extended)? The hold could get stuck in limbo basically forever, while it sits on the shelf in a library close by.

I believe pre-allocating is not good, as we don't have a way to "review" those.

What do you mean by have RTHoldsQueue check tomorrow? 

RTHoldsQueue means that every time a hold/item/biblio is added or updated we trigger a 'review' of the holds queue entry for the bibliographic record concerned. It's triggered by an action - so we'd still need to locate the unallocated and trigger this step for these holds.
Comment 13 Nick Clemens (kidclamp) 2024-01-25 12:32:25 UTC
Created attachment 161393 [details] [review]
Bug 32565: Add unallocated option to holds queue

Add an unallocated option to CreateQueue and pass through as needed
Avoid deletion of the tmp_holdsqueue, and only check holds
and items that are not currently matched

A future hold with a higher priority will still fail here - because the
item may already be assigned, but on next change to the biblio it would
be corrected

To test:
1) Apply both patches
2) Enable RealTimeHoldsQueue and set HoldsQueueSkipClosed to "open"
3) Add a holiday to the calendar for all libraries for today, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Holiday repeated every same day of the week"
-- Click "Copy to all libraries". Hit "Save.
4) Place a biblio-level hold on a biblio record and set the pickup location to a library that has available copies, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=76&borrowernumber=51
-- Click the first "Place hold" button to place the biblio-level hold.
5) Verify that that hold got added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
6) Place a biblio-level hold on a biblio record where there are no other holds and copies are available at another location, but not the pickup location, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=437&borrowernumber=51
-- On the "pickup at" dropdown, pick something else other than "Centerville", e.g. "Fairfield".
-- Click the first "Place hold" button to place the biblio-level hold.
7) Check the holds queue again, notice that this 2nd hold was not added to the queue:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
8) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
9) Notice nothing changed in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
10) Remove the holiday we created previously, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Delete this holiday"
-- Click "Copy to all libraries". Hit "Save.
11) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
12) Confirm the second hold is added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
Comment 14 Nick Clemens (kidclamp) 2024-01-25 12:32:28 UTC
Created attachment 161394 [details] [review]
Bug 32565: Unit tests
Comment 15 Nick Clemens (kidclamp) 2024-01-25 12:41:47 UTC
This is similar to another bug we filed 3467

Pedro, if you want to pursue something different here I am happy to move my patches to that bug. Otherwise we could combine the two
Comment 16 Pedro Amorim 2024-01-31 12:07:22 UTC
Created attachment 161679 [details] [review]
Bug 32565: Add unallocated option to holds queue

Add an unallocated option to CreateQueue and pass through as needed
Avoid deletion of the tmp_holdsqueue, and only check holds
and items that are not currently matched

A future hold with a higher priority will still fail here - because the
item may already be assigned, but on next change to the biblio it would
be corrected

To test:
1) Apply both patches
2) Enable RealTimeHoldsQueue and set HoldsQueueSkipClosed to "open"
3) Add a holiday to the calendar for all libraries for today, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Holiday repeated every same day of the week"
-- Click "Copy to all libraries". Hit "Save.
4) Place a biblio-level hold on a biblio record and set the pickup location to a library that has available copies, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=76&borrowernumber=51
-- Click the first "Place hold" button to place the biblio-level hold.
5) Verify that that hold got added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
6) Place a biblio-level hold on a biblio record where there are no other holds and copies are available at another location, but not the pickup location, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=437&borrowernumber=51
-- On the "pickup at" dropdown, pick something else other than "Centerville", e.g. "Fairfield".
-- Click the first "Place hold" button to place the biblio-level hold.
7) Check the holds queue again, notice that this 2nd hold was not added to the queue:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
8) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
9) Notice nothing changed in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
10) Remove the holiday we created previously, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Delete this holiday"
-- Click "Copy to all libraries". Hit "Save.
11) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
12) Confirm the second hold is added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 17 Pedro Amorim 2024-01-31 12:07:25 UTC
Created attachment 161680 [details] [review]
Bug 32565: Unit tests

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 18 Pedro Amorim 2024-01-31 12:09:47 UTC
(In reply to Nick Clemens from comment #15)
> This is similar to another bug we filed 3467
> 
> Pedro, if you want to pursue something different here I am happy to move my
> patches to that bug. Otherwise we could combine the two

Hey, thanks Nick!
Yeah, I think bug 34676 should definitely be made a duplicate of this.

I've signed-off and I believe this is good to go, although I have 2 observations:
1) With this new enhancement, would there ever be a situation where using RTHQ without the cron build_holds_queue.pl --force --unallocated be advisable?
In other words, it seems that it would always be beneficial to run RTHQ in conjunction with the cron build_holds_queue.pl --force --unallocated, in order to catch unallocated holds "just in case".
If the above is true, we're moving from:
"It's recommended to use RTHQ or the build_holds_queue.pl cron, not both"
to
"RTHQ should be used in conjunction with build_holds_queue.pl --force --unallocated cron".

This works, but it may be confusing in terms of system administration. Maybe not, ignore if that's the case.

2) Should we rename the unallocated option to unallocated-only to make it clearer? We're not adding "unallocated" on top of what the script already does, instead we're making the script consider unallocated holds only, correct? Ignore if not.
Comment 19 Victor Grousset/tuxayo 2024-02-18 01:21:31 UTC
Good questions Pedro.

I do not know enough that functional area to help here. But at least here is a finding on another thing:

test "Test unallocated option" actually doesn't test it.
1. Apply test patch
2. run tests
3. They pass! :o they shouldn't since there is not implementation
Comment 20 Pedro Amorim 2024-02-19 12:18:20 UTC
Created attachment 162269 [details] [review]
Bug 32565: QA Follow-up: Fix tests

Apply original tests patch + this follow-up. Run:
prove t/db_dependent/HoldsQueue.t
Notice tests fail.
Apply fix patch, run the above prove command again.
Notice tests pass.
Comment 21 Pedro Amorim 2024-02-20 14:45:53 UTC
Created attachment 162297 [details] [review]
Bug 32565: Remove unallocated param from cron script

unallocated param is redundant, it is only relevant if RealTimeHoldsQueue is enabled, the cron should just fall back to unallocated mode if the sys pref is enabled.
This simplifies system administration, and does not require current Koha installations to update their cron configuration entry to make use of this fix.

I think we should add a documentation note recommending using this cronjob when RealTimeHoldsQueue is enabled
Comment 22 Pedro Amorim 2024-02-20 14:56:42 UTC
Hey guys, I'd love some feedback on my latest patch.
I think it makes sense considering build_holds_queue.pl --unallocated should be idempotent.
I'm okay with it being dropped if you feel that's best or if I missed something.
Comment 23 Nick Clemens (kidclamp) 2024-02-29 15:39:40 UTC
(In reply to Pedro Amorim from comment #22)
> Hey guys, I'd love some feedback on my latest patch.
> I think it makes sense considering build_holds_queue.pl --unallocated should
> be idempotent.
> I'm okay with it being dropped if you feel that's best or if I missed
> something.

In a system without RTHQ there are times a library would like the queue to remain static for a period of time, but have new holds be picked up. i.e. giving a library more time to fill holds until a full rebuild, so I think the switch is necessary even when not using the RTHQ. Having the switch allows this code to satisfy bug 34676 and 32328 at leats partially
Comment 24 Pedro Amorim 2024-03-01 11:50:11 UTC
(In reply to Nick Clemens from comment #23)
> (In reply to Pedro Amorim from comment #22)
> > Hey guys, I'd love some feedback on my latest patch.
> > I think it makes sense considering build_holds_queue.pl --unallocated should
> > be idempotent.
> > I'm okay with it being dropped if you feel that's best or if I missed
> > something.
> 
> In a system without RTHQ there are times a library would like the queue to
> remain static for a period of time, but have new holds be picked up. i.e.
> giving a library more time to fill holds until a full rebuild, so I think
> the switch is necessary even when not using the RTHQ. Having the switch
> allows this code to satisfy bug 34676 and 32328 at leats partially

I've made my latest patch obsolete for these reasons, I had not considered this specific use case.
As it stands now, the following cron entries configurations will work like this:

Without RTHQ:
0 * * * * /usr/share/koha/bin/cronjobs/holds/build_holds_queue.pl --unallocated-only # run this hourly
0 1 * * * /usr/share/koha/bin/cronjobs/holds/build_holds_queue.pl # run this daily

With RTHQ:
0 * * * * /usr/share/koha/bin/cronjobs/holds/build_holds_queue.pl --unallocated-only # run this hourly
0 1 * * * /usr/share/koha/bin/cronjobs/holds/build_holds_queue.pl # run this daily !This will not do anything because RTHQ is enabled and --force is missing

Nick, please intervene if any of the above needs correcting.
My opinion is that a note along the lines of "It is recommended to use the cron build_holds_queue.pl --unallocated-only cron if using RTHQ" would be useful in the UI.

I think this is good to go and I don't have anything else to add, but because of my involvement in it I'll leave it to someone else to PQA.
Comment 25 Victor Grousset/tuxayo 2024-03-01 21:32:15 UTC
(In reply to Pedro Amorim from comment #20)
> Created attachment 162269 [details] [review] [review]
> Bug 32565: QA Follow-up: Fix tests
> 
> Apply original tests patch + this follow-up. Run:
> prove t/db_dependent/HoldsQueue.t
> Notice tests fail.
> Apply fix patch, run the above prove command again.
> Notice tests pass.

Confirmed :)
Comment 26 Pedro Amorim 2024-04-02 14:03:03 UTC
Created attachment 164272 [details] [review]
Bug 32565: Add unallocated option to holds queue

Add an unallocated option to CreateQueue and pass through as needed
Avoid deletion of the tmp_holdsqueue, and only check holds
and items that are not currently matched

A future hold with a higher priority will still fail here - because the
item may already be assigned, but on next change to the biblio it would
be corrected

To test:
1) Apply both patches
2) Enable RealTimeHoldsQueue and set HoldsQueueSkipClosed to "open"
3) Add a holiday to the calendar for all libraries for today, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Holiday repeated every same day of the week"
-- Click "Copy to all libraries". Hit "Save.
4) Place a biblio-level hold on a biblio record and set the pickup location to a library that has available copies, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=76&borrowernumber=51
-- Click the first "Place hold" button to place the biblio-level hold.
5) Verify that that hold got added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
6) Place a biblio-level hold on a biblio record where there are no other holds and copies are available at another location, but not the pickup location, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=437&borrowernumber=51
-- On the "pickup at" dropdown, pick something else other than "Centerville", e.g. "Fairfield".
-- Click the first "Place hold" button to place the biblio-level hold.
7) Check the holds queue again, notice that this 2nd hold was not added to the queue:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
8) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
9) Notice nothing changed in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
10) Remove the holiday we created previously, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Delete this holiday"
-- Click "Copy to all libraries". Hit "Save.
11) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
12) Confirm the second hold is added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 27 Pedro Amorim 2024-04-02 14:03:06 UTC
Created attachment 164273 [details] [review]
Bug 32565: Unit tests

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 28 Pedro Amorim 2024-04-02 14:03:10 UTC
Created attachment 164274 [details] [review]
Bug 32565: QA Follow-up: Fix tests

Apply original tests patch + this follow-up. Run:
prove t/db_dependent/HoldsQueue.t
Notice tests fail.
Apply fix patch, run the above prove command again.
Notice tests pass.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 29 Pedro Amorim 2024-04-02 14:03:41 UTC
HoldsQueue.t needed a rebase after bug 35997
Comment 30 Nick Clemens (kidclamp) 2024-04-04 14:35:44 UTC
*** Bug 34676 has been marked as a duplicate of this bug. ***
Comment 31 Nick Clemens (kidclamp) 2024-04-19 11:27:06 UTC
Created attachment 165166 [details] [review]
Bug 32565: Add unallocated option to holds queue

Add an unallocated option to CreateQueue and pass through as needed
Avoid deletion of the tmp_holdsqueue, and only check holds
and items that are not currently matched

A future hold with a higher priority will still fail here - because the
item may already be assigned, but on next change to the biblio it would
be corrected

To test:
1) Apply both patches
2) Enable RealTimeHoldsQueue and set HoldsQueueSkipClosed to "open"
3) Add a holiday to the calendar for all libraries for today, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Holiday repeated every same day of the week"
-- Click "Copy to all libraries". Hit "Save.
4) Place a biblio-level hold on a biblio record and set the pickup location to a library that has available copies, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=76&borrowernumber=51
-- Click the first "Place hold" button to place the biblio-level hold.
5) Verify that that hold got added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
6) Place a biblio-level hold on a biblio record where there are no other holds and copies are available at another location, but not the pickup location, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=437&borrowernumber=51
-- On the "pickup at" dropdown, pick something else other than "Centerville", e.g. "Fairfield".
-- Click the first "Place hold" button to place the biblio-level hold.
7) Check the holds queue again, notice that this 2nd hold was not added to the queue:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
8) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
9) Notice nothing changed in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
10) Remove the holiday we created previously, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Delete this holiday"
-- Click "Copy to all libraries". Hit "Save.
11) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
12) Confirm the second hold is added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 32 Nick Clemens (kidclamp) 2024-04-19 11:27:09 UTC
Created attachment 165167 [details] [review]
Bug 32565: Unit tests

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 33 Nick Clemens (kidclamp) 2024-04-19 11:27:12 UTC
Created attachment 165168 [details] [review]
Bug 32565: (QA follow-up) Fix tests

Apply original tests patch + this follow-up. Run:
prove t/db_dependent/HoldsQueue.t
Notice tests fail.
Apply fix patch, run the above prove command again.
Notice tests pass.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 34 Nick Clemens (kidclamp) 2024-04-19 11:27:14 UTC
Created attachment 165169 [details] [review]
Bug 32565: (follow-up) Tidy
Comment 35 Tomás Cohen Arazi 2024-04-29 18:45:53 UTC
Created attachment 165800 [details] [review]
Bug 32565: Add unallocated option to holds queue

Add an unallocated option to CreateQueue and pass through as needed
Avoid deletion of the tmp_holdsqueue, and only check holds
and items that are not currently matched

A future hold with a higher priority will still fail here - because the
item may already be assigned, but on next change to the biblio it would
be corrected

To test:
1) Apply both patches
2) Enable RealTimeHoldsQueue and set HoldsQueueSkipClosed to "open"
3) Add a holiday to the calendar for all libraries for today, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Holiday repeated every same day of the week"
-- Click "Copy to all libraries". Hit "Save.
4) Place a biblio-level hold on a biblio record and set the pickup location to a library that has available copies, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=76&borrowernumber=51
-- Click the first "Place hold" button to place the biblio-level hold.
5) Verify that that hold got added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
6) Place a biblio-level hold on a biblio record where there are no other holds and copies are available at another location, but not the pickup location, visit:
-- /cgi-bin/koha/reserve/request.pl?biblionumber=437&borrowernumber=51
-- On the "pickup at" dropdown, pick something else other than "Centerville", e.g. "Fairfield".
-- Click the first "Place hold" button to place the biblio-level hold.
7) Check the holds queue again, notice that this 2nd hold was not added to the queue:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
8) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
9) Notice nothing changed in the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1
10) Remove the holiday we created previously, visit:
/cgi-bin/koha/tools/holidays.pl
-- Click today's day on the calendar and pick "Delete this holiday"
-- Click "Copy to all libraries". Hit "Save.
11) Run the updated cronscript:
perl misc/cronjobs/holds/build_holds_queue.pl --force --unallocated
12) Confirm the second hold is added to the holds queue, visit:
/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&ccodeslimit=&locationslimit=&run_report=1

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 36 Tomás Cohen Arazi 2024-04-29 18:45:57 UTC
Created attachment 165801 [details] [review]
Bug 32565: Unit tests

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 37 Tomás Cohen Arazi 2024-04-29 18:46:01 UTC
Created attachment 165802 [details] [review]
Bug 32565: (QA follow-up) Fix tests

Apply original tests patch + this follow-up. Run:
prove t/db_dependent/HoldsQueue.t
Notice tests fail.
Apply fix patch, run the above prove command again.
Notice tests pass.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 38 Tomás Cohen Arazi 2024-04-29 18:46:04 UTC
Created attachment 165803 [details] [review]
Bug 32565: (follow-up) Tidy
Comment 39 Tomás Cohen Arazi 2024-04-29 18:47:26 UTC
(In reply to Pedro Amorim from comment #24)
> 
> Nick, please intervene if any of the above needs correcting.
> My opinion is that a note along the lines of "It is recommended to use the
> cron build_holds_queue.pl --unallocated-only cron if using RTHQ" would be
> useful in the UI.

Agreed. And we need good release notes as well.
Comment 40 Katrin Fischer 2024-04-30 12:33:06 UTC
Pushed for 24.05!

Well done everyone, thank you!