From 3d938ffc823b0193fc61822ffeb08cb127c6682e Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 11 Dec 2013 19:06:15 +0100 Subject: [PATCH] Bug 10861: (follow-up) various refactoring 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. Signed-off-by: Kyle M Hall Signed-off-by: Galen Charlton --- 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 664a6c41f7..9b358051f6 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -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 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 8bceeb4197..0f28cef071 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. @@ -894,10 +897,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 f9bc0c7551..28ab5e0782 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -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 index 0000000000..8132c93f10 --- /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 94388b9912..0000000000 --- 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 } ); -} -- 2.39.5