From 5adcbe8fb6d7a2bbf8893a692db9eeacbe8deca6 Mon Sep 17 00:00:00 2001 From: Barton Chittenden Date: Fri, 25 Sep 2015 13:04:55 -0700 Subject: [PATCH] Bug 14292: QA Follow-up Make borrower category handling more user friendly: Make --category and --skip-category mutally exclusive Check that categories specified by --category and --skip-category exist in categories table. Add --list-categories opttion to show available borrower categories. --- misc/cronjobs/longoverdue.pl | 111 ++++++++++++++++++++++++++---------------- 1 file changed, 68 insertions(+), 43 deletions(-) diff --git a/misc/cronjobs/longoverdue.pl b/misc/cronjobs/longoverdue.pl index 8bc1518..7a44b1e 100755 --- a/misc/cronjobs/longoverdue.pl +++ b/misc/cronjobs/longoverdue.pl @@ -39,6 +39,7 @@ use C4::Circulation qw/LostItem/; use Getopt::Long; use C4::Log; use Pod::Usage; +use Koha::Borrowers; my $lost; # key=lost value, value=num days. my ($charge, $verbose, $confirm, $quiet); @@ -48,37 +49,56 @@ my $borrower_category = []; my $skip_borrower_category = []; my $help=0; my $man=0; +my $list_categories = 0; GetOptions( 'lost=s%' => \$lost, 'c|charge=s' => \$charge, 'confirm' => \$confirm, - 'verbose' => \$verbose, + 'v|verbose' => \$verbose, 'quiet' => \$quiet, 'maxdays=s' => \$endrange, 'mark-returned' => \$mark_returned, - 'help' => \$help, + 'h|help' => \$help, 'man|manual' => \$man, 'category=s' => $borrower_category, - 'skip-category=s' => $skip_borrower_category + 'skip-category=s' => $skip_borrower_category, + 'list-categories' => \$list_categories, ); if ( $man ) { - pod2usage( -verbose => 2 ); - exit; + pod2usage( -verbose => 2 + -exitval => 0 + ); } if ( $help ) { - pod2usage(1); - exit; + pod2usage( -verbose => 1, + -exitval => 0 + ); +} + +if ( scalar @$borrower_category && scalar @$skip_borrower_category) { + pod2usage( -verbose => 1, + -message => "The options --category and --skip-category are muually exclusive.\n" + . "Use one or the other.", + -exitval => 1 + ); +} + +if ( $list_categories ) { + my @categories = sort map { uc $_->[0] } @{ C4::Context->dbh->selectall_arrayref(q|SELECT categorycode FROM categories|) }; + print "\nBorrowrer Categories: " . join( " ", @categories ) . "\n\n"; + exit 0; } =head1 SYNOPSIS - longoverdue.pl [ --help | -h | --man ] + longoverdue.pl [ --help | -h | --man | --list-categories ] longoverdue.pl --lost | -l DAYS=LOST_CODE [ --charge | -c CHARGE_CODE ] [ --verbose | -v ] [ --quiet ] - [ --maxdays MAX_DAYS ] [ --mark-returned ] [ --category BORROWER_CATEGOERY ] ... - [ --skip-category BORROWER_CATEGOERY ] ... [ --commit ] + [ --maxdays MAX_DAYS ] [ --mark-returned ] [ --category BORROWER_CATEGORY ] ... + [ --skip-category BORROWER_CATEGOERY ] ... + [ --commit ] WARNING: Flippant use of this script could set all or most of the items in your catalog to Lost and charge your @@ -125,11 +145,20 @@ When an item is marked lost, remove it from the borrowers issued items. =item B<--category> -Act on the listed borrower category code (borrowers.categorycode). Exclude all others. This may be specified multiple times to include multiple categories. +Act on the listed borrower category code (borrowers.categorycode). +Exclude all others. This may be specified multiple times to include multiple categories. +May not be used with B<--skip-category> =item B<--skip-category> -Act on all available borrower category codes, except those listed. This may be specified multiple times, to exclude multiple categories. +Act on all available borrower category codes, except those listed. +This may be specified multiple times, to exclude multiple categories. +May not be used with B<--category> + +=item B<--list-categories> + +List borrower categories available for use by B<--category> or +B<--skip-category>, and exit. =item B<--help | -h> @@ -141,9 +170,6 @@ Display entire manual and exit. =back -All 'category' options will be applied before any 'skip-category' options, meaning that the only categories available to skip -are those which have already been specified on the command line. - =cut =head1 Description @@ -184,8 +210,10 @@ if ( ! defined($lost) ) { $lost->{$longoverdue_days} = $longoverdue_value; } else { - pod2usage(1); - die "ERROR: No --lost (-l) option defined"; + pod2usage( { + -exitval => 1, + -msg => q|ERROR: No --lost (-l) option defined|, + } ); } } if ( ! defined($charge) ) { @@ -223,21 +251,25 @@ sub longoverdue_sth { return C4::Context->dbh->prepare($query); } -# The following two functions can and should replaced by a call to -# Koha::Database. -sub borrower_categories_sth { - my $query = "select distinct categorycode from categories"; - return C4::Context->dbh->prepare($query); -} - -sub defined_borrower_categories { - my $sth = borrower_categories_sth(); - my @categories = (); - $sth->execute(); - CATEGORY: while ( my $row = $sth->fetchrow_hashref ) { - push @categories, uc( $row->{categorycode} ); +sub check_user_categories { + + my $categories = shift; + my $test_categories = shift; + my $category_label = shift; + my %good = map { $_ => 1 } @$categories; + my @bad_categories = grep { ! $good{ uc $_ } } @$test_categories; + + if ( scalar @bad_categories ) { + my $error = "$category_label: category/categories '" + . join( ' ', @bad_categories ) + . "' not in the available borrower cateogories: [ " + . join( ' ', @$categories ) + . " ]"; + pod2usage( '-verbose' => 1 + , '-exitval' => 1 + , '-message' => $error + ); } - return @categories; } # returns a hash of borrower category codes to test for long-overdue-ness. @@ -248,32 +280,25 @@ sub get_borrower_categories { return () unless ( scalar @$borrower_category || scalar @$skip_borrower_category ); - my @categories = defined_borrower_categories(); + my $dbh = C4::Context->dbh; + my @categories = map { uc $_->[0] } @{ $dbh->selectall_arrayref(q|SELECT categorycode FROM categories|) }; if( @$borrower_category ) { + check_user_categories( \@categories, $borrower_category, "--category" ); my %borcat = map { ( uc($_) => 1 ) } @$borrower_category; @categories = grep { $borcat{$_} } @categories; } if( @$skip_borrower_category ) { + check_user_categories( \@categories, $skip_borrower_category, "--skip-category" ); my %skip_borcat = map { ( uc($_) => 1 ) } @$skip_borrower_category; @categories = grep { ! $skip_borcat{$_} } @categories; } return map { $_ => 1 } @categories; } -# TODO: category handling should be more user friendly: -# * Warn if a category specified using --category or -# --skip-category is not in the categories table -# * Add --list-categories argument so that you don't -# need to bring up the staff client or koha-mysql to -# find the valid borrower categories. - my %category_to_process = get_borrower_categories( $borrower_category, $skip_borrower_category ); my $filter_borrower_categories = ( scalar @$borrower_category || scalar @$skip_borrower_category ); -#FIXME - Should add a 'system' user and get suitable userenv for it for logging, etc. - my $count; -# my @ranges = map { my @report; my $total = 0; my $i = 0; @@ -295,7 +320,7 @@ foreach my $startrange (sort keys %$lost) { $count=0; ITEM: while (my $row=$sth_items->fetchrow_hashref) { if( $filter_borrower_categories ) { - my $category = uc Koha::Database->new()->schema->resultset('Borrower')->find( $row->{borrowernumber} )->get_column('categorycode'); + my $category = uc Koha::Borrowers->find( $row->{borrowernumber} )->categorycode(); next ITEM unless ( $category_to_process{ $category } ); } printf ("Due %s: item %5s from borrower %5s to lost: %s\n", $row->{date_due}, $row->{itemnumber}, $row->{borrowernumber}, $lostvalue) if($verbose); -- 1.7.10.4