From 459b2e5896502360c8d787888d455aac7c616783 Mon Sep 17 00:00:00 2001 From: Pedro Amorim Date: Tue, 8 Nov 2022 14:53:07 -0100 Subject: [PATCH] Bug 30719: (QA Follow-up): - Fix wrong link 'Create a new batch status message' in case there are no existing batch statuses - Detail of batch update - View status of any request within a batch - Fix typo to correctly display status code on batch statuses list page - Fixed an issue with illrequest api definition that prevented the listing of any ILL requests if any request had a status_alias - Updated "Add items to batch" css class to match new 22.11 style - Fixed conflict issue where it prevented the creation of a single request in a backend that is using JS field validation utilizing the #cardnumber selector - Added a check for metadata enrichment plugins and bail batches JS code if none are found, this prevents a console error - Remove ysearch from patron auto complete in batch creation and use patron_autocomplete instead - 'New batch' and 'list batches' buttons no longer show if no metadata plugin is installed - Fixed tests, all passing now - Improved error messaging for non-existent cardnumber input in patron in ill batch creation - Improved error messaging for duplicate batch name creation error - Fix sidemenu filters and search not working after new batch_id column Sponsored-by: UKHSA --- Koha/REST/V1/Illbatches.pm | 18 ++++++ api/v1/swagger/definitions/illrequest.yaml | 2 +- api/v1/swagger/paths/illbatches.yaml | 4 ++ .../prog/en/includes/ill-batch-modal.inc | 7 ++- .../prog/en/includes/ill-toolbar.inc | 2 +- .../en/modules/admin/ill_batch_statuses.tt | 4 +- .../intranet-tmpl/prog/js/ill-batch-modal.js | 58 ++++++++++++------- .../intranet-tmpl/prog/js/ill-list-table.js | 6 +- t/db_dependent/IllbatchStatuses.t | 17 ++++-- t/db_dependent/api/v1/illbatches.t | 31 ++++++++-- 10 files changed, 107 insertions(+), 42 deletions(-) diff --git a/Koha/REST/V1/Illbatches.pm b/Koha/REST/V1/Illbatches.pm index f75eba5f78..487c4132df 100644 --- a/Koha/REST/V1/Illbatches.pm +++ b/Koha/REST/V1/Illbatches.pm @@ -23,6 +23,8 @@ use Koha::Illbatches; use Koha::IllbatchStatuses; use Koha::Illrequests; +use Try::Tiny qw( catch try ); + =head1 NAME Koha::REST::V1::Illbatches @@ -140,6 +142,14 @@ sub add { # We receive cardnumber, so we need to look up the corresponding # borrowernumber my $patron = Koha::Patrons->find({ cardnumber => $body->{cardnumber} }); + + if ( not defined $patron ) { + return $c->render( + status => 404, + openapi => { error => "Patron with cardnumber " . $body->{cardnumber} . " not found" } + ); + } + delete $body->{cardnumber}; $body->{borrowernumber} = $patron->borrowernumber; @@ -162,6 +172,14 @@ sub add { ); } catch { + if ( blessed $_ ) { + if ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) { + return $c->render( + status => 409, + openapi => { error => "A batch named " . $body->{name} . " already exists" } + ); + } + } $c->unhandled_exception($_); }; } diff --git a/api/v1/swagger/definitions/illrequest.yaml b/api/v1/swagger/definitions/illrequest.yaml index c37f7a30d3..78f9dc8385 100644 --- a/api/v1/swagger/definitions/illrequest.yaml +++ b/api/v1/swagger/definitions/illrequest.yaml @@ -128,7 +128,7 @@ properties: description: The request's current status status_alias: type: - - string + - object - "null" description: The ID of a user defined status for this request updated: diff --git a/api/v1/swagger/paths/illbatches.yaml b/api/v1/swagger/paths/illbatches.yaml index d19df76f75..30a67355d8 100644 --- a/api/v1/swagger/paths/illbatches.yaml +++ b/api/v1/swagger/paths/illbatches.yaml @@ -72,6 +72,10 @@ description: Access forbidden schema: $ref: "../swagger.yaml#/definitions/error" + "404": + description: Patron with given cardnumber not found + schema: + $ref: "../swagger.yaml#/definitions/error" "409": description: Conflict in creating resource schema: diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal.inc index f0abfa8a64..b7587606e1 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal.inc @@ -18,8 +18,8 @@ value="" />
  • - - + +
  • @@ -65,7 +65,7 @@ @@ -74,6 +74,7 @@ + diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc index ffc6055071..cbb6bfd13e 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc @@ -32,7 +32,7 @@ List requests [% END %] - [% IF have_batch.size > 0 %] + [% IF have_batch.size > 0 && metadata_enrichment_services %]
    - + [% ELSE %]
    - There are no batch statuses defined. Create new batch status + There are no batch statuses defined. Create new batch status
    [% END %] [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/js/ill-batch-modal.js b/koha-tmpl/intranet-tmpl/prog/js/ill-batch-modal.js index 70f9e29c5a..bf885a31dd 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/ill-batch-modal.js +++ b/koha-tmpl/intranet-tmpl/prog/js/ill-batch-modal.js @@ -1,4 +1,10 @@ (function () { + // Bail if there aren't any metadata enrichment plugins installed + if (typeof metadata_enrichment_services === 'undefined') { + console.log('No metadata enrichment plugins found.') + return; + } + window.addEventListener('load', onload); // Delay between API requests @@ -7,7 +13,7 @@ // Elements we work frequently with var textarea = document.getElementById("identifiers_input"); var nameInput = document.getElementById("name"); - var cardnumberInput = document.getElementById("cardnumber"); + var cardnumberInput = document.getElementById("batchcardnumber"); var branchcodeSelect = document.getElementById("branchcode"); var processButton = document.getElementById("process_button"); var createButton = document.getElementById("button_create_batch"); @@ -21,7 +27,6 @@ var createRequestsButton = document.getElementById('create-requests-button'); var statusesSelect = document.getElementById('statuscode'); - // We need a data structure keyed on identifier type, which tells us how to parse that // identifier type and what services can get its metadata. We receive an array of // available services @@ -299,6 +304,7 @@ tableContent.data = tableContent.data.map(function (row) { if (row.value === identifier) { row.requestId = data.illrequest_id; + row.requestStatus = data.status; } return row; }); @@ -512,7 +518,10 @@ }) }) .then(function (response) { - return response.json(); + if ( response.ok ) { + return response.json(); + } + return Promise.reject(response); }) .then(function (body) { batchId = body.id; @@ -528,8 +537,14 @@ }; initPostCreate(); }) - .catch(function () { - handleApiError(ill_batch_create_api_fail); + .catch(function (response) { + response.json().then((json) => { + if( json.error ) { + handleApiError(json.error); + } else { + handleApiError(ill_batch_create_api_fail); + } + }) }); }; @@ -893,6 +908,10 @@ return data.requestId || '-'; } + function createRequestStatus(x, y, data) { + return data.requestStatus || '-'; + } + function buildTable(identifiers) { table = KohaTable('identifier-table', { processing: true, @@ -918,9 +937,14 @@ }, { data: 'requestId', - width: '13%', + width: '6.5%', render: createRequestId }, + { + data: 'requestStatus', + width: '6.5%', + render: createRequestStatus + }, { width: '18%', render: createActions, @@ -1021,23 +1045,15 @@ } function patronAutocomplete() { - // Add autocomplete for patron selection - $('#batch-form #cardnumber').autocomplete({ - appendTo: '#batch-form', - source: "/cgi-bin/koha/circ/ysearch.pl", - minLength: 3, - select: function (event, ui) { - var field = ui.item.cardnumber; - $('#batch-form #cardnumber').val(field) + patron_autocomplete( + $('#batch-form #batchcardnumber'), + { + 'on-select-callback': function( event, ui ) { + $("#batch-form #batchcardnumber").val( ui.item.cardnumber ); return false; + } } - }) - .data("ui-autocomplete")._renderItem = function (ul, item) { - return $("
  • ") - .data("ui-autocomplete-item", item) - .append("" + item.surname + ", " + item.firstname + " (" + item.cardnumber + ") " + item.address + " " + item.city + " " + item.zipcode + " " + item.country + "") - .appendTo(ul); - }; + ); }; function createPatronLink() { diff --git a/koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js b/koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js index 245941c5d6..5904b398db 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js +++ b/koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js @@ -51,7 +51,7 @@ $(document).ready(function() { var sel = $('#illfilter_status option:selected').val(); if (sel && sel.length > 0) { activeFilters[me] = function() { - table.api().column(13).search(sel); + table.api().column(14).search(sel); } } else { if (activeFilters.hasOwnProperty(me)) { @@ -83,7 +83,7 @@ $(document).ready(function() { var sel = $('#illfilter_branchname option:selected').val(); if (sel && sel.length > 0) { activeFilters[me] = function() { - table.api().column(12).search(sel); + table.api().column(13).search(sel); } } else { if (activeFilters.hasOwnProperty(me)) { @@ -103,7 +103,7 @@ $(document).ready(function() { var val = $('#illfilter_patron').val(); if (val && val.length > 0) { activeFilters[me] = function() { - table.api().column(10).search(val); + table.api().column(11).search(val); } } else { if (activeFilters.hasOwnProperty(me)) { diff --git a/t/db_dependent/IllbatchStatuses.t b/t/db_dependent/IllbatchStatuses.t index a0378e0279..487b20d7d4 100755 --- a/t/db_dependent/IllbatchStatuses.t +++ b/t/db_dependent/IllbatchStatuses.t @@ -28,7 +28,7 @@ use t::lib::TestBuilder; use Test::MockObject; use Test::MockModule; -use Test::More tests => 12; +use Test::More tests => 13; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; @@ -98,6 +98,16 @@ is( "Creation of status calls log_something" ); +# Try creating a system status and ensure it's not created +my $cannot_create_system = Koha::IllbatchStatus->new({ + name => "Jar Jar Binks", + code => "GUNGAN", + is_system => 1 +}); +$cannot_create_system->create_and_log; +my $created_but_not_system = Koha::IllbatchStatuses->find({ code => "GUNGAN" }); +is($created_but_not_system->{is_system}, undef, "is_system statuses cannot be created"); + ## Status update # Ensure only name can be updated @@ -118,19 +128,16 @@ is( ); ## Status delete - -# Prevent deletion of system statuses my $cannot_delete = Koha::IllbatchStatus->new({ name => "Palapatine", code => "SITH", is_system => 1 -}); +})->store; my $can_delete = Koha::IllbatchStatus->new({ name => "Windu", code => "JEDI", is_system => 0 }); -$cannot_delete->create_and_log; $cannot_delete->delete_and_log; my $not_deleted = Koha::IllbatchStatuses->find({ code => "SITH" }); isa_ok( $not_deleted, 'Koha::IllbatchStatus', "is_system statuses cannot be deleted" ); diff --git a/t/db_dependent/api/v1/illbatches.t b/t/db_dependent/api/v1/illbatches.t index 4e7512fadd..088f7d296f 100755 --- a/t/db_dependent/api/v1/illbatches.t +++ b/t/db_dependent/api/v1/illbatches.t @@ -26,6 +26,7 @@ use t::lib::Mocks; use Koha::Illbatch; use Koha::Illbatches; use Koha::Illrequests; +use Koha::IllbatchStatuses; use Koha::Database; my $schema = Koha::Database->new->schema; @@ -200,7 +201,7 @@ subtest 'get() tests' => sub { subtest 'add() tests' => sub { - plan tests =>16; + plan tests =>19; $schema->storage->txn_begin; @@ -230,11 +231,18 @@ subtest 'add() tests' => sub { } ); + my $batch_status = $builder->build_object( + { + class => 'Koha::IllbatchStatuses' + } + ); + my $batch_metadata = { name => "Anakin's requests", backend => "Mock", - borrowernumber => $librarian->borrowernumber, - branchcode => $branch->branchcode + cardnumber => $librarian->cardnumber, + branchcode => $branch->branchcode, + statuscode => $batch_status->code }; # Unauthorized attempt to write @@ -264,9 +272,12 @@ subtest 'add() tests' => sub { ->status_is( 201 ) ->json_is( '/name' => $batch_metadata->{name} ) ->json_is( '/backend' => $batch_metadata->{backend} ) - ->json_is( '/borrowernumber' => $batch_metadata->{borrowernumber} ) + ->json_is( '/borrowernumber' => $librarian->borrowernumber ) ->json_is( '/branchcode' => $batch_metadata->{branchcode} ) + ->json_is( '/statuscode' => $batch_status->code ) ->json_has( '/patron' ) + ->json_has( '/status' ) + ->json_has( '/requests_count' ) ->json_has( '/branch' ); # Authorized attempt to create with null id @@ -316,11 +327,18 @@ subtest 'update() tests' => sub { $t->put_ok( "//$unauth_userid:$password@/api/v1/illbatches/$batch_id" => json => { name => 'These are not the droids you are looking for' } ) ->status_is(403); + my $batch_status = $builder->build_object( + { + class => 'Koha::IllbatchStatuses' + } + ); + # Attempt partial update on a PUT my $batch_with_missing_field = { backend => "Mock", borrowernumber => $librarian->borrowernumber, - branchcode => $branch->branchcode + branchcode => $branch->branchcode, + statuscode => $batch_status->code }; $t->put_ok( "//$userid:$password@/api/v1/illbatches/$batch_id" => json => $batch_with_missing_field ) @@ -334,7 +352,8 @@ subtest 'update() tests' => sub { name => "Master Ploo Koon", backend => "Mock", borrowernumber => $librarian->borrowernumber, - branchcode => $branch->branchcode + branchcode => $branch->branchcode, + statuscode => $batch_status->code }; $t->put_ok( "//$userid:$password@/api/v1/illbatches/$batch_id" => json => $batch_with_updated_field ) -- 2.30.2