From 24d02038f8aff7ca70d308da94a365f629cdb103 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Tue, 7 Sep 2021 12:22:05 +0000 Subject: [PATCH] Bug 28943: Lower the risk of accidental patron deletion If you do not use a temporary self registration patron category, you should actually make the preference PatronSelfRegistrationExpireTemporaryAccountsDelay empty. As the comment in sysprefs.sql already said, we should not let a zero value in the pref delete patrons too. The module is changed now, the test adjusted and the description of both related sysprefs modified. Test plan: Run t/db_dependent/Members.t Check in Administration the two adjusted OPAC pref descriptions. Signed-off-by: Marcel de Rooy Signed-off-by: David Nind Signed-off-by: Martin Renvoize Signed-off-by: Chris Cormack --- C4/Members.pm | 6 +++++- .../prog/en/modules/admin/preferences/opac.pref | 5 +++-- t/db_dependent/Members.t | 15 +++++++++++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 8f92c27f9f..86ed21bd09 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -625,7 +625,11 @@ sub DeleteExpiredOpacRegistrations { my $delay = C4::Context->preference('PatronSelfRegistrationExpireTemporaryAccountsDelay'); my $category_code = C4::Context->preference('PatronSelfRegistrationDefaultCategory'); - return 0 if not $category_code or not defined $delay or $delay eq q||; + 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 ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref index 956a05e807..ffa599a3e8 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref @@ -807,13 +807,14 @@ OPAC: - pref: PatronSelfRegistrationDefaultCategory class: short - "as the default patron category for patrons registered via the OPAC." - - "
WARNING: Do not use a regular patron category for self registration. If the misc/cronjobs/cleanup_database.pl cronjob is setup to delete unverified and unfinished OPAC self registrations, it will permanently and unrecoverably delete all patrons that have registered more than PatronSelfRegistrationExpireTemporaryAccountsDelay days ago." + - "
WARNING: Do not use a regular patron category for self registration.
+If the misc/cronjobs/cleanup_database.pl cronjob is setup to delete unverified and unfinished OPAC self registrations, it will permanently and unrecoverably delete all patrons that have registered more than PatronSelfRegistrationExpireTemporaryAccountsDelay days ago (unless that delay is empty or zero)." - - "Delete patrons still in the category indicated by PatronSelfRegistrationDefaultCategory" - pref: PatronSelfRegistrationExpireTemporaryAccountsDelay class: integer - "days after account creation." - - "
NOTE: This system preference requires the misc/cronjobs/cleanup_database.pl cronjob. Ask your system administrator to schedule it." + - "
NOTE: This system preference requires the misc/cronjobs/cleanup_database.pl cronjob. Ask your system administrator to schedule it.
No patrons will be deleted if you set the pref to zero or make it empty." - - "The following database columns must be filled in on the patron entry screen:" - pref: PatronSelfRegistrationBorrowerMandatoryField diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index 9c42333fdd..770ee81887 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -389,7 +389,7 @@ $borrower = Koha::Patrons->find( $borrowernumber )->unblessed; ok( $borrower->{userid}, 'A userid should have been generated correctly' ); subtest 'purgeSelfRegistration' => sub { - plan tests => 5; + plan tests => 8; #purge unverified my $d=360; @@ -403,7 +403,6 @@ subtest 'purgeSelfRegistration' => sub { my $c= 'XYZ'; $dbh->do("INSERT IGNORE INTO categories (categorycode) VALUES ('$c')"); t::lib::Mocks::mock_preference('PatronSelfRegistrationDefaultCategory', $c ); - t::lib::Mocks::mock_preference('PatronSelfRegistrationExpireTemporaryAccountsDelay', 360); C4::Members::DeleteExpiredOpacRegistrations(); my $self_reg = $builder->build_object({ class => 'Koha::Patrons', @@ -413,6 +412,18 @@ subtest 'purgeSelfRegistration' => sub { } }); + # 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=>{ -- 2.39.5