Add tests for patron activity task

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()

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__}.")

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",
+        )

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