[Bug 13049] New: Improve self-registration cronjobs
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Bug ID: 13049 Summary: Improve self-registration cronjobs Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Command-line Utilities Assignee: gmcharlt@gmail.com Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |11945 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #2 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- 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..
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #4 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |m.de.rooy@rijksmuseum.nl Depends on|11945 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #5 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 36929 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36929&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Improve self-registration |Merge selfreg cron jobs |cronjobs |into cleanup_database -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36929|0 |1 is obsolete| | --- Comment #6 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 37792 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37792&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37792|0 |1 is obsolete| | --- Comment #7 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 37793 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37793&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #8 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37793|0 |1 is obsolete| | --- Comment #9 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 37903 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37903&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> ---
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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #11 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Yes, this is separate, just thought maybe better to discuss first before code written :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37903|0 |1 is obsolete| | --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 38042 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38042&action=edit [PASSED QA] 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |enhancement CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38042|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 39276 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39276&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Rebased: cronlogaction is called in cleanup_database; so the action is logged. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 39343 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39343&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Nice work Marcel! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10744 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10744 [Bug 10744] ExpireReservesMaxPickUpDelay works with hold(s) over report -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13049 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|10744 | Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10744 [Bug 10744] ExpireReservesMaxPickUpDelay works with hold(s) over report -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org