Support for Conditional Negative Balances

[Jason Stephenson]

There are a couple of scenarios that cause Evergreen to automatically apply a negative balance to a patron's records.

Evergreen provides a library setting to void a lost item's billing when that item is returned. The policy at many libraries is to void this lost item billing, but only if the lost bill has not yet been paid. If those libraries enable the current setting, when a patron returns a lost item that has already been paid, a negative balance is applied to the patron record. Staff then need to go to Local Administration > Patrons with Negative Balances to view a list of patrons with a negative balance, retrieve each of these patrons, and manually clear the negative balance on their records.

Those libraries that do provide refunds often will only refund the lost payment if the item is returned within x many days of being paid. A setting similar to the "Void lost max interval" setting would be useful for these libraries.

Negative balances can also accrue in situations where a patron partially pays for an overdue fine while the item is still checked out.

An item checked out to the patron is overdue and starts accruing fines.
The patron visits the library and is told he owes $1.00 in the fines that have accrued. He pays the fines at that visit even though the item has not been checked back in.
The overdue fines keep accruing, adding another $2.00 in fines to his record.
The patron reports that he did return the item on time. A check of the shelves confirms that the item had been previously checked in.
Using the "amnesty mode" option, staff checks the item in.
"Amnesty mode" is set to void fines. It voids the total overdue fines applied to the item, $3.00, creating a negative balance on the patron record.

Because a library's policy for refunding bills for lost/long overdue items may be different from its policy for refunding overdue fines, there should be two distinct library settings covering negative balances for both these scenarios.

To implement this enhancement, six new settings will be added to the “Finances” category of the organizational unit setting types. Three settings will specify whether or not a library allows negative balances (refunds) on bills. There will be 1 such setting for overdue fines, 1 for lost item fees, and 1 that will serve as a default setting if an organization's policy is consistent across both fine types. The other three settings will allow a library that does allow refunds to set a maximum interval after which no negative balance is applied. As with the settings prohibiting refunds, there will be 1 setting each for overdues, lost fees, and a default setting.

For more details, see http://www.sigio.com/evergreen/billing2013.html#negativebalances

[Jason Stephenson (jstephenson)]

This has passed local testing, so we're asking for a review for inclusion into master:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1198465-Conditional-Negative-Balances

[Kathy Lussier (klussier)]

Added a needsreleasenotes tag as a reminder to myself to add them to the branch. I'll try to pull something together over the next few days.

[Jason Stephenson (jstephenson)]

I force pushed a rebase and an additional commit to the branch. Testing another branch that depends on this one revealed an interesting situation. Read the commit messages for the gory details.

[Kathy Lussier (klussier)]

Added a release notes entry in a working branch at http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/negative-balance-release-notes.

[Kathy Lussier (klussier)]

I've spent quite a bit of time testing this branch over the past few weeks. I've tested the following scenarios:

* Enabling the default prohibit negative balances setting stopped negative balances from occurring on paid lost items that were checked in.
* Changing that same setting to false produced the negative balances for those same items.
* Setting a negative balance interval allowed the negative balances to be created when a paid lost item was checked in within that interval. Checking a paid lost item in after the interval did not produce a negative balance.
* Prohibiting negative balances for overdue fines while allowing it for lost bills (and vice versa) worked as expected and vice versa.

I've added a sign-off to Jason's testing and added my release notes entry based on this functional testing. I think Jason and I also would like to see a developer who is familiar with the guts of the billing system to look at it too since it does make some significant changes to billing.

My sign-offs are available at:

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

Thanks Jason! I know a lot of librarians who are going to be thrilled with these changes!

[Ben Shum (bshum)]

Still have to test this more fully, but pushing a tiny typo fix for one of the new library setting's description. "long-overde" should be "long overdue"

See: working/user/bshum/lp1198465-conditional-negative-balances

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/bshum/lp1198465-conditional-negative-balances

Will test more this coming week and will follow up with more feedback or signoff/push.

[Dan Wells (dbw2)]

Upon first review this code looks solid. I also know that this can be a prickly area to navigate, so kudos for making it through.

I have two initial concerns:

1) I think we have two more settings than we need. These:

bill.prohibit_negative_balance_default
bill.prohibit_negative_balance_on_lost
bill.negative_balance_interval_default
bill.negative_balance_interval_on_lost

should be sufficient for all possible cases, right? I think this change would make both the configuration and the code in a few places a little bit cleaner/simpler.

2) I mentioned this when the idea first came up, but I wanted to see how things played out rather than rush to judgment. I've really wrestled with it, but my opinion has not changed. As I understand it, the push to turn voids into payments was driven primarily by the desire to allow for "partial voiding", but I think that's conflating two things which should remain distinguished. Those two things are whether a fine is voided or whether it is credited/canceled.

A "void" is very specific and absolute. It says "this didn't happen", but once a fine has been partially paid, it's too late to say that. A "credit", on the other hand, is very grey and flexible. It simply means "we believe you deserve something", and can be because we made a mistake, we value you, we're running a promotion, whatever. It's a simpler concept and mechanism, and is actually quite close to what we are asking the "void payment" to do.

So, short term, I think we should keep this code, but change the terminology to something more akin to "account credit" rather than "void payment". This might even come after commit if we end up agreeing on that direction (so as to not delay either this feature or the beta). Eventually we may want to distinguish a true "void" again, but frankly, with a robust crediting system (which I think we have here), a true void isn't particularly essential for the level of bookkeeping we support.

What do others think?

[Jason Stephenson (jstephenson)]

I've got no problem with removing the extra settings as you see them, Dan. I'll leave that up to Kathy Lussier to discuss if she wants to.

I'd also like to point out that right now, it isn't possible to partially void a bill with this code. Not from the staff client anyway. This was one of the original intentions of the work, but it got too complicated.

I'm not hung up on the terminology if there is something everyone can agree on, I'll roll with it. :)

[Jason Stephenson (jstephenson)]

After looking at this, I think the default setting is the one that ought to go if any of the settings go. It covers both overdue and lost/long overdue. To offer the most flexibility, the setting for overdue fines should remain. Someone might decide they want the setting on overdue, but not lost.

[Jason Stephenson (jstephenson)]

I have not made any of the suggested changes above, but I did checkout Ben's branch, rebase it, and sign off on both his and Kahty's commits. I've pushed to a collab branch, so no more rebasing, please. (That last bit is mostly for myself.)

To <email address hidden>:working/Evergreen.git
 * [new branch] lp1198465-conditional-negative-balances -> collab/dyrcona/lp1198465-conditional-negative-balances

I think we need some more discussion on dropping the default versus the overdue settings before anything is done on that front. I tend to be "settings happy" 'cause I favor flexibility over simplicity.

[Dan Wells (dbw2)]

To be clear about my settings suggestion, I don't think we're losing any flexibility. Of course, I might be misunderstanding something fundamental, so please correct me if needed :)

If we keep:

bill.prohibit_negative_balance_default
bill.prohibit_negative_balance_on_lost
bill.negative_balance_interval_default
bill.negative_balance_interval_on_lost

Ideally, under most scenarios, we only need to set one setting, the "default" setting. This treats lost and overdue the same. On the other hand, if we need lost and overdue treated differently, we set the "default" one way and "lost" the other way (as appropriate for our needs).

I do think it is best to keep the "default" to preserve the "set one setting" behavior.

[Dan Wells (dbw2)]

We have encountered an issue in testing this branch. We attempted to void a bill using our common practice of Right Click->Void All Billings and got a skull-and-crossbones error. The error referenced bill2.js, line 1031, and the initial problem seems to be that the void_all_billings() function in this file still expects bills to have a 'voided' attribute. It's probable that other parts of the function will need reworking, but we didn't go any further with it yet.

I did a quick grep for "\.voided" and found a number of references still existing here and there. Some look to apply to payments, but others were more suspect and could probably use further review. Perhaps at least some are simply in dead code and should be removed.

