Bugzilla – Attachment 152109 Details for
Bug 33940
Move get_cardnumber_length and checkcardnumber to Koha
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber
Bug-33940-Move-C4Members-cardnumber-sub-to-KohaPol.patch (text/plain), 27.41 KB, created by
Jonathan Druart
on 2023-06-07 13:49:38 UTC
(
hide
)
Description:
Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2023-06-07 13:49:38 UTC
Size:
27.41 KB
patch
obsolete
>From 276e7cd0755742000c9ace766e007d6b0817c730 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >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 >--- > C4/Members.pm | 61 ------- > Koha/Patrons/Import.pm | 5 +- > Koha/Policy/Patrons/Cardnumber.pm | 110 ++++++++++++ > installer/onboarding.pl | 17 +- > members/memberentry.pl | 19 ++- > opac/opac-memberentry.pl | 25 +-- > t/Members/cardnumber.t | 96 ----------- > .../Koha/Policy/Patrons/Cardnumber.t | 160 ++++++++++++++++++ > t/db_dependent/Members.t | 23 +-- > 9 files changed, 311 insertions(+), 205 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 c17bbb2a326..956836fac7b 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -52,8 +52,6 @@ BEGIN { > IssueSlip > > checkuserpassword >- get_cardnumber_length >- checkcardnumber > > DeleteUnverifiedOpacRegistrations > DeleteExpiredOpacRegistrations >@@ -291,65 +289,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 <http://www.gnu.org/licenses>. >+ >+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<Koha::Patron> 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..a8e63f066a5 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,15 @@ 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 $message ( @{ $is_cardnumber_valid->messages } ) { >+ 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..d9b027070f7 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,15 @@ 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 $message ( @{ $is_valid->messages } ) { >+ if ( $message eq 'already_exists' ) { >+ push @messages, { code => 'ERROR_cardnumber_already_exists' }; >+ } elsif ( $message eq 'invalid_length' ) { >+ push @messages, { code => 'ERROR_cardnumber_length' }; >+ } >+ } > } > > my $dateofbirth; >@@ -786,7 +789,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..47d321d22c7 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,24 @@ 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($borrower{cardnumber}, $patron); >+ unless ($is_cardnumber_valid) { >+ for my $message ( @{ $is_cardnumber_valid->messages } ) { >+ 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 <http://www.gnu.org/licenses>. >+ >+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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=152109">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 33940
:
152109
|
153758
|
153759
|
153763
|
153764
|
153766
|
153768
|
153769
|
153770
|
153771
|
153772
|
153773
|
155687
|
155688
|
155689