feat: Channels should have some way to determine if they can send the notification (closes #19)

Author: kevinrenskers

generic_notifications/__init__.py

@@ -80,23 +80,22 @@ def send_notification(
         if notification_type.should_save(notification):
             notification.save()
 
-            # Create NotificationChannel entries for each enabled channel
+            # Check should_send and process for each enabled channel
             for channel_cls in enabled_channel_classes:
-                NotificationChannel.objects.create(
-                    notification=notification,
-                    channel=channel_cls.key,
-                )
-
-            # Process through enabled channels only
-            enabled_channel_instances = [channel_cls() for channel_cls in enabled_channel_classes]
-            for channel_instance in enabled_channel_instances:
-                try:
-                    channel_instance.process(notification)
-                except Exception as e:
-                    # Log error but don't crash - other channels should still work
-                    logger = logging.getLogger(__name__)
-                    logger.error(
-                        f"Channel {channel_instance.key} failed to process notification {notification.id}: {e}"
+                if channel_cls.should_send(notification):
+                    # Create NotificationChannel entry only for channels that can send
+                    NotificationChannel.objects.create(
+                        notification=notification,
+                        channel=channel_cls.key,
                     )
 
+                    # Process the notification through this channel
+                    try:
+                        channel_instance = channel_cls()
+                        channel_instance.process(notification)
+                    except Exception as e:
+                        # Log error but don't crash - other channels should still work
+                        logger = logging.getLogger(__name__)
+                        logger.error(f"Channel {channel_cls.key} failed to process notification {notification.id}: {e}")
+
             return notification

generic_notifications/channels.py

@@ -25,6 +25,20 @@ class BaseChannel(ABC):
     supports_realtime: bool = True
     supports_digest: bool = False
 
+    @classmethod
+    def should_send(cls, notification: "Notification") -> bool:
+        """
+        Check if this channel can send the given notification.
+        Override in subclasses to add channel-specific validation.
+
+        Args:
+            notification: Notification instance to check
+
+        Returns:
+            bool: True if the channel can send this notification, False otherwise
+        """
+        return True
+
     def process(self, notification: "Notification") -> None:
         """
         Process a notification through this channel based on channel capabilities
@@ -64,9 +78,7 @@ def send_now(self, notification: "Notification") -> None:
         """
         raise NotImplementedError(f"{self.__class__.__name__} does not support realtime sending")
 
-    def send_digest(
-        self, notifications: "QuerySet[Notification]", frequency: type[BaseFrequency] | None = None
-    ) -> None:
+    def send_digest(self, notifications: "QuerySet[Notification]", frequency: type[BaseFrequency]) -> None:
         """
         Send a digest with specific notifications.
         Override in subclasses that support digest delivery.
@@ -129,6 +141,19 @@ class EmailChannel(BaseChannel):
     supports_realtime = True
     supports_digest = True
 
+    @classmethod
+    def should_send(cls, notification: "Notification") -> bool:
+        """
+        Check if the recipient has an email address.
+
+        Args:
+            notification: Notification instance to check
+
+        Returns:
+            bool: True if the recipient has an email address, False otherwise
+        """
+        return bool(getattr(notification.recipient, "email", None))
+
     def send_now(self, notification: "Notification") -> None:
         """
         Send an individual email notification immediately.
@@ -198,7 +223,7 @@ def send_now(self, notification: "Notification") -> None:
             logger = logging.getLogger(__name__)
             logger.error(f"Failed to send email for notification {notification.id}: {e}")
 
-    def send_digest(self, notifications: "QuerySet[Notification]", frequency: type[BaseFrequency] | None = None):
+    def send_digest(self, notifications: "QuerySet[Notification]", frequency: type[BaseFrequency]):
         """
         Send a digest email with specific notifications.
         This method is used by the management command.
@@ -240,8 +265,7 @@ def send_digest(self, notifications: "QuerySet[Notification]", frequency: type[B
                 subject = render_to_string(subject_template, context).strip()
             except Exception:
                 # Fallback subject
-                frequency_name = frequency.name if frequency else "Digest"
-                subject = f"{frequency_name} - {notifications_count} new notification{pluralize(notifications_count)}"
+                subject = f"{frequency.name} - {notifications_count} new notification{pluralize(notifications_count)}"
 
             # Load HTML message
             try:

tests/test_channels.py

@@ -7,7 +7,7 @@
 from django.test import TestCase, override_settings
 
 from generic_notifications.channels import BaseChannel, EmailChannel
-from generic_notifications.frequencies import RealtimeFrequency
+from generic_notifications.frequencies import DailyFrequency, RealtimeFrequency
 from generic_notifications.models import DisabledNotificationTypeChannel, Notification, NotificationFrequency
 from generic_notifications.registry import registry
 from generic_notifications.types import NotificationType
@@ -167,6 +167,29 @@ def test_process_digest_frequency(self):
         notification.refresh_from_db()
         self.assertFalse(notification.is_sent_on_channel(EmailChannel))
 
+    def test_should_send_with_email(self):
+        """Test that should_send returns True when user has email"""
+        notification = create_notification_with_channels(
+            user=self.user,
+            notification_type="test_type",
+        )
+
+        self.assertTrue(EmailChannel.should_send(notification))
+
+    def test_should_send_without_email(self):
+        """Test that should_send returns False when user has no email"""
+        # Create user without email
+        user_no_email = User.objects.create_user(username="user2", password="testpass")
+        user_no_email.email = ""
+        user_no_email.save()
+
+        notification = create_notification_with_channels(
+            user=user_no_email,
+            notification_type="test_type",
+        )
+
+        self.assertFalse(EmailChannel.should_send(notification))
+
     @override_settings(DEFAULT_FROM_EMAIL="test@example.com")
     def test_send_now_basic(self):
         notification = create_notification_with_channels(
@@ -312,15 +335,6 @@ def test_send_now_template_error_fallback(self):
         self.assertEqual(email.subject, "Test Subject")
         self.assertEqual(len(email.alternatives), 0)  # type: ignore[attr-defined]  # No HTML alternative
 
-    @override_settings(DEFAULT_FROM_EMAIL="test@example.com")
-    def test_send_digest_emails_empty_queryset(self):
-        # No notifications exist, so digest should not send anything
-        empty_notifications = Notification.objects.none()
-        EmailChannel().send_digest(empty_notifications)
-
-        # No email should be sent when no notifications exist
-        self.assertEqual(len(mail.outbox), 0)
-
     @override_settings(DEFAULT_FROM_EMAIL="test@example.com")
     def test_send_digest_emails_basic(self):
         # Set user to daily frequency to prevent realtime sending
@@ -342,13 +356,13 @@ def test_send_digest_emails_basic(self):
         )
 
         # Send digest email for this user
-        EmailChannel().send_digest(notifications)
+        EmailChannel().send_digest(notifications, DailyFrequency)
 
         # Check email was sent
         self.assertEqual(len(mail.outbox), 1)
         email = mail.outbox[0]
         self.assertEqual(email.to, [self.user.email])
-        self.assertEqual(email.subject, "Digest - 3 new notifications")
+        self.assertEqual(email.subject, "Daily digest - 3 new notifications")
 
         # Check all notifications marked as sent
         for notification in notifications:
@@ -371,98 +385,12 @@ def test_send_digest_emails_with_frequency(self):
                 recipient=self.user,
                 channels__channel="email",
                 channels__sent_at__isnull=True,
-            )
-        )
-
-        email = mail.outbox[0]
-        self.assertEqual(email.subject, "Digest - 1 new notification")
-
-    @override_settings(DEFAULT_FROM_EMAIL="test@example.com")
-    def test_send_digest_emails_without_frequency(self):
-        # Set user to daily frequency to prevent realtime sending
-        NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily")
-
-        create_notification_with_channels(
-            user=self.user,
-            notification_type="test_type",
-            subject="Test",
-        )
-
-        EmailChannel().send_digest(
-            Notification.objects.filter(
-                recipient=self.user,
-                channels__channel="email",
-                channels__sent_at__isnull=True,
-            )
-        )
-
-        email = mail.outbox[0]
-        self.assertEqual(email.subject, "Digest - 1 new notification")
-
-    @override_settings(DEFAULT_FROM_EMAIL="test@example.com")
-    def test_send_digest_emails_text_limit(self):
-        # Set user to daily frequency to prevent realtime sending
-        NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily")
-
-        # Create more than 10 notifications to test text limit
-        for i in range(15):
-            create_notification_with_channels(
-                user=self.user,
-                notification_type="test_type",
-                subject=f"Test {i}",
-            )
-
-        EmailChannel().send_digest(
-            Notification.objects.filter(
-                recipient=self.user,
-                channels__channel="email",
-                channels__sent_at__isnull=True,
-            )
+            ),
+            DailyFrequency,
         )
 
-        # The implementation may not have this feature, so we'll just check that email was sent
-        self.assertEqual(len(mail.outbox), 1)
         email = mail.outbox[0]
-        self.assertEqual(email.subject, "Digest - 15 new notifications")
-
-    @override_settings(DEFAULT_FROM_EMAIL="test@example.com")
-    @patch("generic_notifications.channels.render_to_string")
-    def test_send_digest_emails_with_html_template(self, mock_render):
-        mock_render.return_value = "<html>Digest HTML</html>"
-
-        # Set user to daily frequency to prevent realtime sending
-        NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily")
-
-        create_notification_with_channels(
-            user=self.user,
-            notification_type="test_type",
-            subject="Test",
-        )
-
-        EmailChannel().send_digest(
-            Notification.objects.filter(
-                recipient=self.user,
-                channels__channel="email",
-                channels__sent_at__isnull=True,
-            )
-        )
-
-        # Check templates were rendered (subject, HTML, then text)
-        self.assertEqual(mock_render.call_count, 3)
-
-        # Check subject template call
-        subject_call = mock_render.call_args_list[0]
-        self.assertEqual(subject_call[0][0], "notifications/email/digest/subject.txt")
-
-        # Check HTML template call
-        html_call = mock_render.call_args_list[1]
-        self.assertEqual(html_call[0][0], "notifications/email/digest/message.html")
-
-        # Check text template call
-        text_call = mock_render.call_args_list[2]
-        self.assertEqual(text_call[0][0], "notifications/email/digest/message.txt")
-        self.assertEqual(html_call[0][1]["user"], self.user)
-        self.assertEqual(html_call[0][1]["count"], 1)
+        self.assertEqual(email.subject, "Daily digest - 1 new notification")
 
     def test_send_now_fallback_includes_url(self):
         """Test that fallback email content includes URL when available"""
@@ -505,7 +433,8 @@ def test_send_digest_emails_fallback_includes_urls(self):
                 recipient=self.user,
                 channels__channel="email",
                 channels__sent_at__isnull=True,
-            )
+            ),
+            DailyFrequency,
         )
 
         # Check that one email was sent
@@ -584,7 +513,7 @@ def send_email(
 
         # Test the custom channel
         custom_channel = TestEmailChannel()
-        custom_channel.send_digest(notifications)
+        custom_channel.send_digest(notifications, DailyFrequency)
 
         # Verify the custom _send_email method was called
         self.assertEqual(len(custom_channel.sent_emails), 1)