From b8a933c30a094c9002d875da72055bbe20204afa Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@biblibre.com>
Date: Wed, 11 Dec 2013 19:06:15 +0100
Subject: [PATCH] Bug 10861: Followup Add a check on cardnumber length
Content-Type: text/plain; charset=utf-8

This patch refactors the previous code and moves the logic from the pl
to a new routine.

Same test plan as previous patch.

/!\ new unit test filename.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 C4/Members.pm                                      |   31 ++++++++++-------
 .../prog/en/modules/members/memberentrygen.tt      |   11 +++---
 members/memberentry.pl                             |   35 ++++++++-----------
 t/Members/{checkcardnumber.t => cardnumber.t}      |   25 ++++++++------
 4 files changed, 53 insertions(+), 49 deletions(-)
 rename t/Members/{checkcardnumber.t => cardnumber.t} (62%)

diff --git a/C4/Members.pm b/C4/Members.pm
index cbcb78f..fb0f03b 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -1345,26 +1345,31 @@ sub checkcardnumber {
 
     return 1 if $sth->fetchrow_hashref;
 
-    if ( my $length = C4::Context->preference('CardnumberLength') ) {
+    my ( $min_length, $max_length ) = get_cardnumber_length();
+    return 2
+        if length $cardnumber > $max_length
+        or length $cardnumber < $min_length;
+
+    return 0;
+}
+
+sub get_cardnumber_length {
+    my ( $min, $max ) = ( 0, 16 );
+    if ( my $cardnumber_length = C4::Context->preference('CardnumberLength') ) {
         # Is integer and length match
-        if (
-            $length =~ m|^\d+$|
-                and length $cardnumber == $length
-        ) {
-            return 0
+        if ( $cardnumber_length =~ m|^\d+$| ) {
+            $min = $max = $cardnumber_length;
         }
         # Else assuming it is a range
-        else {
-            my $qr = qr|^\d{$length}$|;
-            return 0
-                if $cardnumber =~ $qr;
+        elsif ( $cardnumber_length =~ m|(\d+),(\d*)| ) {
+            $min = $1;
+            $max = $2 || 16; # borrowers.cardnumber is a varchar(16)
         }
-        return 1
+
     }
-    return 0;
+    return ( $min, $max );
 }
 
-
 =head2 getzipnamecity (OUEST-PROVENCE)
 
 take all info from table city for the fields city and  zip
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
index 10e3ef1..8b5a7d6 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ -178,8 +178,11 @@
 			[% IF ( ERROR_login_exist ) %]
 				<li id="ERROR_login_exist">Username/password already exists.</li>
 			[% END %]
-            [% IF ( ERROR_cardnumber ) %]
-                <li id="ERROR_cardnumber">Cardnumber already in use or not in a good format.</li>
+            [% IF ERROR_cardnumber_already_exists %]
+                <li id="ERROR_cardnumber">Cardnumber already in use.</li>
+            [% END %]
+            [% IF ERROR_cardnumber_length %]
+                <li id="ERROR_cardnumber">Cardnumber length is incorrect.</li>
             [% END %]
 			[% IF ( ERROR_age_limitations ) %]
 				<li id="ERROR_age_limitations">Patron's age is incorrect for their category.  
@@ -886,11 +889,7 @@
             <label for="cardnumber">
           [% END %]
           Card number: </label>
-          [% IF minlength_cardnumber && maxlength_cardnumber %]
-              <input type="text" id="cardnumber" name="cardnumber" size="20" value="[% cardnumber %]" pattern=".{[% minlength_cardnumber %],[% maxlength_cardnumber %]}" title="between [% minlength_cardnumber %] and [% maxlength_cardnumber %] characters" />
-          [% ELSE %]
               <input type="text" id="cardnumber" name="cardnumber" size="20" value="[% cardnumber %]" />
-          [% END %]
           [% IF ( mandatorycardnumber ) %]<span class="required">Required</span>[% END %]
         </li>
       [% END %]
diff --git a/members/memberentry.pl b/members/memberentry.pl
index 93a5e5e..fcf4967 100755
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -294,9 +294,14 @@ if ($op eq 'save' || $op eq 'insert'){
     # If the cardnumber is blank, treat it as null.
     $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/;
 
-    if (checkcardnumber($newdata{cardnumber},$newdata{borrowernumber})){ 
-        push @errors, 'ERROR_cardnumber';
-    } 
+    if (my $error_code = checkcardnumber($newdata{cardnumber},$newdata{borrowernumber})){
+        push @errors, $error_code == 1
+            ? 'ERROR_cardnumber_already_exists'
+            : $error_code == 2
+                ? 'ERROR_cardnumber_length'
+                : ()
+    }
+
     if ($newdata{dateofbirth} && $dateofbirthmandatory) {
         my $age = GetAge($newdata{dateofbirth});
         my $borrowercategory=GetBorrowercategory($newdata{'categorycode'});   
@@ -751,23 +756,13 @@ if(defined($data{'contacttitle'})){
   $template->param("contacttitle_" . $data{'contacttitle'} => "SELECTED");
 }
 
-if ( my $cardnumber_length = C4::Context->preference('CardnumberLength') ) {
-    my ( $min, $max );
-    # Is integer and length match
-    if ( $cardnumber_length =~ m|^\d+$| ) {
-        $min = $max = $cardnumber_length;
-    }
-    # Else assuming it is a range
-    elsif ( $cardnumber_length =~ m|(\d+),(\d*)| ) {
-        $min = $1;
-        $max = $2 || 16; # borrowers.cardnumber is a varchar(16)
-    }
-    if ( defined $min ) {
-        $template->param(
-            minlength_cardnumber => $min,
-            maxlength_cardnumber => $max
-        );
-    }
+
+my ( $min, $max ) = C4::Members::get_cardnumber_length();
+if ( defined $min ) {
+    $template->param(
+        minlength_cardnumber => $min,
+        maxlength_cardnumber => $max
+    );
 }
 
 output_html_with_http_headers $input, $cookie, $template->output;
diff --git a/t/Members/checkcardnumber.t b/t/Members/cardnumber.t
similarity index 62%
rename from t/Members/checkcardnumber.t
rename to t/Members/cardnumber.t
index 94388b9..2fe606c 100644
--- a/t/Members/checkcardnumber.t
+++ b/t/Members/cardnumber.t
@@ -1,7 +1,7 @@
 #!/usr/bin/env perl
 
 use Modern::Perl;
-use Test::More tests =>14;
+use Test::More tests =>18;
 
 use Test::MockModule;
 use DBD::Mock;
@@ -23,43 +23,48 @@ my $rs = [];
 
 my $pref = "10";
 set_pref( $module_context, $pref );
-
+is_deeply( [ C4::Members::get_cardnumber_length() ], [ 10, 10 ], '10 => min=10 and max=10');
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{123456789} ), 1, "123456789 is shorter than $pref");
+is( C4::Members::checkcardnumber( q{123456789} ), 2, "123456789 is shorter than $pref");
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{12345678901234567890} ), 1, "12345678901234567890 is longer than $pref");
+is( C4::Members::checkcardnumber( q{1234567890123456} ), 2, "1234567890123456 is longer than $pref");
 $dbh->{mock_add_resultset} = $rs;
 is( C4::Members::checkcardnumber( q{1234567890} ), 0, "1234567890 is equal to $pref");
 
 $pref = q|10,10|; # Same as before !
 set_pref( $module_context, $pref );
+is_deeply( [ C4::Members::get_cardnumber_length() ], [ 10, 10 ], '10,10 => min=10 and max=10');
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{123456789} ), 1, "123456789 is shorter than $pref");
+is( C4::Members::checkcardnumber( q{123456789} ), 2, "123456789 is shorter than $pref");
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{12345678901234567890} ), 1, "12345678901234567890 is longer than $pref");
+is( C4::Members::checkcardnumber( q{1234567890123456} ), 2, "1234567890123456 is longer than $pref");
 $dbh->{mock_add_resultset} = $rs;
 is( C4::Members::checkcardnumber( q{1234567890} ), 0, "1234567890 is equal to $pref");
 
 $pref = q|8,10|; # between 8 and 10 chars
 set_pref( $module_context, $pref );
