Bug 10419 - There is no cronjob script for deleting patrons
Summary: There is no cronjob script for deleting patrons
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 20915
  Show dependency treegraph
 
Reported: 2013-06-05 14:27 UTC by Jonathan Druart
Modified: 2018-09-21 21:00 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10419: Script for deleting patrons (3.78 KB, patch)
2013-06-05 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: Script for deleting patrons (3.74 KB, patch)
2013-06-07 08:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: Script for deleting patrons (3.81 KB, patch)
2013-06-07 13:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10419: Followup - Script for deleting patrons (3.18 KB, patch)
2013-06-07 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: Followup - Script for deleting patrons (3.23 KB, patch)
2013-06-14 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10419: Add the branchcode parameter. (2.35 KB, patch)
2013-07-15 07:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: Add the branchcode parameter. (2.40 KB, patch)
2013-07-15 12:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10419: Script for deleting patrons (3.84 KB, patch)
2013-08-05 22:46 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10419: Followup - Script for deleting patrons (3.28 KB, patch)
2013-08-05 22:46 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10419: Add the branchcode parameter. (2.45 KB, patch)
2013-08-05 22:46 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10419 : Fixing license number (1.00 KB, patch)
2013-08-05 22:47 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10419: new cronjob for deleting patrons (3.90 KB, patch)
2013-08-06 15:49 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10419: (followup) - tidy up cronjob for deleting patrons (3.34 KB, patch)
2013-08-06 15:49 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10419: add branchcode parameter to patron deletion script (2.53 KB, patch)
2013-08-06 15:49 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10419: (followup) fix license statement (1.07 KB, patch)
2013-08-06 15:49 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10419: (follow-up) tidy names of command-line switches (3.06 KB, patch)
2013-08-06 15:49 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10419: (follow-up) functional improvements (3.65 KB, patch)
2013-08-06 15:49 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10419: (followup) fix license statement (1.13 KB, patch)
2013-08-13 12:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: (follow-up) tidy names of command-line switches (3.12 KB, patch)
2013-08-13 12:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: (follow-up) functional improvements (3.72 KB, patch)
2013-08-13 12:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: FIX hardcoded records and pod (1.46 KB, patch)
2013-08-13 12:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10419: Patrons with fines should not be deleted (2.47 KB, patch)
2013-09-30 12:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10419: FIX hardcoded records and pod (1.57 KB, patch)
2013-10-03 14:13 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10419: Patrons with fines should not be deleted (2.81 KB, patch)
2013-10-03 14:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10419: FIX hardcoded records and pod (1.57 KB, patch)
2013-10-03 14:22 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10419: Patrons with fines should not be deleted (2.81 KB, patch)
2013-10-03 14:22 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: new cronjob for deleting patrons (4.05 KB, patch)
2013-10-03 16:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: (followup) - tidy up cronjob for deleting patrons (3.41 KB, patch)
2013-10-03 16:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: add branchcode parameter to patron deletion script (2.59 KB, patch)
2013-10-03 16:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: (followup) fix license statement (1.19 KB, patch)
2013-10-03 16:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: (follow-up) tidy names of command-line switches (3.19 KB, patch)
2013-10-03 16:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: (follow-up) functional improvements (3.78 KB, patch)
2013-10-03 16:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: FIX hardcoded records and pod (1.63 KB, patch)
2013-10-03 16:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10419: Patrons with fines should not be deleted (4.17 KB, patch)
2013-10-03 16:01 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-06-05 14:27:22 UTC

    
Comment 1 Jonathan Druart 2013-06-05 14:34:29 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-06-07 04:48:46 UTC
Not nit picking, but would be nicer to write

eval {};
if ($@) {
    say "Failed ... $@";
    next;
}

eval {};
if ($@) {
    say "Failed ... $@";
    next;
}

say OK;

Thanks.
Comment 3 Jonathan Druart 2013-06-07 08:26:53 UTC
Srdjan, yep, you are right! I don't know at all why I wrote that this way, not really readable... :)
Thanks for the catch.
Comment 4 Jonathan Druart 2013-06-07 08:27:03 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-06-07 13:04:57 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-06-07 13:08:34 UTC
Looks good, but I've a few suggestions:

1) I'd switch from --dry-run to --confirm. This seems to be a more standard way to do things ( in so far as a number of scripts have --confirm and none have --dry-run ).

2) The script has "first person" style output, whereas no other Koha cli scripts do. It might be better to remove the "I"s from the script output

