http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 42978 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42978 [SIGNED-OFF] Bug 14504: Add delete_items.pl: a command line batch deletion tool Review of attachment 42978: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14504&attachment=42978) ----------------------------------------------------------------- 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? -- You are receiving this mail because: You are watching all bug changes.