Fixes bug in set_user_preference where existing preferences cannot be…

… updated (#385)

keystone_api/apps/notifications/admin.py

@@ -4,6 +4,7 @@
 interfaces for managing application database constructs.
 """
 
+from django.conf import settings
 from django.contrib import admin
 
 from .models import *

keystone_api/apps/notifications/migrations/0001_initial.py

@@ -31,9 +31,9 @@ class Migration(migrations.Migration):
             name='Preference',
             fields=[
                 ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
-                ('alloc_thresholds', models.JSONField(default=apps.notifications.models._default_alloc_thresholds)),
+                ('alloc_thresholds', models.JSONField(default=apps.notifications.models.default_alloc_thresholds)),
                 ('notify_status_update', models.BooleanField(default=True)),
-                ('expiry_thresholds', models.JSONField(default=apps.notifications.models._default_expiry_thresholds)),
+                ('expiry_thresholds', models.JSONField(default=apps.notifications.models.default_expiry_thresholds)),
                 ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
             ],
         ),

keystone_api/apps/notifications/models.py

@@ -11,12 +11,24 @@
 from django.conf import settings
 from django.db import models
 
+__all__ = ['Notification', 'Preference']
+
+
+def default_alloc_thresholds() -> list[int]:  # pragma: nocover
+    """The default allocating usage thresholds at which to issue a user notification.
+
+    Returned values are defined in units of percent usage.
+    """
 
-def _default_alloc_thresholds() -> list[int]:
     return [90]
 
 
-def _default_expiry_thresholds() -> list[int]:
+def default_expiry_thresholds() -> list[int]:  # pragma: nocover
+    """The default expiration thresholds at which to issue a user notification.
+
+    Returned values are defined in units of days until expiration.
+    """
+
     return [30, 14, 0]
 
 
@@ -47,9 +59,9 @@ class NotificationType(models.TextChoices):
 class Preference(models.Model):
     """User notification preferences."""
 
-    alloc_thresholds = models.JSONField(default=_default_alloc_thresholds)
+    alloc_thresholds = models.JSONField(default=default_alloc_thresholds)
     notify_status_update = models.BooleanField(default=True)
-    expiry_thresholds = models.JSONField(default=_default_expiry_thresholds)
+    expiry_thresholds = models.JSONField(default=default_expiry_thresholds)
 
     user = models.OneToOneField(settings.AUTH_USER_MODEL, on_delete=models.CASCADE)
 
@@ -61,7 +73,7 @@ def get_user_preference(cls, user: settings.AUTH_USER_MODEL) -> Preference:
         return preference
 
     @classmethod
-    def set_user_preference(cls, *args, **kwargs) -> None:
+    def set_user_preference(cls, user: settings.AUTH_USER_MODEL, **kwargs) -> None:
         """Set user preferences, creating or updating as necessary."""
 
-        cls.objects.create(*args, **kwargs)
+        cls.objects.update_or_create(user=user, defaults=kwargs)

keystone_api/apps/notifications/tests/models/test_Preference.py

@@ -0,0 +1,65 @@
+"""Unit tests for the `Preference` class."""
+
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+
+from apps.notifications.models import default_alloc_thresholds, default_expiry_thresholds, Preference
+
+User = get_user_model()
+
+
+class PreferenceModelTest(TestCase):
+    """Tests for getting user preferences."""
+
+    def setUp(self) -> None:
+        """Create a test user."""
+
+        self.user = User.objects.create_user(username='testuser', password='foobar123!')
+
+    def test_get_user_preference_creates_new_preference(self) -> None:
+        """Test a new Preference object is created if one does not exist."""
+
+        # Test a record is created
+        self.assertFalse(Preference.objects.filter(user=self.user).exists())
+        preference = Preference.get_user_preference(user=self.user)
+        self.assertTrue(Preference.objects.filter(user=self.user).exists())
+
+        # Ensure preference is created with appropriate defaults
+        self.assertEqual(self.user, preference.user)
+        self.assertListEqual(default_alloc_thresholds(), preference.alloc_thresholds)
+        self.assertListEqual(default_expiry_thresholds(), preference.expiry_thresholds)
+
+    def test_get_user_preference_returns_existing_preference(self) -> None:
+        """Test an existing Preference object is returned if it already exists."""
+
+        existing_preference = Preference.objects.create(user=self.user)
+        preference = Preference.get_user_preference(user=self.user)
+        self.assertEqual(existing_preference, preference)
+
+
+class PreferenceSetTest(TestCase):
+    """Tests for setting user preferences."""
+
+    def setUp(self) -> None:
+        """Create a test user."""
+
+        self.user = User.objects.create_user(username='testuser', password='foobar123!')
+
+    def test_set_user_preference_creates_preference(self) -> None:
+        """Test that a new Preference object is created with specified values."""
+
+        self.assertFalse(Preference.objects.filter(user=self.user).exists())
+
+        Preference.set_user_preference(user=self.user, notify_status_update=False)
+        preference = Preference.objects.get(user=self.user)
+        self.assertFalse(preference.notify_status_update)
+
+    def test_set_user_preference_updates_existing_preference(self) -> None:
+        """Test that an existing Preference object is updated with specified values."""
+
+        preference = Preference.objects.create(user=self.user, notify_status_update=True)
+        self.assertTrue(Preference.objects.filter(user=self.user).exists())
+
+        Preference.set_user_preference(user=self.user, notify_status_update=False)
+        preference.refresh_from_db()
+        self.assertFalse(preference.notify_status_update)