From 114c1dfb2c0d8a14ea12e800ab57f1f942d0aa2b Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Fri, 2 Feb 2018 08:11:55 +0100
Subject: [PATCH] Bug 15494: (QA follow-up) Additional polishing

[1] Fix two typos in Circulation.t.
    Although the test does not fail, line 2127 contains two typos.
    Changing INVISILE to INVISIBLE :)
    And type should be itype.
[2] Remove $yaml as leftover from older code.
[3] Add a next when the split on /:/ does not give two results. This will
    prevent uninit warnings (although still disabled now in Circulation).
[4] For the same reason we should switch the lines for NULL and empty
    string. The undefs you insert should trigger a warn.
[5] The line for empty string should not insert undef, but empty string.
    For the same reason adding the condition defined($_) ...
    And proving it by adding two tests for the opposite values of
    callnumber and itemnotes.
[6] Adding a strip spaces around the fieldname. User friendly..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
---
 C4/Circulation.pm            |  9 +++++----
 t/db_dependent/Circulation.t | 14 ++++++++++++--
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index a929c6ca25..56ef56920f 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -4130,15 +4130,16 @@ sub _item_denied_renewal {
     my $item = $params->{item};
     return unless $item;
 
-    my $yaml = C4::Context->preference('ItemsDeniedRenewal');
-    my @lines = split /\n/, $yaml;
+    my @lines = split /\n/, C4::Context->preference('ItemsDeniedRenewal')//'';
     my $denyingrules;
     foreach my $line (@lines){
         my ($field,$array) = split /:/, $line;
+        next if !$array;
+        $field =~ s/^\s*|\s*$//g;
         $array =~ s/[ [\]\r]//g;
         my @array = split /,/, $array;
+        @array = map { $_ eq '""' || $_ eq "''" ? '' : $_ } @array;
         @array = map { $_ eq 'NULL' ? undef : $_ } @array;
-        @array = map { $_ eq '""' || $_ eq "''" ? undef : $_ } @array;
         $denyingrules->{$field} = \@array;
     }
     return unless $denyingrules;
@@ -4148,7 +4149,7 @@ sub _item_denied_renewal {
             if ( any { !defined $_ }  @{$denyingrules->{$field}} ){
                 return 1;
             }
-        } elsif (any { $val eq $_ } @{$denyingrules->{$field}}) {
+        } elsif (any { defined($_) && $val eq $_ } @{$denyingrules->{$field}}) {
            # If the results matches the values in the syspref
            # We return true if match found
             return 1;
diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index ab3435fa38..83732f37c0 100755
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -2461,7 +2461,7 @@ subtest 'CanBookBeIssued | is_overdue' => sub {
 };
 
 subtest 'ItemsDeniedRenewal preference' => sub {
-    plan tests => 16;
+    plan tests => 18;
 
     t::lib::Mocks::mock_preference( 'ItemsDeniedRenewal', '' );
 
@@ -2538,7 +2538,7 @@ subtest 'ItemsDeniedRenewal preference' => sub {
     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]\ntype: [HIDE,INVISILE]";
+    $idr_rules="withdrawn: [1]\nitype: [HIDE,INVISIBLE]";
 
     t::lib::Mocks::mock_preference( 'ItemsDeniedRenewal', $idr_rules );
     ( $idr_mayrenew, $idr_error ) =
@@ -2567,7 +2567,17 @@ subtest 'ItemsDeniedRenewal preference' => sub {
     ( $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: ['']";
+    t::lib::Mocks::mock_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]";
+    t::lib::Mocks::mock_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: ['']";
     t::lib::Mocks::mock_preference( 'ItemsDeniedRenewal', $idr_rules );
     ( $idr_mayrenew, $idr_error ) =