Bug 20287: generate_userid now set the userid

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index 3b0d804..420b576 100644 (file)
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -177,7 +177,7 @@ sub store {
             unless ( $self->in_storage ) {    #AddMember
 
                 # Generate a valid userid/login if needed
-                $self->userid($self->generate_userid)
+                $self->generate_userid
                   if not $self->userid or not $self->has_valid_userid;
 
                 # Add expiration date if it isn't already there
@@ -1234,40 +1234,31 @@ sub has_valid_userid {
 =head3 generate_userid
 
 my $patron = Koha::Patron->new( $params );
-my $userid = $patron->generate_userid
+$patron->generate_userid
 
 Generate a userid using the $surname and the $firstname (if there is a value in $firstname).
 
-Return the generate userid ($firstname.$surname if there is a $firstname, or $surname if there is no value in $firstname) plus offset (0 if the $userid is unique, or a higher numeric value if not unique).
-
-# TODO: Set $self->userid with the generated value
+Set a generated userid ($firstname.$surname if there is a $firstname, or $surname if there is no value in $firstname) plus offset (0 if the $userid is unique, or a higher numeric value if not unique).
 
 =cut
 
 sub generate_userid {
     my ($self) = @_;
-    my $userid;
     my $offset = 0;
-    my $existing_userid = $self->userid;
     my $firstname = $self->firstname // q{};
     my $surname = $self->surname // q{};
     #The script will "do" the following code and increment the $offset until the generated userid is unique
     do {
       $firstname =~ s/[[:digit:][:space:][:blank:][:punct:][:cntrl:]]//g;
       $surname =~ s/[[:digit:][:space:][:blank:][:punct:][:cntrl:]]//g;
-      $userid = lc(($firstname)? "$firstname.$surname" : $surname);
+      my $userid = lc(($firstname)? "$firstname.$surname" : $surname);
       $userid = unac_string('utf-8',$userid);
       $userid .= $offset unless $offset == 0;
       $self->userid( $userid );
       $offset++;
      } while (! $self->has_valid_userid );
 
-     # Resetting to the previous value as the callers do not expect
-     # this method to modify the userid attribute
-     # This will be done later (move of AddMember and ModMember)
-     $self->userid( $existing_userid );
-
-     return $userid;
+     return $self;
 
 }
 

diff --git a/members/memberentry.pl b/members/memberentry.pl
index 041dda2..7f48bfd 100755 (executable)
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -283,7 +283,8 @@ if ( ( defined $newdata{'userid'} && $newdata{'userid'} eq '' ) || $check_Borrow
         # Full page edit, firstname and surname input zones are present
         $patron->firstname($newdata{firstname});
         $patron->surname($newdata{surname});
-        $newdata{'userid'} = $patron->generate_userid;
+        $patron->generate_userid;
+        $newdata{'userid'} = $patron->userid;
     }
     elsif ( ( defined $data{'firstname'} || $category_type eq 'I' ) && ( defined $data{'surname'} ) ) {
         # Partial page edit (access through "Details"/"Library details" tab), firstname and surname input zones are not used
@@ -291,7 +292,8 @@ if ( ( defined $newdata{'userid'} && $newdata{'userid'} eq '' ) || $check_Borrow
         # FIXME clean thiscode newdata vs data is very confusing
         $patron->firstname($data{firstname});
         $patron->surname($data{surname});
-        $newdata{'userid'} = $patron->generate_userid;
+        $patron->generate_userid;
+        $newdata{'userid'} = $patron->userid;
     }
     else {
         $newdata{'userid'} = $data{'userid'};

diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
index 0a88ea8..fa3c8dd 100644 (file)
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -1324,13 +1324,15 @@ subtest 'generate_userid' => sub {
 
     my $expected_userid_patron_1 = 'tomasito.none';
     my $new_patron = Koha::Patron->new({ firstname => $data{firstname}, surname => $data{surname} } );
-    my $userid = $new_patron->generate_userid;
+    $new_patron->generate_userid;
+    my $userid = $new_patron->userid;
     is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
     my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
     my $patron_1 = Koha::Patrons->find($borrowernumber);
     is ( $patron_1->userid, $expected_userid_patron_1, 'The userid generated should be the one we expect' );
 
-    $userid = $new_patron->generate_userid;
+    $new_patron->generate_userid;
+    $userid = $new_patron->userid;
     is( $userid, $expected_userid_patron_1 . '1', 'generate_userid should generate the userid we expect' );
     $data{cardnumber} = '987654321';
     my $new_borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
@@ -1340,12 +1342,14 @@ subtest 'generate_userid' => sub {
     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" );
 
-    $userid = $new_patron->generate_userid;
+    $new_patron->generate_userid;
+    $userid = $new_patron->userid;
     is( $userid, $expected_userid_patron_1 . '2', 'generate_userid should generate the userid we expect' );
 
     $patron_1 = Koha::Patrons->find($borrowernumber);
     $patron_1->userid(undef);
-    $userid = $patron_1->generate_userid;
+    $patron_1->generate_userid;
+    $userid = $patron_1->userid;
     is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
 
     # Cleanup