Wishlist: Age billing/payment data with circs

Bug #1793802 reported by Bill Erickson
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Wishlist
Unassigned

Bug Description

Wishlist
Evergreen 3.2

When a circulation is aged, any billings and payments linked to the transaction remain in the active billing and payment tables. While this data may be useful for reporting, it has limited value to the application, because detailed transaction data on aged circulations is never displayed in the staff client. Similarly, the link between billings/payments and patrons is severed when the circ is aged, so the data is never included in any patron displays.

I propose we move billings and payments to "aged" tables to keep the active billing and payment tables from growing unbounded. What's more, we should support a purge-after date for the aged billing/payment data so it may be fully deleted after a configurable age.

For simplicity sake, I also propose we avoid duplicating the full money.payment table hierarchy and do something like this instead:

create table money.aged_payment as select * from money.payment_view where ... ;

With this, we retain payment type information, but not all of the per-payment-type details.

Tags: pullrequest
Bill Erickson (berick)
Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
Revision history for this message
Bill Erickson (berick) wrote :

I have some interest in data cleanup prior to upgrading to 3.1, so I put together a patch to implement the above for feedback, minus the purge-interval setting. May revisit that later via another LP.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1793802-aged-billing-payment

Similar to aged_circulation, the new tables are created with indexes and NULL constraints, but no foreign key refs.

Includes pgtap tests and release notes.

Changed in evergreen:
milestone: none → 3.next
Revision history for this message
Bill Erickson (berick) wrote :

TODO:

* Purge payments before billings to avoid removing a billing linked to an account_adjustment before the account_adjustment is deleted.

* Improve delete speed / disable triggers / consider truncate/rebuild of money.billing.

Revision history for this message
Bill Erickson (berick) wrote :

Patch to address the two above TODO items pushed.

The upgrade script now assumes most of the billings will be migrated to aged_billing. Specifically, money.billing is truncated and rebuilt. So now I have a TODO item to document / offer faster options where that is not the case.

Revision history for this message
Chris Sharp (chrissharp123) wrote :

I installed this branch on my 3.0.2(-ish) database and the migration went well. However, when I attempted a purge of circulations, I got this:

ERROR: update or delete on table "billing" violates foreign key constraint "account_adjustment_billing_fkey" on table "account_adjustment"
DETAIL: Key (id)=(225414351) is still referenced from table "account_adjustment".
CONTEXT: SQL statement "DELETE FROM money.billing WHERE xact = OLD.id"
PL/pgSQL function action.age_circ_on_delete() line 44 at SQL statement
SQL statement "DELETE FROM action.circulation WHERE id = circ_chain_tail.id"
PL/pgSQL function action.purge_circulations() line 74 at SQL statement

Does we need an aging table for money.account_adjustment too?

Revision history for this message
Bill Erickson (berick) wrote :

Chris, the action.age_circ_on_delete() trigger function needs to delete payments before it deletes billings. (Swap the last 2 lines of the function). I fixed this in the migration script, but forgot about the circ aging function... I've pushed a commit to the working branch to change the function (and add a minor efficiency improvement to the migration).

Bill Erickson (berick)
Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Revision history for this message
Chris Sharp (chrissharp123) wrote :

Signoff branch at http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1793802-aged-billing-payment

Not sure which branch(es) to apply it to, though. It is very useful to us as we migrate from 3.0 to 3.2, as it reduces the time required for the 3.0-3.1 upgrade script. I'll let release managers decide that.

tags: added: signedoff
Changed in evergreen:
status: New → Confirmed
Revision history for this message
Bill Erickson (berick) wrote :

Beware, if this code is deployed to a 3.0 system, an extra SQL step is required during the 3.1 upgrade to add create_date, period_start, and period_end fields to money.aged_billing.

Bill Erickson (berick)
Changed in evergreen:
milestone: 3.next → 3.3-beta1
Revision history for this message
Bill Erickson (berick) wrote :

Continuing from my previous comment... The new money.aged_billing columns can be created during the 3.0 -> 3.1 upgrade as NULL-able so admins may back-fill the data and re-synchronize the columns at their leisure (i.e. not during an upgrade outage).

Given the extra steps required for deploying to a 3.0 system, however, I propose we not back-port the code to 3.0, unless we find the 3.0 -> 3.1 upgrade to be a big issue for a lot of sites, in which case we'll need additional SQL and documentation.

Dan Wells (dbw2)
tags: added: pullrequest
Changed in evergreen:
milestone: 3.3-beta1 → 3.3-rc
Changed in evergreen:
milestone: 3.3-rc → 3.next
Galen Charlton (gmc)
Changed in evergreen:
milestone: 3.next → 3.4-beta1
Revision history for this message
Chris Sharp (chrissharp123) wrote :

After applying this to PINES production and running it since January 2019, all has been well overall, but I did discover an issue with the Combined Aged and Active Circulations view(s) where they are not linked to these new sources, causing us to undercount bills/payments. I will rebase and update my signoff branch accordingly shortly.

Changed in evergreen:
assignee: nobody → Chris Sharp (chrissharp123)
Revision history for this message
Chris Sharp (chrissharp123) wrote :
Changed in evergreen:
assignee: Chris Sharp (chrissharp123) → nobody
tags: removed: signedoff
Revision history for this message
Chris Sharp (chrissharp123) wrote :

Removed signoff so others can review the full branch including my changes.

Revision history for this message
Galen Charlton (gmc) wrote :

Pushed to master for inclusion in 3.4. Thanks, Bill and Chris!

Changed in evergreen:
status: Confirmed → Fix Committed
Galen Charlton (gmc)
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.