Bugzilla – Attachment 172015 Details for
Bug 37845
Remove C4::Members::DeleteExpiredOpacRegistrations
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 37845: Remove C4::Members::DeleteExpiredOpacRegistrations
Bug-37845-Remove-C4MembersDeleteExpiredOpacRegistr.patch (text/plain), 14.36 KB, created by
Kyle M Hall (khall)
on 2024-09-26 18:46:48 UTC
(
hide
)
Description:
Bug 37845: Remove C4::Members::DeleteExpiredOpacRegistrations
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2024-09-26 18:46:48 UTC
Size:
14.36 KB
patch
obsolete
>From 8ba30602abde39084b35895d90258237c2c5b888 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >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 <sukhmandeep.benipal@inLibro.com> >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > 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 4d37b33ad2e..82a013db3b1 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 ccc7dad65c8..04d3bf05f6d 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 526900c86f0..d76499b8734 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 d3adc77dd78..23421fc5d5a 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 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.39.5 (Apple Git-154)
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=172015">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 37845
:
171201
|
171202
|
171999
| 172015