xact_finish (prematurely?) set on claimed returned item if transaction balance reaches zero

[Jason Etheridge]

I believe this behavior has always existed with Evergreen (1.2 through trunk). A checked out item that is marked claimed returned retains a status of Checked Out, and gains a stop fines date and a stop fines reason of claimed returned. The transaction remains open. If any payment are made toward that transaction such that the balance owed is 0, then the transaction is closed, and the item status remains Checked Out. It no longer shows up on the patron's account. This is arguably non-intuitive, though I believe it was meant as a means of resolving claimed returns (I think a lot of libraries "check in" such items, though I don't believe that was intended for any case except when the item was actually found).

Here are some thoughts from an Evergreen staff user, quoted with permission:

On 1.6 with "checked out" as the status for claimed returned items I don't
think the transaction should be closed whether there is bill or not and whether
the bill has been resolved or not. For any checked out item there should be
linked open circulation.

With the new option of choosing a status for items marked claimed returned (new
entry on Library Settings Editor) on 2.0, things are indeed getting complex. I
think item's status should play a role.

On 2.0 the default status is still checkedout, so the circulation should remain
open regardless of bills. As we always tell our sites if the item has
checkedout status the transaction is not over yet though the patron claimed
returned it. ClaimedReturned is an action and one step in the whole process.
How to continue the process depends on whether the library buys the patron's
story. If yes, they should check in the item then mark it missing. If no, mark
the item lost and charge the patron.

The new library setting, Claim Return Copy Status, allows library to choose a
status for claimed returned item. I tested a couple and notice that there is no
consistency between the item's status and the circulation transaction. A
Missing or Temporarily Unavailable item can be still in a patron's account. It
is possible to have an Available item in a patron account. Though we believe
our library staff are sane enough not to make it happen, but they can if they
want to. Besides those two type of status can be edited in copy record. But
Lost and CheckedOut can not due the linked transaction. If status is changed,
the circulation record should be taken care of. (Personally I feel confused by
this new setting.)

To simplify the issue, I think maybe there should be a new status,
ClaimedReturned. Items being claimed returned should all have this status. It's
clear to both patron and staff what has happened to the item. As closing the
circ or not, it should be invidual library's choice. If the library chooses to
close the circ, put in a checkin date and fine stop reason of CR and
xact-finish if no bill. This is the end of the process. (If the item surfaces
later, checking it in will just change the status. The checkin date in the circ
will be the real CR date.) Libraries may use the CR count to control how many
times CR can be done for each patron.

If the library chooses to keep the circ open, they need to follow up. Now their
option is marking the item lost and charge the patron some time later. (If they
don't buy the patron's story now they should not buy it later. But they always
have the choice of checking in the item then marking it missing.) So in this
scenario there should not be checkin date but the fine stop reason in circ on
marking claimed returned.

[Rogan Hamby (rhamby)]

This is an ongoing issue for SC LENDS also. We have been discussing the intent of the software design as well and are still undecided but are currently leaning towards the design perspective of creating a new status as well.

[Steve Callender (stevecallender)]

It looks like it's not just claims returns, but if there is any stop_fines set on a transaction, the transaction will close if the billing is balanced out.

For example, an overdue item hits MAXFINES. The billing is paid before the item is checked in. At the point the billing hits 0, the transaction is closed. The item still remains checked out. Now if an attempt is made to renew the item, there will be an error. "No open transaction found". If the item is checked in, the system treats it as if it was an available copy being checked in and puts it in reshelving. The old transaction that was prematurely closed never gets a checkin_time.

It looks like the problem may be in the make_payments sub in Money.pm,

            if(!$circ || $circ->stop_fines) {
                # If this is a circulation, we can't close the transaction
                # unless stop_fines is set.
                $trans = $e->retrieve_money_billable_transaction($transid);
                $trans->xact_finish("now");
                if (!$e->update_money_billable_transaction($trans)) {
                    return _recording_failure(
                        $e, "update_money_billable_transaction() failed",
                        $payment, $cc_payload
                    )
                }
            }

It's a fairly important bug, since technically a patron could keep an item forever if they just pay the maxfines on it and never bring it back to the library.

[Jason Etheridge (phasefx)]

> It's a fairly important bug, since technically a patron could keep an
> item forever if they just pay the maxfines on it and never bring it back
> to the library.

But lest any patrons reading this get such an idea, we can run reports
to detect the situation until the bug gets fixed. :)

[Ben Shum (bshum)]

Finally crossed this one in our production system. Scratched our heads for a bit. Assigning targets as it affects 2.3 and probably 2.2, 2.1, etc.

[Jason Stephenson (jstephenson)]

I have written a branch that seems to address this. Testing appreciated:

master:

working/user/dyrcona/lp793550

backport to rel_2_3:

working/user/dyrcona/lp793550_rel_2_3

backport to rel_2_2:

working/user/dyrcona/lp793550_rel_2_2

The branch also adds a new ou setting (circ.lost.xact_finish_on_zero) that controls whether or not LOST transactions are closed (xact_finish set) when the balance reaches zero. Setting that to true will set xact_finish and the lost item will disappear from the patron's record. By default this is off, so lost and paid for items stay on a patron's record.

[Ben Shum (bshum)]