[Mike Rylander (mrylander)]

I have not tested the code to confirm any difference in output in the face of settings that should preserve the current behavior, but Dan's (2) in comment #7 strikes home hard with me.

That said, I think I disagree with the current push toward less settings. If this goes in soon, I would rather see it go in with the full set of 6 settings. My reasoning is that if this feature set is attractive for a broad set of libraries, but there is no consensus on whether exactly one setting or all six are needed (IOW, different combinations of settings are common), then it stands to reason that the functionality will likely want to grow and extend to other billing types and situations. In that case, the precedent of a "all in" setting, and one per situation or billing type for more granular control, will have already been set. I look at it as one extra knob now so that we can add more knobs in the future without breaking folks' mental models.

It also means less churn before commit.

... my 0$.02

[Dan Wells (dbw2)]

Concerning again the settings suggestion, I'm happy to drop that. I saw it as an opportunity to have fewer settings with the same net effect, since there is some sensitivity in the community of the over-settingization of Evergreen, but it would really be a false improvement, since the code support itself would be largely unchanged. Also, if we suspect more settings might be added, the mental model argument has a lot of weight.

[Jason Stephenson (jstephenson)]

RE Comment #12:

Yeah, looks like we completed missed something there. I'm not sure there is a simple solution for the staff client. I may have to create a new backend method or something.

[Jason Stephenson (jstephenson)]

Well, I did the fliter-branch to add the LP bug number on the new commits, rebased against origin master, resolved 1 conflict along the way, and force pushed to the collab branch.

I'm going to work on getting void_all_bills to work tonight. Hopefully, we can get it in before noon on 2/21.

[Jason Stephenson (jstephenson)]

I think I have the void_all_bills issue resovled. Give it a whirl and let me know.

[Dan Wells (dbw2)]

I spent a large part of the last few days reviewing this, and Jason and I agree that this needs more work. As such, I am removing the 2.6 target, and considering this as "returned with feedback" for hopeful inclusion in the next version of Evergreen.

In addition to the notes already given above, please expect more feedback in the near future. Thanks!

[Kathy Lussier (klussier)]

Hi Dan,

I'm just catching up on this bug now that I'm back from vacation. I'm curious about what outstanding issues need to be resolved that kept this code from making it into 2.6. In looking through the IRC logs, I saw that Jason mentioned something about backdated checkins producing a negative balance. I just tested the following scenario on Jason's server:

1. I enabled the "Prohibit negative balance on bills for overdue materials" for the consortium.
2. I pulled up a patron account with an item that had accrued $1.00 in overdue fines.
3. I applied a $.50 cash payment for towards the overdue fine.
4. I backdated the checkin to match the due date for the item. The remaining $.50 was voided. There was no negative balance on the account.

I don't think we're seeing the same issue where backdated checkins are producing a negative balance? Were your steps done differently?

I know the latest deadline for beta was Friday, but it would be a real shame if this code doesn't make it into 2.6 since there is so much interest in the feature. Are there other issues that still need to be resolved?

Thanks!
Kathy

[Dan Wells (dbw2)]

Hello Kathy,

First, let me clarify how I produced the one issue you ask about. In brief, I did everything the same as you, except I only set the "Prohibit negative balance on bills (DEFAULT)" setting. I am not sure if that's the key, but that's the only thing I see that I did differently. For the record, I also had failures when testing lost-item-returned behavior (it never generated a negative balance even when the settings were false).

In reality, though, these particular testing failures were just two among several factors in why this got pushed back. I'm wearing two different hats here, and am really trying hard to fulfill my duties to each.

As RM, my job is to make sure the release is smooth and timely. Excluding things is no fun for anyone, but the only other option is to keep pushing back deadlines (which directly hurts timeliness) or push in code which may need more development time (which can hurt both smoothness and timeliness). The fact that I find excluding code so distasteful leads me to wear my reviewer/committer hat far more than I really have time to during these crunch times.

As a reviewer/committer, I am just one of many. None of the other committers fully took this on (at least not publicly), so I did my best to give it a fair review. In my initial review on 2/14, I thought we might get away with simply renaming some of the conventions in this code to preserve (at least for the future) the concept of voiding, but as I tried out some of those ideas, I began to feel that we were definitely losing something of value by combining voids/credits into a single implementation, particularly when considering how using this code as "credit" code would necessarily rewrite past void history. In other words, if voids and credits are different, we should keep them separate, and any attempt to combine them is going to sacrifice something from one or both. Now, if the code had been completely seamless and worked perfectly, I would have been more inclined to overlook my misgivings. Instead, I felt in the end that the problems I experienced were at least in part a consequence of these underlying design issues.

This branch seems to imply from the start that the current 'voided' logic is fundamentally flawed, but that is only true if you try to make it do more than it was meant to do. My overall recommendation (as an EG dev, not an RM) for this code would be:

1) Scale back the scope of this change and restore the 'voided' pieces as they were before.
2) Keep the new payment type, but rename it something along the lines of 'account_credit' (still wishing for a better name). (We might also consider renaming 'credit_payment' to 'patron_credit_payment' for clarity.)
3) Rather than attempt to redo all the void logic, make the minimal amount of strategic changes necessary to branch and apply credits in situations where voiding would produce a negative balance. I'd think this would allow us to preserve a significant portion of the work done for this branch, but have it applied in a less pervasive way.

This is all just my opinion of a direction I would be comfortable committing to master. The fewer changes we can make, and the less...

[Ben Shum (bshum)]

