Daylight savings incorrect due dates

[Bill Erickson]

Evergreen 2.11, affects all versions.

Creating a circulation between midnight and 1 am whose due date falls on the other side of the daylight savings boundary results in a circulation with a due date landing 1 day before the expected due date.

For example:

xact_start => 2016-10-10 00:05:00-07
due_date => 2016-11-06 23:59:59-08

To compare the expected behavior:

evergreen=# select '2016-10-10 00:05:00-07'::timestamptz + '28 days'::interval;
-[ RECORD 1 ]--------------------
?column? | 2016-11-07 00:05:00-08

==

This is the result of (only) using interval_to_seconds when calculating the due date. Adding 28 days across a DST boundary is not the same as adding 2419200 seconds, since the DST switch-over day is longer.

As discussed in IRC, DateTime::add(days => ...) understands DST boundaries and handles them correctly. The plan is to teach the Perl code to take advantage of this fact by adding days, months, etc. (by leveraging seconds_to_interval) to the due date instead of only adding seconds. Patch en route.

[Bill Erickson (berick)]

Here's an experiment that calculates due dates based on what was discussed in IRC:

http://irc.evergreen-ils.org/evergreen/2016-10-21#i_273165

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1635737-due-date-interval-smarts

This has a number of problems, though. The normalization step, that cross-walks the duration string between interval_to_seconds then seconds_to_interval causes loss of information. For example, "30 days" becomes "1 month 14 hours" which, depending on when it's applied (e.g Feb 1), could be several days shy of "30 days".

I believe we either have the make the Perl interval parsing essentially as smart as PG's interval handling or ask PG to calculate the date for us.

On interesting side effect of this general approach -- honoring the lengths of various duration components based on when the components are applied -- means that we could create duration rules for 1 month and have the due dates always fall on the same day the following month, instead of falling exactly 30 days later regardless of what month it is. And in cases where that's not desired, using day-based durations will still behave as expected.

[Bill Erickson (berick)]

New experiment using PG's internal routine for adding intervals to dates:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1635737-due-date-interval-from-pg

This approach resolves the time change problem, while retaining the behavior of standard day-based durations as before. As previously mentioned, it also adds support for things like month-based durations, where a checkout on Feb 1 becomes due March 1, even though they are not the normal ~30 days apart.

Includes live test.

Another way to manually test this change is to create (or modify) a "10 year" duration rule. Such circs under this new code will be due the same date 10 years later, whereas in the current code it's due 10 years minus a few (leap) days.

I'll give this some time to marinate before adding a pullrequest.

[Bill Erickson (berick)]

Marinate, schmarinate. Adding pullrequest for visibility.

[Galen Charlton (gmc)]

I've looked through other uses of OpenSRF::Utils::interval_to_seconds, and a number of them (such as booking elbow room, fine intervals, grace periods, auto renewal intervals, and the like) where typical values are not likely to be long enough for the difference between "30 days" and "1 month" to matter. However, there's one other case where, for the sake of consistency, I think it would be good to extend this patch: calculating due dates for non-catalogued items.

Also, I think it would be worth reaching out to open-ils-general to give people a heads up about the potential change and ask for feedback; while I imagine it wouldn't be a huge deal, I know of at least one consortium that has loan durations that are multiples of a month.

[Galen Charlton (gmc)]

I should also mention that I support the general idea.

[Bill Erickson (berick)]

Thanks Galen. open-ils-general emailed. Will look at non-cataloged items shortly.

[Bill Erickson (berick)]

Patch pushed to teach non-cataloged circulations to use the new duration calc code. Rebased to current master. Code still lives in user/berick/lp1635737-due-date-interval-from-pg.

[Bill Erickson (berick)]

Setting 3.0-alpha milestone since the code is ready (assuming no concerns raised on open-ils-general) and for added visibility, but I also understand if we want to give this one more time to percolate.

[Dan Wells (dbw2)]

I think the current direction is workable, but I feel like the original direction still had legs. Basically, go straight from the interval to the DateTime::Duration, no? They are almost the same already, so it seems (from here) to be an easy win. Doing so would also allow for interval_to_seconds to implicitly (or optionally) become interval_to_seconds(...from now, or given date) and in many cases do the right thing in a backwards compatible way.

