Bug 30181 - Koha::BackgroundJob->_derived_class returns an empty object
Summary: Koha::BackgroundJob->_derived_class returns an empty object
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords: rel_21_11_candidate
Depends on:
Blocks: 29346
  Show dependency treegraph
 
Reported: 2022-02-25 09:39 UTC by Tomás Cohen Arazi
Modified: 2022-11-29 19:02 UTC (History)
7 users (show)

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


Attachments
Bug 30181: Regression tests (2.34 KB, patch)
2022-02-25 09:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30181: Make Koha::BackgroundJob->_derived_class return the right thing (1.20 KB, patch)
2022-02-25 09:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30181: (follow-up) Remove redundant queries (16.43 KB, patch)
2022-02-25 10:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30181: Regression tests (2.34 KB, patch)
2022-03-03 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30181: Make Koha::BackgroundJob->_derived_class return the right thing (1.20 KB, patch)
2022-03-03 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30181: (follow-up) Remove redundant queries and parameters (17.26 KB, patch)
2022-03-03 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30181: (QA follow-up) Update BatchTest (4.49 KB, patch)
2022-03-03 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30181: Regression tests (2.41 KB, patch)
2022-03-16 14:32 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30181: Make Koha::BackgroundJob->_derived_class return the right thing (1.26 KB, patch)
2022-03-16 14:32 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30181: (follow-up) Remove redundant queries and parameters (17.33 KB, patch)
2022-03-16 14:32 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30181: (QA follow-up) Update BatchTest (4.55 KB, patch)
2022-03-16 14:32 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30181: Regression tests (2.46 KB, patch)
2022-03-17 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30181: Make Koha::BackgroundJob->_derived_class return the right thing (1.32 KB, patch)
2022-03-17 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30181: (follow-up) Remove redundant queries and parameters (17.38 KB, patch)
2022-03-17 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30181: (QA follow-up) Update BatchTest (4.61 KB, patch)
2022-03-17 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-02-25 09:39:30 UTC
If you look at Koha::BackgroundJob->process, you will notice some funky stuff. After re-reading how background_jobs_worker.pl does its job, you notice that the main loop can only deal with Koha::BackgrounJob objects (i.e. not Koha::BackgroundJob::BatchUpdateItem). This is because it either gets a job id (STOMP use case) or it uses a Koha::BackgroundJobs->search to find jobs.

So, when a job is found (say $job), the worker eventually calls $job->process. At that point, it needs to instantiate the 'real' class to properly call its overridden ->process method. When it does so, it creates a new/empty object, that is passed... the id. So, devs implementing background jobs need to take care of retrieving the job object so they are able to update the job status.

This is not cool.
Comment 1 Tomás Cohen Arazi 2022-02-25 09:46:55 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2022-02-25 09:46:59 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2022-02-25 10:03:48 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2022-02-28 17:09:37 UTC
Missing changes in t/lib/Koha/BackgroundJob/BatchTest.pm
Comment 5 Jonathan Druart 2022-02-28 17:14:12 UTC
158     return $derived_class->process({job_id => $self->id, %$args});

213     return $derived_class->additional_report({job_id => $self->id});

We do no longer need to pass job_id
Comment 6 Tomás Cohen Arazi 2022-03-03 14:23:01 UTC
Created attachment 131322 [details] [review]
Bug 30181: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2022-03-03 14:23:07 UTC
Created attachment 131323 [details] [review]
Bug 30181: Make Koha::BackgroundJob->_derived_class return the right thing

This patch makes _derived_class rely on _new_from_dbic to generate a new
object with the right class, but based on the same DB row. Not an empty
one as it was before. This way we can remove some biolerplate that is
required now when writing background job classes.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJob.t
=> FAIL: Boo, 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 8 Tomás Cohen Arazi 2022-03-03 14:23:13 UTC
Created attachment 131324 [details] [review]
Bug 30181: (follow-up) Remove redundant queries and parameters

Now $self is actually an instance of the job class, there's no need to
have the job_id parameter passed, or the have the ->process method
re-fetch the object from the database.

This patch cleans things up.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-03-03 14:23:18 UTC
Created attachment 131325 [details] [review]
Bug 30181: (QA follow-up) Update BatchTest

This patch updates t::lib::Koha::BackgroundJob::BatchTest to the new
style, and also removes a couple stray cases in which job_id was still
passed as a parameter.

Tests are rewritten a bit, so they actually test more of the behaviors.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJobs.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2022-03-03 14:24:27 UTC
Thanks for the valuable feedback, Jonathan. I have submitted a revised patchset.
Comment 11 Andrew Fuerste-Henry 2022-03-16 14:32:45 UTC
Created attachment 131799 [details] [review]
Bug 30181: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Andrew Fuerste-Henry 2022-03-16 14:32:50 UTC
Created attachment 131800 [details] [review]
Bug 30181: Make Koha::BackgroundJob->_derived_class return the right thing

This patch makes _derived_class rely on _new_from_dbic to generate a new
object with the right class, but based on the same DB row. Not an empty
one as it was before. This way we can remove some biolerplate that is
required now when writing background job classes.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJob.t
=> FAIL: Boo, 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: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 13 Andrew Fuerste-Henry 2022-03-16 14:32:54 UTC
Created attachment 131801 [details] [review]
Bug 30181: (follow-up) Remove redundant queries and parameters

Now $self is actually an instance of the job class, there's no need to
have the job_id parameter passed, or the have the ->process method
re-fetch the object from the database.

This patch cleans things up.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 14 Andrew Fuerste-Henry 2022-03-16 14:32:57 UTC
Created attachment 131802 [details] [review]
Bug 30181: (QA follow-up) Update BatchTest

This patch updates t::lib::Koha::BackgroundJob::BatchTest to the new
style, and also removes a couple stray cases in which job_id was still
passed as a parameter.

Tests are rewritten a bit, so they actually test more of the behaviors.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJobs.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 15 Andrew Fuerste-Henry 2022-03-16 14:36:53 UTC
Note that the test for the regression tests patch is BackgroundJob.t (singular) while the test for the QA follow-up patch is BackgroundJobs.t (plural). My own inattentive reading confused me here for a bit.
Comment 16 Kyle M Hall 2022-03-17 15:30:43 UTC
Created attachment 131845 [details] [review]
Bug 30181: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2022-03-17 15:30:55 UTC
Created attachment 131846 [details] [review]
Bug 30181: Make Koha::BackgroundJob->_derived_class return the right thing

This patch makes _derived_class rely on _new_from_dbic to generate a new
object with the right class, but based on the same DB row. Not an empty
one as it was before. This way we can remove some biolerplate that is
required now when writing background job classes.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJob.t
=> FAIL: Boo, 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: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2022-03-17 15:30:59 UTC
Created attachment 131847 [details] [review]
Bug 30181: (follow-up) Remove redundant queries and parameters

Now $self is actually an instance of the job class, there's no need to
have the job_id parameter passed, or the have the ->process method
re-fetch the object from the database.

This patch cleans things up.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2022-03-17 15:31:03 UTC
Created attachment 131848 [details] [review]
Bug 30181: (QA follow-up) Update BatchTest

This patch updates t::lib::Koha::BackgroundJob::BatchTest to the new
style, and also removes a couple stray cases in which job_id was still
passed as a parameter.

Tests are rewritten a bit, so they actually test more of the behaviors.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJobs.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Fridolin Somers 2022-03-22 20:19:31 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