From 9dbf0ff0e0ea968fa408fc6e12bfbc943ceb9f26 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 29 Oct 2019 10:27:18 -0300 Subject: [PATCH] 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 Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Lisette Scheer Signed-off-by: Martin Renvoize --- Koha/REST/V1/ReturnClaims.pm | 122 ++++++++++++------------ api/v1/swagger/paths/return_claims.json | 2 +- t/db_dependent/api/v1/return_claims.t | 19 ++-- 3 files changed, 75 insertions(+), 68 deletions(-) diff --git a/Koha/REST/V1/ReturnClaims.pm b/Koha/REST/V1/ReturnClaims.pm index 1bed731344..c497e01561 100644 --- 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 1104bf1666..b711f957bc 100644 --- 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 975c0ec12a..986ff512af 100644 --- 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' ); -- 2.39.5