Bug 17699 - DateTime durations are not correctly subtracted
Summary: DateTime durations are not correctly subtracted
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 16911
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-30 08:06 UTC by Jonathan Druart
Modified: 2017-10-24 03:40 UTC (History)
5 users (show)

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


Attachments
Bug 17699: Fix negative duration calculations in tests (1.55 KB, patch)
2016-11-30 08:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17699: Add more tests to highlight the problem (8.76 KB, patch)
2017-07-31 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17699: Use limit as end_of_month (2.83 KB, patch)
2017-07-31 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17699: Add test descriptions (5.69 KB, patch)
2017-07-31 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17699: Add more tests to highlight the problem (8.71 KB, patch)
2017-07-31 16:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17699: Use limit as end_of_month (2.81 KB, patch)
2017-07-31 16:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17699: Add test descriptions (5.69 KB, patch)
2017-07-31 16:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17699: Add test descriptions (5.76 KB, patch)
2017-07-31 17:05 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17699: Add more tests to highlight the problem (8.76 KB, patch)
2017-07-31 17:07 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17699: Use limit as end_of_month (2.86 KB, patch)
2017-07-31 17:07 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17699: Add test descriptions (5.74 KB, patch)
2017-07-31 17:07 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 17699: Reset time simulation (692 bytes, patch)
2017-08-01 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17699: Add more tests to highlight the problem (8.85 KB, patch)
2017-08-02 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17699: Use limit as end_of_month (2.94 KB, patch)
2017-08-02 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17699: Add test descriptions (5.82 KB, patch)
2017-08-02 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17699: Reset time simulation (782 bytes, patch)
2017-08-02 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-11-30 08:06:53 UTC
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
Comment 1 Jonathan Druart 2016-11-30 08:11:58 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-11-30 12:05:13 UTC
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)
Comment 3 M. Tompsett 2017-02-21 01:31:10 UTC
#!/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.
Comment 4 M. Tompsett 2017-02-21 01:46:01 UTC
I got the values from: http://search.cpan.org/~drolsky/DateTime-1.42/lib/DateTime/Duration.pm#$dur->end_of_month_mode()
Comment 5 Jonathan Druart 2017-02-22 10:21:39 UTC
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...
Comment 6 M. Tompsett 2017-02-22 13:53:21 UTC
(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?
Comment 7 Jonathan Druart 2017-03-31 18:08:14 UTC
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'
Comment 8 Jonathan Druart 2017-07-31 13:26:43 UTC
Same today (July 31)
Comment 9 Jonathan Druart 2017-07-31 15:16:25 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2017-07-31 15:16:31 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2017-07-31 15:16:36 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2017-07-31 15:36:21 UTC
Important note: these patches are going to change the existing behaviour (how the dates are calculated), not only fix the tests.
Comment 13 Jonathan Druart 2017-07-31 16:48:48 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2017-07-31 16:48:54 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2017-07-31 16:48:58 UTC Comment hidden (obsolete)
Comment 16 Owen Leonard 2017-07-31 17:05:19 UTC
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.
Comment 17 Owen Leonard 2017-07-31 17:07:21 UTC
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>
Comment 18 Owen Leonard 2017-07-31 17:07:24 UTC
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>
Comment 19 Owen Leonard 2017-07-31 17:07:27 UTC
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>
Comment 20 Marcel de Rooy 2017-08-01 14:12:02 UTC
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.
Comment 21 Jonathan Druart 2017-08-01 14:45:48 UTC
Created attachment 65400 [details] [review]
Bug 17699: Reset time simulation
Comment 22 Jonathan Druart 2017-08-01 14:47:50 UTC
(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.
Comment 23 Marcel de Rooy 2017-08-01 14:57:52 UTC
(In reply to Jonathan Druart from comment #22)
> It's already a Koha dependency.
So it is indeed. Wrong assumption..
t/Circulation/AgeRestrictionMarkers.t
Comment 24 Marcel de Rooy 2017-08-01 14:59:55 UTC
(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.
Comment 25 Marcel de Rooy 2017-08-01 15:02:17 UTC
QA: Finish this one tomorrow
Comment 26 Marcel de Rooy 2017-08-02 12:03:35 UTC
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.
Comment 27 Marcel de Rooy 2017-08-02 12:04:45 UTC
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>
Comment 28 Marcel de Rooy 2017-08-02 12:04:50 UTC
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>
Comment 29 Marcel de Rooy 2017-08-02 12:04:54 UTC
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>
Comment 30 Marcel de Rooy 2017-08-02 12:04:58 UTC
Created attachment 65422 [details] [review]
Bug 17699: Reset time simulation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Jonathan Druart 2017-08-08 12:44:51 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 32 Fridolin SOMERS 2017-09-29 09:03:52 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 33 Katrin Fischer 2017-10-01 21:24:20 UTC
These patches have been pushed to 16.11.x and will be in 16.11.13.
Comment 34 Mason James 2017-10-24 03:40:02 UTC
Blocked by Enhancement, skipping for 16.05.x