Bug 22052

Summary: DeleteExpiredOpacRegistrations should skip bad borrowers
Product: Koha Reporter: Nick Clemens <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Alex Arnaud <alex.arnaud>
Severity: major    
Priority: P5 - low CC: alex.arnaud, charles.farmer, fridolin.somers, jesse, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21065
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 22052: Unit tests
Bug 22052: Refactor DeleteExpiredOpacReigstrations
Bug 22052: Unit tests
Bug 22052: Unit tests
Bug 22052: Refactor DeleteExpiredOpacReigstrations
Bug 22052: Unit tests
Bug 22052: Refactor DeleteExpiredOpacReigstrations

Description Nick Clemens 2018-12-28 15:58:26 UTC
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
Comment 1 Nick Clemens 2018-12-28 19:19:10 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2018-12-28 19:19:13 UTC Comment hidden (obsolete)
Comment 3 Charles Farmer 2019-01-04 20:33:36 UTC Comment hidden (obsolete)
Comment 4 Charles Farmer 2019-01-04 20:33:56 UTC
Created attachment 83661 [details] [review]
Bug 22052: Unit tests

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 5 Charles Farmer 2019-01-04 20:34:00 UTC
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>
Comment 6 Charles Farmer 2019-01-04 20:40:15 UTC
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.
Comment 7 Alex Arnaud 2019-01-07 11:26:06 UTC
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>
Comment 8 Alex Arnaud 2019-01-07 11:26:15 UTC
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>
Comment 9 Alex Arnaud 2019-01-07 11:35:21 UTC
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 ?
Comment 10 Nick Clemens 2019-01-07 18:36:05 UTC
(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
Comment 11 Nick Clemens 2019-01-08 15:56:14 UTC
Awesome work all!

Pushed to master for 19.05
Comment 12 Martin Renvoize 2019-01-10 13:51:43 UTC
Pushed to 18.11.x for 18.11.02
Comment 13 Jesse Maseto 2019-01-11 20:19:14 UTC
Pushed to 18.05.x for 18.05.08
Comment 14 Fridolin Somers 2019-01-16 08:34:04 UTC
Pushed to 17.11.x for 17.11.14

Thanks a lot for the unit test, it makes life sweeter ;)