From 088ec3df94a11dd96d8c75453684e52307a0c932 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 9 Sep 2024 15:50:24 +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 Improve the output of the verbose mode Tell if the something is wrong in the config (syspref not set) --- Koha/Patrons.pm | 62 ++++++++++++++++------- misc/cronjobs/cleanup_database.pl | 31 +++++++----- t/db_dependent/Koha/Patrons.t | 84 ++++++++++++++++++++++++++++++- t/db_dependent/Members.t | 59 +--------------------- 4 files changed, 147 insertions(+), 89 deletions(-) diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index 309828aad74..40cf36857bf 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,29 +567,52 @@ sub filter_by_have_permission { ); } -sub filter_by_expired_opac_registration { - my ($self,$delay) = @_; +=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 $delay = C4::Context->preference('PatronSelfRegistrationExpireTemporaryAccountsDelay'); 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; - # FIXME Raise an exception if PatronSelfRegistrationDefaultCategory is empty - 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 $registrations_to_del = $self->search({ - categorycode => $category_code, - })->filter_by_last_update({ timestamp_column_name => 'dateenrolled', days => $delay }); - - # Loop, get the ids and return another resultset - while ( my $registration = $registrations_to_del->next() ) { - next if $registration->checkouts->count || $registration->account->balance; - $registration->delete; - $cnt++; + # 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 $cnt; + return Koha::Patrons->search({borrowernumber => \@ids}); } =head3 extended_attributes_config diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl index 526900c86f0..875ede90951 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 954b220a39f..3c667aa2eed 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; @@ -2678,4 +2678,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 26327b35fe4..8b4c38fd54a 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