From 9034dd4b1179e911ce8c7b638b6ddef20d5ee9fa 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 Content-Type: text/plain; charset=utf-8 If you do not use a temporary self registration patron category, you should actually make the preference PatronSelfRegistrationExpireTemporaryAccountsDelay empty. This patch adds a comment in the subroutine that does the deletion to not remove the test on empty delay. It also adds a test in Members.t for the same reason. It corrects the wrong explanation and default value in the sysprefs.sql file. Also it adds further explanation to both related sysprefs. Test plan: Run t/db_dependent/Members. Check in Administration the two adjusted OPAC pref descriptions. Signed-off-by: Marcel de Rooy --- C4/Members.pm | 3 +++ installer/data/mysql/mandatory/sysprefs.sql | 2 +- .../prog/en/modules/admin/preferences/opac.pref | 5 +++-- t/db_dependent/Members.t | 13 +++++++++++-- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 210a9ea0de..3e5beacbb1 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -645,6 +645,9 @@ sub DeleteExpiredOpacRegistrations { my $category_code = C4::Context->preference('PatronSelfRegistrationDefaultCategory'); return 0 if not $category_code or not defined $delay or $delay eq q||; + # DO NOT REMOVE the empty tests on delay here + # Some libraries may not use a temporary category, but want to keep patrons + my $date_enrolled = dt_from_string(); $date_enrolled->subtract( days => $delay ); diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 59db857ca6..12db893f67 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -521,7 +521,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('PatronSelfRegistrationConfirmEmail', '0', NULL, 'Require users to confirm their email address by entering it twice.', 'YesNo'), ('PatronSelfRegistrationDefaultCategory','','','A patron registered via the OPAC will receive a borrower category code set in this system preference.','free'), ('PatronSelfRegistrationEmailMustBeUnique', '0', 'If set, the field borrowers.email will be considered as a unique field on self registering', NULL, 'YesNo'), -('PatronSelfRegistrationExpireTemporaryAccountsDelay','0',NULL,'If PatronSelfRegistrationDefaultCategory is enabled, this system preference controls how long a patron can have a temporary status before the account is deleted automatically. It is an integer value representing a number of days to wait before deleting a temporary patron account. Setting it to 0 disables the deleting of temporary accounts.','Integer'), +('PatronSelfRegistrationExpireTemporaryAccountsDelay', '', NULL, 'If PatronSelfRegistrationDefaultCategory is enabled, this system preference controls how long a patron can have a temporary status before the account is deleted automatically. It is an integer value representing a number of days to wait before deleting a temporary patron account. Making it empty disables the deleting of (temporary) accounts.', 'Integer' ), ('PatronSelfRegistrationLibraryList','',NULL,'Only display libraries listed. If empty, all libraries are displayed.','Free'), ('PatronSelfRegistrationPrefillForm','1',NULL,'Display password and prefill login form after a patron has self registered','YesNo'), ('PatronSelfRegistrationVerifyByEmail','0',NULL,'If enabled, any patron attempting to register themselves via the OPAC will be required to verify themselves via email to activate their account.','YesNo'), 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 5d8e9cabd5..c1943444a3 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 @@ -816,13 +816,14 @@ OPAC: - pref: PatronSelfRegistrationDefaultCategory choices: patron-categories - "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 you make that delay EMPTY." - - 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.
A zero will remove all patrons; but if you make the pref EMPTY, it will not delete any patron." - - "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 b2c39378a0..97ec011f49 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -403,7 +403,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 => 7; #purge unverified my $d=360; @@ -417,7 +417,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', @@ -427,6 +426,16 @@ subtest 'purgeSelfRegistration' => sub { } }); + # First test if empty PatronSelfRegistrationExpireTemporaryAccountsDelay returns zero + # DO NOT REMOVE this test; it might prevent future accidents (when touching the code in Members) + t::lib::Mocks::mock_preference('PatronSelfRegistrationExpireTemporaryAccountsDelay', q{} ); + is( C4::Members::DeleteExpiredOpacRegistrations(), 0, "DeleteExpiredOpacRegistrations with empty delay" ); + # 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.20.1