[OPEN-ILS-GENERAL] Negative balances and void payment types

[Mike Rylander]

Kathy,

I'm trying to make my response here as in-depth as your email
deserves.  Please bear with me as I work through my thought on this.
I've considered it quite a bit in isolation, and I have been following
along with the conversation over the last few weeks, but it is a
complicated issue that requires a great deal of focused attention to
reason about correctly.  I apologize in advance if I misrepresent
anyone's position or goals, but I've tried my best to make my
understanding of the desired result the focus of my response here.

On Wed, Mar 5, 2014 at 12:18 PM, Kathy Lussier <klussier at masslnc.org> wrote:
> Hi all,
>
> Following up on a suggestion Chris Sharp made to the Evergreen developers
> list that more development discussion happen on the public lists -
> http://markmail.org/message/cueotsw7ck7crkvd, I would like to bring forward
> a discussion that has been taking place on
> https://bugs.launchpad.net/evergreen/+bug/1198465.
>
> MassLNC contracted with Jason Stephenson to do several projects in the area
> of billing, including this project that would give libraries more control
> over whether negative balances appear on user's record. As many of you know,
> if you configure Evergreen to void a bill when a lost item is returned and
> the patron has already paid for that lost item, a negative balance is
> applied to the patron's record. Some libraries will refund these lost fees,
> so the negative balance makes sense, but most of our libraries are unable to
> issue refunds. Jason shared his plans regarding the project on this list
> back in July - http://markmail.org/message/rbz6atf7j2zhe2k3.
>
> I don't think there is any disagreement that fixing the negative balance
> issue is a good thing, but there have been disagreements about the approach
> taken.
>
> To approach this problem, Jason changed the way voiding works in Evergreen.
> Instead of entirely voiding a payment, the system would start using a void
> payment type, similar to the cash, credit, and forgive payment types that
> are already in use. This would allow the system to partially void bills that
> would otherwise show a negative balance. Quoting from his original specs:
>
>> I believe that a new payment type will need to be added to handle voiding
>> of bills. This new payment type will replace the current void logic that
>> simply flags bills as voided.
>>
>> This new payment type is needed because the current way that Evergreen
>> voids bills requires that all voids happen in the same increments as the
>> bills themselves. This prevents voiding of a partial bill or a bill that has
>> had a partial payment applied.
>>

This presupposes a definition for "void" which (from a historical
perspective) does not match the intent of the code, though the
statements about voiding are, in themselves, completely correct.  I
will address that below, but I just want to put a pin in this point.

> Upon a week's worth reflection on the original code and the alternate
> approaches that have been suggested, I would like to advocate for the
> approach that Jason originally proposed for this project. The void payment
> type makes sense to me for several reasons:
>
> - As was mentioned in the Launchpad discussion, it provides something akin
> to an accounting ledger of credits and debits. No bills simply disappear
> because they have been voided.
>
> - The void payment type works the same way as other credits on the patrons'
> records, providing a level of consistency to the end user.
>
> - There have been suggestions to rename things to fine_reversal or
> account_adjustment. However, I think "void" is the term that is most
> understandable to our users. Conceptually, when a lost book is returned, the
> billing shouldn't have happened and therefore should be voided. For our circ
> desk staff, there is no distinction in the terminology used for bills that
> are fully voided or partially voided.
>

