Bugzilla – Attachment 111724 Details for
Bug 26515
Add Koha::Acquisition::Order->cancel
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 26515: (QA follow-up) Preserve original behaviour
Bug-26515-QA-follow-up-Preserve-original-behaviour.patch (text/plain), 10.80 KB, created by
Tomás Cohen Arazi (tcohen)
on 2020-10-15 12:43:30 UTC
(
hide
)
Description:
Bug 26515: (QA follow-up) Preserve original behaviour
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2020-10-15 12:43:30 UTC
Size:
10.80 KB
patch
obsolete
>From ecd7f7d0027e28db313d57a0e1c65dce52c9dd34 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> >--- > 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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=111724">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 26515
:
110603
|
110604
|
110641
|
110642
|
110751
|
110752
|
110939
|
110940
|
110970
|
110971
|
110992
|
111031
|
111170
|
111172
|
111173
|
111372
|
111430
| 111724