Bug 10861: (follow-up) various refactoring
authorJonathan Druart <jonathan.druart@biblibre.com>
Wed, 11 Dec 2013 18:06:15 +0000 (19:06 +0100)
committerGalen Charlton <gmc@esilibrary.com>
Wed, 12 Mar 2014 02:17:34 +0000 (02:17 +0000)
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>
Bug 10861: Reintroduced the cardnumber length check (client side)

Previous patches has removed the pattern attribute of the input, it was
not needed. This patch reintroduces it. It will only work for new
browser version.

Moreover, it manages with the ',XX' format (see UT).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Squashed the last two follow-ups. The pattern test did not work fully for me
in Firefox 26 (very recent). But I see the message when I clear the field.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
C4/Members.pm
koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
members/memberentry.pl
t/Members/cardnumber.t [new file with mode: 0644]
t/Members/checkcardnumber.t [deleted file]

index 664a6c4..9b35805 100644 (file)
@@ -1341,26 +1341,33 @@ 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 ) = ( 1, 16 ); # borrowers.cardnumber is a varchar(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
+                if $cardnumber_length >= $min
+                    and $cardnumber_length <= $max;
         }
         # 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 if $1 and $min < $1;
+            $max = $2 if $2 and $max > $2;
         }
-        return 1
+
     }
-    return 0;
+    return ( $min, $max );
 }
 
-
 =head2 getzipnamecity (OUEST-PROVENCE)
 
 take all info from table city for the fields city and  zip
index 8bceeb4..0f28cef 100644 (file)
                        [% 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.  
             <label for="cardnumber">
           [% END %]
           Card number: </label>
-          [% IF minlength_cardnumber && maxlength_cardnumber %]
+          [% IF minlength_cardnumber == maxlength_cardnumber %]
+              <input type="text" id="cardnumber" name="cardnumber" size="20" value="[% cardnumber %]" pattern=".{[% minlength_cardnumber %]}" title="Exactly [% minlength_cardnumber %] characters" />
+          [% ELSIF 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 %]" />
+               <input type="text" id="cardnumber" name="cardnumber" size="20" value="[% cardnumber %]" />
           [% END %]
           [% IF ( mandatorycardnumber ) %]<span class="required">Required</span>[% END %]
         </li>
index f9bc0c7..28ab5e0 100755 (executable)
@@ -293,9 +293,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'});   
@@ -711,23 +716,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/cardnumber.t b/t/Members/cardnumber.t
new file mode 100644 (file)
index 0000000..8132c93
--- /dev/null
@@ -0,0 +1,80 @@
+#!/usr/bin/env perl
+
+use Modern::Perl;
+use Test::More tests => 22;
+
+use Test::MockModule;
+use DBD::Mock;
+
+use_ok('C4::Members');
+
+my $module_context = new Test::MockModule('C4::Context');
+$module_context->mock(
+    '_new_dbh',
+    sub {
+        my $dbh = DBI->connect( 'DBI:Mock:', '', '' )
+          || die "Cannot create handle: $DBI::errstr\n";
+        return $dbh;
+    }
+);
+
+my $dbh = C4::Context->dbh;
+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} ), 2, "123456789 is shorter than $pref");
+$dbh->{mock_add_resultset} = $rs;
+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} ), 2, "123456789 is shorter than $pref");
+$dbh->{mock_add_resultset} = $rs;
+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{1234567890123456} ), 2, "1234567890123456 is longer than $pref");
+$dbh->{mock_add_resultset} = $rs;
+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} ), 2, "1234567 is shorter than $pref");
+$dbh->{mock_add_resultset} = $rs;
+is( C4::Members::checkcardnumber( q{1234567890123456} ), 0, "1234567890123456 matches $pref");
+$dbh->{mock_add_resultset} = $rs;
+is( C4::Members::checkcardnumber( q{1234567890} ), 0, "1234567890 matches $pref");
+
+$pref = q|,8|; # max 8 chars
+set_pref( $module_context, $pref );
+is_deeply( [ C4::Members::get_cardnumber_length() ], [ 1, 8 ], ',8 => min=1 and max=8');
+$dbh->{mock_add_resultset} = $rs;
+is( C4::Members::checkcardnumber( q{1234567} ), 0, "1234567 matches $pref");
+$dbh->{mock_add_resultset} = $rs;
+is( C4::Members::checkcardnumber( q{1234567890123456} ), 2, "1234567890123456 is longer than $pref");
+$dbh->{mock_add_resultset} = $rs;
+is( C4::Members::checkcardnumber( q{1234567890} ), 2, "1234567890 is longer than $pref");
+
+
+sub set_pref {
+    my ( $module, $value ) = @_;
+    $module->mock('preference', sub { return $value } );
+}
diff --git a/t/Members/checkcardnumber.t b/t/Members/checkcardnumber.t
deleted file mode 100644 (file)
index 94388b9..0000000
+++ /dev/null
@@ -1,66 +0,0 @@
-#!/usr/bin/env perl
-
-use Modern::Perl;
-use Test::More tests =>14;
-
-use Test::MockModule;
-use DBD::Mock;
-
-use_ok('C4::Members');
-
-my $module_context = new Test::MockModule('C4::Context');
-$module_context->mock(
-    '_new_dbh',
-    sub {
-        my $dbh = DBI->connect( 'DBI:Mock:', '', '' )
-          || die "Cannot create handle: $DBI::errstr\n";
-        return $dbh;
-    }
-);
-
-my $dbh = C4::Context->dbh;
-my $rs = [];
-
-my $pref = "10";
-set_pref( $module_context, $pref );
-
-$dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{123456789} ), 1, "123456789 is shorter than $pref");
-$dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{12345678901234567890} ), 1, "12345678901234567890 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 );
-$dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{123456789} ), 1, "123456789 is shorter than $pref");
-$dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{12345678901234567890} ), 1, "12345678901234567890 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 );
-$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");
-$dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{1234567} ), 1, "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 );
-$dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{1234567} ), 1, "1234567 is shorter than $pref");
-$dbh->{mock_add_resultset} = $rs;
-is( C4::Members::checkcardnumber( q{12345678901234567890} ), 0, "12345678901234567890 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 } );
-}