Bug 35979

Summary: Possible RealTimeHoldsQueue check missing in modrequest.pl for BatchUpdateBiblioHoldsQueue background job
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: Pushed to oldstable --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: dcook, fridolin.somers, jonathan.druart, lucas, m.de.rooy, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30728
GIT URL: Change sponsored?: ---
Patch complexity: Trivial patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,23.05.12
Circulation function:
Bug Depends on:    
Bug Blocks: 36675    
Attachments: Bug 35979: Check pref before inserting holds_queue background jobs
Bug 35997: (follow-up) Add check in ->enqueue
Bug 35979: Check pref before inserting holds_queue background jobs
Bug 35979: (follow-up) Add check in ->enqueue
Bug 35979: Check pref before inserting holds_queue background jobs
Bug 35979: (follow-up) Add check in ->enqueue

Description Pedro Amorim 2024-02-01 14:24:02 UTC
Not sure if a bug or not, but bug 30728 introduced a check for the RealTimeHoldsQueue everywhere the BatchUpdateBiblioHoldsQueue background job is enqueued.
However, modrequest.pl enqueues BatchUpdateBiblioHoldsQueue without this check.
Opening this bug to perhaps get confirmation on this or discover what bug this may cause.
Comment 1 Marcel de Rooy 2024-04-23 11:41:37 UTC
(In reply to Pedro Amorim from comment #0)
> Not sure if a bug or not, but bug 30728 introduced a check for the
> RealTimeHoldsQueue everywhere the BatchUpdateBiblioHoldsQueue background job
> is enqueued.
> However, modrequest.pl enqueues BatchUpdateBiblioHoldsQueue without this
> check.
> Opening this bug to perhaps get confirmation on this or discover what bug
> this may cause.

Sure this is a bug. Patch coming. I saw lots of those jobs in my table while not using holds queue..
Comment 2 Marcel de Rooy 2024-04-23 11:45:12 UTC
Created attachment 165380 [details] [review]
Bug 35979: Check pref before inserting holds_queue background jobs

Test plan:
Confirm that modrequest does no longer insert when pref is off :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2024-04-23 11:45:29 UTC
Trivial: Self SO
Comment 4 Marcel de Rooy 2024-04-23 11:47:24 UTC
Grepped all the occurrences before reading this report. And the last one before the tests was this script ;)
Comment 5 Pedro Amorim 2024-04-23 12:32:38 UTC
Not for now, but would be great to have this safe check be done once at BatchUpdateBiblioHoldsQueue::enqueue instead of having to use it every time that particular background job is invoked.
Without a QA script check it's hard to make sure a bug like this won't creep up again.

Another example of a safe check being WET is GetPlugins having to always check for C4::Context->config("enable_plugins") beforehand.

Bit of a tangent, and not for now, sorry!
Thanks, Marcel.
Comment 6 Marcel de Rooy 2024-04-23 12:49:36 UTC
Created attachment 165385 [details] [review]
Bug 35997: (follow-up) Add check in ->enqueue

Foundation for removing same check elsewhere (later on).

Test plan:
Run t/db_dependent/Koha/BackgroundJob/BatchUpdateBiblioHoldsQueue.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-04-23 12:50:35 UTC
(In reply to Pedro Amorim from comment #5)
> Not for now, but would be great to have this safe check be done once at
> BatchUpdateBiblioHoldsQueue::enqueue instead of having to use it every time
> that particular background job is invoked.
> Without a QA script check it's hard to make sure a bug like this won't creep
> up again.
> 
> Another example of a safe check being WET is GetPlugins having to always
> check for C4::Context->config("enable_plugins") beforehand.
> 
> Bit of a tangent, and not for now, sorry!
> Thanks, Marcel.

Follow-up adds foundation. This could be backported. Changing all occurrences is a future enhancement (will open new report).
Comment 8 Pedro Amorim 2024-04-23 13:24:08 UTC Comment hidden (obsolete)
Comment 9 Pedro Amorim 2024-04-23 13:24:11 UTC Comment hidden (obsolete)
Comment 10 Pedro Amorim 2024-04-23 13:25:48 UTC
Created attachment 165397 [details] [review]
Bug 35979: Check pref before inserting holds_queue background jobs

Test plan:
Confirm that modrequest does no longer insert when pref is off :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 11 Pedro Amorim 2024-04-23 13:25:51 UTC
Created attachment 165398 [details] [review]
Bug 35979: (follow-up) Add check in ->enqueue

Foundation for removing same check elsewhere (later on).

Test plan:
Run t/db_dependent/Koha/BackgroundJob/BatchUpdateBiblioHoldsQueue.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

PA amended: Fix bug # in commit message

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 12 Katrin Fischer 2024-04-26 06:53:39 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 13 David Cook 2024-05-07 06:49:38 UTC
What's the symptom for this one? Entries being added needlessly to a table like tmp_holdsqueue?
Comment 14 Marcel de Rooy 2024-05-07 10:05:56 UTC
(In reply to David Cook from comment #13)
> What's the symptom for this one? Entries being added needlessly to a table
> like tmp_holdsqueue?

Yes
Comment 15 Fridolin Somers 2024-05-23 15:09:38 UTC
Pushed to 23.11.x for 23.11.06
Comment 16 Marcel de Rooy 2024-05-27 08:47:05 UTC
*** Bug 36968 has been marked as a duplicate of this bug. ***
Comment 17 Lucas Gass (lukeg) 2024-05-28 20:24:40 UTC
Backported to 23.05.x for upcoming 23.05.12