Description
Jonathan Druart
2013-10-23 08:12:30 UTC
Created attachment 22304 [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. Patch applied cleanly, go forth and signoff Created attachment 29465 [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. Rebased patch. This does not seem to apply. Also, $today = dt_from_string; should probably be $today = dt_from_string(); for the sake of readability. 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. Created attachment 33929 [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. Note - I have reworked this patch, I think it all works but please test very thorougly - Chris C I *think* I have reworked this, keeping the functionality, please test. If not let me know and I will try again. Created attachment 34952 [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> Created attachment 34953 [details]
script to populate the DB
I use this simple script to insert overdues in the DB.
(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. Created attachment 34962 [details] [review] [PASSED QA] 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. Created attachment 34963 [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 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. 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. (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 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? 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, ... Created attachment 34979 [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 Created attachment 34980 [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. Created attachment 34981 [details] [review] Bug 11120: Should this test fail? 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? Could the problem be similar to what we saw on the patch for the parcels page? (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 on attachment 34981 [details] [review] Bug 11120: Should this test fail? This is fixed by bug 13601. With bug 13601, the wrong date format will be correctly caught. 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 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> 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> Patches pushed to master. Thanks Jonathan! Pushed to 3.18.x will be in 3.18.6 |