[feat] Batch email notifications #276

Dhanus3133 · 2024-06-04T07:52:31Z

Fixes #132

pandafy

@Dhanus3133 I have left some suggestions. I hope these solve your queries.

openwisp_notifications/settings.py

openwisp_notifications/handlers.py

openwisp_notifications/tasks.py

openwisp_notifications/handlers.py

nemesifier

@Dhanus3133 please start writin a short README section that explains how this feature works and add the new setting, remember to add a screenshot (push it to the docs branch).

@pandafy you know what would be handy?

Allow to change the batching using an organization setting that allows user to change this per org via the admin
Allow to specify whether the system should start batching right away instead of waiting 1 email, this could be disabled by default, and could also be available from the organization page

openwisp_notifications/handlers.py

nemesifier · 2024-06-07T19:58:42Z

openwisp_notifications/tests/test_notifications.py

+
+        # 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")


I'd try to explore the possibility of using freezegun to simulate the passing of time and ensure that a new notification sent after the batch interval is not batched.

Dhanus3133 · 2024-06-11T18:15:36Z

In our weekly call, @nemesifier and @pandafy suggested some improvements.

The date should be displayed below the title.
Update the email title with the number of notifications and site name.
Some changes have to be made regarding the email template
1. If there is only one notification in the batch, just use a single mail template.
2. Can follow the current template if there are 2 - 5 notifications.
3. If there are more than 5 emails, a new template can be used where only the level, message, and date can be displayed. Along with a ‘View all new notifications’ link to the admin site.

pandafy

This is looking good. I have done a quick code review and left some suggestion on improving the code.

I will do a functional test tomorrow and report if I find something to improve.

openwisp_notifications/settings.py

openwisp_notifications/handlers.py

pandafy · 2024-06-17T19:12:09Z

openwisp_notifications/settings.py

@@ -37,6 +37,10 @@
    'openwisp-notifications/audio/notification_bell.mp3',
 )

+OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr(
+    settings, 'EMAIL_BATCH_INTERVAL', 30 * 60  # 30 minutes


Thinking out loud

I think 30 minutes is a lot of time. It will affect the real-timeness of the email notifications. Most of the consecutive notifications would be delayed by this time. We should also point this out in the README.

@nemesifier what do you think?

pandafy

Immediate email after batch email

If a notification is created just after the batch email is processed, then the email for this notification is sent immediately after the batch email.

Is this a desired behaviour? Can you please add a test for this?

Duplicate subject in email

The send_email function adds a subject in the email which servers as a heading for all the email communication.

I think, we should keep using the subject provided by the send_email function for consistency.

openwisp_notifications/settings.py

README.rst

openwisp_notifications/templates/emails/batch_email.html

openwisp_notifications/tasks.py

openwisp_notifications/utils.py

openwisp_notifications/tests/test_notifications.py

pandafy

@Dhanus3133 the test is failing because the notification email contains direct URL to the target object while the test expects redirect url

-<div class="msg"><p>Default notification with default verb and level info by <a href="https://example.com/api/v1/notifications/notification/2632e308-0f2d-43a1-8617-7350e48b8fd3/redirect/">Tester Tester (test org)</a></p></div>
+<div class="msg"><p>Default notification with default verb and level info by <a href="https://example.com/admin/openwisp_users/organizationuser/dea5c3cc-8c2a-4021-94a7-9cc36885f9ff/change/">Tester Tester (test org)</a></p></div>

nemesifier

Great work @Dhanus3133 @pandafy, see my comments below.

nemesifier · 2024-07-02T21:17:36Z

README.rst

@@ -1013,6 +1013,19 @@ The default configuration is as follows:
        'max_allowed_backoff': 15,
    }

+``OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


This seems longer than it should

nemesifier · 2024-07-02T21:20:37Z

openwisp_notifications/handlers.py

+    # 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)


if EMAIL_BATCH_INTERVAL is zero, wouldn't this data be cached indefinitely? If that's the case, it's a problem.

nemesifier · 2024-07-02T21:22:44Z

openwisp_notifications/tasks.py

+
+
+@shared_task(base=OpenwispCeleryTask)
+def batch_email_notification(instance_id):


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 send_batched_email_notifications or something similar.

nemesifier · 2024-07-02T21:24:09Z

openwisp_notifications/settings.py

@@ -37,6 +37,10 @@
    'openwisp-notifications/audio/notification_bell.mp3',
 )

+OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr(


Let's shorten this, we usually do this in most modules, eg:

Suggested change

OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr(

EMAIL_BATCH_INTERVAL = getattr(

nemesifier · 2024-07-02T21:25:20Z

openwisp_notifications/tasks.py

+        plain_text_content = render_to_string('emails/batch_email.txt', context)
+
+        extra_context = {}
+        if notifications_count > 5:


are we showing only 5 notifications? Let's show more and make this configurable with a setting, eg: OPENWISP_NOTIFICATIONS_EMAIL_BATCH_DISPLAY_LIMIT (add to README). I think we can default this to 15.

nemesifier · 2024-07-02T21:34:39Z

openwisp_notifications/templates/emails/batch_email.txt

@@ -0,0 +1,15 @@
+Summary of {{ notifications_count }} Notifications from {{ site_name }}


Flag all strings as translatable using translate and blocktranslate: https://docs.djangoproject.com/en/5.0/topics/i18n/translation/#internationalization-in-template-code

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>".

nemesifier · 2024-07-02T21:35:18Z

openwisp_notifications/utils.py

+
+
+def send_notification_email(notification):
+


can we remove this blank line or does black complain about it?

nemesifier · 2024-07-02T21:36:26Z

openwisp_notifications/tasks.py

+        notification = unsent_notifications.first()
+        send_notification_email(notification)
+    else:
+        # Show notification description upto 5 notifications


Suggested change

# Show notification description upto 5 notifications

# Show the amount of notifications according to configured display limit

nemesifier · 2024-07-02T21:36:36Z

openwisp_notifications/tasks.py

+        send_notification_email(notification)
+    else:
+        # Show notification description upto 5 notifications
+        show_notification_description = notifications_count <= 5


use setting instead of hardcoding 5

nemesifier · 2024-07-02T21:38:39Z

openwisp_notifications/tests/test_notifications.py

+        self.assertIn('Summary of 10 Notifications', mail.outbox[1].subject)
+        self.assertIn('View all Notifications', mail.outbox[1].body)
+        self.assertNotIn('Test Notification', mail.outbox[1].body)
+


Can we add another test in which this feature is disabled and ensure that no batching happens in that case?
Using the override_settings from django or mocking the setting should work.

[chore] Basic implementation

2c780c8

pandafy requested changes Jun 4, 2024

View reviewed changes

[chore] Updated changes

949ea24

Dhanus3133 marked this pull request as ready for review June 6, 2024 14:56

Dhanus3133 added 2 commits June 8, 2024 00:31

[chore] Add tests

305d6c8

Merge branch 'master' into feat/batch-email

0e54696

nemesifier reviewed Jun 7, 2024

View reviewed changes

[chore] Update Readme

58bdc9d

[chore] New changes

4abc717

Dhanus3133 force-pushed the feat/batch-email branch from 7736e9d to 4abc717 Compare June 15, 2024 10:31

Merge branch 'master' into feat/batch-email

caf3031

pandafy requested changes Jun 17, 2024

View reviewed changes

Dhanus3133 added 2 commits June 18, 2024 01:26

[chore] Improvements

233a1e7

[chore] Fix cache delete

69365bb

pandafy requested changes Jun 18, 2024

View reviewed changes

Dhanus3133 added 3 commits June 19, 2024 02:03

[chore] New changes

9d38db1

[chore] Updates

01182c4

[chore] Add extra tests

483df00

pandafy requested changes Jun 20, 2024

View reviewed changes

[fix] Tests

68a1604

Dhanus3133 changed the base branch from master to gsoc24 June 23, 2024 13:30

nemesifier requested changes Jul 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Batch email notifications #276

[feat] Batch email notifications #276

Dhanus3133 commented Jun 4, 2024

pandafy left a comment

nemesifier left a comment •

edited

Loading

nemesifier Jun 7, 2024

Dhanus3133 commented Jun 11, 2024

pandafy left a comment •

edited

Loading

pandafy Jun 17, 2024

pandafy left a comment

pandafy left a comment

nemesifier left a comment

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024 •

edited

Loading

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024

nemesifier Jul 2, 2024



		@shared_task(base=OpenwispCeleryTask)
		def batch_email_notification(instance_id):

	OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr(
	EMAIL_BATCH_INTERVAL = getattr(

		@@ -0,0 +1,15 @@
		Summary of {{ notifications_count }} Notifications from {{ site_name }}

	# Show notification description upto 5 notifications
	# Show the amount of notifications according to configured display limit

[feat] Batch email notifications #276

Are you sure you want to change the base?

[feat] Batch email notifications #276

Conversation

Dhanus3133 commented Jun 4, 2024

pandafy left a comment

Choose a reason for hiding this comment

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dhanus3133 commented Jun 11, 2024

pandafy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandafy left a comment

Choose a reason for hiding this comment

Immediate email after batch email

Duplicate subject in email

pandafy left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment •

edited

Loading

pandafy left a comment •

edited

Loading

nemesifier Jul 2, 2024 •

edited

Loading