From b28a97fbe9007937d7b9052282d56e48886c3105 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Joonas=20Kylm=C3=A4l=C3=A4?= 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 Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- 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 6faf53add9..39ce5f252d 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2860,7 +2860,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; @@ -4432,28 +4432,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 610315ac11..e549be6d85 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -1927,6 +1927,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 23757741ce..8d52892972 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; @@ -1758,3 +1758,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.39.5