From 75622eb756d612b1abdbd3d23b78759f9e5aa29c Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 23 Oct 2013 10:13:50 +0200 Subject: [PATCH] Bug 11120: FIX the --date option for overdue_notices cronjob Bug 7447 introduces the --date option for overdue notices. This option has never worked: the code is waiting for a value but the option is defined as a boolean. This patch fixes the option and change the way to calculate the range of dates. This range is now managed in Perl instead of in the SQL query. To do it in Perl allows to build dates simply using the DateTime and DateTime::Duration modules. To test this patch you should have a DB with a lot of overdues, (I tested on a DB with 512 overdues). A test plan could be: 1/ Dump your message_queue table 2/ Verify the number of overdues in the database before applying the patch: mysql> DELETE FROM message_queue; perl misc/cronjobs/overdue_notices.pl -v -t (the triggered option will generate overdue for today) mysql> SELECT COUNT(*) FROM message_queue; Note this value 2A mysql> DELETE FROM message_queue; perl misc/cronjobs/overdue_notices.pl -v mysql> SELECT COUNT(*) FROM message_queue; Note this value 2B 2/ Apply the patch 4/ Verify the number of overdues generated by the patched script: mysql> DELETE FROM message_queue; perl misc/cronjobs/overdue_notices.pl -v -t mysql> SELECT COUNT(*) FROM message_queue; Note this value 4A mysql> DELETE FROM message_queue; perl misc/cronjobs/overdue_notices.pl -v mysql> SELECT COUNT(*) FROM message_queue; Note this value 4B mysql> DELETE FROM message_queue; # The date should be defined depending your dateformat preference # and should be the date of the current day perl misc/cronjobs/overdue_notices.pl -v -t --date="YYYY-MM-DD" mysql> SELECT COUNT(*) FROM message_queue; Note this value 4C mysql> DELETE FROM message_queue; # The date should be defined depending your dateformat preference # and should be the date of the current day perl misc/cronjobs/overdue_notices.pl -v --date="YYYY-MM-DD" mysql> SELECT COUNT(*) FROM message_queue; Note this value 4D 5/ Compare the values: All values generated with the -t options should be equals. Same for values without the -t options. => 2A == 4A == 4C and 2B == 4B == 4D 6/ Go back to a normal activity for 3 days or manually change the date_due for issues in the DB: mysql> update issues SET date_due = DATE_SUB(date_due, INTERVAL 3 DAY); Do again step 4C and 4D with a date equals to today - 3 days. Values should be the same as 4C and 4D. --- misc/cronjobs/overdue_notices.pl | 79 ++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 21 deletions(-) diff --git a/misc/cronjobs/overdue_notices.pl b/misc/cronjobs/overdue_notices.pl index c1c4c19..37bbb0c 100755 --- a/misc/cronjobs/overdue_notices.pl +++ b/misc/cronjobs/overdue_notices.pl @@ -34,6 +34,8 @@ use Pod::Usage; use Text::CSV_XS; use Locale::Currency::Format 1.28; use Encode; +use DateTime; +use DateTime::Duration; use C4::Context; use C4::Dates qw/format_date/; @@ -41,6 +43,7 @@ use C4::Debug; use C4::Letters; use C4::Overdues qw(GetFine); use C4::Budgets qw(GetCurrency); +use Koha::DateUtils; =head1 NAME @@ -276,7 +279,7 @@ my $listall = 0; my $itemscontent = join( ',', qw( date_due title barcode author itemnumber ) ); my @myborcat; my @myborcatout; -my $date; +my ( $date_input, $today ); GetOptions( 'help|?' => \$help, @@ -290,7 +293,7 @@ GetOptions( 'itemscontent=s' => \$itemscontent, 'list-all' => \$listall, 't|triggered' => \$triggered, - 'date' => \$date, + 'date=s' => \$date_input, 'borcat=s' => \@myborcat, 'borcatout=s' => \@myborcatout, 'email=s' => \@emails, @@ -340,11 +343,16 @@ if (@branchcodes) { } } -if ($date){ - $date=$dbh->quote($date); +if ( $date_input ){ + eval { + $today = dt_from_string( $date_input ); + }; + die "$date_input is not a valid date, aborting!" + if $@ or not $today; + } else { - $date="NOW()"; + $today = dt_from_string; } # these are the fields that will be substituted into <> @@ -377,7 +385,6 @@ if ( defined $htmlfilename ) { if ( $htmlfilename eq '' ) { $html_fh = *STDOUT; } else { - my $today = DateTime->now(time_zone => C4::Context->tz ); open $html_fh, ">",File::Spec->catdir ($htmlfilename,"notices-".$today->ymd().".html"); } @@ -403,13 +410,13 @@ foreach my $branchcode (@branches) { $verbose and warn sprintf "branchcode : '%s' using %s\n", $branchcode, $admin_email_address; my $sth2 = $dbh->prepare( <<"END_SQL" ); -SELECT biblio.*, items.*, issues.*, biblioitems.itemtype, TO_DAYS($date)-TO_DAYS(date_due) AS days_overdue +SELECT biblio.*, items.*, issues.*, biblioitems.itemtype FROM issues,items,biblio, biblioitems WHERE items.itemnumber=issues.itemnumber AND biblio.biblionumber = items.biblionumber AND biblio.biblionumber = biblioitems.biblionumber AND issues.borrowernumber = ? - AND TO_DAYS($date)-TO_DAYS(date_due) BETWEEN ? and ? + AND CAST(date_due AS date) BETWEEN ? and ? END_SQL my $query = "SELECT * FROM overduerules WHERE delay1 IS NOT NULL AND branchcode = ? "; @@ -433,7 +440,8 @@ END_SQL while ( my $overdue_rules = $rqoverduerules->fetchrow_hashref ) { PERIOD: foreach my $i ( 1 .. 3 ) { - $verbose and warn "branch '$branchcode', pass $i\n"; + $verbose and warn "branch '$branchcode', categorycode = $overdue_rules->{categorycode} pass $i\n"; + my $mindays = $overdue_rules->{"delay$i"}; # the notice will be sent after mindays days (grace period) my $maxdays = ( $overdue_rules->{ "delay" . ( $i + 1 ) } @@ -441,6 +449,22 @@ END_SQL : ($MAX) ); # issues being more than maxdays late are managed somewhere else. (borrower probably suspended) + next unless defined $mindays; + + my $range_end = output_pref({ + dt => $today->clone->subtract_duration( DateTime::Duration->new( days => $mindays ) ), + dateformat => 'sql', + dateonly => 1, + }); + my $range_begin = $triggered + ? $range_end + : output_pref({ + dt => $today->clone->subtract_duration( DateTime::Duration->new( days => $maxdays ) ), + dateformat => 'sql', + date_only => 1, + }); + + if ( !$overdue_rules->{"letter$i"} ) { $verbose and warn "No letter$i code for branch '$branchcode'"; next PERIOD; @@ -468,18 +492,14 @@ END_SQL push @borrower_parameters, $overdue_rules->{categorycode}; } $borrower_sql .= ' AND categories.overduenoticerequired=1 '; - if($triggered) { - $borrower_sql .= " AND TO_DAYS($date)-TO_DAYS(date_due) = ?"; - push @borrower_parameters, $mindays; - } else { - $borrower_sql .= " AND TO_DAYS($date)-TO_DAYS(date_due) BETWEEN ? and ? " ; - push @borrower_parameters, $mindays, $maxdays; - } + + $borrower_sql .= " AND CAST(date_due AS date) BETWEEN ? and ? " ; + push @borrower_parameters, $range_begin, $range_end; # $sth gets borrower info iff at least one overdue item has triggered the overdue action. my $sth = $dbh->prepare($borrower_sql); $sth->execute(@borrower_parameters); - $verbose and warn $borrower_sql . "\n $branchcode | " . $overdue_rules->{'categorycode'} . "\n ($mindays, $maxdays)\nreturns " . $sth->rows . " rows"; + $verbose and warn $borrower_sql . "\n $branchcode | " . $overdue_rules->{'categorycode'} . "\n ($range_begin, $range_end)\nreturns " . $sth->rows . " rows"; while ( my $data = $sth->fetchrow_hashref ) { my $borrowernumber = $data->{'borrowernumber'}; @@ -507,7 +527,7 @@ END_SQL my $letter = C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode ); unless ($letter) { - $verbose and warn "Message '$overdue_rules->{letter$i}' content not found"; + $verbose and warn qq|Message '$overdue_rules->{"letter$i"}' content not found|; # might as well skip while PERIOD, no other borrowers are going to work. # FIXME : Does this mean a letter must be defined in order to trigger a debar ? @@ -520,8 +540,25 @@ END_SQL C4::Members::DebarMember($borrowernumber, '9999-12-31'); $verbose and warn "debarring $borr\n"; } - my @params = ($listall ? ( $borrowernumber , 1 , $MAX ) : ( $borrowernumber, $mindays, $maxdays )); - $verbose and warn "STH2 PARAMS: borrowernumber = $borrowernumber, mindays = $mindays, maxdays = $maxdays"; + + my @params = ( $listall + ? ( $borrowernumber , output_pref({ + dt => $today->clone->subtract_duration( days => 1 ), + dateformat => 'sql', + date_only => 1 + }), + output_pref({ + dt => $today->clone->subtract_duration( days => $maxdays ), + dateformat => 'sql', + date_only => 1 + }) + ) + : ( $borrowernumber, $range_begin, $range_end ) + ); + $verbose and warn $listall + ? "STH2 PARAMS: borrowernumber = $borrowernumber, from = NOW - 1, to = NOW - $maxdays, maxdays = $maxdays" + : "STH2 PARAMS: borrowernumber = $borrowernumber, from = $range_begin, to = $range_end, maxdays = $maxdays"; + $sth2->execute(@params); my $itemcount = 0; my $titles = ""; @@ -555,7 +592,7 @@ END_SQL } ); unless ($letter) { - $verbose and warn "Message '$overdue_rules->{letter$i}' content not found"; + $verbose and warn qq|Message '$overdue_rules->{"letter$i"}' content not found|; # might as well skip while PERIOD, no other borrowers are going to work. # FIXME : Does this mean a letter must be defined in order to trigger a debar ? -- 1.7.10.4