Attachment 23734 Details for Bug 10861 – Bug 10861: Followup Add a check on cardnumber length

[patch] Bug 10861: Followup Add a check on cardnumber length

Bug-10861-Followup-Add-a-check-on-cardnumber-lengt.patch (text/plain), 13.05 KB, created by Kyle M Hall (khall) on 2013-12-20 19:21:09 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2013-12-20 19:21:09 UTC

Size: 13.05 KB

patch

obsolete

>From ef84aa167ecb693d63e41a2ffec8ed98280a9339 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
>
>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>
>---
> C4/Members.pm                                      |   33 +++++---
> .../prog/en/modules/members/memberentrygen.tt      |   13 ++-
> members/memberentry.pl                             |   35 ++++-----
> t/Members/cardnumber.t                             |   80 ++++++++++++++++++++
> t/Members/checkcardnumber.t                        |   66 ----------------
> 5 files changed, 124 insertions(+), 103 deletions(-)
> create mode 100644 t/Members/cardnumber.t
> delete mode 100644 t/Members/checkcardnumber.t
>
>diff --git a/C4/Members.pm b/C4/Members.pm
>index cbcb78f..069aacd 100644
>--- a/C4/Members.pm
>+++ b/C4/Members.pm
>@@ -1345,26 +1345,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
>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..8fba18e 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,10 +889,12 @@
>             <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>
>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/cardnumber.t b/t/Members/cardnumber.t
>new file mode 100644
>index 0000000..8132c93
>--- /dev/null
>+++ b/t/Members/cardnumber.t
>@@ -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
>index 94388b9..0000000
>--- a/t/Members/checkcardnumber.t
>+++ /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 } );
>-}
>-- 
>1.7.2.5

Actions: View | Diff | Splinter Review

Attachments on bug 10861: 20950 | 21196 | 21586 | 23387 | 23388 | 23439 | 23476 | 23477 | 23563 | 23564 | 23565 | 23732 | 23733 | 23734 | 23735 | 23736