From 3c9fd687b08326d0c60c988923f21b4cd17db8dc Mon Sep 17 00:00:00 2001 From: Jonathan Druart <jonathan.druart@biblibre.com> Date: Fri, 19 Apr 2013 09:58:02 +0200 Subject: [PATCH] Bug 10861: Add a check on cardnumber length Some libraries would like to add a check on the cardnumber length. This patch adds the ability to restrict the cardnumber to a specific length (strictly equal to XX, or length > XX or min < length < max). This restriction is checked on inserting/updating a patron or on importing patrons. This patch adds: - 1 new syspref CardnumberLength. 2 formats: a number or a range (xx,yy). - 1 new unit test file t/Members/checkcardnumber.t for the C4::Members::checkcardnumber routine. Test plan: 1/ Fill the pref CardnumberLength with '5,8' 2/ Create a new patron with an invalid cardnumber (123456789) 3/ Check that you cannot save 4/ With Firebug, replace the pattern attribute value (for the cardnumber input) with ".{5,10}" 5/ You are allowed to save but an error occurred. 6/ Try the same steps for update. 7/ Go to the import borrowers tool. 8/ Play with the import borrowers tool. We must test add/update patrons and the "record matching" field (cardnumber or a uniq patron attribute) --- C4/Members.pm | 43 ++++++++----- installer/data/mysql/updatedatabase.pl | 16 +++++ .../prog/en/modules/admin/preferences/patrons.pref | 4 ++ .../prog/en/modules/members/memberentrygen.tt | 34 +++++----- .../prog/en/modules/tools/import_borrowers.tt | 6 ++ members/memberentry.pl | 20 +++++- t/Members/checkcardnumber.t | 66 ++++++++++++++++++++ tools/import_borrowers.pl | 13 +++- 8 files changed, 172 insertions(+), 30 deletions(-) create mode 100644 t/Members/checkcardnumber.t diff --git a/C4/Members.pm b/C4/Members.pm index 7ac69df..213af9d 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -1327,25 +1327,40 @@ sub checkuniquemember { } sub checkcardnumber { - my ($cardnumber,$borrowernumber) = @_; + my ( $cardnumber, $borrowernumber ) = @_; + # If cardnumber is null, we assume they're allowed. - return 0 if !defined($cardnumber); + return 0 unless defined $cardnumber; + my $dbh = C4::Context->dbh; my $query = "SELECT * FROM borrowers WHERE cardnumber=?"; $query .= " AND borrowernumber <> ?" if ($borrowernumber); - my $sth = $dbh->prepare($query); - if ($borrowernumber) { - $sth->execute($cardnumber,$borrowernumber); - } else { - $sth->execute($cardnumber); - } - if (my $data= $sth->fetchrow_hashref()){ - return 1; - } - else { - return 0; + my $sth = $dbh->prepare($query); + $sth->execute( + $cardnumber, + ( $borrowernumber ? $borrowernumber : () ) + ); + + return 1 if $sth->fetchrow_hashref; + + if ( my $length = C4::Context->preference('CardnumberLength') ) { + # Is integer and length match + if ( + $length =~ m|^\d+$| + and length $cardnumber == $length + ) { + return 0 + } + # Else assuming it is a range + else { + my $qr = qr|^\d{$length}$|; + return 0 + if $cardnumber =~ $qr; + } + return 1 } -} + return 0; +} =head2 getzipnamecity (OUEST-PROVENCE) diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 31f494a..fee6607 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -7139,6 +7139,22 @@ if ( CheckVersion($DBversion) ) { SetVersion ($DBversion); } + + + + + + +$DBversion = "3.13.00.XXX"; +if ( CheckVersion($DBversion) ) { + $dbh->do(" + INSERT INTO systempreferences (variable,value,options,explanation,type) + VALUES('CardnumberLength', '', '', 'Set a length for card numbers.', 'Free'); + "); + print "Upgrade to $DBversion done (Bug 10861: Add CardnumberLength syspref)\n"; + SetVersion ($DBversion); +} + =head1 FUNCTIONS =head2 TableExists($table) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref index 74d14af..a0c9dbf 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref @@ -139,3 +139,7 @@ Patrons: yes: Do no: "Don't" - enable the ability to upload and attach arbitrary files to a borrower record. + - + - Card numbers for patrons must be strictly equal to + - pref: CardnumberLength + - characters. The length can be a simple number or a range separated by a coma (xx,yy). 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 2cb1bb3..c92c8a7 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -152,9 +152,9 @@ [% 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.</li> - [% END %] + [% IF ( ERROR_cardnumber ) %] + <li id="ERROR_cardnumber">Cardnumber already in use or not in a good format.</li> + [% END %] [% IF ( ERROR_age_limitations ) %] <li id="ERROR_age_limitations">Patron's age is incorrect for their category. Ages allowed are [% age_low %]-[% age_high %].</li> @@ -852,17 +852,23 @@ <fieldset class="rows" id="memberentry_library_management"> <legend id="library_management_lgd">Library management</legend><ol> - [% UNLESS nocardnumber %] - <li> [% IF ( mandatorycardnumber ) %] - <label for="cardnumber" class="required"> - [% ELSE %] - <label for="cardnumber"> - [% END %] - Card number: </label> - <input type="text" id="cardnumber" name="cardnumber" size="20" value="[% cardnumber %]" /> - [% IF ( mandatorycardnumber ) %]<span class="required">Required</span>[% END %]</li> - [% END %] - [% UNLESS nobranchcode %] + [% UNLESS nocardnumber %] + <li> + [% IF ( mandatorycardnumber ) %] + <label for="cardnumber" class="required"> + [% ELSE %] + <label for="cardnumber"> + [% END %] + Card number: </label> + [% IF 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 %]" /> + [% END %] + [% IF ( mandatorycardnumber ) %]<span class="required">Required</span>[% END %] + </li> + [% END %] + [% UNLESS nobranchcode %] <li> <label for="libraries" class="required">Library:</label> <select name="branchcode" size="1" id="libraries"> diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt index 6744faf..1ed23d0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt @@ -72,6 +72,12 @@ <br /><code>[% missing_critical.lineraw %]</code> </li> [% END %] + [% IF ERROR.invalid_cardnumber %] + <li class="line_error"> + Cardnumber [% ERROR.cardnumber %] is not a valid cardnumber + [% IF ERROR.borrowernumber %] (for patron with borrowernumber [% ERROR.borrowernumber %])[% END %] + </li> + [% END %] [% END %] </ul> </div> diff --git a/members/memberentry.pl b/members/memberentry.pl index 0bf3122..468314a 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -727,7 +727,25 @@ 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 + ); + } +} + output_html_with_http_headers $input, $cookie, $template->output; sub parse_extended_patron_attributes { diff --git a/t/Members/checkcardnumber.t b/t/Members/checkcardnumber.t new file mode 100644 index 0000000..94388b9 --- /dev/null +++ b/t/Members/checkcardnumber.t @@ -0,0 +1,66 @@ +#!/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 } ); +} diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl index 5f65c97..4445db5 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -231,7 +231,18 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { } } } - + + if ( C4::Members::checkcardnumber( $borrower{cardnumber}, $borrowernumber ) ) { + push @errors, { + invalid_cardnumber => 1, + borrowernumber => $borrowernumber, + cardnumber => $borrower{cardnumber} + }; + $invalid++; + next; + } + + if ($borrowernumber) { # borrower exists unless ($overwrite_cardnumber) { -- 1.7.10.4