From 2349f5dd898a58880afed14462c27e3b019d57d1 Mon Sep 17 00:00:00 2001 From: dbernstein Date: Tue, 6 Aug 2024 08:05:05 -0700 Subject: [PATCH] [PP-1525] Add support for bypassing user blocks on Sirsidynix Authentication Provider. (#1966) --- .../api/sirsidynix_authentication_provider.py | 81 ++++++++++++------- .../api/test_sirsidynix_auth_provider.py | 77 ++++++++++++++++-- 2 files changed, 122 insertions(+), 36 deletions(-) diff --git a/src/palace/manager/api/sirsidynix_authentication_provider.py b/src/palace/manager/api/sirsidynix_authentication_provider.py index 4514c19b7..821de8fdf 100644 --- a/src/palace/manager/api/sirsidynix_authentication_provider.py +++ b/src/palace/manager/api/sirsidynix_authentication_provider.py @@ -56,6 +56,18 @@ class SirsiDynixHorizonAuthSettings(BasicAuthProviderSettings): alias="CLIENT_ID", ) + patron_status_block: bool = FormField( + True, + form=ConfigurationFormItem( + label="Patron Status Block", + description=( + "Block patrons from borrowing based on the status of the ILS's patron block field?" + ), + type=ConfigurationFormItemType.SELECT, + options={"true": "Yes, block.", "false": "No, do not block."}, + ), + ) + class SirsiDynixHorizonAuthLibrarySettings(BasicAuthProviderLibrarySettings): library_id: str = FormField( @@ -153,6 +165,9 @@ def __init__( self.sirsi_disallowed_suffixes = library_settings.library_disallowed_suffixes self.sirsi_library_id = library_settings.library_id + # Check if patrons should be blocked based on ILS status + self.patron_status_should_block = settings.patron_status_block + def remote_authenticate( self, username: str | None, password: str | None ) -> PatronData | None: @@ -213,38 +228,44 @@ def remote_patron_lookup( patrondata.block_reason = SirsiBlockReasons.PATRON_BLOCKED return patrondata - # Get patron "fines" information - status = self.api_patron_status_info( - patron_key=patrondata.permanent_id, - session_token=patrondata.session_token, - ) - - if not status or "fields" not in status: - return None + if self.patron_status_should_block: + # Get patron "fines" information + status = self.api_patron_status_info( + patron_key=patrondata.permanent_id, + session_token=patrondata.session_token, + ) - status_fields: dict = status["fields"] - fines = status_fields.get("estimatedFines") - if fines is not None: - # We ignore currency for now, and assume USD - patrondata.fines = float(fines.get("amount", 0)) - - # Blockable statuses - if status_fields.get("hasMaxDaysWithFines") or status_fields.get("hasMaxFines"): - patrondata.block_reason = PatronData.EXCESSIVE_FINES - elif status_fields.get("hasMaxLostItem"): - patrondata.block_reason = PatronData.TOO_MANY_LOST - elif status_fields.get("hasMaxOverdueDays") or status_fields.get( - "hasMaxOverdueItem" - ): - patrondata.block_reason = PatronData.TOO_MANY_OVERDUE - elif status_fields.get("hasMaxItemsCheckedOut"): - patrondata.block_reason = PatronData.TOO_MANY_LOANS - elif status_fields.get("expired"): - patrondata.block_reason = SirsiBlockReasons.EXPIRED - - # If previously, the patron was blocked this should unset the value in the DB - if patrondata.block_reason is None: + if not status or "fields" not in status: + return None + + status_fields: dict = status["fields"] + fines = status_fields.get("estimatedFines") + if fines is not None: + # We ignore currency for now, and assume USD + patrondata.fines = float(fines.get("amount", 0)) + + # Blockable statuses + if status_fields.get("hasMaxDaysWithFines") or status_fields.get( + "hasMaxFines" + ): + patrondata.block_reason = PatronData.EXCESSIVE_FINES + elif status_fields.get("hasMaxLostItem"): + patrondata.block_reason = PatronData.TOO_MANY_LOST + elif status_fields.get("hasMaxOverdueDays") or status_fields.get( + "hasMaxOverdueItem" + ): + patrondata.block_reason = PatronData.TOO_MANY_OVERDUE + elif status_fields.get("hasMaxItemsCheckedOut"): + patrondata.block_reason = PatronData.TOO_MANY_LOANS + elif status_fields.get("expired"): + patrondata.block_reason = SirsiBlockReasons.EXPIRED + + # If previously, the patron was blocked this should unset the value in the DB + if patrondata.block_reason is None: + patrondata.block_reason = PatronData.NO_VALUE + else: patrondata.block_reason = PatronData.NO_VALUE + return patrondata ### diff --git a/tests/manager/api/test_sirsidynix_auth_provider.py b/tests/manager/api/test_sirsidynix_auth_provider.py index 9567ca11c..b01ca6b6e 100644 --- a/tests/manager/api/test_sirsidynix_auth_provider.py +++ b/tests/manager/api/test_sirsidynix_auth_provider.py @@ -77,8 +77,13 @@ def headers(self, api: SirsiDynixHorizonAuthenticationProvider) -> dict[str, str "x-sirs-clientID": api.sirsi_client_id, } - def provider_mocked_api(self) -> MockedSirsiApi: - provider = self.provider() + def provider_mocked_api( + self, + provider: SirsiDynixHorizonAuthenticationProvider | None = None, + patron_status_info: dict[str, Any] | None = None, + ) -> MockedSirsiApi: + if provider is None: + provider = self.provider() api_patron_login = create_autospec( provider.api_patron_login, @@ -94,16 +99,20 @@ def provider_mocked_api(self) -> MockedSirsiApi: } }, ) - api_patron_status_info = create_autospec( - provider.api_patron_status_info, - return_value={ + + if not patron_status_info: + patron_status_info = { "fields": { "estimatedFines": { "amount": "50.00", "currencyCode": "USD", } } - }, + } + + api_patron_status_info = create_autospec( + provider.api_patron_status_info, + return_value=patron_status_info, ) provider.api_patron_login = api_patron_login @@ -203,6 +212,62 @@ def test_remote_patron_lookup(self, sirsi_auth_fixture: SirsiAuthFixture): assert patrondata.block_reason == PatronData.NO_VALUE assert patrondata.library_identifier == "testtype" + def test_remote_patron_lookup_unblocked_user_with_blocks_ignored( + self, sirsi_auth_fixture: SirsiAuthFixture + ): + settings = sirsi_auth_fixture.settings(patron_status_block=False) + provider = sirsi_auth_fixture.provider(settings=settings) + provider_mock = sirsi_auth_fixture.provider_mocked_api(provider) + patrondata = provider_mock.provider.remote_patron_lookup( + SirsiDynixPatronData(permanent_id="xxxx", session_token="xxx") + ) + + assert provider_mock.api_read_patron_data.call_count == 1 + assert provider_mock.api_patron_status_info.call_count == 0 + assert isinstance(patrondata, PatronData) + assert patrondata.personal_name == "Test User" + assert patrondata.block_reason == PatronData.NO_VALUE + + def test_remote_patron_lookup_blocked_user_with_blocks_enabled( + self, sirsi_auth_fixture: SirsiAuthFixture + ): + """Tests that remote patron block status is called when blocks are enabled.""" + patron_info = {"fields": {"hasMaxDaysWithFines": True}} + provider_mock = sirsi_auth_fixture.provider_mocked_api( + patron_status_info=patron_info + ) + + patrondata = provider_mock.provider.remote_patron_lookup( + SirsiDynixPatronData(permanent_id="xxxx", session_token="xxx") + ) + + assert provider_mock.api_read_patron_data.call_count == 1 + assert provider_mock.api_patron_status_info.call_count == 1 + assert isinstance(patrondata, PatronData) + assert patrondata.personal_name == "Test User" + assert patrondata.block_reason == PatronData.EXCESSIVE_FINES + + def test_remote_patron_lookup_blocked_user_with_blocks_ignored( + self, sirsi_auth_fixture: SirsiAuthFixture + ): + """ "Test that patron block status is not called when blocks are ignored.""" + settings = sirsi_auth_fixture.settings(patron_status_block=False) + provider = sirsi_auth_fixture.provider(settings=settings) + provider_mock = sirsi_auth_fixture.provider_mocked_api(provider) + provider_mock.api_patron_status_info.return_value = { + "fields": {"hasMaxDaysWithFines": True} + } + + patrondata = provider_mock.provider.remote_patron_lookup( + SirsiDynixPatronData(permanent_id="xxxx", session_token="xxx") + ) + + assert provider_mock.api_read_patron_data.call_count == 1 + assert provider_mock.api_patron_status_info.call_count == 0 + assert isinstance(patrondata, PatronData) + assert patrondata.personal_name == "Test User" + assert patrondata.block_reason == PatronData.NO_VALUE + def test_remote_patron_lookup_bad_patrondata( self, sirsi_auth_fixture: SirsiAuthFixture ):