Koha-community/Koha
13
7
Fork 0
Code Issues Releases Wiki Activity

Bug 37686: Fix render_resource_not_found() and render_resource_deleted() misses

Browse source 
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>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This commit is contained in:
Tomás Cohen Arazi 2024-08-20 10:59:23 -03:00 committed by Katrin Fischer
parent ddcf2b9b70
commit 317456da49
Signed by: kfischer
GPG key ID: 0EF6E2C03357A834
16 changed files with 56 additions and 118 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

25
Koha/REST/V1/Biblios/ItemGroups.pm
View file

@ -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;

10
Koha/REST/V1/CashRegisters/Cashups.pm
View file

@ -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 );

8
Koha/REST/V1/DeletedBiblios.pm
View file

@ -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 {

5
Koha/REST/V1/Holds.pm
View file

@ -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);

5
Koha/REST/V1/ILL/Backends.pm
View file

@ -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");
};
}

8
Koha/REST/V1/Items.pm
View file

@ -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;

8
Koha/REST/V1/Patrons/Checkouts.pm
View file

@ -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 {

8
Koha/REST/V1/Patrons/Recalls.pm
View file

@ -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 {

28
Koha/REST/V1/Preservation/Trains.pm
View file

@ -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,

4
Koha/REST/V1/Static.pm
View file

@ -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 {

43
Koha/REST/V1/StockRotation/Rotas.pm
View file

@ -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($_);
};

9
Koha/REST/V1/StockRotation/Stage.pm
View file

@ -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");
}
}

5
Koha/REST/V1/TransferLimits.pm
View file

@ -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;

2
t/db_dependent/api/v1/deleted_biblios.t
View file

@ -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 {

2
t/db_dependent/api/v1/preservation_Trains.t
View file

@ -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?)

4
t/db_dependent/api/v1/stockrotationstage.t
View file

@ -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};