Attachment 80331 Details for Bug 15494 – Bug 15494: (QA follow-up) Additional polishing

[patch] Bug 15494: (QA follow-up) Additional polishing

Bug-15494-QA-follow-up-Additional-polishing.patch (text/plain), 4.42 KB, created by Nick Clemens (kidclamp) on 2018-10-10 11:00:51 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2018-10-10 11:00:51 UTC

Size: 4.42 KB

patch

obsolete

>From f2a156e4d1bdfa440b3bf33d62e0a118a3f41b5f 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>
>---
> 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 8b459fb..0ec7941 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -4108,15 +4108,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;
>@@ -4126,7 +4127,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 e5b225d..a491529 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -2212,7 +2212,7 @@ subtest 'CanBookBeIssued | is_overdue' => sub {
> };
> 
> subtest 'ItemsDeniedRenewal preference' => sub {
>-    plan tests => 16;
>+    plan tests => 18;
> 
>     t::lib::Mocks::mock_preference( 'ItemsDeniedRenewal', '' );
> 
>@@ -2289,7 +2289,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 ) =
>@@ -2318,7 +2318,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 ) =
>-- 
>2.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 15494: 63700 | 63701 | 63702 | 63703 | 67886 | 67887 | 67888 | 67889 | 68146 | 68147 | 68148 | 68149 | 70295 | 70629 | 70630 | 70631 | 70632 | 71129 | 71130 | 71131 | 71132 | 71133 | 71134 | 71135 | 71136 | 71137 | 71138 | 79249 | 79250 | 79251 | 79252 | 79253 | 79254 | 79293 | 80327 | 80328 | 80329 | 80330 | 80331 | 80332 | 80333 | 81411 | 81412 | 81413 | 81414 | 81415 | 81416 | 81417