diff --git a/C4/Members.pm b/C4/Members.pm index 70158ec7ed..919f6c3890 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -48,9 +48,6 @@ BEGIN { IssueSlip - get_cardnumber_length - checkcardnumber - DeleteUnverifiedOpacRegistrations DeleteExpiredOpacRegistrations ); @@ -287,65 +284,6 @@ sub GetAllIssues { return $sth->fetchall_arrayref( {} ); } -sub checkcardnumber { - my ( $cardnumber, $borrowernumber ) = @_; - - # If cardnumber is null, we assume they're allowed. - 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); - $sth->execute( - $cardnumber, - ( $borrowernumber ? $borrowernumber : () ) - ); - - return 1 if $sth->fetchrow_hashref; - - my ( $min_length, $max_length ) = get_cardnumber_length(); - return 2 - if length $cardnumber > $max_length - or length $cardnumber < $min_length; - - 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 $borrower = Koha::Database->new->schema->resultset('Borrower'); - my $field_size = $borrower->result_source->column_info('cardnumber')->{size}; - my ( $min, $max ) = ( 0, $field_size ); # borrowers.cardnumber is a nullable varchar(20) - $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+$| ) { - $min = $max = $cardnumber_length - if $cardnumber_length >= $min - and $cardnumber_length <= $max; - } - # Else assuming it is a range - elsif ( $cardnumber_length =~ m|(\d*),(\d*)| ) { - $min = $1 if $1 and $min < $1; - $max = $2 if $2 and $max > $2; - } - - } - $min = $max if $min > $max; - return ( $min, $max ); -} - =head2 GetBorrowersToExpunge $borrowers = &GetBorrowersToExpunge( diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm index 73d5355149..2e11a2ebb7 100644 --- a/Koha/Patrons/Import.pm +++ b/Koha/Patrons/Import.pm @@ -23,13 +23,13 @@ use Text::CSV; use Encode qw( decode_utf8 ); use Try::Tiny qw( catch try ); -use C4::Members qw( checkcardnumber ); use C4::Letters qw( GetPreparedLetter EnqueueLetter ); use Koha::Libraries; use Koha::Patrons; use Koha::Patron::Categories; use Koha::Patron::Debarments qw( AddDebarment ); +use Koha::Policy::Patrons::Cardnumber; use Koha::DateUtils qw( dt_from_string output_pref ); =head1 NAME @@ -212,7 +212,8 @@ sub import_patrons { $is_new = 1; } - if ( C4::Members::checkcardnumber( $borrower{cardnumber}, $borrowernumber ) ) { + my $is_valid = Koha::Policy::Patrons::Cardnumber->is_valid($borrower{cardnumber}, $patron); + unless ( $is_valid ) { push @errors, { invalid_cardnumber => 1, diff --git a/Koha/Policy/Patrons/Cardnumber.pm b/Koha/Policy/Patrons/Cardnumber.pm new file mode 100644 index 0000000000..43eb124050 --- /dev/null +++ b/Koha/Policy/Patrons/Cardnumber.pm @@ -0,0 +1,110 @@ +package Koha::Policy::Patrons::Cardnumber; + +# Copyright 2023 Koha Development team +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use C4::Context; + +use Koha::Patrons; +use Koha::Result::Boolean; + +=head1 NAME + +Koha::Policy::Patrons::Cardnumber - module to deal with cardnumbers policy + +=head1 API + +=head2 Class Methods + +=head3 new + +=cut + +sub new { + return bless {}, shift; +} + +=head3 is_valid + + my $is_valid = Koha::Policy::Patrons::Cardnumber->is_valid( $cardnumber, [$patron] ); + +Returns whether a cardnumber is valid of not for a given I object. + +=cut + +sub is_valid { + my ( $class, $cardnumber, $patron ) = @_; + + return Koha::Result::Boolean->new(0)->add_message( { message => "is_empty" } ) + unless defined $cardnumber; + + return Koha::Result::Boolean->new(0)->add_message( { message => "already_exists" } ) + if Koha::Patrons->search( + { + cardnumber => $cardnumber, + ( $patron ? ( borrowernumber => { '!=' => $patron->borrowernumber } ) : () ) + } + )->count; + + my ( $min_length, $max_length ) = $class->get_valid_length(); + return Koha::Result::Boolean->new(0)->add_message( { message => "invalid_length" } ) + if length $cardnumber > $max_length + or length $cardnumber < $min_length; + + return Koha::Result::Boolean->new(1); +} + +=head2 get_valid_length + + my ($min, $max) = Koha::Policy::Patrons::Cardnumber::get_valid_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_valid_length { + my ($class) = @_; + my $borrower = Koha::Database->new->schema->resultset('Borrower'); + my $field_size = $borrower->result_source->column_info('cardnumber')->{size}; + my ( $min, $max ) = ( 0, $field_size ); # borrowers.cardnumber is a nullable varchar(20) + $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+$| ) { + $min = $max = $cardnumber_length + if $cardnumber_length >= $min + and $cardnumber_length <= $max; + } + + # Else assuming it is a range + elsif ( $cardnumber_length =~ m|(\d*),(\d*)| ) { + $min = $1 if $1 and $min < $1; + $max = $2 if $2 and $max > $2; + } + + } + $min = $max if $min > $max; + return ( $min, $max ); +} + +1; diff --git a/installer/onboarding.pl b/installer/onboarding.pl index 68a1345df1..457f1fe278 100755 --- a/installer/onboarding.pl +++ b/installer/onboarding.pl @@ -22,12 +22,12 @@ use C4::Context; use C4::InstallAuth qw( checkauth get_template_and_user ); use CGI qw ( -utf8 ); use C4::Output qw( output_html_with_http_headers ); -use C4::Members qw( checkcardnumber ); use Koha::Patrons; use Koha::Libraries; use Koha::Database; use Koha::Patrons; use Koha::Patron::Categories; +use Koha::Policy::Patrons::Cardnumber; use Koha::ItemTypes; use Koha::CirculationRules; @@ -141,12 +141,16 @@ if ( $step == 3 ) { $patron_category ); - if ( my $error_code = checkcardnumber($cardnumber) ) { - if ( $error_code == 1 ) { - push @messages, { code => 'ERROR_cardnumber_already_exists' }; - } - elsif ( $error_code == 2 ) { - push @messages, { code => 'ERROR_cardnumber_length' }; + my $is_cardnumber_valid = Koha::Policy::Patrons::Cardnumber->is_valid($cardnumber); + unless ( $is_cardnumber_valid ) { + for my $m ( @{ $is_cardnumber_valid->messages } ) { + my $message = $m->message; + if ( $message eq 'already_exists' ) { + push @messages, { code => 'ERROR_cardnumber_already_exists' }; + } + elsif ( $message eq 'invalid_length' ) { + push @messages, { code => 'ERROR_cardnumber_length' }; + } } } elsif ( $firstpassword ne $secondpassword ) { diff --git a/members/memberentry.pl b/members/memberentry.pl index 70841759d7..89e32ff181 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -29,7 +29,6 @@ use CGI qw ( -utf8 ); use C4::Auth qw( get_template_and_user haspermission ); use C4::Context; use C4::Output qw( output_and_exit output_and_exit_if_error output_html_with_http_headers ); -use C4::Members qw( checkcardnumber get_cardnumber_length ); use C4::Koha qw( GetAuthorisedValues ); use C4::Letters qw( GetPreparedLetter EnqueueLetter SendQueuedMessages ); use C4::Form::MessagingPreferences; @@ -46,6 +45,7 @@ use Koha::Patron::Attribute::Types; use Koha::Patron::Categories; use Koha::Patron::HouseboundRole; use Koha::Patron::HouseboundRoles; +use Koha::Policy::Patrons::Cardnumber; use Koha::Plugins; use Koha::Token; use Koha::SMS::Providers; @@ -293,12 +293,16 @@ if ($op eq 'save' || $op eq 'insert'){ $newdata{'cardnumber'} = $new_barcode; - if (my $error_code = checkcardnumber( $newdata{cardnumber}, $borrowernumber )){ - push @errors, $error_code == 1 - ? 'ERROR_cardnumber_already_exists' - : $error_code == 2 - ? 'ERROR_cardnumber_length' - : () + my $is_valid = Koha::Policy::Patrons::Cardnumber->is_valid($newdata{cardnumber}, $patron ); + unless ($is_valid) { + for my $m ( @{ $is_valid->messages } ) { + my $message = $m->message; + if ( $message eq 'already_exists' ) { + $template->param( ERROR_cardnumber_already_exists => 1 ); + } elsif ( $message eq 'invalid_length' ) { + $template->param( ERROR_cardnumber_length => 1 ); + } + } } my $dateofbirth; @@ -786,7 +790,7 @@ if(defined($data{'contacttitle'})){ } -my ( $min, $max ) = C4::Members::get_cardnumber_length(); +my ( $min, $max ) = Koha::Policy::Patrons::Cardnumber->get_valid_length(); if ( defined $min ) { $template->param( minlength_cardnumber => $min, diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index ab99cb4e8c..f5a32db85a 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -28,7 +28,6 @@ use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_html_with_http_headers ); use C4::Context; use C4::Letters qw( GetPreparedLetter EnqueueLetter SendQueuedMessages ); -use C4::Members qw( checkcardnumber ); use C4::Form::MessagingPreferences; use Koha::AuthUtils; use Koha::Patrons; @@ -43,6 +42,7 @@ use Koha::Patron::Attribute::Types; use Koha::Patron::Attributes; use Koha::Patron::Images; use Koha::Patron::Categories; +use Koha::Policy::Patrons::Cardnumber; use Koha::Token; use Koha::AuthorisedValues; my $cgi = CGI->new; @@ -88,7 +88,7 @@ if ( $action eq 'create' || $action eq 'new' ) { } my $libraries = Koha::Libraries->search($params); -my ( $min, $max ) = C4::Members::get_cardnumber_length(); +my ( $min, $max ) = Koha::Policy::Patrons::Cardnumber->get_valid_length(); if ( defined $min ) { $template->param( minlength_cardnumber => $min, @@ -133,19 +133,25 @@ if ( $action eq 'create' ) { my @empty_mandatory_fields = (CheckMandatoryFields( \%borrower, $action ), CheckMandatoryAttributes( \%borrower, $attributes ) ); my $invalidformfields = CheckForInvalidFields(\%borrower); delete $borrower{'password2'}; - my $cardnumber_error_code; + my $is_cardnumber_valid; if ( !grep { $_ eq 'cardnumber' } @empty_mandatory_fields ) { # No point in checking the cardnumber if it's missing and mandatory, it'll just generate a # spurious length warning. - $cardnumber_error_code = checkcardnumber( $borrower{cardnumber}, $borrower{borrowernumber} ); + my $patron = Koha::Patrons->find($borrower{borrowernumber}); + $is_cardnumber_valid = Koha::Policy::Patrons::Cardnumber->is_valid($borrower{cardnumber}, $patron); + unless ($is_cardnumber_valid) { + for my $m ( @{ $is_cardnumber_valid->messages } ) { + my $message = $m->message; + if ( $message eq 'already_exists' ) { + $template->param( cardnumber_already_exists => 1 ); + } elsif ( $message eq 'invalid_length' ) { + $template->param( cardnumber_wrong_length => 1 ); + } + } + } } - if ( @empty_mandatory_fields || @$invalidformfields || $cardnumber_error_code || $conflicting_attribute ) { - if ( $cardnumber_error_code == 1 ) { - $template->param( cardnumber_already_exists => 1 ); - } elsif ( $cardnumber_error_code == 2 ) { - $template->param( cardnumber_wrong_length => 1 ); - } + if ( @empty_mandatory_fields || @$invalidformfields || !$is_cardnumber_valid || $conflicting_attribute ) { $template->param( empty_mandatory_fields => \@empty_mandatory_fields, diff --git a/t/Members/cardnumber.t b/t/Members/cardnumber.t deleted file mode 100755 index 7fddc7e676..0000000000 --- a/t/Members/cardnumber.t +++ /dev/null @@ -1,96 +0,0 @@ -#!/usr/bin/env perl - -use Modern::Perl; -use Module::Load::Conditional qw/check_install/; -use Test::More; -use Test::MockModule; - -use t::lib::Mocks; - -use_ok('C4::Members', qw( get_cardnumber_length checkcardnumber )); - -BEGIN { - if ( check_install( module => 'Test::DBIx::Class' ) ) { - plan tests => 25; - } else { - plan skip_all => "Need Test::DBIx::Class" - } -} - -use Test::DBIx::Class; - -my $db = Test::MockModule->new('Koha::Database'); -$db->mock( _new_schema => sub { return Schema(); } ); - -my $dbh = C4::Context->dbh; -my $rs = []; - -my $borrower = Koha::Schema->resultset('Borrower'); -my $cardnumber_size = $borrower->result_source->column_info('cardnumber')->{size}; - -t::lib::Mocks::mock_preference('BorrowerMandatoryField', ''); -my $pref = "10"; -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"); -$dbh->{mock_add_resultset} = $rs; -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 ! -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"); -$dbh->{mock_add_resultset} = $rs; -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 -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"); -$dbh->{mock_add_resultset} = $rs; -is( C4::Members::checkcardnumber( q{1234567890123456} ), 2, "1234567890123456 is longer than $pref"); -$dbh->{mock_add_resultset} = $rs; -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 -t::lib::Mocks::mock_preference('CardnumberLength', $pref); -is_deeply( [ C4::Members::get_cardnumber_length() ], [ 8, $cardnumber_size ], "8, => min=8 and max=$cardnumber_size"); -$dbh->{mock_add_resultset} = $rs; -is( C4::Members::checkcardnumber( q{1234567} ), 2, "1234567 is shorter than $pref"); -$dbh->{mock_add_resultset} = $rs; -is( C4::Members::checkcardnumber( q{1234567890123456} ), 0, "1234567890123456 matches $pref"); -$dbh->{mock_add_resultset} = $rs; -is( C4::Members::checkcardnumber( q{1234567890} ), 0, "1234567890 matches $pref"); - -$pref = q|,8|; # max 8 chars -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; -is( C4::Members::checkcardnumber( q{1234567890123456} ), 2, "1234567890123456 is longer than $pref"); -$dbh->{mock_add_resultset} = $rs; -is( C4::Members::checkcardnumber( q{1234567890} ), 2, "1234567890 is longer than $pref"); - -$pref = sprintf(',%d', $cardnumber_size+1); -t::lib::Mocks::mock_preference('CardnumberLength', $pref); -is_deeply( [ C4::Members::get_cardnumber_length() ], [ 0, $cardnumber_size ], - sprintf(",%d => min=0 and max=%d",$cardnumber_size+1,$cardnumber_size) ); -$dbh->{mock_add_resultset} = $rs; - -my $generated_cardnumber = sprintf("%s1234567890",q|9|x$cardnumber_size); -is( C4::Members::checkcardnumber( $generated_cardnumber ), 2, "$generated_cardnumber is longer than $pref => $cardnumber_size is max!"); - -$pref = q|,8|; # max 8 chars -t::lib::Mocks::mock_preference('CardnumberLength', $pref); -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/Koha/Policy/Patrons/Cardnumber.t b/t/db_dependent/Koha/Policy/Patrons/Cardnumber.t new file mode 100755 index 0000000000..986963c735 --- /dev/null +++ b/t/db_dependent/Koha/Policy/Patrons/Cardnumber.t @@ -0,0 +1,163 @@ +#!/usr/bin/perl + +# Copyright 2023 Koha Development team +# +# This file is part of Koha +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 3; + +use Koha::Database; +use Koha::Policy::Patrons::Cardnumber; +use t::lib::Mocks; +use t::lib::TestBuilder; + +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +subtest 'is_valid' => sub { + + plan tests => 21; + + $schema->storage->txn_begin; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + + t::lib::Mocks::mock_preference( 'CardnumberLength', '' ); + + my $policy = Koha::Policy::Patrons::Cardnumber->new; + + my $is_valid = $policy->is_valid( $patron->cardnumber ); + ok( !$is_valid, "Cardnumber in use, cannot be reused" ); + + $is_valid = $policy->is_valid( $patron->cardnumber, $patron ); + ok( $is_valid, "Cardnumber in use but can be used by the same patron" ); + + my $tmp_patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $available_cardnumber = $tmp_patron->cardnumber; + $tmp_patron->delete; + $is_valid = $policy->is_valid($available_cardnumber); + ok( $is_valid, "Cardnumber not in use" ); + + t::lib::Mocks::mock_preference( 'CardnumberLength', '4' ); + + $is_valid = $policy->is_valid("12345"); + ok( !$is_valid, "Invalid cardnumber length" ); + + $is_valid = $policy->is_valid("123"); + ok( !$is_valid, "Invalid cardnumber length" ); + + my $pref = "10"; + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + ok( !$policy->is_valid(q{123456789}), "123456789 is shorter than $pref" ); + ok( !$policy->is_valid(q{1234567890123456}), "1234567890123456 is longer than $pref" ); + ok( $policy->is_valid(q{1234567890}), "1234567890 is equal to $pref" ); + + $pref = q|10,10|; # Same as before ! + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + ok( !$policy->is_valid(q{123456789}), "123456789 is shorter than $pref" ); + ok( !$policy->is_valid(q{1234567890123456}), "1234567890123456 is longer than $pref" ); + ok( $policy->is_valid(q{1234567890}), "1234567890 is equal to $pref" ); + + $pref = q|8,10|; # between 8 and 10 chars + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + ok( $policy->is_valid(q{12345678}), "12345678 matches $pref" ); + ok( !$policy->is_valid(q{1234567890123456}), "1234567890123456 is longer than $pref" ); + ok( !$policy->is_valid(q{1234567}), "1234567 is shorter than $pref" ); + ok( $policy->is_valid(q{1234567890}), "1234567890 matches $pref" ); + + $pref = q|8,|; # At least 8 chars + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + ok( !$policy->is_valid(q{1234567}), "1234567 is shorter than $pref" ); + ok( $policy->is_valid(q{1234567890123456}), "1234567890123456 matches $pref" ); + ok( $policy->is_valid(q{1234567890}), "1234567890 matches $pref" ); + + $pref = q|,8|; # max 8 chars + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + ok( $policy->is_valid(q{1234567}), "1234567 matches $pref" ); + ok( !$policy->is_valid(q{1234567890123456}), "1234567890123456 is longer than $pref" ); + ok( !$policy->is_valid(q{1234567890}), "1234567890 is longer than $pref" ); + + $schema->storage->txn_rollback; +}; + +subtest 'get_valid_length' => sub { + + plan tests => 5; + + $schema->storage->txn_begin; + + my $policy = Koha::Policy::Patrons::Cardnumber->new; + + t::lib::Mocks::mock_preference( 'BorrowerMandatoryField', '' ); + + my $pref = "10"; + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + is_deeply( [ $policy->get_valid_length() ], [ 10, 10 ], '10 => min=10 and max=10' ); + + $pref = q|10,10|; # Same as before ! + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + is_deeply( [ $policy->get_valid_length() ], [ 10, 10 ], '10,10 => min=10 and max=10' ); + + $pref = q|8,10|; # between 8 and 10 chars + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + is_deeply( [ $policy->get_valid_length() ], [ 8, 10 ], '8,10 => min=8 and max=10' ); + + $pref = q|,8|; # max 8 chars + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + is_deeply( [ $policy->get_valid_length() ], [ 0, 8 ], ',8 => min=0 and max=8' ); + + $pref = q|,8|; # max 8 chars + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + t::lib::Mocks::mock_preference( 'BorrowerMandatoryField', 'cardnumber' ); + is_deeply( [ $policy->get_valid_length() ], [ 1, 8 ], ',8 => min=1 and max=8 if cardnumber is mandatory' ); + + $schema->storage->txn_rollback; + +}; + +subtest 'compare with DB data size' => sub { + + plan tests => 3; + + $schema->storage->txn_begin; + + my $policy = Koha::Policy::Patrons::Cardnumber->new; + my $borrower = Koha::Schema->resultset('Borrower'); + my $cardnumber_size = $borrower->result_source->column_info('cardnumber')->{size}; + t::lib::Mocks::mock_preference( 'BorrowerMandatoryField', '' ); + + my $pref = q|8,|; # At least 8 chars + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + is_deeply( [ $policy->get_valid_length() ], [ 8, $cardnumber_size ], "8, => min=8 and max=$cardnumber_size" ); + + $pref = sprintf( ',%d', $cardnumber_size + 1 ); + t::lib::Mocks::mock_preference( 'CardnumberLength', $pref ); + is_deeply( + [ $policy->get_valid_length() ], [ 0, $cardnumber_size ], + sprintf( ",%d => min=0 and max=%d", $cardnumber_size + 1, $cardnumber_size ) + ); + + my $generated_cardnumber = sprintf( "%s1234567890", q|9| x $cardnumber_size ); + ok( + !$policy->is_valid($generated_cardnumber), + "$generated_cardnumber is longer than $pref => $cardnumber_size is max!" + ); + + $schema->storage->txn_rollback; + +}; diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index 7d9852af57..7dceafd1bb 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 53; +use Test::More tests => 50; use Test::MockModule; use Test::Exception; @@ -33,7 +33,7 @@ use t::lib::Mocks; use t::lib::TestBuilder; BEGIN { - use_ok('C4::Members', qw( checkcardnumber GetBorrowersToExpunge DeleteUnverifiedOpacRegistrations DeleteExpiredOpacRegistrations )); + use_ok('C4::Members', qw( GetBorrowersToExpunge DeleteUnverifiedOpacRegistrations DeleteExpiredOpacRegistrations )); } my $schema = Koha::Database->schema; @@ -58,9 +58,6 @@ my $EMAIL = "Marie\@email.com"; my $EMAILPRO = "Marie\@work.com"; my $PHONE = "555-12123"; -# XXX should be randomised and checked against the database -my $IMPOSSIBLE_CARDNUMBER = "XYZZZ999"; - t::lib::Mocks::mock_userenv(); # Make a borrower for testing @@ -104,22 +101,6 @@ ok ( $changedmember->{firstname} eq $CHANGED_FIRSTNAME && , "Member Changed") or diag("Mismatching member details: ".Dumper($member, $changedmember)); -t::lib::Mocks::mock_preference( 'CardnumberLength', '' ); -C4::Context->clear_syspref_cache(); - -my $checkcardnum=C4::Members::checkcardnumber($CARDNUMBER, ""); -is ($checkcardnum, "1", "Card No. in use"); - -$checkcardnum=C4::Members::checkcardnumber($IMPOSSIBLE_CARDNUMBER, ""); -is ($checkcardnum, "0", "Card No. not used"); - -t::lib::Mocks::mock_preference( 'CardnumberLength', '4' ); -C4::Context->clear_syspref_cache(); - -$checkcardnum=C4::Members::checkcardnumber($IMPOSSIBLE_CARDNUMBER, ""); -is ($checkcardnum, "2", "Card number is too long"); - - # Add a new borrower %data = ( cardnumber => "123456789",