From 2c780c8acda2d174d00689ab58e317e86f19477d Mon Sep 17 00:00:00 2001 From: Dhanus Date: Tue, 4 Jun 2024 13:13:31 +0530 Subject: [PATCH 01/13] [chore] Basic implementation --- openwisp_notifications/handlers.py | 98 ++++++++++++++++++++---------- openwisp_notifications/settings.py | 6 ++ openwisp_notifications/tasks.py | 19 ++++++ 3 files changed, 92 insertions(+), 31 deletions(-) diff --git a/openwisp_notifications/handlers.py b/openwisp_notifications/handlers.py index c14efdf3..55ce8462 100644 --- a/openwisp_notifications/handlers.py +++ b/openwisp_notifications/handlers.py @@ -1,6 +1,7 @@ import logging from celery.exceptions import OperationalError +from celery.result import AsyncResult from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType from django.core.cache import cache @@ -26,6 +27,7 @@ logger = logging.getLogger(__name__) EXTRA_DATA = app_settings.get_config()['USE_JSONFIELD'] +EMAIL_BATCH_INTERVAL = app_settings.get_config()['EMAIL_BATCH_INTERVAL'] User = get_user_model() @@ -161,12 +163,8 @@ def notify_handler(**kwargs): return notification_list -@receiver(post_save, sender=Notification, dispatch_uid='send_email_notification') -def send_email_notification(sender, instance, created, **kwargs): - # Abort if a new notification is not created - if not created: - return - # Get email preference of user for this type of notification. +def can_send_email(instance): + # Get email preference of the user for this type of notification target_org = getattr(getattr(instance, 'target', None), 'organization_id', None) if instance.type and target_org: try: @@ -174,26 +172,28 @@ def send_email_notification(sender, instance, created, **kwargs): organization=target_org, type=instance.type ) except NotificationSetting.DoesNotExist: - return + return False email_preference = notification_setting.email_notification else: - # We can not check email preference if notification type is absent, - # or if target_org is not present - # therefore send email anyway. + # Send email anyway if notification type or target_org is absent email_preference = True + # Check if email is verified email_verified = instance.recipient.emailaddress_set.filter( verified=True, email=instance.recipient.email ).exists() + # Verify email preference and email verification status if not (email_preference and instance.recipient.email and email_verified): - return + return False + # Verify the email subject try: subject = instance.email_subject except NotificationRenderException: - # Do not send email if notification is malformed. - return + return False + + # Get the URL and description url = instance.data.get('url', '') if instance.data else None description = instance.message if url: @@ -203,25 +203,61 @@ def send_email_notification(sender, instance, created, **kwargs): else: target_url = None if target_url: - description += _('\n\nFor more information see %(target_url)s.') % { - 'target_url': target_url - } - - send_email( - subject, - description, - instance.message, - recipients=[instance.recipient.email], - extra_context={ - 'call_to_action_url': target_url, - 'call_to_action_text': _('Find out more'), - }, - ) + description += _('\n\nFor more information see %(target_url)s.') % {'target_url': target_url} - # flag as emailed - instance.emailed = True - # bulk_update is used to prevent emitting post_save signal - Notification.objects.bulk_update([instance], ['emailed']) + return True, subject, description, target_url + + +@receiver(post_save, sender=Notification, dispatch_uid='send_email_notification') +def send_email_notification(sender, instance, created, **kwargs): + # Abort if a new notification is not created + if not created: + return + + can_send, subject, description, target_url = can_send_email(instance) + + if not can_send: + return + + recipient_id = instance.recipient.id + cache_key = f'email_batch_{recipient_id}' + cache_data = cache.get(cache_key, {'last_email_sent_time': None, 'batch_scheduled': False}) + is_send_email = False + + if cache_data['last_email_sent_time']: + if (timezone.now() - cache_data['last_email_sent_time']).seconds < EMAIL_BATCH_INTERVAL: + if not cache_data['batch_scheduled']: + # Schedule batch email notification task + tasks.batch_email_notification.apply_async((recipient_id,), countdown=EMAIL_BATCH_INTERVAL) + cache_data['batch_scheduled'] = True + cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) + return + else: + # If the interval has passed, send the email immediately + is_send_email = True + cache_data['last_email_sent_time'] = timezone.now() + cache_data['batch_scheduled'] = False + else: + # If no email has been sent yet, send the email immediately + is_send_email = True + cache_data['last_email_sent_time'] = timezone.now() + + cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) + if is_send_email: + send_email( + subject, + description, + instance.message, + recipients=[instance.recipient.email], + extra_context={ + 'call_to_action_url': target_url, + 'call_to_action_text': _('Find out more'), + }, + ) + # flag as emailed + instance.emailed = True + # bulk_update is used to prevent emitting post_save signal + Notification.objects.bulk_update([instance], ['emailed']) @receiver(post_save, sender=Notification, dispatch_uid='clear_notification_cache_saved') diff --git a/openwisp_notifications/settings.py b/openwisp_notifications/settings.py index ea57bddb..a4e57501 100644 --- a/openwisp_notifications/settings.py +++ b/openwisp_notifications/settings.py @@ -37,6 +37,12 @@ 'openwisp-notifications/audio/notification_bell.mp3', ) +EMAIL_BATCH_INTERVAL = getattr( + settings, + 'EMAIL_BATCH_INTERVAL', + 30 * 60 # 30 minutes +) + # Remove the leading "/static/" here as it will # conflict with the "static()" call in context_processors.py. # This is done for backward compatibility. diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py index 9869d46a..18bb83f8 100644 --- a/openwisp_notifications/tasks.py +++ b/openwisp_notifications/tasks.py @@ -3,14 +3,21 @@ from celery import shared_task from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType +from django.core.cache import cache from django.db.models import Q from django.db.utils import OperationalError from django.utils import timezone from openwisp_notifications import types +from openwisp_notifications import settings as app_settings +from openwisp_notifications import handlers from openwisp_notifications.swapper import load_model, swapper_load_model +from openwisp_notifications import settings as app_settings +from openwisp_utils.admin_theme.email import send_email from openwisp_utils.tasks import OpenwispCeleryTask +EMAIL_BATCH_INTERVAL = app_settings.get_config()['EMAIL_BATCH_INTERVAL'] + User = get_user_model() Notification = load_model('Notification') @@ -202,3 +209,15 @@ def delete_ignore_object_notification(instance_id): Deletes IgnoreObjectNotification object post it's expiration. """ IgnoreObjectNotification.objects.filter(id=instance_id).delete() + + +@shared_task(base=OpenwispCeleryTask) +def batch_email_notification(instance_id): + # Get all unsent notifications for the user in the last 30 minutes + time_threshold = timezone.now() - timedelta(minutes=30) + unsent_notifications = Notification.objects.filter( + emailed=False, + recipient_id=instance_id, + created__gte=time_threshold + ) + print("Sending email....") From 949ea246c082d18609371444e4ff7bc8284ff515 Mon Sep 17 00:00:00 2001 From: Dhanus Date: Thu, 6 Jun 2024 20:07:00 +0530 Subject: [PATCH 02/13] [chore] Updated changes --- openwisp_notifications/handlers.py | 115 ++++++++---------- openwisp_notifications/settings.py | 6 +- openwisp_notifications/tasks.py | 56 +++++++-- .../templates/emails/batch_email.html | 99 +++++++++++++++ 4 files changed, 198 insertions(+), 78 deletions(-) create mode 100644 openwisp_notifications/templates/emails/batch_email.html diff --git a/openwisp_notifications/handlers.py b/openwisp_notifications/handlers.py index 55ce8462..d54be086 100644 --- a/openwisp_notifications/handlers.py +++ b/openwisp_notifications/handlers.py @@ -1,7 +1,6 @@ import logging from celery.exceptions import OperationalError -from celery.result import AsyncResult from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType from django.core.cache import cache @@ -27,7 +26,7 @@ logger = logging.getLogger(__name__) EXTRA_DATA = app_settings.get_config()['USE_JSONFIELD'] -EMAIL_BATCH_INTERVAL = app_settings.get_config()['EMAIL_BATCH_INTERVAL'] +EMAIL_BATCH_INTERVAL = app_settings.OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL User = get_user_model() @@ -163,8 +162,12 @@ def notify_handler(**kwargs): return notification_list -def can_send_email(instance): - # Get email preference of the user for this type of notification +@receiver(post_save, sender=Notification, dispatch_uid='send_email_notification') +def send_email_notification(sender, instance, created, **kwargs): + # Abort if a new notification is not created + if not created: + return + # Get email preference of user for this type of notification. target_org = getattr(getattr(instance, 'target', None), 'organization_id', None) if instance.type and target_org: try: @@ -172,28 +175,26 @@ def can_send_email(instance): organization=target_org, type=instance.type ) except NotificationSetting.DoesNotExist: - return False + return email_preference = notification_setting.email_notification else: - # Send email anyway if notification type or target_org is absent + # We can not check email preference if notification type is absent, + # or if target_org is not present + # therefore send email anyway. email_preference = True - # Check if email is verified email_verified = instance.recipient.emailaddress_set.filter( verified=True, email=instance.recipient.email ).exists() - # Verify email preference and email verification status if not (email_preference and instance.recipient.email and email_verified): - return False + return - # Verify the email subject try: subject = instance.email_subject except NotificationRenderException: - return False - - # Get the URL and description + # Do not send email if notification is malformed. + return url = instance.data.get('url', '') if instance.data else None description = instance.message if url: @@ -203,61 +204,53 @@ def can_send_email(instance): else: target_url = None if target_url: - description += _('\n\nFor more information see %(target_url)s.') % {'target_url': target_url} - - return True, subject, description, target_url + description += _('\n\nFor more information see %(target_url)s.') % { + 'target_url': target_url + } - -@receiver(post_save, sender=Notification, dispatch_uid='send_email_notification') -def send_email_notification(sender, instance, created, **kwargs): - # Abort if a new notification is not created - if not created: - return - - can_send, subject, description, target_url = can_send_email(instance) - - if not can_send: - return - - recipient_id = instance.recipient.id + recipient_id = instance.recipient.id cache_key = f'email_batch_{recipient_id}' - cache_data = cache.get(cache_key, {'last_email_sent_time': None, 'batch_scheduled': False}) - is_send_email = False + cache_data = cache.get( + cache_key, {'last_email_sent_time': None, 'batch_task_id': None} + ) if cache_data['last_email_sent_time']: - if (timezone.now() - cache_data['last_email_sent_time']).seconds < EMAIL_BATCH_INTERVAL: - if not cache_data['batch_scheduled']: - # Schedule batch email notification task - tasks.batch_email_notification.apply_async((recipient_id,), countdown=EMAIL_BATCH_INTERVAL) - cache_data['batch_scheduled'] = True - cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) - return + if not cache_data['batch_task_id']: + # Schedule batch email notification task + task = tasks.batch_email_notification.apply_async( + (instance.recipient.email,), countdown=EMAIL_BATCH_INTERVAL + ) + cache_data['batch_task_id'] = task.id + cache.set( + cache_data['batch_task_id'], + [instance.id], + timeout=EMAIL_BATCH_INTERVAL * 2, + ) + cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) else: - # If the interval has passed, send the email immediately - is_send_email = True - cache_data['last_email_sent_time'] = timezone.now() - cache_data['batch_scheduled'] = False - else: - # If no email has been sent yet, send the email immediately - is_send_email = True - cache_data['last_email_sent_time'] = timezone.now() + ids = cache.get(cache_data['batch_task_id'], []) + ids.append(instance.id) + cache.set( + cache_data['batch_task_id'], ids, timeout=EMAIL_BATCH_INTERVAL * 2 + ) + return + cache_data['last_email_sent_time'] = timezone.now() cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) - if is_send_email: - send_email( - subject, - description, - instance.message, - recipients=[instance.recipient.email], - extra_context={ - 'call_to_action_url': target_url, - 'call_to_action_text': _('Find out more'), - }, - ) - # flag as emailed - instance.emailed = True - # bulk_update is used to prevent emitting post_save signal - Notification.objects.bulk_update([instance], ['emailed']) + send_email( + subject, + description, + instance.message, + recipients=[instance.recipient.email], + extra_context={ + 'call_to_action_url': target_url, + 'call_to_action_text': _('Find out more'), + }, + ) + # flag as emailed + instance.emailed = True + # bulk_update is used to prevent emitting post_save signal + Notification.objects.bulk_update([instance], ['emailed']) @receiver(post_save, sender=Notification, dispatch_uid='clear_notification_cache_saved') diff --git a/openwisp_notifications/settings.py b/openwisp_notifications/settings.py index a4e57501..b685f511 100644 --- a/openwisp_notifications/settings.py +++ b/openwisp_notifications/settings.py @@ -37,10 +37,8 @@ 'openwisp-notifications/audio/notification_bell.mp3', ) -EMAIL_BATCH_INTERVAL = getattr( - settings, - 'EMAIL_BATCH_INTERVAL', - 30 * 60 # 30 minutes +OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr( + settings, 'EMAIL_BATCH_INTERVAL', 30 * 60 # 30 minutes ) # Remove the leading "/static/" here as it will diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py index 18bb83f8..49db2c1f 100644 --- a/openwisp_notifications/tasks.py +++ b/openwisp_notifications/tasks.py @@ -6,17 +6,17 @@ from django.core.cache import cache from django.db.models import Q from django.db.utils import OperationalError +from django.template.loader import render_to_string from django.utils import timezone +from django.utils.html import strip_tags -from openwisp_notifications import types from openwisp_notifications import settings as app_settings -from openwisp_notifications import handlers +from openwisp_notifications import types from openwisp_notifications.swapper import load_model, swapper_load_model -from openwisp_notifications import settings as app_settings from openwisp_utils.admin_theme.email import send_email from openwisp_utils.tasks import OpenwispCeleryTask -EMAIL_BATCH_INTERVAL = app_settings.get_config()['EMAIL_BATCH_INTERVAL'] +EMAIL_BATCH_INTERVAL = app_settings.OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL User = get_user_model() @@ -211,13 +211,43 @@ def delete_ignore_object_notification(instance_id): IgnoreObjectNotification.objects.filter(id=instance_id).delete() -@shared_task(base=OpenwispCeleryTask) -def batch_email_notification(instance_id): - # Get all unsent notifications for the user in the last 30 minutes - time_threshold = timezone.now() - timedelta(minutes=30) - unsent_notifications = Notification.objects.filter( - emailed=False, - recipient_id=instance_id, - created__gte=time_threshold +@shared_task(base=OpenwispCeleryTask, bind=True) +def batch_email_notification(self, email_id): + ids = cache.get(self.request.id, []) + if not ids: + return + + unsent_notifications = Notification.objects.filter(id__in=ids) + alerts = [] + + for notification in unsent_notifications: + url = notification.data.get('url', '') if notification.data else None + if url: + target_url = url + elif notification.target: + target_url = notification.redirect_view_url + else: + target_url = None + alerts.append( + { + 'level': notification.level, + 'message': notification.message, + 'description': notification.message, + 'timestamp': notification.timestamp, + 'url': target_url, + } + ) + + context = { + 'alerts': alerts, + } + html_content = render_to_string('emails/batch_email.html', context) + plain_text_content = strip_tags(html_content) + send_email( + subject='Summary of Notifications', + body_text=plain_text_content, + body_html=html_content, + recipients=[email_id], ) - print("Sending email....") + unsent_notifications.update(emailed=True) + Notification.objects.bulk_update(unsent_notifications, ['emailed']) diff --git a/openwisp_notifications/templates/emails/batch_email.html b/openwisp_notifications/templates/emails/batch_email.html new file mode 100644 index 00000000..94ea79f9 --- /dev/null +++ b/openwisp_notifications/templates/emails/batch_email.html @@ -0,0 +1,99 @@ +{% block styles %} + +{% endblock styles %} + +{% block mail_body %} +
+ {% for alert in alerts %} +
+

