Bug 11120 - The date option for the overdue_notices cronjob is broken
Summary: The date option for the overdue_notices cronjob is broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 7447
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-23 08:12 UTC by Jonathan Druart
Modified: 2015-12-03 22:02 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this: 2013-10-30 00:00:00
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 11120: FIX the --date option for overdue_notices cronjob (10.66 KB, patch)
2013-10-23 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11120: FIX the --date option for overdue_notices cronjob (10.74 KB, patch)
2014-07-03 08:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11120: FIX the --date option for overdue_notices cronjob (6.89 KB, patch)
2014-11-26 01:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11120: FIX the --date option for overdue_notices cronjob (6.95 KB, patch)
2015-01-05 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
script to populate the DB (2.55 KB, application/x-perl)
2015-01-05 14:51 UTC, Jonathan Druart
Details
[PASSED QA] Bug 11120: FIX the --date option for overdue_notices cronjob (7.12 KB, patch)
2015-01-05 22:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11120: Follow-up: adding a hint about the date format (1.30 KB, patch)
2015-01-05 22:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11120: Follow-up: adding a hint about the date format (1.30 KB, patch)
2015-01-06 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11120: the date input should be in a iso format (YYYY-MM-DD) (1.21 KB, patch)
2015-01-06 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11120: Should this test fail? (1.15 KB, patch)
2015-01-06 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11120: FIX the --date option for overdue_notices cronjob (7.18 KB, patch)
2015-03-02 11:40 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11120: Follow-up: adding a hint about the date format (1.37 KB, patch)
2015-03-02 11:40 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11120: the date input should be in a iso format (YYYY-MM-DD) (1.28 KB, patch)
2015-03-02 11:40 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-10-23 08:12:30 UTC
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.

The date should be defined as --date:s and the script should correctly manage this option.
Comment 1 Jonathan Druart 2013-10-23 08:40:39 UTC Comment hidden (obsolete)
Comment 2 I'm just a bot 2013-10-30 07:51:27 UTC
Patch applied cleanly, go forth and signoff
Comment 3 Jonathan Druart 2014-07-03 08:16:56 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-07-03 08:17:31 UTC
Rebased patch.
Comment 5 Srdjan Jankovic 2014-08-18 23:25:52 UTC
This does not seem to apply.

Also, $today = dt_from_string; should probably be $today = dt_from_string(); for the sake of readability.
Comment 6 Jonathan Druart 2014-11-25 16:09:51 UTC
I alerted on bug 12529 comment 4 that this patch was very important.
Now it does not apply at all and need to be rewrite.

I don't plan to rebase this patch.
Comment 7 Chris Cormack 2014-11-26 01:59:32 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2014-11-26 02:00:56 UTC
I *think* I have reworked this, keeping the functionality, please test. If not let me know and I will try again.
Comment 9 Jonathan Druart 2015-01-05 14:50:14 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-01-05 14:51:53 UTC
Created attachment 34953 [details]
script to populate the DB

