From 8d9eb37aefa21692932e663b0c422685173e3af3 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 7 Jun 2023 15:37:55 +0200 Subject: [PATCH] Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber Test plan: The idea here is to confirm this patch does not introduce regression. For that you will play with the CardnumberLength syspref and create a new user, modify an existing one, and check that the UI does not let you modify an invalid cardnumber. The onboarding process and the patron import tool will also have to be tested Signed-off-by: Marcel de Rooy Bug 33940: Fix selfreg please squash with first patch Bug 33940: Fix messages we sent to templates please squash with the first patch Bug 33940: Fix what we send to memberentry --- C4/Members.pm | 62 ------- Koha/Patrons/Import.pm | 5 +- Koha/Policy/Patrons/Cardnumber.pm | 110 ++++++++++++ installer/onboarding.pl | 18 +- members/memberentry.pl | 20 ++- opac/opac-memberentry.pl | 26 +-- t/Members/cardnumber.t | 96 ----------- .../Koha/Policy/Patrons/Cardnumber.t | 160 ++++++++++++++++++ t/db_dependent/Members.t | 23 +-- 9 files changed, 314 insertions(+), 206 deletions(-) create mode 100644 Koha/Policy/Patrons/Cardnumber.pm delete mode 100755 t/Members/cardnumber.t create mode 100755 t/db_dependent/Koha/Policy/Patrons/Cardnumber.t diff --git a/C4/Members.pm b/C4/Members.pm index 70158ec7ed5..919f6c38908 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 3ba184d1e28..c9133ceb05b 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 00000000000..43eb124050f --- /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 68a1345df1e..457f1fe278f 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 64d9aa2aa7b..fad9b713d6b 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 d18e5f542b6..9af4a2371d3 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 7fddc7e6768..00000000000 --- 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 00000000000..7937cb412c7 --- /dev/null +++ b/t/db_dependent/Koha/Policy/Patrons/Cardnumber.t @@ -0,0 +1,160 @@ +#!/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 7d9852af570..7dceafd1bba 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", -- 2.25.1