+ {{ alert.level|upper }} + + {% if alert.url %} + {{ alert.message }} + {% else %} + {{ alert.message }} + {% endif %} + +

+

{{ alert.description }}
{{ alert.timestamp|date:"F j, Y, g:i a" }}

+
+ {% endfor %} +
+{% endblock mail_body %} From 305d6c8b87403c148831e0fc6c6f28c786004d79 Mon Sep 17 00:00:00 2001 From: Dhanus Date: Sat, 8 Jun 2024 00:31:20 +0530 Subject: [PATCH 03/13] [chore] Add tests --- openwisp_notifications/handlers.py | 21 +++++++------------ openwisp_notifications/tasks.py | 10 ++++++--- .../tests/test_notifications.py | 16 ++++++++++++++ 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/openwisp_notifications/handlers.py b/openwisp_notifications/handlers.py index d54be086..7da31718 100644 --- a/openwisp_notifications/handlers.py +++ b/openwisp_notifications/handlers.py @@ -210,29 +210,24 @@ def send_email_notification(sender, instance, created, **kwargs): recipient_id = instance.recipient.id cache_key = f'email_batch_{recipient_id}' + cache_key_pks = f'{instance.recipient.email}_pks' cache_data = cache.get( - cache_key, {'last_email_sent_time': None, 'batch_task_id': None} + cache_key, {'last_email_sent_time': None, 'batch_scheduled': False} ) if cache_data['last_email_sent_time']: - if not cache_data['batch_task_id']: + if not cache_data['batch_scheduled']: # Schedule batch email notification task - task = tasks.batch_email_notification.apply_async( + tasks.batch_email_notification.apply_async( (instance.recipient.email,), countdown=EMAIL_BATCH_INTERVAL ) - cache_data['batch_task_id'] = task.id - cache.set( - cache_data['batch_task_id'], - [instance.id], - timeout=EMAIL_BATCH_INTERVAL * 2, - ) + cache_data['batch_scheduled'] = True + cache.set(cache_key_pks, [instance.id], timeout=EMAIL_BATCH_INTERVAL * 2) cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) else: - ids = cache.get(cache_data['batch_task_id'], []) + ids = cache.get(cache_key_pks, []) ids.append(instance.id) - cache.set( - cache_data['batch_task_id'], ids, timeout=EMAIL_BATCH_INTERVAL * 2 - ) + cache.set(cache_key_pks, ids, timeout=EMAIL_BATCH_INTERVAL * 2) return cache_data['last_email_sent_time'] = timezone.now() diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py index 49db2c1f..c745f6ba 100644 --- a/openwisp_notifications/tasks.py +++ b/openwisp_notifications/tasks.py @@ -211,9 +211,13 @@ def delete_ignore_object_notification(instance_id): IgnoreObjectNotification.objects.filter(id=instance_id).delete() -@shared_task(base=OpenwispCeleryTask, bind=True) -def batch_email_notification(self, email_id): - ids = cache.get(self.request.id, []) +@shared_task(base=OpenwispCeleryTask) +def batch_email_notification(email_id): + """ + Sends a summary of notifications to the specified email address. + """ + ids = cache.get(f'{email_id}_pks', []) + if not ids: return diff --git a/openwisp_notifications/tests/test_notifications.py b/openwisp_notifications/tests/test_notifications.py index 04894ffd..a30a8649 100644 --- a/openwisp_notifications/tests/test_notifications.py +++ b/openwisp_notifications/tests/test_notifications.py @@ -893,6 +893,22 @@ def test_notification_for_unverified_email(self): # we don't send emails to unverified email addresses self.assertEqual(len(mail.outbox), 0) + @patch('openwisp_notifications.tasks.batch_email_notification.apply_async') + def test_batch_email_notification(self, mock_send_email): + notify.send(**self.notification_options) + notify.send(**self.notification_options) + notify.send(**self.notification_options) + + # Check if only one mail is sent + self.assertEqual(len(mail.outbox), 1) + + # Call the task + tasks.batch_email_notification(self.admin.email) + + # Check if the rest of the notifications are sent in a batch + self.assertEqual(len(mail.outbox), 2) + self.assertEqual(mail.outbox[1].subject, "Summary of Notifications") + class TestTransactionNotifications(TestOrganizationMixin, TransactionTestCase): def setUp(self): From 58bdc9d5d8cce79f832b0c15092d2d8b932db61c Mon Sep 17 00:00:00 2001 From: Dhanus Date: Sat, 8 Jun 2024 14:55:15 +0530 Subject: [PATCH 04/13] [chore] Update Readme --- README.rst | 13 +++++++++++++ openwisp_notifications/handlers.py | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/README.rst b/README.rst index f7f43b7c..e3656d59 100644 --- a/README.rst +++ b/README.rst @@ -956,6 +956,19 @@ The default configuration is as follows: 'max_allowed_backoff': 15, } +``OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ++---------+-----------------------------------+ +| type | ``int`` | ++---------+-----------------------------------+ +| default | ``1800`` `(30 mins, in seconds)` | ++---------+-----------------------------------+ + +This setting defines the interval at which the email notifications are sent in batches to users within the specified interval. + +If you want to send email notifications immediately, then set it to ``0``. + Exceptions ---------- diff --git a/openwisp_notifications/handlers.py b/openwisp_notifications/handlers.py index 7da31718..fab1dccf 100644 --- a/openwisp_notifications/handlers.py +++ b/openwisp_notifications/handlers.py @@ -215,7 +215,7 @@ def send_email_notification(sender, instance, created, **kwargs): cache_key, {'last_email_sent_time': None, 'batch_scheduled': False} ) - if cache_data['last_email_sent_time']: + if cache_data['last_email_sent_time'] and EMAIL_BATCH_INTERVAL > 0: if not cache_data['batch_scheduled']: # Schedule batch email notification task tasks.batch_email_notification.apply_async( From 4abc7174d86ed013f84aad459e64ddb437ffd262 Mon Sep 17 00:00:00 2001 From: Dhanus Date: Sat, 15 Jun 2024 15:53:59 +0530 Subject: [PATCH 05/13] [chore] New changes --- openwisp_notifications/handlers.py | 61 ++--- openwisp_notifications/tasks.py | 80 ++++--- .../templates/emails/batch_email.html | 220 ++++++++++-------- 3 files changed, 212 insertions(+), 149 deletions(-) diff --git a/openwisp_notifications/handlers.py b/openwisp_notifications/handlers.py index fab1dccf..bb01f8fe 100644 --- a/openwisp_notifications/handlers.py +++ b/openwisp_notifications/handlers.py @@ -162,6 +162,36 @@ def notify_handler(**kwargs): return notification_list +def send_single_email(instance): + try: + subject = instance.email_subject + except NotificationRenderException: + # Do not send email if notification is malformed. + return + url = instance.data.get('url', '') if instance.data else None + description = instance.message + if url: + target_url = url + elif instance.target: + target_url = instance.redirect_view_url + else: + target_url = None + if target_url: + description += _('\n\nFor more information see %(target_url)s.') % { + 'target_url': target_url + } + send_email( + subject, + description, + instance.message, + recipients=[instance.recipient.email], + extra_context={ + 'call_to_action_url': target_url, + 'call_to_action_text': _('Find out more'), + }, + ) + + @receiver(post_save, sender=Notification, dispatch_uid='send_email_notification') def send_email_notification(sender, instance, created, **kwargs): # Abort if a new notification is not created @@ -190,23 +220,7 @@ def send_email_notification(sender, instance, created, **kwargs): if not (email_preference and instance.recipient.email and email_verified): return - try: - subject = instance.email_subject - except NotificationRenderException: - # Do not send email if notification is malformed. - return - url = instance.data.get('url', '') if instance.data else None - description = instance.message - if url: - target_url = url - elif instance.target: - target_url = instance.redirect_view_url - else: - target_url = None - if target_url: - description += _('\n\nFor more information see %(target_url)s.') % { - 'target_url': target_url - } + send_single_email(instance) recipient_id = instance.recipient.id cache_key = f'email_batch_{recipient_id}' @@ -232,16 +246,9 @@ def send_email_notification(sender, instance, created, **kwargs): cache_data['last_email_sent_time'] = timezone.now() cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) - send_email( - subject, - description, - instance.message, - recipients=[instance.recipient.email], - extra_context={ - 'call_to_action_url': target_url, - 'call_to_action_text': _('Find out more'), - }, - ) + + send_single_email(instance) + # flag as emailed instance.emailed = True # bulk_update is used to prevent emitting post_save signal diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py index c745f6ba..fae8de9e 100644 --- a/openwisp_notifications/tasks.py +++ b/openwisp_notifications/tasks.py @@ -3,6 +3,7 @@ from celery import shared_task from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType +from django.contrib.sites.models import Site from django.core.cache import cache from django.db.models import Q from django.db.utils import OperationalError @@ -10,6 +11,7 @@ from django.utils import timezone from django.utils.html import strip_tags +from openwisp_notifications import handlers from openwisp_notifications import settings as app_settings from openwisp_notifications import types from openwisp_notifications.swapper import load_model, swapper_load_model @@ -222,36 +224,56 @@ def batch_email_notification(email_id): return unsent_notifications = Notification.objects.filter(id__in=ids) - alerts = [] - - for notification in unsent_notifications: - url = notification.data.get('url', '') if notification.data else None - if url: - target_url = url - elif notification.target: - target_url = notification.redirect_view_url - else: - target_url = None - alerts.append( - { - 'level': notification.level, - 'message': notification.message, - 'description': notification.message, - 'timestamp': notification.timestamp, - 'url': target_url, + notifications_count = unsent_notifications.count() + current_site = Site.objects.get_current() + + if notifications_count == 1: + instance = unsent_notifications.first() + handlers.send_single_email(instance) + else: + alerts = [] + for notification in unsent_notifications: + url = notification.data.get('url', '') if notification.data else None + if url: + target_url = url + elif notification.target: + target_url = notification.redirect_view_url + else: + target_url = None + + description = notification.message if notifications_count <= 5 else None + alerts.append( + { + 'level': notification.level, + 'message': notification.message, + 'description': description, + 'timestamp': notification.timestamp, + 'url': target_url, + } + ) + + context = { + 'alerts': alerts, + 'notifications_count': notifications_count, + 'site_name': current_site.name, + 'site_domain': current_site.domain, + } + html_content = render_to_string('emails/batch_email.html', context) + plain_text_content = strip_tags(html_content) + + extra_context = {} + if notifications_count > 5: + extra_context = { + 'call_to_action_url': f"https://{current_site.domain}/admin", + 'call_to_action_text': 'View all Notifications', } - ) - context = { - 'alerts': alerts, - } - html_content = render_to_string('emails/batch_email.html', context) - plain_text_content = strip_tags(html_content) - send_email( - subject='Summary of Notifications', - body_text=plain_text_content, - body_html=html_content, - recipients=[email_id], - ) + send_email( + subject=f'Summary of {notifications_count} Notifications', + body_text=plain_text_content, + body_html=html_content, + recipients=[email_id], + extra_context=extra_context, + ) unsent_notifications.update(emailed=True) Notification.objects.bulk_update(unsent_notifications, ['emailed']) diff --git a/openwisp_notifications/templates/emails/batch_email.html b/openwisp_notifications/templates/emails/batch_email.html index 94ea79f9..80af47bb 100644 --- a/openwisp_notifications/templates/emails/batch_email.html +++ b/openwisp_notifications/templates/emails/batch_email.html @@ -1,99 +1,133 @@ {% block styles %} - + {% endblock styles %} {% block mail_body %} -
- {% for alert in alerts %} -
-

