diff --git a/docs/guides/build-notifications.rst b/docs/guides/build-notifications.rst index 3e8fcfe394f..bf2d194ccc7 100644 --- a/docs/guides/build-notifications.rst +++ b/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 `. + Using email ----------- diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 372765e507e..3b72b738469 100644 --- a/readthedocs/projects/tasks.py +++ b/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) diff --git a/readthedocs/rtd_tests/tests/test_build_notifications.py b/readthedocs/rtd_tests/tests/test_build_notifications.py index 83f1bd5a765..f09bdffc9df 100644 --- a/readthedocs/rtd_tests/tests/test_build_notifications.py +++ b/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)