From 40e841da984f0aeb13a4632ce83a638cee47a434 Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
Date: Mon, 28 Oct 2019 16:18:52 -0300
Subject: [PATCH] Bug 14697: (follow-up) Rely on the UNIQUE constraint and
 return 409 for issue_id

This patch avoids querying the DB for an already existing
Koha::Checkouts::ReturnClaim with the same issue_id, now that there's a
UNIQUE constraint on it.

Also, 409 should be returned instead. Tests added for this changes.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/return_claims.t
=> SUCCESS: tests pass!

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>
---
 Koha/REST/V1/ReturnClaims.pm            | 22 +++++++++-------------
 api/v1/swagger/paths/return_claims.json | 14 ++++++++++----
 t/db_dependent/api/v1/return_claims.t   | 24 +++++++++++++++---------
 3 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/Koha/REST/V1/ReturnClaims.pm b/Koha/REST/V1/ReturnClaims.pm
index d221518650..1bed731344 100644
--- a/Koha/REST/V1/ReturnClaims.pm
+++ b/Koha/REST/V1/ReturnClaims.pm
@@ -57,17 +57,7 @@ sub claim_returned {
             status  => 404
         ) unless $checkout;
 
-        my $claim = Koha::Checkouts::ReturnClaims->find(
-            {
-                issue_id => $checkout->id
-            }
-        );
-        return $c->render(
-            openapi => { error => "Bad request - claim exists" },
-            status  => 400
-        ) if $claim;
-
-        $claim = $checkout->claim_returned(
+        my $claim = $checkout->claim_returned(
             {
                 charge_lost_fee => $charge_lost_fee,
                 created_by      => $created_by,
@@ -82,12 +72,18 @@ sub claim_returned {
         );
     }
     catch {
-        if ( $_->isa('Koha::Exceptions::Checkouts::ReturnClaims') ) {
+        if ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) {
             return $c->render(
-                status  => 500,
+                status  => 409,
                 openapi => { error => "$_" }
             );
         }
+        elsif ( $_->isa('Koha::Exceptions::Checkouts::ReturnClaims::NoCreatedBy') ) {
+            return $c->render(
+                status  => 400,
+                openapi => { error => "Mandatory attribute created_by missing" }
+            );
+        }
         else {
             return $c->render(
                 status  => 500,
diff --git a/api/v1/swagger/paths/return_claims.json b/api/v1/swagger/paths/return_claims.json
index 068176f1bb..1104bf1666 100644
--- a/api/v1/swagger/paths/return_claims.json
+++ b/api/v1/swagger/paths/return_claims.json
@@ -67,10 +67,16 @@
           }
         },
         "404": {
-          "description": "Checkout not found",
-          "schema": {
-            "$ref": "../definitions.json#/error"
-          }
+            "description": "Checkout not found",
+            "schema": {
+                "$ref": "../definitions.json#/error"
+            }
+        },
+        "409": {
+            "description": "Conflict creating the resource",
+            "schema": {
+                "$ref": "../definitions.json#/error"
+            }
         },
         "500": {
           "description": "Internal server error",
diff --git a/t/db_dependent/api/v1/return_claims.t b/t/db_dependent/api/v1/return_claims.t
index 073bbd5eba..975c0ec12a 100644
--- a/t/db_dependent/api/v1/return_claims.t
+++ b/t/db_dependent/api/v1/return_claims.t
@@ -17,9 +17,10 @@
 
 use Modern::Perl;
 
-use Test::More tests => 25;
+use Test::More tests => 27;
 use Test::MockModule;
 use Test::Mojo;
+use Test::Warn;
 use t::lib::Mocks;
 use t::lib::TestBuilder;
 
@@ -97,18 +98,23 @@ $t->post_ok(
         created_by      => $librarian->id,
         notes           => "This is a test note."
     }
-)->status_is(201);
+)->status_is(201)
+ ->header_like( Location => qr|^\/api\/v1\/return_claims/\d*|, 'SWAGGER3.4.1');
+
 my $claim_id = $t->tx->res->json->{claim_id};
 
 ## Duplicate id
-$t->post_ok(
-    "//$userid:$password@/api/v1/return_claims" => json => {
-        item_id         => $itemnumber1,
-        charge_lost_fee => Mojo::JSON->false,
-        created_by      => $librarian->id,
-        notes           => "This is a test note."
+warning_like {
+        $t->post_ok(
+            "//$userid:$password@/api/v1/return_claims" => json => {
+                item_id         => $itemnumber1,
+                charge_lost_fee => Mojo::JSON->false,
+                created_by      => $librarian->id,
+                notes           => "This is a test note."
+            }
+        )->status_is(409)
     }
-)->status_is(400);
+    qr/^DBD::mysql::st execute failed: Duplicate entry/;
 
 # Test editing a claim note
 ## Valid claim id
-- 
2.39.5