Move money.billing timestamps back to moment of fine

Bug #1422379 reported by Dan Wells on 2015-02-16
30
This bug affects 5 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Dan Wells

Bug Description

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) on 2015-02-16
Changed in evergreen:
milestone: none → 2.8-beta
Dan Wells (dbw2) wrote :

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.

tags: added: pullrequest
Dan Wells (dbw2) wrote :

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

tags: removed: pullrequest
Changed in evergreen:
assignee: nobody → Dan Wells (dbw2)
Dan Wells (dbw2) wrote :

Missing code pushed to new commit in same branch.

tags: added: pullrequest
Dan Wells (dbw2) wrote :

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.

Changed in evergreen:
importance: Undecided → Medium
assignee: Dan Wells (dbw2) → nobody
Ben Shum (bshum) wrote :

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

Changed in evergreen:
milestone: 2.8-beta → 2.next
Dan Wells (dbw2) wrote :

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) wrote :

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) on 2015-07-28
Changed in evergreen:
milestone: 2.next → 2.9-alpha
Dan Wells (dbw2) wrote :

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

Changed in evergreen:
milestone: 2.9-alpha → 2.9-beta
milestone: 2.9-beta → none
milestone: none → 2.next
milestone: 2.next → 2.9-beta
Ben Shum (bshum) wrote :

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.

Changed in evergreen:
importance: Medium → Wishlist
status: New → Triaged
tags: added: needsreleasenote
Ben Shum (bshum) wrote :

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...

Changed in evergreen:
milestone: 2.9-beta → 2.next
Dan Wells (dbw2) wrote :

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).

Changed in evergreen:
importance: Wishlist → Medium
Kathy Lussier (klussier) on 2015-12-14
Changed in evergreen:
assignee: nobody → Kathy Lussier (klussier)
Dan Wells (dbw2) on 2016-02-19
Changed in evergreen:
milestone: 2.next → 2.10-beta
Dan Wells (dbw2) wrote :

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?

Kathy Lussier (klussier) on 2016-02-24
Changed in evergreen:
assignee: Kathy Lussier (klussier) → nobody
Michele Morgan (mmorgan) on 2016-03-02
Changed in evergreen:
assignee: nobody → Michele Morgan (mmorgan)
Dan Wells (dbw2) wrote :

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

Changed in evergreen:
milestone: 2.10-beta → 2.10-rc
Galen Charlton (gmc) wrote :

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

Changed in evergreen:
assignee: Michele Morgan (mmorgan) → nobody
milestone: 2.10-rc → 2.next
Michele Morgan (mmorgan) on 2016-05-26
Changed in evergreen:
assignee: nobody → Michele Morgan (mmorgan)
Michele Morgan (mmorgan) wrote :

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) wrote :

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) wrote :

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!

Changed in evergreen:
assignee: Michele Morgan (mmorgan) → Dan Wells (dbw2)
tags: removed: pullrequest
Kathy Lussier (klussier) on 2018-02-08
Changed in evergreen:
status: Triaged → Confirmed
Chris Sharp (chrissharp123) wrote :

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?

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers