Merge pull request #6033 from saadmk11/user-communication

Send site notification on Build status reporting failure and follow DRY

Author: ericholscher

readthedocs/builds/models.py

@@ -22,6 +22,7 @@
 from readthedocs.projects.constants import (
     BITBUCKET_COMMIT_URL,
     BITBUCKET_URL,
+    GITHUB_BRAND,
     GITHUB_COMMIT_URL,
     GITHUB_URL,
     GITHUB_PULL_REQUEST_URL,
@@ -843,19 +844,11 @@ def is_external(self):
     @property
     def external_version_name(self):
         if self.is_external:
-            try:
-                if self.project.remote_repository.account.provider == 'github':
-                    return GITHUB_EXTERNAL_VERSION_NAME
-                # TODO: Add External Version Name for other Git Providers
-            except RemoteRepository.DoesNotExist:
-                log.info('Remote repository does not exist for %s', self.project)
-                return GENERIC_EXTERNAL_VERSION_NAME
-            except Exception:
-                log.exception(
-                    'Unhandled exception raised for %s while getting external_version_name',
-                    self.project
-                )
-                return GENERIC_EXTERNAL_VERSION_NAME
+            if self.project.git_provider_name == GITHUB_BRAND:
+                return GITHUB_EXTERNAL_VERSION_NAME
+            # TODO: Add External Version Name for other Git Providers
+
+            return GENERIC_EXTERNAL_VERSION_NAME
         return None
 
     def using_latest_config(self):

readthedocs/oauth/notifications.py

@@ -4,6 +4,7 @@
 from messages_extends.constants import ERROR_PERSISTENT
 
 from readthedocs.notifications import SiteNotification
+from readthedocs.notifications.constants import ERROR_NON_PERSISTENT
 
 
 class AttachWebhookNotification(SiteNotification):
@@ -55,3 +56,22 @@ def get_context_data(self):
             ),
         })
         return context
+
+
+class GitBuildStatusFailureNotification(SiteNotification):
+
+    context_object_name = 'project'
+    failure_level = ERROR_NON_PERSISTENT
+    failure_message = _(
+        'Could not send {{ provider_name }} build status report for {{ project.name }}. '
+        'Make sure you have the correct {{ provider_name }} repository permissions</a> and '
+        'your <a href="{{ url_connect_account }}">{{ provider_name }} account</a> '
+        'is connected to ReadtheDocs.'
+    )  # noqa
+
+    def get_context_data(self):
+        context = super().get_context_data()
+        context.update({
+            'url_connect_account': reverse('socialaccount_connections'),
+        })
+        return context

readthedocs/projects/constants.py

@@ -355,3 +355,6 @@
 )
 
 GITHUB_PR_PULL_PATTERN = 'pull/{id}/head:external-{id}'
+
+# Git provider names
+GITHUB_BRAND = 'GitHub'

readthedocs/projects/models.py

@@ -6,6 +6,7 @@
 import re
 from urllib.parse import urlparse
 
+from allauth.socialaccount.providers import registry as allauth_registry
 from django.conf import settings
 from django.contrib.auth.models import User
 from django.core.files.storage import get_storage_class
@@ -839,6 +840,29 @@ def vcs_repo(
             )
         return repo
 
+    def git_service_class(self):
+        """Get the service class for project. e.g: GitHubService, GitLabService."""
+        from readthedocs.oauth.services import registry
+
+        for service_cls in registry:
+            if service_cls.is_project_service(self):
+                service = service_cls
+                break
+        else:
+            log.warning('There are no registered services in the application.')
+            service = None
+
+        return service
+
+    @property
+    def git_provider_name(self):
+        """Get the provider name for project. e.g: GitHub, GitLab, BitBucket."""
+        service = self.git_service_class()
+        if service:
+            provider = allauth_registry.by_id(service.adapter.provider_id)
+            return provider.name
+        return None
+
     def repo_nonblockinglock(self, version, max_lock_age=None):
         """
         Return a ``NonBlockingLock`` to acquire the lock via context manager.

readthedocs/projects/tasks.py

@@ -63,8 +63,9 @@
 from readthedocs.doc_builder.loader import get_builder_class
 from readthedocs.doc_builder.python_environments import Conda, Virtualenv
 from readthedocs.oauth.models import RemoteRepository
-from readthedocs.oauth.services import registry
+from readthedocs.oauth.notifications import GitBuildStatusFailureNotification
 from readthedocs.oauth.services.github import GitHubService
+from readthedocs.projects.constants import GITHUB_BRAND
 from readthedocs.projects.models import APIProject, Feature
 from readthedocs.search.utils import index_new_files, remove_indexed_files
 from readthedocs.sphinx_domains.models import SphinxDomain
@@ -1889,47 +1890,55 @@ def send_build_status(build_pk, commit, status):
     :param status: build status failed, pending, or success to be sent.
     """
     build = Build.objects.get(pk=build_pk)
+    provider_name = build.project.git_provider_name
 
