[feature] Batch email notifications

[Dhanus3133]

Implements and closes #132.

[pandafy]

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

[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

[nemesifier]

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]

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

1. The date should be displayed below the title.
2. Update the email title with the number of notifications and site name.
3. 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.

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

[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]

One thing I had asked before, but I am not sure I put in the review, I think we should add a way to open the web UI of openwisp and trigger the opening of the notification widget automatically, so that we can use that in the email: after clicking on the button to see all the remaining notifications, the user is taken to the application and the notification widget opens automatically to show the notifications.

[nemesifier]

@Dhanus3133 I sent a fix for the test, see my comments below to understand why these changes were needed.

[nemesifier]

LGTM, I think we can merge.

[pandafy]

I created 40 notifications and received this email.

Weren't we supposed to show only limited notifications in the email?

[nemesifier]

We found a few more problems.

[pandafy]

diff --git a/openwisp_notifications/tasks.py b/openwisp_notifications/tasks.py
index cc44d9b..1a71168 100644
--- a/openwisp_notifications/tasks.py
+++ b/openwisp_notifications/tasks.py
@@ -266,15 +266,15 @@ def send_batched_email_notifications(instance_id):
             'site_name': current_site.name,
             'start_time': starting_time,
         }
-        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 > display_limit:
             extra_context = {
                 'call_to_action_url': f"https://{current_site.domain}/admin/#notifications",
-                'call_to_action_text': _('View all Notifications'),
+                'call_to_action_text': _('View all notifications'),
             }
+        context.update(extra_context)
+        html_content = render_to_string('emails/batch_email.html', context)
+        plain_text_content = render_to_string('emails/batch_email.txt', context)

         send_email(
             subject=f'[{current_site.name}] {notifications_count} new notifications since {starting_time}',
diff --git a/openwisp_notifications/templates/emails/batch_email.txt b/openwisp_notifications/templates/emails/batch_email.txt
index 20338cb..3eca567 100644
--- a/openwisp_notifications/templates/emails/batch_email.txt
+++ b/openwisp_notifications/templates/emails/batch_email.txt
@@ -12,5 +12,5 @@
 {% endfor %}

 {% if not show_notification_description  %}
-{% translate "View all Notifications" %}: {{ call_to_action_url }}
+{{ call_to_action_text }}: {{ call_to_action_url }}
 {% endif %}

[nemesifier]

Whitespace looks wrong, please change the template until it looks like the following:

- Default notification with default verb and level info by default
  Date & Time: July 17, 2024, 6:10 p.m.
  URL: http://example.com/api/v1/notifications/notification/9dbf11f7-f3ef-43d2-9a72-a678f2150c84/redirect/

- Default notification with default verb and level info by default
  Date & Time: July 17, 2024, 6:10 p.m.
  URL: http://example.com/api/v1/notifications/notification/fbc5874a-d852-4384-94f2-43280841548e/redirect/

There should be a blank line only to separate one notification from the other, make sure the indentation is consistent.

[nemesifier]

I tested the generated HTML and found the following issue:

<p>
<a href="http://example.com/api/v1/notifications/notification/9f244db8-1897-45ed-9f6c-622d45816f82/redirect/" target="_blank">Default notification with default verb and level info by</a>
<a href="http://example.com/admin/openwisp_users/organization/d811aa06-8020-4d32-bdea-fcbd66e96574/change/">default</a>
</p>

The second link goes straight to the object without passing through the notification API.

@pandafy wasn't this something that we solved recently?

[pandafy]

The last assertion in this test looks bogus. It should have caught this issue

openwisp-notifications/openwisp_notifications/tests/test_notifications.py

Lines 313 to 345 in 3b0fc9d

	def test_default_notification_type(self):
	self.notification_options.pop('verb')
	self.notification_options.pop('url')
	self.notification_options.update(
	{'type': 'default', 'target': self._get_org_user()}
	)
	self._create_notification()
	n = notification_queryset.first()
	self.assertEqual(n.level, 'info')
	self.assertEqual(n.verb, 'default verb')
	self.assertIn(
	'Default notification with default verb and level info by', n.message
	)
	self.assertEqual(n.email_subject, '[example.com] Default Notification Subject')
	email = mail.outbox.pop()
	html_email = email.alternatives[0][0]
	self.assertEqual(
	email.body,
	(
	'Default notification with default verb and'
	' level info by Tester Tester (test org)\n\n'
	f'For more information see {n.redirect_view_url}.'
	),
	)
	self.assertIn(
	(
	'<div class="msg"><p>Default notification with'
	' default verb and level info by'
	f' <a href="{n.redirect_view_url}">'
	'Tester Tester (test org)</a></p></div>'
	),
	html_email,
	)