diff --git a/src/palace/manager/celery/tasks/patron_activity.py b/src/palace/manager/celery/tasks/patron_activity.py index 2fefce7683..af925bbd24 100644 --- a/src/palace/manager/celery/tasks/patron_activity.py +++ b/src/palace/manager/celery/tasks/patron_activity.py @@ -29,7 +29,7 @@ def sync_patron_activity( with patron_activity_status as acquired: if not acquired: status = patron_activity_status.status() - state = status.state if status is not None else "UNKNOWN" + state = status.state.name if status is not None else "UNKNOWN" task.log.info( f"Patron activity sync not needed (state: {state}) for " f"patron (id: {patron_id}) and collection (id: {collection_id})." @@ -41,14 +41,14 @@ def sync_patron_activity( collection = get_one(session, Collection, id=collection_id) if patron is None: - task.log.info( + task.log.error( f"Patron (id: {patron_id}) not found. Marking patron activity as failed." ) patron_activity_status.fail() return if collection is None: - task.log.info( + task.log.error( f"Collection (id: {collection_id}) not found. Marking patron activity as failed." ) patron_activity_status.fail() diff --git a/src/palace/manager/service/redis/models/patron_activity.py b/src/palace/manager/service/redis/models/patron_activity.py index 453acfaa82..238b6f7ba7 100644 --- a/src/palace/manager/service/redis/models/patron_activity.py +++ b/src/palace/manager/service/redis/models/patron_activity.py @@ -222,29 +222,12 @@ def lock(self) -> bool: ) return acquired is not None - def not_supported(self) -> bool: - """ - Mark the patron activity sync task as not supported. This can only be done if - the task currently has no data in redis. The status will never expire. - - :return: True if the status was set, False otherwise. - """ - value_set = self._redis_client.set( - self.key, - PatronActivityStatus( - state=PatronActivityStatus.State.NOT_SUPPORTED, task_id=self._task_id - ).to_redis(), - nx=True, - ex=self.NOT_SUPPORTED_TIMEOUT, - ) - return value_set is not None - def clear(self) -> bool: """ Clear the status of the patron activity sync task. - The status will only be cleared if the task is not currently in progress, so - that one task does not reset the lock while another is running. + If the state is not LOCKED, any task can clear the status. If the state is + LOCKED, only the task that acquired the lock can clear the status. :return: True if the status was cleared, False otherwise. """ @@ -265,8 +248,8 @@ def _update(self, status: PatronActivityStatus, timeout: int) -> bool: def success(self) -> bool: """ - Mark the patron activity sync task as successful. This can only be done if - the task is currently in progress. + Mark the patron activity sync task as successful. This can only be done by + the task that acquired the lock. :return: True if the status was updated, False otherwise. """ @@ -279,8 +262,8 @@ def success(self) -> bool: def fail(self) -> bool: """ - Mark the patron activity sync task as failed. This can only be done if - the task is currently in progress. + Mark the patron activity sync task as failed. This can only be done by + the task that acquired the lock. :return: True if the status was updated, False otherwise. """ @@ -291,6 +274,20 @@ def fail(self) -> bool: self.FAILED_TIMEOUT, ) + def not_supported(self) -> bool: + """ + Mark the patron activity sync task as not supported. This can only be done by + the task that acquired the lock. + + :return: True if the status was set, False otherwise. + """ + return self._update( + PatronActivityStatus( + state=PatronActivityStatus.State.NOT_SUPPORTED, task_id=self._task_id + ), + self.NOT_SUPPORTED_TIMEOUT, + ) + def __enter__(self) -> bool: if self._in_context_manager: raise PatronActivityError(f"Cannot nest {self.__class__.__name__}.") diff --git a/tests/manager/celery/tasks/test_patron_activity.py b/tests/manager/celery/tasks/test_patron_activity.py index e69de29bb2..74b58799a2 100644 --- a/tests/manager/celery/tasks/test_patron_activity.py +++ b/tests/manager/celery/tasks/test_patron_activity.py @@ -0,0 +1,174 @@ +from unittest.mock import create_autospec, patch + +import pytest + +from palace.manager.celery.task import Task +from palace.manager.celery.tasks.patron_activity import sync_patron_activity +from palace.manager.service.integration_registry.license_providers import ( + LicenseProvidersRegistry, +) +from palace.manager.service.logging.configuration import LogLevel +from palace.manager.service.redis.models.patron_activity import ( + PatronActivity, + PatronActivityStatus, +) +from tests.fixtures.celery import CeleryFixture +from tests.fixtures.database import DatabaseTransactionFixture +from tests.fixtures.redis import RedisFixture +from tests.fixtures.services import ServicesFixture +from tests.mocks.circulation import MockPatronActivityCirculationAPI + + +class SyncTaskFixture: + def __init__( + self, + db: DatabaseTransactionFixture, + redis_fixture: RedisFixture, + celery_fixture: CeleryFixture, + services_fixture: ServicesFixture, + ): + self.db = db + self.redis_fixture = redis_fixture + self.celery_fixture = celery_fixture + self.services_fixture = services_fixture + + self.library = db.library() + self.patron = db.patron(library=self.library) + self.collection = db.collection(library=self.library) + + self.redis_record = self.patron_activity(self.patron.id, self.collection.id) + self.mock_registry = create_autospec(LicenseProvidersRegistry) + self.services_fixture.services.integration_registry.license_providers.override( + self.mock_registry + ) + + def patron_activity(self, patron_id: int, collection_id: int) -> PatronActivity: + return PatronActivity( + self.redis_fixture.client, patron_id, collection_id, "test-fixture-task-id" + ) + + +@pytest.fixture +def sync_task_fixture( + db: DatabaseTransactionFixture, + redis_fixture: RedisFixture, + celery_fixture: CeleryFixture, + services_fixture: ServicesFixture, +): + return SyncTaskFixture(db, redis_fixture, celery_fixture, services_fixture) + + +class TestSyncPatronActivity: + def test_unable_to_lock( + self, sync_task_fixture: SyncTaskFixture, caplog: pytest.LogCaptureFixture + ): + caplog.set_level(LogLevel.info) + + # We lock the patron activity record in redis, so the task cannot acquire it. + sync_task_fixture.redis_record.lock() + + # We patch the task to raise an exception if the db is accessed. If we don't acquire the lock + # we should never go out to the database. + with patch.object( + Task, "_session_maker", side_effect=Exception() + ) as mock_session: + sync_patron_activity.apply_async( + (sync_task_fixture.collection.id, sync_task_fixture.patron.id, "pin") + ).wait() + + assert "Patron activity sync not needed (state: LOCKED)" in caplog.text + assert mock_session.call_count == 0 + + def test_patron_not_found( + self, + sync_task_fixture: SyncTaskFixture, + db: DatabaseTransactionFixture, + caplog: pytest.LogCaptureFixture, + ): + patron_id = sync_task_fixture.patron.id + db.session.delete(sync_task_fixture.patron) + task = sync_patron_activity.apply_async( + (sync_task_fixture.collection.id, patron_id, "pin") + ) + task.wait() + + assert f"Patron (id: {patron_id}) not found." in caplog.text + + task_status = sync_task_fixture.redis_record.status() + assert task_status.state == PatronActivityStatus.State.FAILED + assert task_status.task_id == task.id + + def test_collection_not_found( + self, + sync_task_fixture: SyncTaskFixture, + db: DatabaseTransactionFixture, + caplog: pytest.LogCaptureFixture, + ): + collection_id = sync_task_fixture.collection.id + db.session.delete(sync_task_fixture.collection) + task = sync_patron_activity.apply_async( + (collection_id, sync_task_fixture.patron.id, "pin") + ) + task.wait() + + assert f"Collection (id: {collection_id}) not found." in caplog.text + + task_status = sync_task_fixture.redis_record.status() + assert task_status.state == PatronActivityStatus.State.FAILED + assert task_status.task_id == task.id + + def test_exception( + self, sync_task_fixture: SyncTaskFixture, caplog: pytest.LogCaptureFixture + ): + sync_task_fixture.collection.protocol = "unknown" + + with pytest.raises(KeyError): + sync_patron_activity.apply_async( + (sync_task_fixture.collection.id, sync_task_fixture.patron.id, "pin") + ).wait() + + task_status = sync_task_fixture.redis_record.status() + assert task_status.state == PatronActivityStatus.State.FAILED + + assert "An exception occurred during the patron activity sync" in caplog.text + + def test_not_supported( + self, sync_task_fixture: SyncTaskFixture, caplog: pytest.LogCaptureFixture + ): + caplog.set_level(LogLevel.info) + + sync_patron_activity.apply_async( + (sync_task_fixture.collection.id, sync_task_fixture.patron.id, "pin") + ).wait() + + task_status = sync_task_fixture.redis_record.status() + assert task_status.state == PatronActivityStatus.State.NOT_SUPPORTED + + assert "does not support patron activity sync" in caplog.text + sync_task_fixture.mock_registry.from_collection.assert_called_once_with( + sync_task_fixture.db.session, sync_task_fixture.collection + ) + + def test_success(self, sync_task_fixture: SyncTaskFixture): + mock_collection_api = MockPatronActivityCirculationAPI( + sync_task_fixture.db.session, sync_task_fixture.collection + ) + sync_task_fixture.mock_registry.from_collection.return_value = ( + mock_collection_api + ) + + sync_patron_activity.apply_async( + (sync_task_fixture.collection.id, sync_task_fixture.patron.id, "pin") + ).wait() + + task_status = sync_task_fixture.redis_record.status() + assert task_status.state == PatronActivityStatus.State.SUCCESS + + sync_task_fixture.mock_registry.from_collection.assert_called_once_with( + sync_task_fixture.db.session, sync_task_fixture.collection + ) + assert len(mock_collection_api.patron_activity_calls) == 1 + assert mock_collection_api.patron_activity_calls[0] == ( + sync_task_fixture.patron, + "pin", + ) diff --git a/tests/manager/service/redis/models/test_patron_activity.py b/tests/manager/service/redis/models/test_patron_activity.py index 665934ba7c..c907bcd159 100644 --- a/tests/manager/service/redis/models/test_patron_activity.py +++ b/tests/manager/service/redis/models/test_patron_activity.py @@ -304,7 +304,11 @@ def test_fail( # indicating that we want to try to sync the patron activity again. patron_activity_fixture.assert_ttl(patron_activity.FAILED_TIMEOUT) - def test_context_manager(self, patron_activity_fixture: PatronActivityFixture): + def test_context_manager( + self, + patron_activity_fixture: PatronActivityFixture, + caplog: pytest.LogCaptureFixture, + ): patron_activity = patron_activity_fixture.patron_activity other_patron_activity = patron_activity_fixture.other_patron_activity @@ -315,13 +319,15 @@ def test_context_manager(self, patron_activity_fixture: PatronActivityFixture): patron_activity_fixture.assert_state(PatronActivityStatus.State.LOCKED) patron_activity_fixture.assert_state(PatronActivityStatus.State.SUCCESS) - # If there is an exception, the status should be failed instead of completed + # If there is an exception, the status should be failed instead of completed, and we should + # log the exception. patron_activity.clear() with pytest.raises(Exception): with patron_activity as acquired: assert acquired is True raise Exception() patron_activity_fixture.assert_state(PatronActivityStatus.State.FAILED) + assert "An exception occurred during the patron activity sync" in caplog.text # If the status is already LOCKED, the context manager returns False to indicate that # it could not acquire the lock