From 40e841da984f0aeb13a4632ce83a638cee47a434 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi 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 Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Lisette Scheer Signed-off-by: Martin Renvoize --- 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