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