I am going to give it a go and see how it shakes out. I'm not convinced it will be a better overall approach if it even works, but I feel like there will be some benefit to seeing it play out before making that judgment.

[Bill Erickson (berick)]

Hi Dan, any news on this?

Just to be clear, your plan is to tweak interval_to_seconds() so that it chops up the duration string then passes the duration bits off to DateTime::Duration to calculate the seconds instead of applying the inline hard-coded second amounts?

Beware my branch removes some additional data scrubbing that would need to be recovered so the Perl bits can understand the PG hour/min/sec. (Top of apply_modified_due_date()). Maybe that munging should live in interval_to_seconds as well?

[Dan Wells (dbw2)]

Sorry for the delay.

So, here is some working code along the lines I mentioned. Since interval_to_seconds() lives in OpenSRF, the meat of this version is actually an OpenSRF branch:

http://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/dbwells/lp1635737_interval_to_seconds_contextual

With that in place, the EG changes are of course pretty simple:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1635737_better_interval_to_seconds

There may be more places to use this new skill, but these are the ones highlighted in the discussion so far.

This works pretty well for me, but I do wonder a few things:

1) The new behavior is completely optional. If we feel it is *generally* more correct behavior, we could consider reversing that, and making it the default with a context of "now()", with a compatibility switch for "simple"-version calculations. That would obviously be more disruptive short-term, but if it pays off long-term, maybe worth it.

2) It would be pretty simple to sub out some of the logic to a new "interval_to_duration" function in OpenSRF::Utils. This current branch is great for simplicity of the upper layer changes (and we could keep that as an option), but forcing it all down to seconds is probably some unnecessary work in many cases. We could instead use this new duration directly for the math, and everything would work fine. I think that would actually be truest to the spirit of the original discussion/branch, but the round-tripping of the interval tripped (*ahem*) things up a bit.

I'd like to go ahead and try extracting the "interval_to_duration()" sub-layer if we go this direction, but also wanted to get some "early" feedback on this overall. Thanks!

[Bill Erickson (berick)]

Looks great, Dan. Thanks for putting this together. This is a better solution than my DB approach. I think round-tripping the interval tripped me up too.

1. I prefer keeping the new behavior optional for now.

2. +1 to extracting interval_to_duration().

[Mike Rylander (mrylander)]

I think (2) is a good goal also (along with a, say, interval_to_dates that returns bot the $context and $later objectss), but I'd like to see this in 2.5.1 and 3.0 even more. :) So, I'm going to make that happen now.

[Mike Rylander (mrylander)]

Done and done. Thanks, Bill and Dan!

[Mike Rylander (mrylander)]

After adding some tests, reverting the earlier commits, and some IRC discussion, here are two new branches.

For OpenSRF, we have more tests that show how to properly use DST-aware timezones, and why they are important: http://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/miker/lp-1635737-interval_to_seconds-with-context

For Evergreen, we work hard to supply a DST-aware TZ whenever we're using the new context-date functionality of interval_to_secondes: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp-1635737-interval_to_seconds-with-context

[Galen Charlton (gmc)]

Removing the OpenSRF target for this one, as per bug 1552778 the relevant routines are now in Evergreen.

[Dan Wells (dbw2)]

Reworked and rebased branch for this bug is now here:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1635737_better_interval_to_seconds_2018

working/user/dbwells/lp1635737_better_interval_to_seconds_2018

No changes of substance, just reworking for the OpenSRF to OpenILS method shifting.

[Bill Erickson (berick)]

Pushed another branch with my sign-off's added, plus a thinko/syntax repair patch for the tip commit. One last sign-off required.

I ran through my 10-year circ test and all looks good.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1635737-due-date-tz-test

[Dan Wells (dbw2)]

Thanks for testing, Bill! I have pushed to master and rel_3_2.

Unfortunately, it doesn't port cleanly to 3.1 yet because bug 1552778 has not been merged there yet. I think both should be, and will assign myself for some testing in that regard.

[Dan Wells (dbw2)]

Dependent bug #1552778 has now been merged to 3.1, so merging this as well.