From c745fe17d0f96da2800647a925457be658f59993 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 5 Sep 2024 17:06:04 +0200 Subject: [PATCH] Bug 37845: Remove C4::Members::DeleteExpiredOpacRegistrations This patch remove the subroutine C4::Members::DeleteExpiredOpacRegistrations. The code is moved and adjusted to Koha::Patrons. We now have 2 new methods: Koha::Patrons->filter_by_expired_opac_registrations Koha::Patrons->filter_by_safe_to_delete Test plan: Run the misc/cronjobs/cleanup_database.pl script with the --del-exp-selfreg (as well as --verbose and --confirm) and confirm that it behaves as expected Note that it improve the output of the verbose mode and now tell if the something is wrong with the config (syspref not set) Signed-off-by: Sukhmandeep Benipal --- C4/Members.pm | 36 ------------- Koha/Patrons.pm | 49 ++++++++++++++++++ misc/cronjobs/cleanup_database.pl | 31 +++++++----- t/db_dependent/Koha/Patrons.t | 84 ++++++++++++++++++++++++++++++- t/db_dependent/Members.t | 59 +--------------------- 5 files changed, 153 insertions(+), 106 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 4d37b33ad2..82a013db3b 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -45,8 +45,6 @@ BEGIN { GetBorrowersToExpunge IssueSlip - - DeleteExpiredOpacRegistrations ); } @@ -489,40 +487,6 @@ sub IssueSlip { ); } -=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 unless $category_code && $delay; - # DO NOT REMOVE test on delay here! - # Some libraries may not use a temporary category, but want to keep patrons. - # We should not delete patrons when the value is NULL, empty string or 0. - - my $date_enrolled = dt_from_string(); - $date_enrolled->subtract( days => $delay ); - - my $registrations_to_del = Koha::Patrons->search({ - dateenrolled => {'<=' => $date_enrolled->ymd}, - categorycode => $category_code, - }); - - my $cnt=0; - while ( my $registration = $registrations_to_del->next() ) { - next if $registration->checkouts->count || $registration->account->balance; - $registration->delete; - $cnt++; - } - return $cnt; -} - END { } # module clean-up code here (global destructor) 1; diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index ccc7dad65c..04d3bf05f6 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -27,6 +27,7 @@ use Koha::DateUtils qw( dt_from_string ); use Koha::ArticleRequests; use Koha::Patron; use Koha::Exceptions::Patron; +use Koha::Exceptions::SysPref; use Koha::Patron::Categories; use base qw(Koha::Objects::Mixin::ExtendedAttributes Koha::Objects); @@ -566,6 +567,54 @@ sub filter_by_have_permission { ); } +=head3 filter_by_expired_opac_registrations + + my $expired_registrations = $patrons->filter_by_expired_opac_registrations; + +Return patrons that have not upgraded from the 'temporary' category + +=cut + +sub filter_by_expired_opac_registrations { + my ($self) = @_; + + my $category_code = C4::Context->preference('PatronSelfRegistrationDefaultCategory'); + Koha::Exceptions::SysPref::NotSet->throw( syspref => 'PatronSelfRegistrationDefaultCategory' ) + unless $category_code; + + my $delay = C4::Context->preference('PatronSelfRegistrationExpireTemporaryAccountsDelay'); + Koha::Exceptions::SysPref::NotSet->throw( syspref => 'PatronSelfRegistrationExpireTemporaryAccountsDelay' ) + unless $delay; + + # DO NOT REMOVE test on delay here! + # Some libraries may not use a temporary category, but want to keep patrons. + # We should not delete patrons when the value is NULL, empty string or 0. + + return $self->search( + { + categorycode => $category_code, + } + )->filter_by_last_update( { timestamp_column_name => 'dateenrolled', days => $delay } ); +} + +=head3 filter_by_safe_to_delete + + my $safe_to_delete_patrons = $patrons->filter_by_safe_to_delete; + +Return the patrons that are safe to delete + +=cut + +sub filter_by_safe_to_delete { + my ($self) = @_; + my @ids; + while ( my $patron = $self->next ) { + push @ids, $patron->borrowernumber + if $patron->safe_to_delete; + } + return Koha::Patrons->search( { borrowernumber => \@ids } ); +} + =head3 extended_attributes_config =cut diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl index 526900c86f..d76499b873 100755 --- a/misc/cronjobs/cleanup_database.pl +++ b/misc/cronjobs/cleanup_database.pl @@ -32,6 +32,7 @@ use constant DEFAULT_JOBS_PURGETYPES => qw{ update_elastic_index }; use constant DEFAULT_EDIFACT_MSG_PURGEDAYS => 365; use Getopt::Long qw( GetOptions ); +use Try::Tiny; use Koha::Script -cron; @@ -513,14 +514,23 @@ if ($verbose) { say $confirm ? sprintf( "Deleted %d patrons", $count ) : sprintf( "%d patrons would have been deleted", $count ); } -# FIXME The output for dry-run mode needs to be improved -# But non trivial changes to C4::Members need to be done before. if ($pExpSelfReg) { - if ($confirm) { - DeleteExpiredSelfRegs(); - } elsif ($verbose) { - say "self-registered borrowers may be deleted"; - } + try { + my $opac_registrations = Koha::Patrons->search->filter_by_expired_opac_registrations->filter_by_safe_to_delete; + my $count = $opac_registrations->count; + $opac_registrations->delete if $confirm; + if ($verbose) { + say $confirm + ? sprintf( "Done with removing %d expired self-registrations", $count ) + : sprintf( "%d expired self-registrations would have been removed", $count ); + } + } catch { + if ( ref($_) eq 'Koha::Exceptions::SysPref::NotSet' ) { + say sprintf "Self-registrations cannot be removed, system preference %s is not set", $_->syspref; + } else { + $_->rethrow; + } + }; } if ($pUnvSelfReg) { @@ -536,6 +546,8 @@ if ($pUnvSelfReg) { } } +# FIXME The output for dry-run mode needs to be improved +# But non trivial changes to C4::Members need to be done before. if ($special_holidays_days) { if ($confirm) { DeleteSpecialHolidays( abs($special_holidays_days) ); @@ -908,11 +920,6 @@ sub PurgeCreatorBatches { return $count; } -sub DeleteExpiredSelfRegs { - my $cnt = C4::Members::DeleteExpiredOpacRegistrations(); - print "Removed $cnt expired self-registered borrowers\n" if $verbose; -} - sub DeleteSpecialHolidays { my ($days) = @_; diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index d3adc77dd7..23421fc5d5 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 43; +use Test::More tests => 44; use Test::Warn; use Test::Exception; use Test::MockModule; @@ -2700,4 +2700,86 @@ subtest 'filter_by_have_permission' => sub { $schema->storage->txn_rollback; }; +subtest 'filter_by_expired_opac_registrations' => sub { + plan tests => 8; + + $schema->storage->txn_begin; + + my $category = $builder->build_object( { class => 'Koha::Patron::Categories' } ); + t::lib::Mocks::mock_preference( 'PatronSelfRegistrationDefaultCategory', $category->categorycode ); + my $self_reg = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + dateenrolled => '2014-01-01 01:02:03', + categorycode => $category->categorycode + } + } + ); + + # First test if empty PatronSelfRegistrationExpireTemporaryAccountsDelay returns an exception + t::lib::Mocks::mock_preference( 'PatronSelfRegistrationExpireTemporaryAccountsDelay', q{} ); + throws_ok { Koha::Patrons->filter_by_expired_opac_registrations } + 'Koha::Exceptions::SysPref::NotSet', + 'Attempt to filter with empty PatronSelfRegistrationExpireTemporaryAccountsDelay throws exception.'; + + # Test zero too + t::lib::Mocks::mock_preference( 'PatronSelfRegistrationExpireTemporaryAccountsDelay', 0 ); + throws_ok { Koha::Patrons->filter_by_expired_opac_registrations } + 'Koha::Exceptions::SysPref::NotSet', + 'Attempt to filter with PatronSelfRegistrationExpireTemporaryAccountsDelay set to 0 throws exception.'; + + # Also check empty category + t::lib::Mocks::mock_preference( 'PatronSelfRegistrationDefaultCategory', q{} ); + throws_ok { Koha::Patrons->filter_by_expired_opac_registrations } + 'Koha::Exceptions::SysPref::NotSet', + 'Attempt to filter with empty PatronSelfRegistrationDefaultCategory throws exception.'; + + t::lib::Mocks::mock_preference( 'PatronSelfRegistrationExpireTemporaryAccountsDelay', 360 ); + throws_ok { Koha::Patrons->filter_by_expired_opac_registrations } + 'Koha::Exceptions::SysPref::NotSet', + 'Attempt to filter with empty PatronSelfRegistrationDefaultCategory throws exception, even if PatronSelfRegistrationExpireTemporaryAccountsDelay is set.'; + + t::lib::Mocks::mock_preference( 'PatronSelfRegistrationDefaultCategory', $category->categorycode ); + + my $checkout = $builder->build_object( + { + class => 'Koha::Checkouts', + value => { borrowernumber => $self_reg->borrowernumber } + } + ); + is( + Koha::Patrons->filter_by_expired_opac_registrations->filter_by_safe_to_delete->count, 0, + "filter_by_safe_to_delete doesn't delete borrower with checkout" + ); + + my $account_line = $builder->build_object( + { + class => 'Koha::Account::Lines', + value => { + borrowernumber => $self_reg->borrowernumber, + amountoutstanding => 5, + } + } + ); + is( + Koha::Patrons->filter_by_expired_opac_registrations->filter_by_safe_to_delete->count, 0, + "filter_by_safe_to_delete doesn't delete borrower with checkout and fine" + ); + + $checkout->delete; + is( + Koha::Patrons->filter_by_expired_opac_registrations->filter_by_safe_to_delete->count, 0, + "filter_by_safe_to_delete doesn't delete borrower with fine and no checkout" + ); + + $account_line->delete; + is( + Koha::Patrons->filter_by_expired_opac_registrations->filter_by_safe_to_delete->count, 1, + "filter_by_safe_to_delete does delete borrower with no fines and no checkouts" + ); + + $schema->storage->txn_rollback; +}; + $schema->storage->txn_rollback; diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index 26327b35fe..8b4c38fd54 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 51; +use Test::More tests => 50; use Test::MockModule; use Test::Exception; @@ -33,7 +33,7 @@ use t::lib::Mocks; use t::lib::TestBuilder; BEGIN { - use_ok('C4::Members', qw( GetBorrowersToExpunge DeleteExpiredOpacRegistrations )); + use_ok('C4::Members', qw( GetBorrowersToExpunge )); } my $schema = Koha::Database->schema; @@ -388,61 +388,6 @@ ok( $borrowernumber > 0, 'Koha::Patron->store should have inserted the patron ev $borrower = Koha::Patrons->find( $borrowernumber )->unblessed; ok( $borrower->{userid}, 'A userid should have been generated correctly' ); -subtest 'purgeSelfRegistration' => sub { - plan tests => 7; - - #purge members in temporary category - my $c= 'XYZ'; - $dbh->do("INSERT IGNORE INTO categories (categorycode) VALUES ('$c')"); - t::lib::Mocks::mock_preference('PatronSelfRegistrationDefaultCategory', $c ); - C4::Members::DeleteExpiredOpacRegistrations(); - my $self_reg = $builder->build_object({ - class => 'Koha::Patrons', - value => { - dateenrolled => '2014-01-01 01:02:03', - categorycode => $c - } - }); - - # First test if empty PatronSelfRegistrationExpireTemporaryAccountsDelay returns zero - t::lib::Mocks::mock_preference('PatronSelfRegistrationExpireTemporaryAccountsDelay', q{} ); - is( C4::Members::DeleteExpiredOpacRegistrations(), 0, "DeleteExpiredOpacRegistrations with empty delay" ); - # Test zero too - t::lib::Mocks::mock_preference('PatronSelfRegistrationExpireTemporaryAccountsDelay', 0 ); - is( C4::Members::DeleteExpiredOpacRegistrations(), 0, "DeleteExpiredOpacRegistrations with delay 0" ); - # Also check empty category - t::lib::Mocks::mock_preference('PatronSelfRegistrationDefaultCategory', q{} ); - t::lib::Mocks::mock_preference('PatronSelfRegistrationExpireTemporaryAccountsDelay', 360 ); - is( C4::Members::DeleteExpiredOpacRegistrations(), 0, "DeleteExpiredOpacRegistrations with empty category" ); - t::lib::Mocks::mock_preference('PatronSelfRegistrationDefaultCategory', $c ); - - my $checkout = $builder->build_object({ - class=>'Koha::Checkouts', - value=>{ - borrowernumber=>$self_reg->borrowernumber - } - }); - is( C4::Members::DeleteExpiredOpacRegistrations(), 0, "DeleteExpiredOpacRegistrations doesn't delete borrower with checkout"); - - my $account_line = $builder->build_object( - { - class => 'Koha::Account::Lines', - value => { - borrowernumber => $self_reg->borrowernumber, - amountoutstanding => 5, - } - } - ); - is( C4::Members::DeleteExpiredOpacRegistrations(), 0, "DeleteExpiredOpacRegistrations doesn't delete borrower with checkout and fine"); - - $checkout->delete; - is( C4::Members::DeleteExpiredOpacRegistrations(), 0, "DeleteExpiredOpacRegistrations doesn't delete borrower with fine and no checkout"); - - $account_line->delete; - is( C4::Members::DeleteExpiredOpacRegistrations(), 1, "DeleteExpiredOpacRegistrations does delete borrower with no fines and no checkouts"); - -}; - sub _find_member { my ($resultset) = @_; my $found = $resultset && grep( { $_->{cardnumber} && $_->{cardnumber} eq $CARDNUMBER } @$resultset ); -- 2.34.1