Bug 20287: Move fixup_cardnumber

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>

C4/Members.pm

@@ -86,7 +86,6 @@ BEGIN {
     #Check data
     push @EXPORT, qw(
         &checkuserpassword
-        &fixup_cardnumber
         &checkcardnumber
     );
 }
@@ -426,12 +425,6 @@ sub AddMember {
         $data{'dateenrolled'} = output_pref( { dt => dt_from_string, dateonly => 1, dateformat => 'iso' } );
     }
 
-    if ( C4::Context->preference("autoMemberNum") ) {
-        if ( not exists $data{cardnumber} or not defined $data{cardnumber} or $data{cardnumber} eq '' ) {
-            $data{cardnumber} = fixup_cardnumber( $data{cardnumber} );
-        }
-    }
-
     $data{'privacy'} =
         $category->default_privacy() eq 'default' ? 1
       : $category->default_privacy() eq 'never'   ? 2
@@ -481,32 +474,6 @@ sub AddMember {
     return $data{borrowernumber};
 }
 
-=head2 fixup_cardnumber
-
-Warning: The caller is responsible for locking the members table in write
-mode, to avoid database corruption.
-
-=cut
-
-sub fixup_cardnumber {
-    my ($cardnumber) = @_;
-    my $autonumber_members = C4::Context->boolean_preference('autoMemberNum') || 0;
-
-    # Find out whether member numbers should be generated
-    # automatically. Should be either "1" or something else.
-    # Defaults to "0", which is interpreted as "no".
-
-    ($autonumber_members) or return $cardnumber;
-    my $dbh = C4::Context->dbh;
-
-    my $sth = $dbh->prepare(
-        'SELECT MAX( CAST( cardnumber AS SIGNED ) ) FROM borrowers WHERE cardnumber REGEXP "^-?[0-9]+$"'
-    );
-    $sth->execute;
-    my ($result) = $sth->fetchrow;
-    return $result + 1;
-}
-
 =head2 GetAllIssues
 
   $issues = &GetAllIssues($borrowernumber, $sortkey, $limit);
@@ -879,11 +846,10 @@ sub IssueSlip {
 sub AddMember_Auto {
     my ( %borrower ) = @_;
 
-    $borrower{'cardnumber'} ||= fixup_cardnumber();
-
     $borrower{'borrowernumber'} = AddMember(%borrower);
-
-    return ( %borrower );
+    my $patron = Koha::Patrons->find( $borrower{borrowernumber} )->unblessed;
+    $patron->{password} = $borrower{password};
+    return %$patron;
 }
 
 =head2 AddMember_Opac

Koha/Patron.pm

@@ -83,6 +83,49 @@ Koha::Patron - Koha Patron Object class
 
 =cut
 
+=head3 new
+
+=cut
+
+sub new {
+    my ( $class, $params ) = @_;
+
+    return $class->SUPER::new($params);
+}
+
+sub fixup_cardnumber {
+    my ( $self ) = @_;
+    my $max = Koha::Patrons->search({
+        cardnumber => {-regexp => '^-?[0-9]+$'}
+    }, {
+        select => \'CAST(cardnumber AS SIGNED)',
+        as => ['cast_cardnumber']
+    })->_resultset->get_column('cast_cardnumber')->max;
+    $self->cardnumber($max+1);
+}
+
+sub store {
+    my( $self ) = @_;
+
+    $self->_result->result_source->schema->txn_do(
+        sub {
+            if (
+                C4::Context->preference("autoMemberNum")
+                and ( not defined $self->cardnumber
+                    or $self->cardnumber eq '' )
+              )
+            {
+                # Warning: The caller is responsible for locking the members table in write
+                # mode, to avoid database corruption.
+                # We are in a transaction but the table is not locked
+                $self->fixup_cardnumber;
+            }
+
+            $self->SUPER::store;
+        }
+    );
+}
+
 =head3 delete
 
 $patron->delete

Koha/Patrons/Import.pm

@@ -295,11 +295,6 @@ sub import_patrons {
             );
         }
         else {
-            # FIXME: fixup_cardnumber says to lock table, but the web interface doesn't so this doesn't either.
-            # At least this is closer to AddMember than in members/memberentry.pl
-            if ( !$borrower{'cardnumber'} ) {
-                $borrower{'cardnumber'} = fixup_cardnumber(undef);
-            }
             if ( $borrowernumber = AddMember(%borrower) ) {
 
                 if ( $borrower{debarred} ) {

opac/svc/auth/googleopenidconnect

@@ -186,7 +186,6 @@ elsif ( defined $query->param('code') ) {
                 my $auto_registration = C4::Context->preference('GoogleOpenIDConnectAutoRegister') // q{0};
                 my $borrower = Koha::Patrons->find( { email => $email } );
                 if (! $borrower && $auto_registration==1) {
-                    my $cardnumber = fixup_cardnumber();
                     my $firstname = $claims_json->{'given_name'} // q{};
                     my $surname = $claims_json->{'family_name'} // q{};
                     my $delimiter = $firstname ? q{.} : q{};
@@ -198,7 +197,6 @@ elsif ( defined $query->param('code') ) {
                     if (defined $patron_category && defined $library) {
                         my $password = undef;
                         my $borrowernumber = C4::Members::AddMember(
-                            cardnumber   => $cardnumber,
                             firstname    => $firstname,
                             surname      => $surname,
                             email        => $email,

t/db_dependent/Members.t

@@ -388,13 +388,11 @@ is( $borrower->{password} eq $hashed_up, 1, 'Check password hash equals hash of
 
 subtest 'Trivial test for AddMember_Auto' => sub {
     plan tests => 3;
-    my $members_mock = Test::MockModule->new( 'C4::Members' );
-    $members_mock->mock( 'fixup_cardnumber', sub { 12345; } );
     my $library = $builder->build({ source => 'Branch' });
     my $category = $builder->build({ source => 'Category' });
     my %borr = AddMember_Auto( surname=> 'Dick3', firstname => 'Philip', branchcode => $library->{branchcode}, categorycode => $category->{categorycode}, password => '34567890' );
     ok( $borr{borrowernumber}, 'Borrower hash contains borrowernumber' );
-    is( $borr{cardnumber}, 12345, 'Borrower hash contains cardnumber' );
+    like( $borr{cardnumber}, qr/^\d+$/, 'Borrower hash contains cardnumber' );
     my $patron = Koha::Patrons->find( $borr{borrowernumber} );
     isnt( $patron, undef, 'Patron found' );
 };