[change] Make Notification.type non-nullable #481

docs/developer/sending-notifications.rst

@@ -52,7 +52,8 @@ The complete syntax for ``notify`` is:
 .. code-block:: python
 
     notify.send(
-        actor,
+        sender,
+        type,
         recipient,
         verb,
         action_object,
@@ -87,8 +88,7 @@ The ``notify`` signal supports the following parameters:
 ``type``          Set values of other parameters based on registered
                   :doc:`notification types <./notification-types>`
 
-                  Defaults to ``None`` meaning you need to provide other
-                  arguments.
+                  This parameter is required.
 ``email_subject`` Sets subject of email notification to be sent.
 
                   Defaults to the notification message.

openwisp_notifications/base/models.py

@@ -84,7 +84,6 @@ class AbstractNotification(UUIDModel, BaseNotification):
     CACHE_KEY_PREFIX = "ow-notifications-"
     type = models.CharField(
         max_length=30,
-        null=True,
         # TODO: Remove when dropping support for Django 4.2
         choices=(
             NOTIFICATION_CHOICES
@@ -216,16 +215,15 @@ def _get_related_object_url(self, field):
         """
         Returns URLs for "actor", "action_object" and "target" fields.
         """
-        if self.type:
-            # Generate URL according to the notification configuration
-            config = get_notification_configuration(self.type)
-            url = config.get(f"{field}_link", None)
-            if url:
-                try:
-                    url_callable = import_string(url)
-                    return url_callable(self, field=field, absolute_url=True)
-                except ImportError:
-                    return url
+        # Generate URL according to the notification configuration
+        config = get_notification_configuration(self.type)
+        url = config.get(f"{field}_link", None)
+        if url:
+            try:
+                url_callable = import_string(url)
+                return url_callable(self, field=field, absolute_url=True)
+            except ImportError:
+                return url
         return _get_object_link(obj=self._related_object(field), absolute_url=True)
 
     @property
@@ -260,8 +258,6 @@ def email_message(self):
             return self.get_message()
 
     def get_message(self):
-        if not self.type:
-            return self.description
         try:
             config = get_notification_configuration(self.type)
             data = self.data or {}
@@ -283,23 +279,18 @@ def get_message(self):
 
     @cached_property
     def email_subject(self):
-        if self.type:
-            try:
-                config = get_notification_configuration(self.type)
-                data = self.data or {}
-                return config["email_subject"].format(
-                    site=Site.objects.get_current(), notification=self, **data
-                )
-            except (AttributeError, KeyError, NotificationRenderException) as exception:
-                self._invalid_notification(
-                    self.pk,
-                    exception,
-                    "Error encountered in generating notification email",
-                )
-        elif self.data.get("email_subject", None):
-            return self.data.get("email_subject")
-        else:
-            return self.message
+        try:
+            config = get_notification_configuration(self.type)
+            data = self.data or {}
+            return config["email_subject"].format(
+                site=Site.objects.get_current(), notification=self, **data
+            )
+        except (AttributeError, KeyError, NotificationRenderException) as exception:
+            self._invalid_notification(
+                self.pk,
+                exception,
+                "Error encountered in generating notification email",
+            )
 
     def _related_object(self, field):
         obj_id = getattr(self, f"{field}_object_id")

openwisp_notifications/handlers.py

@@ -54,7 +54,7 @@ def notify_handler(**kwargs):
     description = kwargs.pop("description", None)
     timestamp = kwargs.pop("timestamp", timezone.now())
     recipient = kwargs.pop("recipient", None)
-    notification_type = kwargs.pop("type", None)
+    notification_type = kwargs.pop("type")
     target = kwargs.get("target", None)
     target_org = getattr(target, "organization_id", None)
     try:
@@ -88,33 +88,28 @@ def notify_handler(**kwargs):
         # chain can still resolve to True. Org-level web settings
         # are resolved via JOINs in the main query to avoid an
         # additional OrganizationNotificationSettings lookup.
-        if notification_type:
-            web_notification = Q(notificationsetting__web=True)
-            if notification_template["web_notification"]:
-                # Users with web=None inherit the org setting, so
-                # include them unless the org explicitly disables
-                # web notifications.
-                web_notification |= Q(
-                    notificationsetting__web=None,
-                ) & (
-                    Q(
-                        notificationsetting__organization__notification_settings__web=True
-                    )
-                    | Q(
-                        notificationsetting__organization__notification_settings__web=None
-                    )
-                    | Q(
-                        notificationsetting__organization__notification_settings__isnull=True
-                    )
+        web_notification = Q(notificationsetting__web=True)
+        if notification_template["web_notification"]:
+            # Users with web=None inherit the org setting, so
+            # include them unless the org explicitly disables
+            # web notifications.
+            web_notification |= Q(
+                notificationsetting__web=None,
+            ) & (
+                Q(notificationsetting__organization__notification_settings__web=True)
+                | Q(notificationsetting__organization__notification_settings__web=None)
+                | Q(
+                    notificationsetting__organization__notification_settings__isnull=True
                 )
-
-            notification_setting = web_notification & Q(
-                notificationsetting__type=notification_type,
-                notificationsetting__organization_id=target_org,
-                notificationsetting__deleted=False,
             )
-            where = where & notification_setting
-            where_group = where_group & notification_setting
+
+        notification_setting = web_notification & Q(
+            notificationsetting__type=notification_type,
+            notificationsetting__organization_id=target_org,
+            notificationsetting__deleted=False,
+        )
+        where = where & notification_setting
+        where_group = where_group & notification_setting
 
     # Ensure notifications are only sent to active user
     where = where & Q(is_active=True)

openwisp_notifications/migrations/0013_make_notification_type_nonnullable.py

@@ -0,0 +1,27 @@
+import django
+from django.db import migrations, models
+
+from openwisp_notifications.types import NOTIFICATION_CHOICES, get_notification_choices
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("openwisp_notifications", "0012_replace_jsonfield_with_django_builtin"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="notification",
+            name="type",
+            field=models.CharField(
+                choices=(
+                    NOTIFICATION_CHOICES
+                    if django.VERSION < (5, 0)
+                    else get_notification_choices
+                ),
+                max_length=30,
+                verbose_name="Notification Type",
+            ),
+        ),
+    ]

openwisp_notifications/tests/__init__.py

@@ -7,16 +7,16 @@
   Date & Time: {datetime_str}
   URL: https://example.com/api/v1/notifications/notification/{notification_id}/redirect/
 
-- Test Notification
+- Default notification with default verb and level info by None
   Description: Test Notification
   Date & Time: {datetime_str}
 
-- Test Notification
+- Default notification with default verb and level info by None
   Description: Test Notification
   Date & Time: {datetime_str}
   URL: https://localhost:8000/admin
 
-- Test Notification
+- Default notification with default verb and level info by None
   Description: Test Notification
   Date & Time: {datetime_str}
   URL: https://localhost:8000/admin
@@ -76,7 +76,7 @@
                 <div>
                   <span class="badge info">info</span>
                   <div class="title">
-                    <p>Test Notification</p>
+                    <p>Default notification with default verb and level info by None</p>
                   </div>
                 </div>
               </td>
@@ -101,7 +101,7 @@
                   <div>
                     <span class="badge info">info</span>
                     <div class="title">
-                      <p>Test Notification</p>
+                      <p>Default notification with default verb and level info by None</p>
                     </div>
                   </div>
                 </td>
@@ -127,7 +127,7 @@
                   <div>
                     <span class="badge info">info</span>
                     <div class="title">
-                      <p>Test Notification</p>
+                      <p>Default notification with default verb and level info by None</p>
                     </div>
                   </div>
                 </td>

openwisp_notifications/tests/test_admin.py

@@ -63,6 +63,7 @@ def setUp(self):
             description="Test Notification",
             verb="Test Notification",
             email_subject="Test Email subject",
+            type="default",
             url="localhost:8000/admin",
         )
         self.site = AdminSite()

openwisp_notifications/tests/test_notifications.py

@@ -77,6 +77,7 @@ def setUp(self):
             description="Test Notification",
             verb="Test Notification",
             email_subject="Test Email subject",
+            type="default",
             url="https://localhost:8000/admin",
         )
 
@@ -93,6 +94,7 @@ def test_create_notification(self):
             recipient=self.admin,
             description="Test Notification Description",
             verb="Test Notification",
+            type="default",
             action_object=operator,
             target=operator,
             data=data,
@@ -111,7 +113,7 @@ def test_create_notification(self):
             n.target_content_type, ContentType.objects.get_for_model(operator)
         )
         self.assertEqual(n.verb, "Test Notification")
-        self.assertEqual(n.message, "Test Notification Description")
+        self.assertIn("Default notification with", n.message)
         self.assertEqual(n.recipient, self.admin)
 
     def test_lazy_translation(self):
@@ -126,6 +128,7 @@ def test_lazy_translation(self):
             description=gettext_lazy("Test Notification"),
             verb=gettext_lazy("Test Notification"),
             email_subject=gettext_lazy("Test Email subject"),
+            type="default",
             url="https://localhost:8000/admin",
             message=gettext_lazy("Translated message"),
             random=gettext_lazy("any extra kwargs is evaluated"),
@@ -253,9 +256,8 @@ def test_email_sent(self):
         n = notification_queryset.first()
         self.assertEqual(
             mail.outbox[0].subject,
-            "Test Email subject",
+            n.email_subject,
         )
-        self.assertIn(n.message, mail.outbox[0].body)
         self.assertIn(n.data.get("url"), mail.outbox[0].body)
         self.assertIn("https://", n.data.get("url"))
         html_email = mail.outbox[0].alternatives[0][0]
@@ -362,9 +364,10 @@ def test_queryset_recipient(self):
     def test_description_in_email_subject(self):
         self.notification_options.pop("email_subject")
         self._create_notification()
+        n = notification_queryset.first()
         self.assertEqual(
             mail.outbox[0].subject,
-            "Test Notification",
+            n.email_subject,
         )
 
     def test_handler_optional_tag(self):
@@ -1993,6 +1996,7 @@ def setUp(self):
             level="info",
             verb="Test Notification",
             email_subject="Test Email subject",
+            type="default",
             url="https://localhost:8000/admin",
         )
 

openwisp_notifications/utils.py

@@ -163,10 +163,6 @@ def get_user_email_preference(notification):
     configuration when the target has no organization.
     """
     target_org = getattr(getattr(notification, "target", None), "organization_id", None)
-    if not notification.type:
-        # notify.send() may create notifications without a type; when no type is
-        # available, default to email notifications being enabled.
-        return True
     if not target_org:
         type_config = get_notification_configuration(notification.type)
         return type_config.get("email_notification", True)

tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.py

@@ -0,0 +1,27 @@
+import django
+from django.db import migrations, models
+
+from openwisp_notifications.types import NOTIFICATION_CHOICES, get_notification_choices
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("sample_notifications", "0004_replace_jsonfield_with_django_builtin"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="notification",
+            name="type",
+            field=models.CharField(
+                choices=(
+                    NOTIFICATION_CHOICES
+                    if django.VERSION < (5, 0)
+                    else get_notification_choices
+                ),
+                max_length=30,
+                verbose_name="Notification Type",
+            ),
+        ),
+    ]