@@ -, +, @@ Koha::Patrons->filter_by_expired_opac_registrations Koha::Patrons->filter_by_safe_to_delete --del-exp-selfreg (as well as --verbose and --confirm) and confirm that --- 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(-) --- a/C4/Members.pm +++ a/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; --- a/Koha/Patrons.pm +++ a/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 --- a/misc/cronjobs/cleanup_database.pl +++ a/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) = @_; --- a/t/db_dependent/Koha/Patrons.t +++ a/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; --- a/t/db_dependent/Members.t +++ a/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 ); --