Improve email sending

generic_notifications/channels.py

@@ -1,9 +1,9 @@
 import logging
 from abc import ABC, abstractmethod
-from typing import TYPE_CHECKING, Any, Type
+from typing import TYPE_CHECKING, Type
 
 from django.conf import settings
-from django.core.mail import send_mail
+from django.core.mail import send_mail as django_send_mail
 from django.db.models import QuerySet
 from django.template.defaultfilters import pluralize
 from django.template.loader import render_to_string
@@ -23,6 +23,7 @@ class NotificationChannel(ABC):
 
     key: str
     name: str
+    supports_digest: bool = False
 
     @abstractmethod
     def process(self, notification: "Notification") -> None:
@@ -34,6 +35,29 @@ def process(self, notification: "Notification") -> None:
         """
         pass
 
+    def send_now(self, notification: "Notification") -> None:
+        """
+        Send a notification immediately through this channel.
+        Override in subclasses that support realtime delivery.
+
+        Args:
+            notification: Notification instance to send
+        """
+        raise NotImplementedError(f"{self.__class__.__name__} does not support realtime sending")
+
+    def send_digest(
+        self, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None
+    ) -> None:
+        """
+        Send a digest with specific notifications.
+        Override in subclasses that support digest delivery.
+
+        Args:
+            notifications: QuerySet of notifications to include in digest (must all have same recipient)
+            frequency: The frequency for context
+        """
+        raise NotImplementedError(f"{self.__class__.__name__} does not support digest sending")
+
 
 def register(cls: Type[NotificationChannel]) -> Type[NotificationChannel]:
     """
@@ -82,6 +106,7 @@ class EmailChannel(NotificationChannel):
 
     key = "email"
     name = "Email"
+    supports_digest = True
 
     def process(self, notification: "Notification") -> None:
         """
@@ -96,9 +121,9 @@ def process(self, notification: "Notification") -> None:
 
         # Send immediately if realtime, otherwise leave for digest
         if frequency_cls and frequency_cls.is_realtime:
-            self.send_email_now(notification)
+            self.send_now(notification)
 
-    def send_email_now(self, notification: "Notification") -> None:
+    def send_now(self, notification: "Notification") -> None:
         """
         Send an individual email notification immediately.
 
@@ -141,13 +166,11 @@ def send_email_now(self, notification: "Notification") -> None:
                 if absolute_url:
                     text_message += f"\n{absolute_url}"
 