Please, all, take this with a big pinch of pedant-flavored salt (and
feel free to skip this paragraph if you'd rather not see more pedantry
from me), but there's just no such thing as "partially voided".  It's
correct to say that in a double-entry ledger you can use a balancing
credit to partially or completely cancel a debit, as Jason mentions in
the LP thread, but that's not voiding.  Voiding, as it exists in
double-entry accounting, means exactly "strike through this, because
it was a mistake (or premature or incorrect) and does not match
reality, so we will ignore its effects".  For instance, in QuickBooks
(only because I happen to have the docs for that handy), voiding zeros
the effective amount of the ledger entry.  Now, QB is no paragon of
industrial strength double-entry accounting software, but it's
certainly more right than wrong.  (And, while I hesitate to cite them,
MS Office's docs say basically the same thing about voiding ... though
I'm not sure that helps my case. ;) ).  My point is that "void" has a
well-defined meaning, and in all cases where we could compare what
Evergreen does to another accounting software package, those meanings
align along the "ignore" or "zero-out completely" end of the spectrum.

> - After spending many hours testing this development, I can say the learning
> curve for this new approach is small. Since void payments is using the same
> logic as our other payment types, it is something I believe staff can easily
> understand. When a bill is voided, it no longer disappears on the patron
> record, but remains  with a clear indication that the bill was voided,
> making it very easy for staff to track the history of a bill. I have some
> screenshots at http://www.screencast.com/t/ETwr4HCz0 and
> http://www.screencast.com/t/7zgMIiQqu if anyone is interested in seeing how
> it looks to the end user.
>

For cases where partial billing adjustment is what we want to see (and
what we may need), I do like how that works conceptually.  I also
agree that (excepting the "void" terminology) it is very intuitive.

> I also am concerned about an approach that would make voids work one with in
> a certain set of circumstances and in another way for another set of
> circumstances. One thing that can be confusing for our users is when the
> system does things in a particular way in some cases, but not in others. It
> also makes training and documentation more difficult. I wouldn't want to
> exacerbate this problem.
>

I disagree, particularly when considering Dan Wells' offered changes.

In his branch, the only cases where you will see /anything/ are those
were the patron would get a negative balance otherwise.  In other
words, using today's voids where possible is invisible to the user
(until they go to the billing detail interface, where it's available
if they need that level of detail), and using a new balancing credit
payment type in other cases will be as self-explanatory as your
screenshots show, so the transaction history will be as simple as
possible while matching the requested configuration.

> I agree that there are bugs with this code that needs to be fixed. However,
> having been involved in multiple development project over the past few
> years, I can't say this code has more bugs than other code when it is first
> submitted. Those are fixable. I also understand from the LP discussion that
> the code may affect existing reports. I hope there are ways to mitigate this
> issue, but am I correct in assuming this is likely to happen whenever there
> is an infrastructure change? This bug submitted to Launchpad yesterday
> regarding another recent infrastructure change makes me think it isn't
> unique - https://bugs.launchpad.net/evergreen/+bug/1287967. Does this mean
> we shouldn't sometimes look at infrastructure changes when there is an
> overall benefit to the change?
>

I get your meaning, but (and this is my opinion looking at the code
and its complexity from a risk and maintenance cost perspective) these
are not at all alike in my mind.  But this is an important
conversation to have, and I believe this is an appropriate context in
which to discuss it.

As Evergreen's internals become more and more sophisticated, we will
only run into more situations where subtle but important distinctions
will need to be made between given sets of circumstances.  With that
in mind, I'll attempt to lay out why I see these as incomparable, and
at the same time try to explain how I, personally, weigh the risks,
benefits, and costs of deep changes.

On the one hand, with the changes needed to implement MVF and CRA, we
ran into a non-core, example reporting extension view that no users
have championed (sufficiently) to have it included in the stock
schema.  The fix, which I consider to be appropriate based on how
widely used this optional view actually is and the complexity of
recovering from the change is, "You will need to tell the database
about your custom view again if you still need it.  Here is the SQL to
run: ..."  It is important to note, for this discussion and
comparison, that the changes that caused a problem for the optional
view are transitory -- MVF and CRA do not change the exposed
infrastructure once installed, but (simply due to how Postgres works)
there is an intermediate step that bumps up against that view's
requirements.  Put another way, on either side of the installing
database transaction, all core functionality (and the ability to
support existing extensions) is retained.

On the other hand, with Jason's implementation (excluding Dan's
offered changes) we have a deep and core change to how money is
handled in Evergreen.  This will absolutely break existing money-based
reports, nearly across the board.  There is really no "may" in this
case -- it is assured.  This is not similar to a lightly used (however
important to its users) optional reporting sources.  The fix for this
is almost certainly going to be, "You will need to recreate any
templates and reports you have that use the 'voided' property of the
various tables and views in order to avoid including voided amounts."
The structure of the billing table is fundamentally different on
either side of the transaction, so we can't simply reapply an optional
addition.  Unfortunately, reporting is only the most obvious and
direct consequence that I personally foresee.  I do not doubt I'm
missing others.

