Bugzilla – Attachment 146152 Details for
Bug 30719
ILL should provide the ability to create batch requests
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 30719: (QA Follow-up):
Bug-30719-QA-Follow-up.patch (text/plain), 19.43 KB, created by
Pedro Amorim
on 2023-02-03 15:09:32 UTC
(
hide
)
Description:
Bug 30719: (QA Follow-up):
Filename:
MIME Type:
Creator:
Pedro Amorim
Created:
2023-02-03 15:09:32 UTC
Size:
19.43 KB
patch
obsolete
>From 459b2e5896502360c8d787888d455aac7c616783 Mon Sep 17 00:00:00 2001 >From: Pedro Amorim <pjamorim91@gmail.com> >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="" /> > </li> > <li id="batch_patron"> >- <label class="required" for="cardnumber">Card number, username or surname:</label> >- <input type="text" autocomplete="off" name="cardnumber" id="cardnumber" type="text" value="" /> >+ <label class="required" for="batchcardnumber">Card number, username or surname:</label> >+ <input type="text" autocomplete="off" name="batchcardnumber" id="batchcardnumber" type="text" value="" /> > <span id="patron_link"></span> > </li> > <li id="batch_branch"> >@@ -65,7 +65,7 @@ > </div> > </div> > <div id="create-requests" style="display:none"> >- <button id="create-requests-button" type="button" class="btn btn-xs btn-success">Add items to batch</button> >+ <button id="create-requests-button" type="button" class="btn btn-xs btn-primary">Add items to batch</button> > </div> > <table id="identifier-table" style="display:none"> > <thead> >@@ -74,6 +74,7 @@ > <th scope="col">Identifier value</th> > <th scope="col">Metadata</th> > <th scope="col">Request ID</th> >+ <th scope="col">Request Status</th> > <th scope="col"></th> > </tr> > </thead> >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 @@ > <i class="fa fa-list"></i> List requests > </a> > [% END %] >- [% IF have_batch.size > 0 %] >+ [% IF have_batch.size > 0 && metadata_enrichment_services %] > <div id="ill-batch"> > <div class="dropdown btn-group"> > <button class="btn btn-default dropdown-toggle" type="button" id="ill-batch-backend-dropdown" data-toggle="dropdown" aria-haspopup="true" aria-expanded="true"> >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt >index 0bf6e5f53d..c47cc27e2c 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt >@@ -132,7 +132,7 @@ > [% FOREACH status IN statuses %] > <tr> > <td>[% status.name | html %]</td> >- <td>[% name.code | html %]</td> >+ <td>[% status.code | html %]</td> > <td>[% status.is_system ? "Yes" : "No" | html %]</td> > <td class="actions"> > <a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/ill_batch_statuses.pl?op=add_form&code=[% status.code | uri %]"><i class="fa fa-pencil"></i> Edit</a> >@@ -146,7 +146,7 @@ > </table> > [% ELSE %] > <div class="dialog message"> >- There are no batch statuses defined. <a href="/cgi-bin/koha/admin/debit_types.pl?op=add_form">Create new batch status</a> >+ There are no batch statuses defined. <a href="/cgi-bin/koha/admin/ill_batch_statuses.pl?op=add_form">Create new batch status</a> > </div> > [% 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 $("<li></li>") >- .data("ui-autocomplete-item", item) >- .append("<a>" + item.surname + ", " + item.firstname + " (" + item.cardnumber + ") <small>" + item.address + " " + item.city + " " + item.zipcode + " " + item.country + "</small></a>") >- .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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=146152">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 30719
:
144281
|
144285
|
144286
|
144287
|
144288
|
144289
|
144290
|
144291
|
144292
|
144293
|
144294
|
144295
|
144296
|
144297
|
144298
|
144299
|
144300
|
144301
|
144302
|
145760
|
145869
|
145870
|
145876
|
145934
|
146152
|
147658
|
147929
|
147930
|
147931
|
147932
|
147933
|
147934
|
147952
|
147953
|
147954
|
147955
|
147956
|
151152
|
151153
|
151154
|
151155
|
151156
|
151284
|
151285
|
151286
|
151287
|
151288
|
151303
|
151304
|
151305
|
151306
|
151307
|
151308
|
151309
|
151310
|
151311
|
151312
|
151513
|
151514
|
151515
|
151516
|
151517
|
151519
|
151520
|
151521
|
151522
|
151523
|
151571
|
151572
|
151573
|
151574
|
151575
|
151611
|
151612
|
151613
|
151614
|
151615
|
151804
|
151805
|
151806
|
151807
|
151808
|
151809
|
154046
|
154047
|
154048
|
154049
|
154050
|
154051
|
154052
|
155421
|
155422
|
155423
|
155424
|
155425
|
155426
|
155427
|
155428
|
155429
|
155430
|
155431
|
155432
|
155433
|
155434
|
155435
|
156556
|
156748
|
156749
|
156752
|
156783
|
156785
|
156811
|
156812
|
156827
|
156828