Well, I'm late to the party now... but for whatever it's worth, when we had this branch on our test server and people checked in a paid, lost material and the negative balance did not appear (because with how our settings are done, it voids the lost bill and if they paid it, it'll generate the negative balance / credit / whatever you want to call it), there were *a lot* of happy and excited folks in the room. Course, it was only about two minutes after I requested staff feedback at that meeting on the branch testing, that's when it got pulled from 2.6 beta and that kind of killed the moment for them.

We pulled the earlier branch and didn't encounter the void all payments problem that was buggy before, but when I inquired, staff had not tested that piece of it yet. We'll add that to the list of things to review once we add the new commits to our test server.

We are still extremely interested to see further discussion of what needs to happen next for the proposed code. Negative balance issues cause significant problems in our consortium where staff members inconsistently apply payments or partial payments to all sorts of bills that end up being voided by the system in different ways. So, I'm definitely in favor of seeing some sort of path forward that can alleviate many of the issues noted in this bug and other discussions past.

[Kathy Lussier (klussier)]

Hi Dan,

Thank you for the feedback and for taking the time to review the code. I do appreciate the time that was put into the testing since I know it is a hairy area to dive into.

I was able to confirm the two bugs you found. I'm not quite sure what happened with the lost-then-returned-items that did not create negative balances since I know it worked in previous testing, but I'm sure it will be easy enough to fix those two bugs.

In terms of the larger questions you raise, I'm not sure I agree with the idea that these transactions should be considered "credits" or that the previous void logic needs to be maintained. In our opinion, the previous void logic was flawed in the sense that it was an "all or nothing" deal. As Ben mentioned in the previous comment, we have huge issues in public libraries where some kind of payment is already applied to a fine or a fee before the system attempts to void the entire transaction. The void logic needs to be smart enough to know when the entire fee needs to be voided and when just a portion of the fee needs to be voided, and Jason's code does that.

My preference is to maintain the "void" terminology because, as you mentioned, void means "this didn't happen." The situations that cause these void payment types to occur are still situations where libraries want to say "this transaction shouldn't have happened." These are not cases where libraries are saying "we believe you deserve something." We use and will continue to use forgive payments for those situations. When a lost item is returned, we want to say that those lost fees should never have happened and are "voided", even if it now is only a portion of the original lost fee. If an item believed to be checked out to the patron is found on the shelf, what remains of those overdue fines should be "voided" because they shouldn't have happened.

If the consensus is that we need to use another term in place of "void," I strongly suggest that we do not use the word "credit." We already have patron credits in the billing system that can be used as a payment type. Using the same term for two entirely different things is confusing for the end user.

I'm looking at this as an end-user and not as a developer or an accountant, so I may be missing something. However, I also find that Jason's code actually improves the end-user's ability to maintain a history of the voided transactions. When a lost item is currently returned to the library and there are no negative balance scenarios, the billing transaction simply disappears from the patron's record. If a patron, who has received the bill, then pays for it or brings it to the circ desk with a question, the circ person can't easily pull up the patron record to see what happened with the bill. With Jason's code, we continue to see the transaction in the patron's bill history with the last payment type clearly identified as voided - http://www.screencast.com/t/iPd7mbcFGfO. If we pull up full details, we clearly see that the void happened because of a returned lost item - http://www.screencast.com/t/7zgMIiQqu. In the past, we only saw that information in cases where a negative balance was applied.

Is there a way we co...

[Michele Morgan (mmorgan)]

I am in total agreement with Kathy’s comments.

Negative balances have been a big problem for our consortium since day one. It is solely because of the negative balance issue that we don’t automatically age circulations to lost. Negative balances are also a huge problem just with the daily overdue fines many of our libraries charge.

I concur that flaws exist in the current voiding logic. Voiding means it never happened - that makes logical sense. But combining “that never happened” with payments doesn’t make logical sense.

If a bill has been paid, partially paid, or forgiven, voiding the entire bill doesn’t make logical sense. This happens frequently, for example, while checking in using Amnesty mode, overdue fines are voided, regardless of whether they are still outstanding, have been paid or have been forgiven.

Voiding bills that have been forgiven does’t make logical sense. This action credits a patron account for money that the patron never paid.

As Kathy pointed out, the current logic does not support partial voiding, and this functionality is essential, especially in high volume libraries where fines accrue daily.

Regarding the suggestion to keep both the current “void” functionality and add the new payment type, I agree that this would add to general confusion with bills and payment types. We already have six payment types in addition to void functionality:

Cash
Check
Credit Card
Patron Credit
Work
Forgive
Goods

Yet another “credit” payment type while still keeping the existing void functionality would likely make dealing with bills harder rather than easier for staff.

I don’t think I’m providing anything new here, but I just wanted to voice my concurrence with Kathy’s comments and try and convey the impact that the current voiding functionality has on our libraries every day, throughout the day, as they are serving their patrons.

Thanks,
Michele

[Dan Wells (dbw2)]

I'm glad to see more input on this bug. I could respond to a number of points being made, but since my entire stance is predicated on the belief that we can have our void and credit, too, I should probably try my hand a producing a branch which moves in that direction. I'll see what I can do.

[Mike Rylander (mrylander)]

Kathy and Michele,

Thank you for the thoughtful responses!

First, I want to point out that I share Dan's opinions, pretty much to a T. That said, though, I want to address some specific points you bring up (not in order):

> I concur that flaws exist in the current voiding logic. Voiding means it
> never happened - that makes logical sense. But combining “that never
> happened” with payments doesn’t make logical sense.

I disagree that Evergreen is trying to combine them. The fact that the library received money for an event they agree they should not have doesn't mean (from a logic standpoint) that the event now should exist.

However, if the policy is that there can be no refunds, even in the face of "mis-payment", then that is a separate conceptual issue from voiding. Perhaps the implementation of a "shouldn't have happened, but did, and we'll stop trying to accept any more payments on the billing" payment type is more correct (and that's effectively what Jason implemented, IIUC), but that is not voiding from an accounting perspective. In other words, what Dan suggests -- void when you can, but apply a "billing reversal" payment (read: voidish-credity-thing-of-some-sort) when you can't -- seems most correct.

> Voiding bills that have been forgiven does’t make logical sense. This
> action credits a patron account for money that the patron never paid.

I agree completely. In fact, I would say that the forgive payment should likewise be voided. The problem is, of course, that we don't have a direct, persistent link between billings and payments, because there is no one-to-one equivalence ("we billed $0.10 200 times" and "we accepted $20 to cover the balance").

Jeff Godin has been polishing a branch of some of my 1.2-era reporting-oriented code to assist with that linking, and it could certainly be leveraged to logically rectify the problems here. There's much more to the problem than that, but it does provide part of the way forward.

As an alternative, imagine in the "forgive voided billings" case that instead of simply leaving the forgives alone, or removing or voiding them, we void the billings (as today) and then shrink any forgives to the point of $0 (and then remove them) until we either run out of forgives to shrink or the balance hits $0. I think refining the definition of "forgive" to mean a cap instead of fixed amount makes sense, especially in light of the voided-forgiven problem.

This does not cover all cases, certainly, but it would address the forgive-causing-credit issue.

> Yet another “credit” payment type while still keeping the existing
> void functionality would likely make dealing with bills harder rather
> than easier for staff.

I agree that "credit" would be a bad name... but "void" is not right either. What about "billing reversal"? That seems unambiguous and self-explanatory to me ... but, I was involved in naming "conjoined items" and "vandelay", so what makes sense to me may not be useful to front-line librarians. ;)

> When a lost item is currently returned to the library and there are
> no negative balance scenarios, the billing transaction simply
> disappears from the patron's record. If a patron, ...

[Dan Wells (dbw2)]

My branch isn't complete yet, but it's progressing well. I should have something push-able by tomorrow.

FWIW, I chatted with Ben earlier today, and bounced the following table names off him (the interface might not follow these names *exactly*, but they should be kept as close as possible): fine_reversal, reversal_payment, account_adjustment, adjustment_payment. I was partial to fine_reversal, but Ben liked the "adjustment" names better. I am running with "adjustment_payment" for the table name (for now), but would be happy with a "reversal" name as well (it is less generic, which can be seen as either good or bad depending on how many cases we want this type to cover).

[Michele Morgan (mmorgan)]

Mike,

Thanks for your comments on this!

For whatever it's worth, I just wanted to illustrate what I meant when I
referred to "combining" payments and voids as described in this snippet:

 >
 >> I concur that flaws exist in the current voiding logic. Voiding means it
 >> never happened - that makes logical sense. But combining “that never
 >> happened” with payments doesn’t make logical sense.
 >
 > I disagree that Evergreen is trying to combine them. The fact that the
 > library received money for an event they agree they should not have
 > doesn't mean (from a logic standpoint) that the event now should exist.
 >

These may be the wrong words to use to describe it. But here's a use case:

A patron who has items checked out, some overdue, some not, visits the library
and pays off all the bills due in his visit. One bill was $10.00 in overdue
fines for an item the patron still has checked out. The following week is
library amnesty week, when patrons can return anything overdue without fines.
During this week, the patron returns the overdue item. The library checks the
item in using Amnesty mode. This voids the $10.00 fine, and -$10.00 now shows on
the patron's Bills screen.

I did a quick and dirty silent screencast of this with a fake patron record:

http://screencast.com/t/JlGvfklunVDr

Thanks,
Michele

--
Michele Morgan, Technical Assistant
North of Boston Library Exchange, Danvers Massachusetts
<email address hidden>

[Dan Wells (dbw2)]

Okay all, pushed a branch for anyone interested in checking it out:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1198465-conditional-negative-balances

Things worth mentioning:

1) I did a bunch or rebasing of the original branch to hopefully make it easier to digest. I kept Jason's authorship intact on the first group of commits, as there are no substantive changes, but there are significant omissions (to restore the old void code), and a few things renamed (e.g. void_payment -> adjustment_payment). Because of these changes, these commits have no sign-offs other than mine.
2) After that, I have added some commits which mostly move stuff up or down the function stack in order to better bifurcate the void vs "adjustment" logic. There are handful of bug fixes here and there, and some behavior is changed slightly.
3) The behavior changes are for a variety of reasons:
    a) The relationship between the interval and the general "prohibit" setting wasn't clear to me. In particular, which setting takes control if both are set? Because of this, the code currently treats the "generic" setting as the on/off switch to allow for adjustments, THEN considers the interval as a limit on these adjustments. This seemed more natural to me, but upon further review, I think the old code expected you to set one or the other, not both. I think the best thing to do here would be to follow precedent if there are similar overlapping settings already existing.
    b) I have not (yet) enabled the idea of void in every possible case. This is meant to favor simpler behavioral expectations over situational accuracy. Basically, you have three choices:
        1. No setting set = current true-void behavior, with one exception*
        2. "prohibit" setting set = "adjustment" behavior, even if no payments have been made
        3. "prohibit + interval" settings set = void behavior inside interval, adjustment behavior outside
    c) * The one place where the code now always "adjusts" (never voids) is for overdue->lost behavior. Overdue fines are adjusted to zero (if the option is on), not voided. I think this makes sense logically (they still happened, we are just not charging for them), and it makes it much easier to accurately reinstate these fines if the lost item is returned (if *that* option is on).
