From 0e7924c61733ee2a67e8ea16753be452ce7799a2 Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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 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 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. --- 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 ) %]
  • Username/password already exists.
  • [% END %] - [% IF ( ERROR_cardnumber ) %] -
  • Cardnumber already in use or not in a good format.
  • + [% IF ERROR_cardnumber_already_exists %] +
  • Cardnumber already in use.
  • + [% END %] + [% IF ERROR_cardnumber_length %] +
  • Cardnumber length is incorrect.
  • [% END %] [% IF ( ERROR_age_limitations ) %]
  • Patron's age is incorrect for their category. @@ -886,10 +889,12 @@ - [% IF minlength_cardnumber && maxlength_cardnumber %] + [% IF minlength_cardnumber == maxlength_cardnumber %] + + [% ELSIF minlength_cardnumber && maxlength_cardnumber %] [% ELSE %] - + [% END %] [% IF ( mandatorycardnumber ) %]Required[% 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/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.7.6