From 317456da49d68ee95a56adcc2316a2f62e1edf6a Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 20 Aug 2024 10:59:23 -0300 Subject: [PATCH] Bug 37686: Fix render_resource_not_found() and render_resource_deleted() misses This patch refactors some small pieces of code to use the helpers as prescribed on our coding guidelines [1] instead of manually crafting responses. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/ => SUCCESS: All tests pass 3. Sign off :-D [1] https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST4:_Controller_code_.5BDRAFT.5D Signed-off-by: Tomas Cohen Arazi Signed-off-by: David Nind Signed-off-by: Matt Blenkinsop Signed-off-by: Katrin Fischer --- Koha/REST/V1/Biblios/ItemGroups.pm | 25 +++--------- Koha/REST/V1/CashRegisters/Cashups.pm | 10 +---- Koha/REST/V1/DeletedBiblios.pm | 8 +--- Koha/REST/V1/Holds.pm | 5 +-- Koha/REST/V1/ILL/Backends.pm | 5 +-- Koha/REST/V1/Items.pm | 8 +--- Koha/REST/V1/Patrons/Checkouts.pm | 8 +--- Koha/REST/V1/Patrons/Recalls.pm | 8 +--- Koha/REST/V1/Preservation/Trains.pm | 28 ++++--------- Koha/REST/V1/Static.pm | 4 +- Koha/REST/V1/StockRotation/Rotas.pm | 45 +++++++++------------ Koha/REST/V1/StockRotation/Stage.pm | 9 ++--- Koha/REST/V1/TransferLimits.pm | 5 +-- t/db_dependent/api/v1/deleted_biblios.t | 2 +- t/db_dependent/api/v1/preservation_Trains.t | 2 +- t/db_dependent/api/v1/stockrotationstage.t | 4 +- 16 files changed, 57 insertions(+), 119 deletions(-) diff --git a/Koha/REST/V1/Biblios/ItemGroups.pm b/Koha/REST/V1/Biblios/ItemGroups.pm index f6f166137b..f855294027 100644 --- a/Koha/REST/V1/Biblios/ItemGroups.pm +++ b/Koha/REST/V1/Biblios/ItemGroups.pm @@ -77,12 +77,7 @@ sub get { ); } else { - return $c->render( - status => 404, - openapi => { - error => 'Item group not found' - } - ); + return $c->render_resource_not_found("Item group"); } } catch { @@ -102,10 +97,8 @@ sub add { return try { my $biblio = Koha::Biblios->find( $c->param('biblio_id') ); - return $c->render( - status => 404, - openapi => { error => 'Object not found' } - ) unless $biblio; + return $c->render_resource_not_found("Biblio") + unless $biblio; my $item_group_data = $c->req->json; # biblio_id comes from the path @@ -128,10 +121,7 @@ sub add { if ( $_->isa('Koha::Exceptions::Object::FKConstraint') and $_->broken_fk eq 'biblio_id' ) { - return $c->render( - status => 404, - openapi => { error => "Biblio not found" } - ); + return $c->render_resource_not_found("Biblio"); } } @@ -155,12 +145,7 @@ sub update { my $item_group = Koha::Biblio::ItemGroups->find( $item_group_id ); unless ( $item_group && $item_group->biblio_id eq $biblio_id ) { - return $c->render( - status => 404, - openapi => { - error => 'Item group not found' - } - ); + return $c->render_resource_not_found("Item group"); } my $item_group_data = $c->req->json; diff --git a/Koha/REST/V1/CashRegisters/Cashups.pm b/Koha/REST/V1/CashRegisters/Cashups.pm index 5ea6d8e7d0..5f41ef95eb 100644 --- a/Koha/REST/V1/CashRegisters/Cashups.pm +++ b/Koha/REST/V1/CashRegisters/Cashups.pm @@ -42,14 +42,8 @@ sub list { my $register = Koha::Cash::Registers->find( $c->param('cash_register_id') ); - unless ($register) { - return $c->render( - status => 404, - openapi => { - error => "Register not found" - } - ); - } + return $c->render_resource_not_found("Register") + unless $register; return try { my $cashups = $c->objects->search( $register->cashups ); diff --git a/Koha/REST/V1/DeletedBiblios.pm b/Koha/REST/V1/DeletedBiblios.pm index c165b6d84b..5fee139357 100644 --- a/Koha/REST/V1/DeletedBiblios.pm +++ b/Koha/REST/V1/DeletedBiblios.pm @@ -51,12 +51,8 @@ sub get { my $biblio = Koha::Old::Biblios->find( { biblionumber => $c->param('biblio_id') }, $attributes ); - unless ($biblio) { - return $c->render( - status => 404, - openapi => { error => "Object not found." } - ); - } + return $c->render_resource_not_found("Biblio") + unless $biblio; return try { diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm index e0d7c5b271..bdbe6952eb 100644 --- a/Koha/REST/V1/Holds.pm +++ b/Koha/REST/V1/Holds.pm @@ -351,9 +351,8 @@ sub suspend { my $end_date = ($body) ? $body->{end_date} : undef; - unless ($hold) { - return $c->render( status => 404, openapi => { error => 'Hold not found.' } ); - } + return $c->render_resource_not_found("Hold") + unless $hold; return try { $hold->suspend_hold($end_date); diff --git a/Koha/REST/V1/ILL/Backends.pm b/Koha/REST/V1/ILL/Backends.pm index 85e3a9c425..5b40de1222 100644 --- a/Koha/REST/V1/ILL/Backends.pm +++ b/Koha/REST/V1/ILL/Backends.pm @@ -96,10 +96,7 @@ sub get { openapi => $embed ? { %$return, %$embed } : $return, ); } catch { - return $c->render( - status => 404, - openapi => { error => "ILL backend does not exist" } - ); + return $c->render_resource_not_found("ILL backend"); }; } diff --git a/Koha/REST/V1/Items.pm b/Koha/REST/V1/Items.pm index cc1a8fef63..48468b686f 100644 --- a/Koha/REST/V1/Items.pm +++ b/Koha/REST/V1/Items.pm @@ -407,12 +407,8 @@ sub remove_from_bundle { $bundle_item_id = barcodedecode($bundle_item_id); my $bundle_item = Koha::Items->find( { itemnumber => $bundle_item_id } ); - unless ($bundle_item) { - return $c->render( - status => 404, - openapi => { error => "Bundle item not found" } - ); - } + return $c->render_resource_not_found("Bundle item") + unless $bundle_item; return try { $bundle_item->remove_from_bundle; diff --git a/Koha/REST/V1/Patrons/Checkouts.pm b/Koha/REST/V1/Patrons/Checkouts.pm index bce1bedee0..6e219cbe3f 100644 --- a/Koha/REST/V1/Patrons/Checkouts.pm +++ b/Koha/REST/V1/Patrons/Checkouts.pm @@ -40,12 +40,8 @@ sub list { my $patron = Koha::Patrons->find( $c->param('patron_id') ); - unless ($patron) { - return $c->render( - status => 404, - openapi => { error => 'Patron not found' } - ); - } + return $c->render_resource_not_found("Patron") + unless $patron; return try { diff --git a/Koha/REST/V1/Patrons/Recalls.pm b/Koha/REST/V1/Patrons/Recalls.pm index 7a42fb778f..846727673c 100644 --- a/Koha/REST/V1/Patrons/Recalls.pm +++ b/Koha/REST/V1/Patrons/Recalls.pm @@ -40,12 +40,8 @@ sub list { my $patron = Koha::Patrons->find( $c->param('patron_id') ); - unless ($patron) { - return $c->render( - status => 404, - openapi => { error => 'Patron not found' } - ); - } + return $c->render_resource_not_found("Patron") + unless $patron; return try { diff --git a/Koha/REST/V1/Preservation/Trains.pm b/Koha/REST/V1/Preservation/Trains.pm index fc60e7feec..4c5107b202 100644 --- a/Koha/REST/V1/Preservation/Trains.pm +++ b/Koha/REST/V1/Preservation/Trains.pm @@ -228,12 +228,8 @@ sub get_item { { train_item_id => $train_item_id, train_id => $train_id } ); - unless ($train_item) { - return $c->render( - status => 404, - openapi => { error => "Item not found" } - ); - } + return $c->render_resource_not_found("Item") + unless $train_item; return try { Koha::Database->new->schema->txn_do( @@ -316,10 +312,7 @@ sub add_item { openapi => { error => "MissingSettings", parameter => $_->parameter } ); } elsif ( $_->isa('Koha::Exceptions::Preservation::ItemNotFound') ) { - return $c->render( - status => 404, - openapi => { error => "Item not found" } - ); + return $c->render_resource_not_found("Item"); } elsif ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) { return $c->render( status => 409, @@ -376,12 +369,10 @@ sub copy_item { sub { my $new_train_id = delete $body->{train_id}; my $new_train = Koha::Preservation::Trains->find($new_train_id); - unless ($train) { - return $c->render( - status => 404, - openapi => { error => "Train not found" } - ); - } + + return $c->render_resource_not_found("Train") + unless $new_train; + my $new_train_item = $new_train->add_item( { item_id => $train_item->item_id, @@ -412,10 +403,7 @@ sub copy_item { openapi => { error => "MissingSettings", parameter => $_->parameter } ); } elsif ( $_->isa('Koha::Exceptions::Preservation::ItemNotFound') ) { - return $c->render( - status => 404, - openapi => { error => "Item not found" } - ); + return $c->render_resource_not_found("Item"); } elsif ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) { return $c->render( status => 409, diff --git a/Koha/REST/V1/Static.pm b/Koha/REST/V1/Static.pm index 2b7f20e123..82953ff2e9 100644 --- a/Koha/REST/V1/Static.pm +++ b/Koha/REST/V1/Static.pm @@ -55,7 +55,7 @@ sub get { @plugins = grep { $_->api_namespace eq $namespace} @plugins; - return $c->render({ status => 404, openapi => { error => 'File not found' } }) unless scalar(@plugins) > 0; + return $c->render_resource_not_found("File") unless scalar(@plugins) > 0; return $c->render({ status => 500, openapi => { error => 'Namespace not unique' } }) unless scalar(@plugins) == 1; my $plugin = $plugins[0]; @@ -70,7 +70,7 @@ sub get { return $c->reply->asset($asset); } catch { - return $c->render({ status => 404, openapi => { error => 'File not found' } }); + return $c->render_resource_not_found("File"); } } else { diff --git a/Koha/REST/V1/StockRotation/Rotas.pm b/Koha/REST/V1/StockRotation/Rotas.pm index 7abe4dbef2..ce7caa939a 100644 --- a/Koha/REST/V1/StockRotation/Rotas.pm +++ b/Koha/REST/V1/StockRotation/Rotas.pm @@ -52,14 +52,14 @@ sub get { return try { my $rota = Koha::StockRotationRotas->find( $c->param('rota_id') ); - unless ($rota) { - return $c->render( - status => 404, - openapi => { error => "Rota not found" } - ); - } - - return $c->render( status => 200, openapi => $rota->to_api ); + + return $c->render_resource_not_found("Rota") + unless $rota; + + return $c->render( + status => 200, + openapi => $c->objects->to_api($rota), + ); } catch { $c->unhandled_exception($_); } @@ -78,7 +78,7 @@ sub add { $c->res->headers->location( $c->req->url->to_string . '/' . $rota->rota_id ); return $c->render( status => 201, - openapi => $rota->to_api + openapi => $c->objects->to_api($rota), ); } catch { $c->unhandled_exception($_); @@ -94,17 +94,16 @@ sub update { my $rota = Koha::StockRotationRotas->find( $c->param('rota_id') ); - if ( not defined $rota ) { - return $c->render( - status => 404, - openapi => { error => "Object not found" } - ); - } + return $c->render_resource_not_found("Rota") + unless $rota; return try { $rota->set_from_api( $c->req->json ); $rota->store(); - return $c->render( status => 200, openapi => $rota->to_api ); + return $c->render( + status => 200, + openapi => $c->objects->to_api($rota), + ); } catch { $c->unhandled_exception($_); }; @@ -118,19 +117,13 @@ sub delete { my $c = shift->openapi->valid_input or return; my $rota = Koha::StockRotationRotas->find( $c->param('rota_id') ); - if ( not defined $rota ) { - return $c->render( - status => 404, - openapi => { error => "Object not found" } - ); - } + + return $c->render_resource_not_found("Rota") + unless $rota; return try { $rota->delete; - return $c->render( - status => 204, - openapi => q{} - ); + return $c->render_resource_deleted; } catch { $c->unhandled_exception($_); }; diff --git a/Koha/REST/V1/StockRotation/Stage.pm b/Koha/REST/V1/StockRotation/Stage.pm index 787f977f20..a1abd6e467 100644 --- a/Koha/REST/V1/StockRotation/Stage.pm +++ b/Koha/REST/V1/StockRotation/Stage.pm @@ -48,11 +48,10 @@ sub move { status => 400 ); } - else { - return $c->render( - openapi => { error => "Not found - Invalid rota or stage ID" }, - status => 404 - ); + elsif ($rota) { + return $c->render_resource_not_found("Stage"); + } else { + return $c->render_resource_not_found("Rota"); } } diff --git a/Koha/REST/V1/TransferLimits.pm b/Koha/REST/V1/TransferLimits.pm index d79f7c68cf..8e8515621f 100644 --- a/Koha/REST/V1/TransferLimits.pm +++ b/Koha/REST/V1/TransferLimits.pm @@ -103,9 +103,8 @@ sub delete { my $transfer_limit = Koha::Item::Transfer::Limits->find( $c->param( 'limit_id' ) ); - if ( not defined $transfer_limit ) { - return $c->render( status => 404, openapi => { error => "Transfer limit not found" } ); - } + return $c->render_resource_not_found("Transfer limit") + unless $transfer_limit; return try { $transfer_limit->delete; diff --git a/t/db_dependent/api/v1/deleted_biblios.t b/t/db_dependent/api/v1/deleted_biblios.t index c5f04b19b3..b0b7fbb568 100755 --- a/t/db_dependent/api/v1/deleted_biblios.t +++ b/t/db_dependent/api/v1/deleted_biblios.t @@ -103,7 +103,7 @@ subtest 'get() tests' => sub { my $biblio_exist = $builder->build_sample_biblio(); $t->get_ok( "//$userid:$password@/api/v1/deleted/biblios/" . $biblio_exist->biblionumber => { Accept => 'application/marc' } )->status_is(404) - ->json_is( '/error', 'Object not found.' ); + ->json_is( '/error', 'Biblio not found' ); subtest 'marc-in-json encoding tests' => sub { diff --git a/t/db_dependent/api/v1/preservation_Trains.t b/t/db_dependent/api/v1/preservation_Trains.t index 482ddde5a4..3d7d077542 100755 --- a/t/db_dependent/api/v1/preservation_Trains.t +++ b/t/db_dependent/api/v1/preservation_Trains.t @@ -458,7 +458,7 @@ subtest '*_item() tests' => sub { $item_2->delete; $t->post_ok( "//$userid:$password@/api/v1/preservation/trains/$train_id/items" => json => { item_id => $item_2->itemnumber } - )->status_is(404)->json_is( { error => 'Item not found' } ); + )->status_is(404)->json_is( { error => 'Item not found', error_code => 'not_found' } ); # batch add items # Nothing added (FIXME maybe not 201?) diff --git a/t/db_dependent/api/v1/stockrotationstage.t b/t/db_dependent/api/v1/stockrotationstage.t index 48c761a2b6..0281470a1b 100755 --- a/t/db_dependent/api/v1/stockrotationstage.t +++ b/t/db_dependent/api/v1/stockrotationstage.t @@ -82,13 +82,13 @@ subtest 'move() tests' => sub { $t->put_ok( "//$auth_userid:$password@/api/v1/rotas/99999999/stages/$stage1_id/position" => json => 2 ) ->status_is(404) - ->json_is( '/error' => "Not found - Invalid rota or stage ID" ); + ->json_is( '/error' => "Rota not found" ); # Invalid attempt to move an non-existant stage $t->put_ok( "//$auth_userid:$password@/api/v1/rotas/$rota_id/stages/999999999/position" => json => 2 ) ->status_is(404) - ->json_is( '/error' => "Not found - Invalid rota or stage ID" ); + ->json_is( '/error' => "Stage not found" ); # Invalid attempt to move stage to current position my $curr_position = $stage1->{position}; -- 2.39.5