From d3e14b8673bc1e84ec24b8a124659fe3e1e22fae Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 17 Aug 2021 15:33:14 -0300 Subject: [PATCH] Bug 24857: (follow-up) Use $c->unhandled_exception This patch makes the controller up to date with the currently used style and codebase. It also sinlences a useless warning, and makes it return 404 (instead of 409) when trying to add items to a volume on a non-existing biblio. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/volumes.t => FAIL: Some warnings 2. Apply this patch 3. Repeat 1 => SUCCESS: No errors, no warnings Signed-off-by: Tomas Cohen Arazi --- Koha/REST/V1/Biblios/Volumes.pm | 56 +++++---------------------- Koha/REST/V1/Biblios/Volumes/Items.pm | 21 ++-------- t/db_dependent/api/v1/volumes.t | 17 ++++---- 3 files changed, 22 insertions(+), 72 deletions(-) diff --git a/Koha/REST/V1/Biblios/Volumes.pm b/Koha/REST/V1/Biblios/Volumes.pm index f5cac316ce..792f511f48 100644 --- a/Koha/REST/V1/Biblios/Volumes.pm +++ b/Koha/REST/V1/Biblios/Volumes.pm @@ -52,19 +52,7 @@ sub list { ); } catch { - unless ( blessed $_ && $_->can('rethrow') ) { - return $c->render( - status => 500, - openapi => { - error => - "Something went wrong, check Koha logs for details." - } - ); - } - return $c->render( - status => 500, - openapi => { error => "$_" } - ); + $c->unhandled_exception($_); }; } @@ -100,12 +88,7 @@ sub get { } } catch { - return $c->render( - status => 500, - openapi => { - error => "Something went wrong, check the logs ($_)." - } - ); + $c->unhandled_exception($_); }; } @@ -137,22 +120,16 @@ sub add { if ( blessed($_) ) { my $to_api_mapping = Koha::Biblio::Volume->new->to_api_mapping; - if ( $_->isa('Koha::Exceptions::Object::FKConstraint') ) { + if ( $_->isa('Koha::Exceptions::Object::FKConstraint') and + $to_api_mapping->{ $_->broken_fk } eq 'biblio_id') { return $c->render( - status => 409, - openapi => { - error => "Given " . $to_api_mapping->{ $_->broken_fk } . " does not exist" - } + status => 404, + openapi => { error => "Biblio not found" } ); } } - return $c->render( - status => 500, - openapi => { - error => "Unhandled exception ($_)" - } - ); + $c->unhandled_exception($_); }; } @@ -202,12 +179,7 @@ sub update { } } - return $c->render( - status => 500, - openapi => { - error => "Unhandled exception ($_)" - } - ); + $c->unhandled_exception($_); }; } @@ -235,17 +207,7 @@ sub delete { return $c->render( status => 204, openapi => ''); } catch { - unless ( blessed $_ && $_->can('rethrow') ) { - return $c->render( - status => 500, - openapi => { error => "Something went wrong, check Koha logs for details." } - ); - } - - return $c->render( - status => 500, - openapi => { error => "$_" } - ); + $c->unhandled_exception($_); }; } diff --git a/Koha/REST/V1/Biblios/Volumes/Items.pm b/Koha/REST/V1/Biblios/Volumes/Items.pm index 43f17013c4..831690773c 100644 --- a/Koha/REST/V1/Biblios/Volumes/Items.pm +++ b/Koha/REST/V1/Biblios/Volumes/Items.pm @@ -103,12 +103,7 @@ sub add { } } - return $c->render( - status => 500, - openapi => { - error => "Unhandled exception ($_)" - } - ); + $c->unhandled_exception($_); }; } @@ -144,21 +139,11 @@ sub delete { $item_link->delete; return $c->render( status => 204, - openapi => '' + openapi => q{} ); } catch { - unless ( blessed $_ && $_->can('rethrow') ) { - return $c->render( - status => 500, - openapi => { error => "Something went wrong, check Koha logs for details." } - ); - } - - return $c->render( - status => 500, - openapi => { error => "$_" } - ); + $c->unhandled_exception($_); }; } diff --git a/t/db_dependent/api/v1/volumes.t b/t/db_dependent/api/v1/volumes.t index 7bb5e1aca9..2b6b29e201 100755 --- a/t/db_dependent/api/v1/volumes.t +++ b/t/db_dependent/api/v1/volumes.t @@ -110,8 +110,13 @@ subtest 'volumes add() tests' => sub { ->status_is( 201, 'SWAGGER3.2.1' ); # Invalid biblio id - $t->post_ok( "//$auth_userid:$password@/api/v1/biblios/XXX/volumes" => json => $volume ) - ->status_is( 409, 'SWAGGER3.2.1' ); + { # hide useless warnings + local *STDERR; + open STDERR, '>', '/dev/null'; + $t->post_ok( "//$auth_userid:$password@/api/v1/biblios/XXX/volumes" => json => $volume ) + ->status_is( 404 ); + close STDERR; + } $schema->storage->txn_rollback; }; @@ -196,10 +201,10 @@ subtest 'volumes delete() tests' => sub { ->status_is(403); $t->delete_ok( "//$auth_userid:$password@/api/v1/biblios/XXX/volumes/$volume_id" ) - ->status_is(404); + ->status_is(404); $t->delete_ok( "//$auth_userid:$password@/api/v1/biblios/$biblio_id/volumes/XXX" ) - ->status_is(404); + ->status_is(404); $schema->storage->txn_rollback; }; @@ -239,13 +244,11 @@ subtest 'volume items add() + delete() tests' => sub { my $item_2_id = $item_2->id; $t->post_ok( "//$userid:$password@/api/v1/biblios/{biblio_id}/volumes/$volume_id/items" => json => { item_id => $item_2->id } ) - ->status_is( 201, 'SWAGGER3.2.1' ); + ->status_is( 201, 'SWAGGER3.2.1' ); @items = $volume->items; is( scalar(@items), 2, 'Volume now has two items'); - warn "A VOLUME ID: $volume_id"; - warn "A ITEM ID: $item_1_id"; $t->delete_ok( "//$userid:$password@/api/v1/biblios/$biblio_id/volumes/$volume_id/items/$item_1_id" ) ->status_is(204, 'SWAGGER3.2.4') ->content_is('', 'SWAGGER3.3.4'); -- 2.33.0