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

- Removed null=True from AbstractNotification.type field
- Made type required in notify.send() (kwargs.pop without default)
- Removed type=None guard logic in _get_related_object_url, get_message,
  email_subject, notify_handler, and get_user_email_preference
- Updated tests to always pass type='default'
- Updated docs to document type as required
- Created migration 0013_make_notification_type_nonnullable

Author: BHARATH0153

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,32 @@ 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/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,
@@ -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"),
@@ -1983,6 +1986,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

@@ -161,9 +161,8 @@ def get_user_email_preference(notification):
     If the user has no preference set, it defaults to True.
     """
     target_org = getattr(getattr(notification, "target", None), "organization_id", None)
-    if not (notification.type and target_org):
-        # We can not check email preference if notification type is absent,
-        # or if target_org is not present
+    if not target_org:
+        # We can not check email preference if target_org is not present,
         # therefore send email anyway.
         return True
     try:

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