4) There are almost certainly bugs. This branch has been tested a fair amount, but also changed a fair amount while testing, so new bugs may have crept in in addition to those not yet tested for.
5) There are cases where the "right" thing isn't obvious, or where the system already had bugs. I think we should bring those up as we find them (I'm not recalling every detail right now), but we should also be willing to punt some things down the road, especially if they always existed.

Known TODOs:

1) There are still places where the code and interface say "void" where it isn't really a void anymore. These should be touched up.
2) We should consider adding an "Adjust to zero" right-click option to the billing interface to allow for manual application of this code. As it stands, adjustments only result from automated ...

[Bill Erickson (berick)]

On Wed, Feb 26, 2014 at 6:58 PM, Dan Wells <email address hidden> wrote:
<snip>

> 2) We should consider adding an "Adjust to zero" right-click option to the
> billing interface to allow for manual application of this code. As it
> stands, adjustments only result from automated processes (e.g. triggered by
> checkin, mark lost, etc.).
>

I'm only partially following along at this point, but would that be:
https://bugs.launchpad.net/evergreen/+bug/1249398

--
Bill Erickson
| Senior Software Developer
| phone: 877-OPEN-ILS (673-6457)
| email: <email address hidden>
| web: http://esilibrary.com
| Equinox Software, Inc. / The Open Source Experts

[Dan Wells (dbw2)]

Thanks for mentioning that, Bill. I think that is a big part of what would be needed, but I was imagining a single option which would adjust both ways (positive or negative) as appropriate. It would be for any case where you just want the transaction to go away by applying (or undoing) adjustments to get the transaction to zero.

[Tim Spindler (tspindler-cwmars)]

At least from an end-user perspective, this is also a big problem at our consortium from day and concur with Michele and Kathy ;s statements. Personally, I never liked the void concept at all and would have preferred a design that kept a straight accounting ledger with debits and credits applied. I would also be interested in seeing better linking between billing and payments and I guess Jeff Godin had poked at this issue in 1.2 from what Mike is saying. Michele's scenario as follows precisely describes our problem here.

"A patron who has items checked out, some overdue, some not, visits the library and pays off all the bills due in his visit. One bill was $10.00 in overdue fines for an item the patron still has checked out. The following week is library amnesty week, when patrons can return anything overdue without fines. During this week, the patron returns the overdue item. The library checks the item in using Amnesty mode. This voids the $10.00 fine, and -$10.00 now shows on the patron's Bills screen."

I am concerned also about how changes affect reports but I have not had a chance to test this code so I cannot speak to this issue.

[Jason Stephenson (jstephenson)]

I want to say that the notion of the void payment comes from double entry accounting where you have a ledger of credits and a ledger of debits. If you want to void one or the other, you don't scratch it out from the ledger, you add a balancing entry in the other ledger saying it voids entry X from the other ledger. If you look at bills and payments as debits and credits (and it really doesn't matter which is which) this makes perfect sense.

[Kathy Lussier (klussier)]

Following up on Tim's comment, I agree with his preference for a system where straight debits and credits are applied, which is why is another reason I'm partial to the void payment approach. I agree that the reports issue needs to be addressed, but maybe there's a way we can address that issue while keeping most of Jason's code in tact.

Nevertheless, I also asked Jason to load Dan's code on his server so that I can see how his approach in action. I've come across an error message through the following steps:

- I set the Prohibit negative balance on bills (Default) setting to True for my OU.
- I checked out an item on a patron's record and marked it as lost. The patron's account was billed $12.95 for the item.
- I applied a partial payment (3.01) for the item leaving a balance of $9.94
- I checked in the item. I receive an alert telling me the item is Lost and asking me if I want to override. I click "Yes" and get the following error messages. The item is not checked in and the $9.94 bill remains on the patron's account.

Network or server failure. Please check your Internet connection to jasondev.mvlcstaff.org and choose Retry Network. If you need to enter Offline Mode, choose Ignore Errors in this and subsequent dialogs. If you believe this error is due to a bug in Evergreen and not network problems, please contact your help desk or friendly Evergreen administrators, and give them this information:
method=open-ils.circ.checkin.override
params=["xxxx",{"barcode":"xxxxxxxxxxxxxx"}]
THROWN:
{"payload":[],"debug":"osrfMethodException : *** Call to [open-ils.circ.checkin.override] failed for session [1393943642.761727.139394364230049], thread trace [1]:\nCan't use an undefined value as an ARRAY reference at /usr/local/share/perl/5.14.2/OpenILS/Utils/CStoreEditor.pm line 575.\n\n","status":500}
STATUS:

and then:

Network or server failure. Please check your Internet connection to jasondev.mvlcstaff.org and choose Retry Network. If you need to enter Offline Mode, choose Ignore Errors in this and subsequent dialogs. If you believe this error is due to a bug in Evergreen and not network problems, please contact your help desk or friendly Evergreen administrators, and give them this information:
method=open-ils.circ.checkin
params=["xxxx",{"barcode":"xxxxxxxxxxxxxxx"}]
THROWN:
{"fileName":"oils://remote/xul/rel_3_0_20140228_095954/server/util/network.js","lineNumber":49}
STATUS:

Debug output:

Please open a helpdesk ticket and include the following text:

Tue Mar 04 2014 09:36:37 GMT-0500 (Eastern Standard Time)

Check In Failed (in circ.util.checkin) (3):

{
 "fileName":"oils://remote/xul/rel_3_0_20140228_095954/server/circ/util.js",
 "lineNumber":2954
}

[Dan Wells (dbw2)]

Kathy, thanks for giving it a whirl. Unfortunately, I did not reproduce your error when I followed the steps you gave.

An error that deep in CStore suggests to me something more fundamental is askew. Perhaps autogen.sh needs to be re-run? Or maybe you have some cached IDL data in your client? Please try clearing your client cache and/or running autogen.sh and see if that helps.

Finally, I should note that no effort was yet given to making usable upgrade scripts in my branch, so tests should be done from a fresh DB build.

[Kathy Lussier (klussier)]

Hi Dan,

The server where I do my testing uses a production database, not a fresh DB build, so I won't be able to test this code. However, after thinking over this development project for the past week and discussing it with our end users here, I'm even more strongly supportive of sticking with the original development approach in Jason's branch. I won't outline my reasons here, but, following up on this morning's suggestion from Chris Sharp, I'll send an e-mail to the general list.

[Dan Wells (dbw2)]

Well, if that is the case, I could make an upgrade script. I don't think there would be much to it, it's just fairly typical to not do so while things are still in flux, and I wanted to post the branch for the earliest review possible.

[Kathy Lussier (klussier)]

Discussion carried over to the general mailing list at http://georgialibraries.markmail.org/thread/yji6kuzv2ks3pmej.

[Dan Wells (dbw2)]