-    try:
-        if build.project.remote_repository.account.provider == 'github':
-            service = GitHubService(
+    if provider_name == GITHUB_BRAND:
+        # get the service class for the project e.g: GitHubService.
+        service_class = build.project.git_service_class()
+        try:
+            service = service_class(
                 build.project.remote_repository.users.first(),
                 build.project.remote_repository.account
             )
-
-            # send Status report using the API.
+            # Send status report using the API.
             service.send_build_status(build, commit, status)
 
-    except RemoteRepository.DoesNotExist:
-        # Get the service provider for the project
-        for service_cls in registry:
-            if service_cls.is_project_service(build.project):
-                service = service_cls
-                break
-        else:
-            log.warning('There are no registered services in the application.')
-            return False
+        except RemoteRepository.DoesNotExist:
+            users = build.project.users.all()
+
+            # Try to loop through all project users to get their social accounts
+            for user in users:
+                user_accounts = service_class.for_user(user)
+                # Try to loop through users all social accounts to send a successful request
+                for account in user_accounts:
+                    # Currently we only support GitHub Status API
+                    if account.provider_name == provider_name:
+                        success = account.send_build_status(build, commit, status)
+                        if success:
+                            return True
+
+            for user in users:
+                # Send Site notification about Build status reporting failure
+                # to all the users of the project.
+                notification = GitBuildStatusFailureNotification(
+                    context_object=build.project,
+                    extra_context={'provider_name': provider_name},
+                    user=user,
+                    success=False,
+                )
+                notification.send()
 
-        # Try to loop through all project users to get their social accounts
-        for user in build.project.users.all():
-            user_accounts = service.for_user(user)
-            # Try to loop through users all social accounts to send a successful request
-            for account in user_accounts:
-                # Currently we only support GitHub Status API
-                if account.provider_name == 'GitHub':
-                    success = account.send_build_status(build, commit, status)
-                    if success:
-                        return True
+            log.info(
+                'No social account or repository permission available for %s',
+                build.project.slug
+            )
+            return False
 
-        log.info(
-            'No social account or repository permission available for %s',
-            build.project
-        )
-        return False
+        except Exception:
+            log.exception('Send build status task failed for %s', build.project.slug)
+            return False
 
-    except Exception:
-        log.exception('Send build status task failed for %s', build.project)
-        return False
+    return False
 
     # TODO: Send build status for other providers.
 

readthedocs/rtd_tests/tests/test_builds.py

@@ -656,13 +656,8 @@ def test_no_external_version_name(self):
         self.assertEqual(build.external_version_name, None)
 
     def test_external_version_name_github(self):
-        social_account = get(SocialAccount, provider='github')
-        remote_repo = get(
-            RemoteRepository,
-            account=social_account,
-            project=self.project
-        )
-        remote_repo.users.add(self.eric)
+        self.project.repo = 'https://github.com/test/test/'
+        self.project.save()
 
         external_version = get(Version, project=self.project, type=EXTERNAL)
         external_build = get(

readthedocs/rtd_tests/tests/test_celery.py

@@ -5,6 +5,7 @@
 
 from django.contrib.auth.models import User
 from django_dynamic_fixture import get
+from messages_extends.models import Message
 from mock import MagicMock, patch
 
 from allauth.socialaccount.models import SocialAccount
@@ -335,6 +336,9 @@ def test_fileify_logging_when_wrong_version_pk(self, mock_logger):
 
     @patch('readthedocs.projects.tasks.GitHubService.send_build_status')
     def test_send_build_status_task_with_remote_repo(self, send_build_status):
+        self.project.repo = 'https://github.com/test/test/'
+        self.project.save()
+
         social_account = get(SocialAccount, provider='github')
         remote_repo = get(RemoteRepository, account=social_account, project=self.project)
         remote_repo.users.add(self.eric)
@@ -350,6 +354,7 @@ def test_send_build_status_task_with_remote_repo(self, send_build_status):
         send_build_status.assert_called_once_with(
             external_build, external_build.commit, BUILD_STATUS_SUCCESS
         )
+        self.assertEqual(Message.objects.filter(user=self.eric).count(), 0)
 
     @patch('readthedocs.projects.tasks.GitHubService.send_build_status')
     def test_send_build_status_task_with_social_account(self, send_build_status):
@@ -369,9 +374,12 @@ def test_send_build_status_task_with_social_account(self, send_build_status):
         send_build_status.assert_called_once_with(
             external_build, external_build.commit, BUILD_STATUS_SUCCESS
         )
+        self.assertEqual(Message.objects.filter(user=self.eric).count(), 0)
 
     @patch('readthedocs.projects.tasks.GitHubService.send_build_status')
     def test_send_build_status_task_without_remote_repo_or_social_account(self, send_build_status):
+        self.project.repo = 'https://github.com/test/test/'
+        self.project.save()
         external_version = get(Version, project=self.project, type=EXTERNAL)
         external_build = get(
             Build, project=self.project, version=external_version
@@ -381,3 +389,4 @@ def test_send_build_status_task_without_remote_repo_or_social_account(self, send
         )
 
         send_build_status.assert_not_called()
+        self.assertEqual(Message.objects.filter(user=self.eric).count(), 1)