feat(releases): Add email notification on commit fetch failure

src/sentry/tasks/commits.py

@@ -22,12 +22,26 @@ def generate_invalid_identity_email(identity, commit_failure=False):
     }
 
     return MessageBuilder(
-        subject='Action Required',
+        subject='Unable to Fetch Commits' if commit_failure else 'Action Required',
         context=new_context,
         template='sentry/emails/identity-invalid.txt',
         html_template='sentry/emails/identity-invalid.html',
     )
 
+
+def generate_fetch_commits_error_email(release, error_message):
+    new_context = {
+        'release': release,
+        'error_message': error_message,
+    }
+
+    return MessageBuilder(
+        subject='Unable to Fetch Commits',
+        context=new_context,
+        template='sentry/emails/unable-to-fetch-commits.txt',
+        html_template='sentry/emails/unable-to-fetch-commits.html',
+    )
+
 # we're future proofing this function a bit so it could be used with other code
 
 
@@ -107,7 +121,7 @@ def fetch_commits(release_id, user_id, refs, prev_release_id=None, **kwargs):
             repo_commits = provider.compare_commits(repo, start_sha, end_sha, actor=user)
         except NotImplementedError:
             pass
-        except (PluginError, InvalidIdentity) as exc:
+        except Exception as exc:
             logger.exception(
                 'fetch_commits.error',
                 exc_info=True,
@@ -121,6 +135,13 @@ def fetch_commits(release_id, user_id, refs, prev_release_id=None, **kwargs):
             )
             if isinstance(exc, InvalidIdentity) and getattr(exc, 'identity', None):
                 handle_invalid_identity(identity=exc.identity, commit_failure=True)