For any wishing to try the alternative branch, I made a small fix to the upgrade script, rebased to master, and force-pushed. The upgrade script should be functional now. Feedback appreciated!

[Kathy Lussier (klussier)]

I'm taking a look at this branch now. If I return a lost-and-paid item, I'm still unable to produce a negative balance in a cases where a) the negative balance settings are null and b) the Prohibit negative balance settings on bills setting is set to False.

Dan had reported this issue previously before the new branch was offered. However, since voiding in these cases should now be doing a "true-void" behavior, I thought the problem may have been rectified.

I've just started the testing and will keep adding comments if I come across any other issues.

[Dan Wells (dbw2)]

Kathy, thanks for testing.

Regarding the lost-and-paid testing you did, my first thought would be to double-check that "Void processing fee on lost item return" is set to true (this is not the default).

[Kathy Lussier (klussier)]

Sorry Dan. I usually do a better job of identifying all of the settings that I have set. The "Void processing fee on lost item return" is set to true for the entire consortium in our test database, and the void happens in cases where the lost-and-unpaid items are returned.

[Jason Stephenson (jstephenson)]

As the one responsible for the initial development on this, I'll take it back over and look into Kathy's issue. As I recall, Dan also reported a similar situation in the comments above (or maybe it was in a private IRC conversation?). At one point, I had a hunch what might be doing that, but it has been over a month since I looked at this, and I still need to take into account Dan's changes.

It likely has something to do with the bill having been paid already and the void code skipping paid bills. IIRC, the code was altered along those lines in January or December to fix another issue reported in the initial testing.

[Dan Wells (dbw2)]

Ack, I shouldn't try to answer things late in the day. I meant to suggest checking the "Void lost item billing when returned" setting, not the one I said before. I imagine you have that set too, though. I feel pretty sure that I checked this case, but maybe it got missed.

[Dan Wells (dbw2)]

I also wonder if there is a difference between "Prohibit negative balance settings" settings set to false vs unset (i.e. default false). It might be a matter of not checking falseness correctly.

[Kathy Lussier (klussier)]

Heh, I understood what you were saying even if I repeated the error in my response. I was unable to produce a negative balance in either case: with the setting unset and with it set to false.

[Dan Wells (dbw2)]

Kathy, thanks for verifying!

[Kathy Lussier (klussier)]

While Jason looks at the problem with negative balances, I have a question for Dan regarding the application of adjustment payment types. I'm still hazy about when those payments should be applied. Based on previous discussion, I had been thinking that the adjustment payment type would come into play in any case, except maybe when the entire cost of the item was being voided.

However, I'm finding it works differently than I expected. For example, in one case, I marked a book lost and 3.99 was charged to the patron's account. The patron paid 1.99 of the lost fee and then returned it at a later date. I was thinking this was a case where the adjustment payment would kick in. Instead, the system is voiding the $2 that is left of the lost fee.

It then becomes somewhat confusing for the user because, in the interface - http://www.screencast.com/t/KRmCiO8R, you now see $2.00 in the "Bills" area as a voided transaction and 1.99 in the Payments area for the cash payment. The staff user can't see the original bill amount and needs to do some quick addition to get to that original amount. It seems like it would be better if the bills section continue to show the original 3.99 bill and the payments section showed both the cash payment and the 2.00 adjustment. The payment and bills section would then balance.

Similarly, in a case where the patron pays for the entire item before returning it, the bills portion of the screen changes to 0.00 (presumably because 0.00 was voided). http://www.screencast.com/t/hmMCb6mXE If I were a user, I would interpret this as meaning that the patron wasn't billed for anything.

Was if the intent for the code to void in these situations or was the intent that the system should be using the adjustment payment type here?

We also were wondering if "adjustment payment" could simply be called "adjustment." I think account adjustment (mentioned above) might work too. There was a concern that including "payment" in this label might be somewhat confusing when a payment wasn't actually issued.

Thanks!
Kathy

[Dan Wells (dbw2)]

Kathy, those are indeed some strange screens you have there. I'll need to reinstall from scratch and see if I can recreate what you are seeing.

The earlier branch made changes to the client to preserve the "voided" column display-side, even though it didn't exist anymore server-side. Those changes should not be present in the modified branch, but it appears your client is still trying to do something along those lines.

Again, I plan to reinstall and double-check my memories, but is there any chance you might either have old client code on your server or old client code cached/locally installed on your PC?

[Dan Wells (dbw2)]

Okay, I can confirm what Kathy reported in #47, but with one additional detail. This behavior doesn't happen if "Prohibit negative balances" is set at the CONS level, only if it is set at the branch level. This seems to point to some conflicting logic in the way we check for this setting in various places.

[Dan Wells (dbw2)]

I've pushed a new commit:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1198465-conditional-negative-balances

I am not sure if this fixes everything, but it helps a lot. Basically, in cases of lost items, we were making our bills into a map, then trying to void, and that combination of steps doesn't work.

Also, one thing I want to point out is that the Lost "Prohibit" setting is deliberately pulled from the *copy* circ_lib, not the circ circ_lib. This choice was taken straight from the original code, and I'm not sure how it was specified in the original design specs, but I simply want anyone testing to be aware of this design decision.

[Jason Stephenson (jstephenson)]

As a system administrator, which I am called on to do more often than develop for Evergreen, I'm not fond of a dual approach of keeping the current void functionality *and* adding partial payments. That requires the system administrator to understand what is going on in the code more than either keeping the current void functionality or adding voids as a form of payment. Even with access and some understanding of the code, I'm having a hard time figuring out what is going on with bills and void situations in Dan's branch.

[Kathy Lussier (klussier)]

Sorry for the lag in adding feedback for this bug, but we spent a lot of time testing the new code at MassLNC and I then asked Jason to spend some time looking at the issues we discovered in the code. There are a few issues we found with the code that I'll outline below. Sorry for the forthcoming wordiness, but I want to make sure I provide all of the relevant details.

1. Previously-voided fines cause issues.

Relevant settings: Void overdue fines when item is marked lost - true
Restore overdues on lost item return - true
Prohibit negative balance (default) - true

In this case we were working with a patron record with transactions that had occurred before the code was loaded. The patron had accrued $1.75 in overdue fines, but those fines were voided under the old method when the item was marked lost. A $22.00 lost fee was added to the record. There is a $10 payment towards the lost book fee. After the negative balance branch has been added to the system, the patron returns the book. We get $13.75 adjusted for the return of the lost item. The interface shows that 0.00 in overdue fees were reinstated. We also had a $.05 overdue fee added at checkin (more on that below), and end up with a negative balance of $1.70.

If a similar series of events happens with a transaction that starts after the negative balance branch is added, then we don't produce a negative balance. It appears as if the original voiding of the overdue fines is causing this negative balance to appear. When we update to an Evergreen release that has this code, I expect we will have many, many previous transactions like this one that will need to be handled properly so that negative balances aren't produced.

2. As I mentioned above, a stray overdue fine was added at checkin. Following a similar series of steps with transactions that start after the code was loaded also produces that stray overdue fine at checkin. In this testing, "Lost Checkin Generates New Overdues" is NOT set for the OU, so there was no reason new fines should have been added. Jason noted that there are some places where Dan moved the location of the code to handle the lost and long overdue statuses. It is now in front of some of the other code that may generate fines at checkin. Perhaps the changing of status before getting to the next steps is causing it. The change in question is in commit 08591c75a788f55875e1c3b7a16abe2aa853f3b5 in Circulate.pm around line 3591.

3. If you partially pay a lost book fee and then select "Void All Billings" in the staff client when the Prohibit Negative Balances (Default) is set to "True," a negative balance will be produced. Jason's code would first check the settings and then call a real_void_bills function when a negative balance shouldn't be produced, but it looks like this isn't happening anymore. Although the interface provides a warning that the entire bill will be voided, I think people will expect that the "no negative balance" setting means no negative balances will be produced.

