From fd8b41590658887bd666e27a97c75ea6ef993f64 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Mon, 3 Nov 2014 07:38:48 -0500 Subject: [PATCH] [PASSED QA] Bug 1861 [QA Followup] - Fix Check_Userid and unit tests Check_Userid assumes that a borrowernumber will always be passed in and thus fails to to return 0 for an already used userid when creating a new patron. Unit tests must now also me modified to no longer assume it is possible to create multiple patrons with the same userid. Signed-off-by: Martin Renvoize --- C4/Members.pm | 31 +++++++++++++++++-------------- t/db_dependent/Members.t | 13 ++++++------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index d88ea45..1611991 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -855,7 +855,8 @@ sub AddMember { my $dbh = C4::Context->dbh; # generate a proper login if none provided - $data{'userid'} = Generate_Userid($data{'borrowernumber'}, $data{'firstname'}, $data{'surname'}) if $data{'userid'} eq ''; + $data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} ) + if ( $data{'userid'} eq '' || Check_Userid( $data{'userid'} ) ); # add expiration date if it isn't already there unless ( $data{'dateexpiry'} ) { @@ -917,19 +918,21 @@ sub AddMember { =cut sub Check_Userid { - my ($uid,$member) = @_; - my $dbh = C4::Context->dbh; - my $sth = - $dbh->prepare( - "SELECT * FROM borrowers WHERE userid=? AND borrowernumber != ?"); - $sth->execute( $uid, $member ); - if ( (( $uid ne '' ) && ( my $row = $sth->fetchrow_hashref )) or - (( $uid ne '' ) && ( $uid eq C4::Context->config('user') )) ) { - return 0; - } - else { - return 1; - } + my ( $uid, $borrowernumber ) = @_; + + return 1 unless ($uid); + + return 0 if ( $uid eq C4::Context->config('user') ); + + my $rs = Koha::Database->new()->schema()->resultset('Borrower'); + + my $params; + $params->{userid} = $uid; + $params->{borrowernumber} = { '!=' => $borrowernumber } if ($borrowernumber); + + my $count = $rs->count( $params ); + + return $count ? 0 : 1; } =head2 Generate_Userid diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index 0161c7b..cfbb4d8 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -276,15 +276,14 @@ ok (!_find_member($results), "Delete member") branchcode => "MPL", dateofbirth => '', dateexpiry => '9999-12-31', - userid => 'tomasito' ); # Add a new borrower my $borrowernumber = AddMember( %data ); -is( Check_Userid( 'tomasito', $borrowernumber ), 1, +is( Check_Userid( 'tomasito.non', $borrowernumber ), 1, 'recently created userid -> unique (borrowernumber passed)' ); is( Check_Userid( 'tomasitoxxx', $borrowernumber ), 1, 'non-existent userid -> unique (borrowernumber passed)' ); -is( Check_Userid( 'tomasito', '' ), 0, +is( Check_Userid( 'tomasito.none', '' ), 0, 'userid exists (blank borrowernumber)' ); is( Check_Userid( 'tomasitoxxx', '' ), 1, 'non-existent userid -> unique (blank borrowernumber)' ); @@ -292,12 +291,12 @@ is( Check_Userid( 'tomasitoxxx', '' ), 1, # Add a new borrower with the same userid but different cardnumber $data{ cardnumber } = "987654321"; my $new_borrowernumber = AddMember( %data ); -is( Check_Userid( 'tomasito', '' ), 0, +is( Check_Userid( 'tomasito.none', '' ), 0, 'userid not unique (blank borrowernumber)' ); -is( Check_Userid( 'tomasito', $borrowernumber ), 0, - 'userid not unique (first borrowernumber passed)' ); -is( Check_Userid( 'tomasito', $new_borrowernumber ), 0, +is( Check_Userid( 'tomasito.none', $new_borrowernumber ), 0, 'userid not unique (second borrowernumber passed)' ); +my $borrower = GetMember( borrowernumber => $new_borrowernumber ); +ok( $borrower->{userid} ne 'tomasito', "Borrower with duplicate userid has new userid generated" ); # Regression tests for BZ12226 is( Check_Userid( C4::Context->config('user'), '' ), 0, -- 1.7.10.4