PATRON_EXCEEDS_OVERDUE_COUNT threshold still allows one checkout

[Dale Rigney]

Tested in Evergreen in 3.3

PATRON_EXCEEDS_OVERDUE_COUNT threshold does not set the standing penalty until one item is checked out. If you set the PATRON_EXCEEDS_OVERDUE_COUNT threshold it appears the standing penalty is not set until the patron checks out one item. The do_checkout process calls calculate_penalties where Evergreen checks to see how many overdues the patron has. If the patron does exceed that limit Evergreen will create a PATRON_EXCEEDS_OVERDUE_COUNT standing penalty for the patron but does not apply that penalty to the current checkout.

How to test:
1) Find a patron with 2 or more overdue items.
2) set the PATRON_EXCEEDS_OVERDUE_COUNT threshold to 1
3) Checkout an item to the patron identified above.
4) The item will checkout
5) Checkout another item. That checkout will be stopped with the PATRON_EXCEEDS_OVERDUE_COUNT error message.

The standing penalty needs to be created before the first item is approved for checkout.

[Erica Rohlfs (erohlfs)]

Confirming that I can recreate this behavior on EG version 3.7. However, I can only recreate the scenario as described by Dale if I am testing a patron and items within the same depth/scoping of the defined threshold.

[Michele Morgan (mmorgan)]

Related:

bug 1823225
bug 1944986

[Elizabeth Davis (elidavis)]

We have experienced this with 3.7.2

[Elizabeth Davis (elidavis)]

I believe we are also experiencing this with the Patron Exceeds Checkout Count threshold as well.

[Jason Boyer (jboyer)]

It reads like the root of this issue is that penalty thresholds aren't applied retroactively on creation which can be addressed one of two ways. Either by a patch to potentially force penalty recalculation on every user retrieval ( bug 1983621 ) or using the script from bug 1944986 to recalculate all penalties after creating new thresholds.

For those affected, what's the preference on addressing this? Is any further change required?

[Jane Sandberg (sandbergja)]

I made a patch that adds an additional penalty check to the checkout process, which solves this issue (and also potentially bug 1761560). I chose to add the check to checkout, rather than patron retrieval, so that it can also address this issue for SIP checkouts (which still use do_checkout to do their checkout work, but take a different approach to retrieving patrons).

Here is the branch: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/sandbergja/lp1890822_patron_exceeds_overdue_count

Looking forward to hearing your thoughts!

[Jane Sandberg (sandbergja)]

whoops, wait, one of my tests is failing. Removing pullrequest tag while I investigate.

[Jane Sandberg (sandbergja)]

With apologies to the git admins, I have force pushed a corrected branch. I don't know that it covers bug 1761560, but it at least fixes PATRON_EXCEEDS_OVERDUE_COUNT and PATRON_EXCEEDS_CHECKOUT_COUNT.

Branch at https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/sandbergja/lp1890822_patron_exceeds_overdue_count

Some ideas for exercising this patch:
* Run the perl live tests
* Run the pgtap live tests
* Follow the steps from Dale's original bug report

[Galen Charlton (gmc)]

Upon taking an initial look:

* I think this is on the right track.
* PATRON_EXCEEDS_LONGOVERDUE_COUNT was missed. It might be nice to add a threshold_based column to config.standing_penalty so that we're not hard-coding these, but I think that can be another bug.
* There's a comment in another part of Circulate.pm that should be edited:

    # ------------------------------------------------------------------------------
    # Update the patron penalty info in the DB. Run it for permit-overrides
    # since the penalties are not updated during the permit phase
    # ------------------------------------------------------------------------------

* I wonder if the penalty calculation should be done earlier in the process (say in run_method()), and in a separate transaction. That way, the penalty would be saved in the database and available for display when reloading the patron page.
* Extra credit (and I think this is a separate bug) - more frequent updating of the patron summary box whenever something happens that might create or remove a penalty.
* This doesn't cover bug 1761560.

[Elizabeth Davis (elidavis)]

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

[Jane Sandberg (sandbergja)]

Thanks, Galen and Elizabeth. I pushed a second commit to user/sandbergja/lp1890822_patron_exceeds_overdue_count to incorporate the recommended changes. It no longer hard-codes a list of threshold-based penalties, since it turns out that this was not needed in the first place -- calculate_penalties only works with threshold-based penalties. It also moves the penalty check into its own transaction in run_method, updates the comment, and adds some additional checks to the perl live test to confirm that the penalties remain on the patron's account, even if the attempted checkout is rolled back.

[Galen Charlton (gmc)]

Upon review of the update, there are a couple problems:

- the penalty calculation doesn't check whether patron_id is set, and often it will not be (e.g., checkouts via SIP2, renewals, and checkins)
- that per se doesn't throw an error, but it pointed out a problem: various actions that could be blocked by a newly calculated standing penalty won't be caught, including SIP2 checkouts and renewals.

I think it may be necessary to move the penalty calculation closer to the point where we're sure that we have the patron ID if relevant... possibly as late as in run_indb_circ_test.

[Jane Sandberg (sandbergja)]

Good point, Galen. Thanks! I pushed a third commit to https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/sandbergja/lp1890822_patron_exceeds_overdue_count to move the penalty check back to run_indb_circ_test.

[Galen Charlton (gmc)]

An example of what I mean can be found here: user/gmcharlt/lp1890822_move_penalty_calculation, with the tradeoff that if the checkout or renenwal fails because of a newly-calculated penalty, the new penalty will get rolled back.

This may be good enough for now, but if not, options I see include

- refactoring run_method so that the first thing it does (for checkouts and renewals) is identify the user for all possible types of input parameters. Note that if this approach is taken that it will be important to make sure that the big transaction maintained by $circulator gets opened (again?) after the penalties are recalculated
- stick in an out-of-main-transaction recalculation of the user penalties if the checkout or renewal fails (either for any reason, or specifically because of a blocking system penalty)

[Galen Charlton (gmc)]

And if my follow-up is good enough for now, please feel free to test and do a cross-signoff.

[Galen Charlton (gmc)]

I see our commits crossed. Upon reviewing yours, I figured out that I was assuming that the transactions would behave as if they were at the repeatable read transaction isolation, which isn't the case: it's read committed by default in PostgreSQL. Consequently, I've tested and merged your final branch and added a follow-up to extend the documentation change and add a comment about the isolation level.

Thanks, Jane and Elizabeth!