4. I'm not a fan of the way the prohibit negative balance settings interact with the interval setting. I think most users will think "prohibit" means prohibit in all cases and that the i...

[Dan Wells (dbw2)]

Kathy, thank you very much for the extensive testing and feedback. I am happy to look over the major issues you have raised. I haven't had much time for development with the 2.6 release and our subsequent local upgrade, but things have leveled out somewhat, so I expect I can look at this by the end of the week. I'll be sure to post here as things progress.

[Dan Wells (dbw2)]

Testing continues here, but I wanted to report a few findings:

1) I can verify that the code for the particular case of "void"-overdues-for-lost is incomplete. This is the one case in the new code where it now always adjusts rather than voids, but the new restore code still needs to recognize cases where the voiding happened under the old code. Fix is coming soon.

2) I have not yet reproduced a case where an additional overdue was added on checkin. I am focusing on that, since it seems like the largest outstanding bug once #1 is addressed. As I write this, I now realize I might need to have an accrued fine which hasn't been generated yet (meaning the extra fine is of the catch-up-on-checkin variety). I'll give that a try.

Please expect a branch addressing one or both problems tomorrow.

[Remington Steed (rjs7)]

Here is a branch that fixes issue #1 from Dan's comment above (#54). There were two places in the code that needed to be re-taught about the old way of voiding.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/rsteed/lp1198465-negative-balances-fix-old-voids

At this point, I've only tested with overdues that were generated and voided the old way. Following Kathy's scenario #1 from comment #52 correctly produced a balance of zero. We haven't been able to reproduce the stray overdue fine that Kathy said was created when checking in the lost item, but I need to test that more thoroughly.

[Dan Wells (dbw2)]

I'd like to make a few quick comments about potential display code, as I've mostly ignored that until now (in part because we don't want display concerns to unduly effect our model).

The current display has a couple issues:

1) Billings and payments are listed separately, so it isn't intuitive to see how they interact.
2) Void "events" are absorbed into the same visual entries as their original billings, even though they have distinct dates and doers.

I think we would all prefer a more linear display which mixes billings, void events, and payments into a single sequential flow. Does anyone see potential major workflow issues which might be caused by combining these separate lists (i.e. "Bills" and "Payments")?

While the model code branches above still need a lot more testing, I'm ready to give this display some attention, as I think it can actually make the testing quite a bit easier.

[Kathy Lussier (klussier)]

Hi Dan and Remington,

Sorry for the delay with testing again. We've loaded Remington's most recent branch and I re-produced the steps that I followed to produce the error cited in #1. There is improvement, but we're still left with a $.05 fine on the record. Here is what we did:

1. The transaction had accrued 1.75 in overdue fines before it was automatically set to lost back in 2012. The overdue fines were voided, and the patron was assessed with a $22.00 lost fee.

2. I added a $10 cash payment for the transaction today.

3. I then checked in the lost item. The following happens:
   a. $1.75 in overdue fines are reinstated (library has restore overdues on lost item return enabled)
   b. We get an adjustment payment of $13.75 (I'm assuming this is the $12.00 left of the lost book fee + the $1.75 in overdue fines.
   c. An overdue fine of $.05 is generated at checkin time. Above, I referred to this as a stray fine, but, after spending more time on some of our other billing development projects, I now realize that Evergreen typically applies the last overdue fine at checkin.

The patron is left with a $.05 fine on their record from that last overdue fine that was generated. Since the patron had paid $10 to the lost book fee, I would expect that the patron wouldn't have any other charges remaining from this transaction.

Also note that this library has the 'Lost Checkin Generates New Overdues" enabled, but no new overdue fines were generated at checkin. In this example, it doesn't make a huge difference because the patron has already paid above and beyond whatever fines would have accrued, but I am going to do some further testing to see what would happen if the patron had only paid their initial overdue fines and if they would ultimately be assessed the new fines on the lost item return.

What I really think is missing is a really good set of use cases that define all of these different twists and turns for a transaction that then describes which fines/fees a patron should ultimately be assessed given a certain set of library settings. That's something MassLNC could work on if it helps this code move along.

[Kathy Lussier (klussier)]

Following up on my 2nd reported issue in comment 52 (yikes! 52?), I replicated the steps again and believe what I was characterizing as the "stray fine" was indeed that last overdue fine that gets applied when you check in an overdue item. What Dan described as the "catch-up-on-checkin" variety.

I think we're okay here. Unlike the example I revisited in comment #57, the system seems to be handline that fine appropriately. So it just seems to be those historical transactions that are leaving a charge on the patron's record "catch-up-on-checkin" fines.

I still want to do some testing of generating new overdue fines on checkin, but it will take a few days as I need to wait for new overdue fines to accrue.

Also, I just wanted to mention that I have started building that list of test cases that I mentioned above. They are available at http://wiki.evergreen-ils.org/doku.php?id=scratchpad:negative_balances. I'll be sending an e-mail to the list shortly just to see if we can generate consensus around this behavior. In most cases, I think the behavior is fairly straightforward, but, in other cases, it isn't always clear what we should be seeing. In addition to making sure we're all on the same page, it might also help Dan as he looks at the UI.

[Kathy Lussier (klussier)]

Adding a link to a screencast where I show the behavior with historical transactions that I described in comment 57.

Also, I just want to note that I have not yet seen a case where new overdues are being generated on a lost item's return with this latest branch. I mostly have been using those historical transactions. Unfortunately, I made an error when testing for new overdue fine generation on a transaction that starts at a time when the negative balance branch is in place. I therefore will need another few days to re-test that particular scenario.

http://screencast.com/t/KhRbptvzX4I

[Kathy Lussier (klussier)]

Adding another link to a screencast demonstrating item 3 in comment 52, where a negative balance is left if you manually void a bill that has been partially paid.

http://screencast.com/t/qF8LQvnjxGL

[Dan Wells (dbw2)]

Here is an update on progress made on this branch at and since the Hackaway in September.

First, it was determined that some of the "extra" fines were being caused by the way the system was generating fines retrospectively for lost items returned (whenever that option was enabled). Basically, those fines were being added in their own transaction *after* the circ had been otherwise processed, so the new code never saw them. During the Hackaway, Mike and Bill worked together to make a way to bring this fine generation step back into the normal order, which you can see here:
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=e1fdcd3a6885baac3f86402e330aef3d8b36c681
and here:
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=d2a521c0ff32e9921bfc93cb86b2c917e5eda92e

These commit were improvements no matter what else happens, so I have already signed-off and pushed them to master. With those in place, I have now force-pushed a rebased version of this bug's 6/18 branch to here:
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1198465-conditional-negative-balances

While I think this solves the biggest known processing issues, we still have problems with overall understandability of the interface. It isn't quite ready for testing yet, but as mentioned at the last dev meeting, I have two branches nearing test-worthiness which will do the following:
1) Adjust the bill dating logic to date bills at their inception rather than their conclusion. This is necessary to make a sensible timeline which includes both bills and payments. The code is finished from the fine generation angle, but still needs work to create an upgrade script.
2) With the first branch in place, alter the detailed bill display to combine bills, payments, and voids into a single timeline view.

My goal is to have these branches published by (American) Thanksgiving.

[Kathy Lussier (klussier)]

Thanks Dan. I tested the code again today, but I'm still finding problems with the "Lost Checkin Generates New Ovduers

[Kathy Lussier (klussier)]

Sorry - I accidentally hit "Post" before I had barely gotten started on the last comment.

I tested the code again today, but I'm still finding problems with the "Lost Checkin Generates New Overdues" setting. I don't know if this was a problem introduced with the code merged to master earlier this week or if it only occurs with the negative balance branch. I would like to do some more testing next week without the negative balance branch loaded so that we can pin that down.

Here are my findings. I was working on a transaction similar to the one from above.

Relevant settings:
• Void overdue fines when item is marked lost - true
• Restore overdues on lost item return – true
• Lost checkin generates new overdues - true
• Prohibit negative balance (default) - true