To further this discussion, I think another example of a change that
has recently effected non-core features (and has the potential to harm
core features) should be brought up, because it represents something
between the to cases above.  The "floating group" feature modified a
pre-existing field on the copy table and changed its data type.  This
in turn broke the optional Collection HQ scripts.  That feature was
not rejected on those grounds, and I don't think the Collection HQ
scripts should have disqualified it. However, had I been paying
attention to the details of that change, I would have objected to it
on the grounds of risk -- it changes the meaning of an existing,
in-use field, and I will be surprised if we don't end up seeing broken
reports that expect the old datatype at some point in the future.  I
can imagine (as I know many others can as well) at least three
alternate implementations that would end up with the same or greater
features and flexibility, and don't risk existing features.

I want to have this discussion here and in this context because I feel
like it's core to the disagreement at hand.  I agree with you that,
yes, we should be willing to absorb appropriate changes to core
infrastructure in order to improve the software, but I want to provide
examples of the inherent risk that we must weigh with deep changes.
In the case of removing the concept of "void" and its implementation
in the database, the risk is objectively high, and we have not always
done a good job of judging just how high the risks of deep change
might be.

Stated simply, I think this is something we should ask first when
designing the implementation for a new feature: how can we avoid
breaking anything else, and if we must break something, can we replace
it with little or no effort?


Switching gears
============

I'd like to step back for a second and address this from a standpoint
of definitions.  I think to some degree we have been talking past each
other (collectively) and I think some example scenarios would help.

First, my working definition for "void" (as it exists in master today) is:

  *  A billing event should never have been recorded, because it does
not match reality.  However, we will retain the fact that we recorded
it while ignoring its effect.

Consider this sequence of events:

1) item is checked out
2) item goes overdue
3) overdue billings accrue
4) item is declared lost by staff or the system in staff's stead
4a) overdue billings are voided
4b) lost charges of various types are applied
5) item is found
5a) various lost charges are voided
5b) overdue billings are reinstated
6) patron pays their fines

That is the simplest situation (of the "lost" type that also involves
overdues) that can occur.  In this case, voiding makes sense because
it creates the simplest history for the transaction.  However, the
world is never simple, and that's where complication comes in, as in
the following sequence:

1) item is checked out
2) item goes overdue
3) overdue billings accrue
4) patron pays off the overdue billings
5) item is declared lost by staff or the system in staff's stead
5a) overdue billings are voided
5b) lost charges of various types are applied, which are larger in
total than the overdue fines
5c) some amount of the lost charges are effectively addressed by the
previous payments
6) item is found
6a) various lost charges are voided
6b) overdue billings are reinstated
7) patron pays their remaining fines

In this case, voiding is not a problem because the amount paid does
not exceed the amount owed at any point.  The simplest history is
still produced by voiding.  However, conceptually, there's a big
difference, and one where I think the software's current definition
for "void" needs to be refined.  Specifically, we are past the point
where we can say "oops!" because there has been payment activity on
the transaction, implicitly acknowledging the previous billings (of
whatever type) as valid.  I would propose a refined definition for
"void" as follows:

  *  A billing event should never have been recorded, because it does
not match reality, AND the billing event has not been acknowledged as
valid by virtue of the existence of payments against it. However, even
though it is not acknowledged by payments, we will retain the fact
that we recorded it while ignoring its effect.

