Bug 35979 - Possible RealTimeHoldsQueue check missing in modrequest.pl for BatchUpdateBiblioHoldsQueue background job
Summary: Possible RealTimeHoldsQueue check missing in modrequest.pl for BatchUpdateBib...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 36675
  Show dependency treegraph
 
Reported: 2024-02-01 14:24 UTC by Pedro Amorim
Modified: 2024-04-26 06:53 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 35979: Check pref before inserting holds_queue background jobs (959 bytes, patch)
2024-04-23 11:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35997: (follow-up) Add check in ->enqueue (2.17 KB, patch)
2024-04-23 12:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35979: Check pref before inserting holds_queue background jobs (921 bytes, patch)
2024-04-23 13:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35979: (follow-up) Add check in ->enqueue (2.18 KB, patch)
2024-04-23 13:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35979: Check pref before inserting holds_queue background jobs (980 bytes, patch)
2024-04-23 13:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35979: (follow-up) Add check in ->enqueue (2.24 KB, patch)
2024-04-23 13:25 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!