[Koha-patches] [PATCH] Bug 2881 - longoverdue cronjob fails to implement required functionality.

[Joe Atzberger]

Sorry that's actually bug
2883<http://bugs.koha.org/cgi-bin/bugzilla/show_bug.cgi?id=2883>
.

On Tue, Dec 23, 2008 at 5:58 PM, Joe Atzberger <joe.atzberger at liblime.com>wrote:

> This does not fix all problems recorded in 2881 (see all the FIXME's), but
> it does improve the script's basic feedback to an intelligible level.
> It also adjust the documentation and exapmles to correct bogus usage
> instructions.
> ---
>  misc/cronjobs/longoverdue.pl |  129
> ++++++++++++++++++++++++++++++-----------
>  1 files changed, 94 insertions(+), 35 deletions(-)
>
> diff --git a/misc/cronjobs/longoverdue.pl b/misc/cronjobs/longoverdue.pl
> index 13c0e90..ee0c8e9 100755
> --- a/misc/cronjobs/longoverdue.pl
> +++ b/misc/cronjobs/longoverdue.pl
> @@ -26,6 +26,7 @@ longoverdue.pl  cron script to set lost statuses on
> overdue materials.
>  =cut
>
>  use strict;
> +use warnings;
>  BEGIN {
>     # find Koha's Perl modules
>     # test carefully before changing this
> @@ -39,13 +40,15 @@ use Getopt::Long;
>
>  my  $lost;  #  key=lost value,  value=num days.
>  my ($charge, $verbose, $confirm);
> +my $endrange = 366;  # FIXME hardcoded - don't deal with anything overdue
> by more than this num days.
>
>  GetOptions(
> -            'l|lost=s%'    => \$lost,
> -            'c|charge=s'  => \$charge,
> -            'confirm'  => \$confirm,
> -            'v|verbose'  => \$verbose,
> -       );
> +    'lost=s%'    => \$lost,
> +    'c|charge=s' => \$charge,
> +    'confirm'    => \$confirm,
> +    'verbose'    => \$verbose,
> +);
> +
>  my $usage = << 'ENDUSAGE';
>  longoverdue.pl : This cron script set lost values on overdue items and
> optionally sets charges the patron's account
>  for the item's replacement price.  It is designed to be run as a nightly
> job.  The command line options that globally
> @@ -54,77 +57,133 @@ near-term release, so this script is not intended to
> have a long lifetime.
>
>  This script takes the following parameters :
>
> -    --lost | -l         This option may be used multiple times, and takes
> the form of n=lv ,
> -                        where n is num days overdue, and lv is the lost
> value.
> +    --lost | -l         This option takes the form of n=lv,
> +                        where n is num days overdue, and lv is the lost
> value.  See warning below.
>
>     --charge | -c       This specifies what lost value triggers Koha to
> charge the account for the
>                         lost item.  Replacement costs are not charged if
> this is not specified.
>
>     --verbose | v       verbose.
>
> -    --confirm           confirm.  without -c, this script will report the
> number of affected items and
> +    --confirm           confirm.  without this option, the script will
> report the number of affected items and
>                         return without modifying any records.
>
> -  example :  $PERL5LIB/misc/cronjobs/longoverdue.pl --lost 30=2 --lost
> 60=1 --charge 1
> -    would set LOST= 1  after 30 days, LOST= 2 after 60 days, and charge
> the account when setting LOST= 2 (i.e., 60 days).
> -    This would be suitable for the Koha default LOST authorized values of
> 1 -> 'Lost' and 2 -> 'Long Overdue'
> +  examples :
> +  $PERL5LIB/misc/cronjobs/longoverdue.pl --lost 30=1
> +    Would set LOST=1 after 30 days (up to one year), but not charge the
> account.
> +    This would be suitable for the Koha default LOST authorized value of 1
> -> 'Lost'.
> +
> +  $PERL5LIB/misc/cronjobs/longoverdue.pl --lost 60=2 --charge 1
> +    Would set LOST=2 after 60 days (up to one year), and charge the
> account when setting LOST=2.
> +    This would be suitable for the Koha default LOST authorized value of 2
> -> 'Long Overdue'
>
>  WARNING:  Flippant use of this script could set all or most of the items
> in your catalog to Lost and charge your
>  patrons for them!
>
> +WARNING:  This script is known to be faulty.  It is NOT recommended to use
> multiple --lost options.
> +          See http://bugs.koha.org/cgi-bin/bugzilla/show_bug.cgi?id=2881
> +
>  ENDUSAGE
>
> +# FIXME: We need three pieces of data to operate:
> +#         ~ lower bound (number of days),
> +#         ~ upper bound (number of days),
> +#         ~ new lost value.
> +#        Right now we get only two, causing the endrange hack.  This is a
> design-level failure.
> +# FIXME: do checks on --lost ranges to make sure they are exclusive.
> +# FIXME: do checks on --lost ranges to make sure the authorized values
> exist.
> +# FIXME: do checks on --lost ranges to make sure don't go past endrange.
> +# FIXME: convert to using pod2usage
> +# FIXME: allow --help or -h
> +#
>  if ( ! defined($lost) ) {
>     print $usage;
> -    die;
> +    die "ERROR: No --lost (-l) option defined";
> +}
> +unless ($confirm) {
> +    $verbose = 1;     # If you're not running it for real, then the whole
> point is the print output.
> +    print "### TEST MODE -- NO ACTIONS TAKEN ###\n";
>  }
>
> -my $dbh = C4::Context->dbh();
> +# In my opinion, this line is safe SQL to have outside the API. --atz
> +our $bounds_sth = C4::Context->dbh->prepare("SELECT DATE_SUB(CURDATE(),
> INTERVAL ? DAY)");
>
> -#FIXME - Should add a 'system' user and get suitable userenv for it for
> logging, etc.
> +sub bounds ($) {
> +    $bounds_sth->execute(shift);
> +    return $bounds_sth->fetchrow;
> +}
>
> -my $endrange = 366;  # hardcoded - don't deal with anything overdue by
> more than this num days.
> +# FIXME - This sql should be inside the API.
> +sub longoverdue_sth {
> +    my $query = "
> +    SELECT items.itemnumber, borrowernumber, date_due
> +      FROM issues, items
> +     WHERE items.itemnumber = issues.itemnumber
> +      AND  DATE_SUB(CURDATE(), INTERVAL ? DAY)  > date_due
> +      AND  DATE_SUB(CURDATE(), INTERVAL ? DAY) <= date_due
> +      AND  itemlost <> ?
> +     ORDER BY date_due
> +    ";
> +    return C4::Context->dbh->prepare($query);
> +}
>
> -my @interval = sort keys %$lost;
> +#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;
>
>  # FIXME - The item is only marked returned if you supply --charge .
>  #         We need a better way to handle this.
>  #
> -# FIXME - no sql should be outside the API.
> +my $sth_items = longoverdue_sth();
>
> -my $query = "SELECT items.itemnumber,borrowernumber FROM issues,items
> WHERE items.itemnumber=issues.itemnumber AND
> -        DATE_SUB( CURDATE(), INTERVAL ? DAY) > date_due AND DATE_SUB(
> CURDATE(), INTERVAL ? DAY ) <= date_due AND itemlost <> ? ";
> -my $sth_items = $dbh->prepare($query);
> -while ( my $startrange = shift @interval ) {
> +foreach my $startrange (sort keys %$lost) {
>     if( my $lostvalue = $lost->{$startrange} ) {
> -        #warn "query: $query    \\with\\ params: $startrange,$endrange,
> $lostvalue "if($verbose);
> -        warn "starting range: $startrange - $endrange with lost value
> $lostvalue" if($verbose);
> -        $sth_items->execute( $startrange,$endrange, $lostvalue );
> +        my ($date1) = bounds($startrange);
> +        my ($date2) = bounds(  $endrange);
> +        # print "\nRange ", ++$i, "\nDue $startrange - $endrange days ago
> ($date2 to $date1), lost => $lostvalue\n" if($verbose);
> +        $verbose and
> +            printf "\nRange %s\nDue %3s - %3s days ago (%s to %s), lost =>
> %s\n", ++$i,
> +            $startrange, $endrange, $date2, $date1, $lostvalue;
> +        $sth_items->execute($startrange, $endrange, $lostvalue);
>         $count=0;
>         while (my $row=$sth_items->fetchrow_hashref) {
> -        warn "updating $row->{'itemnumber'} for borrower
> $row->{'borrowernumber'} to lost: $lostvalue" if($verbose);
> +            printf ("Due %s: item %5s from borrower %5s to lost: %s\n",
> $row->{date_due}, $row->{itemnumber}, $row->{borrowernumber}, $lostvalue)
> if($verbose);
>             if($confirm) {
>                 ModItem({ itemlost => $lostvalue }, $row->{'biblionumber'},
> $row->{'itemnumber'});
>                 chargelostitem($row->{'itemnumber'}) if( $charge && $charge
> eq $lostvalue);
>             }
>             $count++;
>         }
> -        push @report, { range => "$startrange - $endrange",
> -                        lostvalue =>  $lostvalue,
> -                        count => $count,
> -                     };
> +        push @report, {
> +           startrange => $startrange,
> +             endrange => $endrange,
> +                range => "$startrange - $endrange",
> +                date1 => $date1,
> +                date2 => $date2,
> +            lostvalue => $lostvalue,
> +                count => $count,
> +        };
> +        $total += $count;
>     }
>     $endrange = $startrange;
>  }
> -for my $range (@report) {
> -    for my $var (keys %$range) {
> -        warn "$var :  $range->{$var}";
> +
> +sub summarize ($$) {
> +    my $arg = shift;    # ref to array
> +    my $got_items = shift || 0;     # print "count" line for items
> +    my @report = @$arg or return undef;
> +    my $i = 0;
> +    for my $range (@report) {
> +        printf "\nRange %s\nDue %3s - %3s days ago (%s to %s), lost =>
> %s\n", ++$i,
> +            map {$range->{$_}} qw(startrange endrange date2 date1
> lostvalue);
> +        $got_items and printf "  %4s items\n", $range->{count};
>     }
>  }
>
> -
> -$sth_items->finish;
> -$dbh->disconnect;
> +print "\n### LONGOVERDUE SUMMARY ###";
> +summarize (\@report, 1);
> +print "\nTOTAL: $total items\n";
> --
> 1.5.5.GIT
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20081229/2502747e/attachment-0002.htm>