The circ transaction below should have a max fine amount of $3

1. The patron had an item that accrued $2.40 in fines before being automatically set to lost on 11/14/13.
2. The fines were voided using the old method because it was done before the negative balance code was added.
3. A $20 lost fee was added to the patron's record.
4. A payment of $2.40 was applied to the record.
5. The lost item was checked in.

My expectation is that the user would now owe $.60 because a) the lost book fee has been adjusted b) the original $2.40 overdue fine should be covered by the cash payment and c) the user should only be charged another $.60 before hitting the max fine amount.

Instead, in addition to reinstating the original overdue fine of $2.40, which we expect, the system added new overdue fines from 11/15/14 through 1/14/14. The user is ultimately charged another $6.00 on the record.

Like I said, I plan to do more testing on this next week on a master system without the negative balance branch to see if this is problem in all of master now or if it's just a problem with this particular branch. I'll post my results as soon as I'm done.

[Kathy Lussier (klussier)]

I've confirmed that the above problem is happening in master without the negative balance branch loaded. I have filed a separate bug at https://bugs.launchpad.net/evergreen/+bug/1393533.

[Dan Wells (dbw2)]

The bad news is that I didn't make my self-imposed deadline of Thanksgiving for the display branches. The good news is I now have fine generation working with a cstore backend. As I had expressed on list, I think will help us avoid a lot of the traps we were hitting with running separate simultaneous transactions.

I am assigning this back to myself, and will have something to show for it soon.

[Dan Wells (dbw2)]

Okay, a new branch is now up at:

user/dbwells/lp1198465-conditional-negative-balances-cstore-refactor

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1198465-conditional-negative-balances-cstore-refactor

In this branch, the newest code can be found in both the first four and the last five commits. The first four commits involve the switch to cstore for fine generation, and the last five are more or less refactoring of the circ code to take advantage of the cstore switch.

I would call this code a rough draft. It makes quite a few changes, and doesn't blow up on basic operations, but testing is ongoing. Any and all feedback is very much appreciated.

[Dan Wells (dbw2)]

Haven't gotten any feedback on this yet, but that's understandable with the holidays. I'm going to push hard to get this bug resolved for 2.8, so I am targeting it as requested.

I think this currently conflicts with a recent minor billing bugfix in master, but I also think we should proceed with testing/developing it as-is, and reintegrate once we get closer to fruition.

[Jason Stephenson (jstephenson)]

Now that https://bugs.launchpad.net/evergreen/+bug/1406367 has gone into master, the cstore refactor branch has a conflict in Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm.

I am unsure how to resolve this.

[Dan Wells (dbw2)]

Ok, I refactored the Cstore changes out of the big branch to stand on their own, and also resolved all current conflicts in master for that portion of the code. This code is tested, but the branch is not (if you know what I mean). I will test soon and report back again, but wanted to get this out in case any brave souls wish to forge ahead:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dbwells/1198465_cstore_fines_gen

collab/dbwells/1198465_cstore_fines_gen

Sincerely,
Dan

[Dan Wells (dbw2)]

The cstore branch above unfortunately conflicts in a non-trivial way with part of recently committed bug #1413592. I'll try to work out a reworked version ASAP.

[Dan Wells (dbw2)]

Rebased and reworked branch now at:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dbwells/1198465_cstore_fines_gen_rebase

working/collab/dbwells/1198465_cstore_fines_gen_rebase

[Bill Erickson (berick)]

I pushed a fix to the collab branch to repair batch fine generation (via fine_generator.pl). The problem was introduced w/ latest fine generator commits.

Once that was done, fines generated as expected. However, one concern I have for batch/nightly fine generation is that in the new code, the commit does not occur until after all fines are generated. This could be a very long running transaction if run once a day, which could cause problems if other batch processes are accesses these transactions. I can push a patch that opens/closes a (db) transaction for each (ils) transaction processed if that sounds sane.

[Jason Stephenson (jstephenson)]

That sounds sane to me. I typically do transaction on a (ahem) transaction level in my own scripts.

[Bill Erickson (berick)]

Pushed branch to do per-transaction transactions w/ some minor cleanup of commented-out code.

[Bill Erickson (berick)]

Oops, pushed a commit, not a branch.

[Bill Erickson (berick)]

Found one more issue in my testing. I saw this while testing LOST behavior, but can boil it down to a much simpler test:

1. Manually void all fines on an overdue transaction.
2. Run the fine generator
3. The circulation now has a new set of non-voided fines, analogs to the manually voided fines.

IOW, it's creating new fines were voided fines already exist.

[Bill Erickson (berick)]

I think I see the issue.. patch en route.

[Bill Erickson (berick)]

Fix pushed. I boiled down to billings not getting sorted properly when retrieving them from the database.

[Bill Erickson (berick)]

Lightly squashed branch with sign-off's for just the cstore billing bits pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/berick/lp1198465-cstore-fine-gen-squash

I have a commit on the tip which still requires sign-off.

[Dan Wells (dbw2)]

Okay, pushed the cstore bits to master. Thanks so much, Bill, for the testing and fixes. Will try to rebase the rest now to master and see how it goes.

[Dan Wells (dbw2)]

Rough rebase attempt now at:

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

This still needs TLC, but throwing it out there for any willing eyes.

[Dan Wells (dbw2)]

A couple more supporting pieces got into master (along with two fixes), so here is a rebased branch:

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

working/user/dbwells/lp1198465_cond_neg_bal_20150220_rebase_20150415

[Kathy Lussier (klussier)]

I put the latest branch through some of my test cases. I'm still seeing trouble with getting the "Lost Checkin Generates New Overdues" OU setting to behave properly. When I check in a lost item that should generate new fines, those fines are not being generated.

In addition to testing on master with the negative balance branch loaded, I ran the same test through a 2.7ish system with the same dataset just to verify what pre-existing behavior was.

Here's the use case. Relevant OU settings:

Void overdue fines when item is marked lost - true
Restore overdues on lost item return – true
Generate new overdues on lost item return - true
Prohibit negative balance (default) - unset

I was testing on a transaction that had accrued $1.75 in overdue fines before being marked as lost. $1.70 had accrued before it was marked as lost and another $.05 was added. The item was marked lost on April 13, 2012 before the negative balance branch was loaded. All of those fines were voided using the old method, and a $22 lost fee was added to the record.

I added a cash payment of $10 for the transaction, leaving $12 of the lost fee. I then check the item back in on 4/26/15.

I am left with a balance of -8.20. The system the earlier overdue fines (1.75) and added one more overdue fine with a date of 4/27, totaling $1.80 that was deducted from the $10 that was paid.

Alternatively, when I follow the same set of steps on the same record on the 2.7ish system, I am left with a balance -7.00.
In this case, the $1.75 in previous overdue fines where unvoided, and new fines are generated from April 14, 2012 (the day after it was originally marked lost) through May 9, 2012, at which time we hit the $3 max fine threshold and stop accruing fines.

Let me know if you need any more details!

[Dan Wells (dbw2)]

Okay, two things.

First, I have a fix for the situation reported in comment #83. From the commit:

LP 1198465: Set restored overdue timestamp to time of last overdue
When we have our settings configured to generate new overdues on lost
item return, we start generation after the most recent overdue fine.
Because of this, we need the restored fine to be dated in the past,
which in turn allows the fine generator to apply catch-up fines as
expected.

You can find that commit, plus one more to clean up some variable names, key names, and comments, in a rebased branch here:

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

BUT...

The second thing I want to say is that I plan to attempt to significantly simplify this branch. Having lived with this code through various mutations over the years, I feel perhaps we're being more heavy-handed than we really need to be to accomplish the stated goals. Remington and I are both setting aside time to work out a comprehensive set of tests (we have one working so far, woot!), and I am going to use those tests to experiment with trimming this code down to the bare essentials. Because of that, I am leaving this assigned to myself for now.