I use this simple script to insert overdues in the DB.
Comment 11 Jonathan Druart 2015-01-05 14:53:48 UTC
(In reply to Chris Cormack from comment #8)
> I *think* I have reworked this, keeping the functionality, please test. If
> not let me know and I will try again.

Thank you very much to have resubmitted this patch.
I tested it thoroughly and did not find any regression. The --date parameter works as expected.
Comment 12 Katrin Fischer 2015-01-05 22:05:17 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2015-01-05 22:08:09 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-01-06 08:15:44 UTC
Comment on attachment 34963 [details] [review]
Bug 11120: Follow-up: adding a hint about the date format

Review of attachment 34963 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/overdue_notices.pl
@@ +364,4 @@
>      eval {
>          $date_to_run = dt_from_string( $date_input );
>      };
> +    die "$date_input is not a valid date, aborting! Use a date in format YYYY-MM-DD."

No, this is wrong.
The format depends on the dateformat pref.
Comment 15 Katrin Fischer 2015-01-06 10:59:49 UTC
Well, we can just throw out the second patch, but I am confused. I definitely tested with YYYY-MM-DD as I had been looking for examples in the bug report. And it worked, although my preference is set to mm/dd/yyyy.
Comment 16 Jonathan Druart 2015-01-06 11:58:03 UTC
(In reply to Katrin Fischer from comment #15)
> Well, we can just throw out the second patch, but I am confused. I
> definitely tested with YYYY-MM-DD as I had been looking for examples in the
> bug report. And it worked, although my preference is set to mm/dd/yyyy.

Yes you are right, it works too.
Comment 17 Jonathan Druart 2015-01-06 11:58:59 UTC
Comment on attachment 34962 [details] [review]
[PASSED QA] Bug 11120: FIX the --date option for overdue_notices cronjob

Review of attachment 34962 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/overdue_notices.pl
@@ +359,5 @@
>  }
>  my $date_to_run;
> +my $date;
> +if ( $date_input ){
> +    $date = $dbh->quote($date);

Should be
  $date = $dbh->quote($date_input);
Isn't it?
Comment 18 Katrin Fischer 2015-01-06 12:21:18 UTC
I think you are right - it's used in the SQL script - but not sure what for?

SELECT biblio.*, items.*, issues.*, biblioitems.itemtype, TO_DAYS($date)-TO_DAYS(date_due) AS days_overdue, ...
Comment 19 Jonathan Druart 2015-01-06 13:48:03 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-01-06 13:48:08 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-01-06 13:48:12 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-01-06 13:50:25 UTC
About the last 3 patches:
1/ I reintroduced the Katrin's patch
2/ Try to force the date input format to 'iso'
3/ To me the test should not fail, but I don't have the time at the moment to deep into the DateUtils code. Any idea?
Comment 23 Katrin Fischer 2015-01-18 18:09:41 UTC
Could the problem be similar to what we saw on the patch for the parcels page?
Comment 24 Jonathan Druart 2015-01-20 11:37:21 UTC
(In reply to Katrin Fischer from comment #23)
> Could the problem be similar to what we saw on the patch for the parcels
> page?

Yes, actually I noticed this problem for the first time when I worked on this one.
I started a discussion on koha-devel this morning [1], it seems that DateTime::Format::DateParse introduces more problems than it is supposed to help.

[1] http://lists.koha-community.org/pipermail/koha-devel/2015-January/041095.html
Comment 25 Jonathan Druart 2015-01-21 10:34:55 UTC
Comment on attachment 34981 [details] [review]
Bug 11120: Should this test fail?

This is fixed by bug 13601.
Comment 26 Jonathan Druart 2015-01-21 10:35:36 UTC
With bug 13601, the wrong date format will be correctly caught.
Comment 27 Brendan Gallagher 2015-03-02 11:40:27 UTC
Created attachment 36282 [details] [review]
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.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested with my own test data, checked generating overdues
with and without the --date option.
All worked as expected.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
I test with 5318 overdues - works
Comment 28 Brendan Gallagher 2015-03-02 11:40:35 UTC
Created attachment 36283 [details] [review]
Bug 11120: Follow-up: adding a hint about the date format

Adding 2 hints about the expected date format to the error
message and the help.

To test:
- Run overdue_notices.pl --date <someinvaliddate>
- Run overdue_notices.pl -man
- Verify hint about date format shows up

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 29 Brendan Gallagher 2015-03-02 11:40:44 UTC
Created attachment 36284 [details] [review]
Bug 11120: the date input should be in a iso format (YYYY-MM-DD)

Note that it cans also accept a date in the same format defined in the
dateformat system preference.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 30 Tomás Cohen Arazi 2015-03-30 16:50:43 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 31 Chris Cormack 2015-03-31 05:39:29 UTC
Pushed to 3.18.x will be in 3.18.6