Attachment 140506 Details for Bug 31485 – Bug 31485: Move _item_denied_renewal to Koha::Item

[patch] Bug 31485: Move _item_denied_renewal to Koha::Item

Bug-31485-Move-itemdeniedrenewal-to-KohaItem.patch (text/plain), 12.99 KB, created by David Nind on 2022-09-12 20:11:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2022-09-12 20:11:32 UTC

Size: 12.99 KB

patch

obsolete

>From a1dca00c8697bb43635c2296ceb680814275ef7a 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
>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> 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 58ceeae3d1..4f4e030ca7 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -2854,7 +2854,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;
> 
>@@ -4425,28 +4425,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

Actions: View | Diff | Splinter Review

Attachments on bug 31485: 139965 | 140506 | 141533