From 4cefd02c26b9f750e54346379642aa34e6f048a7 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Fri, 10 Oct 2025 21:57:20 +0200 Subject: [PATCH 01/15] Improve email sending - Make it easier to send email via different means than just Django's send_mail (closes #12) - Make it easier to call the send_digest_emails via different means than just Django's management commands (closes #9) --- generic_notifications/channels.py | 71 +++++-- generic_notifications/digest.py | 154 ++++++++++++++++ .../management/commands/send_digest_emails.py | 124 ++----------- generic_notifications/models.py | 4 +- tests/test_channels.py | 174 ++++++++++++++++-- 5 files changed, 383 insertions(+), 144 deletions(-) create mode 100644 generic_notifications/digest.py diff --git a/generic_notifications/channels.py b/generic_notifications/channels.py index 078becc..993b48c 100644 --- a/generic_notifications/channels.py +++ b/generic_notifications/channels.py @@ -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,30 @@ 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, user: Any, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None + ) -> None: + """ + Send a digest to a specific user with specific notifications. + Override in subclasses that support digest delivery. + + Args: + user: User instance + notifications: QuerySet of notifications to include in digest + 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 +107,7 @@ class EmailChannel(NotificationChannel): key = "email" name = "Email" + supports_digest = True def process(self, notification: "Notification") -> None: """ @@ -96,9 +122,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 +167,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,9 +182,8 @@ 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, user: Any, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None ): """ Send a digest email to a specific user with specific notifications. @@ -226,13 +249,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 +262,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) + """ + send_mail( + subject=subject, + message=text_message, + from_email=settings.DEFAULT_FROM_EMAIL, + recipient_list=[recipient], + html_message=html_message, + fail_silently=False, + ) diff --git a/generic_notifications/digest.py b/generic_notifications/digest.py new file mode 100644 index 0000000..495105d --- /dev/null +++ b/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(user, 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 diff --git a/generic_notifications/management/commands/send_digest_emails.py b/generic_notifications/management/commands/send_digest_emails.py index e371f6b..106dc9a 100644 --- a/generic_notifications/management/commands/send_digest_emails.py +++ b/generic_notifications/management/commands/send_digest_emails.py @@ -1,17 +1,8 @@ import logging -from django.contrib.auth import get_user_model -from django.contrib.auth.models import AbstractUser from django.core.management.base import BaseCommand -from generic_notifications.channels import EmailChannel -from generic_notifications.frequencies import NotificationFrequency -from generic_notifications.models import Notification -from generic_notifications.registry import registry -from generic_notifications.types import NotificationType - -User = get_user_model() - +from generic_notifications.digest import send_digest_notifications logger = logging.getLogger(__name__) @@ -41,106 +32,19 @@ def handle(self, *args, **options): if dry_run: original_level = logger.level logger.setLevel(logging.INFO) - logger.info("DRY RUN - No emails will be sent") + logger.info("DRY RUN - No notifications will be sent") - # Verify email channel is registered try: - registry.get_channel(EmailChannel.key) - except KeyError: - logger.error("Email channel not registered") + total_digests_sent = send_digest_notifications(target_frequency, dry_run) + + if dry_run: + logger.info(f"DRY RUN: Would have sent {total_digests_sent} digest notifications") + # Restore original log level + if original_level is not None: + logger.setLevel(original_level) + else: + logger.info(f"Successfully sent {total_digests_sent} digest notifications") + + except (KeyError, ValueError) as e: + logger.error(str(e)) return - - # Setup - all_notification_types = registry.get_all_types() - - # Get the specific frequency (required argument) - try: - frequency_cls = registry.get_frequency(target_frequency) - except KeyError: - logger.error(f"Frequency '{target_frequency}' not found") - return - - if frequency_cls.is_realtime: - logger.error(f"Frequency '{target_frequency}' is realtime, not a digest frequency") - return - - total_emails_sent = 0 - - logger.info(f"Processing {frequency_cls.name} digests...") - - # Find all users who have unsent, unread notifications for email channel - users_with_notifications = User.objects.filter( - notifications__email_sent_at__isnull=True, - notifications__read__isnull=True, - notifications__channels__icontains=f'"{EmailChannel.key}"', - ).distinct() - - for user in users_with_notifications: - # Determine which notification types should use this frequency for this user - relevant_types = self.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 email 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, - read__isnull=True, - channels__icontains=f'"{EmailChannel.key}"', - ).order_by("-added") - - if notifications.exists(): - logger.info( - f" User {user.email}: {notifications.count()} notifications for {frequency_cls.name} digest" - ) - - if not dry_run: - EmailChannel.send_digest_emails(user, notifications, frequency_cls) - - total_emails_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") - - if dry_run: - logger.info(f"DRY RUN: Would have sent {total_emails_sent} digest emails") - # Restore original log level - if original_level is not None: - logger.setLevel(original_level) - else: - logger.info(f"Successfully sent {total_emails_sent} digest emails") - - def get_notification_types_for_frequency( - self, - 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) - if user_frequency.key == wanted_frequency.key: - relevant_types.append(notification_type) - - return relevant_types diff --git a/generic_notifications/models.py b/generic_notifications/models.py index 0a128f7..95ec681 100644 --- a/generic_notifications/models.py +++ b/generic_notifications/models.py @@ -20,11 +20,11 @@ class NotificationQuerySet(models.QuerySet): def prefetch(self): """Prefetch related objects""" qs = self.select_related("recipient", "actor") - + # Only add target prefetching on Django 5.0+ due to GenericForeignKey limitations if django.VERSION >= (5, 0): qs = qs.prefetch_related("target") - + return qs def for_channel(self, channel: type[NotificationChannel] = WebsiteChannel): diff --git a/tests/test_channels.py b/tests/test_channels.py index 026c434..bedaadc 100644 --- a/tests/test_channels.py +++ b/tests/test_channels.py @@ -141,13 +141,13 @@ def test_process_digest_frequency(self): self.assertIsNone(notification.email_sent_at) @override_settings(DEFAULT_FROM_EMAIL="test@example.com") - def test_send_email_now_basic(self): + def test_send_now_basic(self): notification = Notification.objects.create( recipient=self.user, notification_type="test_type", subject="Test Subject", text="Test message" ) channel = EmailChannel() - channel.send_email_now(notification) + channel.send_now(notification) # Check email was sent self.assertEqual(len(mail.outbox), 1) @@ -162,12 +162,12 @@ def test_send_email_now_basic(self): self.assertIsNotNone(notification.email_sent_at) @override_settings(DEFAULT_FROM_EMAIL="test@example.com") - def test_send_email_now_uses_get_methods(self): + def test_send_now_uses_get_methods(self): # Create notification without stored subject/text to test dynamic generation notification = Notification.objects.create(recipient=self.user, notification_type="test_type") channel = EmailChannel() - channel.send_email_now(notification) + channel.send_now(notification) # Check that email was sent using the get_subject/get_text methods self.assertEqual(len(mail.outbox), 1) @@ -179,7 +179,7 @@ def test_send_email_now_uses_get_methods(self): @override_settings(DEFAULT_FROM_EMAIL="test@example.com") @patch("generic_notifications.channels.render_to_string") - def test_send_email_now_with_template(self, mock_render): + def test_send_now_with_template(self, mock_render): # Set up mock to return different values for different templates def mock_render_side_effect(template_name, context): if template_name.endswith("_subject.txt"): @@ -197,7 +197,7 @@ def mock_render_side_effect(template_name, context): ) channel = EmailChannel() - channel.send_email_now(notification) + channel.send_now(notification) # Check templates were rendered (subject, HTML, then text) self.assertEqual(mock_render.call_count, 3) @@ -227,13 +227,13 @@ def mock_render_side_effect(template_name, context): self.assertEqual(email.alternatives[0][0], "Test HTML") # type: ignore @override_settings(DEFAULT_FROM_EMAIL="test@example.com") - def test_send_email_now_template_error_fallback(self): + def test_send_now_template_error_fallback(self): notification = Notification.objects.create( recipient=self.user, notification_type="test_type", subject="Test Subject" ) channel = EmailChannel() - channel.send_email_now(notification) + channel.send_now(notification) # Should still send email without HTML self.assertEqual(len(mail.outbox), 1) @@ -245,7 +245,7 @@ def test_send_email_now_template_error_fallback(self): 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_emails(self.user, empty_notifications) + EmailChannel().send_digest(self.user, empty_notifications) # No email should be sent when no notifications exist self.assertEqual(len(mail.outbox), 0) @@ -263,7 +263,7 @@ def test_send_digest_emails_basic(self): notifications = Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) # Send digest email for this user - EmailChannel.send_digest_emails(self.user, notifications) + EmailChannel().send_digest(self.user, notifications) # Check email was sent self.assertEqual(len(mail.outbox), 1) @@ -283,7 +283,7 @@ def test_send_digest_emails_with_frequency(self): Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") - EmailChannel.send_digest_emails( + EmailChannel().send_digest( self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) ) @@ -297,7 +297,7 @@ def test_send_digest_emails_without_frequency(self): Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") - EmailChannel.send_digest_emails( + EmailChannel().send_digest( self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) ) @@ -315,7 +315,7 @@ def test_send_digest_emails_text_limit(self): for i in range(15) ] - EmailChannel.send_digest_emails( + EmailChannel().send_digest( self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) ) @@ -334,7 +334,7 @@ def test_send_digest_emails_with_html_template(self, mock_render): Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") - EmailChannel.send_digest_emails( + EmailChannel().send_digest( self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) ) @@ -355,7 +355,7 @@ def test_send_digest_emails_with_html_template(self, mock_render): self.assertEqual(html_call[0][1]["user"], self.user) self.assertEqual(html_call[0][1]["count"], 1) - def test_send_email_now_fallback_includes_url(self): + def test_send_now_fallback_includes_url(self): """Test that fallback email content includes URL when available""" notification = Notification.objects.create( recipient=self.user, @@ -366,7 +366,7 @@ def test_send_email_now_fallback_includes_url(self): url="https://example.com/test/url/123", ) - EmailChannel().send_email_now(notification) + EmailChannel().send_now(notification) # Check that one email was sent self.assertEqual(len(mail.outbox), 1) @@ -394,7 +394,7 @@ def test_send_digest_emails_fallback_includes_urls(self): url="https://example.com/url/2", ) - EmailChannel.send_digest_emails( + EmailChannel().send_digest( self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) ) @@ -410,3 +410,143 @@ def test_send_digest_emails_fallback_includes_urls(self): - First notification https://example.com/url/1""" self.assertEqual(sent_email.body, expected_body) + + +class CustomEmailChannelTest(TestCase): + """Test that custom EmailChannel subclasses work correctly with digest functionality.""" + + user: Any + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.user = User.objects.create_user(username="user1", email="test@example.com", password="testpass") + + def setUp(self): + # Clear any existing emails + mail.outbox.clear() + + def test_custom_email_channel_send_email_override(self): + """Test that a custom EmailChannel subclass can override _send_email method.""" + + class TestEmailChannel(EmailChannel): + """Custom email channel that tracks calls to _send_email.""" + + key = "test_email" + name = "Test Email" + + def __init__(self): + super().__init__() + self.sent_emails = [] + + def _send_email( + self, recipient: str, subject: str, text_message: str, html_message: str | None = None + ) -> None: + """Override to track calls instead of actually sending.""" + self.sent_emails.append( + { + "recipient": recipient, + "subject": subject, + "text_message": text_message, + "html_message": html_message, + } + ) + # Don't call super() - we don't want to actually send emails + + # Create notifications + notification1 = Notification.objects.create( + recipient=self.user, + notification_type=TestNotificationType.key, + channels=["test_email"], + subject="Test Subject 1", + text="Test notification 1", + ) + + notification2 = Notification.objects.create( + recipient=self.user, + notification_type=TestNotificationType.key, + channels=["test_email"], + subject="Test Subject 2", + text="Test notification 2", + ) + + notifications = Notification.objects.filter(id__in=[notification1.id, notification2.id]) + + # Test the custom channel + custom_channel = TestEmailChannel() + custom_channel.send_digest(self.user, notifications) + + # Verify the custom _send_email method was called + self.assertEqual(len(custom_channel.sent_emails), 1) + sent_email = custom_channel.sent_emails[0] + + # Check the email details + self.assertEqual(sent_email["recipient"], "test@example.com") + self.assertIn("2 new notifications", sent_email["subject"]) + self.assertIn("Test notification 1", sent_email["text_message"]) + self.assertIn("Test notification 2", sent_email["text_message"]) + + # Verify no actual emails were sent via Django's mail system + self.assertEqual(len(mail.outbox), 0) + + # Check that notifications were marked as sent + notification1.refresh_from_db() + notification2.refresh_from_db() + self.assertIsNotNone(notification1.email_sent_at) + self.assertIsNotNone(notification2.email_sent_at) + + def test_custom_email_channel_send_now_override(self): + """Test that a custom EmailChannel subclass works with send_now.""" + + class AsyncEmailChannel(EmailChannel): + """Custom email channel that queues emails instead of sending immediately.""" + + key = "async_email" + name = "Async Email" + + def __init__(self): + super().__init__() + self.queued_emails = [] + + def _send_email( + self, recipient: str, subject: str, text_message: str, html_message: str | None = None + ) -> None: + """Queue email for later processing instead of sending immediately.""" + self.queued_emails.append( + { + "recipient": recipient, + "subject": subject, + "text_message": text_message, + "html_message": html_message, + "queued_at": "now", # In real implementation, would use timezone.now() + } + ) + + # Create notification + notification = Notification.objects.create( + recipient=self.user, + notification_type=TestNotificationType.key, + channels=["async_email"], + subject="Realtime Test", + text="Realtime notification", + ) + + # Test the custom channel with send_now + custom_channel = AsyncEmailChannel() + custom_channel.send_now(notification) + + # Verify the email was queued instead of sent + self.assertEqual(len(custom_channel.queued_emails), 1) + queued_email = custom_channel.queued_emails[0] + + self.assertEqual(queued_email["recipient"], "test@example.com") + self.assertEqual(queued_email["subject"], "Realtime Test") + self.assertIn("Realtime notification", queued_email["text_message"]) + self.assertIsNotNone(queued_email["queued_at"]) + + # Verify no actual emails were sent + self.assertEqual(len(mail.outbox), 0) + + # Check that notification was marked as sent + notification.refresh_from_db() + self.assertIsNotNone(notification.email_sent_at) From 881d692a0880287025fcd881b4debb246ad17a5f Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Fri, 10 Oct 2025 22:03:49 +0200 Subject: [PATCH 02/15] Internal API improvements --- generic_notifications/channels.py | 29 +++++++++++++++-------------- generic_notifications/digest.py | 2 +- tests/test_channels.py | 30 ++++++++++-------------------- 3 files changed, 26 insertions(+), 35 deletions(-) diff --git a/generic_notifications/channels.py b/generic_notifications/channels.py index 993b48c..875107a 100644 --- a/generic_notifications/channels.py +++ b/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 @@ -46,15 +46,14 @@ def send_now(self, notification: "Notification") -> None: raise NotImplementedError(f"{self.__class__.__name__} does not support realtime sending") def send_digest( - self, user: Any, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None + self, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None ) -> None: """ - Send a digest to a specific user with specific notifications. + Send a digest with specific notifications. Override in subclasses that support digest delivery. 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 context """ raise NotImplementedError(f"{self.__class__.__name__} does not support digest sending") @@ -167,7 +166,7 @@ def send_now(self, notification: "Notification") -> None: if absolute_url: text_message += f"\n{absolute_url}" - self._send_email( + self.send_email( recipient=notification.recipient.email, subject=subject, text_message=text_message, @@ -183,20 +182,22 @@ def send_now(self, notification: "Notification") -> None: logger.error(f"Failed to send email for notification {notification.id}: {e}") def send_digest( - self, user: Any, notifications: "QuerySet[Notification]", frequency: type[NotificationFrequency] | None = None + 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"]] = {} @@ -249,7 +250,7 @@ def send_digest( message_lines.append(f"... and {notifications_count - 10} more") text_message = "\n".join(message_lines) - self._send_email( + self.send_email( recipient=user.email, subject=subject, text_message=text_message, @@ -263,7 +264,7 @@ def send_digest( 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: + 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). @@ -274,7 +275,7 @@ def _send_email(self, recipient: str, subject: str, text_message: str, html_mess text_message: Plain text email content html_message: HTML email content (optional) """ - send_mail( + django_send_mail( subject=subject, message=text_message, from_email=settings.DEFAULT_FROM_EMAIL, diff --git a/generic_notifications/digest.py b/generic_notifications/digest.py index 495105d..eb54528 100644 --- a/generic_notifications/digest.py +++ b/generic_notifications/digest.py @@ -113,7 +113,7 @@ def _send_digest_for_channel( ) if not dry_run: - channel.send_digest(user, notifications, frequency_cls) + channel.send_digest(notifications, frequency_cls) digests_sent += 1 diff --git a/tests/test_channels.py b/tests/test_channels.py index bedaadc..723d8f5 100644 --- a/tests/test_channels.py +++ b/tests/test_channels.py @@ -245,7 +245,7 @@ def test_send_now_template_error_fallback(self): 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(self.user, empty_notifications) + EmailChannel().send_digest(empty_notifications) # No email should be sent when no notifications exist self.assertEqual(len(mail.outbox), 0) @@ -263,7 +263,7 @@ def test_send_digest_emails_basic(self): notifications = Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) # Send digest email for this user - EmailChannel().send_digest(self.user, notifications) + EmailChannel().send_digest(notifications) # Check email was sent self.assertEqual(len(mail.outbox), 1) @@ -283,9 +283,7 @@ def test_send_digest_emails_with_frequency(self): Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") - EmailChannel().send_digest( - self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) - ) + EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) email = mail.outbox[0] self.assertEqual(email.subject, "Digest - 1 new notification") @@ -297,9 +295,7 @@ def test_send_digest_emails_without_frequency(self): Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") - EmailChannel().send_digest( - self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) - ) + EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) email = mail.outbox[0] self.assertEqual(email.subject, "Digest - 1 new notification") @@ -315,9 +311,7 @@ def test_send_digest_emails_text_limit(self): for i in range(15) ] - EmailChannel().send_digest( - self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) - ) + EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) # The implementation may not have this feature, so we'll just check that email was sent self.assertEqual(len(mail.outbox), 1) @@ -334,9 +328,7 @@ def test_send_digest_emails_with_html_template(self, mock_render): Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") - EmailChannel().send_digest( - self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) - ) + EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) # Check templates were rendered (subject, HTML, then text) self.assertEqual(mock_render.call_count, 3) @@ -394,9 +386,7 @@ def test_send_digest_emails_fallback_includes_urls(self): url="https://example.com/url/2", ) - EmailChannel().send_digest( - self.user, Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) - ) + EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) # Check that one email was sent self.assertEqual(len(mail.outbox), 1) @@ -439,7 +429,7 @@ def __init__(self): super().__init__() self.sent_emails = [] - def _send_email( + def send_email( self, recipient: str, subject: str, text_message: str, html_message: str | None = None ) -> None: """Override to track calls instead of actually sending.""" @@ -474,7 +464,7 @@ def _send_email( # Test the custom channel custom_channel = TestEmailChannel() - custom_channel.send_digest(self.user, notifications) + custom_channel.send_digest(notifications) # Verify the custom _send_email method was called self.assertEqual(len(custom_channel.sent_emails), 1) @@ -508,7 +498,7 @@ def __init__(self): super().__init__() self.queued_emails = [] - def _send_email( + def send_email( self, recipient: str, subject: str, text_message: str, html_message: str | None = None ) -> None: """Queue email for later processing instead of sending immediately.""" From 5bdd8cb6e46b34d8b4b7e34f03def83a3cde8f97 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 18:33:24 +0200 Subject: [PATCH 03/15] - Renamed NotificationChannel -> BaseChannel - Renamed NotificationFrequency -> BaseFrequency - Renamed EmailFrequency model to NotificationFrequency model, it shouldn't be tied to "email" (closes #15) - Renamed NotificationType.default_email_frequency -> NotificationType.default_frequency - Renamed NotificationType.set_email_frequency -> NotificationType.set_frequency - Renamed NotificationType.get_email_frequency -> NotificationType.get_frequency - Renamed NotificationType.reset_email_frequency_to_default -> NotificationType.reset_frequency_to_default - Store sent_at per channel instead of once per notification, stored in new NotificationChannel model (closes #16) Breaking changes: if you made custom channels, frequencies or notification types, than please check the renames above. For most users these renames won't be breaking changes. --- generic_notifications/__init__.py | 12 +- generic_notifications/apps.py | 1 + generic_notifications/channels.py | 25 ++- generic_notifications/digest.py | 16 +- generic_notifications/frequencies.py | 14 +- ...blednotificationtypechannel_id_and_more.py | 80 +++++++++ ...tion_notification_channels_gin_and_more.py | 47 ++++++ ...me_emailfrequency_notificationfrequency.py | 28 ++++ generic_notifications/models.py | 63 ++++--- generic_notifications/preferences.py | 14 +- generic_notifications/registry.py | 36 ++-- generic_notifications/types.py | 58 +++---- generic_notifications/utils.py | 8 +- tests/test_channels.py | 156 +++++++++++------- tests/test_helpers.py | 38 +++++ tests/test_management_commands.py | 144 ++++++++-------- tests/test_models.py | 106 +++++------- tests/test_performance.py | 27 +-- tests/test_preferences.py | 16 +- tests/test_registry.py | 32 ++-- tests/test_types.py | 34 ++-- tests/test_utils.py | 52 +++--- 22 files changed, 631 insertions(+), 376 deletions(-) create mode 100644 generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py create mode 100644 generic_notifications/migrations/0003_remove_notification_notification_channels_gin_and_more.py create mode 100644 generic_notifications/migrations/0004_rename_emailfrequency_notificationfrequency.py create mode 100644 tests/test_helpers.py diff --git a/generic_notifications/__init__.py b/generic_notifications/__init__.py index 3e22a94..f182d13 100644 --- a/generic_notifications/__init__.py +++ b/generic_notifications/__init__.py @@ -62,13 +62,12 @@ def send_notification( if not enabled_channel_classes: return None - # Create the notification record with enabled channels + # Create the notification record notification = Notification( recipient=recipient, notification_type=notification_type.key, actor=actor, target=target, - channels=[channel_cls.key for channel_cls in enabled_channel_classes], subject=subject, text=text, url=url, @@ -81,6 +80,15 @@ def send_notification( if notification_type.should_save(notification): notification.save() + # Create NotificationChannel entries for each enabled channel + from .models import NotificationChannel + + 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: diff --git a/generic_notifications/apps.py b/generic_notifications/apps.py index 7b979b7..b41528b 100644 --- a/generic_notifications/apps.py +++ b/generic_notifications/apps.py @@ -4,3 +4,4 @@ class GenericNotificationsConfig(AppConfig): name = "generic_notifications" verbose_name = "Generic Notifications" + default_auto_field = "django.db.models.BigAutoField" diff --git a/generic_notifications/channels.py b/generic_notifications/channels.py index 875107a..d128877 100644 --- a/generic_notifications/channels.py +++ b/generic_notifications/channels.py @@ -7,16 +7,15 @@ from django.db.models import QuerySet from django.template.defaultfilters import pluralize from django.template.loader import render_to_string -from django.utils import timezone -from .frequencies import NotificationFrequency +from .frequencies import BaseFrequency from .registry import registry if TYPE_CHECKING: from .models import Notification -class NotificationChannel(ABC): +class BaseChannel(ABC): """ Base class for all notification channels. """ @@ -46,7 +45,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[NotificationFrequency] | None = None + self, notifications: "QuerySet[Notification]", frequency: type[BaseFrequency] | None = None ) -> None: """ Send a digest with specific notifications. @@ -59,7 +58,7 @@ def send_digest( raise NotImplementedError(f"{self.__class__.__name__} does not support digest sending") -def register(cls: Type[NotificationChannel]) -> Type[NotificationChannel]: +def register(cls: Type[BaseChannel]) -> Type[BaseChannel]: """ Decorator that registers a NotificationChannel subclass. @@ -80,7 +79,7 @@ def process(self, notification): @register -class WebsiteChannel(NotificationChannel): +class WebsiteChannel(BaseChannel): """ Channel for displaying notifications on the website. Notifications are stored in the database and displayed in the UI. @@ -98,7 +97,7 @@ def process(self, notification: "Notification") -> None: @register -class EmailChannel(NotificationChannel): +class EmailChannel(BaseChannel): """ Channel for sending notifications via email. Supports both realtime delivery and daily digest batching. @@ -117,7 +116,7 @@ def process(self, notification: "Notification") -> None: """ # Get notification type class from key notification_type_cls = registry.get_type(notification.notification_type) - frequency_cls = notification_type_cls.get_email_frequency(notification.recipient) + frequency_cls = notification_type_cls.get_frequency(notification.recipient) # Send immediately if realtime, otherwise leave for digest if frequency_cls and frequency_cls.is_realtime: @@ -174,16 +173,13 @@ def send_now(self, notification: "Notification") -> None: ) # Mark as sent - notification.email_sent_at = timezone.now() - notification.save(update_fields=["email_sent_at"]) + notification.mark_sent_on_channel(self.__class__) except Exception as e: 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[NotificationFrequency] | None = None - ): + def send_digest(self, notifications: "QuerySet[Notification]", frequency: type[BaseFrequency] | None = None): """ Send a digest email with specific notifications. This method is used by the management command. @@ -258,7 +254,8 @@ def send_digest( ) # Mark all as sent - notifications.update(email_sent_at=timezone.now()) + for notification in notifications: + notification.mark_sent_on_channel(self.__class__) except Exception as e: logger = logging.getLogger(__name__) diff --git a/generic_notifications/digest.py b/generic_notifications/digest.py index eb54528..01080b3 100644 --- a/generic_notifications/digest.py +++ b/generic_notifications/digest.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.models import AbstractUser -from .frequencies import NotificationFrequency +from .frequencies import BaseFrequency from .models import Notification from .registry import registry from .types import NotificationType @@ -64,7 +64,7 @@ def send_digest_notifications(frequency_key: str, dry_run: bool = False) -> int: def _send_digest_for_channel( channel: Any, - frequency_cls: type[NotificationFrequency], + frequency_cls: type[BaseFrequency], all_notification_types: list[type[NotificationType]], dry_run: bool, ) -> int: @@ -82,9 +82,9 @@ def _send_digest_for_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}"', + notifications__channels__channel=channel.key, + notifications__channels__sent_at__isnull=True, ).distinct() digests_sent = 0 @@ -102,9 +102,9 @@ def _send_digest_for_channel( 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}"', + channels__channel=channel.key, + channels__sent_at__isnull=True, ).order_by("-added") if notifications.exists(): @@ -128,7 +128,7 @@ def _send_digest_for_channel( def _get_notification_types_for_frequency( user: AbstractUser, - wanted_frequency: type[NotificationFrequency], + wanted_frequency: type[BaseFrequency], all_notification_types: list[type[NotificationType]], ) -> list[type[NotificationType]]: """ @@ -147,7 +147,7 @@ def _get_notification_types_for_frequency( 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 + user_frequency = notification_type.get_frequency(user) if user_frequency.key == wanted_frequency.key: relevant_types.append(notification_type) diff --git a/generic_notifications/frequencies.py b/generic_notifications/frequencies.py index 613ea5f..8a68b93 100644 --- a/generic_notifications/frequencies.py +++ b/generic_notifications/frequencies.py @@ -4,9 +4,9 @@ from .registry import registry -class NotificationFrequency(ABC): +class BaseFrequency(ABC): """ - Represents an email frequency option for notifications. + Represents a frequency option for notifications. """ key: str @@ -18,7 +18,7 @@ def __str__(self) -> str: return self.name -def register(cls: Type[NotificationFrequency]) -> Type[NotificationFrequency]: +def register(cls: Type[BaseFrequency]) -> Type[BaseFrequency]: """ Decorator that registers a NotificationFrequency subclass. @@ -38,16 +38,16 @@ class WeeklyFrequency(NotificationFrequency): @register -class RealtimeFrequency(NotificationFrequency): +class RealtimeFrequency(BaseFrequency): key = "realtime" name = "Real-time" is_realtime = True - description = "Send emails immediately when notifications are created" + description = "Send immediately when notifications are created" @register -class DailyFrequency(NotificationFrequency): +class DailyFrequency(BaseFrequency): key = "daily" name = "Daily digest" is_realtime = False - description = "Bundle notifications into a daily email" + description = "Bundle notifications into a daily digest" diff --git a/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py b/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py new file mode 100644 index 0000000..6692529 --- /dev/null +++ b/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py @@ -0,0 +1,80 @@ +# Generated by Django 5.2.5 on 2025-10-11 12:18 + +import django.db.models.deletion +from django.db import migrations, models + + +def migrate_channels_to_notificationchannel(apps, schema_editor): + """Migrate data from channels JSONField to NotificationChannel model.""" + Notification = apps.get_model("generic_notifications", "Notification") + NotificationChannel = apps.get_model("generic_notifications", "NotificationChannel") + + for notification in Notification.objects.all(): + # Create NotificationChannel entries for each channel + for channel in notification.channels: + delivery, created = NotificationChannel.objects.get_or_create( + notification=notification, + channel=channel, + ) + + # If this is the email channel and email_sent_at is set, mark it as sent + if notification.email_sent_at: + delivery.sent_at = notification.email_sent_at + delivery.save() + + +def reverse_migrate_channels(apps, schema_editor): + """Reverse migration: copy data back from NotificationChannel to channels JSONField.""" + Notification = apps.get_model("generic_notifications", "Notification") + NotificationChannel = apps.get_model("generic_notifications", "NotificationChannel") + + for notification in Notification.objects.all(): + # Rebuild channels list from NotificationChannel entries + channels = list(notification.channels.values_list("channel", flat=True)) + notification.channels = channels + + # Restore email_sent_at from email NotificationChannel + try: + email_delivery = notification.channels.get(channel="email") + if email_delivery.sent_at: + notification.email_sent_at = email_delivery.sent_at + except NotificationChannel.DoesNotExist: + pass + + notification.save() + + +class Migration(migrations.Migration): + dependencies = [ + ("generic_notifications", "0001_initial"), + ] + + operations = [ + migrations.CreateModel( + name="NotificationChannel", + fields=[ + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ("channel", models.CharField(max_length=20)), + ("sent_at", models.DateTimeField(blank=True, null=True)), + ( + "notification", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="channels", + to="generic_notifications.notification", + ), + ), + ], + options={ + "indexes": [ + models.Index(fields=["notification", "channel", "sent_at"], name="generic_not_notific_956af2_idx"), + models.Index(fields=["channel", "sent_at"], name="generic_not_channel_cfce4f_idx"), + ], + "unique_together": {("notification", "channel")}, + }, + ), + migrations.RunPython( + migrate_channels_to_notificationchannel, + reverse_migrate_channels, + ), + ] diff --git a/generic_notifications/migrations/0003_remove_notification_notification_channels_gin_and_more.py b/generic_notifications/migrations/0003_remove_notification_notification_channels_gin_and_more.py new file mode 100644 index 0000000..e95df13 --- /dev/null +++ b/generic_notifications/migrations/0003_remove_notification_notification_channels_gin_and_more.py @@ -0,0 +1,47 @@ +# Generated by Django 5.2.5 on 2025-10-11 12:28 + +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("contenttypes", "0002_remove_content_type_name"), + ("generic_notifications", "0002_alter_disablednotificationtypechannel_id_and_more"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.RemoveIndex( + model_name="notification", + name="notification_channels_gin", + ), + migrations.RemoveIndex( + model_name="notification", + name="notification_unread_channel", + ), + migrations.RemoveIndex( + model_name="notification", + name="notification_recipient_channel", + ), + migrations.RemoveIndex( + model_name="notification", + name="notification_user_email_digest", + ), + migrations.RemoveField( + model_name="notification", + name="channels", + ), + migrations.RemoveField( + model_name="notification", + name="email_sent_at", + ), + migrations.AddIndex( + model_name="notification", + index=models.Index(fields=["recipient", "read"], name="notification_unread_idx"), + ), + migrations.AddIndex( + model_name="notification", + index=models.Index(fields=["recipient", "added"], name="notification_recipient_idx"), + ), + ] diff --git a/generic_notifications/migrations/0004_rename_emailfrequency_notificationfrequency.py b/generic_notifications/migrations/0004_rename_emailfrequency_notificationfrequency.py new file mode 100644 index 0000000..90b6629 --- /dev/null +++ b/generic_notifications/migrations/0004_rename_emailfrequency_notificationfrequency.py @@ -0,0 +1,28 @@ +# Generated by Django 5.2.5 on 2025-10-11 13:46 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("generic_notifications", "0003_remove_notification_notification_channels_gin_and_more"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.RenameModel( + old_name="EmailFrequency", + new_name="NotificationFrequency", + ), + migrations.AlterField( + model_name="notificationfrequency", + name="user", + field=models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="notification_frequencies", + to=settings.AUTH_USER_MODEL, + ), + ), + ] diff --git a/generic_notifications/models.py b/generic_notifications/models.py index 95ec681..342e37c 100644 --- a/generic_notifications/models.py +++ b/generic_notifications/models.py @@ -3,12 +3,11 @@ from django.contrib.auth import get_user_model from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType -from django.contrib.postgres.indexes import GinIndex from django.core.exceptions import ValidationError from django.db import models from django.utils import timezone -from .channels import NotificationChannel, WebsiteChannel +from .channels import BaseChannel, WebsiteChannel from .registry import registry User = get_user_model() @@ -27,9 +26,9 @@ def prefetch(self): return qs - def for_channel(self, channel: type[NotificationChannel] = WebsiteChannel): + def for_channel(self, channel: type[BaseChannel] = WebsiteChannel): """Filter notifications by channel""" - return self.filter(channels__icontains=f'"{channel.key}"') + return self.filter(channels__channel=channel.key) def unread(self): """Filter only unread notifications""" @@ -83,13 +82,14 @@ def __str__(self) -> str: return f"{self.user} disabled {self.notification_type} on {self.channel}" -class EmailFrequency(models.Model): +class NotificationFrequency(models.Model): """ - Email delivery frequency preference per notification type. - Default is `NotificationType.default_email_frequency` if no row exists. + Delivery frequency preference per notification type. + This applies to all channels that support the chosen frequency. + Default is `NotificationType.default_frequency` if no row exists. """ - user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="email_frequencies") + user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="notification_frequencies") notification_type = models.CharField(max_length=50) frequency = models.CharField(max_length=20) @@ -129,6 +129,27 @@ def __str__(self) -> str: return f"{self.user} - {self.notification_type}: {self.frequency}" +class NotificationChannel(models.Model): + """ + Tracks which channels a notification should be sent through and their delivery status. + """ + + notification = models.ForeignKey("Notification", on_delete=models.CASCADE, related_name="channels") + channel = models.CharField(max_length=20) # e.g., 'email', 'website', etc. + sent_at = models.DateTimeField(null=True, blank=True) + + class Meta: + unique_together = ["notification", "channel"] + indexes = [ + models.Index(fields=["notification", "channel", "sent_at"]), + models.Index(fields=["channel", "sent_at"]), # For digest queries + ] + + def __str__(self): + status = "sent" if self.sent_at else "pending" + return f"{self.notification} - {self.channel} ({status})" + + class Notification(models.Model): """ A specific notification instance for a user @@ -153,12 +174,6 @@ class Notification(models.Model): object_id = models.PositiveIntegerField(null=True, blank=True) target = GenericForeignKey("content_type", "object_id") - # Email tracking - email_sent_at = models.DateTimeField(null=True, blank=True) - - # Channels this notification is enabled for - channels = models.JSONField(default=list, blank=True) - # Flexible metadata for any extra data metadata = models.JSONField(default=dict, blank=True) @@ -166,12 +181,8 @@ class Notification(models.Model): class Meta: indexes = [ - GinIndex(fields=["channels"], name="notification_channels_gin"), - models.Index(fields=["recipient", "read", "channels"], name="notification_unread_channel"), - models.Index(fields=["recipient", "channels"], name="notification_recipient_channel"), - models.Index( - fields=["recipient", "email_sent_at", "read", "channels"], name="notification_user_email_digest" - ), + models.Index(fields=["recipient", "read"], name="notification_unread_idx"), + models.Index(fields=["recipient", "added"], name="notification_recipient_idx"), ] ordering = ["-added"] @@ -231,6 +242,18 @@ def get_text(self) -> str: except KeyError: return "You have a new notification" + def get_channels(self) -> list[str]: + """Get all channels this notification is configured for.""" + return list(self.channels.values_list("channel", flat=True)) + + def is_sent_on_channel(self, channel: type["BaseChannel"]) -> bool: + """Check if notification was sent on a specific channel.""" + return self.channels.filter(channel=channel.key, sent_at__isnull=False).exists() + + def mark_sent_on_channel(self, channel: type["BaseChannel"]) -> None: + """Mark notification as sent on a specific channel.""" + self.channels.filter(channel=channel.key).update(sent_at=timezone.now()) + @property def is_read(self) -> bool: return self.read is not None diff --git a/generic_notifications/preferences.py b/generic_notifications/preferences.py index 7254eba..0beb406 100644 --- a/generic_notifications/preferences.py +++ b/generic_notifications/preferences.py @@ -2,7 +2,7 @@ from django.contrib.auth.models import AbstractUser -from .models import DisabledNotificationTypeChannel, EmailFrequency +from .models import DisabledNotificationTypeChannel, NotificationFrequency from .registry import registry @@ -27,7 +27,9 @@ def get_notification_preferences(user: "AbstractUser") -> List[Dict[str, Any]]: ) # Get user's email frequency preferences - email_frequencies = dict(EmailFrequency.objects.filter(user=user).values_list("notification_type", "frequency")) + email_frequencies = dict( + NotificationFrequency.objects.filter(user=user).values_list("notification_type", "frequency") + ) # Build settings data structure settings_data = [] @@ -36,7 +38,7 @@ def get_notification_preferences(user: "AbstractUser") -> List[Dict[str, Any]]: type_data: Dict[str, Any] = { "notification_type": notification_type, "channels": {}, - "email_frequency": email_frequencies.get(type_key, notification_type.default_email_frequency.key), + "email_frequency": email_frequencies.get(type_key, notification_type.default_frequency.key), } for channel in channels.values(): @@ -68,7 +70,7 @@ def save_notification_preferences(user: "AbstractUser", form_data: Dict[str, Any """ # Clear existing preferences to rebuild from form data DisabledNotificationTypeChannel.objects.filter(user=user).delete() - EmailFrequency.objects.filter(user=user).delete() + NotificationFrequency.objects.filter(user=user).delete() notification_types = {nt.key: nt for nt in registry.get_all_types()} channels = {ch.key: ch for ch in registry.get_all_channels()} @@ -99,5 +101,5 @@ def save_notification_preferences(user: "AbstractUser", form_data: Dict[str, Any if frequency_value in frequencies: frequency_obj = frequencies[frequency_value] # Only save if different from default - if frequency_value != notification_type.default_email_frequency.key: - notification_type.set_email_frequency(user=user, frequency=frequency_obj) + if frequency_value != notification_type.default_frequency.key: + notification_type.set_frequency(user=user, frequency=frequency_obj) diff --git a/generic_notifications/registry.py b/generic_notifications/registry.py index 3570957..a791f9d 100644 --- a/generic_notifications/registry.py +++ b/generic_notifications/registry.py @@ -1,8 +1,8 @@ from typing import TYPE_CHECKING, Type if TYPE_CHECKING: - from .channels import NotificationChannel - from .frequencies import NotificationFrequency + from .channels import BaseChannel + from .frequencies import BaseFrequency from .types import NotificationType @@ -14,8 +14,8 @@ class NotificationRegistry: def __init__(self) -> None: self._type_classes: dict[str, Type["NotificationType"]] = {} - self._channel_classes: dict[str, Type["NotificationChannel"]] = {} - self._frequency_classes: dict[str, Type["NotificationFrequency"]] = {} + self._channel_classes: dict[str, Type["BaseChannel"]] = {} + self._frequency_classes: dict[str, Type["BaseFrequency"]] = {} self._registered_class_ids: set[int] = set() def _register(self, cls, base_class, registry_dict: dict, class_type_name: str, force: bool = False) -> None: @@ -42,27 +42,27 @@ def register_type(self, notification_type_class: Type["NotificationType"], force self._register(notification_type_class, NotificationType, self._type_classes, "NotificationType", force) - def register_channel(self, channel_class: Type["NotificationChannel"], force: bool = False) -> None: + def register_channel(self, channel_class: Type["BaseChannel"], force: bool = False) -> None: """Register a notification channel class""" - from .channels import NotificationChannel + from .channels import BaseChannel - self._register(channel_class, NotificationChannel, self._channel_classes, "NotificationChannel", force) + self._register(channel_class, BaseChannel, self._channel_classes, "BaseChannel", force) - def register_frequency(self, frequency_class: Type["NotificationFrequency"], force: bool = False) -> None: - """Register an email frequency option class""" - from .frequencies import NotificationFrequency + def register_frequency(self, frequency_class: Type["BaseFrequency"], force: bool = False) -> None: + """Register a frequency option class""" + from .frequencies import BaseFrequency - self._register(frequency_class, NotificationFrequency, self._frequency_classes, "NotificationFrequency", force) + self._register(frequency_class, BaseFrequency, self._frequency_classes, "BaseFrequency", force) def get_type(self, key: str) -> Type["NotificationType"]: """Get a registered notification type class by key""" return self._type_classes[key] - def get_channel(self, key: str) -> Type["NotificationChannel"]: + def get_channel(self, key: str) -> Type["BaseChannel"]: """Get a registered channel class by key""" return self._channel_classes[key] - def get_frequency(self, key: str) -> Type["NotificationFrequency"]: + def get_frequency(self, key: str) -> Type["BaseFrequency"]: """Get a registered frequency class by key""" return self._frequency_classes[key] @@ -70,15 +70,15 @@ def get_all_types(self) -> list[Type["NotificationType"]]: """Get all registered notification type classes""" return list(self._type_classes.values()) - def get_all_channels(self) -> list[Type["NotificationChannel"]]: + def get_all_channels(self) -> list[Type["BaseChannel"]]: """Get all registered channel classes""" return list(self._channel_classes.values()) - def get_all_frequencies(self) -> list[Type["NotificationFrequency"]]: + def get_all_frequencies(self) -> list[Type["BaseFrequency"]]: """Get all registered frequency classes""" return list(self._frequency_classes.values()) - def get_realtime_frequencies(self) -> list[Type["NotificationFrequency"]]: + def get_realtime_frequencies(self) -> list[Type["BaseFrequency"]]: """Get all frequencies marked as realtime""" return [cls for cls in self._frequency_classes.values() if cls.is_realtime] @@ -94,7 +94,7 @@ def unregister_type(self, type_class: Type["NotificationType"]) -> bool: """ return self._type_classes.pop(type_class.key, None) is not None - def unregister_channel(self, channel_class: Type["NotificationChannel"]) -> bool: + def unregister_channel(self, channel_class: Type["BaseChannel"]) -> bool: """ Unregister a channel by class. @@ -106,7 +106,7 @@ def unregister_channel(self, channel_class: Type["NotificationChannel"]) -> bool """ return self._channel_classes.pop(channel_class.key, None) is not None - def unregister_frequency(self, frequency_class: Type["NotificationFrequency"]) -> bool: + def unregister_frequency(self, frequency_class: Type["BaseFrequency"]) -> bool: """ Unregister a frequency by class. diff --git a/generic_notifications/types.py b/generic_notifications/types.py index c5bd958..792cf44 100644 --- a/generic_notifications/types.py +++ b/generic_notifications/types.py @@ -1,8 +1,8 @@ from abc import ABC from typing import TYPE_CHECKING, Any, Type -from .channels import EmailChannel, NotificationChannel -from .frequencies import DailyFrequency, NotificationFrequency, RealtimeFrequency +from .channels import BaseChannel, EmailChannel +from .frequencies import BaseFrequency, DailyFrequency, RealtimeFrequency from .registry import registry if TYPE_CHECKING: @@ -17,8 +17,8 @@ class NotificationType(ABC): key: str name: str description: str - default_email_frequency: Type[NotificationFrequency] = DailyFrequency - required_channels: list[Type[NotificationChannel]] = [] + default_frequency: Type[BaseFrequency] = DailyFrequency + required_channels: list[Type[BaseChannel]] = [] def __str__(self) -> str: return self.name @@ -51,53 +51,53 @@ def get_text(self, notification: "Notification") -> str: return "" @classmethod - def set_email_frequency(cls, user: Any, frequency: Type[NotificationFrequency]) -> None: + def set_frequency(cls, user: Any, frequency: Type[BaseFrequency]) -> None: """ - Set the email frequency for this notification type for a user. + Set the delivery frequency for this notification type for a user. Args: user: The user to set the frequency for - frequency: NotificationFrequency class + frequency: BaseFrequency class """ - from .models import EmailFrequency + from .models import NotificationFrequency - EmailFrequency.objects.update_or_create( + NotificationFrequency.objects.update_or_create( user=user, notification_type=cls.key, defaults={"frequency": frequency.key} ) @classmethod - def get_email_frequency(cls, user: Any) -> Type[NotificationFrequency]: + def get_frequency(cls, user: Any) -> Type[BaseFrequency]: """ - Get the email frequency for this notification type for a user. + Get the delivery frequency for this notification type for a user. Args: user: The user to get the frequency for Returns: - NotificationFrequency class (either user preference or default) + BaseFrequency class (either user preference or default) """ - from .models import EmailFrequency + from .models import NotificationFrequency try: - user_frequency = EmailFrequency.objects.get(user=user, notification_type=cls.key) + user_frequency = NotificationFrequency.objects.get(user=user, notification_type=cls.key) return registry.get_frequency(user_frequency.frequency) - except EmailFrequency.DoesNotExist: - return cls.default_email_frequency + except NotificationFrequency.DoesNotExist: + return cls.default_frequency @classmethod - def reset_email_frequency_to_default(cls, user: Any) -> None: + def reset_frequency_to_default(cls, user: Any) -> None: """ - Reset the email frequency to default for this notification type for a user. + Reset the delivery frequency to default for this notification type for a user. Args: user: The user to reset the frequency for """ - from .models import EmailFrequency + from .models import NotificationFrequency - EmailFrequency.objects.filter(user=user, notification_type=cls.key).delete() + NotificationFrequency.objects.filter(user=user, notification_type=cls.key).delete() @classmethod - def get_enabled_channels(cls, user: Any) -> list[Type[NotificationChannel]]: + def get_enabled_channels(cls, user: Any) -> list[Type[BaseChannel]]: """ Get all enabled channels for this notification type for a user. This is more efficient than calling is_channel_enabled for each channel individually. @@ -106,7 +106,7 @@ def get_enabled_channels(cls, user: Any) -> list[Type[NotificationChannel]]: user: User instance Returns: - List of enabled NotificationChannel classes + List of enabled BaseChannel classes """ from .models import DisabledNotificationTypeChannel @@ -126,13 +126,13 @@ def get_enabled_channels(cls, user: Any) -> list[Type[NotificationChannel]]: return enabled_channels @classmethod - def is_channel_enabled(cls, user: Any, channel: Type[NotificationChannel]) -> bool: + def is_channel_enabled(cls, user: Any, channel: Type[BaseChannel]) -> bool: """ Check if a channel is enabled for this notification type for a user. Args: user: User instance - channel: NotificationChannel class + channel: BaseChannel class Returns: True if channel is enabled, False if disabled @@ -144,26 +144,26 @@ def is_channel_enabled(cls, user: Any, channel: Type[NotificationChannel]) -> bo ).exists() @classmethod - def disable_channel(cls, user: Any, channel: Type[NotificationChannel]) -> None: + def disable_channel(cls, user: Any, channel: Type[BaseChannel]) -> None: """ Disable a channel for this notification type for a user. Args: user: User instance - channel: NotificationChannel class + channel: BaseChannel class """ from .models import DisabledNotificationTypeChannel DisabledNotificationTypeChannel.objects.get_or_create(user=user, notification_type=cls.key, channel=channel.key) @classmethod - def enable_channel(cls, user: Any, channel: Type[NotificationChannel]) -> None: + def enable_channel(cls, user: Any, channel: Type[BaseChannel]) -> None: """ Enable a channel for this notification type for a user. Args: user: User instance - channel: NotificationChannel class + channel: BaseChannel class """ from .models import DisabledNotificationTypeChannel @@ -198,7 +198,7 @@ class SystemMessage(NotificationType): key = "system_message" name = "System Message" description = "Important system notifications" - default_email_frequency = RealtimeFrequency + default_frequency = RealtimeFrequency required_channels = [EmailChannel] def get_subject(self, notification: "Notification") -> str: diff --git a/generic_notifications/utils.py b/generic_notifications/utils.py index e56f37d..bc5c5c4 100644 --- a/generic_notifications/utils.py +++ b/generic_notifications/utils.py @@ -3,7 +3,7 @@ from django.db.models import QuerySet from django.utils import timezone -from .channels import NotificationChannel, WebsiteChannel +from .channels import BaseChannel, WebsiteChannel def mark_notifications_as_read(user: Any, notification_ids: list[int] | None = None) -> None: @@ -23,7 +23,7 @@ def mark_notifications_as_read(user: Any, notification_ids: list[int] | None = N queryset.update(read=timezone.now()) -def get_unread_count(user: Any, channel: type[NotificationChannel] = WebsiteChannel) -> int: +def get_unread_count(user: Any, channel: type[BaseChannel] = WebsiteChannel) -> int: """ Get count of unread notifications for a user, filtered by channel. @@ -34,11 +34,11 @@ def get_unread_count(user: Any, channel: type[NotificationChannel] = WebsiteChan Returns: Count of unread notifications for the specified channel """ - return user.notifications.filter(read__isnull=True, channels__icontains=f'"{channel.key}"').count() + return user.notifications.filter(read__isnull=True).for_channel(channel).count() def get_notifications( - user: Any, channel: type[NotificationChannel] = WebsiteChannel, unread_only: bool = False, limit: int | None = None + user: Any, channel: type[BaseChannel] = WebsiteChannel, unread_only: bool = False, limit: int | None = None ) -> QuerySet: """ Get notifications for a user, filtered by channel. diff --git a/tests/test_channels.py b/tests/test_channels.py index 723d8f5..97cddc8 100644 --- a/tests/test_channels.py +++ b/tests/test_channels.py @@ -5,12 +5,14 @@ from django.core import mail from django.test import TestCase, override_settings -from generic_notifications.channels import EmailChannel, NotificationChannel +from generic_notifications.channels import BaseChannel, EmailChannel from generic_notifications.frequencies import RealtimeFrequency -from generic_notifications.models import DisabledNotificationTypeChannel, EmailFrequency, Notification +from generic_notifications.models import DisabledNotificationTypeChannel, Notification, NotificationFrequency from generic_notifications.registry import registry from generic_notifications.types import NotificationType +from .test_helpers import create_notification_with_channels + User = get_user_model() @@ -19,7 +21,7 @@ class TestNotificationType(NotificationType): key = "test_type" name = "Test Type" description = "A test notification type" - default_email_frequency = RealtimeFrequency + default_frequency = RealtimeFrequency class NotificationChannelTest(TestCase): @@ -31,7 +33,7 @@ def setUpClass(cls): cls.user = User.objects.create_user(username="user1", email="test@example.com", password="testpass") def test_notification_channel_is_abstract(self): - class TestChannel(NotificationChannel): + class TestChannel(BaseChannel): key = "test" name = "Test" @@ -43,7 +45,7 @@ def process(self, notification): self.assertEqual(channel.name, "Test") def test_is_enabled_default_true(self): - class TestChannel(NotificationChannel): + class TestChannel(BaseChannel): key = "test" name = "Test" @@ -54,7 +56,7 @@ def process(self, notification): self.assertTrue(TestNotificationType.is_channel_enabled(self.user, TestChannel)) def test_is_enabled_with_disabled_notification(self): - class TestChannel(NotificationChannel): + class TestChannel(BaseChannel): key = "test" name = "Test" @@ -106,8 +108,8 @@ def tearDown(self): @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_process_realtime_frequency(self): - notification = Notification.objects.create( - recipient=self.user, notification_type="test_type", channels=["website", "email"] + notification = create_notification_with_channels( + user=self.user, channels=["website", "email"], notification_type="test_type" ) channel = EmailChannel() @@ -120,14 +122,14 @@ def test_process_realtime_frequency(self): # Check notification was marked as sent notification.refresh_from_db() - self.assertIsNotNone(notification.email_sent_at) + self.assertTrue(notification.is_sent_on_channel(EmailChannel)) def test_process_digest_frequency(self): # Set user preference to daily (non-realtime) - EmailFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") - notification = Notification.objects.create( - recipient=self.user, notification_type="test_type", channels=["website", "email"] + notification = create_notification_with_channels( + user=self.user, channels=["website", "email"], notification_type="test_type" ) channel = EmailChannel() @@ -138,12 +140,16 @@ def test_process_digest_frequency(self): # Notification should not be marked as sent notification.refresh_from_db() - self.assertIsNone(notification.email_sent_at) + self.assertFalse(notification.is_sent_on_channel(EmailChannel)) @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_send_now_basic(self): - notification = Notification.objects.create( - recipient=self.user, notification_type="test_type", subject="Test Subject", text="Test message" + notification = create_notification_with_channels( + user=self.user, + notification_type="test_type", + subject="Test Subject", + text="Test message", + channels=["email"], ) channel = EmailChannel() @@ -159,12 +165,14 @@ def test_send_now_basic(self): # Check notification was marked as sent notification.refresh_from_db() - self.assertIsNotNone(notification.email_sent_at) + self.assertTrue(notification.is_sent_on_channel(EmailChannel)) @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_send_now_uses_get_methods(self): # Create notification without stored subject/text to test dynamic generation - notification = Notification.objects.create(recipient=self.user, notification_type="test_type") + notification = create_notification_with_channels( + user=self.user, notification_type="test_type", channels=["email"] + ) channel = EmailChannel() channel.send_now(notification) @@ -192,8 +200,12 @@ def mock_render_side_effect(template_name, context): mock_render.side_effect = mock_render_side_effect - notification = Notification.objects.create( - recipient=self.user, notification_type="test_type", subject="Test Subject", text="Test message" + notification = create_notification_with_channels( + user=self.user, + notification_type="test_type", + subject="Test Subject", + text="Test message", + channels=["email"], ) channel = EmailChannel() @@ -228,8 +240,8 @@ def mock_render_side_effect(template_name, context): @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_send_now_template_error_fallback(self): - notification = Notification.objects.create( - recipient=self.user, notification_type="test_type", subject="Test Subject" + notification = create_notification_with_channels( + user=self.user, notification_type="test_type", subject="Test Subject", channels=["email"] ) channel = EmailChannel() @@ -253,14 +265,18 @@ def test_send_digest_emails_empty_queryset(self): @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_send_digest_emails_basic(self): # Set user to daily frequency to prevent realtime sending - EmailFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") # Create test notifications without email_sent_at (unsent) for i in range(3): - Notification.objects.create(recipient=self.user, notification_type="test_type", subject=f"Test {i}") + create_notification_with_channels( + user=self.user, notification_type="test_type", subject=f"Test {i}", channels=["email"] + ) # Get notifications as queryset - notifications = Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True) + notifications = Notification.objects.filter( + recipient=self.user, channels__channel="email", channels__sent_at__isnull=True + ) # Send digest email for this user EmailChannel().send_digest(notifications) @@ -274,16 +290,20 @@ def test_send_digest_emails_basic(self): # Check all notifications marked as sent for notification in notifications: notification.refresh_from_db() - self.assertIsNotNone(notification.email_sent_at) + self.assertTrue(notification.is_sent_on_channel(EmailChannel)) @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_send_digest_emails_with_frequency(self): # Set user to daily frequency to prevent realtime sending - EmailFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") - Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") + create_notification_with_channels( + user=self.user, notification_type="test_type", subject="Test", channels=["email"] + ) - EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) + 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") @@ -291,11 +311,15 @@ def test_send_digest_emails_with_frequency(self): @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 - EmailFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") - Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") + create_notification_with_channels( + user=self.user, notification_type="test_type", subject="Test", channels=["email"] + ) - EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) + 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") @@ -303,15 +327,17 @@ def test_send_digest_emails_without_frequency(self): @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 - EmailFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") # Create more than 10 notifications to test text limit - _ = [ - Notification.objects.create(recipient=self.user, notification_type="test_type", subject=f"Test {i}") - for i in range(15) - ] + for i in range(15): + create_notification_with_channels( + user=self.user, notification_type="test_type", subject=f"Test {i}", channels=["email"] + ) - EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) + EmailChannel().send_digest( + Notification.objects.filter(recipient=self.user, channels__channel="email", channels__sent_at__isnull=True) + ) # The implementation may not have this feature, so we'll just check that email was sent self.assertEqual(len(mail.outbox), 1) @@ -324,11 +350,15 @@ def test_send_digest_emails_with_html_template(self, mock_render): mock_render.return_value = "Digest HTML" # Set user to daily frequency to prevent realtime sending - EmailFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") - Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") + create_notification_with_channels( + user=self.user, notification_type="test_type", subject="Test", channels=["email"] + ) - EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) + 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) @@ -349,10 +379,10 @@ def test_send_digest_emails_with_html_template(self, mock_render): def test_send_now_fallback_includes_url(self): """Test that fallback email content includes URL when available""" - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, + notification = create_notification_with_channels( + user=self.user, channels=["email"], + notification_type=TestNotificationType.key, subject="Test Subject", text="Test notification text", url="https://example.com/test/url/123", @@ -371,22 +401,24 @@ def test_send_now_fallback_includes_url(self): def test_send_digest_emails_fallback_includes_urls(self): """Test that digest fallback email content includes URLs when available""" # Create notifications with URLs - Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, + create_notification_with_channels( + user=self.user, channels=["email"], + notification_type=TestNotificationType.key, text="First notification", url="https://example.com/url/1", ) - Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, + create_notification_with_channels( + user=self.user, channels=["email"], + notification_type=TestNotificationType.key, text="Second notification", url="https://example.com/url/2", ) - EmailChannel().send_digest(Notification.objects.filter(recipient=self.user, email_sent_at__isnull=True)) + EmailChannel().send_digest( + Notification.objects.filter(recipient=self.user, channels__channel="email", channels__sent_at__isnull=True) + ) # Check that one email was sent self.assertEqual(len(mail.outbox), 1) @@ -444,18 +476,18 @@ def send_email( # Don't call super() - we don't want to actually send emails # Create notifications - notification1 = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, + notification1 = create_notification_with_channels( + user=self.user, channels=["test_email"], + notification_type=TestNotificationType.key, subject="Test Subject 1", text="Test notification 1", ) - notification2 = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, + notification2 = create_notification_with_channels( + user=self.user, channels=["test_email"], + notification_type=TestNotificationType.key, subject="Test Subject 2", text="Test notification 2", ) @@ -482,8 +514,8 @@ def send_email( # Check that notifications were marked as sent notification1.refresh_from_db() notification2.refresh_from_db() - self.assertIsNotNone(notification1.email_sent_at) - self.assertIsNotNone(notification2.email_sent_at) + self.assertTrue(notification1.is_sent_on_channel(TestEmailChannel)) + self.assertTrue(notification2.is_sent_on_channel(TestEmailChannel)) def test_custom_email_channel_send_now_override(self): """Test that a custom EmailChannel subclass works with send_now.""" @@ -513,10 +545,10 @@ def send_email( ) # Create notification - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, + notification = create_notification_with_channels( + user=self.user, channels=["async_email"], + notification_type=TestNotificationType.key, subject="Realtime Test", text="Realtime notification", ) @@ -539,4 +571,4 @@ def send_email( # Check that notification was marked as sent notification.refresh_from_db() - self.assertIsNotNone(notification.email_sent_at) + self.assertTrue(notification.is_sent_on_channel(AsyncEmailChannel)) diff --git a/tests/test_helpers.py b/tests/test_helpers.py new file mode 100644 index 0000000..574898f --- /dev/null +++ b/tests/test_helpers.py @@ -0,0 +1,38 @@ +"""Shared test helpers for django-generic-notifications tests""" + +from django.contrib.auth import get_user_model + +from generic_notifications.channels import EmailChannel, WebsiteChannel +from generic_notifications.models import Notification, NotificationChannel + +User = get_user_model() + + +def create_notification_with_channels(user, channels=None, **kwargs): + """ + Helper to create notification with the new model structure. + + Args: + user: User instance to be the recipient + channels: List of channel keys. Defaults to [website, email] + **kwargs: Additional fields for the Notification model + + Returns: + Notification instance with associated channels + """ + if channels is None: + channels = [WebsiteChannel.key, EmailChannel.key] + + defaults = { + "recipient": user, + "notification_type": "test_type", # Default test type + } + defaults.update(kwargs) + + notification = Notification.objects.create(**defaults) + + # Create NotificationChannel entries + for channel in channels: + NotificationChannel.objects.create(notification=notification, channel=channel) + + return notification diff --git a/tests/test_management_commands.py b/tests/test_management_commands.py index 65133dd..c67bafb 100644 --- a/tests/test_management_commands.py +++ b/tests/test_management_commands.py @@ -6,15 +6,18 @@ from django.test import TestCase from django.utils import timezone -from generic_notifications.frequencies import DailyFrequency, NotificationFrequency, RealtimeFrequency -from generic_notifications.models import EmailFrequency, Notification +from generic_notifications.channels import EmailChannel +from generic_notifications.frequencies import BaseFrequency, DailyFrequency, RealtimeFrequency +from generic_notifications.models import Notification, NotificationFrequency from generic_notifications.registry import registry from generic_notifications.types import NotificationType +from .test_helpers import create_notification_with_channels + User = get_user_model() -class WeeklyFrequency(NotificationFrequency): +class WeeklyFrequency(BaseFrequency): key = "weekly" name = "Weekly" is_realtime = False @@ -25,14 +28,14 @@ class TestNotificationType(NotificationType): key = "test_type" name = "Test Type" description = "" - default_email_frequency = DailyFrequency # Defaults to daily like comments + default_frequency = DailyFrequency # Defaults to daily like comments class OtherNotificationType(NotificationType): key = "other_type" name = "Other Type" description = "" - default_email_frequency = RealtimeFrequency # Defaults to realtime like system messages + default_frequency = RealtimeFrequency # Defaults to realtime like system messages class SendDigestEmailsCommandTest(TestCase): @@ -76,11 +79,11 @@ def test_target_frequency_is_realtime(self): def test_send_digest_emails_basic_flow(self): # Set up user with daily frequency preference - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") # Create a notification - notification = Notification.objects.create( - recipient=self.user1, + notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Test notification", text="This is a test notification", @@ -103,13 +106,14 @@ def test_send_digest_emails_basic_flow(self): # Verify notification was marked as sent notification.refresh_from_db() - self.assertIsNotNone(notification.email_sent_at) + + self.assertTrue(notification.is_sent_on_channel(EmailChannel)) def test_dry_run_does_not_send_emails(self): - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") - notification = Notification.objects.create( - recipient=self.user1, notification_type="test_type", subject="Test notification", channels=["email"] + notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Test notification", channels=["email"] ) # Ensure no emails in outbox initially @@ -122,14 +126,15 @@ def test_dry_run_does_not_send_emails(self): # Notification should not be marked as sent notification.refresh_from_db() - self.assertIsNone(notification.email_sent_at) + + self.assertFalse(notification.is_sent_on_channel(EmailChannel)) def test_only_includes_unread_notifications(self): - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") # Create read and unread notifications - read_notification = Notification.objects.create( - recipient=self.user1, + read_notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Read notification subject", text="Read notification text", @@ -137,8 +142,8 @@ def test_only_includes_unread_notifications(self): ) read_notification.mark_as_read() - unread_notification = Notification.objects.create( - recipient=self.user1, + unread_notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Unread notification subject", text="Unread notification text", @@ -156,22 +161,24 @@ def test_only_includes_unread_notifications(self): read_notification.refresh_from_db() unread_notification.refresh_from_db() - self.assertIsNone(read_notification.email_sent_at) # Still not sent - self.assertIsNotNone(unread_notification.email_sent_at) # Now sent + self.assertFalse(read_notification.is_sent_on_channel(EmailChannel)) # Still not sent + self.assertTrue(unread_notification.is_sent_on_channel(EmailChannel)) # Now sent def test_only_includes_unsent_notifications(self): - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") # Create sent and unsent notifications - Notification.objects.create( - recipient=self.user1, + sent_notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Sent notification", - email_sent_at=timezone.now(), + channels=["email"], ) + # Mark as sent + sent_notification.mark_sent_on_channel(EmailChannel) - unsent_notification = Notification.objects.create( - recipient=self.user1, + unsent_notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Unsent notification subject", text="Unsent notification text", @@ -187,14 +194,15 @@ def test_only_includes_unsent_notifications(self): # Unsent notification should now be marked as sent unsent_notification.refresh_from_db() - self.assertIsNotNone(unsent_notification.email_sent_at) + + self.assertTrue(unsent_notification.is_sent_on_channel(EmailChannel)) def test_sends_all_unsent_notifications(self): - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") # Create notification older than time window (>1 day ago) - old_notification = Notification.objects.create( - recipient=self.user1, + old_notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Old notification subject", text="Old notification text", @@ -205,8 +213,8 @@ def test_sends_all_unsent_notifications(self): Notification.objects.filter(id=old_notification.id).update(added=old_time) # Create recent notification - recent_notification = Notification.objects.create( - recipient=self.user1, + recent_notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Recent notification subject", text="Recent notification text", @@ -227,24 +235,24 @@ def test_sends_all_unsent_notifications(self): old_notification.refresh_from_db() recent_notification.refresh_from_db() - self.assertIsNotNone(old_notification.email_sent_at) # Old but unsent, so included - self.assertIsNotNone(recent_notification.email_sent_at) # Recent, sent + self.assertTrue(old_notification.is_sent_on_channel(EmailChannel)) # Old but unsent, so included + self.assertTrue(recent_notification.is_sent_on_channel(EmailChannel)) # Recent, sent def test_specific_frequency_filter(self): # Set up users with different frequency preferences - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") - EmailFrequency.objects.create(user=self.user2, notification_type="test_type", frequency="weekly") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user2, notification_type="test_type", frequency="weekly") # Create notifications for both - Notification.objects.create( - recipient=self.user1, + create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Daily user notification subject", text="Daily user notification text", channels=["email"], ) - Notification.objects.create( - recipient=self.user2, + create_notification_with_channels( + user=self.user2, notification_type="test_type", subject="Weekly user notification subject", text="Weekly user notification text", @@ -271,19 +279,19 @@ def test_specific_frequency_filter(self): def test_multiple_notification_types_for_user(self): # Set up user with multiple notification types for daily frequency - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") - EmailFrequency.objects.create(user=self.user1, notification_type="other_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="other_type", frequency="daily") # Create notifications of both types - notification1 = Notification.objects.create( - recipient=self.user1, + notification1 = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Test type notification subject", text="Test type notification text", channels=["email"], ) - notification2 = Notification.objects.create( - recipient=self.user1, + notification2 = create_notification_with_channels( + user=self.user1, notification_type="other_type", subject="Other type notification subject", text="Other type notification text", @@ -304,11 +312,12 @@ def test_multiple_notification_types_for_user(self): # Both notifications should be marked as sent notification1.refresh_from_db() notification2.refresh_from_db() - self.assertIsNotNone(notification1.email_sent_at) - self.assertIsNotNone(notification2.email_sent_at) + + self.assertTrue(notification1.is_sent_on_channel(EmailChannel)) + self.assertTrue(notification2.is_sent_on_channel(EmailChannel)) def test_no_notifications_to_send(self): - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") # No notifications created @@ -321,8 +330,8 @@ def test_users_with_disabled_email_channel_dont_get_digest(self): """Test that users who disabled email channel for a type don't get digest emails.""" # With the new architecture, if email is disabled, notifications won't have email channel # So create a notification without email channel to simulate this - Notification.objects.create( - recipient=self.user1, notification_type="test_type", subject="Test notification", channels=["website"] + create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Test notification", channels=["website"] ) # Run daily digest - should not send anything (no email channel) @@ -331,11 +340,11 @@ def test_users_with_disabled_email_channel_dont_get_digest(self): def test_users_with_default_frequencies_get_digest(self): """Test that users without explicit preferences get digest emails based on default frequencies.""" - # Don't create any EmailFrequency preferences - user will use defaults + # Don't create any NotificationFrequency preferences - user will use defaults # Create a test_type notification (defaults to daily) - test_notification = Notification.objects.create( - recipient=self.user1, + test_notification = create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Test notification", text="This is a test notification", @@ -343,8 +352,8 @@ def test_users_with_default_frequencies_get_digest(self): ) # Create an other_type notification (defaults to realtime) - other_notification = Notification.objects.create( - recipient=self.user1, + other_notification = create_notification_with_channels( + user=self.user1, notification_type="other_type", subject="Other notification", text="This is another type of notification", @@ -362,25 +371,26 @@ def test_users_with_default_frequencies_get_digest(self): # Verify only test notification was marked as sent test_notification.refresh_from_db() other_notification.refresh_from_db() - self.assertIsNotNone(test_notification.email_sent_at) - self.assertIsNone(other_notification.email_sent_at) + + self.assertTrue(test_notification.is_sent_on_channel(EmailChannel)) + self.assertFalse(other_notification.is_sent_on_channel(EmailChannel)) def test_mixed_explicit_and_default_preferences(self): """Test that users with some explicit preferences and some defaults work correctly.""" # User explicitly sets test_type to weekly - EmailFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="weekly") + NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="weekly") # other_type will use its default (realtime) # Create notifications - Notification.objects.create( - recipient=self.user1, + create_notification_with_channels( + user=self.user1, notification_type="test_type", subject="Test notification subject", text="Test notification text", channels=["email"], ) - Notification.objects.create( - recipient=self.user1, + create_notification_with_channels( + user=self.user1, notification_type="other_type", subject="Other notification subject", text="Other notification text", @@ -403,13 +413,13 @@ def test_multiple_users_default_and_explicit_mix(self): # user2: Explicit preference (test_type=weekly, other_type uses default=realtime) # user3: Mixed (test_type=daily explicit, other_type uses default=realtime) - EmailFrequency.objects.create(user=self.user2, notification_type="test_type", frequency="weekly") - EmailFrequency.objects.create(user=self.user3, notification_type="test_type", frequency="daily") + NotificationFrequency.objects.create(user=self.user2, notification_type="test_type", frequency="weekly") + NotificationFrequency.objects.create(user=self.user3, notification_type="test_type", frequency="daily") # Create test notifications for all users for i, user in enumerate([self.user1, self.user2, self.user3], 1): - Notification.objects.create( - recipient=user, + create_notification_with_channels( + user=user, notification_type="test_type", subject=f"Test notification for user {i} subject", text=f"Test notification for user {i} text", diff --git a/tests/test_models.py b/tests/test_models.py index 06cff13..e93849e 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -9,10 +9,12 @@ from generic_notifications.channels import EmailChannel, WebsiteChannel from generic_notifications.frequencies import DailyFrequency -from generic_notifications.models import DisabledNotificationTypeChannel, EmailFrequency, Notification +from generic_notifications.models import DisabledNotificationTypeChannel, Notification, NotificationFrequency from generic_notifications.registry import registry from generic_notifications.types import NotificationType, SystemMessage +from .test_helpers import create_notification_with_channels + User = get_user_model() @@ -105,7 +107,7 @@ def test_clean_allows_disabling_non_required_channel(self): disabled.clean() -class EmailFrequencyModelTest(TestCase): +class NotificationFrequencyModelTest(TestCase): user: Any # User model instance created in setUpClass @classmethod @@ -119,7 +121,7 @@ def setUpClass(cls): registry.register_frequency(DailyFrequency, force=True) def test_create_email_frequency(self): - frequency = EmailFrequency.objects.create( + frequency = NotificationFrequency.objects.create( user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key ) @@ -128,17 +130,19 @@ def test_create_email_frequency(self): self.assertEqual(frequency.frequency, DailyFrequency.key) def test_unique_together_constraint(self): - EmailFrequency.objects.create( + NotificationFrequency.objects.create( user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key ) with self.assertRaises(IntegrityError): - EmailFrequency.objects.create( + NotificationFrequency.objects.create( user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key ) def test_clean_with_invalid_notification_type(self): - frequency = EmailFrequency(user=self.user, notification_type="invalid_type", frequency=DailyFrequency.key) + frequency = NotificationFrequency( + user=self.user, notification_type="invalid_type", frequency=DailyFrequency.key + ) with self.assertRaises(ValidationError) as cm: frequency.clean() @@ -146,7 +150,7 @@ def test_clean_with_invalid_notification_type(self): self.assertIn("Unknown notification type: invalid_type", str(cm.exception)) def test_clean_with_invalid_frequency(self): - frequency = EmailFrequency( + frequency = NotificationFrequency( user=self.user, notification_type=TestNotificationType.key, frequency="invalid_frequency" ) @@ -156,7 +160,7 @@ def test_clean_with_invalid_frequency(self): self.assertIn("Unknown frequency: invalid_frequency", str(cm.exception)) def test_clean_with_valid_data(self): - frequency = EmailFrequency( + frequency = NotificationFrequency( user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key ) @@ -178,11 +182,7 @@ def setUpClass(cls): registry.register_type(TestNotificationType) def test_create_minimal_notification(self): - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key, EmailChannel.key], - ) + notification = create_notification_with_channels(user=self.user, notification_type=TestNotificationType.key) self.assertEqual(notification.recipient, self.user) self.assertEqual(notification.notification_type, TestNotificationType.key) @@ -191,8 +191,8 @@ def test_create_minimal_notification(self): self.assertEqual(notification.metadata, {}) def test_create_full_notification(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, subject="Test Subject", text="Test notification text", @@ -214,8 +214,8 @@ def test_notification_with_generic_relation(self): target_user = User.objects.create_user(username="target", email="target@example.com", password="testpass") content_type = ContentType.objects.get_for_model(User) - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, content_type=content_type, object_id=target_user.id, @@ -238,11 +238,7 @@ def test_clean_with_valid_notification_type(self): notification.clean() def test_mark_as_read(self): - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key, EmailChannel.key], - ) + notification = create_notification_with_channels(user=self.user, notification_type=TestNotificationType.key) self.assertFalse(notification.is_read) self.assertIsNone(notification.read) @@ -254,11 +250,7 @@ def test_mark_as_read(self): self.assertIsNotNone(notification.read) def test_mark_as_read_idempotent(self): - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key, EmailChannel.key], - ) + notification = create_notification_with_channels(user=self.user, notification_type=TestNotificationType.key) # Mark as read first time notification.mark_as_read() @@ -273,11 +265,7 @@ def test_mark_as_read_idempotent(self): self.assertEqual(notification.read, first_read_time) def test_is_read_property(self): - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key, EmailChannel.key], - ) + notification = create_notification_with_channels(user=self.user, notification_type=TestNotificationType.key) self.assertFalse(notification.is_read) @@ -285,33 +273,29 @@ def test_is_read_property(self): self.assertTrue(notification.is_read) def test_email_sent_tracking(self): - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key, EmailChannel.key], - ) + notification = create_notification_with_channels(user=self.user, notification_type=TestNotificationType.key) - self.assertIsNone(notification.email_sent_at) + # Test that email is not sent initially + self.assertFalse(notification.is_sent_on_channel(EmailChannel)) # Simulate email being sent - sent_time = timezone.now() - notification.email_sent_at = sent_time - notification.save() + notification.mark_sent_on_channel(EmailChannel) - notification.refresh_from_db() - self.assertEqual(notification.email_sent_at, sent_time) + # Test that email is now marked as sent + self.assertTrue(notification.is_sent_on_channel(EmailChannel)) + + # Website should still be unsent + self.assertFalse(notification.is_sent_on_channel(WebsiteChannel)) def test_get_absolute_url_empty_url(self): - notification = Notification.objects.create( - recipient=self.user, - notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key] ) self.assertEqual(notification.get_absolute_url(), "") def test_get_absolute_url_already_absolute(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key], url="https://example.com/path", @@ -319,8 +303,8 @@ def test_get_absolute_url_already_absolute(self): self.assertEqual(notification.get_absolute_url(), "https://example.com/path") def test_get_absolute_url_with_setting(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key], url="/notifications/123", @@ -330,8 +314,8 @@ def test_get_absolute_url_with_setting(self): self.assertEqual(notification.get_absolute_url(), "https://mysite.com/notifications/123") def test_get_absolute_url_with_setting_no_protocol_debug(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key], url="/notifications/123", @@ -342,8 +326,8 @@ def test_get_absolute_url_with_setting_no_protocol_debug(self): self.assertEqual(notification.get_absolute_url(), "http://mysite.com/notifications/123") def test_get_absolute_url_with_setting_no_protocol_production(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key], url="/notifications/123", @@ -354,8 +338,8 @@ def test_get_absolute_url_with_setting_no_protocol_production(self): self.assertEqual(notification.get_absolute_url(), "https://mysite.com/notifications/123") def test_get_absolute_url_fallback_settings(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key], url="/notifications/123", @@ -365,8 +349,8 @@ def test_get_absolute_url_fallback_settings(self): self.assertEqual(notification.get_absolute_url(), "https://fallback.com/notifications/123") def test_get_absolute_url_fallback_settings_no_protocol(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key], url="/notifications/123", @@ -377,8 +361,8 @@ def test_get_absolute_url_fallback_settings_no_protocol(self): self.assertEqual(notification.get_absolute_url(), "https://fallback.com/notifications/123") def test_get_absolute_url_no_base_url(self): - notification = Notification.objects.create( - recipient=self.user, + notification = create_notification_with_channels( + user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key], url="/notifications/123", diff --git a/tests/test_performance.py b/tests/test_performance.py index 99a8117..fb5bcda 100644 --- a/tests/test_performance.py +++ b/tests/test_performance.py @@ -2,9 +2,10 @@ from django.test import TestCase from generic_notifications.channels import WebsiteChannel -from generic_notifications.models import Notification from generic_notifications.utils import get_notifications +from .test_helpers import create_notification_with_channels + User = get_user_model() @@ -14,8 +15,8 @@ def setUp(self): self.actor = User.objects.create_user(username="actor", email="actor@example.com", password="testpass") for i in range(5): - Notification.objects.create( - recipient=self.user, + create_notification_with_channels( + user=self.user, actor=self.actor, notification_type="test_notification", subject=f"Test notification {i}", @@ -62,8 +63,8 @@ def test_notification_target_access_queries(self): """Test queries when accessing notification.target in template""" # Create notifications with targets for i in range(5): - Notification.objects.create( - recipient=self.user, + create_notification_with_channels( + user=self.user, actor=self.actor, notification_type="test_notification", subject=f"Test notification {i}", @@ -88,8 +89,8 @@ def test_notification_target_relationship_access(self): # Create notifications where each has a different notification as its target for i in range(5): # Create the target notification - target_notification = Notification.objects.create( - recipient=self.actor, + target_notification = create_notification_with_channels( + user=self.actor, notification_type="target_notification", subject=f"Target notification {i}", text=f"Target text {i}", @@ -97,8 +98,8 @@ def test_notification_target_relationship_access(self): ) # Create notification pointing to it - Notification.objects.create( - recipient=self.user, + create_notification_with_channels( + user=self.user, actor=self.actor, notification_type="test_notification", subject=f"Test notification {i}", @@ -124,8 +125,8 @@ def test_notification_target_relationship_preselect_access(self): # Create notifications where each has a different notification as its target for i in range(5): # Create the target notification - target_notification = Notification.objects.create( - recipient=self.actor, + target_notification = create_notification_with_channels( + user=self.actor, notification_type="target_notification", subject=f"Target notification {i}", text=f"Target text {i}", @@ -133,8 +134,8 @@ def test_notification_target_relationship_preselect_access(self): ) # Create notification pointing to it - Notification.objects.create( - recipient=self.user, + create_notification_with_channels( + user=self.user, actor=self.actor, notification_type="test_notification", subject=f"Test notification {i}", diff --git a/tests/test_preferences.py b/tests/test_preferences.py index ec1a113..bb0997c 100644 --- a/tests/test_preferences.py +++ b/tests/test_preferences.py @@ -5,7 +5,7 @@ from generic_notifications.channels import WebsiteChannel from generic_notifications.frequencies import DailyFrequency, RealtimeFrequency -from generic_notifications.models import DisabledNotificationTypeChannel, EmailFrequency +from generic_notifications.models import DisabledNotificationTypeChannel, NotificationFrequency from generic_notifications.preferences import get_notification_preferences, save_notification_preferences from generic_notifications.registry import registry from generic_notifications.types import NotificationType @@ -17,7 +17,7 @@ class TestNotificationType(NotificationType): key = "test_notification" name = "Test Notification" description = "A test notification type" - default_email_frequency = RealtimeFrequency + default_frequency = RealtimeFrequency required_channels = [] @@ -25,7 +25,7 @@ class RequiredChannelNotificationType(NotificationType): key = "required_channel_notification" name = "Required Channel Notification" description = "A notification with required channels" - default_email_frequency = DailyFrequency + default_frequency = DailyFrequency required_channels = [WebsiteChannel] @@ -78,7 +78,7 @@ def test_email_frequency_defaults_and_overrides(self): self.assertEqual(pref["email_frequency"], "daily") # Now override one - EmailFrequency.objects.create(user=self.user, notification_type="test_notification", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_notification", frequency="daily") preferences = get_notification_preferences(self.user) @@ -121,7 +121,7 @@ def test_complete_form_save_workflow(self): self.assertEqual(disabled.first().channel, "email") # Verify frequencies (only non-defaults saved) - frequencies = EmailFrequency.objects.filter(user=self.user).order_by("notification_type") + frequencies = NotificationFrequency.objects.filter(user=self.user).order_by("notification_type") self.assertEqual(frequencies.count(), 2) test_freq = frequencies.filter(notification_type="test_notification").first() @@ -136,7 +136,7 @@ def test_preferences_cleared_before_saving_new_ones(self): DisabledNotificationTypeChannel.objects.create( user=self.user, notification_type="test_notification", channel="website" ) - EmailFrequency.objects.create(user=self.user, notification_type="test_notification", frequency="daily") + NotificationFrequency.objects.create(user=self.user, notification_type="test_notification", frequency="daily") # Save completely different preferences form_data = { @@ -152,7 +152,7 @@ def test_preferences_cleared_before_saving_new_ones(self): self.assertEqual(disabled.count(), 0) # Old frequency should be gone - frequencies = EmailFrequency.objects.filter(user=self.user) + frequencies = NotificationFrequency.objects.filter(user=self.user) self.assertEqual(frequencies.count(), 0) def test_required_channels_ignored_in_form_data(self): @@ -206,7 +206,7 @@ def test_only_non_default_frequencies_are_saved(self): save_notification_preferences(self.user, form_data) # Only the non-default frequency should be saved - frequencies = EmailFrequency.objects.filter(user=self.user) + frequencies = NotificationFrequency.objects.filter(user=self.user) self.assertEqual(frequencies.count(), 1) self.assertEqual(frequencies.first().notification_type, "required_channel_notification") self.assertEqual(frequencies.first().frequency, "realtime") diff --git a/tests/test_registry.py b/tests/test_registry.py index 328dd5e..91702de 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -1,7 +1,7 @@ from django.test import TestCase -from generic_notifications.channels import NotificationChannel -from generic_notifications.frequencies import NotificationFrequency +from generic_notifications.channels import BaseChannel +from generic_notifications.frequencies import BaseFrequency from generic_notifications.registry import NotificationRegistry from generic_notifications.types import NotificationType @@ -38,28 +38,28 @@ def test_notification_type_str(self): self.assertEqual(str(notification_type), "Test Name") -class TestRealtimeFrequency(NotificationFrequency): +class TestRealtimeFrequency(BaseFrequency): key = "realtime" name = "Realtime" is_realtime = True description = "" -class TestDailyFrequency(NotificationFrequency): +class TestDailyFrequency(BaseFrequency): key = "daily" name = "Daily" is_realtime = False description = "" -class TestWeeklyFrequency(NotificationFrequency): +class TestWeeklyFrequency(BaseFrequency): key = "weekly" name = "Weekly" is_realtime = False description = "Once per week" -class TestDefaultFrequency(NotificationFrequency): +class TestDefaultFrequency(BaseFrequency): key = "test" name = "Test" is_realtime = False @@ -125,7 +125,7 @@ def test_register_invalid_notification_type(self): self.assertIn("Must register a NotificationType subclass", str(cm.exception)) def test_register_channel(self): - class TestChannel(NotificationChannel): + class TestChannel(BaseChannel): key = "test" name = "Test" @@ -144,7 +144,7 @@ def test_register_invalid_channel(self): with self.assertRaises(ValueError) as cm: self.registry.register_channel("not_a_channel_object") # type: ignore[arg-type] - self.assertIn("Must register a NotificationChannel subclass", str(cm.exception)) + self.assertIn("Must register a BaseChannel subclass", str(cm.exception)) def test_register_frequency(self): self.registry.register_frequency(TestDailyFrequency) @@ -159,7 +159,7 @@ def test_register_invalid_frequency(self): with self.assertRaises(ValueError) as cm: self.registry.register_frequency("not_a_frequency_object") # type: ignore[arg-type] - self.assertIn("Must register a NotificationFrequency subclass", str(cm.exception)) + self.assertIn("Must register a BaseFrequency subclass", str(cm.exception)) def test_get_nonexistent_items(self): with self.assertRaises(KeyError): @@ -210,7 +210,7 @@ class NonexistentType(NotificationType): self.assertFalse(result) def test_unregister_channel(self): - class TestChannel(NotificationChannel): + class TestChannel(BaseChannel): key = "test" name = "Test" @@ -231,7 +231,7 @@ def process(self, notification): self.assertEqual(len(self.registry.get_all_channels()), 0) def test_unregister_channel_nonexistent(self): - class NonexistentChannel(NotificationChannel): + class NonexistentChannel(BaseChannel): key = "nonexistent" name = "Nonexistent Channel" @@ -256,7 +256,7 @@ def test_unregister_frequency(self): self.assertEqual(len(self.registry.get_all_frequencies()), 0) def test_unregister_frequency_nonexistent(self): - class NonexistentFrequency(NotificationFrequency): + class NonexistentFrequency(BaseFrequency): key = "nonexistent" name = "Nonexistent Frequency" is_realtime = False @@ -292,14 +292,14 @@ class Type2(NotificationType): self.registry.get_type("type2") def test_clear_channels(self): - class Channel1(NotificationChannel): + class Channel1(BaseChannel): key = "channel1" name = "Channel 1" def process(self, notification): pass - class Channel2(NotificationChannel): + class Channel2(BaseChannel): key = "channel2" name = "Channel 2" @@ -322,13 +322,13 @@ def process(self, notification): self.registry.get_channel("channel2") def test_clear_frequencies(self): - class Freq1(NotificationFrequency): + class Freq1(BaseFrequency): key = "freq1" name = "Frequency 1" is_realtime = False description = "" - class Freq2(NotificationFrequency): + class Freq2(BaseFrequency): key = "freq2" name = "Frequency 2" is_realtime = False diff --git a/tests/test_types.py b/tests/test_types.py index 7e091e8..2e1dd00 100644 --- a/tests/test_types.py +++ b/tests/test_types.py @@ -5,7 +5,7 @@ from generic_notifications.channels import EmailChannel, WebsiteChannel from generic_notifications.frequencies import DailyFrequency, RealtimeFrequency -from generic_notifications.models import DisabledNotificationTypeChannel, EmailFrequency +from generic_notifications.models import DisabledNotificationTypeChannel, NotificationFrequency from generic_notifications.registry import registry from generic_notifications.types import NotificationType @@ -141,15 +141,15 @@ def test_get_enabled_channels(self): def test_set_frequency(self): # Set frequency for the first time - TestNotificationType.set_email_frequency(self.user, DailyFrequency) + TestNotificationType.set_frequency(self.user, DailyFrequency) # Verify it was created - freq = EmailFrequency.objects.get(user=self.user, notification_type=TestNotificationType.key) + freq = NotificationFrequency.objects.get(user=self.user, notification_type=TestNotificationType.key) self.assertEqual(freq.frequency, DailyFrequency.key) # Update to a different frequency registry.register_frequency(RealtimeFrequency, force=True) - TestNotificationType.set_email_frequency(self.user, RealtimeFrequency) + TestNotificationType.set_frequency(self.user, RealtimeFrequency) # Verify it was updated freq.refresh_from_db() @@ -157,23 +157,23 @@ def test_set_frequency(self): # Verify there's still only one record self.assertEqual( - EmailFrequency.objects.filter(user=self.user, notification_type=TestNotificationType.key).count(), 1 + NotificationFrequency.objects.filter(user=self.user, notification_type=TestNotificationType.key).count(), 1 ) def test_get_frequency_with_user_preference(self): # Set user preference - EmailFrequency.objects.create( + NotificationFrequency.objects.create( user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key ) # Get frequency should return the user's preference - frequency_cls = TestNotificationType.get_email_frequency(self.user) + frequency_cls = TestNotificationType.get_frequency(self.user) self.assertEqual(frequency_cls.key, DailyFrequency.key) self.assertEqual(frequency_cls, DailyFrequency) def test_get_frequency_returns_default_when_no_preference(self): - # TestNotificationType has default_email_frequency = DailyFrequency - frequency_cls = TestNotificationType.get_email_frequency(self.user) + # TestNotificationType has default_frequency = DailyFrequency + frequency_cls = TestNotificationType.get_frequency(self.user) self.assertEqual(frequency_cls.key, DailyFrequency.key) self.assertEqual(frequency_cls, DailyFrequency) @@ -184,32 +184,32 @@ def test_get_frequency_with_custom_default(self): class RealtimeNotificationType(NotificationType): key = "realtime_type" name = "Realtime Type" - default_email_frequency = RealtimeFrequency + default_frequency = RealtimeFrequency registry.register_type(RealtimeNotificationType) # Should return the custom default - frequency_cls = RealtimeNotificationType.get_email_frequency(self.user) + frequency_cls = RealtimeNotificationType.get_frequency(self.user) self.assertEqual(frequency_cls.key, RealtimeFrequency.key) self.assertEqual(frequency_cls, RealtimeFrequency) def test_reset_to_default(self): # First set a custom preference - EmailFrequency.objects.create( + NotificationFrequency.objects.create( user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key ) self.assertTrue( - EmailFrequency.objects.filter(user=self.user, notification_type=TestNotificationType.key).exists() + NotificationFrequency.objects.filter(user=self.user, notification_type=TestNotificationType.key).exists() ) # Reset to default - TestNotificationType.reset_email_frequency_to_default(self.user) + TestNotificationType.reset_frequency_to_default(self.user) # Verify the custom preference was removed self.assertFalse( - EmailFrequency.objects.filter(user=self.user, notification_type=TestNotificationType.key).exists() + NotificationFrequency.objects.filter(user=self.user, notification_type=TestNotificationType.key).exists() ) # Getting frequency should now return the default - frequency_cls = TestNotificationType.get_email_frequency(self.user) - self.assertEqual(frequency_cls, TestNotificationType.default_email_frequency) + frequency_cls = TestNotificationType.get_frequency(self.user) + self.assertEqual(frequency_cls, TestNotificationType.default_frequency) diff --git a/tests/test_utils.py b/tests/test_utils.py index d0609f7..36b4e8b 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -10,6 +10,8 @@ from generic_notifications.types import NotificationType from generic_notifications.utils import get_notifications, get_unread_count, mark_notifications_as_read +from .test_helpers import create_notification_with_channels + User = get_user_model() @@ -106,17 +108,19 @@ def test_send_notification_processes_channels(self): # Verify notification was created with channels self.assertIsNotNone(notification) - self.assertIn("website", notification.channels) - self.assertIn("email", notification.channels) + channel_keys = notification.get_channels() + self.assertIn("website", channel_keys) + self.assertIn("email", channel_keys) self.assertEqual(notification.notification_type, "test_type") def test_send_notification_multiple_channels(self): notification = send_notification(recipient=self.user, notification_type=self.notification_type) # Notification should have both channels enabled - self.assertIn("website", notification.channels) - self.assertIn("email", notification.channels) - self.assertEqual(len(notification.channels), 2) + channel_keys = notification.get_channels() + self.assertIn("website", channel_keys) + self.assertIn("email", channel_keys) + self.assertEqual(len(channel_keys), 2) class MarkNotificationsAsReadTest(TestCase): @@ -181,8 +185,8 @@ def test_mark_already_read_notifications(self): def test_mark_notifications_other_user_not_affected(self): other_user = User.objects.create_user(username="other", email="other@example.com", password="testpass") - other_notification = Notification.objects.create( - recipient=other_user, notification_type="test_type", channels=["website", "email"] + other_notification = create_notification_with_channels( + user=other_user, notification_type="test_type", channels=["website", "email"] ) mark_notifications_as_read(self.user) @@ -203,21 +207,21 @@ def test_get_unread_count_empty(self): def test_get_unread_count_with_unread_notifications(self): # Create unread notifications - Notification.objects.create(recipient=self.user, notification_type="test_type", channels=["website"]) - Notification.objects.create(recipient=self.user, notification_type="test_type", channels=["website"]) - Notification.objects.create(recipient=self.user, notification_type="test_type", channels=["website"]) + create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) + create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) + create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) count = get_unread_count(self.user, channel=WebsiteChannel) self.assertEqual(count, 3) def test_get_unread_count_with_mix_of_read_unread(self): # Create mix of read and unread - notification1 = Notification.objects.create( - recipient=self.user, notification_type="test_type", channels=["website"] + notification1 = create_notification_with_channels( + user=self.user, notification_type="test_type", channels=["website"] ) - Notification.objects.create(recipient=self.user, notification_type="test_type", channels=["website"]) - notification3 = Notification.objects.create( - recipient=self.user, notification_type="test_type", channels=["website"] + create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) + notification3 = create_notification_with_channels( + user=self.user, notification_type="test_type", channels=["website"] ) # Mark some as read @@ -231,8 +235,8 @@ def test_get_unread_count_other_user_not_counted(self): other_user = User.objects.create_user(username="other", email="other@example.com", password="testpass") # Create notifications for both users - Notification.objects.create(recipient=self.user, notification_type="test_type", channels=["website"]) - Notification.objects.create(recipient=other_user, notification_type="test_type", channels=["website", "email"]) + create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) + create_notification_with_channels(user=other_user, notification_type="test_type", channels=["website", "email"]) count = get_unread_count(self.user, channel=WebsiteChannel) self.assertEqual(count, 1) @@ -245,14 +249,14 @@ def setUp(self): registry.register_type(OtherNotificationType) # Create test notifications - self.notification1 = Notification.objects.create( - recipient=self.user, notification_type="test_type", subject="Test 1", channels=["website", "email"] + self.notification1 = create_notification_with_channels( + user=self.user, notification_type="test_type", subject="Test 1", channels=["website", "email"] ) - self.notification2 = Notification.objects.create( - recipient=self.user, notification_type="other_type", subject="Other 1", channels=["website", "email"] + self.notification2 = create_notification_with_channels( + user=self.user, notification_type="other_type", subject="Other 1", channels=["website", "email"] ) - self.notification3 = Notification.objects.create( - recipient=self.user, notification_type="test_type", subject="Test 2", channels=["website", "email"] + self.notification3 = create_notification_with_channels( + user=self.user, notification_type="test_type", subject="Test 2", channels=["website", "email"] ) # Mark one as read @@ -283,7 +287,7 @@ def test_get_with_limit(self): def test_get_notifications_other_user_not_included(self): other_user = User.objects.create_user(username="other", email="other@example.com", password="testpass") - Notification.objects.create(recipient=other_user, notification_type="test_type", channels=["website", "email"]) + create_notification_with_channels(user=other_user, notification_type="test_type", channels=["website", "email"]) notifications = get_notifications(self.user, channel=WebsiteChannel) From 086faa062465fe7e74eef9787e0c31490c8a96f6 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 18:52:27 +0200 Subject: [PATCH 04/15] The process function shouldn't live in EmailChannel --- generic_notifications/__init__.py | 4 +--- generic_notifications/channels.py | 35 +++++++++++++------------------ 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/generic_notifications/__init__.py b/generic_notifications/__init__.py index f182d13..75cba26 100644 --- a/generic_notifications/__init__.py +++ b/generic_notifications/__init__.py @@ -38,7 +38,7 @@ def send_notification( Raises: ValueError: If notification_type is not registered """ - from .models import Notification + from .models import Notification, NotificationChannel from .registry import registry # Validate notification type is registered @@ -81,8 +81,6 @@ def send_notification( notification.save() # Create NotificationChannel entries for each enabled channel - from .models import NotificationChannel - for channel_cls in enabled_channel_classes: NotificationChannel.objects.create( notification=notification, diff --git a/generic_notifications/channels.py b/generic_notifications/channels.py index d128877..283a6ce 100644 --- a/generic_notifications/channels.py +++ b/generic_notifications/channels.py @@ -1,5 +1,5 @@ import logging -from abc import ABC, abstractmethod +from abc import ABC from typing import TYPE_CHECKING, Type from django.conf import settings @@ -24,15 +24,26 @@ class BaseChannel(ABC): name: str supports_digest: bool = False - @abstractmethod def process(self, notification: "Notification") -> None: """ Process a notification through this channel. + For digest-supporting channels, this checks frequency preference. + For non-digest channels, this sends immediately. Args: notification: Notification instance to process """ - pass + if self.supports_digest: + # Get notification type class from key + notification_type_cls = registry.get_type(notification.notification_type) + frequency_cls = notification_type_cls.get_frequency(notification.recipient) + + # Send immediately if realtime, otherwise leave for digest + if frequency_cls and frequency_cls.is_realtime: + self.send_now(notification) + else: + # Non-digest channels always send immediately + self.send_now(notification) def send_now(self, notification: "Notification") -> None: """ @@ -88,10 +99,9 @@ class WebsiteChannel(BaseChannel): key = "website" name = "Website" - def process(self, notification: "Notification") -> None: + def send_now(self, notification: "Notification") -> None: """ Website notifications are just stored in DB - no additional processing needed. - The notification was already created before channels are processed. """ pass @@ -107,21 +117,6 @@ class EmailChannel(BaseChannel): name = "Email" supports_digest = True - def process(self, notification: "Notification") -> None: - """ - Process email notification based on user's frequency preference. - - Args: - notification: Notification instance to process - """ - # Get notification type class from key - notification_type_cls = registry.get_type(notification.notification_type) - frequency_cls = notification_type_cls.get_frequency(notification.recipient) - - # Send immediately if realtime, otherwise leave for digest - if frequency_cls and frequency_cls.is_realtime: - self.send_now(notification) - def send_now(self, notification: "Notification") -> None: """ Send an individual email notification immediately. From 1f2156c2a0215cf260e60e8073bee0281b0bb70e Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 18:56:39 +0200 Subject: [PATCH 05/15] More logical order of models --- generic_notifications/models.py | 230 ++++++++++++++++---------------- 1 file changed, 115 insertions(+), 115 deletions(-) diff --git a/generic_notifications/models.py b/generic_notifications/models.py index 342e37c..f68cdca 100644 --- a/generic_notifications/models.py +++ b/generic_notifications/models.py @@ -35,121 +35,6 @@ def unread(self): return self.filter(read__isnull=True) -class DisabledNotificationTypeChannel(models.Model): - """ - If a row exists here, that notification type/channel combination is DISABLED for the user. - By default (no row), all notifications are enabled on all channels. - """ - - user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="disabled_notification_type_channels") - notification_type = models.CharField(max_length=50) - channel = models.CharField(max_length=20) - - class Meta: - unique_together = ["user", "notification_type", "channel"] - - def clean(self): - try: - notification_type_cls = registry.get_type(self.notification_type) - except KeyError: - available_types = [t.key for t in registry.get_all_types()] - if available_types: - raise ValidationError( - f"Unknown notification type: {self.notification_type}. Available types: {available_types}" - ) - else: - raise ValidationError( - f"Unknown notification type: {self.notification_type}. No notification types are currently registered." - ) - - # Check if trying to disable a required channel - required_channel_keys = [cls.key for cls in notification_type_cls.required_channels] - if self.channel in required_channel_keys: - raise ValidationError( - f"Cannot disable {self.channel} channel for {notification_type_cls.name} - this channel is required" - ) - - try: - registry.get_channel(self.channel) - except KeyError: - available_channels = [c.key for c in registry.get_all_channels()] - if available_channels: - raise ValidationError(f"Unknown channel: {self.channel}. Available channels: {available_channels}") - else: - raise ValidationError(f"Unknown channel: {self.channel}. No channels are currently registered.") - - def __str__(self) -> str: - return f"{self.user} disabled {self.notification_type} on {self.channel}" - - -class NotificationFrequency(models.Model): - """ - Delivery frequency preference per notification type. - This applies to all channels that support the chosen frequency. - Default is `NotificationType.default_frequency` if no row exists. - """ - - user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="notification_frequencies") - notification_type = models.CharField(max_length=50) - frequency = models.CharField(max_length=20) - - class Meta: - unique_together = ["user", "notification_type"] - - def clean(self): - if self.notification_type: - try: - registry.get_type(self.notification_type) - except KeyError: - available_types = [t.key for t in registry.get_all_types()] - if available_types: - raise ValidationError( - f"Unknown notification type: {self.notification_type}. Available types: {available_types}" - ) - else: - raise ValidationError( - f"Unknown notification type: {self.notification_type}. No notification types are currently registered." - ) - - if self.frequency: - try: - registry.get_frequency(self.frequency) - except KeyError: - available_frequencies = [f.key for f in registry.get_all_frequencies()] - if available_frequencies: - raise ValidationError( - f"Unknown frequency: {self.frequency}. Available frequencies: {available_frequencies}" - ) - else: - raise ValidationError( - f"Unknown frequency: {self.frequency}. No frequencies are currently registered." - ) - - def __str__(self) -> str: - return f"{self.user} - {self.notification_type}: {self.frequency}" - - -class NotificationChannel(models.Model): - """ - Tracks which channels a notification should be sent through and their delivery status. - """ - - notification = models.ForeignKey("Notification", on_delete=models.CASCADE, related_name="channels") - channel = models.CharField(max_length=20) # e.g., 'email', 'website', etc. - sent_at = models.DateTimeField(null=True, blank=True) - - class Meta: - unique_together = ["notification", "channel"] - indexes = [ - models.Index(fields=["notification", "channel", "sent_at"]), - models.Index(fields=["channel", "sent_at"]), # For digest queries - ] - - def __str__(self): - status = "sent" if self.sent_at else "pending" - return f"{self.notification} - {self.channel} ({status})" - - class Notification(models.Model): """ A specific notification instance for a user @@ -302,3 +187,118 @@ def get_absolute_url(self) -> str: # No base URL configured, return relative URL return self.url + + +class NotificationChannel(models.Model): + """ + Tracks which channels a notification should be sent through and their delivery status. + """ + + notification = models.ForeignKey(Notification, on_delete=models.CASCADE, related_name="channels") + channel = models.CharField(max_length=20) # e.g., 'email', 'website', etc. + sent_at = models.DateTimeField(null=True, blank=True) + + class Meta: + unique_together = ["notification", "channel"] + indexes = [ + models.Index(fields=["notification", "channel", "sent_at"]), + models.Index(fields=["channel", "sent_at"]), # For digest queries + ] + + def __str__(self): + status = "sent" if self.sent_at else "pending" + return f"{self.notification} - {self.channel} ({status})" + + +class DisabledNotificationTypeChannel(models.Model): + """ + If a row exists here, that notification type/channel combination is DISABLED for the user. + By default (no row), all notifications are enabled on all channels. + """ + + user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="disabled_notification_type_channels") + notification_type = models.CharField(max_length=50) + channel = models.CharField(max_length=20) + + class Meta: + unique_together = ["user", "notification_type", "channel"] + + def clean(self): + try: + notification_type_cls = registry.get_type(self.notification_type) + except KeyError: + available_types = [t.key for t in registry.get_all_types()] + if available_types: + raise ValidationError( + f"Unknown notification type: {self.notification_type}. Available types: {available_types}" + ) + else: + raise ValidationError( + f"Unknown notification type: {self.notification_type}. No notification types are currently registered." + ) + + # Check if trying to disable a required channel + required_channel_keys = [cls.key for cls in notification_type_cls.required_channels] + if self.channel in required_channel_keys: + raise ValidationError( + f"Cannot disable {self.channel} channel for {notification_type_cls.name} - this channel is required" + ) + + try: + registry.get_channel(self.channel) + except KeyError: + available_channels = [c.key for c in registry.get_all_channels()] + if available_channels: + raise ValidationError(f"Unknown channel: {self.channel}. Available channels: {available_channels}") + else: + raise ValidationError(f"Unknown channel: {self.channel}. No channels are currently registered.") + + def __str__(self) -> str: + return f"{self.user} disabled {self.notification_type} on {self.channel}" + + +class NotificationFrequency(models.Model): + """ + Delivery frequency preference per notification type. + This applies to all channels that support the chosen frequency. + Default is `NotificationType.default_frequency` if no row exists. + """ + + user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="notification_frequencies") + notification_type = models.CharField(max_length=50) + frequency = models.CharField(max_length=20) + + class Meta: + unique_together = ["user", "notification_type"] + + def clean(self): + if self.notification_type: + try: + registry.get_type(self.notification_type) + except KeyError: + available_types = [t.key for t in registry.get_all_types()] + if available_types: + raise ValidationError( + f"Unknown notification type: {self.notification_type}. Available types: {available_types}" + ) + else: + raise ValidationError( + f"Unknown notification type: {self.notification_type}. No notification types are currently registered." + ) + + if self.frequency: + try: + registry.get_frequency(self.frequency) + except KeyError: + available_frequencies = [f.key for f in registry.get_all_frequencies()] + if available_frequencies: + raise ValidationError( + f"Unknown frequency: {self.frequency}. Available frequencies: {available_frequencies}" + ) + else: + raise ValidationError( + f"Unknown frequency: {self.frequency}. No frequencies are currently registered." + ) + + def __str__(self) -> str: + return f"{self.user} - {self.notification_type}: {self.frequency}" From 1701fee12ab149c047e2d3dda0b7ae41c0d9bf9a Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 19:10:00 +0200 Subject: [PATCH 06/15] Finish renames --- README.md | 12 ++++++------ example/notifications/admin.py | 6 +++--- ...er_disablednotificationtypechannel_id_and_more.py | 1 - 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 86db6cf..a956d0c 100644 --- a/README.md +++ b/README.md @@ -137,7 +137,7 @@ save_notification_preferences(user, request.POST) You can also manage preferences directly: ```python -from generic_notifications.models import DisabledNotificationTypeChannel, EmailFrequency +from generic_notifications.models import DisabledNotificationTypeChannel, NotificationFrequency from generic_notifications.channels import EmailChannel from generic_notifications.frequencies import RealtimeFrequency from myapp.notifications import CommentNotification @@ -146,7 +146,7 @@ from myapp.notifications import CommentNotification CommentNotification.disable_channel(user=user, channel=EmailChannel) # Change to realtime digest for a notification type -CommentNotification.set_email_frequency(user=user, frequency=RealtimeFrequency) +CommentNotification.set_frequency(user=user, frequency=RealtimeFrequency) ``` ## Custom Channels @@ -154,10 +154,10 @@ CommentNotification.set_email_frequency(user=user, frequency=RealtimeFrequency) Create custom delivery channels: ```python -from generic_notifications.channels import NotificationChannel, register +from generic_notifications.channels import BaseChannel, register @register -class SMSChannel(NotificationChannel): +class SMSChannel(BaseChannel): key = "sms" name = "SMS" @@ -174,10 +174,10 @@ class SMSChannel(NotificationChannel): Add custom email frequencies: ```python -from generic_notifications.frequencies import NotificationFrequency, register +from generic_notifications.frequencies import BaseFrequency, register @register -class WeeklyFrequency(NotificationFrequency): +class WeeklyFrequency(BaseFrequency): key = "weekly" name = "Weekly digest" is_realtime = False diff --git a/example/notifications/admin.py b/example/notifications/admin.py index 035e366..1104d5c 100644 --- a/example/notifications/admin.py +++ b/example/notifications/admin.py @@ -1,5 +1,5 @@ from django.contrib import admin -from generic_notifications.models import DisabledNotificationTypeChannel, EmailFrequency, Notification +from generic_notifications.models import DisabledNotificationTypeChannel, NotificationFrequency, Notification @admin.register(Notification) @@ -12,6 +12,6 @@ class DisabledNotificationTypeChannelAdmin(admin.ModelAdmin): list_display = ["user", "notification_type", "channel"] -@admin.register(EmailFrequency) -class EmailFrequencyAdmin(admin.ModelAdmin): +@admin.register(NotificationFrequency) +class NotificationFrequencyAdmin(admin.ModelAdmin): list_display = ["user", "notification_type", "frequency"] diff --git a/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py b/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py index 6692529..67bcd37 100644 --- a/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py +++ b/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py @@ -17,7 +17,6 @@ def migrate_channels_to_notificationchannel(apps, schema_editor): channel=channel, ) - # If this is the email channel and email_sent_at is set, mark it as sent if notification.email_sent_at: delivery.sent_at = notification.email_sent_at delivery.save() From 9c0099471af4e768e80f6f7a888ed77bb7bc8f14 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 20:26:37 +0200 Subject: [PATCH 07/15] Renamed send_digest_emails to send_notification_digests --- README.md | 6 ++-- ...emails.py => send_notification_digests.py} | 4 +-- generic_notifications/preferences.py | 29 +++++++++---------- 3 files changed, 19 insertions(+), 20 deletions(-) rename generic_notifications/management/commands/{send_digest_emails.py => send_notification_digests.py} (94%) diff --git a/README.md b/README.md index a956d0c..dd74418 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,7 @@ Create a cron job to send daily digests: ```bash # Send daily digests at 9 AM -0 9 * * * cd /path/to/project && uv run ./manage.py send_digest_emails --frequency daily +0 9 * * * cd /path/to/project && uv run ./manage.py send_notification_digests --frequency daily ``` ## User Preferences @@ -184,11 +184,11 @@ class WeeklyFrequency(BaseFrequency): description = "Receive a weekly summary every Monday" ``` -When you add custom email frequencies you’ll have to run `send_digest_emails` for them as well. For example, if you created that weekly digest: +When you add custom email frequencies you'll have to run `send_notification_digests` for them as well. For example, if you created that weekly digest: ```bash # Send weekly digest every Monday at 9 AM -0 9 * * 1 cd /path/to/project && uv run ./manage.py send_digest_emails --frequency weekly +0 9 * * 1 cd /path/to/project && uv run ./manage.py send_notification_digests --frequency weekly ``` ## Email Templates diff --git a/generic_notifications/management/commands/send_digest_emails.py b/generic_notifications/management/commands/send_notification_digests.py similarity index 94% rename from generic_notifications/management/commands/send_digest_emails.py rename to generic_notifications/management/commands/send_notification_digests.py index 106dc9a..e3309cc 100644 --- a/generic_notifications/management/commands/send_digest_emails.py +++ b/generic_notifications/management/commands/send_notification_digests.py @@ -8,13 +8,13 @@ class Command(BaseCommand): - help = "Send digest emails to users who have opted for digest delivery" + help = "Send notification digests to users who have opted for digest delivery" def add_arguments(self, parser): parser.add_argument( "--dry-run", action="store_true", - help="Show what would be sent without actually sending emails", + help="Show what would be sent without actually sending the digests", ) parser.add_argument( "--frequency", diff --git a/generic_notifications/preferences.py b/generic_notifications/preferences.py index 0beb406..7207b35 100644 --- a/generic_notifications/preferences.py +++ b/generic_notifications/preferences.py @@ -13,7 +13,7 @@ def get_notification_preferences(user: "AbstractUser") -> List[Dict[str, Any]]: Returns a list of dictionaries, each containing: - notification_type: The NotificationType instance - channels: Dict of channel_key -> {channel, enabled, required} - - email_frequency: The current email frequency key for this type + - notification_frequency: The current notification frequency key for this type This data structure can be used directly in templates to render notification preference forms. @@ -26,8 +26,8 @@ def get_notification_preferences(user: "AbstractUser") -> List[Dict[str, Any]]: DisabledNotificationTypeChannel.objects.filter(user=user).values_list("notification_type", "channel") ) - # Get user's email frequency preferences - email_frequencies = dict( + # Get user's notification frequency preferences + notification_frequencies = dict( NotificationFrequency.objects.filter(user=user).values_list("notification_type", "frequency") ) @@ -38,7 +38,7 @@ def get_notification_preferences(user: "AbstractUser") -> List[Dict[str, Any]]: type_data: Dict[str, Any] = { "notification_type": notification_type, "channels": {}, - "email_frequency": email_frequencies.get(type_key, notification_type.default_frequency.key), + "notification_frequency": notification_frequencies.get(type_key, notification_type.default_frequency.key), } for channel in channels.values(): @@ -63,7 +63,7 @@ def save_notification_preferences(user: "AbstractUser", form_data: Dict[str, Any Expected form_data format: - For channels: "{notification_type_key}__{channel_key}" -> "on" (if enabled) - - For email frequencies: "{notification_type_key}__frequency" -> frequency_key + - For notification frequencies: "{notification_type_key}__frequency" -> frequency_key This function implements an opt-out model: channels are enabled by default and only disabled entries are stored in the database. @@ -93,13 +93,12 @@ def save_notification_preferences(user: "AbstractUser", form_data: Dict[str, Any if form_key not in form_data: notification_type.disable_channel(user=user, channel=channel) - # Handle email frequency preference - if "email" in [ch.key for ch in channels.values()]: - frequency_key = f"{type_key}__frequency" - if frequency_key in form_data: - frequency_value = form_data[frequency_key] - if frequency_value in frequencies: - frequency_obj = frequencies[frequency_value] - # Only save if different from default - if frequency_value != notification_type.default_frequency.key: - notification_type.set_frequency(user=user, frequency=frequency_obj) + # Handle notification frequency preference + frequency_key = f"{type_key}__frequency" + if frequency_key in form_data: + frequency_value = form_data[frequency_key] + if frequency_value in frequencies: + frequency_obj = frequencies[frequency_value] + # Only save if different from default + if frequency_value != notification_type.default_frequency.key: + notification_type.set_frequency(user=user, frequency=frequency_obj) From ada7f3395299cc6e258e8c7c244a283bf736ce62 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 20:35:28 +0200 Subject: [PATCH 08/15] Finish the rename, fix tests --- example/notifications/admin.py | 2 +- generic_notifications/digest.py | 4 +- .../commands/send_notification_digests.py | 4 +- tests/test_management_commands.py | 38 +++++++++---------- tests/test_preferences.py | 6 +-- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/example/notifications/admin.py b/example/notifications/admin.py index 1104d5c..6e955c2 100644 --- a/example/notifications/admin.py +++ b/example/notifications/admin.py @@ -1,5 +1,5 @@ from django.contrib import admin -from generic_notifications.models import DisabledNotificationTypeChannel, NotificationFrequency, Notification +from generic_notifications.models import DisabledNotificationTypeChannel, Notification, NotificationFrequency @admin.register(Notification) diff --git a/generic_notifications/digest.py b/generic_notifications/digest.py index 01080b3..0b7ff1c 100644 --- a/generic_notifications/digest.py +++ b/generic_notifications/digest.py @@ -14,9 +14,9 @@ logger = logging.getLogger(__name__) -def send_digest_notifications(frequency_key: str, dry_run: bool = False) -> int: +def send_notification_digests(frequency_key: str, dry_run: bool = False) -> int: """ - Send digest notifications for a specific frequency across all channels that support digests. + Send notification digests for a specific frequency across all channels that support digests. Args: frequency_key: The frequency key to process (e.g., 'daily', 'weekly') diff --git a/generic_notifications/management/commands/send_notification_digests.py b/generic_notifications/management/commands/send_notification_digests.py index e3309cc..598d2df 100644 --- a/generic_notifications/management/commands/send_notification_digests.py +++ b/generic_notifications/management/commands/send_notification_digests.py @@ -2,7 +2,7 @@ from django.core.management.base import BaseCommand -from generic_notifications.digest import send_digest_notifications +from generic_notifications.digest import send_notification_digests logger = logging.getLogger(__name__) @@ -35,7 +35,7 @@ def handle(self, *args, **options): logger.info("DRY RUN - No notifications will be sent") try: - total_digests_sent = send_digest_notifications(target_frequency, dry_run) + total_digests_sent = send_notification_digests(target_frequency, dry_run) if dry_run: logger.info(f"DRY RUN: Would have sent {total_digests_sent} digest notifications") diff --git a/tests/test_management_commands.py b/tests/test_management_commands.py index c67bafb..11744d9 100644 --- a/tests/test_management_commands.py +++ b/tests/test_management_commands.py @@ -58,7 +58,7 @@ def tearDown(self): def test_dry_run_option(self): # Just test that dry-run option works without errors - call_command("send_digest_emails", "--frequency", "daily", "--dry-run") + call_command("send_notification_digests", "--frequency", "daily", "--dry-run") def test_no_digest_frequencies(self): # Clear all frequencies and add only realtime @@ -66,16 +66,16 @@ def test_no_digest_frequencies(self): registry.register_frequency(RealtimeFrequency) # Should complete without sending any emails - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") self.assertEqual(len(mail.outbox), 0) def test_target_frequency_not_found(self): # Should complete without error when frequency not found (logging is handled internally) - call_command("send_digest_emails", "--frequency", "nonexistent") + call_command("send_notification_digests", "--frequency", "nonexistent") def test_target_frequency_is_realtime(self): # Should complete without error when frequency is realtime (logging is handled internally) - call_command("send_digest_emails", "--frequency", "realtime") + call_command("send_notification_digests", "--frequency", "realtime") def test_send_digest_emails_basic_flow(self): # Set up user with daily frequency preference @@ -93,7 +93,7 @@ def test_send_digest_emails_basic_flow(self): # No emails should be in outbox initially self.assertEqual(len(mail.outbox), 0) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") # Verify email was sent self.assertEqual(len(mail.outbox), 1) @@ -119,7 +119,7 @@ def test_dry_run_does_not_send_emails(self): # Ensure no emails in outbox initially self.assertEqual(len(mail.outbox), 0) - call_command("send_digest_emails", "--frequency", "daily", "--dry-run") + call_command("send_notification_digests", "--frequency", "daily", "--dry-run") # Should not send any emails in dry run self.assertEqual(len(mail.outbox), 0) @@ -150,7 +150,7 @@ def test_only_includes_unread_notifications(self): channels=["email"], ) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") # Should send one email with only unread notification self.assertEqual(len(mail.outbox), 1) @@ -185,7 +185,7 @@ def test_only_includes_unsent_notifications(self): channels=["email"], ) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") # Should send one email with only unsent notification self.assertEqual(len(mail.outbox), 1) @@ -221,7 +221,7 @@ def test_sends_all_unsent_notifications(self): channels=["email"], ) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") # Should send one email with both notifications (no time window filtering) self.assertEqual(len(mail.outbox), 1) @@ -259,7 +259,7 @@ def test_specific_frequency_filter(self): channels=["email"], ) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") # Should only send email to daily user self.assertEqual(len(mail.outbox), 1) @@ -269,7 +269,7 @@ def test_specific_frequency_filter(self): # Clear outbox and test weekly frequency mail.outbox.clear() - call_command("send_digest_emails", "--frequency", "weekly") + call_command("send_notification_digests", "--frequency", "weekly") # Should only send email to weekly user self.assertEqual(len(mail.outbox), 1) @@ -298,7 +298,7 @@ def test_multiple_notification_types_for_user(self): channels=["email"], ) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") # Should send one digest email with both notifications self.assertEqual(len(mail.outbox), 1) @@ -321,7 +321,7 @@ def test_no_notifications_to_send(self): # No notifications created - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") # Should not send any emails self.assertEqual(len(mail.outbox), 0) @@ -335,7 +335,7 @@ def test_users_with_disabled_email_channel_dont_get_digest(self): ) # Run daily digest - should not send anything (no email channel) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") self.assertEqual(len(mail.outbox), 0) def test_users_with_default_frequencies_get_digest(self): @@ -361,7 +361,7 @@ def test_users_with_default_frequencies_get_digest(self): ) # Run daily digest - should include comment but not system message - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") self.assertEqual(len(mail.outbox), 1) email = mail.outbox[0] @@ -398,11 +398,11 @@ def test_mixed_explicit_and_default_preferences(self): ) # Run daily digest - should get nothing (test_type is weekly, other_type is realtime) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") self.assertEqual(len(mail.outbox), 0) # Run weekly digest - should get test notification - call_command("send_digest_emails", "--frequency", "weekly") + call_command("send_notification_digests", "--frequency", "weekly") self.assertEqual(len(mail.outbox), 1) email = mail.outbox[0] self.assertEqual(email.body, "You have 1 new notification:\n\n- Test notification text") @@ -427,7 +427,7 @@ def test_multiple_users_default_and_explicit_mix(self): ) # Run daily digest - should get user1 and user3 (both have test_type=daily) - call_command("send_digest_emails", "--frequency", "daily") + call_command("send_notification_digests", "--frequency", "daily") self.assertEqual(len(mail.outbox), 2) recipients = {email.to[0] for email in mail.outbox} @@ -435,7 +435,7 @@ def test_multiple_users_default_and_explicit_mix(self): # Clear outbox and run weekly digest - should get user2 mail.outbox.clear() - call_command("send_digest_emails", "--frequency", "weekly") + call_command("send_notification_digests", "--frequency", "weekly") self.assertEqual(len(mail.outbox), 1) self.assertEqual(mail.outbox[0].to[0], self.user2.email) self.assertEqual(mail.outbox[0].body, "You have 1 new notification:\n\n- Test notification for user 2 text") diff --git a/tests/test_preferences.py b/tests/test_preferences.py index bb0997c..8d97d51 100644 --- a/tests/test_preferences.py +++ b/tests/test_preferences.py @@ -73,9 +73,9 @@ def test_email_frequency_defaults_and_overrides(self): for pref in preferences: if pref["notification_type"].key == "test_notification": - self.assertEqual(pref["email_frequency"], "realtime") + self.assertEqual(pref["notification_frequency"], "realtime") elif pref["notification_type"].key == "required_channel_notification": - self.assertEqual(pref["email_frequency"], "daily") + self.assertEqual(pref["notification_frequency"], "daily") # Now override one NotificationFrequency.objects.create(user=self.user, notification_type="test_notification", frequency="daily") @@ -84,7 +84,7 @@ def test_email_frequency_defaults_and_overrides(self): for pref in preferences: if pref["notification_type"].key == "test_notification": - self.assertEqual(pref["email_frequency"], "daily") + self.assertEqual(pref["notification_frequency"], "daily") class SaveNotificationPreferencesTest(TestCase): From 5ec8e13fd2cfb95adf83d5319e189ee5a4adf073 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 20:55:56 +0200 Subject: [PATCH 09/15] send_notification_digests takes a class instead of a string --- generic_notifications/digest.py | 19 ++++++------------- .../commands/send_notification_digests.py | 8 +++++++- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/generic_notifications/digest.py b/generic_notifications/digest.py index 0b7ff1c..d45f4a8 100644 --- a/generic_notifications/digest.py +++ b/generic_notifications/digest.py @@ -14,29 +14,22 @@ logger = logging.getLogger(__name__) -def send_notification_digests(frequency_key: str, dry_run: bool = False) -> int: +def send_notification_digests(frequency: type[BaseFrequency], dry_run: bool = False) -> int: """ Send notification digests for a specific frequency across all channels that support digests. Args: - frequency_key: The frequency key to process (e.g., 'daily', 'weekly') + frequency: The frequency class to process (e.g., DailyFrequency, WeeklyFrequency) 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") + if frequency.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] @@ -45,13 +38,13 @@ def send_notification_digests(frequency_key: str, dry_run: bool = False) -> int: logger.warning("No channels support digest functionality") return 0 - logger.info(f"Processing {frequency_cls.name} digests for {len(digest_channels)} channel(s)...") + logger.info(f"Processing {frequency.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) + channel_digests_sent = _send_digest_for_channel(channel, frequency, all_notification_types, dry_run) total_digests_sent += channel_digests_sent if dry_run: diff --git a/generic_notifications/management/commands/send_notification_digests.py b/generic_notifications/management/commands/send_notification_digests.py index 598d2df..2290a4e 100644 --- a/generic_notifications/management/commands/send_notification_digests.py +++ b/generic_notifications/management/commands/send_notification_digests.py @@ -3,6 +3,7 @@ from django.core.management.base import BaseCommand from generic_notifications.digest import send_notification_digests +from generic_notifications.registry import registry logger = logging.getLogger(__name__) @@ -35,7 +36,12 @@ def handle(self, *args, **options): logger.info("DRY RUN - No notifications will be sent") try: - total_digests_sent = send_notification_digests(target_frequency, dry_run) + # Get the frequency class from the registry + frequency_cls = registry.get_frequency(target_frequency) + if not frequency_cls: + raise KeyError(f"Frequency '{target_frequency}' not found") + + total_digests_sent = send_notification_digests(frequency_cls, dry_run) if dry_run: logger.info(f"DRY RUN: Would have sent {total_digests_sent} digest notifications") From 18fb33cec8567a338ff1572e3da859c211bcf534 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 21:21:56 +0200 Subject: [PATCH 10/15] Code formatting --- tests/test_channels.py | 82 +++++++++++++++++++++++++++++++----------- 1 file changed, 61 insertions(+), 21 deletions(-) diff --git a/tests/test_channels.py b/tests/test_channels.py index 97cddc8..51ce817 100644 --- a/tests/test_channels.py +++ b/tests/test_channels.py @@ -73,7 +73,9 @@ class OtherNotificationType(NotificationType): # Disable notification channel for this user DisabledNotificationTypeChannel.objects.create( - user=self.user, notification_type="disabled_type", channel="test" + user=self.user, + notification_type="disabled_type", + channel="test", ) # Should be disabled for this type @@ -89,7 +91,9 @@ def setUp(self): registry.register_type(TestNotificationType) self.notification = Notification.objects.create( - recipient=self.user, notification_type="test_type", subject="Test Subject" + recipient=self.user, + notification_type="test_type", + subject="Test Subject", ) def tearDown(self): @@ -109,7 +113,9 @@ def tearDown(self): @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_process_realtime_frequency(self): notification = create_notification_with_channels( - user=self.user, channels=["website", "email"], notification_type="test_type" + user=self.user, + channels=["website", "email"], + notification_type="test_type", ) channel = EmailChannel() @@ -129,7 +135,9 @@ def test_process_digest_frequency(self): NotificationFrequency.objects.create(user=self.user, notification_type="test_type", frequency="daily") notification = create_notification_with_channels( - user=self.user, channels=["website", "email"], notification_type="test_type" + user=self.user, + channels=["website", "email"], + notification_type="test_type", ) channel = EmailChannel() @@ -171,7 +179,9 @@ def test_send_now_basic(self): def test_send_now_uses_get_methods(self): # Create notification without stored subject/text to test dynamic generation notification = create_notification_with_channels( - user=self.user, notification_type="test_type", channels=["email"] + user=self.user, + notification_type="test_type", + channels=["email"], ) channel = EmailChannel() @@ -205,7 +215,6 @@ def mock_render_side_effect(template_name, context): notification_type="test_type", subject="Test Subject", text="Test message", - channels=["email"], ) channel = EmailChannel() @@ -241,7 +250,9 @@ def mock_render_side_effect(template_name, context): @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_send_now_template_error_fallback(self): notification = create_notification_with_channels( - user=self.user, notification_type="test_type", subject="Test Subject", channels=["email"] + user=self.user, + notification_type="test_type", + subject="Test Subject", ) channel = EmailChannel() @@ -270,12 +281,16 @@ def test_send_digest_emails_basic(self): # Create test notifications without email_sent_at (unsent) for i in range(3): create_notification_with_channels( - user=self.user, notification_type="test_type", subject=f"Test {i}", channels=["email"] + user=self.user, + notification_type="test_type", + subject=f"Test {i}", ) # Get notifications as queryset notifications = Notification.objects.filter( - recipient=self.user, channels__channel="email", channels__sent_at__isnull=True + recipient=self.user, + channels__channel="email", + channels__sent_at__isnull=True, ) # Send digest email for this user @@ -298,11 +313,17 @@ def test_send_digest_emails_with_frequency(self): 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", channels=["email"] + 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) + Notification.objects.filter( + recipient=self.user, + channels__channel="email", + channels__sent_at__isnull=True, + ) ) email = mail.outbox[0] @@ -314,11 +335,17 @@ def test_send_digest_emails_without_frequency(self): 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", channels=["email"] + 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) + Notification.objects.filter( + recipient=self.user, + channels__channel="email", + channels__sent_at__isnull=True, + ) ) email = mail.outbox[0] @@ -332,11 +359,17 @@ def test_send_digest_emails_text_limit(self): # 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}", channels=["email"] + 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) + Notification.objects.filter( + recipient=self.user, + channels__channel="email", + channels__sent_at__isnull=True, + ) ) # The implementation may not have this feature, so we'll just check that email was sent @@ -353,11 +386,17 @@ def test_send_digest_emails_with_html_template(self, mock_render): 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", channels=["email"] + 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) + Notification.objects.filter( + recipient=self.user, + channels__channel="email", + channels__sent_at__isnull=True, + ) ) # Check templates were rendered (subject, HTML, then text) @@ -381,7 +420,6 @@ def test_send_now_fallback_includes_url(self): """Test that fallback email content includes URL when available""" notification = create_notification_with_channels( user=self.user, - channels=["email"], notification_type=TestNotificationType.key, subject="Test Subject", text="Test notification text", @@ -403,21 +441,23 @@ def test_send_digest_emails_fallback_includes_urls(self): # Create notifications with URLs create_notification_with_channels( user=self.user, - channels=["email"], notification_type=TestNotificationType.key, text="First notification", url="https://example.com/url/1", ) create_notification_with_channels( user=self.user, - channels=["email"], notification_type=TestNotificationType.key, text="Second notification", url="https://example.com/url/2", ) EmailChannel().send_digest( - Notification.objects.filter(recipient=self.user, channels__channel="email", channels__sent_at__isnull=True) + Notification.objects.filter( + recipient=self.user, + channels__channel="email", + channels__sent_at__isnull=True, + ) ) # Check that one email was sent From 866d89181de74e87b82239b997d3ff529ef9efdf Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sat, 11 Oct 2025 21:36:46 +0200 Subject: [PATCH 11/15] Code formatting --- tests/test_channels.py | 9 +---- tests/test_management_commands.py | 25 ++++--------- tests/test_models.py | 59 +++++++++++++++++++------------ tests/test_performance.py | 7 ---- tests/test_utils.py | 37 ++++++++++--------- 5 files changed, 65 insertions(+), 72 deletions(-) diff --git a/tests/test_channels.py b/tests/test_channels.py index 51ce817..0f0d961 100644 --- a/tests/test_channels.py +++ b/tests/test_channels.py @@ -114,7 +114,6 @@ def tearDown(self): def test_process_realtime_frequency(self): notification = create_notification_with_channels( user=self.user, - channels=["website", "email"], notification_type="test_type", ) @@ -136,7 +135,6 @@ def test_process_digest_frequency(self): notification = create_notification_with_channels( user=self.user, - channels=["website", "email"], notification_type="test_type", ) @@ -157,7 +155,6 @@ def test_send_now_basic(self): notification_type="test_type", subject="Test Subject", text="Test message", - channels=["email"], ) channel = EmailChannel() @@ -178,11 +175,7 @@ def test_send_now_basic(self): @override_settings(DEFAULT_FROM_EMAIL="test@example.com") def test_send_now_uses_get_methods(self): # Create notification without stored subject/text to test dynamic generation - notification = create_notification_with_channels( - user=self.user, - notification_type="test_type", - channels=["email"], - ) + notification = create_notification_with_channels(user=self.user, notification_type="test_type") channel = EmailChannel() channel.send_now(notification) diff --git a/tests/test_management_commands.py b/tests/test_management_commands.py index 11744d9..5eb61ed 100644 --- a/tests/test_management_commands.py +++ b/tests/test_management_commands.py @@ -87,7 +87,6 @@ def test_send_digest_emails_basic_flow(self): notification_type="test_type", subject="Test notification", text="This is a test notification", - channels=["email"], ) # No emails should be in outbox initially @@ -113,7 +112,9 @@ def test_dry_run_does_not_send_emails(self): NotificationFrequency.objects.create(user=self.user1, notification_type="test_type", frequency="daily") notification = create_notification_with_channels( - user=self.user1, notification_type="test_type", subject="Test notification", channels=["email"] + user=self.user1, + notification_type="test_type", + subject="Test notification", ) # Ensure no emails in outbox initially @@ -138,7 +139,6 @@ def test_only_includes_unread_notifications(self): notification_type="test_type", subject="Read notification subject", text="Read notification text", - channels=["email"], ) read_notification.mark_as_read() @@ -147,7 +147,6 @@ def test_only_includes_unread_notifications(self): notification_type="test_type", subject="Unread notification subject", text="Unread notification text", - channels=["email"], ) call_command("send_notification_digests", "--frequency", "daily") @@ -172,7 +171,6 @@ def test_only_includes_unsent_notifications(self): user=self.user1, notification_type="test_type", subject="Sent notification", - channels=["email"], ) # Mark as sent sent_notification.mark_sent_on_channel(EmailChannel) @@ -182,7 +180,6 @@ def test_only_includes_unsent_notifications(self): notification_type="test_type", subject="Unsent notification subject", text="Unsent notification text", - channels=["email"], ) call_command("send_notification_digests", "--frequency", "daily") @@ -206,7 +203,6 @@ def test_sends_all_unsent_notifications(self): notification_type="test_type", subject="Old notification subject", text="Old notification text", - channels=["email"], ) # Manually set old timestamp old_time = timezone.now() - timedelta(days=2) @@ -218,7 +214,6 @@ def test_sends_all_unsent_notifications(self): notification_type="test_type", subject="Recent notification subject", text="Recent notification text", - channels=["email"], ) call_command("send_notification_digests", "--frequency", "daily") @@ -249,14 +244,12 @@ def test_specific_frequency_filter(self): notification_type="test_type", subject="Daily user notification subject", text="Daily user notification text", - channels=["email"], ) create_notification_with_channels( user=self.user2, notification_type="test_type", subject="Weekly user notification subject", text="Weekly user notification text", - channels=["email"], ) call_command("send_notification_digests", "--frequency", "daily") @@ -288,14 +281,12 @@ def test_multiple_notification_types_for_user(self): notification_type="test_type", subject="Test type notification subject", text="Test type notification text", - channels=["email"], ) notification2 = create_notification_with_channels( user=self.user1, notification_type="other_type", subject="Other type notification subject", text="Other type notification text", - channels=["email"], ) call_command("send_notification_digests", "--frequency", "daily") @@ -331,7 +322,10 @@ def test_users_with_disabled_email_channel_dont_get_digest(self): # With the new architecture, if email is disabled, notifications won't have email channel # So create a notification without email channel to simulate this create_notification_with_channels( - user=self.user1, notification_type="test_type", subject="Test notification", channels=["website"] + user=self.user1, + notification_type="test_type", + subject="Test notification", + channels=["website"], ) # Run daily digest - should not send anything (no email channel) @@ -348,7 +342,6 @@ def test_users_with_default_frequencies_get_digest(self): notification_type="test_type", subject="Test notification", text="This is a test notification", - channels=["email"], ) # Create an other_type notification (defaults to realtime) @@ -357,7 +350,6 @@ def test_users_with_default_frequencies_get_digest(self): notification_type="other_type", subject="Other notification", text="This is another type of notification", - channels=["email"], ) # Run daily digest - should include comment but not system message @@ -387,14 +379,12 @@ def test_mixed_explicit_and_default_preferences(self): notification_type="test_type", subject="Test notification subject", text="Test notification text", - channels=["email"], ) create_notification_with_channels( user=self.user1, notification_type="other_type", subject="Other notification subject", text="Other notification text", - channels=["email"], ) # Run daily digest - should get nothing (test_type is weekly, other_type is realtime) @@ -423,7 +413,6 @@ def test_multiple_users_default_and_explicit_mix(self): notification_type="test_type", subject=f"Test notification for user {i} subject", text=f"Test notification for user {i} text", - channels=["email"], ) # Run daily digest - should get user1 and user3 (both have test_type=daily) diff --git a/tests/test_models.py b/tests/test_models.py index e93849e..121bbc7 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -51,7 +51,9 @@ def setUpClass(cls): def test_create_disabled_notification(self): disabled = DisabledNotificationTypeChannel.objects.create( - user=self.user, notification_type=TestNotificationType.key, channel=WebsiteChannel.key + user=self.user, + notification_type=TestNotificationType.key, + channel=WebsiteChannel.key, ) self.assertEqual(disabled.user, self.user) @@ -60,7 +62,9 @@ def test_create_disabled_notification(self): def test_clean_with_invalid_notification_type(self): disabled = DisabledNotificationTypeChannel( - user=self.user, notification_type="invalid_type", channel=WebsiteChannel.key + user=self.user, + notification_type="invalid_type", + channel=WebsiteChannel.key, ) with self.assertRaises(ValidationError) as cm: @@ -70,7 +74,9 @@ def test_clean_with_invalid_notification_type(self): def test_clean_with_invalid_channel(self): disabled = DisabledNotificationTypeChannel( - user=self.user, notification_type=TestNotificationType.key, channel="invalid_channel" + user=self.user, + notification_type=TestNotificationType.key, + channel="invalid_channel", ) with self.assertRaises(ValidationError) as cm: @@ -80,7 +86,9 @@ def test_clean_with_invalid_channel(self): def test_clean_with_valid_data(self): disabled = DisabledNotificationTypeChannel( - user=self.user, notification_type=TestNotificationType.key, channel=WebsiteChannel.key + user=self.user, + notification_type=TestNotificationType.key, + channel=WebsiteChannel.key, ) # Should not raise any exception @@ -89,7 +97,9 @@ def test_clean_with_valid_data(self): def test_clean_prevents_disabling_required_channel(self): """Test that users cannot disable required channels for notification types""" disabled = DisabledNotificationTypeChannel( - user=self.user, notification_type=SystemMessage.key, channel=EmailChannel.key + user=self.user, + notification_type=SystemMessage.key, + channel=EmailChannel.key, ) with self.assertRaises(ValidationError) as cm: @@ -100,7 +110,9 @@ def test_clean_prevents_disabling_required_channel(self): def test_clean_allows_disabling_non_required_channel(self): """Test that users can disable non-required channels for notification types with required channels""" disabled = DisabledNotificationTypeChannel( - user=self.user, notification_type=SystemMessage.key, channel=WebsiteChannel.key + user=self.user, + notification_type=SystemMessage.key, + channel=WebsiteChannel.key, ) # Should not raise any exception - website is not required for system_message @@ -122,7 +134,9 @@ def setUpClass(cls): def test_create_email_frequency(self): frequency = NotificationFrequency.objects.create( - user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key + user=self.user, + notification_type=TestNotificationType.key, + frequency=DailyFrequency.key, ) self.assertEqual(frequency.user, self.user) @@ -131,17 +145,23 @@ def test_create_email_frequency(self): def test_unique_together_constraint(self): NotificationFrequency.objects.create( - user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key + user=self.user, + notification_type=TestNotificationType.key, + frequency=DailyFrequency.key, ) with self.assertRaises(IntegrityError): NotificationFrequency.objects.create( - user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key + user=self.user, + notification_type=TestNotificationType.key, + frequency=DailyFrequency.key, ) def test_clean_with_invalid_notification_type(self): frequency = NotificationFrequency( - user=self.user, notification_type="invalid_type", frequency=DailyFrequency.key + user=self.user, + notification_type="invalid_type", + frequency=DailyFrequency.key, ) with self.assertRaises(ValidationError) as cm: @@ -151,7 +171,9 @@ def test_clean_with_invalid_notification_type(self): def test_clean_with_invalid_frequency(self): frequency = NotificationFrequency( - user=self.user, notification_type=TestNotificationType.key, frequency="invalid_frequency" + user=self.user, + notification_type=TestNotificationType.key, + frequency="invalid_frequency", ) with self.assertRaises(ValidationError) as cm: @@ -161,7 +183,9 @@ def test_clean_with_invalid_frequency(self): def test_clean_with_valid_data(self): frequency = NotificationFrequency( - user=self.user, notification_type=TestNotificationType.key, frequency=DailyFrequency.key + user=self.user, + notification_type=TestNotificationType.key, + frequency=DailyFrequency.key, ) # Should not raise any exception @@ -288,16 +312,13 @@ def test_email_sent_tracking(self): self.assertFalse(notification.is_sent_on_channel(WebsiteChannel)) def test_get_absolute_url_empty_url(self): - notification = create_notification_with_channels( - user=self.user, notification_type=TestNotificationType.key, channels=[WebsiteChannel.key] - ) + notification = create_notification_with_channels(user=self.user, notification_type=TestNotificationType.key) self.assertEqual(notification.get_absolute_url(), "") def test_get_absolute_url_already_absolute(self): notification = create_notification_with_channels( user=self.user, notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], url="https://example.com/path", ) self.assertEqual(notification.get_absolute_url(), "https://example.com/path") @@ -306,7 +327,6 @@ def test_get_absolute_url_with_setting(self): notification = create_notification_with_channels( user=self.user, notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], url="/notifications/123", ) @@ -317,7 +337,6 @@ def test_get_absolute_url_with_setting_no_protocol_debug(self): notification = create_notification_with_channels( user=self.user, notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], url="/notifications/123", ) @@ -329,7 +348,6 @@ def test_get_absolute_url_with_setting_no_protocol_production(self): notification = create_notification_with_channels( user=self.user, notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], url="/notifications/123", ) @@ -341,7 +359,6 @@ def test_get_absolute_url_fallback_settings(self): notification = create_notification_with_channels( user=self.user, notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], url="/notifications/123", ) @@ -352,7 +369,6 @@ def test_get_absolute_url_fallback_settings_no_protocol(self): notification = create_notification_with_channels( user=self.user, notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], url="/notifications/123", ) @@ -364,7 +380,6 @@ def test_get_absolute_url_no_base_url(self): notification = create_notification_with_channels( user=self.user, notification_type=TestNotificationType.key, - channels=[WebsiteChannel.key], url="/notifications/123", ) diff --git a/tests/test_performance.py b/tests/test_performance.py index fb5bcda..51fcbf5 100644 --- a/tests/test_performance.py +++ b/tests/test_performance.py @@ -1,7 +1,6 @@ from django.contrib.auth import get_user_model from django.test import TestCase -from generic_notifications.channels import WebsiteChannel from generic_notifications.utils import get_notifications from .test_helpers import create_notification_with_channels @@ -21,7 +20,6 @@ def setUp(self): notification_type="test_notification", subject=f"Test notification {i}", text=f"This is test notification {i}", - channels=[WebsiteChannel.key], url=f"/notification/{i}/", ) @@ -69,7 +67,6 @@ def test_notification_target_access_queries(self): notification_type="test_notification", subject=f"Test notification {i}", text=f"This is test notification {i}", - channels=[WebsiteChannel.key], target=self.actor, ) @@ -94,7 +91,6 @@ def test_notification_target_relationship_access(self): notification_type="target_notification", subject=f"Target notification {i}", text=f"Target text {i}", - channels=[WebsiteChannel.key], ) # Create notification pointing to it @@ -104,7 +100,6 @@ def test_notification_target_relationship_access(self): notification_type="test_notification", subject=f"Test notification {i}", text=f"This is test notification {i}", - channels=[WebsiteChannel.key], target=target_notification, ) @@ -130,7 +125,6 @@ def test_notification_target_relationship_preselect_access(self): notification_type="target_notification", subject=f"Target notification {i}", text=f"Target text {i}", - channels=[WebsiteChannel.key], ) # Create notification pointing to it @@ -140,7 +134,6 @@ def test_notification_target_relationship_preselect_access(self): notification_type="test_notification", subject=f"Test notification {i}", text=f"This is test notification {i}", - channels=[WebsiteChannel.key], target=target_notification, ) diff --git a/tests/test_utils.py b/tests/test_utils.py index 36b4e8b..e569528 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -186,7 +186,8 @@ def test_mark_already_read_notifications(self): def test_mark_notifications_other_user_not_affected(self): other_user = User.objects.create_user(username="other", email="other@example.com", password="testpass") other_notification = create_notification_with_channels( - user=other_user, notification_type="test_type", channels=["website", "email"] + user=other_user, + notification_type="test_type", ) mark_notifications_as_read(self.user) @@ -207,22 +208,18 @@ def test_get_unread_count_empty(self): def test_get_unread_count_with_unread_notifications(self): # Create unread notifications - create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) - create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) - create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) + create_notification_with_channels(user=self.user, notification_type="test_type") + create_notification_with_channels(user=self.user, notification_type="test_type") + create_notification_with_channels(user=self.user, notification_type="test_type") count = get_unread_count(self.user, channel=WebsiteChannel) self.assertEqual(count, 3) def test_get_unread_count_with_mix_of_read_unread(self): # Create mix of read and unread - notification1 = create_notification_with_channels( - user=self.user, notification_type="test_type", channels=["website"] - ) - create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) - notification3 = create_notification_with_channels( - user=self.user, notification_type="test_type", channels=["website"] - ) + notification1 = create_notification_with_channels(user=self.user, notification_type="test_type") + create_notification_with_channels(user=self.user, notification_type="test_type") + notification3 = create_notification_with_channels(user=self.user, notification_type="test_type") # Mark some as read notification1.mark_as_read() @@ -235,8 +232,8 @@ def test_get_unread_count_other_user_not_counted(self): other_user = User.objects.create_user(username="other", email="other@example.com", password="testpass") # Create notifications for both users - create_notification_with_channels(user=self.user, notification_type="test_type", channels=["website"]) - create_notification_with_channels(user=other_user, notification_type="test_type", channels=["website", "email"]) + create_notification_with_channels(user=self.user, notification_type="test_type") + create_notification_with_channels(user=other_user, notification_type="test_type") count = get_unread_count(self.user, channel=WebsiteChannel) self.assertEqual(count, 1) @@ -250,13 +247,19 @@ def setUp(self): # Create test notifications self.notification1 = create_notification_with_channels( - user=self.user, notification_type="test_type", subject="Test 1", channels=["website", "email"] + user=self.user, + notification_type="test_type", + subject="Test 1", ) self.notification2 = create_notification_with_channels( - user=self.user, notification_type="other_type", subject="Other 1", channels=["website", "email"] + user=self.user, + notification_type="other_type", + subject="Other 1", ) self.notification3 = create_notification_with_channels( - user=self.user, notification_type="test_type", subject="Test 2", channels=["website", "email"] + user=self.user, + notification_type="test_type", + subject="Test 2", ) # Mark one as read @@ -287,7 +290,7 @@ def test_get_with_limit(self): def test_get_notifications_other_user_not_included(self): other_user = User.objects.create_user(username="other", email="other@example.com", password="testpass") - create_notification_with_channels(user=other_user, notification_type="test_type", channels=["website", "email"]) + create_notification_with_channels(user=other_user, notification_type="test_type") notifications = get_notifications(self.user, channel=WebsiteChannel) From d2e4df23a21c2c229e5175d244f3509ea3f5a143 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sun, 12 Oct 2025 14:12:51 +0200 Subject: [PATCH 12/15] Fix migration, fix example app admin --- example/notifications/admin.py | 10 ++++++- example/uv.lock | 2 +- .../commands/send_notification_digests.py | 2 +- ...blednotificationtypechannel_id_and_more.py | 26 ++++++++++++++++--- generic_notifications/models.py | 1 + 5 files changed, 34 insertions(+), 7 deletions(-) diff --git a/example/notifications/admin.py b/example/notifications/admin.py index 6e955c2..ac28304 100644 --- a/example/notifications/admin.py +++ b/example/notifications/admin.py @@ -4,7 +4,15 @@ @admin.register(Notification) class NotificationAdmin(admin.ModelAdmin): - list_display = ["recipient", "notification_type", "added", "channels"] + list_display = ["recipient", "notification_type", "added", "get_channels"] + + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related("channels") + + @admin.display(description="Channels") + def get_channels(self, obj): + channels = obj.channels.values_list("channel", flat=True) + return ", ".join(channels) if channels else "-" @admin.register(DisabledNotificationTypeChannel) diff --git a/example/uv.lock b/example/uv.lock index a8b3b7c..850cce6 100644 --- a/example/uv.lock +++ b/example/uv.lock @@ -65,7 +65,7 @@ wheels = [ [[package]] name = "django-generic-notifications" -version = "1.1.1" +version = "1.4.0" source = { editable = "../" } dependencies = [ { name = "django" }, diff --git a/generic_notifications/management/commands/send_notification_digests.py b/generic_notifications/management/commands/send_notification_digests.py index 2290a4e..6f5ea65 100644 --- a/generic_notifications/management/commands/send_notification_digests.py +++ b/generic_notifications/management/commands/send_notification_digests.py @@ -40,7 +40,7 @@ def handle(self, *args, **options): frequency_cls = registry.get_frequency(target_frequency) if not frequency_cls: raise KeyError(f"Frequency '{target_frequency}' not found") - + total_digests_sent = send_notification_digests(frequency_cls, dry_run) if dry_run: diff --git a/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py b/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py index 67bcd37..a15096a 100644 --- a/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py +++ b/generic_notifications/migrations/0002_alter_disablednotificationtypechannel_id_and_more.py @@ -10,13 +10,17 @@ def migrate_channels_to_notificationchannel(apps, schema_editor): NotificationChannel = apps.get_model("generic_notifications", "NotificationChannel") for notification in Notification.objects.all(): + # Access the channels_old JSONField (renamed to avoid conflict) + channels_data = notification.channels_old if hasattr(notification, "channels_old") else [] + # Create NotificationChannel entries for each channel - for channel in notification.channels: + for channel in channels_data: delivery, created = NotificationChannel.objects.get_or_create( notification=notification, channel=channel, ) + # If this is the email channel and email_sent_at is set, update sent_at if notification.email_sent_at: delivery.sent_at = notification.email_sent_at delivery.save() @@ -29,12 +33,12 @@ def reverse_migrate_channels(apps, schema_editor): for notification in Notification.objects.all(): # Rebuild channels list from NotificationChannel entries - channels = list(notification.channels.values_list("channel", flat=True)) - notification.channels = channels + channels = list(NotificationChannel.objects.filter(notification=notification).values_list("channel", flat=True)) + notification.channels_old = channels # Restore email_sent_at from email NotificationChannel try: - email_delivery = notification.channels.get(channel="email") + email_delivery = NotificationChannel.objects.get(notification=notification, channel="email") if email_delivery.sent_at: notification.email_sent_at = email_delivery.sent_at except NotificationChannel.DoesNotExist: @@ -49,6 +53,13 @@ class Migration(migrations.Migration): ] operations = [ + # First, rename the channels field to avoid naming conflict with the related_name + migrations.RenameField( + model_name="notification", + old_name="channels", + new_name="channels_old", + ), + # Create the new NotificationChannel model migrations.CreateModel( name="NotificationChannel", fields=[ @@ -72,8 +83,15 @@ class Migration(migrations.Migration): "unique_together": {("notification", "channel")}, }, ), + # Migrate the data migrations.RunPython( migrate_channels_to_notificationchannel, reverse_migrate_channels, ), + # Rename back to channels for migration 3 to handle removal + migrations.RenameField( + model_name="notification", + old_name="channels_old", + new_name="channels", + ), ] diff --git a/generic_notifications/models.py b/generic_notifications/models.py index f68cdca..a69d082 100644 --- a/generic_notifications/models.py +++ b/generic_notifications/models.py @@ -270,6 +270,7 @@ class NotificationFrequency(models.Model): class Meta: unique_together = ["user", "notification_type"] + verbose_name_plural = "Notification frequencies" def clean(self): if self.notification_type: From a8eee579d8078e309eb89f8f990dcb689de9b530 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sun, 12 Oct 2025 14:17:45 +0200 Subject: [PATCH 13/15] Fix notifications setting screen in the example app --- example/notifications/templates/settings.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/example/notifications/templates/settings.html b/example/notifications/templates/settings.html index 481deaf..4e38aa5 100644 --- a/example/notifications/templates/settings.html +++ b/example/notifications/templates/settings.html @@ -48,7 +48,7 @@

Notification settings

{% endfor %} - + {% if "email" in channels %} From f4c0fd9c0c3646628ef1b6a08e6e72ba67d648e4 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sun, 12 Oct 2025 14:37:01 +0200 Subject: [PATCH 14/15] Add support for new supports_realtime flag in channels --- generic_notifications/channels.py | 28 ++++++++++++++++++++-------- tests/test_channels.py | 18 ++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/generic_notifications/channels.py b/generic_notifications/channels.py index 283a6ce..679a18b 100644 --- a/generic_notifications/channels.py +++ b/generic_notifications/channels.py @@ -22,27 +22,36 @@ class BaseChannel(ABC): key: str name: str + supports_realtime: bool = True supports_digest: bool = False def process(self, notification: "Notification") -> None: """ - Process a notification through this channel. - For digest-supporting channels, this checks frequency preference. - For non-digest channels, this sends immediately. + Process a notification through this channel based on channel capabilities + and user preferences. If the notification should be handled realtime, + then call `send_now`. If it should be handled in a digest delivery, + then do nothing, as the send_notification_digests function/command will + pick it up. Args: notification: Notification instance to process """ + # Digest-only channels: never send immediately + if self.supports_digest and not self.supports_realtime: + return + + # Channels that support both: check user preference if self.supports_digest: # Get notification type class from key notification_type_cls = registry.get_type(notification.notification_type) frequency_cls = notification_type_cls.get_frequency(notification.recipient) - # Send immediately if realtime, otherwise leave for digest - if frequency_cls and frequency_cls.is_realtime: - self.send_now(notification) - else: - # Non-digest channels always send immediately + # User prefers digest delivery (not realtime) + if frequency_cls and not frequency_cls.is_realtime: + return + + # Send immediately if channel supports realtime + if self.supports_realtime: self.send_now(notification) def send_now(self, notification: "Notification") -> None: @@ -98,6 +107,8 @@ class WebsiteChannel(BaseChannel): key = "website" name = "Website" + supports_realtime = True + supports_digest = False def send_now(self, notification: "Notification") -> None: """ @@ -115,6 +126,7 @@ class EmailChannel(BaseChannel): key = "email" name = "Email" + supports_realtime = True supports_digest = True def send_now(self, notification: "Notification") -> None: diff --git a/tests/test_channels.py b/tests/test_channels.py index 0f0d961..4e44239 100644 --- a/tests/test_channels.py +++ b/tests/test_channels.py @@ -84,6 +84,24 @@ class OtherNotificationType(NotificationType): # But enabled for other types self.assertTrue(OtherNotificationType.is_channel_enabled(self.user, TestChannel)) + def test_digest_only_channel_never_sends_immediately(self): + """Test that channels with supports_realtime=False never send immediately.""" + + class DigestOnlyChannel(BaseChannel): + key = "digest_only" + name = "Digest Only" + supports_realtime = False + supports_digest = True + + def send_now(self, notification): + raise AssertionError("send_now should never be called for digest-only channel") + + channel = DigestOnlyChannel() + notification = Notification.objects.create(recipient=self.user, notification_type="test_type", subject="Test") + + # Process should not call send_now (would raise AssertionError if it did) + channel.process(notification) + class WebsiteChannelTest(TestCase): def setUp(self): From b97edb1281978dd3400f7e6cdffd5621d6963d0d Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Sun, 12 Oct 2025 14:58:10 +0200 Subject: [PATCH 15/15] Add `prefetch_related("channels")` Show correct usage of the .prefetch() method in the readme --- README.md | 37 ++++++++++++++++++--------------- generic_notifications/models.py | 2 +- tests/test_performance.py | 8 +++---- 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index dd74418..fb24040 100644 --- a/README.md +++ b/README.md @@ -54,11 +54,13 @@ NOTIFICATION_BASE_URL = "www.example.com" ``` **Protocol handling**: If you omit the protocol, it's automatically added: -- `https://` in production (`DEBUG = False`) + +- `https://` in production (`DEBUG = False`) - `http://` in development (`DEBUG = True`) **Fallback order** if `NOTIFICATION_BASE_URL` is not set: -1. `BASE_URL` setting + +1. `BASE_URL` setting 2. `SITE_URL` setting 3. Django Sites framework (if `django.contrib.sites` is installed) 4. URLs remain relative if no base URL is found (not ideal in emails!) @@ -109,7 +111,7 @@ Create a cron job to send daily digests: By default every user gets notifications of all registered types delivered to every registered channel, but users can opt-out of receiving notification types, per channel. -All notification types default to daily digest, except for `SystemMessage` which defaults to real-time. Users can choose different frequency per notification type. +All notification types default to daily digest, except for `SystemMessage` which defaults to real-time. Users can choose different frequency per notification type. This project doesn't come with a UI (view + template) for managing user preferences, but an example is provided in the [example app](#example-app). @@ -242,7 +244,7 @@ unread_count = get_unread_count(user=user, channel=WebsiteChannel) unread_notifications = get_notifications(user=user, channel=WebsiteChannel, unread_only=True) # Get notifications by channel -website_notifications = Notification.objects.for_channel(WebsiteChannel) +website_notifications = Notification.objects.prefetch().for_channel(WebsiteChannel) # Mark as read notification = website_notifications.first() @@ -303,9 +305,9 @@ The `should_save` method is called before saving each notification. Return `Fals The `target` field is a GenericForeignKey that can point to any Django model instance. While convenient, accessing targets requires careful consideration for performance. -When using Django 5.0+, this library automatically includes `.prefetch_related("target")` when using the standard query methods. This efficiently fetches target objects, but only the *direct* targets - accessing relationships *through* the target will still cause additional queries. +When using Django 5.0+, this library automatically includes `.prefetch_related("target")` when using the standard query methods. This efficiently fetches target objects, but only the _direct_ targets - accessing relationships _through_ the target will still cause additional queries. -*On Django 4.2, you'll need to manually deal with prefetching the `target` relationship.* +_On Django 4.2, you'll need to manually deal with prefetching the `target` relationship._ Consider this problematic example that will cause N+1 queries: @@ -331,7 +333,7 @@ class CommentNotificationType(NotificationType): actor_name = notification.actor.full_name article = notification.target.article comment_text = notification.target.comment_text - + # This causes an extra query per notification! return f'{actor_name} commented on your article "{article.title}": "{comment_text}"' ``` @@ -365,7 +367,7 @@ However, this only works if you don’t need to dynamically generate the text - If you must access relationships through the target, you can prefetch them: ```python -# On Django 5.0+ the library already prefetches targets, +# On Django 5.0+ the library already prefetches targets, # but you need to add deeper relationships yourself notifications = get_notifications(user).prefetch_related( "target__article" # This prevents the N+1 problem @@ -373,6 +375,7 @@ notifications = get_notifications(user).prefetch_related( ``` **Note**: This approach has limitations: + - You need to know the target's type and relationships in advance - It won't work efficiently with heterogeneous targets (different model types) - Each additional relationship level requires explicit prefetching @@ -408,7 +411,7 @@ class CommentNotificationType(NotificationType): current_lang = get_language() # Get parameters for current language, fallback to English lang_params = notification.metadata.get(current_lang, notification.metadata.get("en", {})) - + return _("%(commenter_name)s commented on %(page_title)s") % lang_params # When creating the notification @@ -418,7 +421,7 @@ from django.utils.translation import activate, get_language def create_multilingual_notification(recipient, commenter, page): current_lang = get_language() multilingual_metadata = {} - + # Store parameters for each language for lang_code, _ in settings.LANGUAGES: activate(lang_code) @@ -426,9 +429,9 @@ def create_multilingual_notification(recipient, commenter, page): "commenter_name": commenter.get_full_name(), "page_title": page.get_title(), # Assumes this returns translated title } - + activate(current_lang) # Restore original language - + send_notification( recipient=recipient, notification_type=CommentNotificationType, @@ -454,7 +457,7 @@ class CommentNotificationType(NotificationType): def get_text(self, notification): from django.utils.translation import gettext as _ - + # Access current language data from the target if notification.target: return _("%(commenter)s commented on %(page_title)s") % { @@ -477,10 +480,10 @@ send_notification( ### Performance considerations -| Approach | Storage Overhead | Query Performance | Translation Freshness | -|----------|------------------|-------------------|----------------------| -| Approach 1 | Moderate | Excellent | Frozen when created | -| Approach 2 | None | Good (with prefetching) | Always current | +| Approach | Storage Overhead | Query Performance | Translation Freshness | +| ---------- | ---------------- | ----------------------- | --------------------- | +| Approach 1 | Moderate | Excellent | Frozen when created | +| Approach 2 | None | Good (with prefetching) | Always current | - Use **approach 1** if you have performance-critical displays and can accept that text is frozen when the notification is created - Use **approach 2** if you need always-current data diff --git a/generic_notifications/models.py b/generic_notifications/models.py index a69d082..0ebd331 100644 --- a/generic_notifications/models.py +++ b/generic_notifications/models.py @@ -18,7 +18,7 @@ class NotificationQuerySet(models.QuerySet): def prefetch(self): """Prefetch related objects""" - qs = self.select_related("recipient", "actor") + qs = self.select_related("recipient", "actor").prefetch_related("channels") # Only add target prefetching on Django 5.0+ due to GenericForeignKey limitations if django.VERSION >= (5, 0): diff --git a/tests/test_performance.py b/tests/test_performance.py index 51fcbf5..9bf17ed 100644 --- a/tests/test_performance.py +++ b/tests/test_performance.py @@ -25,7 +25,7 @@ def setUp(self): def test_get_notifications_queries(self): """Test the number of queries made by get_notifications""" - with self.assertNumQueries(1): + with self.assertNumQueries(2): # 1 for notifications + 1 for channels prefetch notifications = get_notifications(self.user) # Force evaluation of the queryset list(notifications) @@ -71,7 +71,7 @@ def test_notification_target_access_queries(self): ) # First, evaluate the queryset - with self.assertNumQueries(2): # 1 for notifications + 1 for targets + with self.assertNumQueries(3): # 1 for notifications + 1 for channels prefetch + 1 for targets notifications = get_notifications(self.user) notifications_list = list(notifications) @@ -104,7 +104,7 @@ def test_notification_target_relationship_access(self): ) # First, evaluate the queryset - with self.assertNumQueries(2): # 1 for notifications + 1 for targets + with self.assertNumQueries(3): # 1 for notifications + 1 for channels prefetch + 1 for targets notifications = get_notifications(self.user) notifications_list = list(notifications) @@ -138,7 +138,7 @@ def test_notification_target_relationship_preselect_access(self): ) # First, evaluate the queryset - with self.assertNumQueries(3): # 1 for notifications + 1 for targets + 1 for target recipients + with self.assertNumQueries(4): # 1 for notifications + 1 for channels + 1 for targets + 1 for target recipients notifications = get_notifications(self.user).prefetch_related("target__recipient") notifications_list = list(notifications)