Attachment 23563 Details for Bug 10861 – Bug 10861: Add a check on cardnumber length

[patch] Bug 10861: Add a check on cardnumber length

Bug-10861-Add-a-check-on-cardnumber-length.patch (text/plain), 12.53 KB, created by Marcel de Rooy on 2013-12-16 09:17:33 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2013-12-16 09:17:33 UTC

Size: 12.53 KB

patch

obsolete

>From 8b1710ccf527dc161d474f1aaded066201459bdf 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
>Content-Type: text/plain; charset=utf-8
>
>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)
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Tested adding, updating; importing and ran unit test.
>Preliminary QA comments on Bugzilla
>---
> C4/Members.pm                                      |   43 +++++++++----
> installer/data/mysql/updatedatabase.pl             |   11 +++
> .../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, 167 insertions(+), 30 deletions(-)
> create mode 100644 t/Members/checkcardnumber.t
>
>diff --git a/C4/Members.pm b/C4/Members.pm
>index b04cc64..cbcb78f 100644
>--- a/C4/Members.pm
>+++ b/C4/Members.pm
>@@ -1329,25 +1329,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 d099a48..012c318 100755
>--- a/installer/data/mysql/updatedatabase.pl
>+++ b/installer/data/mysql/updatedatabase.pl
>@@ -7831,6 +7831,17 @@ if ( CheckVersion($DBversion) ) {
>    SetVersion ($DBversion);
> }
> 
>+$DBversion = "3.15.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 4aa88cf..10e3ef1 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
>@@ -178,9 +178,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>
>@@ -878,17 +878,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 0d3a3c3..93a5e5e 100755
>--- a/members/memberentry.pl
>+++ b/members/memberentry.pl
>@@ -751,7 +751,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.7.6

Actions: View | Diff | Splinter Review

Attachments on bug 10861: 20950 | 21196 | 21586 | 23387 | 23388 | 23439 | 23476 | 23477 | 23563 | 23564 | 23565 | 23732 | 23733 | 23734 | 23735 | 23736