Add minimum to ItemCounter and refactor redis client init

allianceauth/authentication/task_statistics/event_series.py

@@ -5,59 +5,27 @@ import logging
 from typing import List, Optional
 
 from pytz import utc
-from redis import Redis, RedisError
+from redis import Redis
 
-from allianceauth.utils.cache import get_redis_client
+from .helpers import get_redis_client_or_stub
 
 logger = logging.getLogger(__name__)
 
 
-class _RedisStub:
-    """Stub of a Redis client.
-
-    It's purpose is to prevent EventSeries objects from trying to access Redis
-    when it is not available. e.g. when the Sphinx docs are rendered by readthedocs.org.
-    """
-
-    def delete(self, *args, **kwargs):
-        pass
-
-    def incr(self, *args, **kwargs):
-        return 0
-
-    def zadd(self, *args, **kwargs):
-        pass
-
-    def zcount(self, *args, **kwargs):
-        pass
-
-    def zrangebyscore(self, *args, **kwargs):
-        pass
-
-
 class EventSeries:
     """API for recording and analyzing a series of events."""
 
     _ROOT_KEY = "ALLIANCEAUTH_EVENT_SERIES"
 
-    def __init__(self, key_id: str, redis: Redis = None) -> None:
-        self._redis = get_redis_client() if not redis else redis
-        try:
-            if not self._redis.ping():
-                raise RuntimeError()
-        except (AttributeError, RedisError, RuntimeError):
-            logger.exception(
-                "Failed to establish a connection with Redis. "
-                "This EventSeries object is disabled.",
-            )
-            self._redis = _RedisStub()
+    def __init__(self, key_id: str, redis: Optional[Redis] = None) -> None:
+        self._redis = get_redis_client_or_stub() if not redis else redis
         self._key_id = str(key_id)
         self.clear()
 
     @property
     def is_disabled(self):
         """True when this object is disabled, e.g. Redis was not available at startup."""
-        return isinstance(self._redis, _RedisStub)
+        return hasattr(self._redis, "IS_STUB")
 
     @property
     def _key_counter(self):
@@ -97,7 +65,7 @@ class EventSeries:
         self._redis.delete(self._key_counter)
 
     def count(self, earliest: dt.datetime = None, latest: dt.datetime = None) -> int:
-        """Count of events, can be restricted to given timeframe.
+        """Count of events, can be restricted to given time frame.
 
         Args:
         - earliest: Date of first events to count(inclusive), or -infinite if not specified

allianceauth/authentication/task_statistics/helpers.py

@@ -1,21 +1,62 @@
 """Helpers for Task Statistics."""
 
+import logging
 from typing import Optional
 
+from app_utils.allianceauth import get_redis_client
+from redis import Redis, RedisError
+
 from django.core.cache import cache
 
+logger = logging.getLogger(__name__)
+
+
+class _RedisStub:
+    """Stub of a Redis client.
+
+    It's purpose is to prevent EventSeries objects from trying to access Redis
+    when it is not available. e.g. when the Sphinx docs are rendered by readthedocs.org.
+    """
+
+    IS_STUB = True
+
+    def delete(self, *args, **kwargs):
+        pass
+
+    def incr(self, *args, **kwargs):
+        return 0
+
+    def zadd(self, *args, **kwargs):
+        pass
+
+    def zcount(self, *args, **kwargs):
+        pass
+
+    def zrangebyscore(self, *args, **kwargs):
+        pass
+
 
 class ItemCounter:
-    """A process safe item counter."""
+    """A process safe item counter.
+
+    Args:
+        - name: Unique name for the counter
+        - minimum: Counter can not go below the minimum, when set
+        - redis: A Redis client. Will use AA's cache client by default
+    """
 
     CACHE_KEY_BASE = "allianceauth-item-counter"
     DEFAULT_CACHE_TIMEOUT = 24 * 3600
 
-    def __init__(self, name: str) -> None:
+    def __init__(
+        self, name: str, minimum: Optional[int] = None, redis: Optional[Redis] = None
+    ) -> None:
         if not name:
             raise ValueError("Must define a name")
 
         self._name = str(name)
+        self._minimum = minimum
+        self._redis = redis if redis else get_redis_client()
 
     @property
     def _cache_key(self) -> str:
@@ -23,6 +64,9 @@ class ItemCounter:
 
     def reset(self, init_value: int = 0):
         """Reset counter to initial value."""
+        if self._minimum is not None and init_value < self._minimum:
+            raise ValueError("Can not reset below minimum")
+
         cache.set(self._cache_key, init_value, self.DEFAULT_CACHE_TIMEOUT)
 
     def incr(self, delta: int = 1):
@@ -34,6 +78,8 @@ class ItemCounter:
 
     def decr(self, delta: int = 1):
         """Decrement counter by delta."""