This presents a problem, because there is no hard link between a
billing and the payment that acknowledges it.  This is partially
addressed from a logical perspective in some code Jeff Godin and I
have offered, but that is not in the mainline, so we can't rely on it
(without incorporating it) in development.  Let's assume, though, for
a moment in this thought experiment that it is in place.  That code
would allow us to identify the state where billings have been
acknowledged by a payment and do something other than voiding them, if
that's what should happen to that type of billing.  This keeps the
history simple (unacknowledged billings go away, but something else
can happen to acknowledged billings to avoid, if desirable to the
site, any negative balance for the patron).

Finally you have a sequence like this, in the current code:

1) item is checked out
2) item goes overdue
3) overdue billings accrue
4) item is declared lost by staff or the system in staff's stead
4a) overdue billings are voided
4b) lost charges of various types are applied, which are larger in
total than the overdue fines
5) patron pays off the various billings
6) item is found
6a) various lost charges are voided
6b) overdue billings are reinstated
7) patron suddenly has a negative balance on the transaction

There are many variations on this theme -- partial payments of all the
various billing totals, etc -- but this is the simplest to analyze,
and is representative.  Working under my refined definition of "void",
the simplest transaction history would be to not voiding any of the
billings (they've all been acknowledged by payment, and so fail our
new voidable test), and instead apply an adjustment "payment" to bring
the balance down to zero, but not past it.  It would look something
like this:

1) item is checked out
2) item goes overdue
3) overdue billings accrue
4) item is declared lost by staff or the system in staff's stead
4a) overdue billings are voided
4b) lost charges of various types are applied, which are larger in
total than the overdue fines
5) patron pays off the various lost billings
6) item is found
6a) lost charges are left in place, because they have been acknowledged
6b) overdue billings are reinstated, because they have been acknowledged
6c) if the balance would go negative by voiding the various lost
billings (a patron credit), which we can obviously detect, an
adjustment payment is applied for up to the amount of the
would-be-voided billings to bring the balance to zero
7) patron has a 0 balance on the transaction at this time
(disregarding possible future billings that may occur, which would
bring the balance positive)

Constraining adjustments in this way extends and refines existing
functionality and definitions, instead of attempting to replace them,
and is much easier to get right in the code.  Put another way, this
covers the corner case without changing the common case, presents the
information you want (as in your screenshots) to the user in those
cases where it is necessary, and avoids breaking other dependent
subsystems.

It is my understanding that this is what Dan Wells' branch aims to do.
 His branch also leverages the majority of Jason's logical changes --
when, how, and to what degree the system should apply a balancing
payment -- but modifies Jason's code just to the degree that it avoids
removing functionality that exists today.

To sum up, I'm concerned for three major reasons, in this order:

 - the risk and cost to all existing users that rely on reports that
have been build, laboriously, over years
 - the risk of not getting things quite right with a reimplementation
of voiding as opposed to a less invasive extension of existing logic
 - unforeseen negative consequences to areas of the code that are not
often touched and have proved stable under current definitions and
workflows

I think Dan's offered branch should really be given a chance, because
it actively avoids these risks, and it seems to me that the middle
ground he has found fits MassLNC's goals to the best of my
understanding, and remains based, to a very degree, on Jason's
implementation.  In short, it seems the safest way forward.

Thanks for bearing with me ... I know this is long, and I don't want
it to sound as if simply trying to negate your points, or that I'm
dismissing Jason's work or MassLNC's testing and the consideration
you've personally put into this feature.  I really do think that a
balancing payment is a good way to go in the cases where we would
otherwise cause a negative balance but should not, and I appreciate
all the time that everyone on the LP bug has been putting into this to
make sure that the end result is the best overall solution.

Now I need to go rest, because my brain is leaking out of my ear.

-- 
Mike Rylander
 | Director of Research and Development
 | Equinox Software, Inc. / Your Library's Guide to Open Source
 | phone:  1-877-OPEN-ILS (673-6457)
 | email:  miker at esilibrary.com
 | web:  http://www.esilibrary.com