Bug 13049: Merge selfreg cron jobs into cleanup_database

This patch moves the core code of two selfreg cron jobs into the Members
module. The new routines are called from cleanup_database with two new
parameters. The old cron jobs are now wrappers to cleanup_database.
As a bonus, we can add a unit test now.

In time, we can obsolete the selfreg cron jobs. For now, the code is in one
place and behavior does not change.
A next step (as described on the Bugzilla report) would be: remove the Delay
pref for self regs.

Test plan:
Run the unit test t/db_dependent/Members.t.
Test the two new parameters of cleanup_database.pl.
Verify if delete_expired_opac_registrations.pl still works.
Same for delete_unverified_opac_registrations.pl.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
. Fixed minor merge confict on UT & cleanup_database.pl
. UT ok
. The two deprecated scripts still work as before, with a warning
  message.
. cleanup_database.pl do the deletion job, calling new C4::Members
  function rather that doing it directly.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

diff --git a/C4/Members.pm b/C4/Members.pm
index 1a172db..95fb4e7 100644 (file)
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -2459,6 +2459,10 @@ sub GetBorrowersWithEmail {
     return @result;
 }
 
+=head2 AddMember_Opac
+
+=cut
+
 sub AddMember_Opac {
     my ( %borrower ) = @_;
 
@@ -2505,6 +2509,10 @@ sub AddEnrolmentFeeIfNeeded {
     }
 }
 
