Bug 30982 - Use the REST API for background job list view
Summary: Use the REST API for background job list view
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords: rel_22_05_candidate
Depends on: 30462 31565
Blocks: 31666 32580 32596
  Show dependency treegraph
 
Reported: 2022-06-17 08:46 UTC by Jonathan Druart
Modified: 2023-12-28 20:42 UTC (History)
11 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:
22.11.00, 22.05.07


Attachments
Bug 30982: REST API specs (7.16 KB, patch)
2022-06-17 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30982: Use the REST API for background job list view (18.18 KB, patch)
2022-06-17 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30982: REST API specs (7.21 KB, patch)
2022-06-28 07:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Use the REST API for background job list view (18.24 KB, patch)
2022-06-28 07:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Double quoting and console.log (3.93 KB, patch)
2022-06-28 07:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Add tests and implement GET /background_jobs/$id (6.69 KB, patch)
2022-06-29 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30982: Try allow-owner (2.54 KB, patch)
2022-06-29 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30982: Make code more re-usable (2.61 KB, patch)
2022-06-29 14:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Add 'context' to the REST API specs (1.80 KB, patch)
2022-07-26 06:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30982: REST API specs (7.16 KB, patch)
2022-08-23 00:31 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30982: Use the REST API for background job list view (18.24 KB, patch)
2022-08-23 00:31 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Double quoting and console.log (3.93 KB, patch)
2022-08-23 00:31 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30982: Add tests and implement GET /background_jobs/$id (6.68 KB, patch)
2022-08-23 00:31 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30982: Make code more re-usable (2.61 KB, patch)
2022-08-23 00:31 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30982: Add 'context' to the REST API specs (1.79 KB, patch)
2022-08-23 00:31 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30982: REST API specs (7.16 KB, patch)
2022-09-12 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: Use the REST API for background job list view (12.08 KB, patch)
2022-09-12 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Double quoting and console.log (3.90 KB, patch)
2022-09-12 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: Add tests and implement GET /background_jobs/$id (6.68 KB, patch)
2022-09-12 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: Make code more re-usable (2.61 KB, patch)
2022-09-12 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: Add 'context' to the REST API specs (1.79 KB, patch)
2022-09-12 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: Add Koha::BackgroundJobs->filter_by_current (3.40 KB, patch)
2022-09-12 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: API tweaks (17.12 KB, patch)
2022-09-12 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: (follow-up) Only escape asterisk chars when non-exact search (2.07 KB, patch)
2022-09-12 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: Adapt table to new API spec (13.39 KB, patch)
2022-09-12 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[OPTIONAL] Bug 30982: Rename 'Background Jobs' => 'Jobs' (3.33 KB, patch)
2022-09-12 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30982: REST API specs (7.16 KB, patch)
2022-09-13 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Use the REST API for background job list view (12.08 KB, patch)
2022-09-13 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Double quoting and console.log (3.96 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Add tests and implement GET /background_jobs/$id (6.75 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Make code more re-usable (2.67 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Add 'context' to the REST API specs (1.86 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Add Koha::BackgroundJobs->filter_by_current (3.46 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: API tweaks (17.18 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: (follow-up) Only escape asterisk chars when non-exact search (2.14 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Adapt table to new API spec (13.46 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: Rename 'Background Jobs' => 'Jobs' (3.39 KB, patch)
2022-09-13 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Resolve pod warning (934 bytes, patch)
2022-09-20 14:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: REST API specs (7.26 KB, patch)
2022-09-23 09:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: Use the REST API for background job list view (12.18 KB, patch)
2022-09-23 09:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Double quoting and console.log (4.06 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: Add tests and implement GET /background_jobs/$id (6.84 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: Make code more re-usable (2.77 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: Add 'context' to the REST API specs (1.95 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: Add Koha::BackgroundJobs->filter_by_current (3.56 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: API tweaks (17.28 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: Adapt table to new API spec (13.55 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: Rename 'Background Jobs' => 'Jobs' (3.48 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Resolve pod warning (934 bytes, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Spelling (2.48 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) Remove redundancy from template (5.56 KB, patch)
2022-09-23 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: (QA follow-up) No userenv, no jobs (3.17 KB, patch)
2022-09-23 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30982: [22.05.x] Use the REST API for background job list view (45.46 KB, patch)
2022-11-01 11:36 UTC, Nick Clemens
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-06-17 08:46:03 UTC
We should implement and use a REST API route to retrieve the background job list.
Comment 1 Jonathan Druart 2022-06-17 09:15:08 UTC
Created attachment 136245 [details] [review]
Bug 30982: REST API specs
Comment 2 Jonathan Druart 2022-06-17 09:15:12 UTC
Created attachment 136246 [details] [review]
Bug 30982: Use the REST API for background job list view
Comment 3 Jonathan Druart 2022-06-17 09:15:35 UTC
Would that work for you, Martin?
Comment 4 Martin Renvoize 2022-06-28 07:12:14 UTC
Created attachment 136670 [details] [review]
Bug 30982: REST API specs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-06-28 07:12:17 UTC
Created attachment 136671 [details] [review]
Bug 30982: Use the REST API for background job list view

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-06-28 07:12:21 UTC
Created attachment 136672 [details] [review]
Bug 30982: (QA follow-up) Double quoting and console.log

This patch fixes the issues highlighted by the QA script; We use double
quotes for translatable strings in JS and remove an errant console.log
call.
Comment 7 Martin Renvoize 2022-06-28 07:12:49 UTC
This is great, much cleaner!

Signing off.
Comment 8 Martin Renvoize 2022-06-28 08:22:20 UTC
Comment on attachment 136671 [details] [review]
Bug 30982: Use the REST API for background job list view

Review of attachment 136671 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/BackgroundJobs.pm
@@ +43,5 @@
> +        my $background_jobs_set =
> +          $can_manage_background_jobs
> +          ? Koha::BackgroundJobs->new
> +          : Koha::BackgroundJobs->search(
> +            { borrowernumber => $patron->borrowernumber } );

This feels like it should be achievable with the 'allow_owner: true' swagger definition rather than needing special handling here.
Comment 9 Jonathan Druart 2022-06-29 09:41:02 UTC
Created attachment 136707 [details] [review]
Bug 30982: Add tests and implement GET /background_jobs/$id
Comment 10 Jonathan Druart 2022-06-29 09:41:06 UTC
Created attachment 136708 [details] [review]
Bug 30982: Try allow-owner
Comment 11 Jonathan Druart 2022-06-29 09:42:07 UTC
Martin, can you have a look at the last patch please? I've tried to implement your suggestion but either it does not work or I don't understand how it's supposed to work.

I've added missing tests and implement the GET /background_jobs/$id route.
Comment 12 Jonathan Druart 2022-06-29 10:12:02 UTC
From IRC:
@ashimema> ack.. looking at the code it's allow-owner that I've discussed before and bemoaned it's a bit of a mess.
@ashimema> I'm thinking drop that last patch as you've proved it doesn't work right now.. and we can follow through on that stuff in another way..
Comment 13 Martin Renvoize 2022-06-29 14:39:24 UTC
Created attachment 136715 [details] [review]
Bug 30982: Make code more re-usable
Comment 14 Jonathan Druart 2022-07-06 09:48:26 UTC
(In reply to Martin Renvoize from comment #13)
> Created attachment 136715 [details] [review] [review]
> Bug 30982: Make code more re-usable

Shouldn't you call search_limited instead of search then?
Comment 15 Martin Renvoize 2022-07-06 10:05:11 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Martin Renvoize from comment #13)
> > Created attachment 136715 [details] [review] [review] [review]
> > Bug 30982: Make code more re-usable
> 
> Shouldn't you call search_limited instead of search then?

We do so internally: https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/REST/Plugin/Objects.pm#L189-L191

So $c->objects->search is calling a mojolicious helper method above.. as you can see it looks for the presence of a search_limited in the plural object and calls it in place of search if one is found.
Comment 16 Martin Renvoize 2022-07-20 14:19:57 UTC
I think this is pretty much ready to go.. just needs a QA eye.. which I can't lend as I SO'd :(
Comment 17 Katrin Fischer 2022-07-24 22:08:51 UTC
Can you please update the patches with some descriptions and a test plan?
Comment 18 Jonathan Druart 2022-07-25 07:29:04 UTC
Those patches are simply using a new REST API routes to search and display the jobs. It make use of the DT wrapper to let the user search and filter the table, like we do in other places.

There is nothing much I could add.

Test plan:
Add new jobs and filter the table (?)
Comment 19 Katrin Fischer 2022-07-25 21:36:56 UTC
Applied the patches, restart_all:

* The page loads ok, table is empty in my installation
* I check the "Only include jobs started in the last hour" checkbox:
  Error: Something went wrong when loading the table.
  500: Internal Server Error

In my background_jobs table I have several entries, one for batch_item_record_modification and several update_elastic_index. All are finished.
Comment 20 Jonathan Druart 2022-07-26 06:45:02 UTC
Created attachment 138114 [details] [review]
Bug 30982: Add 'context' to the REST API specs

context has been added by bug 30889
Comment 21 Victor Grousset/tuxayo 2022-08-23 00:31:13 UTC
Created attachment 139636 [details] [review]
Bug 30982: REST API specs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Victor Grousset/tuxayo 2022-08-23 00:31:18 UTC
Created attachment 139637 [details] [review]
Bug 30982: Use the REST API for background job list view

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Victor Grousset/tuxayo 2022-08-23 00:31:23 UTC
Created attachment 139638 [details] [review]
Bug 30982: (QA follow-up) Double quoting and console.log

This patch fixes the issues highlighted by the QA script; We use double
quotes for translatable strings in JS and remove an errant console.log
call.
Comment 24 Victor Grousset/tuxayo 2022-08-23 00:31:27 UTC
Created attachment 139639 [details] [review]
Bug 30982: Add tests and implement GET /background_jobs/$id
Comment 25 Victor Grousset/tuxayo 2022-08-23 00:31:33 UTC
Created attachment 139640 [details] [review]
Bug 30982: Make code more re-usable
Comment 26 Victor Grousset/tuxayo 2022-08-23 00:31:38 UTC
Created attachment 139641 [details] [review]
Bug 30982: Add 'context' to the REST API specs

context has been added by bug 30889
Comment 27 Victor Grousset/tuxayo 2022-08-23 00:33:42 UTC
Rebased to fix conflict. Tests still pass. (t/db_dependent/api/v1/background_jobs.t)
Comment 28 Victor Grousset/tuxayo 2022-08-23 00:48:26 UTC
(In reply to Katrin Fischer from comment #19)
> Applied the patches, restart_all:
> 
> * The page loads ok, table is empty in my installation
> * I check the "Only include jobs started in the last hour" checkbox:
>   Error: Something went wrong when loading the table.
>   500: Internal Server Error
> 
> In my background_jobs table I have several entries, one for
> batch_item_record_modification and several update_elastic_index. All are
> finished.

I don't get the issue. Tested with empty db table and after checking in an item of a record on hold I get a Holds queue update and Update Elasticsearch index entry.

(why is there an ES entry update?)

The change here since your test doesn't look like to be able to fix that. And there were no changes on bug 30462.
Comment 29 Katrin Fischer 2022-08-23 07:20:29 UTC
Maybe another patch/bug changed things?
Comment 30 Joonas Kylmälä 2022-09-04 10:26:55 UTC
The table filter "Type" doesn't work, if you select something else than the empty filter you get no results.
Comment 31 Joonas Kylmälä 2022-09-04 10:35:07 UTC
(In reply to Victor Grousset/tuxayo from comment #28)
> (In reply to Katrin Fischer from comment #19)
> > Applied the patches, restart_all:
> > 
> > * The page loads ok, table is empty in my installation
> > * I check the "Only include jobs started in the last hour" checkbox:
> >   Error: Something went wrong when loading the table.
> >   500: Internal Server Error
> > 
> > In my background_jobs table I have several entries, one for
> > batch_item_record_modification and several update_elastic_index. All are
> > finished.
> 
> I don't get the issue. Tested with empty db table and after checking in an
> item of a record on hold I get a Holds queue update and Update Elasticsearch
> index entry.
> 
> (why is there an ES entry update?)
> 
> The change here since your test doesn't look like to be able to fix that.
> And there were no changes on bug 30462.

Unable to reproduce here as well, so maybe fixed in the dependency or there was some configuration issue in the setup katrin used, although if so, then I think we have a bug somewhere.
Comment 32 Tomás Cohen Arazi 2022-09-12 18:08:26 UTC
Created attachment 140488 [details] [review]
Bug 30982: REST API specs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Tomás Cohen Arazi 2022-09-12 18:08:32 UTC
Created attachment 140489 [details] [review]
Bug 30982: Use the REST API for background job list view

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Tomás Cohen Arazi 2022-09-12 18:08:39 UTC
Created attachment 140490 [details] [review]
Bug 30982: (QA follow-up) Double quoting and console.log

This patch fixes the issues highlighted by the QA script; We use double
quotes for translatable strings in JS and remove an errant console.log
call.
Comment 35 Tomás Cohen Arazi 2022-09-12 18:08:45 UTC
Created attachment 140491 [details] [review]
Bug 30982: Add tests and implement GET /background_jobs/$id
Comment 36 Tomás Cohen Arazi 2022-09-12 18:08:51 UTC
Created attachment 140492 [details] [review]
Bug 30982: Make code more re-usable
Comment 37 Tomás Cohen Arazi 2022-09-12 18:08:56 UTC
Created attachment 140493 [details] [review]
Bug 30982: Add 'context' to the REST API specs

context has been added by bug 30889
Comment 38 Tomás Cohen Arazi 2022-09-12 18:09:01 UTC
Created attachment 140494 [details] [review]
Bug 30982: Add Koha::BackgroundJobs->filter_by_current

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2022-09-12 18:09:07 UTC
Created attachment 140495 [details] [review]
Bug 30982: API tweaks

This patch makes the following changes to the 'background_jobs' API:

* We now call them 'jobs'
* Removed deprecated query parameter definitions
* Added only_current query parameter
* Controller gets adapted to use $rs->filter_by_current when
  only_current is passed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2022-09-12 18:09:13 UTC
Created attachment 140496 [details] [review]
Bug 30982: (follow-up) Only escape asterisk chars when non-exact search

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2022-09-12 18:09:38 UTC
Created attachment 140497 [details] [review]
Bug 30982: Adapt table to new API spec

Disclaimer: this patch is highly opinionated :-D

When I started looking at this patch I felt like the two tables
(current/past jobs) implemented on bug 30462 was the way to go.

In order to make this patches apply after it I had to redo all the
things. Or most of them.

But I decided to keep the idea of filtering out completed tasks, not
just having the option to display 'the last hour' tasks. For the task I
added some required helper methods and the relevant tests as well. So a
behavior change.

Hope you all agree with it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi 2022-09-12 18:09:59 UTC
Created attachment 140498 [details] [review]
[OPTIONAL] Bug 30982: Rename 'Background Jobs' => 'Jobs'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Tomás Cohen Arazi 2022-09-12 18:11:43 UTC
Please review ASAP for pushing it.

Thanks everyone!
Comment 44 Tomás Cohen Arazi 2022-09-12 18:13:00 UTC
(In reply to Joonas Kylmälä from comment #30)
> The table filter "Type" doesn't work, if you select something else than the
> empty filter you get no results.

Solved on 'Bug 30982: (follow-up) Only escape asterisk chars when non-exact search'
Comment 45 Martin Renvoize 2022-09-13 14:56:53 UTC
Created attachment 140589 [details] [review]
Bug 30982: REST API specs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2022-09-13 14:56:58 UTC
Created attachment 140590 [details] [review]
Bug 30982: Use the REST API for background job list view

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2022-09-13 14:57:04 UTC
Created attachment 140591 [details] [review]
Bug 30982: (QA follow-up) Double quoting and console.log

This patch fixes the issues highlighted by the QA script; We use double
quotes for translatable strings in JS and remove an errant console.log
call.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2022-09-13 14:57:09 UTC
Created attachment 140592 [details] [review]
Bug 30982: Add tests and implement GET /background_jobs/$id

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2022-09-13 14:57:14 UTC
Created attachment 140593 [details] [review]
Bug 30982: Make code more re-usable

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2022-09-13 14:57:19 UTC
Created attachment 140594 [details] [review]
Bug 30982: Add 'context' to the REST API specs

context has been added by bug 30889

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2022-09-13 14:57:24 UTC
Created attachment 140595 [details] [review]
Bug 30982: Add Koha::BackgroundJobs->filter_by_current

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2022-09-13 14:57:29 UTC
Created attachment 140596 [details] [review]
Bug 30982: API tweaks

This patch makes the following changes to the 'background_jobs' API:

* We now call them 'jobs'
* Removed deprecated query parameter definitions
* Added only_current query parameter
* Controller gets adapted to use $rs->filter_by_current when
  only_current is passed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize 2022-09-13 14:57:34 UTC
Created attachment 140597 [details] [review]
Bug 30982: (follow-up) Only escape asterisk chars when non-exact search

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2022-09-13 14:57:39 UTC
Created attachment 140598 [details] [review]
Bug 30982: Adapt table to new API spec

Disclaimer: this patch is highly opinionated :-D

When I started looking at this patch I felt like the two tables
(current/past jobs) implemented on bug 30462 was the way to go.

In order to make this patches apply after it I had to redo all the
things. Or most of them.

But I decided to keep the idea of filtering out completed tasks, not
just having the option to display 'the last hour' tasks. For the task I
added some required helper methods and the relevant tests as well. So a
behavior change.

Hope you all agree with it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2022-09-13 14:57:44 UTC
Created attachment 140599 [details] [review]
Bug 30982: Rename 'Background Jobs' => 'Jobs'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2022-09-13 15:05:10 UTC
I really like the follow-ups here and think the end result is nicely polished in general.

We could perhaps continue to refine UI was at a later stage after the staff client UI refresh.. things like the checkboxes for the set filter on the datatable feel slightly inconsistent/out of place and we do it inconsistently elsewhere too.  I wonder if 'Pills' might be a nice generic approach.. I digress.

As for the API, I think generally it's an improvement. I again wonder slightly about the quick filtering options tied to additional parameters, but so long as they're well documented on the spec I think they mostly make sense.  It's an area I've contemplated a few times now and not come up with a nice solution for.

Signing off
Comment 57 Tomás Cohen Arazi 2022-09-19 18:10:11 UTC
Comment on attachment 140597 [details] [review]
Bug 30982: (follow-up) Only escape asterisk chars when non-exact search

Obsoleting patch as of bug 31565.
Comment 58 Marcel de Rooy 2022-09-20 12:40:56 UTC
If no one beats me, I will qa friday
Comment 59 Marcel de Rooy 2022-09-20 14:48:50 UTC
Created attachment 140789 [details] [review]
Bug 30982: (QA follow-up) Resolve pod warning

Empty section in previous paragraph at line 32 in file Koha/BackgroundJobs.pm

Test plan:
Run podchecker again on this module.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 60 Marcel de Rooy 2022-09-20 14:56:06 UTC
My first impression for this patch
Subject: [PATCH 10/11] Bug 30982: Rename 'Background Jobs' => 'Jobs'
is:
No. This does not make things clearer to me. What is Jobs. New positions within library staff? :)
Just curious if I change opinion while going on.

As a side note, only changing two templates for this will not make it easier to switch terminology.
Example:
             [% IF CAN_user_parameters_manage_background_jobs %]
-                <h3>Background jobs</h3>
+                <h3>Jobs</h3>
We will be constantly thinking jobs or background_jobs while editing code..
Comment 61 Marcel de Rooy 2022-09-20 15:00:07 UTC
Resuming later on
Comment 62 Marcel de Rooy 2022-09-23 07:08:58 UTC
Working here now
Comment 63 Marcel de Rooy 2022-09-23 07:22:57 UTC
No blocker, not fixing it here:

        const job_statuses = [
            {'_id': 'new',       '_str': _("New")},
            {'_id': 'cancelled', '_str': _("Cancelled")},
            {'_id': 'finished',  '_str': _("Finished")},
            {'_id': 'started',   '_str': _("Started")},
            {'_id': 'running',   '_str': _("Running")},
            {'_id': 'failed',    '_str': _("Failed")},

We have status running and started. Probably an error.
The status 'running' may probably be coming from C4::BackgroundJob.pm
    $self->{'status'} = "running";
It seems that Koha::BackgroundJob does not use 'running'.
Comment 64 Marcel de Rooy 2022-09-23 07:25:32 UTC
> Disclaimer: this patch is highly opinionated :-D

I personally think that that is highly exaggerated :)
Comment 65 Marcel de Rooy 2022-09-23 09:44:53 UTC
Created attachment 140884 [details] [review]
Bug 30982: REST API specs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Marcel de Rooy 2022-09-23 09:44:59 UTC
Created attachment 140885 [details] [review]
Bug 30982: Use the REST API for background job list view

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Marcel de Rooy 2022-09-23 09:45:05 UTC
Created attachment 140886 [details] [review]
Bug 30982: (QA follow-up) Double quoting and console.log

This patch fixes the issues highlighted by the QA script; We use double
quotes for translatable strings in JS and remove an errant console.log
call.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Marcel de Rooy 2022-09-23 09:45:10 UTC
Created attachment 140887 [details] [review]
Bug 30982: Add tests and implement GET /background_jobs/$id

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 69 Marcel de Rooy 2022-09-23 09:45:15 UTC
Created attachment 140888 [details] [review]
Bug 30982: Make code more re-usable

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2022-09-23 09:45:21 UTC
Created attachment 140889 [details] [review]
Bug 30982: Add 'context' to the REST API specs

context has been added by bug 30889

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 71 Marcel de Rooy 2022-09-23 09:45:26 UTC
Created attachment 140890 [details] [review]
Bug 30982: Add Koha::BackgroundJobs->filter_by_current

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2022-09-23 09:45:31 UTC
Created attachment 140891 [details] [review]
Bug 30982: API tweaks

This patch makes the following changes to the 'background_jobs' API:

* We now call them 'jobs'
* Removed deprecated query parameter definitions
* Added only_current query parameter
* Controller gets adapted to use $rs->filter_by_current when
  only_current is passed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 73 Marcel de Rooy 2022-09-23 09:45:36 UTC
Created attachment 140892 [details] [review]
Bug 30982: Adapt table to new API spec

Disclaimer: this patch is highly opinionated :-D

When I started looking at this patch I felt like the two tables
(current/past jobs) implemented on bug 30462 was the way to go.

In order to make this patches apply after it I had to redo all the
things. Or most of them.

But I decided to keep the idea of filtering out completed tasks, not
just having the option to display 'the last hour' tasks. For the task I
added some required helper methods and the relevant tests as well. So a
behavior change.

Hope you all agree with it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Marcel de Rooy 2022-09-23 09:45:41 UTC
Created attachment 140893 [details] [review]
Bug 30982: Rename 'Background Jobs' => 'Jobs'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 75 Marcel de Rooy 2022-09-23 09:45:47 UTC
Created attachment 140894 [details] [review]
Bug 30982: (QA follow-up) Resolve pod warning

Empty section in previous paragraph at line 32 in file Koha/BackgroundJobs.pm

Test plan:
Run podchecker again on this module.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 76 Marcel de Rooy 2022-09-23 09:45:52 UTC
Created attachment 140895 [details] [review]
Bug 30982: (QA follow-up) Spelling

[1] Correct: BackgrounJob
[2] If should filter out not current jobs
=> Had a hard time reading that one until I replaced if by it.
=> Decided to rephrase it in a more positive way.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 77 Marcel de Rooy 2022-09-23 09:45:57 UTC
Created attachment 140896 [details] [review]
Bug 30982: (QA follow-up) Remove redundancy from template

The template now contains two lists for both status and type:
a TT list and a JS list. The type list already proves that
redundancy leads to bugs. We miss three types at one side:
    Unknown job type 'stage_marc_for_import'
    Unknown job type 'marc_import_commit_batch'
    Unknown job type 'marc_import_revert_batch'

This patch removes the TT list. And gets the status and type
via an additional js call. For that reason I hide the fieldset
until document ready. This can be improved later when needed.

Test plan:
Look at status and type on both job list and detail view.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 78 Marcel de Rooy 2022-09-23 09:46:02 UTC
Created attachment 140897 [details] [review]
Bug 30982: (QA follow-up) No userenv, no jobs

+    # Assume permission if context has no user
+    my $can_manage_background_jobs = 1;
=> This felt a bit unsafe.

Test plan:
Try interface for jobs. Call API with cookie. Call API with OAuth.
Run t/db_dependent/Koha/BackgroundJobs.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 79 Marcel de Rooy 2022-09-23 09:57:05 UTC
Passing QA but with some further comments that imo need to be addressed at some point.

[1] The switch Jobs / Background jobs is still a bit arguable for me, especially since we create another patron/borrower/member thing in the codebase.

[2] Just want to note that this patch set did not contain any test plans. Although it is REST API, we should just add test plans.

[3] A superlibrarian of one with can_manage may see the jobs of other users, no problem, but the interface should show the job owner. It is confusing now since I do not see if it is my job. (The feature was apparently developed with showing only your own jobs in mind.)

[4] The default setting for Current jobs only and Started last hour is arguable. With the current setting I never see a job, since they just finished. I tend to say that Current jobs only should be off at the start.

Great work for the rest !
Comment 80 Marcel de Rooy 2022-09-23 09:58:02 UTC
s/of one/or one/
Comment 81 Tomás Cohen Arazi 2022-09-23 12:11:53 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 82 Nick Clemens 2022-10-26 15:05:22 UTC
This is wonderful.

While I know it is an enhancement, the presence of elastic indexing and holds queue updates in the jobs table can lead to large tables quickly. I would request this one be considered for backport
Comment 83 Lucas Gass 2022-10-31 17:29:06 UTC
(In reply to Nick Clemens from comment #82)
> This is wonderful.
> 
> While I know it is an enhancement, the presence of elastic indexing and
> holds queue updates in the jobs table can lead to large tables quickly. I
> would request this one be considered for backport

I would be happy to add this patchset to 22.05.x but there are conflicts with this patchset in 22.05.x that I do not feel comfortable solving. Could you rebase?
Comment 84 Nick Clemens 2022-11-01 11:36:01 UTC
Created attachment 142859 [details] [review]
Bug 30982: [22.05.x] Use the REST API for background job list view

Bug 30982: REST API specs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: (QA follow-up) Double quoting and console.log

This patch fixes the issues highlighted by the QA script; We use double
quotes for translatable strings in JS and remove an errant console.log
call.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: Add tests and implement GET /background_jobs/$id

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: Make code more re-usable

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: Add 'context' to the REST API specs

context has been added by bug 30889

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: Add Koha::BackgroundJobs->filter_by_current

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: API tweaks

This patch makes the following changes to the 'background_jobs' API:

* We now call them 'jobs'
* Removed deprecated query parameter definitions
* Added only_current query parameter
* Controller gets adapted to use $rs->filter_by_current when
  only_current is passed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: Adapt table to new API spec

Disclaimer: this patch is highly opinionated :-D

When I started looking at this patch I felt like the two tables
(current/past jobs) implemented on bug 30462 was the way to go.

In order to make this patches apply after it I had to redo all the
things. Or most of them.

But I decided to keep the idea of filtering out completed tasks, not
just having the option to display 'the last hour' tasks. For the task I
added some required helper methods and the relevant tests as well. So a
behavior change.

Hope you all agree with it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: Rename 'Background Jobs' => 'Jobs'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Bug 30982: (QA follow-up) Resolve pod warning

Empty section in previous paragraph at line 32 in file Koha/BackgroundJobs.pm

Test plan:
Run podchecker again on this module.

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

Bug 30982: (QA follow-up) Spelling

[1] Correct: BackgrounJob
[2] If should filter out not current jobs
=> Had a hard time reading that one until I replaced if by it.
=> Decided to rephrase it in a more positive way.

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

Bug 30982: (QA follow-up) Remove redundancy from template

The template now contains two lists for both status and type:
a TT list and a JS list. The type list already proves that
redundancy leads to bugs. We miss three types at one side:
    Unknown job type 'stage_marc_for_import'
    Unknown job type 'marc_import_commit_batch'
    Unknown job type 'marc_import_revert_batch'

This patch removes the TT list. And gets the status and type
via an additional js call. For that reason I hide the fieldset
until document ready. This can be improved later when needed.

Test plan:
Look at status and type on both job list and detail view.

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

Bug 30982: (QA follow-up) No userenv, no jobs

+    # Assume permission if context has no user
+    my $can_manage_background_jobs = 1;
=> This felt a bit unsafe.

Test plan:
Try interface for jobs. Call API with cookie. Call API with OAuth.
Run t/db_dependent/Koha/BackgroundJobs.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 85 Lucas Gass 2022-11-01 21:43:18 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 86 Arthur Suzuki 2022-11-14 17:10:03 UTC
unmet dependencies, can't apply on 21.11.14
Comment 87 Jonathan Druart 2022-11-18 13:09:08 UTC
+    is( Koha::BackgroundJobs->search_limited->count, 0, 'No jobs found without userenv' );

This is wrong if jobs exist.

Jenkins is failing because of this.
Comment 88 Jonathan Druart 2022-11-18 13:14:53 UTC
I don't recreate locally. I've run the different tests that (directly) insert background jobs, there are some leftover in the DB, but the tests are still passing.

21:28:55 koha_1       |     #   Failed test 'No jobs found without userenv'
21:28:55 koha_1       |     #   at t/db_dependent/Koha/BackgroundJobs.t line 135.
21:28:55 koha_1       |     #          got: '2'
21:28:55 koha_1       |     #     expected: '0'
21:28:55 koha_1       |     # Looks like you failed 1 test of 3.
21:28:55 koha_1       | 
21:28:55 koha_1       | #   Failed test 'search_limited'
21:32:22 koha_1       | #   at t/db_dependent/Koha/BackgroundJobs.t line 142.
21:32:22 koha_1       | # Looks like you failed 1 test of 15.
21:32:22 koha_1       | [20:28:55] t/db_dependent/Koha/BackgroundJobs.t
Comment 89 Jonathan Druart 2022-11-21 10:21:33 UTC
(In reply to Jonathan Druart from comment #88)
> I don't recreate locally. I've run the different tests that (directly)
> insert background jobs, there are some leftover in the DB, but the tests are
> still passing.
> 
> 21:28:55 koha_1       |     #   Failed test 'No jobs found without userenv'
> 21:28:55 koha_1       |     #   at t/db_dependent/Koha/BackgroundJobs.t line
> 135.
> 21:28:55 koha_1       |     #          got: '2'
> 21:28:55 koha_1       |     #     expected: '0'
> 21:28:55 koha_1       |     # Looks like you failed 1 test of 3.
> 21:28:55 koha_1       | 
> 21:28:55 koha_1       | #   Failed test 'search_limited'
> 21:32:22 koha_1       | #   at t/db_dependent/Koha/BackgroundJobs.t line 142.
> 21:32:22 koha_1       | # Looks like you failed 1 test of 15.
> 21:32:22 koha_1       | [20:28:55] t/db_dependent/Koha/BackgroundJobs.t

See bug 32304.