Payment by billing type breakdown

[Mike Rylander]

Teach Evergreen to maintain a payment-to-billing mat-view that maps a payment to all the billings it covers. The code attached here does not know about the 1.6-era controlled billing types, so it needs updating, but is otherwise close to ready after having a solid once-over.

[Mike Rylander (mrylander)]

And here is the IDL entry needed (again, 1.2/1.4 era).

[Mike Rylander (mrylander)]

This is a replacement to the (now removed) SQL from long ago. This one is from the same era, and has a minor set of changes the correct an issue spotted by Jeff Godin back then.

[Jeff Godin (jgodin)]

With some help from Mike Rylander, I'm working on this "vintage" code with the intention of bringing it current and preparing it for possible inclusion.

Current status: the stored proc functions without raising errors in a 2.2 snapshot of production, focusing on transactions with payments made on or after 2012-01-01.

Next steps: output will be checked for accuracy while some of the stated issues (lack of awareness of numeric billing types) is addressed.

Working branch to follow.

[Mike Rylander (mrylander)]

Jeff, do you have this branch handy that I might help in final polishing?

[Jeff Godin (jgodin)]

We've been using mmpbbt in production for several weeks, and I'd like to shoot for getting it ready for pullrequest in time for the 2.6 beta. As such, I'm targeting it and shall endeavor to do some final tweaking with enough time for review.

[Blake GH (bmagic)]

When trying to load this new table with existing data I get many unique constraint errors. I cannot seem to come up with a way of finding those unique constraint breakers. I think it has something to do with transactions containing refunds. I did have success inserting rows into the table by narrowing the query a bit:

CREATE OR REPLACE FUNCTION tmp_populate_p_b_bt () RETURNS BOOL AS $$
DECLARE
    p RECORD;
BEGIN
    FOR p IN
        SELECT DISTINCT xact
          FROM money.payment
          WHERE NOT voided
                AND amount > 0.0
    and xact in(select id from money.billable_xact where xact_start>'2014-01-01')
    and
    xact not in
    (
    select id from money.billable_xact where id in
(
select xact from (
select xact,count(*) from money.billing where amount>0 group by xact having count(*)>1
) as a
)
and id in
(
select xact from (
select xact,count(*) from money.payment where amount>0 group by xact having count(*)>1
) as b
) and xact_start> '2014-01-01'
)
    LOOP
...
...
..

This will only populate transactions since the beginning of the year. Of those transactions, only ones that didn't have multiple billing lines AND multiple payments.

In case anyone finds it useful.

[Blake GH (bmagic)]

Here are some details for one constraint example:

ERROR: duplicate key value violates unique constraint "x_p_b_once"
DETAIL: Key (xact, payment, billing)=(2504059, 188199, 3408744) already exists.

money.payment rows:
188199;2504059;"2014-08-18 16:27:43.876616-05";f;0.25;""
188198;2504059;"2014-08-18 16:27:24.274333-05";f;0.10;""

money.billing rows:
3413280;2504059;"2014-08-16 23:59:59-05";f;;"";0.15;"Overdue materials";1;"System Generated Overdue Fine"
3410418;2504059;"2014-08-15 23:59:59-05";f;;"";0.15;"Overdue materials";1;"System Generated Overdue Fine"
3408744;2504059;"2014-08-14 23:59:59-05";f;;"";0.15;"Overdue materials";1;"System Generated Overdue Fine"

So, it looks like the function has targeted the same billing id twice for the same payment. The 25 cents is paying billing line 3408744 two times. There must be something wrong with the logic or I don't have the correct function on my test machine.

[Blake GH (bmagic)]

I went ahead and put this in my working branch. I committed the original code from Jeff and Mike. Then I committed my changes.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=b37eb844ea81105c6ee7601b397f86c747b321c6

I found that the function was applying payments to the same billing lines. For each payment row, it would start over at the first billing row again. I added logic to keep track of the billing row. I also corrected an issue with voided billing rows by sorting the query by voided.

[Blake GH (bmagic)]

Added the upgrade script

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

We have been running this in production for over 6 months. Many of our libraries enjoy the extra reporting detail that this is providing!

[Kathy Lussier (klussier)]

Adding a comment that this code will need a release notes entry as well as tests. Maybe something to tackle during our community test day.

[Ben Shum (bshum)]

Changing tag to use the new singular "needstest" for consistency.

[Blake GH (bmagic)]

I just committed a change to this branch. We were seeing issues with EG 2.9. Specifically the account_adjustment payment type. Those lines should* match a billing line with an equal amount. The change does a better job of lining those up. Same link
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/blake/LP1174498

[Bill Erickson (berick)]

Removing pullrequest tag, given 'needstests' and 'needsreleasenotes'.

[Blake GH (bmagic)]

Haven't got around to writing tests for this one. However, it looks like I might be hacking on this a bit in order to change the payment_ou to be set to the staff account's OU instead of the patron's OU in order to make it consistent with the "Cash Report" located in Local Administration.

[Blake GH (bmagic)]

Jeff - Where is your version of this?

[Jason Boyer (jboyer)]

Hi! I've taken Blake's branch and had some users give it a good once-over and I think it's finally ready to go. A squashed and rebased branch is available here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1174498_mmpbbt_signoff_pr / working/user/jboyer/lp1174498_mmpbbt_signoff_pr including release notes. I'll admit I'm out of practice at putting together live tests so if we want a test for this I'll need a hand on that front. It's so close! Let's close a 7 year old bug! :)

[Heather Lindskold (heatherlindskold)]