In the meantime, if anyone wants to take the last patches for a spin, please do. After all, there are no guarantees that simplification will work out, so it would be nice to have this current code "in our back pocket" when the time comes to make a move.

[Kathy Lussier (klussier)]

I just successfully tested two scenarios like the one in comment #83. In one use case, the prohibit negative balances setting was unset and, in the other case, it was set to true. In both cases, the system behaved as expected.

 When the negative balance setting was unset, overdue fines were reinstated and new ones were added as expected. The remaining balance was the same as it would have been if the code had not been loaded. When it was set to true,the fines were reinstated and new fines generated, but there was no negative balance left on the record.

Overall, it looks promising, but I would like to go through all of my use cases again to make sure the various library settings are working as expected while Dan and Remington work on the other branch. I will make the time to do so this week.

Thanks for sticking with this Dan!

[Dan Wells (dbw2)]

Thanks so much for testing, Kathy. I'm glad to hear it is working so far!

Just to follow up on my last comment, after some more review, I no longer think there are big wins to be had in refactoring, so the branch is potentially stable. That said, the overlap of this code with parts of bug #1174498 probably does warrant further poking, probably from both sides.

Also test code *is* still forthcoming.

[Kathy Lussier (klussier)]

I've run the latest code through my series of tests. For reference, the test scenarios are available at http://wiki.evergreen-ils.org/doku.php?id=qa:billing_test_cases. The code is looking great! I only encountered problems with one test (more on that below.)

I can confirm that the code now no longer has trouble with generating new overdue fines on a lost checkin. Looking back on the six issues I identified back in comment 52, number 1 has been fixed, number 2 was user error on my part, and number 5 was a UI issue that I think can be addressed at some time in the future. This leaves us with the following remaining issues which I'll list in order of importance:

1 - Manually voiding a partially-paid lost transaction continues to produce a negative balance when the "Prohibit Negative Balances" OU setting is enabled. This test is the one mentioned above where I did not have success. Although the interface provides an alert saying that the entire bill will be voided, I think people will expect that the "prohibit negative balance" setting means no negative balances will be produced in any situation.

2 - We prefer not to use the label "adjustment payment" for adjustments that are made when a lost item is returned. Our concern is that staff will see the word "payment" and think an actual payment has been issued. Our preference is to use something like "adjustment" or "account adjustment." I would love to hear what other Evergreen sites would like to see here for a label.

3 -We don't like the way the prohibit negative balance settings interact with the interval settings. With the current code, if a library wants to only allow negative balances for 30 days after payment is made, they must enable the "Prohibit Negative Balances" setting and then set the interval setting to 30 days. I think most users will think "prohibit" means prohibit in all cases and that the interval settings would be used when the negative balances weren't prohibited. It would be more intuitive if the interval settings worked when the prohibit settings were either Unset or set to "false." In either case, we should probably provide some guidance on what needs to be done in the description for the interval settings.

FWIW, I don't think any of the three above issues are deal breakers for putting the code in. However, since we do have time until the beta cutoff, it would be great if we could make it a bit more intuitive for the users.

Thanks so much Dan! Despite the three issues I identified above, I want to reiterate that, overall, everything looks great. I'm very excited to see this code make its way into 2.9!

Kathy

[Dan Wells (dbw2)]

Just a quick update: I'm not going to make my self-imposed deadline of getting tests pushed out today. Apologies to all eager beavers among us. We did manage to get this to the top of the queue (which is often the critical step), so it's reasonable to expect something on Monday or Tuesday next week. Have a good weekend, all.

[Dan Wells (dbw2)]

Just to inch this ball forward, I'm posting to say that Remington has now taken the reigns on working out these tests. He was out sick yesterday, but made good progress today, so we're getting close to something clean and repeatable. He is trying to emulate Kathy's posted test "scripts" directly, and things are looking good, so I think we'll soon be on solid ground. Thank you for your patience!

[Dan Wells (dbw2)]

Okay all, pushed a couple more commits for the testing work Remington and I (mostly Remington) have been done (so far) out to the current branch:

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

working/user/dbwells/lp1198465_cond_neg_bal_rebase_20150529

This work-in-progress for testing the conditional covers 9 of the 14 test cases listed here (as of today):

http://evergreen-ils.org/dokuwiki/doku.php?id=qa:billing_test_cases

NOTES AND TODOS:
- The test is currently an SQL setup file plus a Perl live test file. You must first load/run the SQL, then the Perl live test. One simple improvement would be to ditch the SQL file and switch the setup process to cstore calls within the Perl test file. This would be both more contained and more robust.
- A second step to more advanced and useful tests would be to use higher-level API calls to create portions of the setup rather than doing everything manually. However, some test conditions cannot be reasonably setup with the higher-level API calls (e.g. bills of a specific age, or bills using a legacy format no longer generated by current code), so certain areas will likely always require direct manipulation.

We plan to keep pushing to get the rest of the tests in, which should go more smoothly now. Improving the tests as explained above will be a secondary priority.

Feedback welcome!

Sincerely,
Dan

[Dan Wells (dbw2)]

Remington was able to finish up the rest of the missing tests, and everything is pushed. All in all it comes through as 127 test checks, so not too shabby :)

As I mentioned in the first test commit, there is still room for improvement in making the tests themselves more future proof, but I think what we have so far is a very valuable effort.

[Dan Wells (dbw2)]

P.S. I should mention that Kathy's concerns in comment #87, while all valid, have not yet been addressed.

Starting with her highest priority concern, I'm hoping for more conversation on that, as I am uneasy anytime we take control away from the user wholesale. My first thought would be to let the current code and settings target the automatic behavior (as it does now), as that seems to the primary source of grief. Then we could go ahead and expose a user option to "Adjust Bill to Zero" and use permissions to control who can void and who can zero bills.

Going this route would reinforce that these are separate ideas, and also gives libraries the option to keep the user in control as situations and policies dictate.

[Kathy Lussier (klussier)]

If there is a permission that prevents people from manually voiding bills in environments that never want to see a negative balance, then I would be in favor of the approach Dan suggests. Are you thinking these would be two distinct permissions:? One for adjusting to zero and one for voiding. I think our sites would be likely to allow frontline staff to adjust to zero, but not to void, so I do think they would need to be distinct.

Dan, are you suggesting that we merge this code as is and tackle the other issues with new LP bugs?

Also, I just want to mention that I would be willing to do the code to change the "adjustment payment" labels to "account adjustment" or any other label we might want to use. I'm just not sure there is consensus around what works best for a label. I know "account adjustment" was one of the labels under consideration way back in comment #26.

[Kathy Lussier (klussier)]

Adding a link to additional discussion in IRC today:
http://irc.evergreen-ils.org/evergreen/2015-07-22#i_190842

At this time, we are considering swapping the current option to manually void with an option to "adjust to zero."

Feedback is welcome!

[Kathy Lussier (klussier)]

Signed off on the code from Jason, Dan and Remington and then added a commit that 1) changes "adjustment payment" to "account adjustment" as described above and 2) removes references to credits in the description for the OU settings. Patron credits are different than negative balances and are not impacted by the code in this branch.

Working branch at

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

[Ben Shum (bshum)]

Commit pushed along to master for inclusion towards 2.9. Cheerio.

[Kathy Lussier (klussier)]

Adding links to separate LP entries for the two remaining issues we found in testing:

Replace manual void option with "adjust to zero" option - https://bugs.launchpad.net/evergreen/+bug/1479107

Negative balance settings used in combination with one another should interact differently - https://bugs.launchpad.net/evergreen/+bug/1479110

Thanks to everyone who worked on getting rid of negative balances in Evergreen (for those who want to get rid of them)!

[Steve Callender (stevecallender)]

I've created a new bug that appears to be related to these changes.

https://bugs.launchpad.net/evergreen/+bug/1593815

Steve