+=head2 HasOverdues
+
+=cut
+
 sub HasOverdues {
     my ( $borrowernumber ) = @_;
 
@@ -2516,6 +2524,53 @@ sub HasOverdues {
     return $count;
 }
 
+=head2 DeleteExpiredOpacRegistrations
+
+    Delete accounts that haven't been upgraded from the 'temporary' category
+    Returns the number of removed patrons
+
+=cut
+
+sub DeleteExpiredOpacRegistrations {
+
+    my $delay = C4::Context->preference('PatronSelfRegistrationExpireTemporaryAccountsDelay');
+    my $category_code = C4::Context->preference('PatronSelfRegistrationDefaultCategory');
+
+    return 0 if not $category_code or not defined $delay or $delay eq q||;
+
+    my $query = qq|
+SELECT borrowernumber
+FROM borrowers
+WHERE categorycode = ? AND DATEDIFF( NOW(), dateenrolled ) > ? |;
+
+    my $dbh = C4::Context->dbh;
+    my $sth = $dbh->prepare($query);
+    $sth->execute( $category_code, $delay );
+    my $cnt=0;
+    while ( my ($borrowernumber) = $sth->fetchrow_array() ) {
+        DelMember($borrowernumber);
+        $cnt++;
+    }
+    return $cnt;
+}
+
+=head2 DeleteUnverifiedOpacRegistrations
+
+    Delete all unverified self registrations in borrower_modifications,
+    older than the specified number of days.
+
+=cut
+
+sub DeleteUnverifiedOpacRegistrations {
+    my ( $days ) = @_;
+    my $dbh = C4::Context->dbh;
+    my $sql=qq|
+DELETE FROM borrower_modifications
+WHERE borrowernumber = 0 AND DATEDIFF( NOW(), timestamp ) > ?|;
+    my $cnt=$dbh->do($sql, undef, ($days) );
+    return $cnt eq '0E0'? 0: $cnt;
+}
+
 END { }    # module clean-up code here (global destructor)
 
 1;

diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl
index cd92dc4..0766141 100755 (executable)
--- a/misc/cronjobs/cleanup_database.pl
+++ b/misc/cronjobs/cleanup_database.pl
@@ -69,14 +69,29 @@ Usage: $0 [-h|--help] [--sessions] [--sessdays DAYS] [-v|--verbose] [--zebraqueu
    --restrictions DAYS   purge patrons restrictions expired since more than DAYS days.
                          Defaults to 30 days if no days specified.
     --all-restrictions   purge all expired patrons restrictions.
+   --del-exp-selfreg  Delete expired self registration accounts
+   --del-unv-selfreg DAYS  Delete unverified self registrations older than DAYS
 USAGE
     exit $_[0];
 }
 
 my (
-    $help,   $sessions,          $sess_days, $verbose, $zebraqueue_days,
-    $mail,   $purge_merged,      $pImport,   $pLogs,   $pSearchhistory,
-    $pZ3950, $pListShareInvites, $pDebarments, $allDebarments,
+    $help,
+    $sessions,
+    $sess_days,
+    $verbose,
+    $zebraqueue_days,
+    $mail,
+    $purge_merged,
+    $pImport,
+    $pLogs,
+    $pSearchhistory,
+    $pZ3950,
+    $pListShareInvites,
+    $pDebarments,
+    $allDebarments,
+    $pExpSelfReg,
+    $pUnvSelfReg,
 );
 
 GetOptions(
@@ -94,6 +109,8 @@ GetOptions(
     'list-invites:i'  => \$pListShareInvites,
     'restrictions:i'  => \$pDebarments,
     'all-restrictions' => \$allDebarments,
+    'del-exp-selfreg' => \$pExpSelfReg,
+    'del-unv-selfreg' => \$pUnvSelfReg,
 ) || usage(1);
 
 # Use default values
@@ -120,8 +137,10 @@ unless ( $sessions
     || $pZ3950
     || $pListShareInvites
     || $pDebarments
-    || $allDebarments )
-{
+    || $allDebarments
+    || $pExpSelfReg
+    || $pUnvSelfReg
+) {
     print "You did not specify any cleanup work for the script to do.\n\n";
     usage(1);
 }
@@ -253,6 +272,13 @@ if($allDebarments) {
     print "$count restrictions were deleted.\nDone with all restrictions purge.\n" if $verbose;
 }
 
+if( $pExpSelfReg ) {
+    DeleteExpiredSelfRegs();
+}
+if( $pUnvSelfReg ) {
+    DeleteUnverifiedSelfRegs( $pUnvSelfReg );
+}
+
 exit(0);
 
 sub RemoveOldSessions {
@@ -340,3 +366,13 @@ sub PurgeDebarments {
     }
     return $count;
 }
+
+sub DeleteExpiredSelfRegs {
+    my $cnt= C4::Members::DeleteExpiredOpacRegistrations();
+    print "Removed $cnt expired self-registered borrowers\n" if $verbose;
+}
+
+sub DeleteUnverifiedSelfRegs {
+    my $cnt= C4::Members::DeleteUnverifiedOpacRegistrations( $_[0] );
+    print "Removed $cnt unverified self-registrations\n" if $verbose;
+}

diff --git a/misc/cronjobs/delete_expired_opac_registrations.pl b/misc/cronjobs/delete_expired_opac_registrations.pl
index 44cc9da..206c0ec 100755 (executable)
--- a/misc/cronjobs/delete_expired_opac_registrations.pl
+++ b/misc/cronjobs/delete_expired_opac_registrations.pl
@@ -43,6 +43,9 @@ GetOptions(
 );
 my $usage = << 'ENDUSAGE';
 
+IMPORTANT: You should no longer call this script. Please use
+cleanup_database.pl with parameter --del-exp-selfreg.
+
 This script removes confirmed OPAC based patron registrations
 that have not been changed from the patron category specified
 in the system preference PatronSelfRegistrationDefaultCategory
@@ -64,33 +67,6 @@ if ( $help || !$confirm ) {
     exit;
 }
 
-cronlogaction();
-
-# Delete accounts that haven't been upgraded from the 'temporary' category code
-my $delay =
-  C4::Context->preference('PatronSelfRegistrationExpireTemporaryAccountsDelay');
-my $category_code =
-  C4::Context->preference('PatronSelfRegistrationDefaultCategory');
-
-die "PatronSelfRegistrationExpireTemporaryAccountsDelay and PatronSelfRegistrationDefaultCategory should be filled to use this script!"
-    if not $category_code or not defined $delay or $delay eq q||;
-
-my $query = "
-    SELECT borrowernumber
-    FROM borrowers
-    WHERE
-        categorycode = ?
-      AND
-        DATEDIFF( NOW(), dateenrolled ) > ?
-";
-
-my $dbh = C4::Context->dbh;
-my $sth = $dbh->prepare($query);
-$sth->execute( $category_code, $delay );
-
-my $cnt=0;
-while ( my ($borrowernumber) = $sth->fetchrow_array() ) {
-    DelMember($borrowernumber);
-    $cnt++;
-}
-print "Removed $cnt expired self-registered borrowers in category $category_code\n" if $verbose;
+my $c= "$FindBin::Bin/cleanup_database.pl --del-exp-selfreg";
+$c.= " -v" if $verbose;
+system($c);

diff --git a/misc/cronjobs/delete_unverified_opac_registrations.pl b/misc/cronjobs/delete_unverified_opac_registrations.pl
index 1a7e700..8f4eecb 100755 (executable)
--- a/misc/cronjobs/delete_unverified_opac_registrations.pl
+++ b/misc/cronjobs/delete_unverified_opac_registrations.pl
@@ -43,6 +43,9 @@ GetOptions(
 );
 my $usage = << 'ENDUSAGE';
 
+IMPORTANT: You should no longer call this script. Please use
+cleanup_database.pl with parameter --del-unv-selfreg.
+
 This script removes unconfirmed OPAC based patron registrations
 that have not been confirmed within the required time period.
 
@@ -60,14 +63,6 @@ if ( $help || !$confirm ) {
     exit;
 }
 
-cronlogaction();
-
-my $dbh = C4::Context->dbh;
-
-$dbh->do( "
-         DELETE FROM borrower_modifications
-         WHERE
-             borrowernumber = 0
-           AND
-             TIME_TO_SEC( TIMEDIFF( NOW(), timestamp )) / 3600 > ?
-", undef, $hours );
+my $d= $hours>=24? int($hours/24): 1;
+my $c= "$FindBin::Bin/cleanup_database.pl -del-unv-selfreg $d";
+system($c);

diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t
index d9136fb..ad9ef7e 100755 (executable)
--- a/t/db_dependent/Members.t
+++ b/t/db_dependent/Members.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 61;
+use Test::More tests => 62;
 use Test::MockModule;
 use Data::Dumper;
 use C4::Context;
@@ -314,6 +314,28 @@ subtest 'GetMemberAccountBalance' => sub {
     $dbh->rollback();
 };
 
+subtest 'purgeSelfRegistration' => sub {
+    plan tests => 2;
+
+    #purge unverified
+    my $d=360;
+    C4::Members::DeleteUnverifiedOpacRegistrations($d);
+    foreach(1..3) {
+        $dbh->do("INSERT INTO borrower_modifications (timestamp, borrowernumber, verification_token) VALUES ('2014-01-01 01:02:03',0,?)", undef, (scalar localtime)."_$_");
+    }
+    is( C4::Members::DeleteUnverifiedOpacRegistrations($d), 3, 'Test for DeleteUnverifiedOpacRegistrations' );
+
+    #purge members in temporary category
+    my $c= 'XYZ';
+    $dbh->do("INSERT IGNORE INTO categories (categorycode) VALUES ('$c')");
+    C4::Context->set_preference('PatronSelfRegistrationDefaultCategory', $c );
+    C4::Context->set_preference('PatronSelfRegistrationExpireTemporaryAccountsDelay', 360);
+    C4::Members::DeleteExpiredOpacRegistrations();
+    $dbh->do("INSERT INTO borrowers (surname, address, city, branchcode, categorycode, dateenrolled) VALUES ('Testaabbcc', 'Street 1', 'CITY', 'CPL', '$c', '2014-01-01 01:02:03')");
+    is( C4::Members::DeleteExpiredOpacRegistrations(), 1, 'Test for DeleteExpiredOpacRegistrations');
+    $dbh->rollback();
+};
+
 sub _find_member {
     my ($resultset) = @_;
     my $found = $resultset && grep( { $_->{cardnumber} && $_->{cardnumber} eq $CARDNUMBER } @$resultset );