From aa4c6ff62fff4f49a4e0e67ae1616d0f04c95275 Mon Sep 17 00:00:00 2001 From: Joe Atzberger Date: Mon, 25 Aug 2008 22:57:01 -0500 Subject: [PATCH] Fines fixes: apparent problems with fines prevent processing. CalcFine returned values that mismatched expectations in fines.pl. fines.pl refactored: added debugging, prevent needless recreation of Calendar objects by storing them in hash by branch. Still outstanding problems with fines, including the output of a field that has no other references in Koha (so is always undef) and the incorrect description of FinesMode. Calendar exported "new" erroneously. I also cleaned up the queries to avoid needlessly compiling additional statement handles. Please test and consider application to 3.0 maintenance. Signed-off-by: Galen Charlton --- C4/Calendar.pm | 139 ++++++++++++++----------------------- C4/Overdues.pm | 29 ++++---- misc/cronjobs/fines.pl | 136 +++++++++++++++++++++--------------- t/lib/KohaTest/Calendar.pm | 2 - 4 files changed, 150 insertions(+), 156 deletions(-) diff --git a/C4/Calendar.pm b/C4/Calendar.pm index a279e2fd8e..8accf2b684 100644 --- a/C4/Calendar.pm +++ b/C4/Calendar.pm @@ -16,13 +16,32 @@ package C4::Calendar; # Suite 330, Boston, MA 02111-1307 USA use strict; -require Exporter; use vars qw($VERSION @EXPORT); +use Carp; use Date::Calc qw( Date_to_Days ); -# set the version for version checking -$VERSION = 3.00; +use C4::Context; + +BEGIN { + # set the version for version checking + $VERSION = 3.01; + require Exporter; + @EXPORT = qw( + &get_week_days_holidays + &get_day_month_holidays + &get_exception_holidays + &get_single_holidays + &insert_week_day_holiday + &insert_day_month_holiday + &insert_single_holiday + &insert_exception_holiday + &delete_holiday + &isHoliday + &addDate + &daysBetween + ); +} =head1 NAME @@ -40,123 +59,73 @@ This package is used to deal with holidays. Through this package, you can set al =over 2 -=cut - -@EXPORT = qw(&new - &change_branchcode - &get_week_days_holidays - &get_day_month_holidays - &get_exception_holidays - &get_single_holidays - &insert_week_day_holiday - &insert_day_month_holiday - &insert_single_holiday - &insert_exception_holiday - &delete_holiday - &isHoliday - &addDate - &daysBetween); - =item new $calendar = C4::Calendar->new(branchcode => $branchcode); -C<$branchcode> Is the branch code wich you want to use calendar. +Each library branch has its own Calendar. +C<$branchcode> specifies which Calendar you want. =cut sub new { my $classname = shift @_; my %options = @_; - - my %hash; - my $self = bless(\%hash, $classname); - + my $self = bless({}, $classname); foreach my $optionName (keys %options) { $self->{lc($optionName)} = $options{$optionName}; } - - $self->_init; - + defined($self->{branchcode}) or croak "No branchcode argument to new. Should be C4::Calendar->new(branchcode => \$branchcode)"; + $self->_init($self->{branchcode}); return $self; } sub _init { my $self = shift @_; - + my $branch = shift; + defined($branch) or die "No branchcode sent to _init"; # must test for defined here and above to allow "" my $dbh = C4::Context->dbh(); - my $week_days_sql = $dbh->prepare( 'SELECT weekday, title, description - FROM repeatable_holidays - WHERE ( branchcode = ? ) - AND (NOT(ISNULL(weekday)))' ); - $week_days_sql->execute( $self->{'branchcode'} ); + my $repeatable = $dbh->prepare( 'SELECT * + FROM repeatable_holidays + WHERE ( branchcode = ? ) + AND (ISNULL(weekday) = ?)' ); + $repeatable->execute($branch,0); my %week_days_holidays; - while (my ($weekday, $title, $description) = $week_days_sql->fetchrow) { - $week_days_holidays{$weekday}{title} = $title; - $week_days_holidays{$weekday}{description} = $description; + while (my $row = $repeatable->fetchrow_hashref) { + my $key = $row->{weekday}; + $week_days_holidays{$key}{title} = $row->{title}; + $week_days_holidays{$key}{description} = $row->{description}; } - $week_days_sql->finish; $self->{'week_days_holidays'} = \%week_days_holidays; - my $day_month_sql = $dbh->prepare( 'SELECT day, month, title, description - FROM repeatable_holidays - WHERE ( branchcode = ? ) - AND ISNULL(weekday)' ); - $day_month_sql->execute( $self->{'branchcode'} ); + $repeatable->execute($branch,1); my %day_month_holidays; - while (my ($day, $month, $title, $description) = $day_month_sql->fetchrow) { - $day_month_holidays{"$month/$day"}{title} = $title; - $day_month_holidays{"$month/$day"}{description} = $description; + while (my $row = $repeatable->fetchrow_hashref) { + my $key = $row->{month} . "/" . $row->{day}; + $day_month_holidays{$key}{title} = $row->{title}; + $day_month_holidays{$key}{description} = $row->{description} } - $day_month_sql->finish; $self->{'day_month_holidays'} = \%day_month_holidays; - my $exception_holidays_sql = $dbh->prepare( 'SELECT day, month, year, title, description - FROM special_holidays - WHERE ( branchcode = ? ) - AnD (isexception = 1)' ); - $exception_holidays_sql->execute( $self->{'branchcode'} ); + my $special = $dbh->prepare( 'SELECT day, month, year, title, description + FROM special_holidays + WHERE ( branchcode = ? ) + AND (isexception = ?)' ); + $special->execute($branch,1); my %exception_holidays; - while (my ($day, $month, $year, $title, $description) = $exception_holidays_sql->fetchrow) { + while (my ($day, $month, $year, $title, $description) = $special->fetchrow) { $exception_holidays{"$year/$month/$day"}{title} = $title; $exception_holidays{"$year/$month/$day"}{description} = $description; } - $exception_holidays_sql->finish; $self->{'exception_holidays'} = \%exception_holidays; - my $holidays_sql = $dbh->prepare( 'SELECT day, month, year, title, description - FROM special_holidays - WHERE ( branchcode = ? ) - AND (isexception = 0)' ); - $holidays_sql->execute( $self->{'branchcode'} ); + $special->execute($branch,0); my %single_holidays; - while (my ($day, $month, $year, $title, $description) = $holidays_sql->fetchrow) { + while (my ($day, $month, $year, $title, $description) = $special->fetchrow) { $single_holidays{"$year/$month/$day"}{title} = $title; $single_holidays{"$year/$month/$day"}{description} = $description; } - $holidays_sql->finish; $self->{'single_holidays'} = \%single_holidays; -} - -=item change_branchcode - - $calendar->change_branchcode(branchcode => $branchcode) - -Change the calendar branch code. This means to change the holidays structure. - -C<$branchcode> Is the branch code wich you want to use calendar. - -=cut - -sub change_branchcode { - my ($self, $branchcode) = @_; - my %options = @_; - - foreach my $optionName (keys %options) { - $self->{lc($optionName)} = $options{$optionName}; - } - $self->_init; - return $self; } @@ -456,10 +425,10 @@ sub isHoliday { $year=$year+0; $day=$day+0; my $weekday = &Date::Calc::Day_of_Week($year, $month, $day) % 7; - my $weekDays = $self->get_week_days_holidays(); - my $dayMonths = $self->get_day_month_holidays(); + my $weekDays = $self->get_week_days_holidays(); + my $dayMonths = $self->get_day_month_holidays(); my $exceptions = $self->get_exception_holidays(); - my $singles = $self->get_single_holidays(); + my $singles = $self->get_single_holidays(); if (defined($exceptions->{"$year/$month/$day"})) { return 0; } else { diff --git a/C4/Overdues.pm b/C4/Overdues.pm index bacd80a722..63b0842c52 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -116,21 +116,22 @@ Koha database. #' sub Getoverdues { my $params = shift; - my $dbh = C4::Context->dbh; my $statement; if ( C4::Context->preference('item-level_itypes') ) { $statement = " -SELECT issues.*,items.itype as itemtype, items.homebranch FROM issues -LEFT JOIN items USING (itemnumber) -WHERE date_due < now() + SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode + FROM issues +LEFT JOIN items USING (itemnumber) + WHERE date_due < now() "; } else { $statement = " -SELECT issues.*,biblioitems.itemtype,items.itype, items.homebranch FROM issues - LEFT JOIN items USING (itemnumber) - LEFT JOIN biblioitems USING (biblioitemnumber) - WHERE date_due < now() + SELECT issues.*, biblioitems.itemtype, items.itype, items.homebranch, items.barcode + FROM issues +LEFT JOIN items USING (itemnumber) +LEFT JOIN biblioitems USING (biblioitemnumber) + WHERE date_due < now() "; } @@ -237,11 +238,10 @@ 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'), + ($item ? '{item}' : 'UNDEF'), ($bortype || 'UNDEF'), ($branchcode || 'UNDEF'), ($difference || 'UNDEF'), @@ -253,7 +253,8 @@ sub CalcFine { my $amount = 0; my $daystocharge; # get issuingrules (fines part will be used) - my $data = C4::Circulation::GetIssuingRule($bortype, $item->{'itemtype'},$branchcode); + $debug and warn sprintf("CalcFine calling GetIssuingRule(%s, %s, %s)", $bortype, $item->{'itemtype'}, $branchcode); + 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 ). @@ -264,7 +265,7 @@ sub CalcFine { } else { # if $difference is not supplied, we have C4::Dates objects giving us the date range, and we use the calendar module. if(C4::Context->preference('finesCalendar') eq 'noFinesWhenClosed') { - my $calendar = C4::Calendar->new( branchcode => $branchcode ); + my $calendar = C4::Calendar->new( branchcode => $branchcode ); $daystocharge = $calendar->daysBetween( $start_date, $end_date ); } else { $daystocharge = Date_to_Days(split('-',$end_date->output('iso'))) - Date_to_Days(split('-',$start_date->output('iso'))); @@ -278,7 +279,9 @@ sub CalcFine { # a zero (or null) chargeperiod means no charge. } $amount = C4::Context->preference('maxFine') if(C4::Context->preference('maxFine') && ( $amount > C4::Context->preference('maxFine'))); - return ( $amount, $data->{'chargename'}, $days_minus_grace, $daystocharge); + $debug and warn sprintf("CalcFine returning (%s, %s, %s, %s)", $amount, $data->{'chargename'}, $days_minus_grace, $daystocharge); + return ($amount, $data->{'chargename'}, $days_minus_grace, $daystocharge); + # FIXME: chargename is NEVER populated anywhere. } diff --git a/misc/cronjobs/fines.pl b/misc/cronjobs/fines.pl index afc4dca791..0e28357db3 100755 --- a/misc/cronjobs/fines.pl +++ b/misc/cronjobs/fines.pl @@ -26,83 +26,107 @@ # Suite 330, Boston, MA 02111-1307 USA +# FIXME: use FinesMode as described or change syspref description use strict; + BEGIN { # find Koha's Perl modules # test carefully before changing this use FindBin; eval { require "$FindBin::Bin/kohalib.pl" }; } + +use Date::Calc qw/Date_to_Days/; + use C4::Context; use C4::Circulation; use C4::Overdues; -use C4::Calendar; -use Date::Calc qw/Date_to_Days/; +use C4::Calendar qw(); # don't need any exports from Calendar use C4::Biblio; +use C4::Debug; # supplying $debug and $cgi_debug + +use vars qw(@borrower_fields @item_fields @other_fields); +use vars qw($fldir $libname $control $mode $delim $dbname $today $today_iso $today_days); +use vars qw($filename $summary); + +CHECK { + @borrower_fields = qw(cardnumber categorycode surname firstname email phone address citystate); + @item_fields = qw(itemnumber barcode date_due); + @other_fields = qw(type days_overdue fine); + $libname = C4::Context->preference('LibraryName'); + $control = C4::Context->preference('CircControl'); + $mode = C4::Context->preference('finesMode'); + $dbname = C4::Context->config('database'); + $delim = "\t"; # ? C4::Context->preference('delimiter') || "\t"; + + $today = C4::Dates->new(); + $today_iso = $today->output('iso'); + $today_days = Date_to_Days(split(/-/,$today_iso)); + $fldir = $ENV{TMPDIR} || "/tmp"; # TODO: use GetOpt + $filename = $dbname; + $filename =~ s/\W//; + $filename = $fldir . '/'. $filename . '_' . $today_iso . ".log"; + $summary = 1; # TODO: use GetOpt +} +INIT { + $debug and print "Each line will contain the following fields:\n", + "From borrowers : ", join(', ', @borrower_fields), "\n", + "From items : ", join(', ', @item_fields), "\n", + "Per overdue: ", join(', ', @other_fields), "\n", + "Delimiter: '$delim'\n"; +} -my $fldir = "/tmp" ; - -my $libname=C4::Context->preference('LibraryName'); -my $dbname= C4::Context->config('database'); - -my $today = C4::Dates->new(); -my $datestr = $today->output('iso'); -my $today_days= Date_to_Days(split(/-/,$today->output('iso'))); -my $filename= $dbname; -$filename =~ s/\W//; -$filename = $fldir . '/'. $filename . $datestr . ".log"; -open (FILE,">$filename") || die "Can't open LOG"; -print FILE "cardnumber\tcategory\tsurname\tfirstname\temail\tphone\taddress\tcitystate\tbarcode\tdate_due\ttype\titemnumber\tdays_overdue\tfine\n"; - - -my $DEBUG =1; - -my $data=Getoverdues(); -my $overdueItemsCounted=0 ; -my $borrowernumber; - -for (my $i=0;$inew($data->[$i]->{'date_due'},'iso'); - my $datedue_days = Date_to_Days(split(/-/,$datedue->output('iso'))); - my $due_str=$datedue->output(); - my $borrower=BorType($data->[$i]->{'borrowernumber'}); - my $branchcode; - if ( C4::Context->preference('CircControl') eq 'ItemHomeLibrary' ) { - $branchcode = $data->[$i]->{'homebranch'}; - } elsif ( C4::Context->preference('CircControl') eq 'PatronLibrary' ) { - $branchcode = $borrower->{'branchcode'}; -} else { - # CircControl must be PickupLibrary. (branchcode comes from issues table here). - $branchcode = $data->[$i]->{'branchcode'}; - } - my $calendar = C4::Calendar->new( branchcode => $branchcode ); - - my $isHoliday = $calendar->isHoliday( split( '/', C4::Dates->new()->output('metric') ) ); +open (FILE, ">$filename") or die "Cannot write file $filename: $!"; +print FILE join $delim, (@borrower_fields, @item_fields, @other_fields); +print FILE "\n"; + +my $data = Getoverdues(); +my $overdueItemsCounted = 0; +my %calendars = (); + +for (my $i=0; $inew($data->[$i]->{'date_due'},'iso'); + my $datedue_days = Date_to_Days(split(/-/,$datedue->output('iso'))); + my $due_str = $datedue->output(); + my $borrower = BorType($data->[$i]->{'borrowernumber'}); + my $branchcode = ($control eq 'ItemHomeLibrary') ? $data->[$i]->{homebranch} : + ($control eq 'PatronLibrary' ) ? $borrower->{branchcode} : + $data->[$i]->{branchcode} ; + # In final case, CircControl must be PickupLibrary. (branchcode comes from issues table here). + my $calendar; + unless (defined ($calendars{$branchcode})) { + $calendars{$branchcode} = C4::Calendar->new(branchcode => $branchcode); + } + $calendar = $calendars{$branchcode}; + my $isHoliday = $calendar->isHoliday(split '/', $today->output('metric')); - if ($datedue_days <= $today_days){ - $overdueItemsCounted++ if $DEBUG; - my $difference=$today_days - $datedue_days; - my ($amount,$type,$printout,$daycounttotal,$daycount)= - CalcFine($data->[$i], $borrower->{'categorycode'}, $branchcode,undef,undef, $datedue ,$today); - my ($delays1,$delays2,$delays3)=GetOverdueDelays($borrower->{'categorycode'}); + ($datedue_days <= $today_days) or next; # or it's not overdue, right? + $overdueItemsCounted++; + my ($amount,$type,$daycounttotal,$daycount)= + CalcFine($data->[$i], $borrower->{'categorycode'}, $branchcode,undef,undef, $datedue, $today); + # FIXME: $type NEVER gets populated by anything. + (defined $type) or $type = ''; # Don't update the fine if today is a holiday. # This ensures that dropbox mode will remove the correct amount of fine. - if( (C4::Context->preference('finesMode') eq 'production') && ! $isHoliday ) { - # FIXME - $type is always null, afaict. + if ($mode eq 'production' and ! $isHoliday) { UpdateFine($data->[$i]->{'itemnumber'},$data->[$i]->{'borrowernumber'},$amount,$type,$due_str) if( $amount > 0 ) ; } - print FILE "$printout\t$borrower->{'cardnumber'}\t$borrower->{'categorycode'}\t$borrower->{'surname'}\t$borrower->{'firstname'}\t$borrower->{'email'}\t$borrower->{'phone'}\t$borrower->{'address'}\t$borrower->{'city'}\t$data->[$i]->{'barcode'}\t$data->[$i]->{'date_due'}\t$type\t$data->[$i]->{'itemnumber'}\t$daycounttotal\t$amount\n"; - } + my @cells = (); + push @cells, map {$borrower->{$_}} @borrower_fields; + push @cells, map {$data->[$i]->{$_}} @item_fields; + push @cells, $type, $daycounttotal, $amount; + print FILE join($delim, @cells), "\n"; } -my $numOverdueItems=scalar(@$data); -if ($DEBUG) { - print <