From 7d929cb6e2bf82e6352dbe3961aba9922e39b6eb Mon Sep 17 00:00:00 2001
From: ErikKalkoken <erik.kalkoken@gmail.com>
Date: Thu, 9 Dec 2021 18:12:18 +0100
Subject: [PATCH] Fix: NOTIFICATIONS_MAX_PER_USER warning when not set

---
 allianceauth/notifications/managers.py        | 23 +++++----
 .../notifications/tests/test_managers.py      | 51 ++++++++++++++-----
 2 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/allianceauth/notifications/managers.py b/allianceauth/notifications/managers.py
index 628ba1b9..3798428e 100644
--- a/allianceauth/notifications/managers.py
+++ b/allianceauth/notifications/managers.py
@@ -49,19 +49,22 @@ class NotificationManager(models.Manager):
         logger.info("Created notification %s", obj)
         return obj
 
-    def _max_notifications_per_user(self):
-        """return the maximum number of notifications allowed per user"""
-        max_notifications = getattr(settings, 'NOTIFICATIONS_MAX_PER_USER', None)
-        if (
-            max_notifications is None
-            or not isinstance(max_notifications, int)
-            or max_notifications < 0
-        ):
+    def _max_notifications_per_user(self) -> int:
+        """Maximum number of notifications allowed per user."""
+        max_notifications = getattr(
+            settings,
+            "NOTIFICATIONS_MAX_PER_USER",
+            self.model.NOTIFICATIONS_MAX_PER_USER_DEFAULT
+        )
+        try:
+            max_notifications = int(max_notifications)
+        except ValueError:
+            max_notifications = None
+        if max_notifications is None or max_notifications < 0:
             logger.warning(
-                'NOTIFICATIONS_MAX_PER_USER setting is invalid. Using default.'
+                "NOTIFICATIONS_MAX_PER_USER setting is invalid. Using default."
             )
             max_notifications = self.model.NOTIFICATIONS_MAX_PER_USER_DEFAULT
-
         return max_notifications
 
     def user_unread_count(self, user_pk: int) -> int:
diff --git a/allianceauth/notifications/tests/test_managers.py b/allianceauth/notifications/tests/test_managers.py
index 19d40161..a74ca2b9 100644
--- a/allianceauth/notifications/tests/test_managers.py
+++ b/allianceauth/notifications/tests/test_managers.py
@@ -1,5 +1,6 @@
 from unittest.mock import patch
 
+from django.conf import settings
 from django.contrib.auth.models import User
 from django.test import TestCase, override_settings
 
@@ -113,29 +114,53 @@ class TestUserNotify(TestCase):
         self.assertSetEqual(result, expected)
 
 
+@patch("allianceauth.notifications.managers.logger")
 @patch(
-    MODULE_PATH + '.Notification.NOTIFICATIONS_MAX_PER_USER_DEFAULT',
+    MODULE_PATH + ".Notification.NOTIFICATIONS_MAX_PER_USER_DEFAULT",
     NOTIFICATIONS_MAX_PER_USER_DEFAULT
 )
 class TestMaxNotificationsPerUser(TestCase):
-
-    @override_settings(NOTIFICATIONS_MAX_PER_USER=None)
-    def test_reset_to_default_if_not_defined(self):
+    @override_settings(NOTIFICATIONS_MAX_PER_USER=42)
+    def test_should_use_custom_integer_setting(self, mock_logger):
+        # when
         result = Notification.objects._max_notifications_per_user()
-        expected = NOTIFICATIONS_MAX_PER_USER_DEFAULT
-        self.assertEqual(result, expected)
+        # then
+        self.assertEqual(result, 42)
+        self.assertFalse(mock_logger.warning.called)
 
-    @override_settings(NOTIFICATIONS_MAX_PER_USER='11')
-    def test_reset_to_default_if_not_int(self):
+    @override_settings(NOTIFICATIONS_MAX_PER_USER="42")
+    def test_should_use_custom_string_setting(self, mock_logger):
+        # when
         result = Notification.objects._max_notifications_per_user()
-        expected = NOTIFICATIONS_MAX_PER_USER_DEFAULT
-        self.assertEqual(result, expected)
+        # then
+        self.assertEqual(result, 42)
+        self.assertFalse(mock_logger.warning.called)
+
+    @override_settings()
+    def test_should_use_default_if_not_defined(self, mock_logger):
+        # given
+        del settings.NOTIFICATIONS_MAX_PER_USER
+        # when
+        result = Notification.objects._max_notifications_per_user()
+        # then
+        self.assertEqual(result, NOTIFICATIONS_MAX_PER_USER_DEFAULT)
+        self.assertFalse(mock_logger.warning.called)
+
+    @override_settings(NOTIFICATIONS_MAX_PER_USER="abc")
+    def test_should_reset_to_default_if_not_int(self, mock_logger):
+        # when
+        result = Notification.objects._max_notifications_per_user()
+        # then
+        self.assertEqual(result, NOTIFICATIONS_MAX_PER_USER_DEFAULT)
+        self.assertTrue(mock_logger.warning.called)
 
     @override_settings(NOTIFICATIONS_MAX_PER_USER=-1)
-    def test_reset_to_default_if_lt_zero(self):
+    def test_should_reset_to_default_if_lt_zero(self, mock_logger):
+        # when
         result = Notification.objects._max_notifications_per_user()
-        expected = NOTIFICATIONS_MAX_PER_USER_DEFAULT
-        self.assertEqual(result, expected)
+        # then
+        self.assertEqual(result, NOTIFICATIONS_MAX_PER_USER_DEFAULT)
+        self.assertTrue(mock_logger.warning.called)
 
 
 @patch('allianceauth.notifications.managers.cache')