Attachment 129013 Details for Bug 29788 – Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean

[patch] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean

Bug-29788-Make-KohaItem-safetodelete-use-KohaResul.patch (text/plain), 16.60 KB, created by Tomás Cohen Arazi (tcohen) on 2022-01-04 20:07:36 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2022-01-04 20:07:36 UTC

Size: 16.60 KB

patch

obsolete

>From 464c31b019f1fb4a9df5b18539106fef605a2278 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Tue, 4 Jan 2022 15:34:48 +0100
>Subject: [PATCH] Bug 29788: Make Koha::Item->safe_to_delete use
> Koha::Result::Boolean
>
>This patch reuse the (awesome) Koha::Result::Boolean module to retrieve
>the return of Koha::Item->safe_to_delete.
>
>Test plan:
>Try to delete an item that has previously been checked out and confirm
>that you are still blocked.
>Try using the cronjobs, the item and biblio detail pages, as well as the
>batch delete item tool.
>
>Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
>---
> C4/ImportBatch.pm                             |  3 +-
> Koha/Acquisition/Order.pm                     |  4 +--
> Koha/BackgroundJob/BatchDeleteBiblio.pm       |  6 ++--
> Koha/BackgroundJob/BatchDeleteItem.pm         |  2 +-
> Koha/Item.pm                                  | 21 +++++++++-----
> 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 ++++++++++++-------
> 11 files changed, 68 insertions(+), 52 deletions(-)
>
>diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm
>index 6ae148d6d2..bf060758b1 100644
>--- a/C4/ImportBatch.pm
>+++ b/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'});
>diff --git a/Koha/Acquisition/Order.pm b/Koha/Acquisition/Order.pm
>index 090920d92c..517c88af18 100644
>--- a/Koha/Acquisition/Order.pm
>+++ b/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 }
>                 }
>             );
>         }
>diff --git a/Koha/BackgroundJob/BatchDeleteBiblio.pm b/Koha/BackgroundJob/BatchDeleteBiblio.pm
>index 6931759286..63e12935af 100644
>--- a/Koha/BackgroundJob/BatchDeleteBiblio.pm
>+++ b/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;
>diff --git a/Koha/BackgroundJob/BatchDeleteItem.pm b/Koha/BackgroundJob/BatchDeleteItem.pm
>index d5882e1c93..66f0044624 100644
>--- a/Koha/BackgroundJob/BatchDeleteItem.pm
>+++ b/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;
>diff --git a/Koha/Item.pm b/Koha/Item.pm
>index d70fbc3f8f..57a04656cf 100644
>--- a/Koha/Item.pm
>+++ b/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
>diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl
>index c98eacae84..4588b13fb5 100755
>--- a/cataloguing/additem.pl
>+++ b/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" );
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc
>index a2ea6ebd20..56707256be 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc
>+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc
>@@ -264,13 +264,16 @@
>                             [% IF item.safe_to_delete == 1 %]
>                                 <td><input type="checkbox" name="itemnumber" value="[% item.itemnumber | html %]" id="row[% item.itemnumber | html %]" checked="checked" /></td>
>                             [% 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 %]
> 
>                                 <td><input type="checkbox" name="itemnumber" value="[% item.itemnumber | html %]" id="row[% item.itemnumber | html %]" disabled="disabled" title="[% cannot_delete_reason | html %]"/></td>
>diff --git a/misc/cronjobs/delete_items.pl b/misc/cronjobs/delete_items.pl
>index 2aa9e575d9..2eab937b21 100755
>--- a/misc/cronjobs/delete_items.pl
>+++ b/misc/cronjobs/delete_items.pl
>@@ -78,12 +78,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]->message
>     }
> }
> 
>diff --git a/misc/cronjobs/delete_records_via_leader.pl b/misc/cronjobs/delete_records_via_leader.pl
>index 2794efb8d7..bc919ec6ba 100755
>--- a/misc/cronjobs/delete_records_via_leader.pl
>+++ b/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++;
>diff --git a/t/db_dependent/Circulation/IsItemIssued.t b/t/db_dependent/Circulation/IsItemIssued.t
>index b2625617ed..8238969566 100755
>--- a/t/db_dependent/Circulation/IsItemIssued.t
>+++ b/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',
> );
> 
>diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t
>index db3ff9a987..0139cd40af 100755
>--- a/t/db_dependent/Koha/Item.t
>+++ b/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'
>     );
> 
>-- 
>2.34.1

Actions: View | Diff | Splinter Review

Attachments on bug 29788: 128993 | 129010 | 129013 | 129018 | 129075 | 129076 | 129077 | 129097 | 129098 | 129169 | 129170 | 129326 | 132052