From 8b0c657ab86ff8e18fa0fe0a2929f0b728d367e5 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 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. --- 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 213af9d..b26ed63 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -1343,26 +1343,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 c92c8a7..cda4e9c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -152,8 +152,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. @@ -860,11 +863,7 @@ - [% IF minlength_cardnumber && maxlength_cardnumber %] - - [% ELSE %] - [% END %] [% IF ( mandatorycardnumber ) %]Required[% END %]
    • [% END %] diff --git a/members/memberentry.pl b/members/memberentry.pl index 468314a..5d997f9 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -274,9 +274,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'}); @@ -727,23 +732,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.10.4