From 2ba8763bb49b76acd8777f766cd1c4f79292515c Mon Sep 17 00:00:00 2001 From: Joe Atzberger Date: Tue, 27 May 2008 18:06:01 -0500 Subject: [PATCH] Fines repair. Make fines2.pl work, give feedback, improve comments and perldoc. Remove $dbh->disconnect statements as counterproductive. Prevent description field from begining with whitespace. Added robust debug elements. Test script behavior with: perl misc/cronjobs/fines2.pl -v and: mysql> select * from accountlines; Signed-off-by: Joshua Ferraro --- C4/Overdues.pm | 160 +++++++++++++++++++----------------- misc/cronjobs/fines2.pl | 175 ++++++++++++++++++++++------------------ 2 files changed, 180 insertions(+), 155 deletions(-) diff --git a/C4/Overdues.pm b/C4/Overdues.pm index cdda769111..27c282df5e 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -25,6 +25,7 @@ use C4::Circulation; use C4::Context; use C4::Accounts; use C4::Log; # logaction +use C4::Debug; use vars qw($VERSION @ISA @EXPORT); @@ -131,14 +132,7 @@ sub Getoverdues { WHERE date_due < now() ORDER BY borrowernumber " ); $sth->execute; - - my @results; - while ( my $data = $sth->fetchrow_hashref ) { - push @results, $data; - } - $sth->finish; - - return \@results; + return $sth->fetchall_arrayref({}); } =head2 checkoverdues @@ -178,8 +172,8 @@ sub checkoverdues { =item CalcFine - ($amount, $chargename, $message, $daycounttotal, $daycount) = - &CalcFine($itemnumber, $categorycode, $branch, $days_overdue, $description, $start_date, $end_date ); + ($amount, $chargename, $daycount, $daycounttotal) = + &CalcFine($item, $categorycode, $branch, $days_overdue, $description, $start_date, $end_date ); Calculates the fine for a book. @@ -190,12 +184,12 @@ members might get a longer grace period between the first and second reminders that a book is overdue). -C<$itemnumber> is the book's item number. +C<$item> is an item object (hashref). -C<$categorycode> is the category code of the patron who currently has +C<$categorycode> is the category code (string) of the patron who currently has the book. -C<$branchcode> is the library whose issuingrules govern this transaction. +C<$branchcode> is the library (string) whose issuingrules govern this transaction. C<$days_overdue> is the number of days elapsed since the book's due date. NOTE: supplying days_overdue is deprecated. @@ -207,34 +201,48 @@ but retain these for backwards-comptibility with extant fines scripts. Fines scripts should just supply the date range over which to calculate the fine. -C<&CalcFine> returns a list of four values: +C<&CalcFine> returns four values: C<$amount> is the fine owed by the patron (see above). C<$chargename> is the chargename field from the applicable record in the categoryitem table, whatever that is. + +C<$daycount> is the number of days between start and end dates, Calendar adjusted (where needed), +minus any applicable grace period. + +C<$daycounttotal> is C<$daycount> without consideration of grace period. + FIXME - What is chargename supposed to be ? -C<$message> is a text message, either "First Notice", "Second Notice", -or "Final Notice". +FIXME: previously attempted to return C<$message> as a text message, either "First Notice", "Second Notice", +or "Final Notice". But CalcFine never defined any value. =cut #' sub CalcFine { my ( $item, $bortype, $branchcode, $difference ,$dues , $start_date, $end_date ) = @_; + $debug and warn sprintf("CalcFine(%s, %s, %s, %s, %s, %s, %s)", + ($item ? '{item}' : 'UNDEF'), + ($bortype || 'UNDEF'), + ($branchcode || 'UNDEF'), + ($difference || 'UNDEF'), + ($dues || 'UNDEF'), + ($start_date ? ($start_date->output('iso') || 'Not a C4::Dates object') : 'UNDEF'), + ( $end_date ? ( $end_date->output('iso') || 'Not a C4::Dates object') : 'UNDEF') + ); my $dbh = C4::Context->dbh; my $amount = 0; - my $printout; my $daystocharge; # get issuingrules (fines part will be used) my $data = C4::Circulation::GetIssuingRule($bortype, $item->{'itemtype'},$branchcode); if($difference) { # if $difference is supplied, the difference has already been calculated, but we still need to adjust for the calendar. # use copy-pasted functions from calendar module. (deprecated -- these functions will be removed from C4::Overdues ). - my $countspecialday=&GetSpecialHolidays($dues,$item->{itemnumber}); - my $countrepeatableday=&GetRepeatableHolidays($dues,$item->{itemnumber},$difference); - my $countalldayclosed = $countspecialday + $countrepeatableday; + my $countspecialday = &GetSpecialHolidays($dues,$item->{itemnumber}); + my $countrepeatableday = &GetRepeatableHolidays($dues,$item->{itemnumber},$difference); + my $countalldayclosed = $countspecialday + $countrepeatableday; $daystocharge = $difference - $countalldayclosed; } else { # if $difference is not supplied, we have C4::Dates objects giving us the date range, and we use the calendar module. @@ -246,15 +254,13 @@ sub CalcFine { } } # correct for grace period. - $daystocharge -= $data->{'firstremind'}; - if ($data->{'chargeperiod'} > 0 && $daystocharge > 0 ) { - $amount = int($daystocharge / $data->{'chargeperiod'}) * $data->{'fine'}; + my $days_minus_grace = $daystocharge - $data->{'firstremind'}; + if ($data->{'chargeperiod'} > 0 && $days_minus_grace > 0 ) { + $amount = int($days_minus_grace / $data->{'chargeperiod'}) * $data->{'fine'}; } else { # a zero (or null) chargeperiod means no charge. } - - # warn "Calc Fine: " . join(", ", ($item->{'itemnumber'}, $bortype, $difference , $data->{'fine'} . " * " . $daycount . " days = \$ " . $amount , "desc: $dues")) ; - return ( $amount, $data->{'chargename'}, $printout ,$daystocharge , $daystocharge + $data->{'firstremind'} ); + return ( $amount, $data->{'chargename'}, $days_minus_grace, $daystocharge); } @@ -429,13 +435,17 @@ accountlines table of the Koha database. =cut -#' -# FIXME - This API doesn't look right: why should the caller have to +# +# Question: Why should the caller have to # specify both the item number and the borrower number? A book can't # be on loan to two different people, so the item number should be # sufficient. +# +# Possible Answer: You might update a fine for a damaged item, *after* it is returned. +# sub UpdateFine { my ( $itemnum, $borrowernumber, $amount, $type, $due ) = @_; + $debug and warn "UpdateFine($itemnum, $borrowernumber, $amount, " . ($type||'""') . ", $due) called"; my $dbh = C4::Context->dbh; # FIXME - What exactly is this query supposed to do? It looks up an # entry in accountlines that matches the given item and borrower @@ -443,45 +453,58 @@ sub UpdateFine { # account type has one of several values, but what does this _mean_? # Does it look up existing fines for this item? # FIXME - What are these various account types? ("FU", "O", "F", "M") + # "L" is LOST item + # "A" is Account Management Fee + # "N" is New Card + # "M" is Sundry + # "O" is Overdue ?? + # "F" is Fine ?? + # "FU" is Fine UPDATE?? + # "Pay" is Payment + # "REF" is Cash Refund my $sth = $dbh->prepare( - "Select * from accountlines where itemnumber=? and - borrowernumber=? and (accounttype='FU' or accounttype='O' or - accounttype='F' or accounttype='M') and description like ?" + "SELECT * FROM accountlines + WHERE itemnumber=? + AND borrowernumber=? + AND accounttype IN ('FU','O','F','M') + AND description like ? " ); $sth->execute( $itemnum, $borrowernumber, "%$due%" ); if ( my $data = $sth->fetchrow_hashref ) { # we're updating an existing fine. - if ( $data->{'amount'} != $amount ) { - + if ( $data->{'amount'} != $amount ) { my $diff = $amount - $data->{'amount'}; my $out = $data->{'amountoutstanding'} + $diff; - my $sth2 = $dbh->prepare( - "UPDATE accountlines SET date=now(), amount=?, - amountoutstanding=?, lastincrement=? , accounttype='FU' WHERE - borrowernumber=? AND itemnumber=? - AND (accounttype='FU' OR accounttype='O') AND description LIKE ?" - ); - $sth2->execute( $amount, $out, $diff , $data->{'borrowernumber'}, - $data->{'itemnumber'}, "%$due%" ); - $sth2->finish; - } - else { - + my $query = " + UPDATE accountlines + SET date=now(), amount=?, amountoutstanding=?, + lastincrement=?, accounttype='FU' + WHERE borrowernumber=? + AND itemnumber=? + AND accounttype IN ('FU','O') + AND description LIKE ? + LIMIT 1 "; + my $sth2 = $dbh->prepare($query); + # FIXME: BOGUS query cannot ensure uniqueness w/ LIKE %x% !!! + # LIMIT 1 added to prevent multiple affected lines + # FIXME: accountlines table needs unique key!! Possibly a combo of borrowernumber and accountline. + # But actually, we should just have a regular autoincrementing PK and forget accountline, + # including the bogus getnextaccountno function (doesn't prevent conflict on simultaneous ops). + # FIXME: Why only 2 account types here? + $debug and print STDERR "UpdateFine query: $query\n" . + "w/ args: $amount, $out, $diff, $data->{'borrowernumber'}, $data->{'itemnumber'}, \"\%$due\%\"\n"; + $sth2->execute($amount, $out, $diff, $data->{'borrowernumber'}, $data->{'itemnumber'}, "%$due%"); + } else { # print "no update needed $data->{'amount'}" } - } - else { - - # I think this else-clause deals with the case where we're adding - # a new fine. + } else { my $sth4 = $dbh->prepare( "SELECT title FROM biblio LEFT JOIN items ON biblio.biblionumber=items.biblionumber WHERE items.itemnumber=?" ); $sth4->execute($itemnum); - my $title = $sth4->fetchrow_hashref; - $sth4->finish; + my $title = $sth4->fetchrow; # # print "not in account"; # my $sth3 = $dbh->prepare("Select max(accountno) from accountlines"); @@ -492,17 +515,14 @@ sub UpdateFine { # $sth3->finish; # $accountno[0]++; # begin transaction - my $nextaccntno = C4::Accounts::getnextacctno($borrowernumber); - my $sth2 = $dbh->prepare( - "INSERT INTO accountlines - (borrowernumber,itemnumber,date,amount, - description,accounttype,amountoutstanding,lastincrement,accountno) VALUES - (?,?,now(),?,?,'FU',?,?,?)" - ); - $sth2->execute( $borrowernumber, $itemnum, $amount, - "$type $title->{'title'} $due", - $amount,$amount, $nextaccntno); - $sth2->finish; + my $nextaccntno = C4::Accounts::getnextacctno($borrowernumber); + my $desc = ($type ? "$type " : '') . "$title $due"; # FIXEDME, avoid whitespace prefix on empty $type + my $query = "INSERT INTO accountlines + (borrowernumber,itemnumber,date,amount,description,accounttype,amountoutstanding,lastincrement,accountno) + VALUES (?,?,now(),?,?,'FU',?,?,?)"; + my $sth2 = $dbh->prepare($query); + $debug and print STDERR "UpdateFine query: $query\nw/ args: $borrowernumber, $itemnum, $amount, $desc, $amount, $amount, $nextaccntno\n"; + $sth2->execute($borrowernumber, $itemnum, $amount, $desc, $amount, $amount, $nextaccntno); } # logging action &logaction( @@ -511,8 +531,6 @@ sub UpdateFine { $borrowernumber, "due=".$due." amount=".$amount." itemnumber=".$itemnum ) if C4::Context->preference("FinesLog"); - - $sth->finish; } =item BorType @@ -587,14 +605,10 @@ sub GetFine { my $sth = $dbh->prepare($query); $sth->execute( $itemnum, $borrowernumber ); my $data = $sth->fetchrow_hashref(); - $sth->finish(); - $dbh->disconnect(); return ( $data->{'sum(amountoutstanding)'} ); } - - =item GetIssuingRules FIXME - This sub should be deprecated and removed. @@ -614,6 +628,7 @@ category he or she belongs to. =cut sub GetIssuingRules { + warn "GetIssuingRules is deprecated: use GetIssuingRule from C4::Circulation instead."; my ($itemtype,$categorycode)=@_; my $dbh = C4::Context->dbh(); my $query=qq|SELECT * @@ -624,10 +639,7 @@ sub GetIssuingRules { my $sth = $dbh->prepare($query); # print $query; $sth->execute($itemtype,$categorycode); - my ($data) = $sth->fetchrow_hashref; - $sth->finish; -return ($data); - + return $sth->fetchrow_hashref; } @@ -643,8 +655,6 @@ sub ReplacementCost2 { my $sth = $dbh->prepare($query); $sth->execute( $itemnum, $borrowernumber ); my $data = $sth->fetchrow_hashref(); - $sth->finish(); - $dbh->disconnect(); return ( $data->{'amountoutstanding'} ); } diff --git a/misc/cronjobs/fines2.pl b/misc/cronjobs/fines2.pl index ae4b3b9138..080b2e3dcf 100755 --- a/misc/cronjobs/fines2.pl +++ b/misc/cronjobs/fines2.pl @@ -34,108 +34,123 @@ BEGIN { eval { require "$FindBin::Bin/kohalib.pl" }; } +use Date::Manip; # qw( Date_DaysSince1BC ) ; +use Data::Dumper; +use Getopt::Long; + use C4::Context; use C4::Circulation; -use C4::Overdues; -use Date::Manip; +use C4::Overdues; # qw( Getoverdues CalcFine ); use C4::Biblio; use C4::Items; +use C4::Dates; +use C4::Debug; -open (FILE,'>/tmp/fines') || die; -# FIXME -# it looks like $count is just a counter, would it be -# better to rely on the length of the array @$data and turn the -# for loop below into a foreach loop? -# -my $DEBUG=0; -my ($data)=Getoverdues(); -print scalar(@$data) if $DEBUG; -my $overdueItemsCounted=0 if $DEBUG; - -# FIXME - There's got to be a better way to figure out what day -# today is. -my ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) =localtime(time); -$mon++; -$year=$year+1900; +our $verbose; +GetOptions('verbose+', \$verbose); +$debug and $verbose++; -my $date=Date_DaysSince1BC($mon,$mday,$year); +# my $filename = "/tmp/fines"; +# open (FILE, ">$filename") or die "Cannot write to $filename"; -print $date if $DEBUG; +my ($data)=Getoverdues(); +my $overdueItemsCounted=0; -my $borrowernumber; +# FIXME - There's got to be a better way to figure out what day today is. +my ($mday,$mon,$year) = (localtime)[3..5]; # ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) +my $date = Date_DaysSince1BC($mon+1,$mday,$year+1900); +my $today_obj = C4::Dates->new(); +my $today_iso = $today_obj->output('iso'); -# FIXME -# $total isn't used anywhere else in the file, -# can we delete it? -# -my $total=0; +if ($verbose) { + printf "Number of overdues: %7d\nDate_DaysSince1BC : %7d\n", scalar(@$data), $date; + print "today: $today_iso\n"; +} # get the maxfine parameter my $maxFine=C4::Context->preference("MaxFine") || 999999999; -# FIXME -# This should be rewritten to be a foreach loop -# Also, this loop is really long, and could be better grokked if broken -# into a number of smaller, separate functions -# -for (my $i=0;$i[$i]->{'date_due'}); +our $phone_sth = C4::Context->dbh->prepare("Select * from borrowers where borrowernumber=?"); +sub get_guarantor_phone ($) { + $phone_sth->execute(shift); + my $x = $phone_sth->fetchrow_hashref; + return $x->{'phone'}; +} + +our $fine_sth = C4::Context->dbh->prepare(" + INSERT INTO accountlines + (borrowernumber, itemnumber, accountno, date, + amount, description, accounttype, amountoutstanding) + VALUES (?,?,?,now(),?,?,'L',?) + "); +sub insert_fine ($$$$$$) { + $verbose and print "inserting fine: " . join(", ",@_), "\n"; + return $fine_sth->execute(@_); +} + +foreach (@$data){ + my $date_due = $_->{date_due}; + $verbose and print "date_due: $date_due ", ($date_due le $today_iso ? 'fine!' : 'ok'), "\n"; + my @dates=split('-',$date_due); my $date2=Date_DaysSince1BC($dates[1],$dates[2],$dates[0]); my $due="$dates[2]/$dates[1]/$dates[0]"; - my $borrower=BorType($data->[$i]->{'borrowernumber'}); - if ($date2 <= $date){ - $overdueItemsCounted++ if $DEBUG; - my $difference=$date-$date2; - my ($amount,$type,$printout)= - CalcFine($data->[$i], - $borrower->{'categorycode'}, - $difference); - if ($amount > $maxFine){ - $amount=$maxFine; - } - if ($amount > 0){ - UpdateFine($data->[$i]->{'itemnumber'},$data->[$i]->{'borrowernumber'},$amount,$type,$due); - if ($borrower->{'categorycode'} eq 'C'){ # FIXME - my $dbh = C4::Context->dbh; - my $sth=$dbh->prepare("Select * from borrowers where borrowernumber=?"); - $sth->execute($borrower->{'guarantor'}); - my $tdata=$sth->fetchrow_hashref; - $sth->finish; - $borrower->{'phone'}=$tdata->{'phone'}; - } - print "$printout\t$borrower->{'cardnumber'}\t$borrower->{'categorycode'}\t$borrower->{'firstname'}\t$borrower->{'surname'}\t$data->[$i]->{'date_due'}\t$type\t$difference\t$borrower->{'emailaddress'}\t$borrower->{'phone'}\t$borrower->{'streetaddress'}\t$borrower->{'city'}\t$amount\n" if $DEBUG; - } - if ($difference >= C4::Context->preference("NoReturnSetLost")){ - my $borrower=BorType($data->[$i]->{'borrowernumber'}); - if ($borrower->{'cardnumber'} ne ''){ - my $cost=ReplacementCost($data->[$i]->{'itemnumber'}); - my $dbh = C4::Context->dbh; - my $accountno=C4::Accounts::getnextacctno($data->[$i]->{'borrowernumber'}); - my $item=GetBiblioFromItemNumber($data->[$i]->{'itemnumber'}); - if ($item->{'itemlost'} ne '1' && $item->{'itemlost'} ne '2' ){ - # FIXME this should be a separate function - my $sth=$dbh->prepare("INSERT INTO accountlines - (borrowernumber,itemnumber,accountno,date,amount, - description,accounttype,amountoutstanding) VALUES - (?,?,?,now(),?,?,'L',?)"); - $sth->execute($data->[$i]->{'borrowernumber'},$data->[$i]->{'itemnumber'}, - $accountno,$cost,"Lost item $item->{'title'} $item->{'barcode'} $due",$cost); - $sth->finish; - ModItem({ itemlost => 2 }, undef, $data->[$i]->{'itemnumber'}); - } - } - } - } + my $borrowernumber = $_->{borrowernumber}; + my $itemnumber = $_->{itemnumber}; + my $borrower = BorType($borrowernumber); + ($date_due le $today_iso) or next; # it is valid to string compare ISO dates. + $overdueItemsCounted++ if $verbose; + my $difference=$date-$date2; + my (@calc_returns) = CalcFine( + $_, $borrower->{categorycode}, $_->{homebranch}, + undef, undef, C4::Dates->new($date_due,'iso'), $today_obj + ); + if ($verbose) { + my $dump = Dumper($_); + $dump =~ s/;/,/; + $verbose and print "CalcFine($dump" . + "\t$borrower->{categorycode}, $_->{homebranch},undef,undef,[$date_due],[today]) returns:\n" . Dumper(\@calc_returns), "\n"; + } + my ($amount,$type,$printout) = @calc_returns[0..2]; + # ($amount,$chargename,$daycount,$daycounttotal)=&CalcFine($itemnumber,$categorycode,$branch,$days_overdue,$description, $start_date, $end_date ); + + ($amount > $maxFine) and $amount = $maxFine; + if ($amount > 0) { + UpdateFine($itemnumber,$borrowernumber,$amount,$type,$due); + if ($borrower->{'guarantor'}) { + $borrower->{'phone'} = get_guarantor_phone($borrower->{'guarantor'}) || $borrower->{'phone'}; + } + print "$printout\t$borrower->{'cardnumber'}\t$borrower->{'categorycode'}\t$borrower->{'firstname'}\t$borrower->{'surname'}\t", + "$_->{'date_due'}\t$type\t$difference\t", + "$borrower->{'emailaddress'}\t$borrower->{'phone'}\t$borrower->{'streetaddress'}\t$borrower->{'city'}\t$amount\n" if $verbose; + } + if ($difference >= C4::Context->preference("NoReturnSetLost")){ + my $borrower=BorType($borrowernumber); + if ($borrower->{'cardnumber'} ne ''){ + my $cost = ReplacementCost($itemnumber); + my $item = GetBiblioFromItemNumber($itemnumber); + if ($item->{'itemlost'} ne '1' && $item->{'itemlost'} ne '2' ){ + insert_fine( + $borrowernumber, + $itemnumber, + C4::Accounts::getnextacctno($borrowernumber), + $cost, + "Lost item $item->{'title'} $item->{'barcode'} $due", + $cost + ); + ModItem({ itemlost => 2 }, undef, $itemnumber); + } + } + } } -if ($DEBUG) { +if ($verbose) { my $numOverdueItems=scalar(@$data); print <