From eb2dfad0d1c2f1257f9601af195af0acc894bb5f Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 17 Jun 2022 09:21:39 +0200 Subject: [PATCH] Bug 30982: [22.05.x] Use the REST API for background job list view Bug 30982: REST API specs Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy 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 Signed-off-by: Marcel de Rooy Bug 30982: Add tests and implement GET /background_jobs/$id Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy Bug 30982: Make code more re-usable Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy Bug 30982: Add 'context' to the REST API specs context has been added by bug 30889 Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy Bug 30982: Add Koha::BackgroundJobs->filter_by_current Signed-off-by: Tomas Cohen Arazi Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy 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 Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy 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 Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy Bug 30982: Rename 'Background Jobs' => 'Jobs' Signed-off-by: Tomas Cohen Arazi Signed-off-by: Martin Renvoize Signed-off-by: Marcel de Rooy 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 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 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 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 Signed-off-by: Lucas Gass --- Koha/BackgroundJob.pm | 44 ++- Koha/BackgroundJobs.pm | 49 ++- Koha/REST/V1/BackgroundJobs.pm | 99 +++++ admin/background_jobs.pl | 33 -- api/v1/swagger/definitions/job.yaml | 54 +++ api/v1/swagger/paths/jobs.yaml | 87 ++++ api/v1/swagger/swagger.yaml | 15 + .../prog/en/includes/admin-menu.inc | 4 +- .../prog/en/modules/admin/admin-home.tt | 6 +- .../prog/en/modules/admin/background_jobs.tt | 373 ++++++++++-------- t/db_dependent/Koha/BackgroundJobs.t | 53 ++- t/db_dependent/api/v1/jobs.t | 125 ++++++ t/lib/TestBuilder.pm | 3 + 13 files changed, 738 insertions(+), 207 deletions(-) create mode 100644 Koha/REST/V1/BackgroundJobs.pm create mode 100644 api/v1/swagger/definitions/job.yaml create mode 100644 api/v1/swagger/paths/jobs.yaml create mode 100755 t/db_dependent/api/v1/jobs.t diff --git a/Koha/BackgroundJob.pm b/Koha/BackgroundJob.pm index e4aede3fe3..5b8e017940 100644 --- a/Koha/BackgroundJob.pm +++ b/Koha/BackgroundJob.pm @@ -47,7 +47,7 @@ my $job_id = Koha::BackgroundJob->enqueue( ); Consumer: -Koha::BackgrounJobs->find($job_id)->process; +Koha::BackgroundJobs->find($job_id)->process; See also C for a full example =head1 API @@ -386,7 +386,7 @@ sub _derived_class { =head3 type_to_class_mapping - my $mapping = Koha::BackgrounJob->new->type_to_class_mapping; + my $mapping = Koha::BackgroundJob->new->type_to_class_mapping; Returns the available types to class mappings. @@ -404,7 +404,7 @@ sub type_to_class_mapping { =head3 core_types_to_classes - my $mappings = Koha::BackgrounJob->new->core_types_to_classes + my $mappings = Koha::BackgroundJob->new->core_types_to_classes Returns the core background jobs types to class mappings. @@ -469,6 +469,44 @@ sub plugin_types_to_classes { return $self->{_plugin_mapping}; } +=head3 to_api + + my $json = $job->to_api; + +Overloaded method that returns a JSON representation of the Koha::BackgroundJob object, +suitable for API output. + +=cut + +sub to_api { + my ( $self, $params ) = @_; + + my $json = $self->SUPER::to_api( $params ); + + $json->{context} = $self->json->decode($self->context) + if defined $self->context; + $json->{data} = $self->decoded_data; + + return $json; +} + +=head3 to_api_mapping + +This method returns the mapping for representing a Koha::BackgroundJob object +on the API. + +=cut + +sub to_api_mapping { + return { + id => 'job_id', + borrowernumber => 'patron_id', + ended_on => 'ended_date', + enqueued_on => 'enqueued_date', + started_on => 'started_date', + }; +} + =head3 _type =cut diff --git a/Koha/BackgroundJobs.pm b/Koha/BackgroundJobs.pm index d2ff22347f..48afbf68fa 100644 --- a/Koha/BackgroundJobs.pm +++ b/Koha/BackgroundJobs.pm @@ -16,19 +16,64 @@ package Koha::BackgroundJobs; # along with Koha; if not, see . use Modern::Perl; -use base qw(Koha::Objects); + use Koha::BackgroundJob; +use base qw(Koha::Objects); + =head1 NAME Koha::BackgroundJobs - Koha BackgroundJob Object set class =head1 API -=head2 Class Methods +=head2 Class methods + +=head3 search_limited + + my $background_jobs = Koha::BackgroundJobs->search_limited( $params, $attributes ); + +Returns all background jobs the logged in user should be allowed to see + +=cut + +sub search_limited { + my ( $self, $params, $attributes ) = @_; + + my $can_manage_background_jobs; + my $logged_in_user; + my $userenv = C4::Context->userenv; + if ( $userenv and $userenv->{number} ) { + $logged_in_user = Koha::Patrons->find( $userenv->{number} ); + $can_manage_background_jobs = $logged_in_user->has_permission( + { parameters => 'manage_background_jobs' } ); + } + + return $self->search( $params, $attributes ) if $can_manage_background_jobs; + my $id = $logged_in_user ? $logged_in_user->borrowernumber : undef; + return $self->search({ borrowernumber => $id })->search( $params, $attributes ); +} + +=head3 filter_by_current + + my $current_jobs = $jobs->filter_by_current; + +Returns a new resultset, filtering out finished jobs. =cut +sub filter_by_current { + my ($self) = @_; + + return $self->search( + { + status => { not_in => [ 'cancelled', 'failed', 'finished' ] } + } + ); +} + +=head2 Internal methods + =head3 _type =cut diff --git a/Koha/REST/V1/BackgroundJobs.pm b/Koha/REST/V1/BackgroundJobs.pm new file mode 100644 index 0000000000..4db1ea3f3f --- /dev/null +++ b/Koha/REST/V1/BackgroundJobs.pm @@ -0,0 +1,99 @@ +package Koha::REST::V1::BackgroundJobs; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Mojo::Base 'Mojolicious::Controller'; + +use Koha::BackgroundJobs; + +use Try::Tiny; + +=head1 API + +=head2 Methods + +=head3 list + +Controller function that handles listing Koha::BackgroundJob objects + +=cut + +sub list { + my $c = shift->openapi->valid_input or return; + + return try { + + my $only_current = delete $c->validation->output->{only_current}; + + my $bj_rs = Koha::BackgroundJobs->new; + + if ($only_current) { + $bj_rs = $bj_rs->filter_by_current; + } + + return $c->render( + status => 200, + openapi => $c->objects->search($bj_rs) + ); + } catch { + $c->unhandled_exception($_); + }; +} + +=head3 get + +Controller function that handles retrieving a single Koha::BackgroundJob object + +=cut + +sub get { + my $c = shift->openapi->valid_input or return; + + return try { + + my $job_id = $c->validation->param('job_id'); + my $patron = $c->stash('koha.user'); + + my $can_manage_background_jobs = + $patron->has_permission( { parameters => 'manage_background_jobs' } ); + + my $job = Koha::BackgroundJobs->find($job_id); + + return $c->render( + status => 404, + openapi => { error => "Object not found" } + ) unless $job; + + return $c->render( + status => 403, + openapi => { error => "Cannot see background job info" } + ) + if !$can_manage_background_jobs + && $job->borrowernumber != $patron->borrowernumber; + + return $c->render( + status => 200, + openapi => $job->to_api + ); + } + catch { + $c->unhandled_exception($_); + }; +} + +1; diff --git a/admin/background_jobs.pl b/admin/background_jobs.pl index 761c09d069..b85281ce72 100755 --- a/admin/background_jobs.pl +++ b/admin/background_jobs.pl @@ -77,39 +77,6 @@ if ( $op eq 'cancel' ) { $op = 'list'; } - -if ( $op eq 'list' ) { - my $queued_jobs = - $can_manage_background_jobs - ? Koha::BackgroundJobs->search( { ended_on => undef }, - { order_by => { -desc => 'enqueued_on' } } ) - : Koha::BackgroundJobs->search( - { borrowernumber => $logged_in_user->borrowernumber, ended_on => undef }, - { order_by => { -desc => 'enqueued_on' } } - ); - $template->param( queued => $queued_jobs ); - - my $ended_since = dt_from_string->subtract( minutes => '60' ); - my $dtf = Koha::Database->new->schema->storage->datetime_parser; - - my $complete_jobs = - $can_manage_background_jobs - ? Koha::BackgroundJobs->search( - { - ended_on => { '>=' => $dtf->format_datetime($ended_since) } - }, - { order_by => { -desc => 'enqueued_on' } } - ) - : Koha::BackgroundJobs->search( - { - borrowernumber => $logged_in_user->borrowernumber, - ended_on => { '>=' => $dtf->format_datetime($ended_since) } - }, - { order_by => { -desc => 'enqueued_on' } } - ); - $template->param( complete => $complete_jobs ); -} - $template->param( messages => \@messages, op => $op, diff --git a/api/v1/swagger/definitions/job.yaml b/api/v1/swagger/definitions/job.yaml new file mode 100644 index 0000000000..966e3a3ef8 --- /dev/null +++ b/api/v1/swagger/definitions/job.yaml @@ -0,0 +1,54 @@ +--- +type: object +properties: + job_id: + type: integer + description: internally assigned job identifier + readOnly: true + status: + description: job status + type: string + progress: + description: job progress + type: + - string + - "null" + size: + description: job size + type: + - string + - "null" + patron_id: + description: job enqueuer + type: + - string + - "null" + type: + description: job type + type: string + queue: + description: job queue + type: string + data: + description: job data + type: object + context: + description: job context + type: object + enqueued_date: + description: job enqueue date + type: string + format: date-time + started_date: + description: job start date + type: + - string + - "null" + format: date-time + ended_date: + description: job end date + type: + - string + - "null" + format: date-time +additionalProperties: false diff --git a/api/v1/swagger/paths/jobs.yaml b/api/v1/swagger/paths/jobs.yaml new file mode 100644 index 0000000000..62ae157dc0 --- /dev/null +++ b/api/v1/swagger/paths/jobs.yaml @@ -0,0 +1,87 @@ +--- +/jobs: + get: + x-mojo-to: BackgroundJobs#list + operationId: listJobs + tags: + - jobs + summary: List jobs + produces: + - application/json + parameters: + - name: only_current + in: query + required: false + type: boolean + description: Only include current jobs + - $ref: "../swagger.yaml#/parameters/match" + - $ref: "../swagger.yaml#/parameters/order_by" + - $ref: "../swagger.yaml#/parameters/page" + - $ref: "../swagger.yaml#/parameters/per_page" + - $ref: "../swagger.yaml#/parameters/q_param" + - $ref: "../swagger.yaml#/parameters/q_body" + - $ref: "../swagger.yaml#/parameters/q_header" + - $ref: "../swagger.yaml#/parameters/request_id_header" + responses: + "200": + description: A list of jobs + schema: + type: array + items: + $ref: "../swagger.yaml#/definitions/job" + "403": + description: Access forbidden + schema: + $ref: "../swagger.yaml#/definitions/error" + "500": + description: | + Internal server error. Possible `error_code` attribute values: + + * `internal_server_error` + schema: + $ref: "../swagger.yaml#/definitions/error" + "503": + description: Under maintenance + schema: + $ref: "../swagger.yaml#/definitions/error" + x-koha-authorization: + permissions: + catalogue: "1" +"/jobs/{job_id}": + get: + x-mojo-to: BackgroundJobs#get + operationId: getJob + tags: + - jobs + summary: Get a job + parameters: + - $ref: "../swagger.yaml#/parameters/job_id_pp" + produces: + - application/json + responses: + "200": + description: A job + schema: + $ref: "../swagger.yaml#/definitions/job" + "403": + description: Access forbidden + schema: + $ref: "../swagger.yaml#/definitions/error" + "404": + description: Job not found + schema: + $ref: "../swagger.yaml#/definitions/error" + "500": + description: | + Internal server error. Possible `error_code` attribute values: + + * `internal_server_error` + schema: + $ref: "../swagger.yaml#/definitions/error" + "503": + description: Under maintenance + schema: + $ref: "../swagger.yaml#/definitions/error" + x-koha-authorization: + permissions: + catalogue: "1" diff --git a/api/v1/swagger/swagger.yaml b/api/v1/swagger/swagger.yaml index 88e3bef4bd..c9291f820e 100644 --- a/api/v1/swagger/swagger.yaml +++ b/api/v1/swagger/swagger.yaml @@ -42,6 +42,8 @@ definitions: $ref: ./definitions/invoice.yaml item: $ref: ./definitions/item.yaml + job: + $ref: ./definitions/job.yaml library: $ref: ./definitions/library.yaml order: @@ -155,6 +157,10 @@ paths: $ref: "./paths/items.yaml#/~1items~1{item_id}" "/items/{item_id}/pickup_locations": $ref: "./paths/items.yaml#/~1items~1{item_id}~1pickup_locations" + /jobs: + $ref: ./paths/jobs.yaml#/~1jobs + "/jobs/{job_id}": + $ref: "./paths/jobs.yaml#/~1jobs~1{job_id}" /libraries: $ref: ./paths/libraries.yaml#/~1libraries "/libraries/{library_id}": @@ -300,6 +306,12 @@ parameters: name: item_id required: true type: integer + job_id_pp: + description: Job internal identifier + in: path + name: job_id + required: true + type: integer library_id_pp: description: Internal library identifier in: path @@ -560,6 +572,9 @@ tags: - description: "Manage items\n" name: items x-displayName: Items + - description: "Manage jobs\n" + name: jobs + x-displayName: Jobs - description: "Manage libraries\n" name: libraries x-displayName: Libraries diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc index 8fb7ccccd7..bbb6370d85 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc @@ -75,9 +75,9 @@ [% END %] [% IF CAN_user_parameters_manage_background_jobs %] -
Background jobs
+
Jobs
[% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt index cd15f66e04..d100efa638 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt @@ -147,10 +147,10 @@ [% END %] [% IF CAN_user_parameters_manage_background_jobs %] -

Background jobs

+

Jobs

-
Manage background jobs
-
View, manage and cancel background jobs.
+
Manage jobs
+
View, manage and cancel jobs.
[% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt index 0fafb30da4..1432cbdc2a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt @@ -3,54 +3,12 @@ [% USE Asset %] [% USE KohaDates %] [% SET footerjs = 1 %] -[% BLOCK show_job_status %] - [% SWITCH job.status %] - [% CASE "new" %] - New - [% CASE "cancelled" %] - Cancelled - [% CASE "finished" %] - Finished - [% CASE "started" %] - Started - [% CASE "running" %] - Running - [% CASE "failed" %] - Failed - [% CASE # Default case %] - [% job.status | html %] - [% END -%] -[% END %] -[% BLOCK show_job_type %] - [% SWITCH job_type %] - [% CASE 'batch_biblio_record_modification' %] - Batch bibliographic record modification - [% CASE 'batch_biblio_record_deletion' %] - Batch bibliographic record record deletion - [% CASE 'batch_authority_record_modification' %] - Batch authority record modification - [% CASE 'batch_authority_record_deletion' %] - Batch authority record deletion - [% CASE 'batch_item_record_modification' %] - Batch item record modification - [% CASE 'batch_item_record_deletion' %] - Batch item record deletion - [% CASE "batch_hold_cancel" %] - Batch hold cancellation - [% CASE 'update_elastic_index' %] - Update Elasticsearch index - [% CASE 'update_holds_queue_for_biblios' %] - Holds queue update - [% CASE %]Unknown job type '[% job_type | html %]' - [% END %] - -[% END %] [% INCLUDE 'doc-head-open.inc' %] [% IF op == 'view' %] Details of job #[% job.id | html %] › [% END %] - Background jobs › + Jobs › Administration › Koha @@ -73,14 +31,14 @@ [% IF op == 'view' %]
  • - Background jobs + Jobs
  • Details of job #[% job.id | html %]
    • [% ELSE %]
  • - Background jobs + Jobs
    • [% END %] [% ELSE %] @@ -112,17 +70,17 @@ [% PROCESS "background_jobs/${job.type}.inc" %] -
    +