Bug 14504 - Add command-line script to batch delete items based on data in items table
Summary: Add command-line script to batch delete items based on data in items table
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Barton Chittenden
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18329
  Show dependency treegraph
 
Reported: 2015-07-07 19:15 UTC by Barton Chittenden
Modified: 2017-05-08 02:47 UTC (History)
12 users (show)

See Also:
Change sponsored?: 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
add delete_items.pl: a command line batch deletion tool (4.42 KB, patch)
2015-07-07 20:46 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14504: Add delete_items.pl: a command line batch deletion tool (4.48 KB, patch)
2015-09-30 19:28 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14504 [QA Follow-up] (4.61 KB, patch)
2015-10-15 20:41 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14504 [QA Follow-up] (4.64 KB, patch)
2015-10-16 16:38 UTC, Barton Chittenden
Details | Diff | Splinter Review
bug 14504: split logic from DelItemCheck() into ItemSafeToDelete() (8.42 KB, patch)
2015-10-22 21:10 UTC, Barton Chittenden
Details | Diff | Splinter Review
bug 14504: use C4::Items::DelItemCheck in delete_items.pl (1.73 KB, patch)
2015-10-22 21:12 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14504 QA Fixes -- use TestBuilder, remove do_not_commit (9.18 KB, patch)
2015-10-26 17:12 UTC, Barton Chittenden
Details | Diff | Splinter Review
bug 14504: QA Followup -- fixing DelItemCheck arguments (10.80 KB, patch)
2015-10-29 13:17 UTC, Barton Chittenden
Details | Diff | Splinter Review
bug 14504: QA Followup -- fixing DelItemCheck arguments (10.84 KB, patch)
2015-12-17 22:58 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14504: Add delete_items.pl: a command line batch deletion tool (4.47 KB, patch)
2016-03-29 20:05 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 14504: (QA followup) (4.63 KB, patch)
2016-03-29 20:05 UTC, Jesse Weaver
Details | Diff | Splinter Review
bug 14504: split logic from DelItemCheck() into ItemSafeToDelete() (8.36 KB, patch)
2016-03-29 20:05 UTC, Jesse Weaver
Details | Diff | Splinter Review
bug 14504: use C4::Items::DelItemCheck in delete_items.pl (1.72 KB, patch)
2016-03-29 20:06 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 14504: (QA followup) use TestBuilder, remove do_not_commit (8.86 KB, patch)
2016-03-29 20:06 UTC, Jesse Weaver
Details | Diff | Splinter Review
bug 14504: (QA followup) fixing DelItemCheck arguments (10.77 KB, patch)
2016-03-29 20:06 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 14504: (QA followup) fix test transaction, book_on_loan (1.43 KB, patch)
2016-06-15 13:58 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14504: (QA followup) Tidy tests (4.84 KB, patch)
2016-07-08 13:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) Fix delete_records_via_leader.pl call to DelItemCheck (1.07 KB, patch)
2016-07-08 13:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: Add delete_items.pl: a command line batch deletion tool (4.47 KB, patch)
2016-08-16 15:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14504: (QA followup) (4.63 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug 14504: split logic from DelItemCheck() into ItemSafeToDelete() (8.36 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug 14504: use C4::Items::DelItemCheck in delete_items.pl (1.72 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14504: (QA followup) use TestBuilder, remove do_not_commit (8.86 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug 14504: (QA followup) fixing DelItemCheck arguments (10.77 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14504: (QA followup) fix test transaction, book_on_loan (1.42 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14504: (QA followup) Tidy tests (1.07 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14504: (QA followup) Fix delete_records_via_leader.pl call to DelItemCheck (1.07 KB, patch)
2016-08-16 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14504 - Changes missed while fixing patches (4.03 KB, patch)
2016-08-16 16:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14504: (QA followup) Fix error causing item not to be deleted (1.16 KB, patch)
2016-08-16 20:39 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14504: (QA followup) Change argument --criteria to --where (3.32 KB, patch)
2016-08-16 20:39 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14504: Add delete_items.pl: a command line batch deletion tool (4.52 KB, patch)
2016-08-22 13:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) (4.69 KB, patch)
2016-08-22 13:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
bug 14504: split logic from DelItemCheck() into ItemSafeToDelete() (8.41 KB, patch)
2016-08-22 13:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
bug 14504: use C4::Items::DelItemCheck in delete_items.pl (1.77 KB, patch)
2016-08-22 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) use TestBuilder, remove do_not_commit (8.91 KB, patch)
2016-08-22 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
bug 14504: (QA followup) fixing DelItemCheck arguments (10.82 KB, patch)
2016-08-22 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) fix test transaction, book_on_loan (1.47 KB, patch)
2016-08-22 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) Tidy tests (1.07 KB, patch)
2016-08-22 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) Fix delete_records_via_leader.pl call to DelItemCheck (1.07 KB, patch)
2016-08-22 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: Changes missed while fixing patches (4.08 KB, patch)
2016-08-22 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) Fix error causing item not to be deleted (1.21 KB, patch)
2016-08-22 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (QA followup) Change argument --criteria to --where (3.38 KB, patch)
2016-08-22 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14504: (followup) Make deletion conditional on --commit (1.54 KB, patch)
2016-08-22 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2015-07-07 19:15:11 UTC
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
Comment 1 Barton Chittenden 2015-07-07 19:18:59 UTC
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>
Comment 2 Barton Chittenden 2015-07-07 20:46:33 UTC Comment hidden (obsolete)
Comment 3 Barton Chittenden 2015-07-07 21:17:43 UTC
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.
Comment 4 Barton Chittenden 2015-09-30 19:28:08 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-10-01 13:31:58 UTC
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?
Comment 6 Barton Chittenden 2015-10-15 20:41:53 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-10-16 10:23:14 UTC
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).
Comment 8 Barton Chittenden 2015-10-16 16:38:39 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-10-19 10:32:45 UTC
Barton,
Actually I think you should reuse the C4::Items::DelItemCheck subroutine, which does exactly what you are doing.
Comment 10 Barton Chittenden 2015-10-19 21:59:53 UTC
(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'.
Comment 11 Barton Chittenden 2015-10-19 22:28:36 UTC
(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.
Comment 12 Jonathan Druart 2015-10-21 13:58:45 UTC
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?
Comment 13 Barton Chittenden 2015-10-22 21:10:37 UTC Comment hidden (obsolete)
Comment 14 Barton Chittenden 2015-10-22 21:12:01 UTC Comment hidden (obsolete)
Comment 15 Barton Chittenden 2015-10-22 21:34:19 UTC
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
Comment 16 Jonathan Druart 2015-10-23 08:12:28 UTC
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).
Comment 17 Kyle M Hall 2015-10-23 12:36:49 UTC
(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!
Comment 18 Jonathan Druart 2015-10-26 09:46:28 UTC
(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.
Comment 19 Barton Chittenden 2015-10-26 17:12:24 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-10-27 08:23:48 UTC
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.
Comment 21 Jonathan Druart 2015-10-27 08:27:04 UTC
140     "DelItemCheck should delete item if 'do_not_commit' not set"

=> 'do_not_commit' occurrence.
Comment 22 Barton Chittenden 2015-10-29 13:17:42 UTC Comment hidden (obsolete)
Comment 23 Tom Misilo 2015-11-04 18:42:21 UTC
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!
Comment 24 Katrin Fischer 2015-11-04 22:46:36 UTC
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.
Comment 25 Jonathan Druart 2015-11-05 10:58:09 UTC
I have asked Martin to give his opinion.
Comment 26 Martin Renvoize 2015-11-07 08:30:31 UTC
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)
Comment 27 Martin Renvoize 2015-11-08 19:26:05 UTC
s/your poor foundations/our poor foundations/
s/that becoming/that are becoming/
Comment 28 Kyle M Hall 2015-11-16 14:05:35 UTC
(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!
Comment 29 Barton Chittenden 2015-12-17 22:58:05 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2016-01-19 09:36:52 UTC
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...
Comment 31 Jesse Weaver 2016-03-29 19:20:40 UTC
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.
Comment 32 Jesse Weaver 2016-03-29 20:05:38 UTC Comment hidden (obsolete)
Comment 33 Jesse Weaver 2016-03-29 20:05:49 UTC Comment hidden (obsolete)
Comment 34 Jesse Weaver 2016-03-29 20:05:57 UTC Comment hidden (obsolete)
Comment 35 Jesse Weaver 2016-03-29 20:06:07 UTC Comment hidden (obsolete)
Comment 36 Jesse Weaver 2016-03-29 20:06:15 UTC Comment hidden (obsolete)
Comment 37 Jesse Weaver 2016-03-29 20:06:21 UTC Comment hidden (obsolete)
Comment 38 Jesse Weaver 2016-03-29 20:07:24 UTC
Patches rebased.

Just noticed that "bug" isn't capitalized in some of the commit messages, whoever commits/pushes should probably fix that. :)
Comment 39 Jonathan Druart 2016-04-08 10:18:49 UTC
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.
Comment 40 Barton Chittenden 2016-06-15 13:58:32 UTC Comment hidden (obsolete)
Comment 41 Tomás Cohen Arazi 2016-07-08 13:13:27 UTC Comment hidden (obsolete)
Comment 42 Tomás Cohen Arazi 2016-07-08 13:13:32 UTC Comment hidden (obsolete)
Comment 43 Tomás Cohen Arazi 2016-07-08 13:39:21 UTC
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.
Comment 44 Kyle M Hall 2016-08-16 15:54:54 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2016-08-16 15:55:17 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2016-08-16 15:55:20 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2016-08-16 15:55:23 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2016-08-16 15:55:27 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2016-08-16 15:55:30 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2016-08-16 15:55:34 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2016-08-16 15:55:37 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2016-08-16 15:55:40 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2016-08-16 16:40:42 UTC Comment hidden (obsolete)
Comment 54 Barton Chittenden 2016-08-16 20:39:44 UTC Comment hidden (obsolete)
Comment 55 Barton Chittenden 2016-08-16 20:39:57 UTC Comment hidden (obsolete)
Comment 56 Tomás Cohen Arazi 2016-08-22 13:38:37 UTC
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>
Comment 57 Tomás Cohen Arazi 2016-08-22 13:38:45 UTC
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>
Comment 58 Tomás Cohen Arazi 2016-08-22 13:38:54 UTC
Created attachment 54693 [details] [review]
bug 14504: split logic from DelItemCheck() into ItemSafeToDelete()

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 59 Tomás Cohen Arazi 2016-08-22 13:39:02 UTC
Created attachment 54694 [details] [review]
bug 14504: use C4::Items::DelItemCheck in delete_items.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 60 Tomás Cohen Arazi 2016-08-22 13:39:18 UTC
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>
Comment 61 Tomás Cohen Arazi 2016-08-22 13:39:30 UTC
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>
Comment 62 Tomás Cohen Arazi 2016-08-22 13:39:40 UTC
Created attachment 54697 [details] [review]
Bug 14504: (QA followup) fix test transaction, book_on_loan

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Tomás Cohen Arazi 2016-08-22 13:39:49 UTC
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>
Comment 64 Tomás Cohen Arazi 2016-08-22 13:40:05 UTC
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>
Comment 65 Tomás Cohen Arazi 2016-08-22 13:40:25 UTC
Created attachment 54700 [details] [review]
Bug 14504: Changes missed while fixing patches

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 66 Tomás Cohen Arazi 2016-08-22 13:40:33 UTC
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>
Comment 67 Tomás Cohen Arazi 2016-08-22 13:40:44 UTC
Created attachment 54702 [details] [review]
Bug 14504: (QA followup) Change argument --criteria to --where

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Tomás Cohen Arazi 2016-08-22 13:40:53 UTC
Created attachment 54703 [details] [review]
Bug 14504: (followup) Make deletion conditional on --commit

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Kyle M Hall 2016-08-26 13:08:10 UTC
Pushed to master for 16.11, thanks Barton!