From b3e4ac8f9c3d3dc71cfcfd571f7be4c8921ea7d7 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 12 Feb 2024 09:54:04 +0000 Subject: [PATCH] Bug 36066: Only allow cancelled order deletion in API Content-Type: text/plain; charset=utf-8 Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t --- Koha/REST/V1/Acquisitions/Orders.pm | 10 +++++++++- t/db_dependent/api/v1/acquisitions_orders.t | 11 ++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/Koha/REST/V1/Acquisitions/Orders.pm b/Koha/REST/V1/Acquisitions/Orders.pm index 0155a38922..371132acca 100644 --- a/Koha/REST/V1/Acquisitions/Orders.pm +++ b/Koha/REST/V1/Acquisitions/Orders.pm @@ -183,7 +183,9 @@ sub update { =head3 delete -Controller function that handles deleting a Koha::Patron object +Controller function that handles deleting a Koha::Acquisition::Order object + +Note that we only allow deletion when the status is cancelled. =cut @@ -197,6 +199,12 @@ sub delete { status => 404, openapi => { error => 'Order not found' } ); + } elsif ( ( $order->orderstatus && $order->orderstatus ne 'cancelled' ) || !$order->datecancellationprinted ) { + # Koha may (historically) have inconsistent order data here (e.g. cancelled without date) + return $c->render( + status => 403, + openapi => { error => 'Order status must be cancelled' } + ); } return try { diff --git a/t/db_dependent/api/v1/acquisitions_orders.t b/t/db_dependent/api/v1/acquisitions_orders.t index 40a6ec6685..20dde16ac6 100755 --- a/t/db_dependent/api/v1/acquisitions_orders.t +++ b/t/db_dependent/api/v1/acquisitions_orders.t @@ -72,9 +72,9 @@ subtest 'list() tests' => sub { $another_order = $builder->build_object({ class => 'Koha::Acquisition::Orders', value => $another_order }); ## Authorized user tests - my $count_of_orders = Koha::Acquisition::Orders->search->count; + my $count_of_orders = Koha::Acquisition::Orders->search( { orderstatus => 'new' } )->count; # Make sure we are returned with the correct amount of orders - $t->get_ok( "//$userid:$password@/api/v1/acquisitions/orders?_per_page=-1" ) + $t->get_ok( "//$userid:$password@/api/v1/acquisitions/orders?status=new&_per_page=-1" ) ->status_is( 200, 'SWAGGER3.2.2' ) ->json_has('/'.($count_of_orders-1).'/order_id') ->json_hasnt('/'.($count_of_orders).'/order_id'); @@ -449,7 +449,7 @@ subtest 'update() tests' => sub { }; subtest 'delete() tests' => sub { - plan tests => 7; + plan tests => 9; $schema->storage->txn_begin; @@ -474,6 +474,11 @@ subtest 'delete() tests' => sub { $t->delete_ok( "//$unauth_userid:$password@/api/v1/acquisitions/orders/" . $order->ordernumber ) ->status_is(403); + # Check if status is cancelled? + $order->orderstatus('new')->store; + $t->delete_ok( "//$unauth_userid:$password@/api/v1/acquisitions/orders/" . $order->ordernumber )->status_is(403); + + $order->orderstatus('cancelled')->store; $t->delete_ok( "//$auth_userid:$password@/api/v1/acquisitions/orders/" . $order->ordernumber ) ->status_is(204, 'SWAGGER3.2.4') ->content_is('', 'SWAGGER3.3.4'); -- 2.30.2