Bug 7447 - Allow to specify a date in overdue_notice.pl
Summary: Allow to specify a date in overdue_notice.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Paul Poulain
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks: 11120
  Show dependency treegraph
 
Reported: 2012-01-16 16:15 UTC by Paul Poulain
Modified: 2013-10-23 08:12 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7447 allow to specify a date in overdue_notice.pl (3.94 KB, patch)
2012-01-16 16:19 UTC, Paul Poulain
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7447 allow to specify a date in overdue_notice.pl (4.01 KB, patch)
2012-06-09 10:03 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2012-01-16 16:15:59 UTC

    
Comment 1 Paul Poulain 2012-01-16 16:19:39 UTC Comment hidden (obsolete)
Comment 2 Ian Walls 2012-02-07 15:17:48 UTC
This patch doesn't do any kind of format-checking on the 'date' param, and then loads the variable directly into the SQL.  This could cause the query to fail (at best), return completely different values than intended, or destroy entire tables (at very worst).

The date variable should be parameterized for the sth->execute(), and should be rigourously checked for proper date formatting.  If the incoming value is not a correct date, either warn and use NOW(), or abort the script.
Comment 3 Paul Poulain 2012-02-13 14:01:37 UTC
(In reply to comment #2)
> This patch doesn't do any kind of format-checking on the 'date' param, and then
> loads the variable directly into the SQL.  This could cause the query to fail
> (at best), return completely different values than intended, or destroy entire
> tables (at very worst).

I disagree, the following line quote & make the query safe:
+    $date=$dbh->quote($date);
It's needed, as, if there is no $date passed, then the date must be compared to NOW(), which is mad on the else:
+    $date="NOW()";


> The date variable should be parameterized for the sth->execute(), and should be
> rigourously checked for proper date formatting.  If the incoming value is not a
> correct date, either warn and use NOW(), or abort the script.

We don't do any format checking in most of the scripts, this script does not differ from others.

If you think we must enforce our coding guidelines, and it's another topic (and I tend to agree with it), that should be discussed outside from this patch.

Switching back to "needs signoff"
Comment 4 Kyle M Hall 2012-06-09 10:03:15 UTC
Created attachment 9995 [details] [review]
[SIGNED-OFF] Bug 7447 allow to specify a date in overdue_notice.pl

This patch add a new parameter to overdue_notices.pl, that is a date.
If you add --date=YYYY-MM-DD when running overdue_notices, it will generate overdues as if you were on date provided

that's usefull if you want to relaunch an overdue calculation that has failed, of after changing your circ rules

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Ian Walls 2012-06-15 15:23:49 UTC
$dbh->quote takes care of the cleanliness, so my original objection is withdrawn.  Marking Passed QA.
Comment 6 Jonathan Druart 2012-08-09 13:42:31 UTC
This patch introduces a bug, see Bug 8607