+        if self._minimum is not None and self.value() == self._minimum:
+            return
         try:
             cache.decr(self._cache_key, delta)
         except ValueError:
@@ -42,3 +88,18 @@ class ItemCounter:
     def value(self) -> Optional[int]:
         """Return current value or None if not yet initialized."""
         return cache.get(self._cache_key)
+
+
+def get_redis_client_or_stub():
+    """Return AA's default cache client or a stub if Redis is not available."""
+    redis = get_redis_client()
+    try:
+        if not redis.ping():
+            raise RuntimeError()
+    except (AttributeError, RedisError, RuntimeError):
+        logger.exception(
+            "Failed to establish a connection with Redis. "
+            "This EventSeries object is disabled.",
+        )
+        return _RedisStub()
+    return redis

allianceauth/authentication/task_statistics/tests/test_event_series.py

@@ -1,48 +1,19 @@
 import datetime as dt
-from unittest.mock import patch
 
 from pytz import utc
-from redis import RedisError
 
 from django.test import TestCase
 from django.utils.timezone import now
 
 from allianceauth.authentication.task_statistics.event_series import (
     EventSeries,
-    _RedisStub,
 )
+from allianceauth.authentication.task_statistics.helpers import _RedisStub
 
 MODULE_PATH = "allianceauth.authentication.task_statistics.event_series"
 
 
 class TestEventSeries(TestCase):
-    def test_should_abort_without_redis_client(self):
-        # when
-        with patch(MODULE_PATH + ".get_redis_client") as mock:
-            mock.return_value = None
-            events = EventSeries("dummy")
-        # then
-        self.assertTrue(events._redis, _RedisStub)
-        self.assertTrue(events.is_disabled)
-
-    def test_should_disable_itself_if_redis_not_available_1(self):
-        # when
-        with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
-            mock_get_master_client.return_value.ping.side_effect = RedisError
-            events = EventSeries("dummy")
-        # then
-        self.assertIsInstance(events._redis, _RedisStub)
-        self.assertTrue(events.is_disabled)
-
-    def test_should_disable_itself_if_redis_not_available_2(self):
-        # when
-        with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
-            mock_get_master_client.return_value.ping.return_value = False
-            events = EventSeries("dummy")
-        # then
-        self.assertIsInstance(events._redis, _RedisStub)
-        self.assertTrue(events.is_disabled)
-
     def test_should_add_event(self):
         # given
         events = EventSeries("dummy")
@@ -166,3 +137,15 @@ class TestEventSeries(TestCase):
         results = events.all()
         # then
         self.assertEqual(len(results), 2)
+
+    def test_should_not_report_as_disabled_when_initialized_normally(self):
+        # given
+        events = EventSeries("dummy")
+        # when/then
+        self.assertFalse(events.is_disabled)
+
+    def test_should_report_as_disabled_when_initialized_with_redis_stub(self):
+        # given
+        events = EventSeries("dummy", redis=_RedisStub())
+        # when/then
+        self.assertTrue(events.is_disabled)

allianceauth/authentication/task_statistics/tests/test_helpers.py

@@ -1,6 +1,13 @@
 from unittest import TestCase
+from unittest.mock import patch
 
-from allianceauth.authentication.task_statistics.helpers import ItemCounter
+from redis import RedisError
+
+from allianceauth.authentication.task_statistics.helpers import (
+    ItemCounter, _RedisStub, get_redis_client_or_stub,
+)
+
+MODULE_PATH = "allianceauth.authentication.task_statistics.helpers"
 
 COUNTER_NAME = "test-counter"
 
@@ -72,3 +79,37 @@ class TestItemCounter(TestCase):
         counter.decr(1)
         # then
         self.assertEqual(counter.value(), -1)
+
+    def test_can_not_decrement_counter_below_minimum(self):
+        # given
+        counter = ItemCounter(COUNTER_NAME, minimum=0)
+        counter.reset(0)
+        # when
+        counter.decr(1)
+        # then
+        self.assertEqual(counter.value(), 0)
+
+    def test_can_not_reset_counter_below_minimum(self):
+        # given
+        counter = ItemCounter(COUNTER_NAME, minimum=0)
+        # when/then
+        with self.assertRaises(ValueError):
+            counter.reset(-1)
+
+
+class TestGetRedisClient(TestCase):
+    def test_should_return_mock_if_redis_not_available_1(self):
+        # when
+        with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
+            mock_get_master_client.return_value.ping.side_effect = RedisError
+            result = get_redis_client_or_stub()
+        # then
+        self.assertIsInstance(result, _RedisStub)
+
+    def test_should_return_mock_if_redis_not_available_2(self):
+        # when
+        with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
+            mock_get_master_client.return_value.ping.return_value = False
+            result = get_redis_client_or_stub()
+        # then
+        self.assertIsInstance(result, _RedisStub)