Comment 7 for bug 793550

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