Created attachment 18677 [details] [review] Bug 10419: Script for deleting patrons This patch provides a cronjob script for deleting patrons. It takes 3 options: -not_borrowered_since -expired_date -category_code See the perldoc misc/cronjobs/delete_patrons.pl use the -v flag for a verbose mode.
Not nit picking, but would be nicer to write eval {}; if ($@) { say "Failed ... $@"; next; } eval {}; if ($@) { say "Failed ... $@"; next; } say OK; Thanks.
Srdjan, yep, you are right! I don't know at all why I wrote that this way, not really readable... :) Thanks for the catch.
Created attachment 18725 [details] [review] Bug 10419: Script for deleting patrons This patch provides a cronjob script for deleting patrons. It takes 3 options: -not_borrowered_since -expired_date -category_code See the perldoc misc/cronjobs/delete_patrons.pl use the -v flag for a verbose mode.
Created attachment 18743 [details] [review] Bug 10419: Script for deleting patrons This patch provides a cronjob script for deleting patrons. It takes 3 options: -not_borrowered_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>
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.
Created attachment 18754 [details] [review] Bug 10419: Followup - Script 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...
Thanks for testing Kyle. I provide a new patch in order to apply your suggestions.
Created attachment 18990 [details] [review] Bug 10419: Followup - Script 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>
Everything looks great! Thanks for following up on my suggestions!
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 );
Created attachment 19635 [details] [review] Bug 10419: Add the branchcode parameter.
Created attachment 19651 [details] [review] Bug 10419: Add the branchcode parameter. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 20112 [details] [review] Bug 10419: Script for deleting patrons This patch provides a cronjob script for deleting patrons. It takes 3 options: -not_borrowered_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>
Created attachment 20113 [details] [review] Bug 10419: Followup - Script 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>
Created attachment 20114 [details] [review] Bug 10419: Add the branchcode parameter. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 20115 [details] [review] Bug 10419 : Fixing license number
Created attachment 20133 [details] [review] 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>
Created attachment 20134 [details] [review] 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>
Created attachment 20135 [details] [review] 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>
Created attachment 20136 [details] [review] Bug 10419: (followup) fix license statement Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 20137 [details] [review] 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>
Created attachment 20138 [details] [review] 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>
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.
Created attachment 20294 [details] [review] Bug 10419: (followup) fix license statement Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 20295 [details] [review] 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>
Created attachment 20296 [details] [review] 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>
Created attachment 20297 [details] [review] Bug 10419: FIX hardcoded records and pod Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Started to look at this, all tests pass and nothing found in code review so far :)
Back to this, sorry it took me so long.
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.
Also, I think there should be feedback about when the script is done? Even if it was just: x patrons deleted.
Created attachment 21658 [details] [review] 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.
Patch applied cleanly, go forth and signoff
Created attachment 21765 [details] [review] [SIGNED-OFF] 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
Created attachment 21766 [details] [review] [SIGNED-OFF] 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
sorry, made a mistake, re-uploading patches
Created attachment 21773 [details] [review] [SIGNED-OFF] 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
Created attachment 21774 [details] [review] [SIGNED-OFF] 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
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
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>
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>
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>
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>
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>
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>
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.
Pushed to master. Thanks, Jonathan!