Move money.billing timestamps back to moment of fine

[Dan Wells]

Current code stamps billings into the future, with the underlying concept that the time represents when the billing ends, not when it begins. This is hard to understand, and requires frequent explanation to those not familiar with it. This is especially true for shorter fine intervals, such as hourly, as the last fine will clearly be dated after the checkin time.

Let's instead do the more natural thing and have the first billing timestamped one second after the due time (the time it actually happens now), then every fine interval after that.

[Dan Wells (dbw2)]

Branch available here:

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

working/user/dbwells/lp1422379_move_billing_timestamps

We've been running this code in production for approximately three weeks with no noted problems. The upgrade script in the branch is what we used in our system, but given the complexity of the money schema, it deserves some extra scrutiny.

[Dan Wells (dbw2)]

I think this might be missing a backdate related piece. Pulling back for now...

[Dan Wells (dbw2)]

Missing code pushed to new commit in same branch.

[Dan Wells (dbw2)]

Branch has been rebased to current billing changes in master:

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

working/user/dbwells/lp1422379_move_billing_timestamps_rebase

If backporting, use the old branch; it is functionally identical.

[Ben Shum (bshum)]

Moving to 2.next for next cycle's review.

[Dan Wells (dbw2)]

Jeff Godin wondered via IRC whether some sites would have mixed dating of fines; that is, some moment-dated and others future-dated. While migrated fines might have this issue, I am pretty sure any overdues ever generated by Evergreen are all future-dated. IIRC, the thing which got tweaked a couple times circa 1.6 was exactly when they would generate, but the timestamp for the fines remained constant. I had to dig deep to verify, but you can see the genesis of generate_fines() from way back when here: b155b2af10084d . There we see that the first fine was dated one fine_interval past the due date (rather than the moment of overdue-ness), and subsequent fines built off that.

[Dan Wells (dbw2)]

Ok, I am remembering another wrinkle. I think the extra confusing part is that the perceived effect (for at least some early versions of Evergreen, roughly pre-1.4 or pre-1.6) was a one fine_interval grace period, which some sites wanted, so the code "worked" from that perspective (the fine was dated late, but also billed at the end of each fine_interval).

In other words, any sites which relied on this baked-in one interval grace will have their historical fines not reflect that policy post upgrade-script from this branch. Is there anything we can or should do about this? I am not sure it would be possible to preserve that idiosyncracy in a meaningful way.

[Dan Wells (dbw2)]

Force-pushed an update to the "rebase" branch to account for the negative balances branch changes, etc.

[Ben Shum (bshum)]

Still checking this one out Dan, but so far, I'm not feeling any major red flags that would stop this from going through. Yet.

Adding a tag that we need a release note for this one.

[Ben Shum (bshum)]

I'm a little wary still on updating all the old billing timestamps to reflect new numbers. For us, it might not matter, but I wonder if we should get a few more eyes on this subject before we push it further. Retargeting to 2.next for now, since I'm not sure we'll get enough opinions to meet the beta cutoff, but you never know...

[Remington Steed (rjs7)]

Added release notes, new branch here:

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

[Dan Wells (dbw2)]

Bumping this back to medium, as I consider this a bugfix, and it has been a fairly big problem for us over the years. I think the easiest way to understand the issue is to test with hourly fines, as it is very difficult to explain to a patron why he or she has a fine dated 45 minutes *after* something got checked in (or to get staff to understand what is happening, for that matter).

[Dan Wells (dbw2)]

I've got an idea for a possibly less onerous upgrade script for this fix, which I think is the main objection. Basically, instead of doing the fine-period math for day-granular fines, we could simply set each time component to the start of the day instead of the end. This would ensure that no fines actually change days no matter what their code heritage might be. Thoughts?

[Dan Wells (dbw2)]

No new upgrade script yet, but rebased, conflict-resolved branch pushed to:

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

working/user/dbwells/lp1422379_move_billing_timestamps_rebase_v2

[Galen Charlton (gmc)]

Setting to 2.next; I'm willing to consider calling this a bug fix, but not without testing.

[Michele Morgan (mmorgan)]

I tested this fix using a variety of daily and hourly interval due dates and recurring fines and the money.billing timestamp consistently was set to one second after the due date/time as advertised.

Regarding the upgrade script, I would like to see the version mentioned in comment #13.

Also meriting review are the concerns posted here:

http://irc.evergreen-ils.org/evergreen/2014-01-08#i_57827

[Dan Wells (dbw2)]

Michele, thanks for testing!

Okay, branch rebased again, and with alternate upgrade script. You can get it here:

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

working/user/dbwells/lp1422379_move_billing_timestamps_rebase_v3

[Dan Wells (dbw2)]

This was heavily discussed at the Hack-a-way, and based on that, I am pulling it back to extend the billing data model a bit for even better flexibility. Expect an updated branch soon!

[Chris Sharp (chrissharp123)]

We have seen this for a while in PINES and have just gotten around to reporting it (see the duplicate: bug 1748201). Comment 18 indicated a more comprehensive re-do of billing code... Two questions 1) is there a status report for that effort and 2) would a rebased version of this branch be something we might consider installing locally, as Dan did?

[Dan Wells (dbw2)]

A major piece of this work has now been incorporated as bug 1748924. This branch must now be reworked to take advantage of these new structures, which will ultimately be less disruptive than the original intent to replace them.

I would not advise using the branch posted here in production unless you understand the steps which will be necessary to eventually adjust your data to conform to 3.1+ expectations.