Bug 13049 - Merge selfreg cron jobs into cleanup_database
Summary: Merge selfreg cron jobs into cleanup_database
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-09 07:36 UTC by Marcel de Rooy
Modified: 2016-06-21 21:40 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13049: Merge selfreg cron jobs into cleanup_database (9.87 KB, patch)
2015-03-16 14:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13049: Merge selfreg cron jobs into cleanup_database (10.05 KB, patch)
2015-04-14 06:12 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13049: Merge selfreg cron jobs into cleanup_database (10.39 KB, patch)
2015-04-14 06:15 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13049: Merge selfreg cron jobs into cleanup_database (10.37 KB, patch)
2015-04-15 11:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 13049: Merge selfreg cron jobs into cleanup_database (10.40 KB, patch)
2015-04-17 12:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13049: Merge selfreg cron jobs into cleanup_database (10.46 KB, patch)
2015-05-18 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13049: [QA Follow-up] Remove unneeded module (1.50 KB, patch)
2015-05-20 11:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!