Intention of development: To create a script to delete items based on a series of criteria, primarily itemlost, timestamp, itemlost_on, and withdrawn delete_items.pl --criteria EXPRESSION [--commit] Where EXPRESSION is a valid SQL conditional, operating on the items table, e.g. items.itemlost => 1 or items.timestamp < 2014-06-07 --criteria may be called multiple times. --commit is necessary to actually delete the items. Here are a few examples of how this might be called: delete_items.pl --verbose --criteria "items.itemlost >= 1" --criteria "items.itemlost <= 4" --criteria "items.itemlost_on < '$(date --date="13 month ago" --rfc-3339=date)'" --commit delete_items.pl --verbose --criteria "items.itemlost >= 1" --criteria "items.itemlost <= 4" --criteria "items.timestamp < '$(date --date="13 month ago" --rfc-3339=date)'" --commit delete_items.pl --verbose --criteria "items.withdrawn != 0" --criteria "items.timestamp < '$(date --date="13 month ago" --rfc-3339=date)'" --commit delete_items.pl --verbose --criteria "items.withdrawn != 0" --criteria "items.itemlost_on < '$(date --date="13 month ago" --rfc-3339=date)'" --commit
NAME delete_items.pl - A batch item deletion tool, which generates a query against the items database and deletes the items matching the criteria specified in the command line arguments. SYNOPSIS delete_items.pl [--help|--manual|--version] delete_items.pl [--verbose] [--dry-run] --criteria "*SQL CONDITIONAL EXPRESSION*" ... [--commit] OPTIONS --help Show the brief help information. --manual Read the manual, with examples. --version Show the version number and exit. --verbose Send the "WHERE" clause generated by the collected "--criteria" arguments, as well as items affected to Standard Out. --criteria The "--criteria" option may called multiple times. The following argument must be a syntactically valid SQL statement which is part of the "WHERE" clause querying the items table. These are joined by "AND". --commit No items will be deleted unless the "--commit" flag is present. --dry-run Disables "--commit" flag and enables "--verbose" flag. EXAMPLES The following is an example of this script: delete_items.pl --criteria "items.withdrawn ! 0" --criteria "items.withdrawn_on < $(date --date="13 month ago" --rfc-3339=date)" --commit delete_items.pl --criteria "itemlost >= '1'" --criteria "itemlost <='4'" --criteria "itemlost_on < '2014-04-28'" --commit DESCRIPTION This is a lightweight batch deletion tool for items, suitable for running in a cron job. AUTHOR Barton Chittenden <barton@bywatersolutions.com>
Created attachment 40830 [details] [review] add delete_items.pl: a command line batch deletion tool
Test plan: TESTING THE MAIN FUNCTIONALITY OF THE SCRIPT: ============================================= Create items with various values of itemlost and, withdrawn. Back-date the respective timestamps (itemlost_on, timestamp). Check out one or two of these items (this will exercise the safeguards against deleting checked out items). run delete_items.pl --criteria "items.itemlost = <your lost value here>" --criteria "itemlost_on < <one day after items was lost>" --dry-run do the same using items.withdrawn and items.timestamp. The results of the queries will be written to STDOUT. If you are working in an instance that has lost or withdrawn items that you want to keep, you may want to adjust the itemlost_on or timestamp values of your test items accordingly. Once satisfied with the output using the '--dry-run' option, use '--commit' instead, then verifiy that the items have been deleted. run delete_items.pl on checked-out items. If you are running '--verbose' or '--dry-run', you should receive a message to stdout showing that the item was not deleted. OTHER THINGS TO TEST: ===================== Run the script with the following options. Check the spelling and accuracy on --help and --manual, and make sure that the output from --version contains a version number. --help Show the brief help information. --manual Read the manual, with examples. --version Show the version number and exit.
Created attachment 42978 [details] [review] [SIGNED-OFF] Bug 14504: Add delete_items.pl: a command line batch deletion tool http://bugs.koha-community.org/show_bug.cgi?id=14504 Signed-off-by: Heather Braum <hbraum@nekls.org>
Comment on attachment 42978 [details] [review] [SIGNED-OFF] Bug 14504: Add delete_items.pl: a command line batch deletion tool Review of attachment 42978 [details] [review]: ----------------------------------------------------------------- FAIL misc/cronjobs/delete_items.pl FAIL pod *** WARNING: Verbatim paragraph in NAME section in file misc/cronjobs/delete_items.pl ::: misc/cronjobs/delete_items.pl @@ +1,4 @@ > +#! /usr/bin/perl > + > +use warnings; > +use strict; warnings and strict are useless when Modern::Perl is already used. @@ +8,5 @@ > +use C4::Circulation; > +use Modern::Perl; > +use Pod::Usage; > + > +my $VERSION='1.0'; How this is useful? We usually don't use it in scripts. @@ +30,5 @@ > + , help => '' > + , manual => '' > + , version => '' > + } > +}; I am not really in favor of these 2 variables. IMO it is preferable to stick to the structure of the already existing scripts in misc/* @@ +39,5 @@ > + , 'V|version' => sub { $OPTIONS->{flags}->{version} = 1 } > + , 'h|help' => sub { $OPTIONS->{flags}->{help} = 1 } > + , 'm|manual' => sub { $OPTIONS->{flags}->{manual} = 1 } > + , 'c|commit' => sub { $OPTIONS->{flags}->{commit} = 1 } # aka DO-EET! > + , 'dry-run' => sub { $OPTIONS->{flags}->{commit} = 0; I don't think dry-run is useful, it's in dry-run mode if commit is not given. @@ +51,5 @@ > + exit; > +} > + > +pod2usage( -verbose => 2 ) if $OPTIONS->{flags}->{manual}; > +pod2usage(1) if ( $OPTIONS->{flags}->{help} || scalar @criteria == 0 ); The script should not return 1 if help is specified. It would be good to display on error for the @criteria==0 case (see msg option of pod2usage). @@ +65,5 @@ > +my $where_clause = ' where ' . join ( " and ", @criteria ); > + > +verbose "Where statement: $where_clause"; > + > +$GLOBAL->{sth}->{target_tiems} = $dbh->prepare( $query->{target_items} . $where_clause ); typo tiems vs items, I suppose. @@ +71,5 @@ > + > +DELITEM: while ( my $item = $GLOBAL->{sth}->{target_tiems}->fetchrow_hashref() ) { > + my $issue = GetOpenIssue( $item->{itemnumber} ); > + if( defined $issue ) { > + verbose "Cannot delete '$item->{itemnumber}' -- item is checked out." Shouldn't we also search for holds?
Created attachment 43445 [details] [review] Bug 14504 [QA Follow-up] * Fix POD warning. * Remove redundant 'use stric' and 'use warnings' * Remove $VERSION and --version option. * Remove --dry-run option * Split test for --help and check for @criteria into two separate pod2usage calls, enabling -msg on the latter. * Fix 'target_tiems' typo. * Test for holds on items to be deleted.
Barton, it does not work: the items are never deleted. You should also launch the qa script every time you submit a patch, it will catch warnings/errors for you (here 2 trailing white-spaces).
Created attachment 43533 [details] [review] Bug 14504 [QA Follow-up] * Fix POD warning. * Remove redundant 'use stric' and 'use warnings' * Remove $VERSION and --version option. * Remove --dry-run option * Split test for --help and check for @criteria into two separate pod2usage calls, enabling -msg on the latter. * Fix 'target_tiems' typo. * Test for holds on items to be deleted. * Fix whitespace * Fix test for holds.
Barton, Actually I think you should reuse the C4::Items::DelItemCheck subroutine, which does exactly what you are doing.
(In reply to Jonathan Druart from comment #9) > Barton, > Actually I think you should reuse the C4::Items::DelItemCheck subroutine, > which does exactly what you are doing. Jonathan, C4::Items::DelItemCheck doesn't have any kind of 'commit' check... I'm torn, because I would *like* to use it to avoid duplication of code (plus there are a number of cases that it covers that I don't), but I don't want to sacrifice the ability to accurately report what's going to happen if I run the script in verbose mode without '--commit'.
(In reply to Barton Chittenden from comment #10) > (In reply to Jonathan Druart from comment #9) > > Barton, > > Actually I think you should reuse the C4::Items::DelItemCheck subroutine, > > which does exactly what you are doing. > > Jonathan, > > C4::Items::DelItemCheck doesn't have any kind of 'commit' check... I'm torn, > because I would *like* to use it to avoid duplication of code (plus there > are a number of cases that it covers that I don't), but I don't want to > sacrifice the ability to accurately report what's going to happen if I run > the script in verbose mode without '--commit'. Jonathan, Jesse Weaver (pianohacker) and I discussed this: 18:15 <@pianohacker> barton: I'd suggest to Joubu to either add a 'check_only' kind of parameter to DelItemCheck or move the check logic into a completely different function like ItemSafeToDelete (with a slight personal preference for the latter) 18:18 < barton> cleanest would probably be both -- write ItemSafeToDelete, then have DelItemCheck be a wrapper around DelItem which uses ItemSafeToDelete as its test. 18:18 <@pianohacker> yeah, definitely So I'd like to open a separate bug to * add C4::Items::ItemSafeToDelete() * add a 'do-not-commit' option to DelItemCheck (omitting the flag would make it work as usual) * Use C4::Items::ItemSafeToDelete to do the internal checking for DelItemCheck.
I think an even better solution would be to do the job in a transaction and rollback if the commit flag is not there. So it would be something like: start transaction foreach items: is_deleted = DelItemCheck display error unless is_deleted if commit flag: commit else: rollback What do you think about that?
Created attachment 43788 [details] [review] bug 14504: split logic from DelItemCheck() into ItemSafeToDelete()
Created attachment 43789 [details] [review] bug 14504: use C4::Items::DelItemCheck in delete_items.pl
Jonathan, The call to C4::Context->IsSuperLibrarian() in C4::Items::ItemSafeToDelete (formerly in C4::Items::DelItemCheck) is throwing the following warning: C4::Context->userenv not defined! at /var/lib/koha/nekls2/kohaclone/C4/Items.pm line 2336. I've gone ahead and attached the patches, because everything else works, but thoughts about how to fix the "not_same_branch" code would be appreciated. --Barton
Barton, have you seen my suggestion on comment 12? I think it will be really easy to implement what you want using it (no change in pm and no additional tests).
(In reply to Jonathan Druart from comment #16) > Barton, have you seen my suggestion on comment 12? > I think it will be really easy to implement what you want using it (no > change in pm and no additional tests). While your suggestions would definitely work, I think what Barton's changes are a good and worthwhile improvement. The logic to for deciding if an item is deletable should definitely be separate from the actual deleting of items, and now we've got more through unit tests as well!
(In reply to Kyle M Hall from comment #17) > (In reply to Jonathan Druart from comment #16) > > Barton, have you seen my suggestion on comment 12? > > I think it will be really easy to implement what you want using it (no > > change in pm and no additional tests). > > While your suggestions would definitely work, I think what Barton's changes > are a good and worthwhile improvement. The logic to for deciding if an item > is deletable should definitely be separate from the actual deleting of > items, and now we've got more through unit tests as well! Yes maybe you are right but we are adding 1 new subroutine and lot of code when the 2 start_transaction/rollback lines would make the job. Anyway, if you really want this way, I have still some concerns: 1/ Tests should create their own data (branchcode/categorycode), it is easier to use TestBuilder for that. 2/ I don't think a subroutine should take a do_not_commit parameter, this should not do it in the subroutine but from the scripts. Keeping the signed off status to get feedback from other QAers.
Created attachment 44013 [details] [review] Bug 14504 QA Fixes -- use TestBuilder, remove do_not_commit Use t::lib::TestBuilder in t/db_dependent/Items_DelItemCheck.t Remove the option 'do_not_commit' from C4::Items::DelItemCheck. Whitespace cleanup.
Comment on attachment 44013 [details] [review] Bug 14504 QA Fixes -- use TestBuilder, remove do_not_commit Review of attachment 44013 [details] [review]: ----------------------------------------------------------------- ::: C4/Items.pm @@ +2378,2 @@ > > + DelItemCheck( $dbh, $biblionumber, $itemnumber ); $dbh shouldn't pass as a parameter. @@ +2381,4 @@ > =cut > > sub DelItemCheck { > + my ( $dbh, $biblionumber, $itemnumber ) = @_; $dbh shouldn't pass as a parameter.
140 "DelItemCheck should delete item if 'do_not_commit' not set" => 'do_not_commit' occurrence.
Created attachment 44149 [details] [review] bug 14504: QA Followup -- fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t
Is there any chance of this getting into the next version? Just curious as this would be very beneficial with automating our weeding procedures base on being missing for a set time period. Thanks!
Hi Tom, this patch has missed the feature slush, which means it was not 'passed QA' by a certain date. It won't be included in 3.22.
I have asked Martin to give his opinion.
I'm in agreement with Jonathan that adding subs needlessly is a bad thing and therefore I do tend towards the transaction idea. More and more I'm of the opinion that data integrity lies firmly within the remit of the database. We really aught to get the constraints correct there first and not rely on lots of additional code logic all over the place doing it instead :| It's a hard thing to say, but I would prefer to have some of the foundations cleaned up rather than just adding yet more code to koha to compensate for your poor foundations. Sorry if this sounds really negative, but it's something I've felt for a while and I totally understand the want to get new and exciting features in quicker and quicker but I feel this is really leading to obfuscation and code bloat that becoming unmanageable.. where do we drawer the line? I won't complain if this is pushed.. but I don't think it's the most forward thinking solution I'm afraid. Also, as we've missed feature slush now it may be worth mentioning a few other queries: 1) Why use a dbh over using the dbic objects? (By using C4::Context to get your handle, you are already going through the pain of instantiating all the dbic class objects I believe anyway, so I don't see that using the handle directly here is of any benefit - Jonathan let me know if this is wrong?) 2) Line 928 in the final patch.. appears to have a double $$ (C4/ImportBatch.pm)
s/your poor foundations/our poor foundations/ s/that becoming/that are becoming/
(In reply to Martin Renvoize from comment #26) I think I must disagree on this one. I think the logic to determine if an item is safe to delete should definitely be divorced from the actual deletion. I think these changes along with the provided unit tests are a good improvement! I guess I don't see how using transactions in this situation is anything but a band-aid so to speak. If you think I've missed your point, please let me know!
Created attachment 45789 [details] [review] bug 14504: QA Followup -- fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t Fixed doubled "$$" in C4/ImportBatch.pm
These patches don't apply anymore. There are too much changes and too many patches, I have suggested a very simple solution months ago, now we have patches modifying 10 different files...
To add my own 2 cents: The primary reason that these patches modify 10 different files is that it was asked, as part of the QA process, that the $dbh parameter be removed from DelItemCheck. (Which requires that all of the connecting files be modified.) While the alternate solution is reasonable, it is not an existing coding pattern in Koha, and is none of this is in the coding guidelines. Barton and I will be working on rebasing the patches, but please keep the above in mind.
Created attachment 49648 [details] [review] Bug 14504: Add delete_items.pl: a command line batch deletion tool http://bugs.koha-community.org/show_bug.cgi?id=14504 Signed-off-by: Heather Braum <hbraum@nekls.org>
Created attachment 49649 [details] [review] Bug 14504: (QA followup) * Fix POD warning. * Remove redundant 'use stric' and 'use warnings' * Remove $VERSION and --version option. * Remove --dry-run option * Split test for --help and check for @criteria into two separate pod2usage calls, enabling -msg on the latter. * Fix 'target_tiems' typo. * Test for holds on items to be deleted. * Fix whitespace * Fix test for holds.
Created attachment 49650 [details] [review] bug 14504: split logic from DelItemCheck() into ItemSafeToDelete()
Created attachment 49651 [details] [review] bug 14504: use C4::Items::DelItemCheck in delete_items.pl
Created attachment 49652 [details] [review] Bug 14504: (QA followup) use TestBuilder, remove do_not_commit Use t::lib::TestBuilder in t/db_dependent/Items_DelItemCheck.t Remove the option 'do_not_commit' from C4::Items::DelItemCheck. Whitespace cleanup.
Created attachment 49653 [details] [review] bug 14504: (QA followup) fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t Fixed doubled "$$" in C4/ImportBatch.pm
Patches rebased. Just noticed that "bug" isn't capitalized in some of the commit messages, whoever commits/pushes should probably fix that. :)
QA comments: 1/ The tests are not executed in a transaction 2/ I get the following warning: Argument "book_on_loan" isn't numeric in numeric eq (==) at misc/cronjobs/delete_items.pl line 62.
Created attachment 52425 [details] [review] Bug 14504: (QA followup) fix test transaction, book_on_loan
Created attachment 53224 [details] [review] Bug 14504: (QA followup) Tidy tests This patch re-introduces the transaction into t/db_dependent/Items_DelItemCheck.t and does some cleaning on the touched tests so they raise less warnings. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 53225 [details] [review] Bug 14504: (QA followup) Fix delete_records_via_leader.pl call to DelItemCheck Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I've tested this patches and found some issues: * Somehow, it is not deleting items: I ran this: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ misc/cronjobs/delete_items.pl --verbose --criteria 'holdingbranch="MPL"' --commit Where statement: where holdingbranch="MPL" Deleting '41' But the item is still there. I'm not sure why :-D MariaDB [koha_kohadev]> SELECT itemnumber,holdingbranch,homebranch FROM items WHERE itemnumber=41; +------------+---------------+------------+ | itemnumber | holdingbranch | homebranch | +------------+---------------+------------+ | 41 | MPL | CPL | +------------+---------------+------------+ 1 row in set (0.00 sec) * t/db_dependent/Items_DelItemCheck.t was missing the transaction, thus leaving cruft on the db (FIXED WITH A FOLLOWUP). This happened at some point when refactoring. * This is an opinionated item, and could be discussed, but I'd rather stick with --where instead of --criteria. I'd sign it if it was left as-is anyway.
Created attachment 54514 [details] [review] Bug 14504: Add delete_items.pl: a command line batch deletion tool http://bugs.koha-community.org/show_bug.cgi?id=14504 Signed-off-by: Heather Braum <hbraum@nekls.org>
Created attachment 54515 [details] [review] Bug 14504: (QA followup) * Fix POD warning. * Remove redundant 'use stric' and 'use warnings' * Remove $VERSION and --version option. * Remove --dry-run option * Split test for --help and check for @criteria into two separate pod2usage calls, enabling -msg on the latter. * Fix 'target_tiems' typo. * Test for holds on items to be deleted. * Fix whitespace * Fix test for holds.
Created attachment 54516 [details] [review] bug 14504: split logic from DelItemCheck() into ItemSafeToDelete()
Created attachment 54517 [details] [review] bug 14504: use C4::Items::DelItemCheck in delete_items.pl
Created attachment 54518 [details] [review] Bug 14504: (QA followup) use TestBuilder, remove do_not_commit Use t::lib::TestBuilder in t/db_dependent/Items_DelItemCheck.t Remove the option 'do_not_commit' from C4::Items::DelItemCheck. Whitespace cleanup.
Created attachment 54519 [details] [review] bug 14504: (QA followup) fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t Fixed doubled "$$" in C4/ImportBatch.pm
Created attachment 54520 [details] [review] Bug 14504: (QA followup) fix test transaction, book_on_loan
Created attachment 54521 [details] [review] Bug 14504: (QA followup) Tidy tests This patch re-introduces the transaction into t/db_dependent/Items_DelItemCheck.t and does some cleaning on the touched tests so they raise less warnings. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54522 [details] [review] Bug 14504: (QA followup) Fix delete_records_via_leader.pl call to DelItemCheck Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54525 [details] [review] Bug 14504 - Changes missed while fixing patches
Created attachment 54530 [details] [review] Bug 14504: (QA followup) Fix error causing item not to be deleted
Created attachment 54531 [details] [review] Bug 14504: (QA followup) Change argument --criteria to --where
Created attachment 54691 [details] [review] Bug 14504: Add delete_items.pl: a command line batch deletion tool http://bugs.koha-community.org/show_bug.cgi?id=14504 Signed-off-by: Heather Braum <hbraum@nekls.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54692 [details] [review] Bug 14504: (QA followup) * Fix POD warning. * Remove redundant 'use stric' and 'use warnings' * Remove $VERSION and --version option. * Remove --dry-run option * Split test for --help and check for @criteria into two separate pod2usage calls, enabling -msg on the latter. * Fix 'target_tiems' typo. * Test for holds on items to be deleted. * Fix whitespace * Fix test for holds. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54693 [details] [review] bug 14504: split logic from DelItemCheck() into ItemSafeToDelete() Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54694 [details] [review] bug 14504: use C4::Items::DelItemCheck in delete_items.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54695 [details] [review] Bug 14504: (QA followup) use TestBuilder, remove do_not_commit Use t::lib::TestBuilder in t/db_dependent/Items_DelItemCheck.t Remove the option 'do_not_commit' from C4::Items::DelItemCheck. Whitespace cleanup. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54696 [details] [review] bug 14504: (QA followup) fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t Fixed doubled "$$" in C4/ImportBatch.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54697 [details] [review] Bug 14504: (QA followup) fix test transaction, book_on_loan Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54698 [details] [review] Bug 14504: (QA followup) Tidy tests This patch re-introduces the transaction into t/db_dependent/Items_DelItemCheck.t and does some cleaning on the touched tests so they raise less warnings. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54699 [details] [review] Bug 14504: (QA followup) Fix delete_records_via_leader.pl call to DelItemCheck Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54700 [details] [review] Bug 14504: Changes missed while fixing patches Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54701 [details] [review] Bug 14504: (QA followup) Fix error causing item not to be deleted Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54702 [details] [review] Bug 14504: (QA followup) Change argument --criteria to --where Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 54703 [details] [review] Bug 14504: (followup) Make deletion conditional on --commit Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 16.11, thanks Barton!