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 <lucas@bywatersolutions.com>

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 );

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,

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();
 

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;

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;