Ensure that borrowing privileges are allowed when patron.block_reason…

[dbernstein]

… == PatronData.NO_VALUE

Description

There was some privilege checks outside for the sirsidynix horizon auth provider that I missed on the last round of changes.
This update ensures that patron.block_reason = NO_VALUE is understood to mean do not block even if there are fines.

Motivation and Context

Follow on ticket for https://ebce-lyrasis.atlassian.net/browse/PP-1589

How Has This Been Tested?

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.63%. Comparing base (0cf8c8d) to head (581c86d).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1999   +/-   ##
=======================================
  Coverage   90.63%   90.63%           
=======================================
  Files         339      339           
  Lines       40210    40208    -2     
  Branches     8702     8701    -1     
=======================================
  Hits        36444    36444           
+ Misses       2493     2492    -1     
+ Partials     1273     1272    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tdilauro]

One comment below...

[tdilauro]

It's not obvious to me why it is okay to move this ahead of checking whether the patron's card has expired. It seems unlikely that a patron with an expired card would have borrowing privileges.

If this case is actually allowed for some reason, it would be good to both (1) add a test with this scenario, and (2) update the docstring with some explanatory text.

[jonathangreen]

I don't think its safe to assume that PatronData.NO_VALUE for blocks means that we shouldn't block on fines.

We have a separate setting max_outstanding_fines

circulation/src/palace/manager/integration/configuration/library.py

Lines 436 to 443 in b1ab61a

	max_outstanding_fines: PositiveFloat | None = FormField(
	None,
	form=LibraryConfFormItem(
	label="Maximum amount in fines a patron can have before losing lending privileges",
	category="Loans, Holds, & Fines",
	level=Level.ALL_ACCESS,
	),
	)

that this change would break. Especially for ILS that don't send block information, but do send fine information.

I think what you are trying to accomplished can be done by setting max_outstanding_fines to None (no value in the config form), without any code changes.

Am I understanding what you are trying to accomplish here correctly? If not can you give some more context for this change so I can understand?

[dbernstein]

@jonathangreen and @tdilauro : The context here is that Vancouver uses SirsiDynixHorizon which supports the ability to ignore blocks on non-expired cards. When that setting is enabled we are setting the patron.block_reason to PatronData.NO_VALUE when we want the circulation manager to ignore block reasons. So if there are excessive fines, we just leave them be and set the block_reason to NO_VALUE. So when the code reaches the borrow check we are looking only at the fine info and not taking into account that upstream we overwrote the block reason to avoid being blocked on excessive fines.

So yes - I believe you're right @jonathangreen that setting max_outstanding_fines to None should accomplish what we want there, but we'll have to make sure to have the user both ignore blocks AND set max fines to zero. In other words it's not clear that the ignore blocks will be overridden by a max_outstanding_fines value being exceeded.

[dbernstein]

I see now what you are suggesting is also how SIP2 works - ie max_outstanding_fines is set at the library level. So maybe I can close this PR.

[jonathangreen]

@dbernstein what we currently do in the sirsi provider is the same thing we do in the SIP provider.

circulation/src/palace/manager/api/sip/__init__.py

Lines 452 to 475 in e384f76

	if "fee_amount" in info:
	fines = info["fee_amount"]
	else:
	fines = "0"
	patrondata.fines = MoneyUtility.parse(fines)
	if "sipserver_patron_class" in info:
	patrondata.external_type = info["sipserver_patron_class"]
	for expire_field in [
	"sipserver_patron_expiration",
	"polaris_patron_expiration",
	]:
	if expire_field in info:
	value = info.get(expire_field)
	value = self.parse_date(value)
	if value:
	patrondata.authorization_expires = value
	break
	if self.patron_status_should_block:
	patrondata.block_reason = self.info_to_patrondata_block_reason(
	info, patrondata
	)
	else:
	patrondata.block_reason = PatronData.NO_VALUE

We set the blocks based on the configuration, and set the fines as well, with the fines being enforced later. To me this makes sense. I think it would be more confusing if setting Enforce Patron ILS Blocks to false in the patron auth settings would also stop enforcing the max fines setting in the library configuration. @dwilcox might have a opinion on this one, since its question about admin facing settings.

Edit: Sorry @dbernstein your comment happened while I was writing this reply, so I didn't see it until after I posted this one. Looks like you already found the SIP2 code I referenced here.

[dbernstein]

I was confused. This PR does not need to be.