From 06e23f028b496b17bc47eab86cd74a6e7582aa77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= <joonas.kylmala@iki.fi>
Date: Sun, 28 Aug 2022 23:01:19 +0300
Subject: [PATCH] Bug 31485: Move _item_denied_renewal to Koha::Item

The question whether item is denied renewal doesn't depend on
circulation rules or the patron, it is only a property of the item and
only changes to the item's attributes can cause the return value of
the check to change, thus we should move this to be a method of
Koha::Item.

To test:
 1) Run unit tests
    $ prove t/db_dependent/Circulation.t
    $ prove t/db_dependent/Koha/Item.t
---
 C4/Circulation.pm            |  24 +-------
 Koha/Item.pm                 |  29 +++++++++
 t/db_dependent/Circulation.t | 110 ++++++-----------------------------
 t/db_dependent/Koha/Item.t   |  64 +++++++++++++++++++-
 4 files changed, 111 insertions(+), 116 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 87f892ab4b..1ddacc12c6 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -2877,7 +2877,7 @@ sub CanBookBeRenewed {
     my $item      = Koha::Items->find($itemnumber)      or return ( 0, 'no_item' );
     my $issue = $item->checkout or return ( 0, 'no_checkout' );
     return ( 0, 'onsite_checkout' ) if $issue->onsite_checkout;
-    return ( 0, 'item_denied_renewal') if _item_denied_renewal({ item => $item });
+    return ( 0, 'item_denied_renewal') if $item->is_denied_renewal;
 
     my $patron = $issue->patron or return;
 
@@ -4448,28 +4448,6 @@ sub _CanBookBeAutoRenewed {
     return "ok";
 }
 
-sub _item_denied_renewal {
-    my ($params) = @_;
-
-    my $item = $params->{item};
-    return unless $item;
-
-    my $denyingrules = Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash();
-    return unless $denyingrules;
-    foreach my $field (keys %$denyingrules) {
-        my $val = $item->$field;
-        if( !defined $val) {
-            if ( any { !defined $_ }  @{$denyingrules->{$field}} ){
-                return 1;
-            }
-        } elsif (any { defined($_) && $val eq $_ } @{$denyingrules->{$field}}) {
-           # If the results matches the values in the syspref
-           # We return true if match found
-            return 1;
-        }
-    }
-    return 0;
-}
 
 1;
 
diff --git a/Koha/Item.pm b/Koha/Item.pm
index e5d4bbccd6..688e0b2b79 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -1928,6 +1928,35 @@ sub is_notforloan {
     return $is_notforloan;
 }
 
+=head3 is_denied_renewal
+
+    my $is_denied_renewal = $item->is_denied_renewal;
+
+Determine whether or not this item can be renewed based on the
+rules set in the ItemsDeniedRenewal system preference.
+
+=cut
+
+sub is_denied_renewal {
+    my ( $self ) = @_;
+
+    my $denyingrules = Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash();
+    return 0 unless $denyingrules;
+    foreach my $field (keys %$denyingrules) {
+        my $val = $self->$field;
+        if( !defined $val) {
+            if ( any { !defined $_ }  @{$denyingrules->{$field}} ){
+                return 1;
+            }
+        } elsif (any { defined($_) && $val eq $_ } @{$denyingrules->{$field}}) {
+           # If the results matches the values in the syspref
+           # We return true if match found
+            return 1;
+        }
+    }
+    return 0;
+}
+
 =head3 _type
 
 =cut
diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index 28c3238301..53fb268024 100755
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -4107,10 +4107,8 @@ subtest 'CanBookBeIssued | is_overdue' => sub {
     is( $needsconfirmation->{TOO_MANY}, undef, "Not too many, is a renewal");
 };
 
-subtest 'ItemsDeniedRenewal preference' => sub {
-    plan tests => 18;
-
-    C4::Context->set_preference('ItemsDeniedRenewal','');
+subtest 'ItemsDeniedRenewal rules are checked' => sub {
+    plan tests => 4;
 
     my $idr_lib = $builder->build_object({ class => 'Koha::Libraries'});
     Koha::CirculationRules->set_rules(
@@ -4132,15 +4130,6 @@ subtest 'ItemsDeniedRenewal preference' => sub {
         }
     );
 
-    my $deny_book = $builder->build_object({ class => 'Koha::Items', value => {
-        homebranch => $idr_lib->branchcode,
-        withdrawn => 1,
-        itype => 'HIDE',
-        location => 'PROC',
-        itemcallnumber => undef,
-        itemnotes => "",
-        }
-    });
     my $allow_book = $builder->build_object({ class => 'Koha::Items', value => {
         homebranch => $idr_lib->branchcode,
         withdrawn => 0,
@@ -4154,21 +4143,7 @@ subtest 'ItemsDeniedRenewal preference' => sub {
         }
     });
     my $future = dt_from_string->add( days => 1 );
-    my $deny_issue = $builder->build_object(
-        {
-            class => 'Koha::Checkouts',
-            value => {
-                returndate      => undef,
-                renewals_count  => 0,
-                auto_renew      => 0,
-                borrowernumber  => $idr_borrower->borrowernumber,
-                itemnumber      => $deny_book->itemnumber,
-                onsite_checkout => 0,
-                date_due        => $future,
-            }
-        }
-    );
-    my $allow_issue = $builder->build_object(
+    my $issue = $builder->build_object(
         {
             class => 'Koha::Checkouts',
             value => {
@@ -4183,70 +4158,21 @@ subtest 'ItemsDeniedRenewal preference' => sub {
         }
     );
 
-    my $idr_rules;
-
-    my ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 1, 'Renewal allowed when no rules' );
-    is( $idr_error, undef, 'Renewal allowed when no rules' );
-
-    $idr_rules="withdrawn: [1]";
-
-    C4::Context->set_preference('ItemsDeniedRenewal',$idr_rules);
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 0, 'Renewal blocked when 1 rules (withdrawn)' );
-    is( $idr_error, 'item_denied_renewal', 'Renewal blocked when 1 rule (withdrawn)' );
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $allow_issue->itemnumber );
-    is( $idr_mayrenew, 1, 'Renewal allowed when 1 rules not matched (withdrawn)' );
-    is( $idr_error, undef, 'Renewal allowed when 1 rules not matched (withdrawn)' );
-
-    $idr_rules="withdrawn: [1]\nitype: [HIDE,INVISIBLE]";
-
-    C4::Context->set_preference('ItemsDeniedRenewal',$idr_rules);
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 0, 'Renewal blocked when 2 rules matched (withdrawn, itype)' );
-    is( $idr_error, 'item_denied_renewal', 'Renewal blocked when 2 rules matched (withdrawn,itype)' );
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $allow_issue->itemnumber );
-    is( $idr_mayrenew, 1, 'Renewal allowed when 2 rules not matched (withdrawn, itype)' );
-    is( $idr_error, undef, 'Renewal allowed when 2 rules not matched (withdrawn, itype)' );
-
-    $idr_rules="withdrawn: [1]\nitype: [HIDE,INVISIBLE]\nlocation: [PROC]";
-
-    C4::Context->set_preference('ItemsDeniedRenewal',$idr_rules);
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 0, 'Renewal blocked when 3 rules matched (withdrawn, itype, location)' );
-    is( $idr_error, 'item_denied_renewal', 'Renewal blocked when 3 rules matched (withdrawn,itype, location)' );
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $allow_issue->itemnumber );
-    is( $idr_mayrenew, 1, 'Renewal allowed when 3 rules not matched (withdrawn, itype, location)' );
-    is( $idr_error, undef, 'Renewal allowed when 3 rules not matched (withdrawn, itype, location)' );
-
-    $idr_rules="itemcallnumber: [NULL]";
-    C4::Context->set_preference('ItemsDeniedRenewal',$idr_rules);
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 0, 'Renewal blocked for undef when NULL in pref' );
-    $idr_rules="itemcallnumber: ['']";
-    C4::Context->set_preference('ItemsDeniedRenewal',$idr_rules);
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 1, 'Renewal not blocked for undef when "" in pref' );
-
-    $idr_rules="itemnotes: [NULL]";
-    C4::Context->set_preference('ItemsDeniedRenewal',$idr_rules);
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 1, 'Renewal not blocked for "" when NULL in pref' );
-    $idr_rules="itemnotes: ['']";
-    C4::Context->set_preference('ItemsDeniedRenewal',$idr_rules);
-    ( $idr_mayrenew, $idr_error ) =
-    CanBookBeRenewed( $idr_borrower->borrowernumber, $deny_issue->itemnumber );
-    is( $idr_mayrenew, 0, 'Renewal blocked for empty string when "" in pref' );
+    my $mock_item_class = Test::MockModule->new("Koha::Item");
+    $mock_item_class->mock( 'is_denied_renewal', sub { return 1; } );
+
+    my ( $mayrenew, $error ) = CanBookBeRenewed( $idr_borrower->borrowernumber, $issue->itemnumber );
+    is( $mayrenew, 0, 'Renewal blocked when $item->is_denied_renewal returns true' );
+    is( $error, 'item_denied_renewal', 'Renewal blocked when $item->is_denied_renewal returns true' );
+
+    $mock_item_class->unmock( 'is_denied_renewal' );
+    $mock_item_class->mock( 'is_denied_renewal', sub { return 0; } );
+
+    ( $mayrenew, $error ) = CanBookBeRenewed( $idr_borrower->borrowernumber, $issue->itemnumber );
+    is( $mayrenew, 1, 'Renewal allowed when $item->is_denied_renewal returns false' );
+    is( $error, undef, 'Renewal allowed when $item->is_denied_renewal returns false' );
+
+    $mock_item_class->unmock( 'is_denied_renewal' );
 };
 
 subtest 'CanBookBeIssued | item-level_itypes=biblio' => sub {
diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t
index b274f0d688..134cbe6863 100755
--- a/t/db_dependent/Koha/Item.t
+++ b/t/db_dependent/Koha/Item.t
@@ -20,7 +20,7 @@
 use Modern::Perl;
 use utf8;
 
-use Test::More tests => 26;
+use Test::More tests => 27;
 use Test::Exception;
 use Test::MockModule;
 
@@ -1749,3 +1749,65 @@ subtest 'has_pending_recall() tests' => sub {
 
     $schema->storage->txn_rollback;
 };
+
+subtest 'is_denied_renewal' => sub {
+    plan tests => 11;
+
+    $schema->storage->txn_begin;
+
+    my $library = $builder->build_object({ class => 'Koha::Libraries'});
+
+    my $deny_book = $builder->build_object({ class => 'Koha::Items', value => {
+        homebranch => $library->branchcode,
+        withdrawn => 1,
+        itype => 'HIDE',
+        location => 'PROC',
+        itemcallnumber => undef,
+        itemnotes => "",
+        }
+    });
+
+    my $allow_book = $builder->build_object({ class => 'Koha::Items', value => {
+        homebranch => $library->branchcode,
+        withdrawn => 0,
+        itype => 'NOHIDE',
+        location => 'NOPROC'
+        }
+    });
+
+    my $idr_rules = "";
+    C4::Context->set_preference('ItemsDeniedRenewal', $idr_rules);
+    is( $deny_book->is_denied_renewal, 0, 'Renewal allowed when no rules' );
+
+    $idr_rules="withdrawn: [1]";
+    C4::Context->set_preference('ItemsDeniedRenewal', $idr_rules);
+    is( $deny_book->is_denied_renewal, 1, 'Renewal blocked when 1 rules (withdrawn)' );
+    is( $allow_book->is_denied_renewal, 0, 'Renewal allowed when 1 rules not matched (withdrawn)' );
+
+    $idr_rules="withdrawn: [1]\nitype: [HIDE,INVISIBLE]";
+    is( $deny_book->is_denied_renewal, 1, 'Renewal blocked when 2 rules matched (withdrawn, itype)' );
+    is( $allow_book->is_denied_renewal, 0, 'Renewal allowed when 2 rules not matched (withdrawn, itype)' );
+
+    $idr_rules="withdrawn: [1]\nitype: [HIDE,INVISIBLE]\nlocation: [PROC]";
+    C4::Context->set_preference('ItemsDeniedRenewal', $idr_rules);
+    is( $deny_book->is_denied_renewal, 1, 'Renewal blocked when 3 rules matched (withdrawn, itype, location)' );
+    is( $allow_book->is_denied_renewal, 0, 'Renewal allowed when 3 rules not matched (withdrawn, itype, location)' );
+
+    $idr_rules="itemcallnumber: [NULL]";
+    C4::Context->set_preference('ItemsDeniedRenewal', $idr_rules);
+    is( $deny_book->is_denied_renewal, 1, 'Renewal blocked for undef when NULL in pref' );
+
+    $idr_rules="itemcallnumber: ['']";
+    C4::Context->set_preference('ItemsDeniedRenewal', $idr_rules);
+    is( $deny_book->is_denied_renewal, 0, 'Renewal not blocked for undef when "" in pref' );
+
+    $idr_rules="itemnotes: [NULL]";
+    C4::Context->set_preference('ItemsDeniedRenewal', $idr_rules);
+    is( $deny_book->is_denied_renewal, 0, 'Renewal not blocked for "" when NULL in pref' );
+
+    $idr_rules="itemnotes: ['']";
+    C4::Context->set_preference('ItemsDeniedRenewal', $idr_rules);
+    is( $deny_book->is_denied_renewal, 1, 'Renewal blocked for empty string when "" in pref' );
+
+    $schema->storage->txn_rollback;
+};
-- 
2.30.2