-            send_mail(
+            self.send_email(
+                recipient=notification.recipient.email,
                 subject=subject,
-                message=text_message,
-                from_email=settings.DEFAULT_FROM_EMAIL,
-                recipient_list=[notification.recipient.email],
+                text_message=text_message,
                 html_message=html_message,
-                fail_silently=False,
             )
 
             # Mark as sent
@@ -158,22 +181,23 @@ def send_email_now(self, notification: "Notification") -> None:
             logger = logging.getLogger(__name__)
             logger.error(f"Failed to send email for notification {notification.id}: {e}")
 
-    @classmethod
-    def send_digest_emails(
-        cls, user: Any, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None
+    def send_digest(
+        self, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None
     ):
         """
-        Send a digest email to a specific user with specific notifications.
+        Send a digest email with specific notifications.
         This method is used by the management command.
 
         Args:
-            user: User instance
-            notifications: QuerySet of notifications to include in digest
+            notifications: QuerySet of notifications to include in digest (must all have same recipient)
             frequency: The frequency for template context
         """
         if not notifications.exists():
             return
 
+        # Get user from first notification (all have same recipient)
+        user = notifications.first().recipient
+
         try:
             # Group notifications by type for better digest formatting
             notifications_by_type: dict[str, list["Notification"]] = {}
@@ -226,13 +250,11 @@ def send_digest_emails(
                     message_lines.append(f"... and {notifications_count - 10} more")
                 text_message = "\n".join(message_lines)
 
-            send_mail(
+            self.send_email(
+                recipient=user.email,
                 subject=subject,
-                message=text_message,
-                from_email=settings.DEFAULT_FROM_EMAIL,
-                recipient_list=[user.email],
+                text_message=text_message,
                 html_message=html_message,
-                fail_silently=False,
             )
 
             # Mark all as sent
@@ -241,3 +263,23 @@ def send_digest_emails(
         except Exception as e:
             logger = logging.getLogger(__name__)
             logger.error(f"Failed to send digest email for user {user.id}: {e}")
+
+    def send_email(self, recipient: str, subject: str, text_message: str, html_message: str | None = None) -> None:
+        """
+        Actually send the email. This method can be overridden by subclasses
+        to use different email backends (e.g., Celery, different email services).
+
+        Args:
+            recipient: Email address of the recipient
+            subject: Email subject
+            text_message: Plain text email content
+            html_message: HTML email content (optional)
+        """
+        django_send_mail(
+            subject=subject,
+            message=text_message,
+            from_email=settings.DEFAULT_FROM_EMAIL,
+            recipient_list=[recipient],
+            html_message=html_message,
+            fail_silently=False,
+        )

generic_notifications/digest.py

@@ -0,0 +1,154 @@
+import logging
+from typing import Any
+
+from django.contrib.auth import get_user_model
+from django.contrib.auth.models import AbstractUser
+
+from .frequencies import NotificationFrequency
+from .models import Notification
+from .registry import registry
+from .types import NotificationType
+
+User = get_user_model()
+
+logger = logging.getLogger(__name__)
+
+
+def send_digest_notifications(frequency_key: str, dry_run: bool = False) -> int:
+    """
+    Send digest notifications for a specific frequency across all channels that support digests.
+
+    Args:
+        frequency_key: The frequency key to process (e.g., 'daily', 'weekly')
+        dry_run: If True, don't actually send notifications, just log what would be sent
+
+    Returns:
+        Total number of digests sent across all channels
+
+    Raises:
+        KeyError: If the frequency key is not found
+        ValueError: If the frequency is realtime (not a digest frequency)
+    """
+    # Get the specific frequency (required argument)
+    try:
+        frequency_cls = registry.get_frequency(frequency_key)
+    except KeyError:
+        raise KeyError(f"Frequency '{frequency_key}' not found")
+
+    if frequency_cls.is_realtime:
+        raise ValueError(f"Frequency '{frequency_key}' is realtime, not a digest frequency")
+
+    # Get all channels that support digest functionality
+    digest_channels = [channel_cls() for channel_cls in registry.get_all_channels() if channel_cls.supports_digest]
+
+    if not digest_channels:
+        logger.warning("No channels support digest functionality")
+        return 0
+
+    logger.info(f"Processing {frequency_cls.name} digests for {len(digest_channels)} channel(s)...")
+
+    all_notification_types = registry.get_all_types()
+    total_digests_sent = 0
+
+    for channel in digest_channels:
+        channel_digests_sent = _send_digest_for_channel(channel, frequency_cls, all_notification_types, dry_run)
+        total_digests_sent += channel_digests_sent
+
+        if dry_run:
+            logger.info(f"  {channel.name}: Would have sent {channel_digests_sent} digests")
+        else:
+            logger.info(f"  {channel.name}: Sent {channel_digests_sent} digests")
+
+    return total_digests_sent
+
+
+def _send_digest_for_channel(
+    channel: Any,
+    frequency_cls: type[NotificationFrequency],
+    all_notification_types: list[type[NotificationType]],
+    dry_run: bool,
+) -> int:
+    """
+    Send digests for a specific channel and frequency.
+
+    Args:
+        channel: Channel instance to send digests through
+        frequency_cls: Frequency class being processed
+        all_notification_types: List of all registered notification types
+        dry_run: If True, don't actually send
+
+    Returns:
+        Number of digests sent for this channel
+    """
+    # Find all users who have unsent, unread notifications for this channel
+    users_with_notifications = User.objects.filter(
+        notifications__email_sent_at__isnull=True,  # TODO: This should be channel-agnostic
+        notifications__read__isnull=True,
+        notifications__channels__icontains=f'"{channel.key}"',
+    ).distinct()
+
+    digests_sent = 0
+
+    for user in users_with_notifications:
+        # Determine which notification types should use this frequency for this user
+        relevant_types = _get_notification_types_for_frequency(user, frequency_cls, all_notification_types)
+
+        if not relevant_types:
+            continue
+
+        # Get unsent notifications for these types
+        # Exclude read notifications - don't send what user already saw on website
+        relevant_type_keys = [nt.key for nt in relevant_types]
+        notifications = Notification.objects.filter(
+            recipient=user,
+            notification_type__in=relevant_type_keys,
+            email_sent_at__isnull=True,  # TODO: This should be channel-agnostic
+            read__isnull=True,
+            channels__icontains=f'"{channel.key}"',
+        ).order_by("-added")
+
+        if notifications.exists():
+            logger.debug(
+                f"    User {user.email}: {notifications.count()} notifications for {frequency_cls.name} digest"
+            )
+
+            if not dry_run:
+                channel.send_digest(notifications, frequency_cls)
+
+            digests_sent += 1
+
+            # List notification subjects for debugging
+            for notification in notifications[:3]:  # Show first 3
+                logger.debug(f"      - {notification.subject or notification.text[:30]}")
+            if notifications.count() > 3:
+                logger.debug(f"      ... and {notifications.count() - 3} more")
+
+    return digests_sent
+
+
+def _get_notification_types_for_frequency(
+    user: AbstractUser,
+    wanted_frequency: type[NotificationFrequency],
+    all_notification_types: list[type[NotificationType]],
+) -> list[type[NotificationType]]:
+    """
+    Get all notification types that should use this frequency for the given user.
+    This includes both explicit preferences and types that default to this frequency.
+    Since notifications are only created for enabled channels, we don't need to check is_enabled.
+
+    Args:
+        user: The user to check preferences for
+        wanted_frequency: The frequency to filter by (e.g. DailyFrequency, RealtimeFrequency)
+        all_notification_types: List of all registered notification type classes
+
+    Returns:
+        List of notification type classes that use this frequency for this user
+    """
+    relevant_types: list[type[NotificationType]] = []
+
+    for notification_type in all_notification_types:
+        user_frequency = notification_type.get_email_frequency(user)  # TODO: This should be channel-agnostic
+        if user_frequency.key == wanted_frequency.key:
+            relevant_types.append(notification_type)
+
+    return relevant_types