From 4b525d9e3eb6edbb27e155a0e74a0f17e9ceaf63 Mon Sep 17 00:00:00 2001 From: Pedro Amorim Date: Tue, 12 May 2026 14:39:40 +0000 Subject: [PATCH] Bug 37762: (QA follow-up): Warn and return 500 on schema validation failure in progress_request An exception is not implemented as progress_request is called in different circulation contexts (hold is placed, checkout happens, etc) and those actions should not be dependent on the outcome of progress_request, i.e. if progress_request fails due to schema validation failure, the circulation action should happen as before. prove t/db_dependent/Koha/ILL/ISO18626/Request.t prove t/db_dependent/api/v1/iso18626/request.t cypress run --spec t/cypress/integration/ILL/SupplyingRequests.ts Signed-off-by: Lucas Gass --- Koha/ILL/ISO18626/Request.pm | 13 +++--- Koha/REST/V1/ILL/ISO18626/Requests.pm | 4 +- .../integration/ILL/SupplyingRequests.ts | 41 +++++++++++++++++++ t/db_dependent/Koha/ILL/ISO18626/Request.t | 32 ++++++++++++++- t/db_dependent/api/v1/iso18626/request.t | 37 ++++++++++++++++- 5 files changed, 117 insertions(+), 10 deletions(-) diff --git a/Koha/ILL/ISO18626/Request.pm b/Koha/ILL/ISO18626/Request.pm index cbe347a28a..7e9e7943f0 100644 --- a/Koha/ILL/ISO18626/Request.pm +++ b/Koha/ILL/ISO18626/Request.pm @@ -433,13 +433,12 @@ sub progress_request { my @errors = $schema->validate($json); if (@errors) { - - #TODO: Throw exception - use Data::Dumper; - $Data::Dumper::Maxdepth = 4; - warn Dumper( '##### 2 #######################################################line: ' . __LINE__ ); - warn Dumper( \@errors ); - warn Dumper('##### end2 #######################################################'); + warn sprintf( + "ISO18626: Schema validation failed for request %s: %s", + $self->iso18626_request_id, + join( ', ', map { "$_" } @errors ) + ); + return 0; } $self->status($resulting_status)->store; $self->send_message( 'supplyingAgencyMessage', $json ); diff --git a/Koha/REST/V1/ILL/ISO18626/Requests.pm b/Koha/REST/V1/ILL/ISO18626/Requests.pm index 95122dc304..c7713241c1 100644 --- a/Koha/REST/V1/ILL/ISO18626/Requests.pm +++ b/Koha/REST/V1/ILL/ISO18626/Requests.pm @@ -89,7 +89,9 @@ sub edit { my $body = $c->req->json; my $status = $body->{status}; - $request->progress_request( 'supplyingAgency', $body ); #TODO: Rethink this but good enough for now + my $result = $request->progress_request( 'supplyingAgency', $body ); + return $c->render( status => 500, json => { error => "Request could not be progressed" } ) + unless $result; return $c->render( status => 200, diff --git a/t/cypress/integration/ILL/SupplyingRequests.ts b/t/cypress/integration/ILL/SupplyingRequests.ts index 2efd30f4e4..2eda46d5cb 100644 --- a/t/cypress/integration/ILL/SupplyingRequests.ts +++ b/t/cypress/integration/ILL/SupplyingRequests.ts @@ -355,6 +355,47 @@ describe("Supplying ILL Requests operations", () => { cy.contains("label", "Active checkout").next().should("contain", "Yes"); }); + it("Shows error banner when progressing status fails", () => { + let request = get_supplying_request(); + + cy.intercept("GET", "/api/v1/ill/iso18626_requests*", { + statusCode: 200, + body: [request], + headers: { + "X-Base-Total-Count": "1", + "X-Total-Count": "1", + }, + }).as("getRequests"); + cy.intercept("GET", "/api/v1/ill/iso18626_requests/*", { + ...request, + hold: null, + messages: [], + }).as("getRequest"); + + cy.visit("/cgi-bin/koha/ill/iso18626_requests"); + cy.wait("@getRequests"); + cy.get( + "#iso18626_requests_list table tbody tr:first td:first a" + ).click(); + cy.wait("@getRequest"); + + cy.intercept("PATCH", "/api/v1/ill/iso18626_requests/*", { + statusCode: 500, + body: { error: "Request could not be progressed" }, + }).as("patchRequest"); + + cy.get("#iso18626_requests_show").contains("Unfilled").click(); + cy.get("#confirmation #reasonUnfilled .vs__search").type( + "Not held{enter}", + { force: true } + ); + cy.get("#accept_modal").click(); + cy.wait("@patchRequest"); + cy.get("main div[class='alert alert-warning']").contains( + "Something went wrong: Error: Request could not be progressed" + ); + }); + it("Progresses status via the RetryPossible modal", () => { let request = get_supplying_request(); diff --git a/t/db_dependent/Koha/ILL/ISO18626/Request.t b/t/db_dependent/Koha/ILL/ISO18626/Request.t index 692835a7e3..a123324486 100755 --- a/t/db_dependent/Koha/ILL/ISO18626/Request.t +++ b/t/db_dependent/Koha/ILL/ISO18626/Request.t @@ -18,7 +18,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 5; +use Test::More tests => 6; use Test::MockModule; use Test::MockObject; @@ -372,6 +372,36 @@ subtest 'Koha::Hold::progress_iso18626_request() state machine' => sub { $schema->storage->txn_rollback; }; +subtest 'progress_request() returns 0 and does not update status on schema validation failure' => sub { + + plan tests => 2; + + $schema->storage->txn_begin; + + my $request = $builder->build_object( + { + class => 'Koha::ILL::ISO18626::Requests', + value => { status => 'RequestReceived' } + } + ); + + my $mock_validator = Test::MockModule->new('JSON::Validator::Schema::OpenAPIv2'); + $mock_validator->mock( 'validate', sub { return ('fake validation error') } ); + + my $result; + { + local $SIG{__WARN__} = sub { }; + $result = $request->progress_request( 'supplyingAgency', { status => 'ExpectToSupply' } ); + } + + is( $result, 0, 'progress_request returns 0 when schema validation fails' ); + + $request->discard_changes; + is( $request->status, 'RequestReceived', 'Request status is not updated when validation fails' ); + + $schema->storage->txn_rollback; +}; + subtest 'add_message() and messages()' => sub { plan tests => 2; diff --git a/t/db_dependent/api/v1/iso18626/request.t b/t/db_dependent/api/v1/iso18626/request.t index dc026dc32c..b54eb570e9 100755 --- a/t/db_dependent/api/v1/iso18626/request.t +++ b/t/db_dependent/api/v1/iso18626/request.t @@ -18,7 +18,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 3; +use Test::More tests => 4; use Test::MockModule; use Test::MockObject; @@ -267,6 +267,41 @@ XML $schema->storage->txn_rollback; }; +subtest 'edit() tests' => sub { + + plan tests => 3; + + $schema->storage->txn_begin; + + my $librarian = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 2**22 } # 22 => ill + } + ); + my $password = 'thePassword123'; + $librarian->set_password( { password => $password, skip_validation => 1 } ); + my $userid = $librarian->userid; + + my $request = $builder->build_object( + { + class => 'Koha::ILL::ISO18626::Requests', + value => { status => 'RequestReceived' } + } + ); + + my $mock_request = Test::MockModule->new('Koha::ILL::ISO18626::Request'); + $mock_request->mock( 'progress_request', sub { return 0 } ); + + $t->patch_ok( + "//$userid:$password@/api/v1/ill/iso18626_requests/" . $request->iso18626_request_id => + json => { status => 'ExpectToSupply' } + )->status_is(500) + ->json_is( '/error' => 'Request could not be progressed' ); + + $schema->storage->txn_rollback; +}; + subtest 'send_message() tests' => sub { plan tests => 18;