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.
Created attachment 131105 [details] [review] Bug 30181: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131106 [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>
Created attachment 131107 [details] [review] Bug 30181: (follow-up) Remove redundant queries Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Missing changes in t/lib/Koha/BackgroundJob/BatchTest.pm
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
Created attachment 131322 [details] [review] Bug 30181: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
Thanks for the valuable feedback, Jonathan. I have submitted a revised patchset.
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>
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>
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>
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>
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.
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>
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>
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>
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>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]