From 5c3c67972841f7ec99638a9387bd53d62ce9ef7d Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 9 Oct 2014 09:52:44 +0200 Subject: [PATCH] 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 . 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 Signed-off-by: Tomas Cohen Arazi --- C4/Members.pm | 55 +++++++++++++++++++ misc/cronjobs/cleanup_database.pl | 46 ++++++++++++++-- .../delete_expired_opac_registrations.pl | 36 ++---------- .../delete_unverified_opac_registrations.pl | 17 ++---- t/db_dependent/Members.t | 24 +++++++- 5 files changed, 131 insertions(+), 47 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 1a172dbffd..95fb4e736a 100644 --- 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 cd92dc4957..0766141026 100755 --- 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 44cc9dabed..206c0ec4c1 100755 --- 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 1a7e70007c..8f4eecb00e 100755 --- 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 d9136fbe2f..ad9ef7eac2 100755 --- 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 ); -- 2.39.5