Bug 31245 - Job detail view for batch mod explode if job not started
Summary: Job detail view for batch mod explode if job not started
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 31336
  Show dependency treegraph
 
Reported: 2022-07-27 08:33 UTC by Jonathan Druart
Modified: 2022-08-10 13:43 UTC (History)
5 users (show)

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


Attachments
Bug 31245: Do not try to report results if task not started (1.05 KB, patch)
2022-07-27 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 31245: Do not call report for job detail view if job not started (2.22 KB, patch)
2022-07-27 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31245: Do not try to report results if task not started (1.10 KB, patch)
2022-08-03 20:54 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 31245: Do not call report for job detail view if job not started (2.26 KB, patch)
2022-08-03 20:54 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 31245: Do not call report for job detail view if job not started (2.33 KB, patch)
2022-08-10 09:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31245: Do not try to report results if task not started (1.16 KB, patch)
2022-08-10 09:43 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-07-27 08:33:22 UTC
If the job has failed to start (status=new), the job detail view is exploding with 

Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateItem.pm line 175                                                                                                
        Koha::BackgroundJob::BatchUpdateItem::additional_report('Koha::BackgroundJob::BatchUpdateItem=HASH(0x5592c6c6ced8)') called at /kohadevbox/koha/Koha/BackgroundJob.pm line 339                                       Koha::BackgroundJob::additional_report('Koha::BackgroundJob=HASH(0x5592c6df3268)') called at /kohadevbox/koha/admin/background_jobs.pl line 55
Comment 1 Jonathan Druart 2022-07-27 08:35:01 UTC
Created attachment 138169 [details] [review]
Bug 31245: Do not try to report results if task not started

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 2 Jonathan Druart 2022-07-27 08:37:27 UTC
Created attachment 138170 [details] [review]
[ALTERNATIVE-PATCH] Bug 31245: Do not call report for job detail view if job not started

We should not generate the report if the job hasn't started yet

Test plan:
Stop the koha-worker processes, modify a new item using the batch mod
tool, then see the detail page of the job

Without this patch an error is generated:
Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateItem.pm line 175
Comment 3 Jonathan Druart 2022-07-27 08:37:57 UTC
What about this alternative patch, Tomas?
Comment 4 Kyle M Hall 2022-07-27 11:28:35 UTC
(In reply to Jonathan Druart from comment #3)
> What about this alternative patch, Tomas?

Why not both?
Comment 5 Tomás Cohen Arazi 2022-07-27 11:30:39 UTC
I felt like doing it your way, Jonathan, would place unnecessary constraints on what we want to display. Specially for plugins. We could want to display something on other background jobs in the 'new' status.

But I think either solution is fine. If required we can step back later.
Comment 6 Kyle M Hall 2022-07-27 11:37:46 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> I felt like doing it your way, Jonathan, would place unnecessary constraints
> on what we want to display. Specially for plugins. We could want to display
> something on other background jobs in the 'new' status.


^ This is a good point I hadn't thought of
Comment 7 Jonathan Druart 2022-07-28 06:34:09 UTC
I don't understand your use case. "new": the job hasn't started yet. report: what happened, the job has started (still running or failed or succeeded).
Comment 8 Tomás Cohen Arazi 2022-07-29 19:20:06 UTC
(In reply to Jonathan Druart from comment #7)
> I don't understand your use case. "new": the job hasn't started yet. report:
> what happened, the job has started (still running or failed or succeeded).

Ok, let's go ahead with your alternative patch. We can enhance it later if needed.
Comment 9 Andrew Fuerste-Henry 2022-08-03 20:54:12 UTC
Without these patches, I'm getting this same error on master when trying to view a job that's currently underway (status='started'). I'm on a sandbox, so I don't have a way to stop the koha-workers for testing. Here's how I've been testing:

1 - write a report, SELECT itemnumber FROM items. With the default dev data, that's 961 items.
2 - use your report to send all of those to batch mod. The page will take a while to load
3 - while your batch mod page is loading, open background_jobs.pl in another tab and have it ready to go
4 - modify something in all 961 records -- add a public note or something -- once you submit this job it will take about 45 seconds to complete
5 - BEFORE THE JOB IS DONE - reload background_jobs.pl, see your job is listed and shows status=started. try to view details of your job, get an error
6 - once the job has finished, click View again and see your job details successfully

With these patches, I do not get an error. When I try to see the details for a job that's underway, the details page loads fine. It shows the status (started) and progress (122/961) correctly. However, it says "No items modified." This is inaccurate. If we can't load and list the items that have already been successfully modified, I'd rather the message say something like "This job is not yet complete."

All that said, these patches do what they say and correct a messy error, so I'm signing off. Maybe that message can get changed in a followup.
Comment 10 ByWater Sandboxes 2022-08-03 20:54:48 UTC
Created attachment 138550 [details] [review]
Bug 31245: Do not try to report results if task not started

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Andrew <andrewfh@dubcolib.org>
Comment 11 ByWater Sandboxes 2022-08-03 20:54:51 UTC
Created attachment 138551 [details] [review]
Bug 31245: Do not call report for job detail view if job not started

We should not generate the report if the job hasn't started yet

Test plan:
Stop the koha-worker processes, modify a new item using the batch mod
tool, then see the detail page of the job

Without this patch an error is generated:
Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateItem.pm line 175

Signed-off-by: Andrew <andrewfh@dubcolib.org>
Comment 12 Jonathan Druart 2022-08-10 08:56:03 UTC
Upping severity
Comment 13 Martin Renvoize 2022-08-10 09:42:04 UTC
Created attachment 138968 [details] [review]
Bug 31245: Do not call report for job detail view if job not started

We should not generate the report if the job hasn't started yet

Test plan:
Stop the koha-worker processes, modify a new item using the batch mod
tool, then see the detail page of the job

Without this patch an error is generated:
Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateItem.pm line 175

Signed-off-by: Andrew <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2022-08-10 09:43:11 UTC
Created attachment 138969 [details] [review]
Bug 31245: Do not try to report results if task not started

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Andrew <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-08-10 09:44:00 UTC
So, it's belt and braces with both patches.. one to prevent explosion of we somehow call the method still and one to prevent calling the method in the first place.

I'm happy here, Passing QA
Comment 16 Jonathan Druart 2022-08-10 13:43:05 UTC
With bug 31336 it seems that we are still not doing things correctly.

Shouldn't we display the detail only if status != new, started, running?