From c69edcbbaf02f3f3804aa8be99e6e54c6fce4441 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 8 Jan 2018 17:50:48 -0300 Subject: [PATCH] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers --- Koha/Patron.pm | 35 ++++++++++++++++++++++++++++++++++ t/db_dependent/Koha/Patrons.t | 44 ++++++++++++++++++++++++++++--------------- 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index e83eed6b55..fd7e06194e 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -709,6 +709,41 @@ sub account_locked { and $self->login_attempts >= $FailedLoginAttempts )? 1 : 0; } +=head3 has_valid_userid + +my $patron = Koha::Patrons->find(42); +$patron->userid( $new_userid ); +my $has_a_valid_userid = $patron->has_valid_userid + +my $patron = Koha::Patron->new( $params ); +my $has_a_valid_userid = $patron->has_valid_userid + +Return true if the current userid of this patron is valid/unique, otherwise false. + +Note that this should be done in $self->store instead and raise an exception if needed. + +=cut + +sub has_valid_userid { + my ($self) = @_; + + return 0 unless $self->userid; + + return 0 if ( $self->userid eq C4::Context->config('user') ); # DB user + + my $already_exists = Koha::Patrons->search( + { + userid => $self->userid, + ( + $self->in_storage + ? ( borrowernumber => { '!=' => $self->borrowernumber } ) + : () + ), + } + )->count; + return $already_exists ? 0 : 1; +} + =head3 type =cut diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 33a3792b64..0c7331add7 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -956,7 +956,7 @@ subtest 'account_locked' => sub { }; subtest 'userid_is_valid' => sub { - plan tests => 9; + plan tests => 10; my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my $patron_category = $builder->build_object( @@ -973,32 +973,46 @@ subtest 'userid_is_valid' => sub { branchcode => $library->branchcode, ); + my $expected_userid_patron_1 = 'tomasito.none'; my $borrowernumber = AddMember(%data); my $patron_1 = Koha::Patrons->find($borrowernumber); + is ( $patron_1->userid, $expected_userid_patron_1, 'The userid generated should be the one we expect' ); - is( Check_Userid( 'tomasito.non', $patron_1->borrowernumber ), + $patron_1->userid( 'tomasito.non' ); + is( $patron_1->has_valid_userid, # FIXME Joubu: What is the difference with the next test? 1, 'recently created userid -> unique (borrowernumber passed)' ); - is( Check_Userid( 'tomasitoxxx', $patron_1->borrowernumber ), + + $patron_1->userid( 'tomasitoxxx' ); + is( $patron_1->has_valid_userid, 1, 'non-existent userid -> unique (borrowernumber passed)' ); - is( Check_Userid( 'tomasito.none', '' ), - 0, 'userid exists (blank borrowernumber)' ); - is( Check_Userid( 'tomasitoxxx', '' ), - 1, 'non-existent userid -> unique (blank borrowernumber)' ); + $patron_1->discard_changes; # We compare with the original userid later + + my $patron_not_in_storage = Koha::Patron->new( { userid => '' } ); + is( $patron_not_in_storage->has_valid_userid, + 0, 'userid exists for another patron, patron is not in storage yet' ); + + $patron_not_in_storage = Koha::Patron->new( { userid => 'tomasitoxxx' } ); + is( $patron_not_in_storage->has_valid_userid, + 1, 'non-existent userid, patron is not in storage yet' ); # Regression tests for BZ12226 - is( Check_Userid( C4::Context->config('user'), '' ), - 0, 'Check_Userid should return 0 for the DB user (Bug 12226)' ); + my $db_patron = Koha::Patron->new( { userid => C4::Context->config('user') } ); + is( $db_patron->has_valid_userid, + 0, 'Koha::Patron->has_valid_userid should return 0 for the DB user (Bug 12226)' ); # Add a new borrower with the same userid but different cardnumber $data{cardnumber} = "987654321"; my $new_borrowernumber = AddMember(%data); - is( Check_Userid( 'tomasito.none', '' ), - 0, 'userid not unique (blank borrowernumber)' ); - is( Check_Userid( 'tomasito.none', $new_borrowernumber ), - 0, 'userid not unique (second borrowernumber passed)' ); my $patron_2 = Koha::Patrons->find($new_borrowernumber); - ok( $patron_2->userid ne 'tomasito', - "Borrower with duplicate userid has new userid generated" ); + $patron_2->userid($patron_1->userid); + is( $patron_2->has_valid_userid, + 0, 'The userid is already in used, it cannot be used for another patron' ); + + $patron_2 = Koha::Patrons->find($new_borrowernumber); + isnt( $patron_2->userid, 'tomasito', + "Patron with duplicate userid has new userid generated" ); + is( $patron_2->userid, $expected_userid_patron_1 . '1', # TODO we could make that configurable + "Patron with duplicate userid has new userid generated (1 is appened" ); my $new_userid = 'a_user_id'; $data{cardnumber} = "234567890"; -- 2.11.0