Bug 13049

Summary: Merge selfreg cron jobs into cleanup_database
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Command-line UtilitiesAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 20856, 21855    
Attachments: Bug 13049: Merge selfreg cron jobs into cleanup_database
Bug 13049: Merge selfreg cron jobs into cleanup_database
Bug 13049: Merge selfreg cron jobs into cleanup_database
Bug 13049: Merge selfreg cron jobs into cleanup_database
[PASSED QA] Bug 13049: Merge selfreg cron jobs into cleanup_database
Bug 13049: Merge selfreg cron jobs into cleanup_database
Bug 13049: [QA Follow-up] Remove unneeded module

Description Marcel de Rooy 2014-10-09 07:36:42 UTC
There are two small cronjobs for self-registration: delete_expired_opac_registrations.pl and delete_unverified_opac_registrations.pl

The expired job is meant to delete from borrowers when the patron category is equal to PatronSelfRegistrationDefaultCategory and date enrolled past PatronSelfRegistrationExpireTemporaryAccountsDelay. (See also bug 11945.) It seems to me that this behavior is unexpected since the pref descriptions do not suggest that the default category is temporary.
Perhaps, it was meant to add some verification step into the process when you do not use the email verification triggered by pref PatronSelfRegistrationVerifyByEmail.

The unverified job is meant to delete from borrower_modifications and uses a time parameter (defaulting to 24 hours) to find unverified entries. It seems to me that it should use the PatronSelfRegistrationExpireTemporaryAccountsDelay preference. (Read its description.)

I propose the following changes:
[1] Move these actions to cleanupdatabase. Do they really need two separate scripts?
[2] Rename the Delay preference. Name and description conflict now. We could make name and description cover both cases: (a) verify is on and still unverified and (b) verify is off and still in the initial category.
[3] Trigger the borrowers/category delete only if Verify is off and the renamed delay has been past.
Comment 1 Kyle M Hall 2014-10-09 11:04:03 UTC
I have no objections to moving these behaviors to cleanupdatabase. My only concern would be to make sure upgraders have clear notification that the old cron scripts have gone away and the functionality has been moved.

delete_expired_opac_registrations.pl is indeed meant to delete patron accounts that are still set to the default category after a set time. I agree there is some confusion here. There are two use cases:
1) This PatronSelfRegistrationDefaultCategory is a provisional category which is meant to allow a patron to immediately access online resources but not to check out items. In the scenario, the patron is supposed to come into the library within a given amount of time to verify his or her identity, and have a librarian change the patron category to a permanent category. That way the patron is now a full-fledged member who can check out items. This is the scenario where delete_expired_opac_registrations.pl is meant to be used.
2) The PatronSelfRegistrationDefaultCategory *is* full fledged membership category. In this scenario no verification is required by the library and delete_expired_opac_registrations.pl should *not* be used, ever.

delete_unverified_opac_registrations.pl is more of a garbage cleanup. If PatronSelfRegistrationVerifyByEmail is enabled, patrons try to create an account, but never confirm the account creation via the emailed link. We don't want these to just pile up, so this script removes them. Note this is only for new patron account submissions, and not for patron modification requests to existing accounts ( hence where borrowernumber = 0 ).

> I propose the following changes:
> [1] Move these actions to cleanupdatabase. Do they really need two separate
> scripts?

I think delete_unverified_opac_registrations.pl is a prime candidate for 
folding into cleanupdatabase.

I suppose delete_expired_opac_registrations.pl could be moved into it as well. I would not object to that, but it's not quite the same as removing crufty old data. This script deletes what may be active patron accounts. It isn't really cleanup. I do like the idea of one unified script though.

> [2] Rename the Delay preference. Name and description conflict now. We could
> make name and description cover both cases: (a) verify is on and still
> unverified and (b) verify is off and still in the initial category.

I'm questioning whether we should have the preference at all. Is PatronSelfRegistrationExpireTemporaryAccountsDelay something we really need libraries to be able to change without the help of an administrator? Maybe this should be a days parameter like it is for delete_unverified_opac_registrations.pl?

