From DateTime::Duration pod: """ For positive durations, the "end_of_month" parameter defaults to wrap. For negative durations, the default is "limit". This should match how most people "intuitively" expect datetime math to work. """ Today we are Nov 30, that means $today->subtract( months => 1 ) will return Oct 31, when we usually expect Oct 30 in tests
Created attachment 57827 [details] [review] Bug 17699: Fix negative duration calculations in tests From DateTime::Duration pod: """ For positive durations, the "end_of_month" parameter defaults to wrap. For negative durations, the default is "limit". This should match how most people "intuitively" expect datetime math to work. """ Today we are Nov 30, that means $today->subtract( months => 1 ) will return Oct 31, when we usually expect Oct 30 in tests Test plan: prove t/db_dependent/Koha/Patrons.t should return green
I think this is more tricky than that, I don't find a way to make these tests pass whenever they are launch (think about 31 March - 1 month + 1 month)
#!/usr/bin/perl -w use Modern::Perl; use Data::Dumper; use List::MoreUtils qw / none /; use Time::Fake; BEGIN { my @paths = split ':', $ENV{'PERL5LIB'}; if (@paths) { foreach my $path (@paths) { if (none { $_ eq $path } @INC) { push @INC, $path; } } } } use Koha::DateUtils; for my $count (1..366) { Time::Fake->offset(sprintf("-%dd",$count)); my $old_last_month = dt_from_string->add( months => -1 )->truncate( to => 'day' ); my $wrap_last_month = dt_from_string->subtract( months => 1, end_of_month => 'wrap' )->truncate( to => 'day' ); my $preserve_last_month = dt_from_string->subtract( months => 1, end_of_month => 'preserve' )->truncate( to => 'day' ); my $limit_last_month = dt_from_string->subtract( months => 1, end_of_month => 'limit' )->truncate( to => 'day' ); my $this_day = dt_from_string; print sprintf("T:%s O:%s W:%s P:%s L:%s\n",$this_day,$old_last_month,$wrap_last_month,$preserve_last_month,$limit_last_month); } I'm thinking 'limit'for month subtraction.
I got the values from: http://search.cpan.org/~drolsky/DateTime-1.42/lib/DateTime/Duration.pm#$dur->end_of_month_mode()
Just redone some tests and indeed limit seems like to be what we want: use Modern::Perl; use Time::Fake; use Koha::DateUtils; for my $date ( '2016-03-31', '2016-11-30' ) { my $dt = dt_from_string( $date, 'iso' ); Time::Fake->offset( $dt->epoch ); say "=== $date ==="; say dt_from_string->subtract( months => 1 )->add( months => 1 ); say dt_from_string->subtract( months => 1, end_of_month => 'limit' )->add( months => 1 ); say dt_from_string->subtract( months => 1, end_of_month => 'wrap' )->add( months => 1 ); say dt_from_string->subtract( months => 1, end_of_month => 'wrap' )->add( months => 1 ); } Returns: === 2016-03-31 === 2016-03-29T00:00:00 2016-03-29T00:00:00 => What we want 2016-04-02T00:00:00 2016-04-02T00:00:00 === 2016-11-30 === 2016-12-01T00:00:00 2016-11-30T00:00:00 => What we want 2016-11-30T00:00:00 2016-11-30T00:00:00 Wondering why 'limit' does not work as default...
(In reply to Jonathan Druart from comment #5) > Just redone some tests and indeed limit seems like to be what we want: [SNIP] > Wondering why 'limit' does not work as default... You'd have to ask the developer of the module. :) "For positive durations, the ' "end_of_month" parameter defaults to wrap. For negative durations, the default is "limit". This should match how most people "intuitively" expect datetime math to work. ' -- DateTime::Duration perl Perhaps we uncovered a bug in the method? "date vs datetime math If you only care about the date (calendar) portion of a datetime, you should use either delta_md() or delta_days(), not subtract_datetime(). This will give predictable, unsurprising results, free from DST-related complications." -- http://search.cpan.org/~drolsky/DateTime-1.42/lib/DateTime.pm#How_DateTime_Math_Works Perhaps we shouldn't be taking the 'to_days' way of doing the math?
Same problem appears today (March 31) for t/db_dependent/Koha/Patrons.t # Failed test at t/db_dependent/Koha/Patrons.t line 290. # got: '2018-02-28T00:00:00' # expected: '2018-03-03T00:00:00' # Failed test at t/db_dependent/Koha/Patrons.t line 292. # got: '2018-02-28T00:00:00' # expected: '2018-03-03T00:00:00' # Looks like you failed 2 tests of 10. # Failed test 'renew_account'
Same today (July 31)
Created attachment 65358 [details] [review] Bug 17699: Add more tests to highlight the problem Add problematic cases to highlight the problem.
Created attachment 65359 [details] [review] Bug 17699: Use limit as end_of_month From DateTime::Duration pod: "" For positive durations, the "end_of_month" parameter defaults to wrap. For negative durations, the default is "limit". This should match how most people "intuitively" expect datetime math to work. """" We need end_of_month => limit for positive durations as well.
Created attachment 65360 [details] [review] Bug 17699: Add test descriptions Test plan: prove -v t/db_dependent/Koha/Patrons.t Subtest: renew_account 1..30 ok 1 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 2 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 3 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 4 - today + 12 months must be 2017-03-31T00:00:00 ok 5 - today + 12 months must be 2017-03-31T00:00:00 ok 6 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 7 - today + 12 months must be 2017-03-31T00:00:00 ok 8 - today + 12 months must be 2017-03-31T00:00:00 ok 9 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 10 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 11 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 12 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 13 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 14 - today + 12 months must be 2017-11-30T00:00:00 ok 15 - today + 12 months must be 2017-11-30T00:00:00 ok 16 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 17 - today + 12 months must be 2017-11-30T00:00:00 ok 18 - today + 12 months must be 2017-11-30T00:00:00 ok 19 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 20 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 21 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 22 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 23 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 24 - today + 12 months must be 2018-07-31T00:00:00 ok 25 - today + 12 months must be 2018-07-31T00:00:00 ok 26 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 27 - today + 12 months must be 2018-07-31T00:00:00 ok 28 - today + 12 months must be 2018-07-31T00:00:00 ok 29 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 ok 30 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00
Important note: these patches are going to change the existing behaviour (how the dates are calculated), not only fix the tests.
Created attachment 65365 [details] [review] Bug 17699: Add more tests to highlight the problem Add problematic cases to highlight the problem.
Created attachment 65366 [details] [review] Bug 17699: Use limit as end_of_month From DateTime::Duration pod: "" For positive durations, the "end_of_month" parameter defaults to wrap. For negative durations, the default is "limit". This should match how most people "intuitively" expect datetime math to work. """" We need end_of_month => limit for positive durations as well.
Created attachment 65367 [details] [review] Bug 17699: Add test descriptions Test plan: prove -v t/db_dependent/Koha/Patrons.t Subtest: renew_account 1..30 ok 1 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 2 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 3 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 4 - today + 12 months must be 2017-03-31T00:00:00 ok 5 - today + 12 months must be 2017-03-31T00:00:00 ok 6 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 7 - today + 12 months must be 2017-03-31T00:00:00 ok 8 - today + 12 months must be 2017-03-31T00:00:00 ok 9 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 10 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 11 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 12 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 13 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 14 - today + 12 months must be 2017-11-30T00:00:00 ok 15 - today + 12 months must be 2017-11-30T00:00:00 ok 16 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 17 - today + 12 months must be 2017-11-30T00:00:00 ok 18 - today + 12 months must be 2017-11-30T00:00:00 ok 19 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 20 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 21 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 22 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 23 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 24 - today + 12 months must be 2018-07-31T00:00:00 ok 25 - today + 12 months must be 2018-07-31T00:00:00 ok 26 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 27 - today + 12 months must be 2018-07-31T00:00:00 ok 28 - today + 12 months must be 2018-07-31T00:00:00 ok 29 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 ok 30 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00
Created attachment 65369 [details] [review] [SIGNED-OFF] Bug 17699: Add test descriptions Test plan: prove -v t/db_dependent/Koha/Patrons.t Subtest: renew_account 1..30 ok 1 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 2 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 3 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 4 - today + 12 months must be 2017-03-31T00:00:00 ok 5 - today + 12 months must be 2017-03-31T00:00:00 ok 6 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 7 - today + 12 months must be 2017-03-31T00:00:00 ok 8 - today + 12 months must be 2017-03-31T00:00:00 ok 9 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 10 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 11 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 12 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 13 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 14 - today + 12 months must be 2017-11-30T00:00:00 ok 15 - today + 12 months must be 2017-11-30T00:00:00 ok 16 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 17 - today + 12 months must be 2017-11-30T00:00:00 ok 18 - today + 12 months must be 2017-11-30T00:00:00 ok 19 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 20 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 21 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 22 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 23 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 24 - today + 12 months must be 2018-07-31T00:00:00 ok 25 - today + 12 months must be 2018-07-31T00:00:00 ok 26 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 27 - today + 12 months must be 2018-07-31T00:00:00 ok 28 - today + 12 months must be 2018-07-31T00:00:00 ok 29 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 ok 30 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 Signed-off-by: Owen Leonard <oleonard@myacpl.org> All tests successful.
Created attachment 65370 [details] [review] [SIGNED-OFF] Bug 17699: Add more tests to highlight the problem Add problematic cases to highlight the problem. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 65371 [details] [review] [SIGNED-OFF] Bug 17699: Use limit as end_of_month From DateTime::Duration pod: "" For positive durations, the "end_of_month" parameter defaults to wrap. For negative durations, the default is "limit". This should match how most people "intuitively" expect datetime math to work. """" We need end_of_month => limit for positive durations as well. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 65372 [details] [review] [SIGNED-OFF] Bug 17699: Add test descriptions Test plan: prove -v t/db_dependent/Koha/Patrons.t Subtest: renew_account 1..30 ok 1 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 2 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 3 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 4 - today + 12 months must be 2017-03-31T00:00:00 ok 5 - today + 12 months must be 2017-03-31T00:00:00 ok 6 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 7 - today + 12 months must be 2017-03-31T00:00:00 ok 8 - today + 12 months must be 2017-03-31T00:00:00 ok 9 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 10 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 11 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 12 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 13 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 14 - today + 12 months must be 2017-11-30T00:00:00 ok 15 - today + 12 months must be 2017-11-30T00:00:00 ok 16 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 17 - today + 12 months must be 2017-11-30T00:00:00 ok 18 - today + 12 months must be 2017-11-30T00:00:00 ok 19 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 20 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 21 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 22 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 23 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 24 - today + 12 months must be 2018-07-31T00:00:00 ok 25 - today + 12 months must be 2018-07-31T00:00:00 ok 26 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 27 - today + 12 months must be 2018-07-31T00:00:00 ok 28 - today + 12 months must be 2018-07-31T00:00:00 ok 29 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 ok 30 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Looks good to me, but I am wondering if we really need an additional module to test the behavior of BorrowerRenewalPeriodBase. Does this new dependency really bring us great benefits here? Time::Fake is Debian packaged though. It seems to me that testing the "now" variant could be done too with current time. The other variant "dateexpiry" allows you to set dates so that you can see that the end_of_month limit really works. And if we should use it anyway, I would prefer to see the offset call just before the code it should actually work on and an additional Time::Fake->reset after that block. Changing status to reflect need for feedback.
Created attachment 65400 [details] [review] Bug 17699: Reset time simulation
(In reply to Marcel de Rooy from comment #20) > Looks good to me, but I am wondering if we really need an additional module > to test the behavior of BorrowerRenewalPeriodBase. Does this new dependency > really bring us great benefits here? > Time::Fake is Debian packaged though. It's already a Koha dependency. > It seems to me that testing the "now" variant could be done too with current > time. The other variant "dateexpiry" allows you to set dates so that you can > see that the end_of_month limit really works. I think it is less error prone to use Time::Fake > And if we should use it anyway, I would prefer to see the offset call just > before the code it should actually work on and an additional > Time::Fake->reset after that block. I added the ->reset call, but I do not understand your other remark. Where do you want it to be moved? It sounds better to me to have it visible at the beginning of the loop.
(In reply to Jonathan Druart from comment #22) > It's already a Koha dependency. So it is indeed. Wrong assumption.. t/Circulation/AgeRestrictionMarkers.t
(In reply to Jonathan Druart from comment #22) > I added the ->reset call, but I do not understand your other remark. Where > do you want it to be moved? It sounds better to me to have it visible at the > beginning of the loop. Do you need it in the whole loop? I would say that you only need it somewhere deeper when you calculate the new expiry date for 'now'. But adding the reset is good enough, I suppose.
QA: Finish this one tomorrow
Just a remark: Date::Calc's Add_Delta_YM does the same as what you are doing now with limit. ($year,$month,$day) = Add_Delta_YM($year,$month,$day, $Dy,$Dm); From POD: This function does no "wrapping" into the next month if the day happens to lie outside the valid range for the resulting year and month (after adding the year and month offsets). Instead, it simply truncates the day to the last possible day of the resulting month.
Created attachment 65419 [details] [review] Bug 17699: Add more tests to highlight the problem Add problematic cases to highlight the problem. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65420 [details] [review] Bug 17699: Use limit as end_of_month From DateTime::Duration pod: "" For positive durations, the "end_of_month" parameter defaults to wrap. For negative durations, the default is "limit". This should match how most people "intuitively" expect datetime math to work. """" We need end_of_month => limit for positive durations as well. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65421 [details] [review] Bug 17699: Add test descriptions Test plan: prove -v t/db_dependent/Koha/Patrons.t Subtest: renew_account 1..30 ok 1 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 2 - 2016-02-29T00:00:00 + 12 months must be 2017-02-28T00:00:00 ok 3 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 4 - today + 12 months must be 2017-03-31T00:00:00 ok 5 - today + 12 months must be 2017-03-31T00:00:00 ok 6 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 7 - today + 12 months must be 2017-03-31T00:00:00 ok 8 - today + 12 months must be 2017-03-31T00:00:00 ok 9 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 10 - 2016-04-30T00:00:00 + 12 months must be 2017-04-30T00:00:00 ok 11 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 12 - 2016-10-30T00:00:00 + 12 months must be 2017-10-30T00:00:00 ok 13 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 14 - today + 12 months must be 2017-11-30T00:00:00 ok 15 - today + 12 months must be 2017-11-30T00:00:00 ok 16 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 17 - today + 12 months must be 2017-11-30T00:00:00 ok 18 - today + 12 months must be 2017-11-30T00:00:00 ok 19 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 20 - 2016-12-30T00:00:00 + 12 months must be 2017-12-30T00:00:00 ok 21 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 22 - 2017-06-30T00:00:00 + 12 months must be 2018-06-30T00:00:00 ok 23 - With BorrowerLogs, Koha::Patron->renew_account should have logged ok 24 - today + 12 months must be 2018-07-31T00:00:00 ok 25 - today + 12 months must be 2018-07-31T00:00:00 ok 26 - Without BorrowerLogs, Koha::Patron->renew_account should not have logged ok 27 - today + 12 months must be 2018-07-31T00:00:00 ok 28 - today + 12 months must be 2018-07-31T00:00:00 ok 29 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 ok 30 - 2017-08-31T00:00:00 + 12 months must be 2018-08-31T00:00:00 Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65422 [details] [review] Bug 17699: Reset time simulation Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.05.
These patches have been pushed to 16.11.x and will be in 16.11.13.
Blocked by Enhancement, skipping for 16.05.x