From 9fdcd6ec3e8c1615d852296d9eed40cc65044bee 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 - Move 'add items to batch' button to bottom right and make button bigger - Add a11y aria tags - Update cancel/close button label depending on step of batch creation - Keep 'finish and see batch' button disabled until 'add items to batch' has been clicked Sponsored-by: UKHSA --- Koha/REST/V1/Illbatches.pm | 18 ++++ api/v1/swagger/definitions/illrequest.yaml | 2 +- api/v1/swagger/paths/illbatches.yaml | 4 + .../en/includes/ill-batch-modal-strings.inc | 1 + .../prog/en/includes/ill-batch-modal.inc | 17 ++-- .../prog/en/includes/ill-toolbar.inc | 2 +- .../en/modules/admin/ill_batch_statuses.tt | 4 +- .../intranet-tmpl/prog/js/ill-batch-modal.js | 83 ++++++++++++------- .../intranet-tmpl/prog/js/ill-list-table.js | 6 +- t/db_dependent/IllbatchStatuses.t | 17 ++-- t/db_dependent/api/v1/illbatches.t | 31 +++++-- 11 files changed, 128 insertions(+), 57 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-strings.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal-strings.inc index 98d48daaea..69737a6259 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal-strings.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal-strings.inc @@ -17,6 +17,7 @@ var ill_batch_create_api_fail = _("Unable to create batch request"); var ill_batch_update_api_fail = _("Unable to updatecreate batch request"); var ill_batch_item_remove = _("Are you sure you want to remove this item from the batch"); + var ill_batch_create_cancel_button = _("Close"); var ill_batch_metadata_more = _("More"); var ill_batch_metadata_less = _("Less"); var ill_batch_available_via = _("Available via"); 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..72903bb48d 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="" />
  • - - + +
  • @@ -49,7 +49,7 @@
  • - +
    • @@ -64,9 +64,6 @@ - @@ -74,6 +71,7 @@ + @@ -81,14 +79,17 @@ +
    - +
    - +
    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 %]
    '; + return ''; } // Redraw the table @@ -1021,23 +1050,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 8639d415f9..2ffeaf5062 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