Aged Payment (and Billing) Table Breaks Cash Report and Removes Relevant Payment Tracking Abilities

Bug #1858448 reported by John Amundson
50
This bug affects 9 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
High
Unassigned
3.4
Fix Released
High
Unassigned

Bug Description

Evergreen 3.4beta1+ (Tested on 3.4.1)

LP Bug #1793802 introduced two new tables aged_payment and aged_billing, (along with their "all" counterpart views).

In testing 3.4.1 in preparation for our upgrade, I noticed the aged_payment table in particular strips too much relevant data.

When a payment is aged, it appears to be removed from its corresponding type-specific payment table, such as cash_payment, forgive_payment, etc, and is moved to the aged_payment table.

You can run this query as an example to see that no payments simultaneously exist in the aged_payment table and the bnm_payment table. You can use any type-specific table for pay2 in this query.

select *
from money.aged_payment pay
join money.bnm_payment pay2 on pay2.xact=pay.xact
limit 100

This creates a major problem. The type-specific payment tables include information on Accepting User and Cash Drawer, which tells us who and where the payment was accepted.

This information isn't in the aged_payment table.

So, for every circulation we age that had payments attached, we now lose where the payments were made and who accepted the payments. This is a major problem for CW MARS as we age circulations after 7 days of return. That means a patron could make a payment on a long checked in circulation, and later that day that payment could be aged. As far as Evergreen is concerned at that point, there is no way to know that library accepted that payment, just that it was made somewhere.

Where this is most immediately seen is in the the Cash Report found under Local Administration. This report is severely affected by aging payments. When comparing a monthly cash drawer from just last month on a server running 3.2.8 and another running 3.4.1, the one running 3.4.1 was calculating roughly half of the amount as 3.2.8.

This is a major issue in our network because libraries rely on cash reports to compare their cash drawer and ensure they're taking in the correct amount of money. We also have roughly a dozen reports that libraries can run that list payments accepted by a certain library. These reports are no longer accurate as no aged payments are listed in them since aging payments strips away accepting user and cash drawer, (accepting workstation).

In the short term, we may want to remove this aging function from release 3.4 before more networks go live on the release. Libraries may not be realizing payments are "disappearing".

In the long term, I would argue the aged tables should contain more data so that libraries can track payments made at their library regardless of if the payment belongs to an aged circulation.

There is also precedent for keeping this type of data. The aged_circulation data contains a ton of library tracking data such as checkout and checkin library and workstation, item owning library, etc. Similarly, if we are aging payments and billings, we should ensure a library can keep track of their data.

Just quickly looking through the database and client, I've come up with the following changes to keep the tables relevant and useful:

all_payments and aged_payment tables need:
-- Accepting_User (for both active and aged payments in ALL)
-- Cash_drawer (for both active and aged payments in ALL)
---- Cash_drawer will be NULL for non-drawer payments.
-- Billing (for account_adjustments; for both active and aged payments in ALL)

all_billings and aged_billing tables need:
--Billing_location (for grocery bills; for both active and aged billings in ALL)

For credit cards, there are also the following fields, but it may not be necessary to keep this information:
--cc_number
--cc_processer
--cc_order_number
--approval_code

** OPAC Credit card payments store the patron's ID as accepting_user. When aging credit card payments, we may want to NULL out the accepting_user field or assign it a '1'. Otherwise the link between user and payment/circ is kept: https://bugs.launchpad.net/evergreen/+bug/1417148

Changes to the Reporter:
--all_payments and all_billings need to stay linked in the Reporter from the Aged Circulation source and Combined Aged and Active Circulations source.
--all_payments need to provide a link in the reporter interface to actor.usr via accepting_user and actor.workstation via cash_drawer.
--all_payments and all_billings need to provide a link to money.billable_transaction via xact.

** It needs to be possible to start at Combined Circs and obtain the barcode/login for accepting user as well as the workstation owning library for the cash drawer regardless of if the circulation is aged or active.

Local Admin > Cash Report
--The Cash Report needs to be updated to work with aged payments. Once accepting_user and cash_drawer are added to all_payments, it may be possible to point to this table.

