Bug 14697: Make controller methods rely on the stashed user

This patch adjusts the return values and HTTP status codes, as well as
removing the use of C4::Context->userenv. It also makes the date
calculation happen on the DB engine in the case of resolving the claim.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

diff --git a/Koha/REST/V1/ReturnClaims.pm b/Koha/REST/V1/ReturnClaims.pm
index 1bed731..c497e01 100644 (file)
--- a/Koha/REST/V1/ReturnClaims.pm
+++ b/Koha/REST/V1/ReturnClaims.pm
@@ -29,7 +29,9 @@ use Koha::DateUtils qw( dt_from_string output_pref );
 
 Koha::REST::V1::ReturnClaims
 
-=head2 Operations
+=head1 API
+
+=head2 Methods
 
 =head3 claim_returned
 
@@ -53,7 +55,7 @@ sub claim_returned {
         my $checkout = Koha::Checkouts->find( { itemnumber => $itemnumber } );
 
         return $c->render(
-            openapi => { error => "Not found - Checkout not found" },
+            openapi => { error => "Checkout not found" },
             status  => 404
         ) unless $checkout;
 
@@ -100,52 +102,45 @@ Update the notes of an existing claim
 =cut
 
 sub update_notes {
-    my $c     = shift->openapi->valid_input or return;
-    my $input = $c->validation->output;
-    my $body  = $c->validation->param('body');
+    my $c = shift->openapi->valid_input or return;
+
+    my $claim_id = $c->validation->param('claim_id');
+    my $body     = $c->validation->param('body');
+
+    my $claim = Koha::Checkouts::ReturnClaims->find( $claim_id );
+
+    return $c->render(
+        status  => 404,
+        openapi => {
+            error => "Claim not found"
+        }
+    ) unless $claim;
 
     return try {
-        my $id         = $input->{claim_id};
         my $updated_by = $body->{updated_by};
         my $notes      = $body->{notes};
 
-        $updated_by ||=
-          C4::Context->userenv ? C4::Context->userenv->{number} : undef;
-
-        my $claim = Koha::Checkouts::ReturnClaims->find($id);
-
-        return $c->render(
-            openapi => { error => "Not found - Claim not found" },
-            status  => 404
-        ) unless $claim;
+        my $user = $c->stash('koha.user');
+        $updated_by //= $user->borrowernumber;
 
         $claim->set(
             {
                 notes      => $notes,
-                updated_by => $updated_by,
-                updated_on => dt_from_string(),
+                updated_by => $updated_by
             }
-        );
-        $claim->store();
-
-        my $data = $claim->unblessed;
-
-        my $c_dt = dt_from_string( $data->{created_on} );
-        my $u_dt = dt_from_string( $data->{updated_on} );
-
-        $data->{created_on_formatted} = output_pref( { dt => $c_dt } );
-        $data->{updated_on_formatted} = output_pref( { dt => $u_dt } );
+        )->store;
+        $claim->discard_changes;
 
-        $data->{created_on} = $c_dt->iso8601;
-        $data->{updated_on} = $u_dt->iso8601;
-
-        return $c->render( openapi => $data, status => 200 );
+        return $c->render(
+            status  => 200,
+            openapi => $claim->to_api
+        );
     }
     catch {
-        if ( $_->isa('DBIx::Class::Exception') ) {
+        if ( $_->isa('Koha::Exceptions::Exception') ) {
             return $c->render(
                 status  => 500,
-                openapi => { error => $_->{msg} }
+                openapi => { error => "$_" }
             );
         }
         else {
@@ -165,40 +160,46 @@ Marks a claim as resolved
 
 sub resolve_claim {
     my $c     = shift->openapi->valid_input or return;
-    my $input = $c->validation->output;
-    my $body  = $c->validation->param('body');
+
+    my $claim_id = $c->validation->param('claim_id');
+    my $body     = $c->validation->param('body');
+
+    my $claim = Koha::Checkouts::ReturnClaims->find($claim_id);
+
+    return $c->render(
+            status => 404,
+            openapi => {
+                error => "Claim not found"
+            }
+    ) unless $claim;
 
     return try {
-        my $id          = $input->{claim_id};
+
         my $resolved_by = $body->{updated_by};
         my $resolution  = $body->{resolution};
 
-        $resolved_by ||=
-          C4::Context->userenv ? C4::Context->userenv->{number} : undef;
-
-        my $claim = Koha::Checkouts::ReturnClaims->find($id);
-
-        return $c->render(
-            openapi => { error => "Not found - Claim not found" },
-            status  => 404
-        ) unless $claim;
+        my $user = $c->stash('koha.user');
+        $resolved_by //= $user->borrowernumber;
 
         $claim->set(
             {
                 resolution  => $resolution,
                 resolved_by => $resolved_by,
-                resolved_on => dt_from_string(),
+                resolved_on => \'NOW()',
             }
-        );
-        $claim->store();
+        )->store;
+        $claim->discard_changes;
 
-        return $c->render( openapi => $claim, status => 200 );
+        return $c->render(
+            status  => 200,
+            openapi => $claim->to_api
+        );
     }
     catch {
-        if ( $_->isa('DBIx::Class::Exception') ) {
+        if ( $_->isa('Koha::Exceptions::Exception') ) {
             return $c->render(
                 status  => 500,
-                openapi => { error => $_->{msg} }
+                openapi => { error => "$_" }
             );
         }
         else {
@@ -217,28 +218,29 @@ Deletes the claim from the database
 =cut
 
 sub delete_claim {
-    my $c     = shift->openapi->valid_input or return;
-    my $input = $c->validation->output;
+    my $c = shift->openapi->valid_input or return;
 
     return try {
-        my $id = $input->{claim_id};
 
-        my $claim = Koha::Checkouts::ReturnClaims->find($id);
+        my $claim = Koha::Checkouts::ReturnClaims->find( $c->validation->param('claim_id') );
 
         return $c->render(
-            openapi => { error => "Not found - Claim not found" },
-            status  => 404
+            status  => 404,
+            openapi => { error => "Claim not found" }
         ) unless $claim;
 
         $claim->delete();
 
-        return $c->render( openapi => $claim, status => 200 );
+        return $c->render(
+            status  => 204,
+            openapi => {}
+        );
     }
     catch {
-        if ( $_->isa('DBIx::Class::Exception') ) {
+        if ( $_->isa('Koha::Exceptions::Exception') ) {
             return $c->render(
                 status  => 500,
-                openapi => { error => $_->{msg} }
+                openapi => { error => "$_" }
             );
         }
         else {

diff --git a/api/v1/swagger/paths/return_claims.json b/api/v1/swagger/paths/return_claims.json
index 1104bf1..b711f95 100644 (file)
--- a/api/v1/swagger/paths/return_claims.json
+++ b/api/v1/swagger/paths/return_claims.json
@@ -215,7 +215,7 @@
         "application/json"
       ],
       "responses": {
-        "200": {
+        "204": {
           "description": "Claim deleted",
           "schema": {
             "$ref": "../definitions.json#/return_claim"

diff --git a/t/db_dependent/api/v1/return_claims.t b/t/db_dependent/api/v1/return_claims.t
index 975c0ec..986ff51 100644 (file)
--- a/t/db_dependent/api/v1/return_claims.t
+++ b/t/db_dependent/api/v1/return_claims.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 27;
+use Test::More tests => 32;
 use Test::MockModule;
 use Test::Mojo;
 use Test::Warn;
@@ -88,7 +88,8 @@ $t->post_ok(
         created_by      => $librarian->id,
         notes           => "This is a test note."
     }
-)->status_is(404);
+)->status_is(404)
+ ->json_is( '/error' => 'Checkout not found' );
 
 ## Valid id
 $t->post_ok(
@@ -135,7 +136,8 @@ $t->put_ok(
         notes      => "This is a different test note.",
         updated_by => $librarian->id,
     }
-)->status_is(404);
+)->status_is(404)
+ ->json_is( '/error' => 'Claim not found' );
 
 # Resolve a claim
 ## Valid claim id
@@ -156,13 +158,16 @@ $t->put_ok(
         resolved_by => $librarian->id,
         resolution  => "FOUNDINLIB",
     }
-)->status_is(404);
+)->status_is(404)
+ ->json_is( '/error' => 'Claim not found' );
 
 # Test deleting a return claim
-$t = $t->delete_ok("//$userid:$password@/api/v1/return_claims/$claim_id")
-  ->status_is(200);
+$t->delete_ok("//$userid:$password@/api/v1/return_claims/$claim_id")
+  ->status_is( 204, 'SWAGGER3.2.4' )
+  ->content_is( '', 'SWAGGER3.3.4' );
 $claim = Koha::Checkouts::ReturnClaims->find($claim_id);
 isnt( $claim, "Return claim was deleted" );
 
 $t->delete_ok("//$userid:$password@/api/v1/return_claims/$claim_id")
-  ->status_is(404);
+  ->status_is(404)
+  ->json_is( '/error' => 'Claim not found' );