[Koha-patches] [PATCH] Fines fixes: apparent problems with fines prevent processing.
Ryan Higgins
ryan.higgins at liblime.com
Fri Aug 29 06:32:13 CEST 2008
Patch tested.
Requesting expedited push, and I think this fix along with a few others
gives us good reason for a 3.0 bugfix release.
Ryan
On Mon, Aug 25, 2008 at 11:57 PM, Joe Atzberger
<joe.atzberger at liblime.com>wrote:
> 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.
> ---
> 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 a279e2f..8accf2b 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
> -
> - at 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 bacd80a..63b0842 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 afc4dca..0e28357 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;$i<scalar(@$data);$i++){
> - my $datedue=C4::Dates->new($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; $i<scalar(@$data); $i++) {
> + my $datedue = C4::Dates->new($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 <<EOM
> -
> -Number of Overdue Items counted $overdueItemsCounted
> -Number of Overdue Items reported $numOverdueItems
> +my $numOverdueItems = scalar(@$data);
> +if ($summary) {
> + print <<EOM;
> +Fines assessment -- $today_iso -- Saved to $filename
> +Number of Overdue Items:
> + counted $overdueItemsCounted
> + reported $numOverdueItems
>
> EOM
> }
> diff --git a/t/lib/KohaTest/Calendar.pm b/t/lib/KohaTest/Calendar.pm
> index f1825af..8b1cda7 100644
> --- a/t/lib/KohaTest/Calendar.pm
> +++ b/t/lib/KohaTest/Calendar.pm
> @@ -13,8 +13,6 @@ sub testing_class { 'C4::Calendar' };
> sub methods : Test( 1 ) {
> my $self = shift;
> my @methods = qw( new
> - _init
> - change_branchcode
> get_week_days_holidays
> get_day_month_holidays
> get_exception_holidays
> --
> 1.5.5.GIT
>
> _______________________________________________
> Koha-patches mailing list
> Koha-patches at lists.koha.org
> http://lists.koha.org/mailman/listinfo/koha-patches
>
--
Ryan Higgins
LibLime * Open-Source Solutions for Libraries
Featuring KohaZOOM ILS
888-564-2457 x704
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20080829/b4bcb422/attachment-0002.htm>
More information about the Koha-patches
mailing list