Merge pull request #6610 from stsewd/protection-for-none-when-sending-notifications

Protection against None when sending notifications

Author: stsewd

docs/guides/build-notifications.rst

@@ -1,6 +1,11 @@
 Enabling Build Notifications
 ============================
 
+.. note::
+
+   Currently we don't send notifications when
+   a :doc:`build from a pull/merge request fails </guides/autobuild-docs-for-pull-requests>`.
+
 Using email
 -----------
 

readthedocs/projects/tasks.py

@@ -1269,7 +1269,9 @@ def build_docs_class(self, builder_class):
 
     def send_notifications(self, version_pk, build_pk, email=False):
         """Send notifications on build failure."""
-        if self.version.type != EXTERNAL:
+        # Try to infer the version type if we can
+        # before creating a task.
+        if not self.version or self.version.type != EXTERNAL:
             send_notifications.delay(version_pk, build_pk=build_pk, email=email)
 
     def is_type_sphinx(self):
@@ -1846,7 +1848,7 @@ def _sync_imported_files(version, build, changed_files):
 def send_notifications(version_pk, build_pk, email=False):
     version = Version.objects.get_object_or_log(pk=version_pk)
 
-    if not version:
+    if not version or version.type == EXTERNAL:
         return
 
     build = Build.objects.get(pk=build_pk)

readthedocs/rtd_tests/tests/test_build_notifications.py

@@ -1,4 +1,3 @@
-# -*- coding: utf-8 -*-
 """Notifications sent after build is completed."""
 
 import json
@@ -8,6 +7,7 @@
 from django.test import TestCase
 from unittest.mock import patch
 
+from readthedocs.builds.constants import EXTERNAL
 from readthedocs.builds.models import Build, Version
 from readthedocs.projects.forms import WebHookForm
 from readthedocs.projects.models import EmailHook, Project, WebHook
@@ -19,7 +19,7 @@ def setUp(self):
         self.project = fixture.get(Project)
         self.version = fixture.get(Version, project=self.project)
         self.build = fixture.get(Build, version=self.version)
-    
+
     @patch('readthedocs.builds.managers.log')
     def test_send_notification_none_if_wrong_version_pk(self, mock_logger):
         self.assertFalse(Version.objects.filter(pk=345343).exists())
@@ -40,6 +40,18 @@ def test_send_webhook_notification(self):
 
         self.assertEqual(len(mail.outbox), 0)
 
+    def test_dont_send_webhook_notifications_for_external_versions(self):
+        fixture.get(WebHook, project=self.project)
+        self.version.type = EXTERNAL
+        self.version.save()
+
+        with patch('readthedocs.projects.tasks.requests.post') as mock:
+            mock.return_value = None
+            send_notifications(self.version.pk, self.build.pk)
+            mock.assert_not_called()
+
+        self.assertEqual(len(mail.outbox), 0)
+
     def test_send_webhook_notification_has_content_type_header(self):
         hook = fixture.get(WebHook, project=self.project)
         data = json.dumps({
@@ -67,6 +79,14 @@ def test_send_email_notification(self):
         send_notifications(self.version.pk, self.build.pk, email=True)
         self.assertEqual(len(mail.outbox), 1)
 
+    def test_dont_send_email_notifications_for_external_versions(self):
+        fixture.get(EmailHook, project=self.project)
+        self.version.type = EXTERNAL
+        self.version.save()
+
+        send_notifications(self.version.pk, self.build.pk, email=True)
+        self.assertEqual(len(mail.outbox), 0)
+
     def test_send_email_and_webhook__notification(self):
         fixture.get(EmailHook, project=self.project)
         fixture.get(WebHook, project=self.project)