@@ -, +, @@ Koha::Result::Boolean --- C4/ImportBatch.pm | 3 +- Koha/Acquisition/Order.pm | 4 +-- Koha/BackgroundJob/BatchDeleteBiblio.pm | 6 ++-- Koha/BackgroundJob/BatchDeleteItem.pm | 2 +- Koha/Item.pm | 21 +++++++++----- Koha/Result/Boolean.pm | 1 + cataloguing/additem.pl | 13 ++++----- .../prog/en/includes/html_helpers.inc | 17 ++++++----- misc/cronjobs/delete_items.pl | 4 +-- misc/cronjobs/delete_records_via_leader.pl | 14 +++++---- t/db_dependent/Circulation/IsItemIssued.t | 7 ++--- t/db_dependent/Koha/Item.t | 29 ++++++++++++------- 12 files changed, 69 insertions(+), 52 deletions(-) --- a/C4/ImportBatch.pm +++ a/C4/ImportBatch.pm @@ -937,8 +937,7 @@ sub BatchRevertItems { $sth->execute(); while (my $row = $sth->fetchrow_hashref()) { my $item = Koha::Items->find($row->{itemnumber}); - my $error = $item->safe_delete; - if ($error eq '1'){ + if ($item->safe_delete){ my $updsth = $dbh->prepare("UPDATE import_items SET status = ? WHERE import_items_id = ?"); $updsth->bind_param(1, 'reverted'); $updsth->bind_param(2, $row->{'import_items_id'}); --- a/Koha/Acquisition/Order.pm +++ a/Koha/Acquisition/Order.pm @@ -127,11 +127,11 @@ sub cancel { my $items = $self->items; while ( my $item = $items->next ) { my $deleted = $item->safe_delete; - unless ( ref($deleted) eq 'Koha::Item' ) { + unless ( $deleted ) { $self->add_message( { message => 'error_delitem', - payload => { item => $item, reason => $deleted } + payload => { item => $item, reason => @{$deleted->messages}[0]->messages } } ); } --- a/Koha/BackgroundJob/BatchDeleteBiblio.pm +++ a/Koha/BackgroundJob/BatchDeleteBiblio.pm @@ -112,14 +112,14 @@ sub process { # Delete items my $items = Koha::Items->search({ biblionumber => $biblionumber }); while ( my $item = $items->next ) { - my $error = $item->safe_delete; - if(ref($error) ne 'Koha::Item'){ + my $deleted = $item->safe_delete; + if( $deleted ) { push @messages, { type => 'error', code => 'item_not_deleted', biblionumber => $biblionumber, itemnumber => $item->itemnumber, - error => $error, + error => @{$deleted->messages}[0]->messages, }; $schema->storage->txn_rollback; $job->progress( ++$job_progress )->store; --- a/Koha/BackgroundJob/BatchDeleteItem.pm +++ a/Koha/BackgroundJob/BatchDeleteItem.pm @@ -114,7 +114,7 @@ sub process { my $item = Koha::Items->find($record_id) || next; my $return = $item->safe_delete; - unless ( ref($return) ) { + unless ( $return ) { # FIXME Do we need to rollback the whole transaction if a deletion failed? push @not_deleted_itemnumbers, $item->itemnumber; --- a/Koha/Item.pm +++ a/Koha/Item.pm @@ -45,6 +45,7 @@ use Koha::Libraries; use Koha::StockRotationItem; use Koha::StockRotationRotas; use Koha::TrackedLinks; +use Koha::Result::Boolean; use base qw(Koha::Object); @@ -248,7 +249,7 @@ sub safe_delete { my $params = @_ ? shift : {}; my $safe_to_delete = $self->safe_to_delete; - return $safe_to_delete unless $safe_to_delete eq '1'; + return $safe_to_delete unless $safe_to_delete; $self->move_to_deleted; @@ -274,22 +275,24 @@ returns 1 if the item is safe to delete, sub safe_to_delete { my ($self) = @_; - return "book_on_loan" if $self->checkout; + my $error; - return "not_same_branch" + $error = "book_on_loan" if $self->checkout; + + $error = "not_same_branch" if defined C4::Context->userenv and !C4::Context->IsSuperLibrarian() and C4::Context->preference("IndependentBranches") and ( C4::Context->userenv->{branch} ne $self->homebranch ); # check it doesn't have a waiting reserve - return "book_reserved" + $error = "book_reserved" if $self->holds->search( { found => [ 'W', 'T' ] } )->count; - return "linked_analytics" + $error = "linked_analytics" if C4::Items::GetAnalyticsCount( $self->itemnumber ) > 0; - return "last_item_for_hold" + $error = "last_item_for_hold" if $self->biblio->items->count == 1 && $self->biblio->holds->search( { @@ -297,7 +300,11 @@ sub safe_to_delete { } )->count; - return 1; + if ( $error ) { + return Koha::Result::Boolean->new(0)->add_message({ message => $error }); + } + + return Koha::Result::Boolean->new(1); } =head3 move_to_deleted --- a/Koha/Result/Boolean.pm +++ a/Koha/Result/Boolean.pm @@ -21,6 +21,7 @@ package Koha::Result::Boolean; use Modern::Perl; use overload bool => \&as_bool; +use overload '==' => \&as_bool; use Koha::Object::Message; --- a/cataloguing/additem.pl +++ a/cataloguing/additem.pl @@ -376,11 +376,11 @@ if ($op eq "additem") { #------------------------------------------------------------------------------- # check that there is no issue on this item before deletion. my $item = Koha::Items->find($itemnumber); - my $error = $item->safe_delete; - if(ref($error) eq 'Koha::Item'){ + my $deleted = $item->safe_delete; + if($deleted) { print $input->redirect("additem.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid"); - }else{ - push @errors,$error; + } else { + push @errors, @{$deleted->messages}[0]->message; $nextop="additem"; } #------------------------------------------------------------------------------- @@ -388,9 +388,8 @@ if ($op eq "additem") { #------------------------------------------------------------------------------- my $items = Koha::Items->search({ biblionumber => $biblionumber }); while ( my $item = $items->next ) { - my $error = $item->safe_delete({ skip_record_index => 1 }); - next if ref $error eq 'Koha::Item'; # Deleted item is returned if deletion successful - push @errors,$error; + my $deleted = $item->safe_delete({ skip_record_index => 1 }); + push @errors, @{$deleted->messages}[0]->message; } my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" ); --- a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc +++ a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc @@ -263,13 +263,16 @@ [% IF item.safe_to_delete == 1 %] [% ELSE %] - [% SWITCH item.safe_to_delete%] - [% CASE "book_on_loan" %][% SET cannot_delete_reason = t("Item is checked out") %] - [% CASE "not_same_branch" %][% SET cannot_delete_reason = t("Item does not belong to your library") %] - [% CASE "book_reserved" %][% SET cannot_delete_reason = t("Item has a waiting hold") %] - [% CASE "linked_analytics" %][% SET cannot_delete_reason = t("Item has linked analytics") %] - [% CASE "last_item_for_hold" %][% SET cannot_delete_reason = t("Last item for bibliographic record with biblio-level hold on it") %] - [% CASE %][% SET cannot_delete_reason = t("Unknown reason") _ '(' _ item.safe_to_delete _ ')' %] + [% SET messages = item.safe_to_delete.messages %] + [% FOR m IN messages %] + [% SWITCH m %] + [% CASE "book_on_loan" %][% SET cannot_delete_reason = t("Item is checked out") %] + [% CASE "not_same_branch" %][% SET cannot_delete_reason = t("Item does not belong to your library") %] + [% CASE "book_reserved" %][% SET cannot_delete_reason = t("Item has a waiting hold") %] + [% CASE "linked_analytics" %][% SET cannot_delete_reason = t("Item has linked analytics") %] + [% CASE "last_item_for_hold" %][% SET cannot_delete_reason = t("Last item for bibliographic record with biblio-level hold on it") %] + [% CASE %][% SET cannot_delete_reason = t("Unknown reason") _ '(' _ item.safe_to_delete _ ')' %] + [% END %] [% END %] --- a/misc/cronjobs/delete_items.pl +++ a/misc/cronjobs/delete_items.pl @@ -60,12 +60,12 @@ DELITEM: while ( my $item = $GLOBAL->{sth}->{target_items}->fetchrow_hashref() ) my $item_object = Koha::Items->find($item->{itemnumber}); my $safe_to_delete = $item_object->safe_to_delete; - if( $safe_to_delete eq '1' ) { + if( $safe_to_delete ) { $item_object->safe_delete if $OPTIONS->{flags}->{commit}; verbose "Deleting '$item->{itemnumber}'"; } else { - verbose "Item '$item->{itemnumber}' not deleted: $safe_to_delete"; + verbose sprintf "Item '%s' not deleted: %s", $item->{itemnumber}, @{$safe_to_delete->messages}[0] } } --- a/misc/cronjobs/delete_records_via_leader.pl +++ a/misc/cronjobs/delete_records_via_leader.pl @@ -91,19 +91,21 @@ foreach my $m (@metadatas) { my $itemnumber = $item->itemnumber; if( $test ){ - my $result = $item->safe_to_delete; - if ( $result eq "1") { + my $deleted = $item->safe_to_delete; + if ( $deleted ) { say "TEST MODE: Item $itemnumber would have been deleted"; } else { - say "TEST MODE: ERROR DELETING ITEM $itemnumber: $result"; + my $error = @{$deleted->messages}[0]->message + say "TEST MODE: ERROR DELETING ITEM $itemnumber: $error"; } } else { - my $result = $item->safe_delete; - if ( ref $result eq "Koha::Item" ){ + my $deleted = $item->safe_to_delete; + if ( $deleted ) { say "DELETED ITEM $itemnumber" if $verbose; $deleted_items_count++; } else { - say "ERROR DELETING ITEM $itemnumber: $result"; + my $error = @{$deleted->messages}[0]->message + say "ERROR DELETING ITEM $itemnumber: $error"; } } $total_items_count++; --- a/t/db_dependent/Circulation/IsItemIssued.t +++ a/t/db_dependent/Circulation/IsItemIssued.t @@ -67,7 +67,7 @@ AddIssue($borrower, $item->barcode); is ( IsItemIssued( $item->itemnumber ), 1, "item is now on loan" ); is( - $item->safe_delete, + @{$item->safe_delete->messages}[0]->message, 'book_on_loan', 'item that is on loan cannot be deleted', ); @@ -77,9 +77,8 @@ is ( IsItemIssued( $item->itemnumber ), 0, "item has been returned" ); $item->discard_changes; # FIXME We should not need that # If we do not, $self->checkout in safe_to_delete will not know the item has been checked out -is( - ref($item->safe_delete), - 'Koha::Item', +ok( + $item->safe_delete, 'item that is not on loan can be deleted', ); --- a/t/db_dependent/Koha/Item.t +++ a/t/db_dependent/Koha/Item.t @@ -571,7 +571,7 @@ subtest 'request_transfer' => sub { }; subtest 'deletion' => sub { - plan tests => 12; + plan tests => 13; $schema->storage->txn_begin; @@ -606,17 +606,22 @@ subtest 'deletion' => sub { C4::Circulation::AddIssue( $patron->unblessed, $item->barcode ); is( - $item->safe_to_delete, + @{$item->safe_to_delete->messages}[0]->message, 'book_on_loan', 'Koha::Item->safe_to_delete reports item on loan', ); is( - $item->safe_delete, + @{$item->safe_to_delete->messages}[0]->message, 'book_on_loan', 'item that is on loan cannot be deleted', ); + ok( + ! $item->safe_to_delete, + 'Koha::Item->safe_to_delete shows item NOT safe to delete' + ); + AddReturn( $item->barcode, $library->branchcode ); # book_reserved is tested in t/db_dependent/Reserves.t @@ -626,13 +631,13 @@ subtest 'deletion' => sub { my $item_2 = $builder->build_sample_item({ library => $library_2->branchcode }); is( - $item_2->safe_to_delete, + @{$item_2->safe_to_delete->messages}[0]->message, 'not_same_branch', 'Koha::Item->safe_to_delete reports IndependentBranches restriction', ); is( - $item_2->safe_delete, + @{$item_2->safe_to_delete->messages}[0]->message, 'not_same_branch', 'IndependentBranches prevents deletion at another branch', ); @@ -646,13 +651,13 @@ subtest 'deletion' => sub { $item->discard_changes; is( - $item->safe_to_delete, + @{$item->safe_to_delete->messages}[0]->message, 'linked_analytics', 'Koha::Item->safe_to_delete reports linked analytics', ); is( - $item->safe_delete, + @{$item->safe_to_delete->messages}[0]->message, 'linked_analytics', 'Linked analytics prevents deletion of item', ); @@ -661,15 +666,17 @@ subtest 'deletion' => sub { { # last_item_for_hold C4::Reserves::AddReserve({ branchcode => $patron->branchcode, borrowernumber => $patron->borrowernumber, biblionumber => $item->biblionumber }); - is( $item->safe_to_delete, 'last_item_for_hold', 'Item cannot be deleted if a biblio-level is placed on the biblio and there is only 1 item attached to the biblio' ); - + is( + @{$item->safe_to_delete->messages}[0]->message, + 'last_item_for_hold', + 'Item cannot be deleted if a biblio-level is placed on the biblio and there is only 1 item attached to the biblio' + ); # With another item attached to the biblio, the item can be deleted $builder->build_sample_item({ biblionumber => $item->biblionumber }); } - is( + ok( $item->safe_to_delete, - 1, 'Koha::Item->safe_to_delete shows item safe to delete' ); --