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 <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

diff --git a/C4/Members.pm b/C4/Members.pm
index d88ea45..1611991 100644 (file)
--- 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 (executable)
--- 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,