+            elif isinstance(exc, (PluginError, InvalidIdentity)):
+                msg = generate_fetch_commits_error_email(release, exc.message)
+                msg.send_async(to=[user.email])
+            else:
+                msg = generate_fetch_commits_error_email(
+                    release, 'An internal system error occurred.')
+                msg.send_async(to=[user.email])
         else:
             logger.info(
                 'fetch_commits.complete',

src/sentry/templates/sentry/debug/mail/preview.html

@@ -27,6 +27,9 @@
         <option value="mail/recover-account/">Reset Password</option>
         <option value="mail/invalid-identity/">Invalid Identity</option>
       </optgroup>
+      <optgroup label="Releases">
+        <option value="mail/unable-to-fetch-commits/">Unable to Fetch Commits</option>
+      </optgroup>
       <optgroup label="Membership">
         <option value="mail/request-access/">Access Requested</option>
         <option value="mail/access-approved/">Access Approved</option>

src/sentry/templates/sentry/emails/unable-to-fetch-commits.html

@@ -0,0 +1,9 @@
+{% extends "sentry/emails/base.html" %}
+
+{% block main %}
+  <h3>Unable to Fetch Commits</h3>
+  <p>We were unable to fetch the commit log for your release (<em>{{ release.version }}</em>) due to the following error:</p>
+  <p>{{ error_message }}</p>
+{% endblock %}
+
+{% block footer %}{% endblock %}

src/sentry/templates/sentry/emails/unable-to-fetch-commits.txt

@@ -0,0 +1,6 @@
+Unable to Fetch Commits
+-----------------------
+
+We were unable to fetch the commit log for your release ({{ release.version }}) due to the following error:
+
+{{ error_message }}

src/sentry/web/debug_urls.py

@@ -23,6 +23,7 @@ from sentry.web.frontend.debug.debug_resolved_email import (DebugResolvedEmailVi
 from sentry.web.frontend.debug.debug_resolved_in_release_email import (
     DebugResolvedInReleaseEmailView, DebugResolvedInReleaseUpcomingEmailView
 )
+from sentry.web.frontend.debug.debug_unable_to_fetch_commits_email import DebugUnableToFetchCommitsEmailView
 from sentry.web.frontend.debug.debug_unassigned_email import (DebugUnassignedEmailView)
 from sentry.web.frontend.debug.debug_new_processing_issues_email import (
     DebugNewProcessingIssuesEmailView,
@@ -57,6 +58,7 @@ urlpatterns = patterns(
     url(r'^debug/mail/invalid-identity/$', DebugInvalidIdentityEmailView.as_view()),
     url(r'^debug/mail/confirm-email/$', sentry.web.frontend.debug.mail.confirm_email),
     url(r'^debug/mail/recover-account/$', sentry.web.frontend.debug.mail.recover_account),
+    url(r'^debug/mail/unable-to-fetch-commits/$', DebugUnableToFetchCommitsEmailView.as_view()),
     url(r'^debug/mail/unassigned/$', DebugUnassignedEmailView.as_view()),
     url(r'^debug/mail/org-delete-confirm/$', sentry.web.frontend.debug.mail.org_delete_confirm),
     url(r'^debug/mail/mfa-removed/$', DebugMfaRemovedEmailView.as_view()),

src/sentry/web/frontend/debug/debug_unable_to_fetch_commits_email.py

@@ -0,0 +1,23 @@
+from __future__ import absolute_import, print_function
+
+from django.views.generic import View
+
+from sentry.models import Release
+from sentry.tasks.commits import generate_fetch_commits_error_email
+
+from .mail import MailPreview
+
+
+class DebugUnableToFetchCommitsEmailView(View):
+    def get(self, request):
+        release = Release(version='abcdef')
+
+        email = generate_fetch_commits_error_email(
+            release,
+            'An internal server error occurred'
+        )
+        return MailPreview(
+            html_template=email.html_template,
+            text_template=email.template,
+            context=email.context,
+        ).render(request)

tests/acceptance/test_emails.py

@@ -4,6 +4,28 @@ from six.moves.urllib.parse import urlencode
 
 from sentry.testutils import AcceptanceTestCase
 
+EMAILS = (
+    ('/debug/mail/assigned/', 'assigned'),
+    ('/debug/mail/assigned/self/', 'assigned self'),
+    ('/debug/mail/note/', 'note'),
+    ('/debug/mail/regression/', 'regression'),
+    ('/debug/mail/regression/release/', 'regression with version'),
+    ('/debug/mail/new-release/', 'release'),
+    ('/debug/mail/resolved/', 'resolved'),
+    ('/debug/mail/resolved-in-release/', 'resolved in release'),
+    ('/debug/mail/resolved-in-release/upcoming/', 'resolved in release upcoming'),
+    ('/debug/mail/unassigned/', 'unassigned'),
+    ('/debug/mail/unable-to-fetch-commits/', 'unable to fetch commits'),
+    ('/debug/mail/alert/', 'alert'),
+    ('/debug/mail/digest/', 'digest'),
+    ('/debug/mail/invalid-identity/', 'invalid identity'),
+    ('/debug/mail/invitation/', 'invitation'),
+    ('/debug/mail/report/', 'report'),
+    ('/debug/mail/mfa-added/', 'mfa added'),
+    ('/debug/mail/mfa-removed/', 'mfa removed'),
+    ('/debug/mail/password-changed/', 'password changed'),
+)
+
 
 class EmailTestCase(AcceptanceTestCase):
     def setUp(self):
@@ -20,177 +42,12 @@ class EmailTestCase(AcceptanceTestCase):
             }),
         )
 
-    def test_assigned_html(self):
-        self.browser.get(self.build_url('/debug/mail/assigned/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('assigned email html')
-
-    def test_assigned_txt(self):
-        self.browser.get(self.build_url('/debug/mail/assigned/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('assigned email txt')
-
-    def test_assigned_self_html(self):
-        self.browser.get(self.build_url('/debug/mail/assigned/self/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('assigned_self email html')
-
-    def test_assigned_self_txt(self):
-        self.browser.get(self.build_url('/debug/mail/assigned/self/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('assigned_self email txt')
-
-    def test_note_html(self):
-        self.browser.get(self.build_url('/debug/mail/note/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('note email html')
-
-    def test_note_txt(self):
-        self.browser.get(self.build_url('/debug/mail/note/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('note email txt')
-
-    def test_regression_html(self):
-        self.browser.get(self.build_url('/debug/mail/regression/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('regression email html')
-
-    def test_regression_txt(self):
-        self.browser.get(self.build_url('/debug/mail/regression/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('regression email txt')
-
-    def test_regression_with_version_html(self):
-        self.browser.get(self.build_url('/debug/mail/regression/release/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('regression_with_version email html')
-
-    def test_regression_with_version_txt(self):
-        self.browser.get(self.build_url('/debug/mail/regression/release/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('regression_with_version email txt')
-
-    def test_release_html(self):
-        self.browser.get(self.build_url('/debug/mail/new-release/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('release email html')
-
-    def test_release_txt(self):
-        self.browser.get(self.build_url('/debug/mail/new-release/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('release email txt')
-
-    def test_resolved_html(self):
-        self.browser.get(self.build_url('/debug/mail/resolved/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('resolved email html')
-
-    def test_resolved_txt(self):
-        self.browser.get(self.build_url('/debug/mail/resolved/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('resolved email txt')
-
-    def test_resolved_in_release_html(self):
-        self.browser.get(self.build_url('/debug/mail/resolved-in-release/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('resolved_in_release email html')
-
-    def test_resolved_in_release_txt(self):
-        self.browser.get(self.build_url('/debug/mail/resolved-in-release/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('resolved_in_release email txt')
-
-    def test_resolved_in_release_upcoming_html(self):
-        self.browser.get(self.build_url('/debug/mail/resolved-in-release/upcoming/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('resolved_in_release_upcoming email html')
-
-    def test_resolved_in_release_upcoming_txt(self):
-        self.browser.get(self.build_url('/debug/mail/resolved-in-release/upcoming/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('resolved_in_release_upcoming email txt')
-
-    def test_unassigned_html(self):
-        self.browser.get(self.build_url('/debug/mail/unassigned/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('unassigned email html')
-
-    def test_unassigned_txt(self):
-        self.browser.get(self.build_url('/debug/mail/unassigned/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('unassigned email txt')
-
-    def test_new_event_html(self):
-        self.browser.get(self.build_url('/debug/mail/alert/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('new event email html')
-
-    def test_new_event_txt(self):
-        self.browser.get(self.build_url('/debug/mail/alert/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('new event email txt')
-
-    def test_digest_html(self):
-        self.browser.get(self.build_url('/debug/mail/digest/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('digest email html')
-
-    def test_digest_txt(self):
-        self.browser.get(self.build_url('/debug/mail/digest/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('digest email txt')
-
-    def test_invalid_identity_text(self):
-        self.browser.get(self.build_url('/debug/mail/invalid-identity/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('invalid identity email txt')
-
-    def test_invalid_identity_html(self):
-        self.browser.get(self.build_url('/debug/mail/invalid-identity/', 'html'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('invalid identity email html')
-
-    def test_invitation_text(self):
-        self.browser.get(self.build_url('/debug/mail/invitation/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('invitation email txt')
-
-    def test_invitation_html(self):
-        self.browser.get(self.build_url('/debug/mail/invitation/', 'html'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('invitation email html')
-
-    def test_report_html(self):
-        self.browser.get(self.build_url('/debug/mail/report/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('report email html')
-
-    def test_mfa_added_html(self):
-        self.browser.get(self.build_url('/debug/mail/mfa-added/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('mfa added email html')
-
-    def test_mfa_added_txt(self):
-        self.browser.get(self.build_url('/debug/mail/mfa-added/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('mfa added email txt')
-
-    def test_mfa_removed_html(self):
-        self.browser.get(self.build_url('/debug/mail/mfa-removed/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('mfa removed email html')
-
-    def test_mfa_removed_text(self):
-        self.browser.get(self.build_url('/debug/mail/mfa-removed/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('mfa removed email txt')
-
-    def test_password_changed_html(self):
-        self.browser.get(self.build_url('/debug/mail/password-changed/'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('password changed email html')
+    def test_emails(self):
+        for url, name in EMAILS:
+            self.browser.get(self.build_url(url, 'html'))
+            self.browser.wait_until('#preview')
+            self.browser.snapshot('{} email html'.format(name))
 
-    def test_password_changed_text(self):
-        self.browser.get(self.build_url('/debug/mail/password-changed/', 'txt'))
-        self.browser.wait_until('#preview')
-        self.browser.snapshot('password changed email txt')
+            self.browser.get(self.build_url(url, 'txt'))
+            self.browser.wait_until('#preview')
+            self.browser.snapshot('{} email txt'.format(name))

tests/sentry/tasks/test_commits.py

@@ -4,7 +4,7 @@ from django.core import mail
 from mock import patch
 from social_auth.models import UserSocialAuth
 
-from sentry.exceptions import InvalidIdentity
+from sentry.exceptions import InvalidIdentity, PluginError
 from sentry.models import Commit, Deploy, Release, ReleaseHeadCommit, Repository
 from sentry.tasks.commits import fetch_commits, handle_invalid_identity
 from sentry.testutils import TestCase
@@ -85,7 +85,7 @@ class FetchCommitsTest(TestCase):
 
     @patch('sentry.tasks.commits.handle_invalid_identity')
     @patch('sentry.plugins.providers.dummy.repository.DummyRepositoryProvider.compare_commits')
-    def test_invalid_identity(self, mock_compare_commits, mock_handle_invalid_identity):
+    def test_fetch_error_invalid_identity(self, mock_compare_commits, mock_handle_invalid_identity):
         self.login_as(user=self.user)
         org = self.create_organization(owner=self.user, name='baz')
 
@@ -141,6 +141,122 @@ class FetchCommitsTest(TestCase):
             commit_failure=True,
         )
 
+    @patch('sentry.plugins.providers.dummy.repository.DummyRepositoryProvider.compare_commits')
+    def test_fetch_error_plugin_error(self, mock_compare_commits):
+        self.login_as(user=self.user)
+        org = self.create_organization(owner=self.user, name='baz')
+
+        repo = Repository.objects.create(
+            name='example',
+            provider='dummy',
+            organization_id=org.id,
+        )
+        release = Release.objects.create(
+            organization_id=org.id,
+            version='abcabcabc',
+        )
+
+        commit = Commit.objects.create(
+            organization_id=org.id,
+            repository_id=repo.id,
+            key='a' * 40,
+        )
+
+        ReleaseHeadCommit.objects.create(
+            organization_id=org.id,
+            repository_id=repo.id,
+            release=release,
+            commit=commit,
+        )
+
+        refs = [{
+            'repository': repo.name,
+            'commit': 'b' * 40,
+        }]
+
+        release2 = Release.objects.create(
+            organization_id=org.id,
+            version='12345678',
+        )
+
+        UserSocialAuth.objects.create(
+            user=self.user,
+            provider='dummy',
+        )
+
+        mock_compare_commits.side_effect = Exception('secrets')
+
+        with self.tasks():
+            fetch_commits(
+                release_id=release2.id,
+                user_id=self.user.id,
+                refs=refs,
+                previous_release_id=release.id,
+            )
+
+        msg = mail.outbox[-1]
+        assert msg.subject == 'Unable to Fetch Commits'
+        assert msg.to == [self.user.email]
+        assert 'secrets' not in msg.body
+
+    @patch('sentry.plugins.providers.dummy.repository.DummyRepositoryProvider.compare_commits')
+    def test_fetch_error_random_exception(self, mock_compare_commits):
+        self.login_as(user=self.user)
+        org = self.create_organization(owner=self.user, name='baz')
+
+        repo = Repository.objects.create(
+            name='example',
+            provider='dummy',
+            organization_id=org.id,
+        )
+        release = Release.objects.create(
+            organization_id=org.id,
+            version='abcabcabc',
+        )
+
+        commit = Commit.objects.create(
+            organization_id=org.id,
+            repository_id=repo.id,
+            key='a' * 40,
+        )
+
+        ReleaseHeadCommit.objects.create(
+            organization_id=org.id,
+            repository_id=repo.id,
+            release=release,
+            commit=commit,
+        )
+
+        refs = [{
+            'repository': repo.name,
+            'commit': 'b' * 40,
+        }]
+
+        release2 = Release.objects.create(
+            organization_id=org.id,
+            version='12345678',
+        )
+
+        UserSocialAuth.objects.create(
+            user=self.user,
+            provider='dummy',
+        )
+
+        mock_compare_commits.side_effect = PluginError('You can read me')
+
+        with self.tasks():
+            fetch_commits(
+                release_id=release2.id,
+                user_id=self.user.id,
+                refs=refs,
+                previous_release_id=release.id,
+            )
+
+        msg = mail.outbox[-1]
+        assert msg.subject == 'Unable to Fetch Commits'
+        assert msg.to == [self.user.email]
+        assert 'You can read me' in msg.body
+
 
 class HandleInvalidIdentityTest(TestCase):
     def test_simple(self):
@@ -157,3 +273,18 @@ class HandleInvalidIdentityTest(TestCase):
         msg = mail.outbox[-1]
         assert msg.subject == 'Action Required'
         assert msg.to == [self.user.email]
+
+    def test_commit_failure(self):
+        usa = UserSocialAuth.objects.create(
+            user=self.user,
+            provider='dummy',
+        )
+
+        with self.tasks():
+            handle_invalid_identity(usa, commit_failure=True)
+
+        assert not UserSocialAuth.objects.filter(id=usa.id).exists()
+
+        msg = mail.outbox[-1]
+        assert msg.subject == 'Unable to Fetch Commits'
+        assert msg.to == [self.user.email]