+is_deeply( [ C4::Members::get_cardnumber_length() ], [ 8, 10 ], '8,10 => min=8 and max=10');
 $dbh->{mock_add_resultset} = $rs;
 is( C4::Members::checkcardnumber( q{12345678} ), 0, "12345678 matches $pref");
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{12345678901234567890} ), 1, "12345678901234567890 is longer than $pref");
+is( C4::Members::checkcardnumber( q{1234567890123456} ), 2, "1234567890123456 is longer than $pref");
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{1234567} ), 1, "1234567 is shorter than $pref");
+is( C4::Members::checkcardnumber( q{1234567} ), 2, "1234567 is shorter than $pref");
 $dbh->{mock_add_resultset} = $rs;
 is( C4::Members::checkcardnumber( q{1234567890} ), 0, "1234567890 matches $pref");
 
 $pref = q|8,|; # At least 8 chars
 set_pref( $module_context, $pref );
+is_deeply( [ C4::Members::get_cardnumber_length() ], [ 8, 16 ], '8, => min=8 and max=16');
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{1234567} ), 1, "1234567 is shorter than $pref");
+is( C4::Members::checkcardnumber( q{1234567} ), 2, "1234567 is shorter than $pref");
 $dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{12345678901234567890} ), 0, "12345678901234567890 matches $pref");
+is( C4::Members::checkcardnumber( q{1234567890123456} ), 0, "1234567890123456 matches $pref");
 $dbh->{mock_add_resultset} = $rs;
 is( C4::Members::checkcardnumber( q{1234567890} ), 0, "12345678 matches $pref");
 
+
+
 sub set_pref {
     my ( $module, $value ) = @_;
     $module->mock('preference', sub { return $value } );
-- 
1.7.7.6