Send notifications to active users only

CHANGELOG.txt

@@ -37,6 +37,7 @@ Changelog
  * Fix: Ensure comment notifications dropdown handles longer translations without overflowing content (Krzysztof Jeziorny)
  * Fix: Set `default_auto_field` in `postgres_search` `AppConfig` (Nick Moreton)
  * Fix: Ensure admin tab JS events are handled on page load (Andrew Stone)
+ * Fix: `EmailNotificationMixin` and `send_notification` should only send emails to active users (Bryan Williams)
 
 
 2.14.1 (12.08.2021)

CONTRIBUTORS.rst

@@ -538,6 +538,7 @@ Contributors
 * Desai Akshata
 * Krzysztof Jeziorny
 * Nick Moreton
+* Bryan Williams
 
 Translators
 ===========

docs/releases/2.15.rst

@@ -52,6 +52,7 @@ Bug fixes
  * Ensure comment notifications dropdown handles longer translations without overflowing content (Krzysztof Jeziorny)
  * Set ``default_auto_field`` in ``postgres_search`` ``AppConfig`` (Nick Moreton)
  * Ensure admin tab JS events are handled on page load (Andrew Stone)
+ * `EmailNotificationMixin` and `send_notification` should only send emails to active users (Bryan Williams)
 
 Upgrade considerations
 ======================

wagtail/admin/mail.py

@@ -84,7 +84,7 @@ def send_notification(recipient_users, notification, extra_context):
     # Get list of email addresses
     email_recipients = [
         recipient for recipient in recipient_users
-        if recipient.email and getattr(
+        if recipient.is_active and recipient.email and getattr(
             UserProfile.get_for_user(recipient),
             notification + '_notifications'
         )
@@ -206,11 +206,13 @@ class EmailNotificationMixin:
     def get_valid_recipients(self, instance, **kwargs):
         """Filters notification recipients to those allowing the notification type on their UserProfile, and those
         with an email address"""
-
-        return {recipient for recipient in self.get_recipient_users(instance, **kwargs) if recipient.email and getattr(
-            UserProfile.get_for_user(recipient),
-            self.notification + '_notifications'
-        )}
+        return {
+            recipient for recipient in self.get_recipient_users(instance, **kwargs)
+            if recipient.is_active and recipient.email and getattr(
+                UserProfile.get_for_user(recipient),
+                self.notification + '_notifications'
+            )
+        }
 
     def get_template_set(self, instance, **kwargs):
         """Return a dictionary of template paths for the templates for the email subject and the text and html

wagtail/admin/tests/pages/test_edit_page.py

@@ -2564,3 +2564,39 @@ class TestCommenting(TestCase, WagtailTestUtils):
 
         # This time, no emails should be submitted because the only subscriber has disabled these emails globally
         self.assertEqual(len(mail.outbox), 0)
+
+    def test_updated_comments_notifications_active_users_only(self):
+        # subscriber is inactive
+        self.subscriber.is_active = False
+        self.subscriber.save()
+
+        post_data = {
+            'title': "I've been edited!",
+            'content': "Some content",
+            'slug': 'hello-world',
+            'comments-TOTAL_FORMS': '1',
+            'comments-INITIAL_FORMS': '0',
+            'comments-MIN_NUM_FORMS': '0',
+            'comments-MAX_NUM_FORMS': '',
+            'comments-0-DELETE': '',
+            'comments-0-resolved': '',
+            'comments-0-id': '',
+            'comments-0-contentpath': 'title',
+            'comments-0-text': 'A test comment',
+            'comments-0-position': '',
+            'comments-0-replies-TOTAL_FORMS': '0',
+            'comments-0-replies-INITIAL_FORMS': '0',
+            'comments-0-replies-MIN_NUM_FORMS': '0',
+            'comments-0-replies-MAX_NUM_FORMS': '0'
+        }
+
+        response = self.client.post(reverse('wagtailadmin_pages:edit', args=[self.child_page.id]), post_data)
+
+        self.assertRedirects(response, reverse('wagtailadmin_pages:edit', args=[self.child_page.id]))
+
+        # Check the comment was added
+        comment = self.child_page.comments.get()
+        self.assertEqual(comment.text, 'A test comment')
+
+        # No emails should be submitted because subscriber is inactive
+        self.assertEqual(len(mail.outbox), 0)

wagtail/admin/tests/test_workflows.py

@@ -1317,6 +1317,33 @@ class TestNotificationPreferences(TestCase, WagtailTestUtils):
         # with `WAGTAILADMIN_NOTIFICATION_INCLUDE_SUPERUSERS` off, the superuser should not get a workflow email
         self.assertNotIn(self.superuser.email, workflow_submission_emailed_addresses)
 
+    @override_settings(WAGTAILADMIN_NOTIFICATION_INCLUDE_SUPERUSERS=True)
+    def test_submit_notification_active_users_only(self):
+        # moderator2 is inactive
+        self.moderator2.is_active = False
+        self.moderator2.save()
+
+        # superuser is inactive
+        self.superuser.is_active = False
+        self.superuser.save()
+
+        # Submit
+        self.login(self.submitter)
+        self.submit()
+
+        workflow_submission_emails = [email for email in mail.outbox if "workflow" in email.subject]
+        workflow_submission_emailed_addresses = [address for email in workflow_submission_emails for address in
+                                                 email.to]
+        task_submission_emails = [email for email in mail.outbox if "task" in email.subject]
+        task_submission_emailed_addresses = [address for email in task_submission_emails for address in email.to]
+
+        # Check that moderator2 didn't receive a task submitted email
+        self.assertNotIn(self.moderator2.email, task_submission_emailed_addresses)
+
+        # Check that the superuser didn't receive a workflow or task email
+        self.assertNotIn(self.superuser.email, task_submission_emailed_addresses)
+        self.assertNotIn(self.superuser.email, workflow_submission_emailed_addresses)
+
     @override_settings(WAGTAILADMIN_NOTIFICATION_INCLUDE_SUPERUSERS=True)
     def test_submit_notification_preferences_respected(self):
         # moderator2 doesn't want emails