From 556eb677186b738c036fd13a7cc3461c3f8c3d48 Mon Sep 17 00:00:00 2001
From: Galen Charlton <gmc@esilibrary.com>
Date: Wed, 12 Mar 2014 03:33:50 +0000
Subject: [PATCH] Bug 108661: (follow-up) enshrine letting CardnumberLength
 specify just a maximum

This patch finishes the work started in one of the previous
follow-ups and allows CardnumberLength to be set to a value
like ',5'.  In conjunction with not including cardnumber in
BorrowerMandatoryField, this allows a cardnumber to not be
required but, if present, to not exceed the specified length.

This patch also updates t/db_dependent/Members.t so that
it runs in a transaction, tests the new return value
of checkcardnumber, and manages the CardnumberLength syspref.

To test:

[1] Verify that prove -v t/db_dependent/Members.t and
    prove -v t/Members/cardnumber.t pass.
[2] Set CardnumberLength to ",5" and take cardnubmer out of
    the BorrowerMandatoryField list.
[3] Verify that you can save a patron record without a cardnumber,
    but if you supply one, that it can be at most 5 characters long.
[4] Add cardnumber back to BorrowerMandatoryField.  This time, the
    minimum length is 1 even though CardnumberLength is ",5".

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
---
 C4/Members.pm                                 | 14 ++++++++++-
 .../en/modules/admin/preferences/patrons.pref |  5 ++--
 .../prog/en/modules/members/memberentrygen.tt |  2 +-
 t/Members/cardnumber.t                        | 23 +++++++++----------
 t/db_dependent/Members.t                      | 16 ++++++++++++-
 5 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/C4/Members.pm b/C4/Members.pm
index 9b358051f6..486a61d771 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -1349,8 +1349,20 @@ sub checkcardnumber {
     return 0;
 }
 