> [3] Trigger the borrowers/category delete only if Verify is off and the
> renamed delay has been past.

This part I'm not understanding. When you say Verify do you mean PatronSelfRegistrationVerifyByEmail, or being verified by having the patron category changed? I don't think we should automate the triggering of these deletions. This behavior may be very custom depending on the needs of the librarians.

So in summary, my suggested course of action would be to:
1) Fold both scripts into cleanupdatabase with separate switches and time parameters for each ( maybe --delete-expired-self-registration=<days> and --delete-unverified-self-registrations=<hours> so we don't need two parameters for each.
2) Remove PatronSelfRegistrationExpireTemporaryAccountsDelay completely
3) Update PatronSelfRegistrationDefaultCategory to indicate that if they use --delete-expired-self-registration=<days> that this category should *only* be for provisional self-registered patrons

What do you think?
Comment 2 Marcel de Rooy 2014-10-09 11:48:09 UTC
(In reply to Kyle M Hall from comment #1)
> What do you think?

Thanks. This is quite clear. I will send a proposed patch. Still thinking a little bit on the provisional category-case..
Comment 3 Kyle M Hall 2015-02-10 18:00:59 UTC
Are you still planning on submitting a patch for this bug?

(In reply to M. de Rooy from comment #2)
> (In reply to Kyle M Hall from comment #1)
> > What do you think?
> 
> Thanks. This is quite clear. I will send a proposed patch. Still thinking a
> little bit on the provisional category-case..
Comment 4 Marcel de Rooy 2015-02-11 12:10:25 UTC
(In reply to Kyle M Hall from comment #3)
> Are you still planning on submitting a patch for this bug?

Yes. It is still on my list :) Just need some time..
Comment 5 Marcel de Rooy 2015-03-16 14:13:51 UTC Comment hidden (obsolete)
Comment 6 Frédéric Demians 2015-04-14 06:12:00 UTC Comment hidden (obsolete)
Comment 7 Frédéric Demians 2015-04-14 06:15:09 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2015-04-14 12:28:46 UTC
Thanks, Frederic for your SO.
I am not sure however if everything went fine?

When applying your patch, I get:
fatal: sha1 information is lacking or useless (misc/cronjobs/cleanup_database.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.

And the signed-off patch contains this line:
+>>>>>>> Bug 13049: Merge selfreg cron jobs into cleanup_database
It is in the usage statement of cleanup_database.pl

If it is easy for you to fix it, go ahead :) Otherwise I will fix it tomorrow or so..
Comment 9 Marcel de Rooy 2015-04-15 11:41:10 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2015-04-15 22:10:42 UTC
> A next step (as described on the Bugzilla report) would be: remove the Delay
> pref for self regs.

Can we rethink this? There is another 'movement' in the patches recently to create preferences for options in the cronjobs. This is helpful when you are running multiple installations on one server, where maintaining separate cronjobs for each library would be quite a hassle. Ultimately I'd like a separate tool for those things, but as we have a pref already, why not keep it until that arrives?
Comment 11 Marcel de Rooy 2015-04-16 06:32:35 UTC
(In reply to Katrin Fischer from comment #10)
> > A next step (as described on the Bugzilla report) would be: remove the Delay
> > pref for self regs.
> 
> Can we rethink this? There is another 'movement' in the patches recently to
> create preferences for options in the cronjobs. This is helpful when you are
> running multiple installations on one server, where maintaining separate
> cronjobs for each library would be quite a hassle. Ultimately I'd like a
> separate tool for those things, but as we have a pref already, why not keep
> it until that arrives?

Fine with me. But this could proceed now separate of that.
Comment 12 Katrin Fischer 2015-04-16 07:44:50 UTC
Yes, this is separate, just thought maybe better to discuss first before code written :)
Comment 13 Kyle M Hall 2015-04-17 12:05:54 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2015-05-18 09:57:16 UTC
Created attachment 39276 [details] [review]
Bug 13049: Merge selfreg cron jobs into cleanup_database

This patch moves the core code of two selfreg cron jobs into the Members
module. The new routines are called from cleanup_database with two new
parameters. The old cron jobs are now wrappers to cleanup_database.
As a bonus, we can add a unit test now.

In time, we can obsolete the selfreg cron jobs. For now, the code is in one
place and behavior does not change.
A next step (as described on the Bugzilla report) would be: remove the Delay
pref for self regs.

Test plan:
Run the unit test t/db_dependent/Members.t.
Test the two new parameters of cleanup_database.pl.
Verify if delete_expired_opac_registrations.pl still works.
Same for delete_unverified_opac_registrations.pl.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
. Fixed minor merge confict on UT & cleanup_database.pl
. UT ok
. The two deprecated scripts still work as before, with a warning
  message.
. cleanup_database.pl do the deletion job, calling new C4::Members
  function rather that doing it directly.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Marcel de Rooy 2015-05-18 09:59:04 UTC
Rebased: cronlogaction is called in cleanup_database; so the action is logged.
Comment 16 Marcel de Rooy 2015-05-20 11:46:36 UTC
Created attachment 39343 [details] [review]
Bug 13049: [QA Follow-up] Remove unneeded module

The call to cronlogaction is done in cleanup_database. So there is no use
in keeping the module here.

Test plan:
Run or compile (perl -c) script delete_expired_opac_registrations.pl.
Run or compile (perl -c) script delete_unverified_opac_registrations.pl.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Tomás Cohen Arazi 2015-07-28 13:38:46 UTC
Patches pushed to master.

Nice work Marcel!
Comment 18 Katrin Fischer 2018-06-01 12:47:51 UTC
It looks like this change introduced a regression. Before the changes it was possible to define the delay for unverified registrations in hours and now it's days. It will also change what actually happens when you switch over?

With GDPR being very strict about saving data the initial feature of being able to delete after x hours would be better.
Comment 19 Marcel de Rooy 2018-06-01 13:17:13 UTC
(In reply to Katrin Fischer from comment #18)
> It looks like this change introduced a regression. Before the changes it was
> possible to define the delay for unverified registrations in hours and now
> it's days. It will also change what actually happens when you switch over?
> 
> With GDPR being very strict about saving data the initial feature of being
> able to delete after x hours would be better.

I think you are being even stricter now :)
Comment 20 Katrin Fischer 2018-06-01 13:29:37 UTC
(In reply to Marcel de Rooy from comment #19)
> (In reply to Katrin Fischer from comment #18)
> > It looks like this change introduced a regression. Before the changes it was
> > possible to define the delay for unverified registrations in hours and now
> > it's days. It will also change what actually happens when you switch over?
> > 
> > With GDPR being very strict about saving data the initial feature of being
> > able to delete after x hours would be better.
> 
> I think you are being even stricter now :)

Triggered by a request from a library.
Comment 21 Marcel de Rooy 2018-06-04 07:32:29 UTC
(In reply to Katrin Fischer from comment #20)
> (In reply to Marcel de Rooy from comment #19)
> > (In reply to Katrin Fischer from comment #18)
> > > It looks like this change introduced a regression. Before the changes it was
> > > possible to define the delay for unverified registrations in hours and now
> > > it's days. It will also change what actually happens when you switch over?
> > > 
> > > With GDPR being very strict about saving data the initial feature of being
> > > able to delete after x hours would be better.
> > 
> > I think you are being even stricter now :)
> 
> Triggered by a request from a library.

I will try to submit some patches on bug 20819 this week. It now includes a (pref controlled) checkbox on opac-memberentry to give consent during selfregistration. No consent means no registration. Which makes this request less urgent again, since you will have a consent. (But note that we could already argue that it is not possible to "perform this contract" of self-registration without data. Second ground of GDPR.)