Bug 22052 - DeleteExpiredOpacRegistrations should skip bad borrowers
Summary: DeleteExpiredOpacRegistrations should skip bad borrowers
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Nick Clemens
QA Contact: Alex Arnaud
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-28 15:58 UTC by Nick Clemens
Modified: 2019-01-16 08:34 UTC (History)
5 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 22052: Unit tests (2.58 KB, patch)
2018-12-28 19:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22052: Refactor DeleteExpiredOpacReigstrations (5.66 KB, patch)
2018-12-28 19:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22052: Unit tests (2.63 KB, patch)
2019-01-04 20:33 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22052: Unit tests (2.63 KB, patch)
2019-01-04 20:33 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22052: Refactor DeleteExpiredOpacReigstrations (5.71 KB, patch)
2019-01-04 20:34 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22052: Unit tests (2.68 KB, patch)
2019-01-07 11:26 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 22052: Refactor DeleteExpiredOpacReigstrations (5.77 KB, patch)
2019-01-07 11:26 UTC, Alex Arnaud
Details | Diff | Splinter Review

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