-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat] Batch email notifications #276
base: gsoc24
Are you sure you want to change the base?
Changes from all commits
2c780c8
949ea24
305d6c8
0e54696
58bdc9d
4abc717
caf3031
233a1e7
69365bb
9d38db1
01182c4
483df00
68a1604
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,12 +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 | ||
Dhanus3133 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
User = get_user_model() | ||
|
||
|
@@ -192,35 +192,43 @@ 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 | ||
body_text = instance.email_message | ||
if url: | ||
target_url = url | ||
elif instance.target: | ||
target_url = instance.redirect_view_url | ||
else: | ||
target_url = None | ||
if target_url: | ||
body_text += _('\n\nFor more information see %(target_url)s.') % { | ||
'target_url': target_url | ||
} | ||
|
||
send_email( | ||
subject=subject, | ||
body_text=body_text, | ||
body_html=instance.email_message, | ||
recipients=[instance.recipient.email], | ||
extra_context={ | ||
'call_to_action_url': target_url, | ||
'call_to_action_text': _('Find out more'), | ||
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, | ||
'pks': [], | ||
'email_id': instance.recipient.email, | ||
}, | ||
) | ||
|
||
if cache_data['last_email_sent_time'] and EMAIL_BATCH_INTERVAL > 0: | ||
Dhanus3133 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Case 1: Batch email sending logic | ||
if not cache_data['batch_scheduled']: | ||
# Schedule batch email notification task if not already scheduled | ||
tasks.batch_email_notification.apply_async( | ||
(instance.recipient.id,), countdown=EMAIL_BATCH_INTERVAL | ||
) | ||
# Mark batch as scheduled to prevent duplicate scheduling | ||
cache_data['batch_scheduled'] = True | ||
cache_data['pks'] = [instance.id] | ||
cache.set(cache_key, cache_data) | ||
else: | ||
# Add current instance ID to the list of IDs for batch | ||
cache_data['pks'].append(instance.id) | ||
cache.set(cache_key, cache_data) | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if |
||
|
||
send_notification_email(instance) | ||
|
||
# flag as emailed | ||
instance.emailed = True | ||
# bulk_update is used to prevent emitting post_save signal | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -37,6 +37,10 @@ | |||||
'openwisp-notifications/audio/notification_bell.mp3', | ||||||
) | ||||||
|
||||||
OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr( | ||||||
Dhanus3133 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's shorten this, we usually do this in most modules, eg:
Suggested change
|
||||||
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. | ||||||
# This is done for backward compatibility. | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,14 +3,22 @@ | |||||
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 | ||||||
from django.template.loader import render_to_string | ||||||
from django.utils import timezone | ||||||
|
||||||
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 | ||||||
|
||||||
User = get_user_model() | ||||||
|
||||||
Notification = load_model('Notification') | ||||||
|
@@ -202,3 +210,67 @@ 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): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not convinced of the name of this function here. Does this function batch emails, or does it send batched emails? From the name it seems it does the former, but from the code it seems to me it does the latter. Please clarify. If it's the latter, we should rename it to something like |
||||||
""" | ||||||
Sends a summary of notifications to the specified email address. | ||||||
""" | ||||||
if not instance_id: | ||||||
return | ||||||
|
||||||
cache_key = f'email_batch_{instance_id}' | ||||||
cache_data = cache.get(cache_key, {'pks': []}) | ||||||
|
||||||
if not cache_data['pks']: | ||||||
return | ||||||
|
||||||
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: | ||||||
notification = unsent_notifications.first() | ||||||
send_notification_email(notification) | ||||||
else: | ||||||
# Show notification description upto 5 notifications | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
show_notification_description = notifications_count <= 5 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use setting instead of hardcoding 5 |
||||||
for notification in unsent_notifications: | ||||||
url = notification.data.get('url', '') if notification.data else None | ||||||
if url: | ||||||
notification.url = url | ||||||
elif notification.target: | ||||||
notification.url = notification.redirect_view_url | ||||||
else: | ||||||
notification.url = None | ||||||
|
||||||
context = { | ||||||
'notifications': unsent_notifications, | ||||||
'notifications_count': notifications_count, | ||||||
'show_notification_description': show_notification_description, | ||||||
'site_name': current_site.name, | ||||||
} | ||||||
html_content = render_to_string('emails/batch_email.html', context) | ||||||
plain_text_content = render_to_string('emails/batch_email.txt', context) | ||||||
|
||||||
extra_context = {} | ||||||
if notifications_count > 5: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we showing only 5 notifications? Let's show more and make this configurable with a setting, eg: |
||||||
extra_context = { | ||||||
'call_to_action_url': f"https://{current_site.domain}/admin", | ||||||
'call_to_action_text': 'View all Notifications', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Always flag strings as translatable:
Suggested change
|
||||||
} | ||||||
|
||||||
send_email( | ||||||
subject=f'Summary of {notifications_count} Notifications from {current_site.name}', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here.
|
||||||
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(cache_key) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
{% block styles %} | ||
<style type="text/css"> | ||
.alert { | ||
border: 1px solid #e0e0e0; | ||
border-radius: 5px; | ||
margin-bottom: 10px; | ||
padding: 10px; | ||
} | ||
.alert.error { | ||
background-color: #ffefef; | ||
} | ||
.alert.info { | ||
background-color: #f0f0f0; | ||
} | ||
.alert.success { | ||
background-color: #e6f9e8; | ||
} | ||
.alert h2 { | ||
margin: 0 0 5px 0; | ||
font-size: 16px; | ||
} | ||
.alert h2 .title { | ||
display: inline-block; | ||
max-width: 80%; | ||
white-space: nowrap; | ||
overflow: hidden; | ||
text-overflow: ellipsis; | ||
vertical-align: middle; | ||
} | ||
.alert.error h2 { | ||
color: #d9534f; | ||
} | ||
.alert.info h2 { | ||
color: #333333; | ||
} | ||
.alert.success h2 { | ||
color: #1c8828; | ||
} | ||
.alert p { | ||
margin: 0; | ||
font-size: 14px; | ||
color: #666; | ||
} | ||
.alert .title p { | ||
display: inline; | ||
overflow: hidden; | ||
text-overflow: ellipsis; | ||
} | ||
.badge { | ||
display: inline-block; | ||
padding: 2px 8px; | ||
border-radius: 3px; | ||
font-size: 12px; | ||
font-weight: bold; | ||
text-transform: uppercase; | ||
margin-right: 5px; | ||
color: white; | ||
} | ||
.badge.error { | ||
background-color: #d9534f; | ||
} | ||
.badge.info { | ||
background-color: #333333; | ||
} | ||
.badge.success { | ||
background-color: #1c8828; | ||
} | ||
.alert a { | ||
text-decoration: none; | ||
} | ||
.alert.error a { | ||
color: #d9534f; | ||
} | ||
.alert.info a { | ||
color: #333333; | ||
} | ||
.alert.success a { | ||
color: #1c8828; | ||
} | ||
.alert a:hover { | ||
text-decoration: underline; | ||
} | ||
</style> | ||
{% endblock styles %} | ||
|
||
{% block mail_body %} | ||
<div> | ||
{% for notification in notifications %} | ||
<div class="alert {{ notification.level }}"> | ||
<h2> | ||
<span class="badge {{ notification.level }}">{{ notification.level|upper }}</span> | ||
<span class="title"> | ||
{% if notification.url %} | ||
<a href="{{ notification.url }}" target="_blank">{{ notification.message }}</a> | ||
{% else %} | ||
{{ notification.message }} | ||
{% endif %} | ||
</span> | ||
</h2> | ||
<p>{{ notification.timestamp|date:"F j, Y, g:i a" }}</p> | ||
{% if show_notification_description %} | ||
<p>{{ notification.rendered_description|safe }}</p> | ||
{% endif %} | ||
</div> | ||
{% endfor %} | ||
</div> | ||
{% endblock mail_body %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
Summary of {{ notifications_count }} Notifications from {{ site_name }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flag all strings as translatable using Also here let's change this sentence as for the change I recommended in the subject, let's maintain consistency, eg: "{notifications_count} new notifications since <UTC_DATE_AND_TIME>". |
||
|
||
{% for notification in notifications %} | ||
- {{ notification.message }} | ||
URL: {{ notification.url }} | ||
Timestamp: {{ notification.timestamp|date:"F j, Y, g:i a" }} | ||
{% if show_notification_description %} | ||
Description: {{ notification.rendered_description }} | ||
{% endif %} | ||
{% endfor %} | ||
|
||
{% if notifications_count > 5 %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the setting here instead of hardcoding 5 |
||
View all Notifications: {{ call_to_action_url }} | ||
{% endif %} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems longer than it should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that it would be okay since we already use this setting
OPENWISP_NOTIFICATIONS_POPULATE_PREFERENCES_ON_MIGRATE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Federico was pointing to the heading underline. The length of the underline should be equal to the length of the text.