Bugzilla – Attachment 170498 Details for
Bug 37686
render_resource_not_found() and render_resource_deleted() misses
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 37686: Fix render_resource_not_found() and render_resource_deleted() misses
Bug-37686-Fix-renderresourcenotfound-and-renderres.patch (text/plain), 16.65 KB, created by
Tomás Cohen Arazi (tcohen)
on 2024-08-20 14:03:10 UTC
(
hide
)
Description:
Bug 37686: Fix render_resource_not_found() and render_resource_deleted() misses
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2024-08-20 14:03:10 UTC
Size:
16.65 KB
patch
obsolete
>From aef8b1c2ddd5935360d19664f55c2642704ae0d1 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> >--- > 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 f6f166137b9..f8552940273 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 5ea6d8e7d0e..5f41ef95eb3 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 c165b6d84b3..5fee139357b 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 e0d7c5b2717..bdbe6952ebc 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 85e3a9c425f..5b40de1222e 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 cc1a8fef634..48468b686f5 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 bce1bedee0b..6e219cbe3fc 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 7a42fb778f3..846727673c1 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 fc60e7feecb..4c5107b2028 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 2b7f20e123d..82953ff2e9e 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 7abe4dbef2a..ce7caa939af 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 787f977f20b..a1abd6e467a 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 d79f7c68cfe..8e8515621f3 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 c5f04b19b39..b0b7fbb5682 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 482ddde5a49..3d7d0775420 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 48c761a2b60..0281470a1bf 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.46.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 37686
:
170498
|
170541
|
170553