If this job hits a patron with a checkout/fine it just dies To recreate: 1 - Add some self reg patrons 2 - checkout some items to one 3 - fine another 4 - backdate their registration dates (in the db) 5 - run this job 6 - note the error and fail
Created attachment 83545 [details] [review] Bug 22052: Unit tests
Created attachment 83546 [details] [review] Bug 22052: Refactor DeleteExpiredOpacReigstrations This patch changes the sub to use Koha::Objects and updates the tests. Previously the sub would die on borrowers with checkouts and would delete borrowers if they had fines To test: 1 - prove -v t/db_dependent/ 2 - Set your selfreg preferences: PatronSelfRegistration: Allow PatronSelfRegistrationDefaultCategory: SELFREG (or of your choice) PatronSelfRegistrationExpireTemporaryAccountsDelay: 30 3 - Register a patron into SELFREG or how you set above 4 - Set their date enrolled to two months ago 5 - Checkout an item to the patron 6 - Issue a fine to that patron 7 - perl misc/cronjobs/cleanup_database.pl --del-exp-selfreg -v 8 - The job should die with an error 9 - Check in the item 10 - run the corn again - patron is deleted, oops 11 - Apply patch 12 - Create another patron in the same way 13 - Checkout and fine the patron 14 - run the cron 15 - they are not deleted, and no error 16 - checkin the item 17 - run the cron 18 - they are not deleted and no error 19 - clear the fine 20 - run the cron 21 - patron is deleted, huzzah
Created attachment 83660 [details] [review] Bug 22052: Unit tests Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Created attachment 83661 [details] [review] Bug 22052: Unit tests Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Created attachment 83662 [details] [review] Bug 22052: Refactor DeleteExpiredOpacReigstrations This patch changes the sub to use Koha::Objects and updates the tests. Previously the sub would die on borrowers with checkouts and would delete borrowers if they had fines To test: 1 - prove -v t/db_dependent/ 2 - Set your selfreg preferences: PatronSelfRegistration: Allow PatronSelfRegistrationDefaultCategory: SELFREG (or of your choice) PatronSelfRegistrationExpireTemporaryAccountsDelay: 30 3 - Register a patron into SELFREG or how you set above 4 - Set their date enrolled to two months ago 5 - Checkout an item to the patron 6 - Issue a fine to that patron 7 - perl misc/cronjobs/cleanup_database.pl --del-exp-selfreg -v 8 - The job should die with an error 9 - Check in the item 10 - run the corn again - patron is deleted, oops 11 - Apply patch 12 - Create another patron in the same way 13 - Checkout and fine the patron 14 - run the cron 15 - they are not deleted, and no error 16 - checkin the item 17 - run the cron 18 - they are not deleted and no error 19 - clear the fine 20 - run the cron 21 - patron is deleted, huzzah Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Followed test plan, got the "DBD::mysql::st execute failed: Cannot delete or update a parent row..." has expected. Patch did what it advertised: self-reg'd borrower couldn't be deleted unless it had no checked out items, no fines, and all this without crashing the cron in the mean time. Signed off.
Created attachment 83678 [details] [review] Bug 22052: Unit tests Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 83679 [details] [review] Bug 22052: Refactor DeleteExpiredOpacReigstrations This patch changes the sub to use Koha::Objects and updates the tests. Previously the sub would die on borrowers with checkouts and would delete borrowers if they had fines To test: 1 - prove -v t/db_dependent/ 2 - Set your selfreg preferences: PatronSelfRegistration: Allow PatronSelfRegistrationDefaultCategory: SELFREG (or of your choice) PatronSelfRegistrationExpireTemporaryAccountsDelay: 30 3 - Register a patron into SELFREG or how you set above 4 - Set their date enrolled to two months ago 5 - Checkout an item to the patron 6 - Issue a fine to that patron 7 - perl misc/cronjobs/cleanup_database.pl --del-exp-selfreg -v 8 - The job should die with an error 9 - Check in the item 10 - run the corn again - patron is deleted, oops 11 - Apply patch 12 - Create another patron in the same way 13 - Checkout and fine the patron 14 - run the cron 15 - they are not deleted, and no error 16 - checkin the item 17 - run the cron 18 - they are not deleted and no error 19 - clear the fine 20 - run the cron 21 - patron is deleted, huzzah Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Patch does the job. Test are ok. Code review is ok. So passed QA. However (and that doesn't put these patches in discussion), i'm wondering if should not move the self registered patrons in the deletedborrower table before deleting. Does that make sense to create an other bug ?
(In reply to Alex Arnaud from comment #9) > Patch does the job. Test are ok. Code review is ok. So passed QA. > > However (and that doesn't put these patches in discussion), i'm wondering if > should not move the self registered patrons in the deletedborrower table > before deleting. Does that make sense to create an other bug ? Bug 22078
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.02
Pushed to 18.05.x for 18.05.08
Pushed to 17.11.x for 17.11.14 Thanks a lot for the unit test, it makes life sweeter ;)