Attachment 171201 Details for Bug 37845 – Bug 37845: Remove C4::Members::DeleteExpiredOpacRegistrations

[patch] Bug 37845: Remove C4::Members::DeleteExpiredOpacRegistrations

Bug-37845-Remove-C4MembersDeleteExpiredOpacRegistr.patch (text/plain), 13.51 KB, created by Jonathan Druart on 2024-09-09 14:00:31 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2024-09-09 14:00:31 UTC

Size: 13.51 KB

patch

obsolete

>From 088ec3df94a11dd96d8c75453684e52307a0c932 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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

Actions: View | Diff | Splinter Review

Attachments on bug 37845: 171201 | 171202 | 171999 | 172015