Seems to be working for me so far. Thanks Jason!

Sign-off: user/bshum/lp793550

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

[Dan Wells (dbw2)]

I haven't had time to thoroughly test this, but I have looked over the code carefully, and I have a few concerns.

First, I think we may be creating the opposite problem, i.e. circs which do not close when they should. In particular, it looks like if a circ reaches MAX_FINES, then is checked in, then is paid off (a common sequence), the xact will remain open. Am I wrong?

Second, we have some logic very similar to the original problem code in _check_open_xact() further down the file. While it seems less likely to happen often, it appears we could still end up with unclosed circs if, for instance, a non-checked-in MAX_FINES circ has its bills voided rather than paid off.

Is there any reason we are not consulting checkin_time when determining whether a circ can close? It looks like we ultimately want to consult both that *and* stop_fines to determine closeable circs (assuming we want to close a circ at all that doesn't end in a checkin, like the LOST option presented here (which, as an option, I think is fine)).

Based on the above, I humbly suggest we create a utility function, something like _circ_is_closable(), where we can consolidate and focus all the relevant logic and return a simple bool, or alternatively that we expand on _check_open_xact() and call it from within make_payments() (where it might be overkill).

Finally, thanks for taking this on! We're definitely making progress, and maybe I'm completely off with my concerns.

Dan

[Jason Stephenson (jstephenson)]

Good call, Dan!

I have pushed another commit on my branch that addresses most of your concerns:

I have moved the logic to check if the circ transaction can be closed to its own utility function in OpenILS::Application::Circ::CircCommon. This potentially consolidates the logic in one place in case we need to use it elsewhere.

I have also added a check for checkin_time being set on the transaction. It turns out that Dan's scenario of a MAXFINES book being checked in then paid did not set xact_finish with the old logic.

I am not going to touch _check_open_xact() at this time. I'm not entirely clear on its purpose at the moment, and the code in branch addresses this bug. If it turns out that _check_open_xact() needs changing, that could be handled in another bug, or tacked onto this one if it is still open.

[Dan Wells (dbw2)]

Hello Jason,

I think this code does the job, thanks! I would like to make one more suggestion which affects the usability of the helper function. The comments say it returns 0 or 1, and that's definitely what it should do, but it looks like the function will currently return 0, 1, 't', or 'f'. I think we're better off if we move the $U->is_true() inside the helper function, ideally inside the logic branch where it's needed. What do you think?

Also, and this is just a stylistic preference, so please take it or leave it, but I'd probably skip the ternary operator and $close variable altogether (since we don't use it again) and just do the test directly, something like (untested!):

if (!$circ or OpenILS::Application::Circ::CircCommon->can_close_circ($e, $circ )) {
        $trans = $e->retrieve_money_billable_transaction($transid);
...(go on and close it up)...

Again, I think the first part is important, but the second part is just a fresh look impression. I have no idea if Perl::Critic would like one or the other :)

I am also happy to toss up a branch if you've moved on from this.

Thanks again,
Dan

[Jason Stephenson (jstephenson)]

Ok, Dan.

Rebased and force pushed with your latest suggetions taken into account.

working/user/dyrcona/lp793550

[Jason Stephenson (jstephenson)]

The solution that I posted to this bug looks like it would also fix LP 638509.

I'd be grateful is someone else could test the branch.

[Dan Wells (dbw2)]

Hello Jason,

I've now tested this pretty systematically and everything is working as intended. Thank you for seeing this through!

I am concerned, though, about what I am experiencing as a new default handling of 'lost' transactions. That's one case where the current code does (in my mind) the "right thing"; if something is marked 'lost' and then paid for, the transaction is closed. I realize you added the new 'lost' option to deal with this, and it works perfectly in my testing, but I feel pretty strongly that closing 'lost' transactions when they are paid for should be the default (as it is now), not something people will need to configure.

The easiest way to accomplish that would be to simply invert the current option (and the handling code), something like 'circ.lost.xact_open_on_zero'. Then if it is unset or set to false, we keep the current behavior in the case of lost-and-paid.

Please let me know if I am mistaken in my testing, or if you think the current default behavior (before this patch) is undesirable.

Thanks,
Dan

[Jason Stephenson (jstephenson)]

Dan,

My short term memory is getting paged out rather quickly lately because I've had a lot going on over the past two months, so I don't really remember why tsbere and I thought that the default should be to leave lost transactions open. I do recall we had a short conversation about it and that was our conclusion. We may have had some local tickets in our system requesting that lost and paid for items stay on a patron's record.

Either way, we can always set the setting to do what we like, and following the principle of least surprise, the default should be to continue what was happening before the changes. So, yeah. I agree with you today.

I'll edit the branch and add a comment when it has been pushed.

[Jason Stephenson (jstephenson)]

Made the changes, rebased, and force-pushed to working/user/dyrcona/lp793550.

Haven't actually tested it, yet, since my dev image is a bit hosed at the moment.

[Dan Wells (dbw2)]

Looks good, tests fine, pushed to master, and backported through rel_2_2. Thanks for this important fix, Jason!

[Michael Peters (mrpeters)]

Backported cleanly to our 2.2.2 system. Thank you for your hard work. This was a tough one, I'm sure!