Finally, there are also two views in the reporter schema, (that I'm not sure are CW MARS specific or not). But xact_billing_totals and xact_payment_totals are currently using the active billing and payment tables, respectively. They should be updated to use all_billings and all_payments.

There may be other places in the Reporter or staff client that are affected by these tables, but no others come to mind right away.

Revision history for this message
Michele Morgan (mmorgan) wrote :

Marking this bug confirmed and setting the priority to High as this will have a far reaching affect on cash reporting even when a system does not routinely age circulations. Each time a patron record is deleted their circulations are aged, so any associated payments will also be aged and ability to track those payments will be lost.

To confirm this, I ran a cash report for a library for a 1 month period.

Desk payments totaled $718.80.

I then deleted a single patron whose record had expired and who had paid a number of charges during the same time period. I ran the cash report again for the same time period.

Desk payments now total $589.50.

It's vital to be able to track all payments, aged or otherwise, back to where they were received.

tags: added: reports
Changed in evergreen:
status: New → Confirmed
importance: Undecided → High
Revision history for this message
Jason Stephenson (jstephenson) wrote :

I see four ways of handling this issue, none of them ideal:

1. Add the fields and make the changes requested by John in the bug description.

2. Add an internal flag to disable this feature for sites that do not want to age bills and payments.

3. Re-implement the feature to be its own process, separate from the aging of circulations.

4. Do nothing and leave it up to sites that do not want this feature to remove it themselves.

One, two, or three require changes in the 3.3.3 to 3.4.0 database upgrade script to be made complete. They also require extra database upgrade scripts for sites that have already upgraded to 3.4, which may require special handling to prevent double application of the changes depending upon what the upgrade scripts actually do. There is no way to help sites that have already upgraded to 3.4 with data loss.

Number 4 leaves these changes up to individual sites and means that more sites will have more code to maintain in their local Evergreen forks.

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

+1 to option #1 -- adding the fields proposed by John.

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

Thinking about this a bit more, sites that age circulations very soon after completion (e.g. after 7 days) likely don't want the money data to be aged that quickly, regardless of whether all of the data is tracked. Once it's aged, it becomes read-only and more difficult to access from the interface. 7 days is just too quick for that to take effect.

The aging process assumes you are essentially done with the data. So, while I think adding the columns is a needed improvement, I'm going to start by decoupling the money aging from the circulation aging.

So I propose a combination of Jason's option 2 and 3. Add a flag to enable money aging concurrent with circulation aging (requires opt-in) and provide a way to age money data manually via SQL function and srfsh script.

The setting will be documented with a caveat re: data loss pending future development.

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

Getting started here for review:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1858448-aged-money-additions

This is most of it. Still needs release notes and the manual ager script.

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

Sample srfsh script and release notes also pushed.

Adding pullrequest for eyes. Once the code is reviewed and agreed upon, we can discuss 3.4 back-porting, which as Jason pointed out, will require changes to DB upgrade scripts.

Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
tags: added: pullrequest
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Thanks, Bill! I started looking at this earlier this morning. I'll pull your latest changes and assign the bug to myself for review.

Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Revision history for this message
Michele Morgan (mmorgan) wrote :

I realize a fix is in progress for implementing options 2 and 3, but I would have voted for option 1 to resolve this issue.

I don't agree that the aging process assumes you are essentially done with the data. Circulations get aged, but reports continue to be run off that data, so it would make sense that aged payment data should still allow for reporting.

If the fields specified in the bug description were restored, all the needed data for cash reports and other reporting would be retained in the aged tables. There would be no need to age payments on a different schedule than circs, and no need for a global flag.

If adding these columns is a needed improvement, why not take that approach now?

Revision history for this message
John Amundson (jamundson) wrote :

I agree with Michele. When I initially saw what Bill was proposing, I read it as a combination of option 1 and 2, which I thought was a great idea.

I don't think we should ever lose where a payment was made, regardless of how long ago the transaction occurred. Michele's earlier comment is a fine case for this. A patron could make a payment, and then later that day their account could be purged. That payment would forever be lost for reporting. Another example: Perhaps a network ages circulations after 2 years. A patron owes on a lost item that was checked out 3 years ago. He comes in to pay for the lost item. Because the transaction is over 2 years old, it is aged that night along with the payment that just happened.

The problem with implementing 2/3 is that the user who turns them on might not realize they are losing/missing payments.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

I will take a look at making the changes requested by John and Michele, either on top of the work that Bill has done or as a separate branch.

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

Thanks for the feedback, Michele and John. Bad assumption on my part, then, re: aging payments too quickly.

We can keep my 2+3 branch in our back pocket in case we do eventually need to decouple them. I'm circling back #1 now.

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

Jason, looks like we talked right over each other. Heads up I have a new branch with the #1 changes that's almost complete. Testing now.

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

Branch to add the new fields, tweak related views, and modify the IDL as needed to address the missing fields. A few other minor IDL repairs are included, like fixing the "xact" to point to "mbt" instead of "acirc".

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1858448-aged-money-add-fields

Note I did not make any changes to the aged_billing table. The "billing_location" value lives on the money.grocery table, which is still available / accessible to aged billings.

This branch does not address the Cash Reports interface or clearing accepting_usr for opac credit card payments. For clearing the accepting_usr, we could clear it for all credit card payments, but that's probably overkill. Otherwise, we could clear it for CC payments only if the accepting_usr matches the transaction user?

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Thanks, Bill! We're looking at this on our training system. If we find any additional changes required, I'll make a collab branch.

Revision history for this message
Anna Goben (agoben) wrote :

We cannot upgrade unless there's an option to have alternate retention schedules for circs vs billing. We have different retention requirements from the state for that information. So Options 2 or 3 would be our definite need. Otherwise we'll have to roll back the aged circulations to a much longer term (multiple years), which is not something we want to do.

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

Thanks, Anna. Glad I wasn't imagining that might happen. I'm looking at merging my branches now to support 1 + 2 + 3.

Jason, I'm going to grab the LP from you to reduce confusion.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → Bill Erickson (berick)
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Actually, Bill, I'd rather that you didn't make more changes. I've already setup a branch to test what you've done so far, and I'll have to redo that work. Though, I suppose there's no real harm at this point, since I have not run the modified DB upgrade that I came up with.

We also suspect that a change you have made to the IDL will need to be undone. I was going to supply more detail after we've had a hard look.

My initial reaction to Anna's comment that there is a requirement to age circulation, billing, and payment information on different schedules isn't possible given the current design of the money schema and the inheritance (or lack thereof) of some tables. The present design does not appear to be conducive to having different schedules based on what I see.

I'm actually off today, so won't go into a lot of detail right now. I'll wait to see what further changes Bill makes.

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

Here's a branch which combines the 2 preceding branches:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1858448-aged-money-add-fields-and-settings

Jason, I'll keep an eye out for your feedback. Thanks for reviewing.

Re: aging everything at the same time, we've been aging circs without aging billings/payments up till now. Seems like we're just trying to recover that functionality. FWIW, my branch only ages money data for circulations if the circulation has already been aged.

I'm hands off now.

Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Revision history for this message
John Amundson (jamundson) wrote :

Thanks for your work on this, Bill.

I was able to test many of the changes last Thursday. Here is what I found:

The tables themselves look good now. All the reports I've run directly on the database have led to positive results. The numbers match between servers with and without the tables.

Local Administration>Cash Reports is still incorrect. I expected this. If no one else gets to it, we will probably look into making changes ourselves.

For the most part, the tables integrate with Evergreen Reports well, too. I ran multiple test reports linking between Combined Circulations and All Payments with various payment types. They all matched the numbers from similar reports on a server without the tables.

I do, however, have some concerns with the IDL/Reporter. For example, it is not always obvious to the user if they are accessing the "all tables" or just the active/aged ones. Here are some changes that would be helpful:

-The link between "All Payments" (called "Transactions Payments" in Combined Circs) and "billings" goes to money.aged_billing. It should instead go to money.all_billings.

-The Core Source "Payments: All" still points to money.payment. It should be renamed "Payments" and be removed as a Core Source.

-"All Payments" should be renamed "Payments: All" and continue to point to money.all_payments.

- "All Billing Line Items" (called "Transaction Billings" in Combined Circs) should have a link to "Billable Transaction", similar to the "Billing Line Item" and "Aged Billing Line Item" sources. Note that with a circulation is aged, its row is completely removed from money.billable_xact. This link is still useful for active circulations.

- "All Payments" links to "Aged Circulation" as the only transactional link. There should be a link to "Billable Transaction" and/or "Combined Aged and Active Circulations"

- "Aged (patroness) Circulation" only links to "Payments: Aged" and "Aged Billings" via "Transaction Payments" and "Transaction Billings", respectively. Now that a flag can be set to age payments/billings separately from circulations, these should probably point to all payments and billings instead.

Revision history for this message
Lynn Floyd (lfloyd) wrote :

We are looking at going to 3.4.next when it comes out, but this will hold us off. I do like the idea of different retention policies, and adding the fields to the aged Billing.

Revision history for this message
Terran McCanna (tmccanna) wrote :

Adding needsdiscussion tag.

Is the work that is completed on this good enough to sign off with it and move the additional suggestions to a separate wish list request, or should this wait to be approved until some or all of those suggestions are included?

tags: added: needsdiscussion
Revision history for this message
Lynn Floyd (lfloyd) wrote :

I'm, OK with moving the Cash reports and IDL/Reporter issues to another Wish list bug, if it facilitates moving this one forward.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

The IDL changes should be relatively straightforward, and I'm willing to do them if Bill agrees.

The cash report could definitely be moved to another bug or rolled into bug 1859701. It will just be busted in Evergreen releases that don't get the Angular port. That said, I haven't looked into how difficult it would be to fix this in the current cash report.

Revision history for this message
Bill Erickson (berick) wrote : Re: [Bug 1858448] Re: Aged Payment (and Billing) Table Breaks Cash Report and Removes Relevant Payment Tracking Abilities

Thanks, Jason. I agree with your comments.

> - "All Payments" links to "Aged Circulation" as the only transactional
> link. There should be a link to "Billable Transaction" and/or "Combined
> Aged and Active Circulations"
>
>
In my latest branch, the label for the "xact" field on class "mallp" is
mistakenly set to "Aged Circulation", but the link goes to "mbt". Clearly
the label needs to be fixed.

I think a secondary field that links to "Combined Aged and Active
Circulations" makes sense here to cover all scenarios.

-b

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Thanks, for your comments, Bill!

I'll make the IDL changes suggested by John, along with your corrections. I'll share my changes in a collab branch shortly, so this can be tested more during bug squashing week.

Changed in evergreen:
status: Confirmed → In Progress
Changed in evergreen:
status: In Progress → Confirmed
assignee: Jason Stephenson (jstephenson) → nobody
Revision history for this message
Jason Stephenson (jstephenson) wrote :

I have pushed the branch to collab/dyrcona/lp1858448-aged-money-add-fields-and-settings in the working repository.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dyrcona/lp1858448-aged-money-add-fields-and-settings

This branch has all of Bill's commits, plus one for the IDL changes. The commits all have John's and my signoffs.

That said, we didn't test the settings for aging bills and payments on a different schedule from circulations.

There should also be a conversation about how to handle the upgrade script. At a minimum, I think the code that ages the bills and payment for aged circulations needs to be removed, particularly in light of the new setting. This change should be retroactively applied to the version upgrade script as well as the original feature upgrade. I didn't make these changes because this could be considered controversial, and there needs to be some discussion of what to do with respect to documenting the aging alternatives.

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

Thanks, Jason. Circling back to this now. The collab branch posted in your previous comment appears to be missing the additional signoff's, though. Mind re-posting?

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Sorry about that, Bill! Not sure how I missed the signoffs.

I have interactively rebased the collab branch from my previous comment on master and added the signoffs. Naturally, this required a force push.

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

Thanks, Jason.

Two new branches pushed including my sign-off on the final commit. One for 3.5/master and one for 3.4 (which had a minor seed data merge conflict).

user/berick/lp1858448-aged-money-add-fields-and-settings-1

user/berick/lp1858448-aged-money-add-fields-and-settings-1-3.4

===

The commits from these branches cause the money aging functionality to be disabled, pending administrative action -- applying settings and potentially running an external script. So going forward, the feature will only be active if configured.

For the existing SQL upgrade scripts which contain the original changes, how about we just comment out the few lines of SQL code that perform the auto money aging instead of, say, back porting all of the changes? That will be less disruptive and fixes the core problem of data loss.

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

Modifications to my previous comments...

I propose we disable the feature entirely in the 3.4 series by commenting out / removing the code that migrates the data at circulation aging time and the code that performs the batch data migration in the upgrade script.

I propose we leave the schema changes in 3.4 though to avoid the need for 2 separate 3.5 upgrade tracks (one for those with the schema changes and one for those without). The schema changes will essentially be ignored in 3.4.

I also propose we perform no automatic data migration for 3.5, but instead provide instructions on how to migrate data for the 2 supported scenarios (age with circs, age by date).

I can create patches for these if this seems reasonable.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Bill, thanks for your work on this and for sharing your current thoughts.

I agree with removing code from the upgrade script to automatically age payments and bills for already aged circulations, as well as with providing instructions on how to migrate the data for the 2 supported scenarios.

I am less certain of making the feature totally inoperative in 3.4. At least 1 site has already upgraded to 3.4 and is presumably aging bills and payments along with their circulations. I think it would be OK to leave the feature intact in 3.4.

That said, I'm willing to defer if more people agree with Bill's plan than mine.

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

Thanks, Jason, I hadn't really thought about the existing 3.4 sites who may want to continue using the feature. As to leaving the feature intact in 3.4, do you mean leave it as-is or back-port the modifications (schema changes, settings, etc.) from this bug to 3.4?

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Bill, I mean with backporting the changes to 3.4.

Revision history for this message
Michele Morgan (mmorgan) wrote :

+1 to backporting the changes to 3.4.

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

I'm fine with back-porting to 3.4. As a heads up, though, anyone upgrading from 3.4.3 to 3.5 will have to remove the aged money changes from the 3.5.0 upgrade script. This kind of thing happens sometimes, of course. As long as it's clearly marked in the SQL, it should be simple to comment it out.

Alternatively, if we cut 3.4.3 today-ish, then we could build the 3.5.0 SQL upgrade atop 3.4.3, allowing us to remove the aged money changes from the 3.5 SQL upgrade.

Preferences? I'm fine delaying 3.5 to get this fixed as long as we maintain momentum.

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

Two new branches pushed to the working repository:

user/berick/lp1858448-aged-money-add-fields-and-settings-2
user/berick/lp1858448-aged-money-add-fields-and-settings-2-3.4

These include a commit which disables the first iteration of the aged money migration steps. It comments out the initial migration and the migration which occurs during circ aging. It retains the schema changes (adding the new tables, etc.) so subsequent schema changes will be consistent for all upgrade paths.

These files are modified:

Open-ILS/src/sql/Pg/upgrade/1181.schema.aged-billing-payment.sql
Open-ILS/src/sql/Pg/upgrade/1192.schema.fix_circ_aging.sql
Open-ILS/src/sql/Pg/version-upgrade/3.3.3-3.4.0-upgrade-db.sql

I tested a number of SQL upgrade scenarios successfully, but additional testing is certainly appreciated and recommended.

===

The current plan, once the above working branches are merged to master/3.5/3.4, is to create a 3.4.3 release, which will include the new money aging changes in its 3.3.3->3.3.4 SQL upgrade script. Once that's released we can create the 3.5 RC1 release using 3.3.4 as the starting point for the 3.5 SQL upgrade script.

Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
tags: removed: needsdiscussion
Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
milestone: none → 3.5.0
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Thanks for continuing to work on this, Bill!

I have tested the DB upgrade changes under a variety of conditions, including with a copy of the CW MARS production database. It works for me in all cases.

I have signed off and pushed the user/berick/lp1858448-aged-money-add-fields-and-settings-2 to master and rel_3_5.

I pushed the user/berick/lp1858448-aged-money-add-fields-and-settings-2-3.4 branch to rel_3_4.

If we need to make any clarifications to the upgrade instructions, we can work those out in the meantime.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
status: Confirmed → Fix Committed
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.