3) The use of the "date" program in the example output, while clever and a great idea, makes the help a bit confusing and hard to read. It would be better to juse put hard dates in the example usage, and a short explanation on how to use `date` to autogenerate the dates.
Comment 7 Jonathan Druart 2013-06-07 13:59:50 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-06-07 14:01:17 UTC
Thanks for testing Kyle. I provide a new patch in order to apply your suggestions.
Comment 9 Kyle M Hall 2013-06-14 12:02:59 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-06-14 12:03:24 UTC
Everything looks great! Thanks for following up on my suggestions!
Comment 11 Katrin Fischer 2013-07-13 08:14:56 UTC
This still needs testing, but I didn't spot problems while reviewing the code. Only wondering if we should offer the branchcode as another option in the script. GetBorrowersToExpunge would allow it already:

2025 =head2 GetBorrowersToExpunge
2026 
2027   $borrowers = &GetBorrowersToExpunge(
2028       not_borrowered_since => $not_borrowered_since,
2029       expired_before       => $expired_before,
2030       category_code        => $category_code,
2031       branchcode           => $branchcode
2032   );
Comment 12 Jonathan Druart 2013-07-15 07:29:14 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-07-15 12:29:29 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2013-08-05 22:46:29 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2013-08-05 22:46:46 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2013-08-05 22:46:58 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2013-08-05 22:47:12 UTC Comment hidden (obsolete)
Comment 18 Galen Charlton 2013-08-06 15:49:10 UTC Comment hidden (obsolete)
Comment 19 Galen Charlton 2013-08-06 15:49:20 UTC Comment hidden (obsolete)
Comment 20 Galen Charlton 2013-08-06 15:49:33 UTC Comment hidden (obsolete)
Comment 21 Galen Charlton 2013-08-06 15:49:42 UTC Comment hidden (obsolete)
Comment 22 Galen Charlton 2013-08-06 15:49:51 UTC Comment hidden (obsolete)
Comment 23 Galen Charlton 2013-08-06 15:49:57 UTC Comment hidden (obsolete)
Comment 24 Galen Charlton 2013-08-06 15:51:21 UTC
I ended up making two follow-ups that add two major and several minor improvements.  Setting to 'Signed Off' and requesting QA of the follow-ups.
Comment 25 Jonathan Druart 2013-08-13 12:06:22 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2013-08-13 12:06:29 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2013-08-13 12:06:35 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2013-08-13 12:06:41 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2013-09-14 13:36:28 UTC
Started to look at this, all tests pass and nothing found in code review so far :)
Comment 30 Katrin Fischer 2013-09-28 11:27:22 UTC
Back to this, sorry it took me so long.
Comment 31 Katrin Fischer 2013-09-29 13:04:34 UTC
I have started testing this, but ran into something that doesn't seem right:

Testing notes:
./delete_patrons.pl
  Gives a helpful message about the use of the script.
./delete_patrons.pl -h
  Outputs useful information about the use of the script
  and its various options.
./delete_patrons.pl --category_code ST --library CPL
  Gives the correct results in numbers and deletion was done
  properly.
  
Testing various conditions where a delete should not occur:
- Patron has checkouts
  Patron is not in list of patrons to delete (x patrons to delete)
- Patron has fines
  Patron is still in list of patrons to delete (x patrons to delete)
  Patron is deleted, script gives no success message.
./delete_patrons.pl --category_code ST --library CPL -c
1 patrons to delete

When you check the database, the patron has been deleted (including the move to deletedborrowers), although he owed money to the library. (I used a MANUAL_INV of copier fees here). I verified that deleting patrons with fines is not possible in the interface.
Comment 32 Katrin Fischer 2013-09-29 13:06:24 UTC
Also, I think there should be feedback about when the script is done? Even if it was just:
x patrons deleted.
Comment 33 Jonathan Druart 2013-09-30 12:16:00 UTC Comment hidden (obsolete)
Comment 34 I'm just a bot 2013-10-01 18:41:51 UTC
Patch applied cleanly, go forth and signoff
Comment 35 Bernardo Gonzalez Kriegel 2013-10-03 14:13:59 UTC Comment hidden (obsolete)
Comment 36 Bernardo Gonzalez Kriegel 2013-10-03 14:14:50 UTC Comment hidden (obsolete)
Comment 37 Bernardo Gonzalez Kriegel 2013-10-03 14:18:51 UTC
sorry, made a mistake, re-uploading patches
Comment 38 Bernardo Gonzalez Kriegel 2013-10-03 14:22:09 UTC Comment hidden (obsolete)
Comment 39 Bernardo Gonzalez Kriegel 2013-10-03 14:22:22 UTC Comment hidden (obsolete)
Comment 40 Katrin Fischer 2013-10-03 16:00:28 UTC
Created attachment 21780 [details] [review]
[PASSED QA] Bug 10419: new cronjob for deleting patrons