- {{ alert.level|upper }} - - {% if alert.url %} - {{ alert.message }} - {% else %} - {{ alert.message }} - {% endif %} - -

-

{{ alert.description }}
{{ alert.timestamp|date:"F j, Y, g:i a" }}

+
+
+

Summary of {{ notifications_count }} Notifications

+

From {{ site_name }}

+
+ {% for alert in alerts %} +
+

+ {{ alert.level|upper }} + + {% if alert.url %} + {{ alert.message }} + {% else %} + {{ alert.message }} + {% endif %} + +

+

{{ alert.timestamp|date:"F j, Y, g:i a" }}

+ {% if alert.description %} +

{{ alert.description }}

+ {% endif %} +
+ {% endfor %}
- {% endfor %} -
{% endblock mail_body %} From 233a1e71466fca117ea79c674ba7c64f51fe7ec1 Mon Sep 17 00:00:00 2001 From: Dhanus Date: Tue, 18 Jun 2024 01:26:41 +0530 Subject: [PATCH 06/13] [chore] Improvements --- README.rst | 2 +- openwisp_notifications/handlers.py | 54 ++++++++---------------------- openwisp_notifications/settings.py | 4 +-- openwisp_notifications/tasks.py | 9 ++--- openwisp_notifications/utils.py | 37 ++++++++++++++++++++ 5 files changed, 58 insertions(+), 48 deletions(-) diff --git a/README.rst b/README.rst index 4fc4a685..e208fb9d 100644 --- a/README.rst +++ b/README.rst @@ -1013,7 +1013,7 @@ The default configuration is as follows: 'max_allowed_backoff': 15, } -``OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL`` +``EMAIL_BATCH_INTERVAL`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +---------+-----------------------------------+ diff --git a/openwisp_notifications/handlers.py b/openwisp_notifications/handlers.py index fbec9a5a..a743f6df 100644 --- a/openwisp_notifications/handlers.py +++ b/openwisp_notifications/handlers.py @@ -10,7 +10,6 @@ from django.db.models.signals import post_delete, post_save from django.dispatch import receiver from django.utils import timezone -from django.utils.translation import gettext as _ from openwisp_notifications import settings as app_settings from openwisp_notifications import tasks @@ -20,13 +19,13 @@ NOTIFICATION_ASSOCIATED_MODELS, get_notification_configuration, ) +from openwisp_notifications.utils import send_notification_email from openwisp_notifications.websockets import handlers as ws_handlers -from openwisp_utils.admin_theme.email import send_email logger = logging.getLogger(__name__) EXTRA_DATA = app_settings.get_config()['USE_JSONFIELD'] -EMAIL_BATCH_INTERVAL = app_settings.OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL +EMAIL_BATCH_INTERVAL = app_settings.EMAIL_BATCH_INTERVAL User = get_user_model() @@ -165,36 +164,6 @@ def notify_handler(**kwargs): return notification_list -def send_single_email(instance): - try: - subject = instance.email_subject - except NotificationRenderException: - # Do not send email if notification is malformed. - return - url = instance.data.get('url', '') if instance.data else None - description = instance.message - if url: - target_url = url - elif instance.target: - target_url = instance.redirect_view_url - else: - target_url = None - if target_url: - description += _('\n\nFor more information see %(target_url)s.') % { - 'target_url': target_url - } - send_email( - subject, - description, - instance.message, - recipients=[instance.recipient.email], - extra_context={ - 'call_to_action_url': target_url, - 'call_to_action_text': _('Find out more'), - }, - ) - - @receiver(post_save, sender=Notification, dispatch_uid='send_email_notification') def send_email_notification(sender, instance, created, **kwargs): # Abort if a new notification is not created @@ -223,32 +192,37 @@ def send_email_notification(sender, instance, created, **kwargs): if not (email_preference and instance.recipient.email and email_verified): return - recipient_id = instance.recipient.id - cache_key = f'email_batch_{recipient_id}' - cache_key_pks = f'{instance.recipient.email}_pks' + recipient_email = instance.recipient.email + cache_key = f'email_batch_{recipient_email}' + cache_key_pks = f'{recipient_email}_batch_pks' cache_data = cache.get( cache_key, {'last_email_sent_time': None, 'batch_scheduled': False} ) if cache_data['last_email_sent_time'] and EMAIL_BATCH_INTERVAL > 0: + # Case 1: Batch email sending logic if not cache_data['batch_scheduled']: - # Schedule batch email notification task + # Schedule batch email notification task if not already scheduled tasks.batch_email_notification.apply_async( (instance.recipient.email,), countdown=EMAIL_BATCH_INTERVAL ) + # Mark batch as scheduled to prevent duplicate scheduling cache_data['batch_scheduled'] = True - cache.set(cache_key_pks, [instance.id], timeout=EMAIL_BATCH_INTERVAL * 2) + cache.set(cache_key_pks, [instance.id]) # Initialize list of IDs for batch cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) else: + # Add current instance ID to the list of IDs for batch ids = cache.get(cache_key_pks, []) ids.append(instance.id) - cache.set(cache_key_pks, ids, timeout=EMAIL_BATCH_INTERVAL * 2) + cache.set(cache_key_pks, ids) return + # Case 2: Single email sending logic + # Update the last email sent time and cache the data cache_data['last_email_sent_time'] = timezone.now() cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) - send_single_email(instance) + send_notification_email(instance) # flag as emailed instance.emailed = True diff --git a/openwisp_notifications/settings.py b/openwisp_notifications/settings.py index b685f511..fd67cb95 100644 --- a/openwisp_notifications/settings.py +++ b/openwisp_notifications/settings.py @@ -37,9 +37,7 @@ 'openwisp-notifications/audio/notification_bell.mp3', ) -OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr( - settings, 'EMAIL_BATCH_INTERVAL', 30 * 60 # 30 minutes -) +EMAIL_BATCH_INTERVAL = getattr(settings, 'EMAIL_BATCH_INTERVAL', 30 * 60) # 30 minutes # Remove the leading "/static/" here as it will # conflict with the "static()" call in context_processors.py. diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py index fae8de9e..231ee0b8 100644 --- a/openwisp_notifications/tasks.py +++ b/openwisp_notifications/tasks.py @@ -11,14 +11,14 @@ from django.utils import timezone from django.utils.html import strip_tags -from openwisp_notifications import handlers from openwisp_notifications import settings as app_settings from openwisp_notifications import types from openwisp_notifications.swapper import load_model, swapper_load_model +from openwisp_notifications.utils import send_notification_email from openwisp_utils.admin_theme.email import send_email from openwisp_utils.tasks import OpenwispCeleryTask -EMAIL_BATCH_INTERVAL = app_settings.OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL +EMAIL_BATCH_INTERVAL = app_settings.EMAIL_BATCH_INTERVAL User = get_user_model() @@ -218,7 +218,7 @@ def batch_email_notification(email_id): """ Sends a summary of notifications to the specified email address. """ - ids = cache.get(f'{email_id}_pks', []) + ids = cache.get(f'{email_id}_batch_pks', []) if not ids: return @@ -229,7 +229,7 @@ def batch_email_notification(email_id): if notifications_count == 1: instance = unsent_notifications.first() - handlers.send_single_email(instance) + send_notification_email(instance) else: alerts = [] for notification in unsent_notifications: @@ -277,3 +277,4 @@ def batch_email_notification(email_id): ) unsent_notifications.update(emailed=True) Notification.objects.bulk_update(unsent_notifications, ['emailed']) + cache.delete(f'{email_id}_pks') diff --git a/openwisp_notifications/utils.py b/openwisp_notifications/utils.py index 1edecde7..93a541de 100644 --- a/openwisp_notifications/utils.py +++ b/openwisp_notifications/utils.py @@ -1,6 +1,10 @@ from django.conf import settings from django.contrib.sites.models import Site from django.urls import NoReverseMatch, reverse +from django.utils.translation import gettext as _ + +from openwisp_notifications.exceptions import NotificationRenderException +from openwisp_utils.admin_theme.email import send_email def _get_object_link(obj, field, absolute_url=False, *args, **kwargs): @@ -28,3 +32,36 @@ def normalize_unread_count(unread_count): return '99+' else: return unread_count + + +def send_notification_email(instance): + + """Send a single email notification""" + + try: + subject = instance.email_subject + except NotificationRenderException: + # Do not send email if notification is malformed. + return + url = instance.data.get('url', '') if instance.data else None + description = instance.message + if url: + target_url = url + elif instance.target: + target_url = instance.redirect_view_url + else: + target_url = None + if target_url: + description += _('\n\nFor more information see %(target_url)s.') % { + 'target_url': target_url + } + send_email( + subject, + description, + instance.message, + recipients=[instance.recipient.email], + extra_context={ + 'call_to_action_url': target_url, + 'call_to_action_text': _('Find out more'), + }, + ) From 69365bb45f349479ef0dbb5feaef228e3302fdb4 Mon Sep 17 00:00:00 2001 From: Dhanus Date: Tue, 18 Jun 2024 01:31:12 +0530 Subject: [PATCH 07/13] [chore] Fix cache delete --- openwisp_notifications/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py index 231ee0b8..909a67e9 100644 --- a/openwisp_notifications/tasks.py +++ b/openwisp_notifications/tasks.py @@ -277,4 +277,4 @@ def batch_email_notification(email_id): ) unsent_notifications.update(emailed=True) Notification.objects.bulk_update(unsent_notifications, ['emailed']) - cache.delete(f'{email_id}_pks') + cache.delete(f'{email_id}_batch_pks') From 9d38db179092e2ad1e7db3f0d5ff2babe0f0ae00 Mon Sep 17 00:00:00 2001 From: Dhanus Date: Wed, 19 Jun 2024 02:03:17 +0530 Subject: [PATCH 08/13] [chore] New changes --- README.rst | 2 +- openwisp_notifications/base/models.py | 2 +- openwisp_notifications/handlers.py | 27 ++++---- openwisp_notifications/settings.py | 4 +- openwisp_notifications/tasks.py | 52 +++++++--------- .../templates/emails/batch_email.html | 62 ++++++------------- .../templates/emails/batch_email.txt | 15 +++++ .../tests/test_notifications.py | 9 ++- 8 files changed, 85 insertions(+), 88 deletions(-) create mode 100644 openwisp_notifications/templates/emails/batch_email.txt diff --git a/README.rst b/README.rst index e208fb9d..4fc4a685 100644 --- a/README.rst +++ b/README.rst @@ -1013,7 +1013,7 @@ The default configuration is as follows: 'max_allowed_backoff': 15, } -``EMAIL_BATCH_INTERVAL`` +``OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +---------+-----------------------------------+ diff --git a/openwisp_notifications/base/models.py b/openwisp_notifications/base/models.py index 56bb0f76..75b5ea4b 100644 --- a/openwisp_notifications/base/models.py +++ b/openwisp_notifications/base/models.py @@ -139,7 +139,7 @@ def message(self): @cached_property def rendered_description(self): if not self.description: - return + return '' with notification_render_attributes(self): data = self.data or {} desc = self.description.format(notification=self, **data) diff --git a/openwisp_notifications/handlers.py b/openwisp_notifications/handlers.py index a743f6df..b4312dae 100644 --- a/openwisp_notifications/handlers.py +++ b/openwisp_notifications/handlers.py @@ -25,7 +25,7 @@ logger = logging.getLogger(__name__) EXTRA_DATA = app_settings.get_config()['USE_JSONFIELD'] -EMAIL_BATCH_INTERVAL = app_settings.EMAIL_BATCH_INTERVAL +EMAIL_BATCH_INTERVAL = app_settings.OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL User = get_user_model() @@ -192,11 +192,17 @@ def send_email_notification(sender, instance, created, **kwargs): if not (email_preference and instance.recipient.email and email_verified): return - recipient_email = instance.recipient.email - cache_key = f'email_batch_{recipient_email}' - cache_key_pks = f'{recipient_email}_batch_pks' + recipient_id = instance.recipient.id + cache_key = f'email_batch_{recipient_id}' + cache_data = cache.get( - cache_key, {'last_email_sent_time': None, 'batch_scheduled': False} + cache_key, + { + 'last_email_sent_time': None, + 'batch_scheduled': False, + 'pks': [], + 'email_id': instance.recipient.email, + }, ) if cache_data['last_email_sent_time'] and EMAIL_BATCH_INTERVAL > 0: @@ -204,17 +210,16 @@ def send_email_notification(sender, instance, created, **kwargs): if not cache_data['batch_scheduled']: # Schedule batch email notification task if not already scheduled tasks.batch_email_notification.apply_async( - (instance.recipient.email,), countdown=EMAIL_BATCH_INTERVAL + (instance.recipient.id,), countdown=EMAIL_BATCH_INTERVAL ) # Mark batch as scheduled to prevent duplicate scheduling cache_data['batch_scheduled'] = True - cache.set(cache_key_pks, [instance.id]) # Initialize list of IDs for batch - cache.set(cache_key, cache_data, timeout=EMAIL_BATCH_INTERVAL) + cache_data['pks'] = [instance.id] + cache.set(cache_key, cache_data) else: # Add current instance ID to the list of IDs for batch - ids = cache.get(cache_key_pks, []) - ids.append(instance.id) - cache.set(cache_key_pks, ids) + cache_data['pks'].append(instance.id) + cache.set(cache_key, cache_data) return # Case 2: Single email sending logic diff --git a/openwisp_notifications/settings.py b/openwisp_notifications/settings.py index fd67cb95..d26cc75b 100644 --- a/openwisp_notifications/settings.py +++ b/openwisp_notifications/settings.py @@ -37,7 +37,9 @@ 'openwisp-notifications/audio/notification_bell.mp3', ) -EMAIL_BATCH_INTERVAL = getattr(settings, 'EMAIL_BATCH_INTERVAL', 30 * 60) # 30 minutes +OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr( + settings, 'OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL', 30 * 60 # 30 minutes +) # Remove the leading "/static/" here as it will # conflict with the "static()" call in context_processors.py. diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py index 909a67e9..6d3eb0bb 100644 --- a/openwisp_notifications/tasks.py +++ b/openwisp_notifications/tasks.py @@ -9,7 +9,6 @@ from django.db.utils import OperationalError from django.template.loader import render_to_string from django.utils import timezone -from django.utils.html import strip_tags from openwisp_notifications import settings as app_settings from openwisp_notifications import types @@ -18,7 +17,7 @@ from openwisp_utils.admin_theme.email import send_email from openwisp_utils.tasks import OpenwispCeleryTask -EMAIL_BATCH_INTERVAL = app_settings.EMAIL_BATCH_INTERVAL +EMAIL_BATCH_INTERVAL = app_settings.OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL User = get_user_model() @@ -214,52 +213,48 @@ def delete_ignore_object_notification(instance_id): @shared_task(base=OpenwispCeleryTask) -def batch_email_notification(email_id): +def batch_email_notification(instance_id): """ Sends a summary of notifications to the specified email address. """ - ids = cache.get(f'{email_id}_batch_pks', []) + if not instance_id: + return + + cache_key = f'email_batch_{instance_id}' + cache_data = cache.get(cache_key, {'pks': []}) - if not ids: + if not cache_data['pks']: return - unsent_notifications = Notification.objects.filter(id__in=ids) + unsent_notifications = Notification.objects.filter(id__in=cache_data['pks']) notifications_count = unsent_notifications.count() current_site = Site.objects.get_current() + email_id = cache_data.get('email_id') + # Send individual email if there is only one notification if notifications_count == 1: - instance = unsent_notifications.first() - send_notification_email(instance) + notification = unsent_notifications.first() + send_notification_email(notification) else: - alerts = [] + # Show notification description upto 5 notifications + show_notification_description = notifications_count <= 5 for notification in unsent_notifications: url = notification.data.get('url', '') if notification.data else None if url: - target_url = url + notification.url = url elif notification.target: - target_url = notification.redirect_view_url + notification.url = notification.redirect_view_url else: - target_url = None - - description = notification.message if notifications_count <= 5 else None - alerts.append( - { - 'level': notification.level, - 'message': notification.message, - 'description': description, - 'timestamp': notification.timestamp, - 'url': target_url, - } - ) + notification.url = None context = { - 'alerts': alerts, + 'notifications': unsent_notifications, 'notifications_count': notifications_count, + 'show_notification_description': show_notification_description, 'site_name': current_site.name, - 'site_domain': current_site.domain, } html_content = render_to_string('emails/batch_email.html', context) - plain_text_content = strip_tags(html_content) + plain_text_content = render_to_string('emails/batch_email.txt', context) extra_context = {} if notifications_count > 5: @@ -269,12 +264,13 @@ def batch_email_notification(email_id): } send_email( - subject=f'Summary of {notifications_count} Notifications', + subject=f'Summary of {notifications_count} Notifications from {current_site.name}', body_text=plain_text_content, body_html=html_content, recipients=[email_id], extra_context=extra_context, ) + unsent_notifications.update(emailed=True) Notification.objects.bulk_update(unsent_notifications, ['emailed']) - cache.delete(f'{email_id}_batch_pks') + cache.delete(cache_key) diff --git a/openwisp_notifications/templates/emails/batch_email.html b/openwisp_notifications/templates/emails/batch_email.html index 80af47bb..46bdc630 100644 --- a/openwisp_notifications/templates/emails/batch_email.html +++ b/openwisp_notifications/templates/emails/batch_email.html @@ -1,27 +1,5 @@ {% block styles %}