From c69edcbbaf02f3f3804aa8be99e6e54c6fce4441 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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