+=head2 get_cardnumber_length
+
+    my ($min, $max) = C4::Members::get_cardnumber_length()
+
+Returns the minimum and maximum length for patron cardnumbers as
+determined by the CardnumberLength system preference, the
+BorrowerMandatoryField system preference, and the width of the
+database column.
+
+=cut
+
 sub get_cardnumber_length {
-    my ( $min, $max ) = ( 1, 16 ); # borrowers.cardnumber is a varchar(16)
+    my ( $min, $max ) = ( 0, 16 ); # borrowers.cardnumber is a nullable varchar(16)
+    $min = 1 if C4::Context->preference('BorrowerMandatoryField') =~ /cardnumber/;
     if ( my $cardnumber_length = C4::Context->preference('CardnumberLength') ) {
         # Is integer and length match
         if ( $cardnumber_length =~ m|^\d+$| ) {
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 ac1d1da805..9732d1aff5 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
@@ -134,6 +134,7 @@ Patrons:
                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
+         - Card numbers for patrons must be
          - pref: CardnumberLength
-         - characters. The length can be a single number or a range separated by a comma ( i.e. Min,Max ).
+         - "characters long. The length can be a single number to specify an exact length, a range separated by a comma (i.e., 'Min,Max'), or a maximum with no minimum (i.e., ',Max')."
+         - "If 'cardnumber' is included in the BorrowerMandatoryField list, the minimum length, if not specified here, defaults to one."
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 ffdb44b2f1..720193f54e 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ -894,7 +894,7 @@
           [% IF ( mandatorycardnumber ) %]
             <label for="cardnumber" class="required">
           [% ELSE %]
-            <label for="cardnumber">
+            <label for="cardnumber" class="validated">
           [% END %]
           Card number: </label>
           [% IF minlength_cardnumber == maxlength_cardnumber %]
diff --git a/t/Members/cardnumber.t b/t/Members/cardnumber.t
index 8132c93f10..bfad5cec8c 100644
--- a/t/Members/cardnumber.t
+++ b/t/Members/cardnumber.t
@@ -1,10 +1,11 @@
 #!/usr/bin/env perl
 
 use Modern::Perl;
-use Test::More tests => 22;
+use Test::More tests => 23;
 
 use Test::MockModule;
 use DBD::Mock;
+use t::lib::Mocks;
 
 use_ok('C4::Members');
 
@@ -21,8 +22,9 @@ $module_context->mock(
 my $dbh = C4::Context->dbh;
 my $rs = [];
 
+t::lib::Mocks::mock_preference('BorrowerMandatoryField', '');
 my $pref = "10";
-set_pref( $module_context, $pref );
+t::lib::Mocks::mock_preference('CardnumberLength', $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");
@@ -32,7 +34,7 @@ $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 );
+t::lib::Mocks::mock_preference('CardnumberLength', $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");
@@ -42,7 +44,7 @@ $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 );
+t::lib::Mocks::mock_preference('CardnumberLength', $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");
@@ -54,7 +56,7 @@ $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 );
+t::lib::Mocks::mock_preference('CardnumberLength', $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");
@@ -64,8 +66,8 @@ $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');
+t::lib::Mocks::mock_preference('CardnumberLength', $pref);
+is_deeply( [ C4::Members::get_cardnumber_length() ], [ 0, 8 ], ',8 => min=0 and max=8');
 $dbh->{mock_add_resultset} = $rs;
 is( C4::Members::checkcardnumber( q{1234567} ), 0, "1234567 matches $pref");
 $dbh->{mock_add_resultset} = $rs;
@@ -73,8 +75,5 @@ is( C4::Members::checkcardnumber( q{1234567890123456} ), 2, "1234567890123456 is
 $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 } );
-}
+t::lib::Mocks::mock_preference('BorrowerMandatoryField', 'cardnumber');
+is_deeply( [ C4::Members::get_cardnumber_length() ], [ 1, 8 ], ',8 => min=1 and max=8 if cardnumber is mandatory');
diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t
index 6904190bc9..007f8eb567 100755
--- a/t/db_dependent/Members.t
+++ b/t/db_dependent/Members.t
@@ -6,13 +6,19 @@
 use strict;
 use warnings;
 
-use Test::More tests => 23;
+use Test::More tests => 24;
 use Data::Dumper;
+use C4::Context;
 
 BEGIN {
         use_ok('C4::Members');
 }
 
+my $dbh = C4::Context->dbh;
+
+# Start transaction
+$dbh->{AutoCommit} = 0;
+$dbh->{RaiseError} = 1;
 
 my $CARDNUMBER   = 'TESTCARD01';
 my $FIRSTNAME    = 'Marie';
@@ -147,6 +153,8 @@ $results = Search(\@searchstring);
 ok (_find_member($results), "Search (arrayref) for independent branches, same branch")
   or diag("Card $CARDNUMBER not found in the resultset for independent branches: ".Dumper(C4::Context->preference($INDEPENDENT_BRANCHES_PREF), $results));
 
+C4::Context->set_preference( 'CardnumberLength', '' );
+C4::Context->clear_syspref_cache();
 
 my $checkcardnum=C4::Members::checkcardnumber($CARDNUMBER, "");
 is ($checkcardnum, "1", "Card No. in use");
@@ -154,6 +162,12 @@ is ($checkcardnum, "1", "Card No. in use");
 $checkcardnum=C4::Members::checkcardnumber($IMPOSSIBLE_CARDNUMBER, "");
 is ($checkcardnum, "0", "Card No. not used");
 
+C4::Context->set_preference( 'CardnumberLength', '4' );
+C4::Context->clear_syspref_cache();
+
+$checkcardnum=C4::Members::checkcardnumber($IMPOSSIBLE_CARDNUMBER, "");
+is ($checkcardnum, "2", "Card number is too long");
+
 my $age=GetAge("1992-08-14", "2011-01-19");
 is ($age, "18", "Age correct");
 
-- 
2.39.5