From ecd7f7d0027e28db313d57a0e1c65dce52c9dd34 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Thu, 15 Oct 2020 09:37:56 -0300 Subject: [PATCH] Bug 26515: (QA follow-up) Preserve original behaviour This patch removes the use of $self->items->safe_delete, as we don't want to change the current behaviour (i.e. delete what can be deleted) As safe_delete would rollback entirely, there was a behaviour change. Now items are deleted in a loop that catches any problem and reports it using the new ->add_message mechanism. The $item object is added to the message payload so it doesn't need to be queried by the caller for providing UI feedback. Tests are augmented accordingly, To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Acquisition/Order.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi --- Koha/Acquisition/Order.pm | 94 +++++++++++++++---------- t/db_dependent/Koha/Acquisition/Order.t | 78 +++++++++++++++++--- 2 files changed, 125 insertions(+), 47 deletions(-) diff --git a/Koha/Acquisition/Order.pm b/Koha/Acquisition/Order.pm index f2aaa8c3cc6..c913a294c64 100644 --- a/Koha/Acquisition/Order.pm +++ b/Koha/Acquisition/Order.pm @@ -124,49 +124,67 @@ sub cancel { my $delete_biblio = $params->{delete_biblio}; my $reason = $params->{reason}; - try { - # Delete the related items - $self->items->safe_delete; - - my $biblio = $self->biblio; - if ( $biblio and $delete_biblio ) { - - if ( - $biblio->active_orders->search( - { ordernumber => { '!=' => $self->ordernumber } } - )->count == 0 - and $biblio->subscriptions->count == 0 - and $biblio->items->count == 0 - ) - { - - my $error = DelBiblio( $biblio->id ); - $self->add_message( { message => 'error_delbiblio'} ) - if $error; - } - else { - if ( $biblio->active_orders->search( - { ordernumber => { '!=' => $self->ordernumber } } - )->count > 0 ) { - $self->add_message( { message => 'error_delbiblio_active_orders' } ); - } - elsif ( $biblio->subscriptions->count > 0 ) { - $self->add_message( { message => 'error_delbiblio_subscriptions' } ); - } - else { # $biblio->items->count > 0 - $self->add_message( { message => 'error_delbiblio_items' } ); + # Delete the related items + my $items = $self->items; + while ( my $item = $items->next ) { + my $safe_to_delete = $item->safe_to_delete; + if ( $safe_to_delete eq '1' ) { + $item->safe_delete; + } + else { + $self->add_message( + { + message => 'error_delitem', + payload => { item => $item, reason => $safe_to_delete } } - } + ); } } - catch { - if ( ref($_) eq 'Koha::Exceptions::Object::CannotBeDeleted' ) { - my $object = $_->object; - if ( ref($object) eq 'Koha::Item' ) { - $self->add_message({ message => 'error_delitem' }); + + my $biblio = $self->biblio; + if ( $biblio and $delete_biblio ) { + + if ( + $biblio->active_orders->search( + { ordernumber => { '!=' => $self->ordernumber } } + )->count == 0 + and $biblio->subscriptions->count == 0 + and $biblio->items->count == 0 + ) + { + + my $error = DelBiblio( $biblio->id ); + $self->add_message( + { + message => 'error_delbiblio', + payload => { biblio => $biblio, reason => $error } + } + ) if $error; + } + else { + + my $message; + + if ( $biblio->active_orders->search( + { ordernumber => { '!=' => $self->ordernumber } } + )->count > 0 ) { + $message = 'error_delbiblio_active_orders'; + } + elsif ( $biblio->subscriptions->count > 0 ) { + $message = 'error_delbiblio_subscriptions'; } + else { # $biblio->items->count > 0 + $message = 'error_delbiblio_items'; + } + + $self->add_message( + { + message => $message, + payload => { biblio => $biblio } + } + ); } - }; + } # Update order status $self->set( diff --git a/t/db_dependent/Koha/Acquisition/Order.t b/t/db_dependent/Koha/Acquisition/Order.t index a9340e05e67..8919fe29af6 100755 --- a/t/db_dependent/Koha/Acquisition/Order.t +++ b/t/db_dependent/Koha/Acquisition/Order.t @@ -592,7 +592,7 @@ subtest 'filter_by_current & filter_by_cancelled' => sub { subtest 'cancel() tests' => sub { - plan tests => 38; + plan tests => 52; $schema->storage->txn_begin; @@ -608,13 +608,13 @@ subtest 'cancel() tests' => sub { # => message about not being able to delete my $item = $builder->build_sample_item; - my $biblio_id = $item->biblio->id; + my $biblio_id = $item->biblionumber; my $order = $builder->build_object( { class => 'Koha::Acquisition::Orders', value => { orderstatus => 'new', - biblionumber => $item->biblio->id, + biblionumber => $item->biblionumber, datecancellationprinted => undef, cancellationreason => undef, } @@ -625,7 +625,7 @@ subtest 'cancel() tests' => sub { my $patron = $builder->build_object({ class => 'Koha::Patrons' }); t::lib::Mocks::mock_userenv({ patron => $patron }); - # Add a checkout so cancelling fails because od 'book_on_loan' + # Add a checkout so deleting the item fails because od 'book_on_loan' C4::Circulation::AddIssue( $patron->unblessed, $item->barcode ); my $result = $order->cancel({ reason => $reason }); @@ -672,7 +672,7 @@ subtest 'cancel() tests' => sub { # => biblio remains untouched my $item_1 = $builder->build_sample_item; - $biblio_id = $item_1->biblio->id; + $biblio_id = $item_1->biblionumber; my $item_2 = $builder->build_sample_item({ biblionumber => $biblio_id }); $order = $builder->build_object( { @@ -708,7 +708,7 @@ subtest 'cancel() tests' => sub { # => biblio delete error notified $item = $builder->build_sample_item; - $biblio_id = $item->biblio->id; + $biblio_id = $item->biblionumber; $order = $builder->build_object( { class => 'Koha::Acquisition::Orders', @@ -755,7 +755,7 @@ subtest 'cancel() tests' => sub { # => biblio delete error notified $item = $builder->build_sample_item; - $biblio_id = $item->biblio->id; + $biblio_id = $item->biblionumber; $order = $builder->build_object( { class => 'Koha::Acquisition::Orders', @@ -798,13 +798,13 @@ subtest 'cancel() tests' => sub { # => biblio in order is removed $item = $builder->build_sample_item; - $biblio_id = $item->biblio->id; + $biblio_id = $item->biblionumber; $order = $builder->build_object( { class => 'Koha::Acquisition::Orders', value => { orderstatus => 'new', - biblionumber => $item->biblio->id, + biblionumber => $item->biblionumber, datecancellationprinted => undef, cancellationreason => undef, } @@ -823,5 +823,65 @@ subtest 'cancel() tests' => sub { @messages = @{ $order->messages }; is( scalar @messages, 0, 'No errors' ); + # Scenario: + # * order with two items attached + # * one of the items is on loan + # => order is cancelled + # => item on loan is kept + # => the other item is removed + # => biblio remains untouched + # => biblio delete error notified + # => item delete error notified + + $item_1 = $builder->build_sample_item; + $item_2 = $builder->build_sample_item({ biblionumber => $item_1->biblionumber }); + my $item_3 = $builder->build_sample_item({ biblionumber => $item_1->biblionumber }); + $biblio_id = $item_1->biblionumber; + $order = $builder->build_object( + { + class => 'Koha::Acquisition::Orders', + value => { + orderstatus => 'new', + biblionumber => $biblio_id, + datecancellationprinted => undef, + cancellationreason => undef, + } + } + ); + $order->add_item( $item_1->id ); + $order->add_item( $item_2->id ); + $order->add_item( $item_3->id ); + + # Add a checkout so deleting the item fails because od 'book_on_loan' + C4::Circulation::AddIssue( $patron->unblessed, $item_2->barcode ); + C4::Reserves::AddReserve( + { + branchcode => $item_3->holdingbranch, + borrowernumber => $patron->borrowernumber, + biblionumber => $biblio_id, + itemnumber => $item_3->id, + found => 'W', + } + ); + + $order->cancel({ reason => $reason, delete_biblio => 1 }) + ->discard_changes; + + is( $order->orderstatus, 'cancelled', 'Order is marked as cancelled' ); + isnt( $order->datecancellationprinted, undef, 'datecancellationprinted is set' ); + is( $order->cancellationreason, $reason, 'cancellationreason is undef' ); + is( Koha::Items->find($item_1->id), undef, 'The item is no longer present' ); + is( ref(Koha::Items->find($item_2->id)), 'Koha::Item', 'The on loan item is still present' ); + is( ref(Koha::Biblios->find($biblio_id)), 'Koha::Biblio', 'The biblio is still present' ); + @messages = @{ $order->messages }; + is( $messages[0]->message, 'error_delitem', 'Cannot delete on loan item' ); + is( $messages[0]->payload->{item}->id, $item_2->id, 'Cannot delete on loan item' ); + is( $messages[0]->payload->{reason}, 'book_on_loan', 'Item on loan notified' ); + is( $messages[1]->message, 'error_delitem', 'Cannot delete reserved and found item' ); + is( $messages[1]->payload->{item}->id, $item_3->id, 'Cannot delete reserved and found item' ); + is( $messages[1]->payload->{reason}, 'book_reserved', 'Item reserved notified' ); + is( $messages[2]->message, 'error_delbiblio_items', 'Cannot delete on loan item' ); + is( $messages[2]->payload->{biblio}->id, $biblio_id, 'The right biblio is attached' ); + $schema->storage->txn_rollback; }; -- 2.28.0