We tested this with PAILS data on our test server and it performed beautifully :)

[Heather Lindskold (heatherlindskold)]

I have tested this code and consent to signing off on it with my name, Katie Greenleaf Martin and my email address, <email address hidden>.

[Elizabeth Davis (elidavis)]

I have tested this code and consent to signing off on it with my name, elizabeth davis and my email address, <email address hidden>.

[Galen Charlton (gmc)]

Pushed to master for inclusion in 3.7. Thanks, Blake, Jason, Katie, and Elizabeth!

[Galen Charlton (gmc)]

Because of at least two issues where the update can break, I'm going to revert this:

[1] bug 1921523
[2] If there are multiple money.bnm_payment rows where the same ID, it can fail like this:

ERROR: more than one row returned by a subquery used as an expression
CONTEXT: SQL statement "SELECT home_ou FROM actor.usr WHERE id = (SELECT accepting_usr FROM money.bnm_payment WHERE id = current_payment.id)"
PL/pgSQL function money.payment_by_billing_type(bigint) line 62 at SQL statement
SQL statement "INSERT INTO money.materialized_payment_by_billing_type (
            xact, payment, billing, payment_ts, billing_ts,
            payment_type, billing_type, amount, billing_ou, payment_ou
        ) SELECT xact, payment, billing, payment_ts, billing_ts,
                    payment_type, billing_type, amount, billing_ou, payment_ou
          FROM money.payment_by_billing_type( p.xact )"
PL/pgSQL function tmp_populate_p_b_bt() line 10 at SQL statement

The second issue is arguably the result of pathological data in the test database where I encountered the problem, but this close to 3.7-beta, it's looking to me like more baking is required.

[Blake GH (bmagic)]

Galen and Jason,

I spent some time on this yesterday and found the issue. It had to do with negative balances. When there was more money in payments than there were bills to pay, it could introduce a duplicate row with a $0 amount. For your consideration:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/blake/LP1174498_payment_by_billing_type_jboyer_base

I also addressed the possibility of getting more than one row from the payment_ou query from your output. I'm not sure if we would see that sort of thing in the wild, but if we do, the function is ready for it :)

I wasn't sure how to handle the git business, so I created a new working branch from jboyer's branch.

-Blake-

[Jason Boyer (jboyer)]

Blake, there was an amount=0.0 guard missing in the upgrade script so I added that to correct an issue I was still having where a user that had all bills voided and account_adjusted away to 0 would still trigger duplicate rows.

My branch is here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1174498_mmpbbt_signoff_pr2 / working/user/jboyer/lp1174498_mmpbbt_signoff_pr2

I'm planning to add pgtap tests so we can verify the function works as designed now and as future changes are made.

That said, it seems exceptionally likely to me that having to add "IF NOT amount = 0.0 THEN ..." everywhere means that it's not data issues that we're running into, but something akin to an off-by-1 error, using < when we mean <=, or other similar logical snafu. So long as all of the (quite possibly ridiculous) tests I throw at it pass then we're probably ok to commit this as-is, but it would benefit from a fine-toothed-combing or refactoring to suss out the actual root cause of these duplicate rows.

[Blake GH (bmagic)]

The loop structure requires that either the payments or the billings are in the outer loop or the inner loop. Payments are on the outer loop, figuring that there are fewer payment rows than billing rows, because people tend to make lump payments. This is issue:

IF billing_remainder >= payment_remainder THEN
....
ELSE
current_result.amount = billing_remainder;
...
END IF

Which, unfortunately is repeated code in different contexts. (I contemplated making another function with that logic that we could call, instead of repeating the lines but I liked that this function was self-contained). The execution falls into the ELSE block and assigns "amount" a 0.0 value because billing_remainder is 0.0 and there are still some greater-than-zero payments we're trying to match. Classic negative balance. I agree, that the loops should exit when we're at the end of the billing rows and billing_remainder is 0.0. I had a choice to either test for that OR introduce the 0.0 guard. I chose the 0.0 guard because it was more full proof. We don't want to write a row with $0 money for any reason (and this is one way we could end up here, but there could be others). I felt that was more solid logic than testing for the single scenario.

Thanks for working on pgTap tests! A much-needed addition. Make this function work for it!

[Jason Boyer (jboyer)]

Good news, bad news.

Good news, adding a few pgtap tests has demonstrated some remaining issues with the current implementation, including a missing trigger on money.account_adjustment. That fix and the current tests have been pushed to user/jboyer/lp1174498_mmpbbt_signoff_pr2 in working.

Bad news, the biggest issue requires a deep change to address. The gist: There is a workflow that is not that uncommon that will lead to mis-assignment of some or all account_adjustment payments to voided billings. Here's how that looks:
Check out item
Item is late and overdues are charged
Staff adjust billings to 0 because *handwave*
Item is marked lost and "Void overdue fines when items are marked lost" is enabled; all overdues are voided
Remainder of bill is paid off in whatever non-account_adjustment fashion, xact is closed.
mmpbt is so determined to assign account adjustments to matching amounts that it assigns them to the voided overdues leaving the lost / lost processing fees only partially covered by the actual payment types.

Comparing mmpbt and billings would show that there are non-voided billings not entirely paid off, where the patron's account would show a 0 balance and a closed xact.

[Michele Morgan (mmorgan)]

Adding a link to follow-up discussion on IRC:

http://irc.evergreen-ils.org/evergreen/2021-09-24#i_492310

[Jeff Godin (jgodin)]

Intending to look at this again.