This patch provides a cronjob script for deleting patrons.

It takes 3 options:

--not_borrowed_since
--expired_date
--category_code

See the perldoc misc/cronjobs/delete_patrons.pl.
Use the -v flag for a verbose mode.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

http://bugs.koha-community.org/show_bug.cgi?id=10219
Comment 41 Katrin Fischer 2013-10-03 16:00:43 UTC
Created attachment 21781 [details] [review]
[PASSED QA] Bug 10419: (followup) - tidy up cronjob for deleting patrons

This followup adds:
- execute flag (+x) for the cronjob script.
- replaces --dry-run with --confirm (according with existing scripts).
- changes output text (remove 'first person' style).
- updates the doc and simplifies the dates parameters.
- changes flags PrintError and RaiseError for the dbh in order to catch
  something if an error occurs...

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 42 Katrin Fischer 2013-10-03 16:00:52 UTC
Created attachment 21782 [details] [review]
[PASSED QA] Bug 10419: add branchcode parameter to patron deletion script

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 43 Katrin Fischer 2013-10-03 16:01:00 UTC
Created attachment 21783 [details] [review]
[PASSED QA] Bug 10419: (followup) fix license statement

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 44 Katrin Fischer 2013-10-03 16:01:10 UTC
Created attachment 21784 [details] [review]
[PASSED QA] Bug 10419: (follow-up) tidy names of command-line switches

[1] Use --library instead of --branchcode to be consistent
    with other scripts and user-facing Koha terminology.
[2] Use --not_borrowed_since instead of --not_borrowered_since;
    no need to expose typos in the API to the user.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 45 Katrin Fischer 2013-10-03 16:01:18 UTC
Created attachment 21785 [details] [review]
[PASSED QA] Bug 10419: (follow-up) functional improvements

[1] Patron deletion now happens atomically; if one part
    of the process fails, the record isn't left in a
    partially deleted state.
[2] The routine for handling lists properly during patron
    deletion is now invoked.
[3] The script now prints an indication if it's run
    without --confirm; otherwise, one might think that
    patron records were actually being deleted.
[4] --verbose now actually does something.

Without --verbose, the script will print the dry-run
warning (if applicable), the number of patrons to be
deleted, and error messages.

With --verbose, the script will also print a line with
the borrowernumber of each patron to be deleted.

To test:

[1] Run the script with and without --verbose and compare
    the, well, verbosity.
[2] Run the script without --confirm and note that the script
    prints a message saying that it's running in dry-run mode.
[3] Use the script to try to delete one or more patrons that have
    loans.  Confirm that error messages are printed reporting
    foreign constraints preventing the deletion.  Also confirm that
    no new rows are added to deletedborrowers for those patrons that
    could not be completely deleted.
[4] Use the script to delete a patron that has a public list.  Verify
    that after the deletion, the public list still exists but now
    has a null owner.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 46 Katrin Fischer 2013-10-03 16:01:27 UTC
Created attachment 21786 [details] [review]
[PASSED QA] Bug 10419: FIX hardcoded records and pod

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Missing a sign. Removes harcoded values

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 47 Katrin Fischer 2013-10-03 16:01:39 UTC
Created attachment 21787 [details] [review]
[PASSED QA] Bug 10419: Patrons with fines should not be deleted

Test plan:
- add a fine for a patron
- verify the script does not delete this patron.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Works, script do not delete a patron with fines.
No koha-qa errors with all patches applied

Test
1) Added a fine to a patron
2) run script
3) reports condition
Trying to delete patron 5... Failed to delete patron 5: patron has 10.00 in fines
4) Patron is not deleted

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

Tested various combinations of options:
./delete_patrons.pl
  Gives a helpful message about the use of the script.
./delete_patrons.pl -h
  Outputs useful information about the use of the script
  and its various options.
./delete_patrons.pl --category_code ST --library CPL
  Gives the correct results in numbers and deletion was done
  properly.

Also tested:
  --expired_before
  --not_borrowed_since
  -v

Testing various conditions where a delete should not occur:
- Patron has checkouts
  Patron is not in list of patrons to delete (x patrons to delete)
- Patron has fines
  Patron is still in list of patrons to delete (x patrons to delete)

Checked deleted patrons had been moved to deletedborrowers.

Notes about possible enhancements:
- only print the success message 'x patrons deleted' when confirm
  flag was set
- patrons with current checkouts are silently excluded from the number
  of patrons to be deleted. Printing the number with current checkouts
  might be helpful.

Changes made:
  Fixed a small error in the documentation: expired_date is expired_before.
Comment 48 Galen Charlton 2013-10-03 22:14:15 UTC
Pushed to master.  Thanks, Jonathan!