Merge pull request #2102 from rtfd/fix-webhook-logic

Refactor GitHub web hook logic.

readthedocs/oauth/services/bitbucket.py

@@ -178,8 +178,8 @@ class BitbucketService(Service):
 
         :param project: project to set up webhook for
         :type project: Project
-        :returns: boolean based on webhook set up success
-        :rtype: bool
+        :returns: boolean based on webhook set up success, and requests Response object
+        :rtype: (Bool, Response)
         """
         session = self.get_session()
         owner, repo = build_utils.get_bitbucket_username_repo(url=project.repo)
@@ -199,11 +199,11 @@ class BitbucketService(Service):
             if resp.status_code == 201:
                 log.info('Bitbucket webhook creation successful for project: %s',
                          project)
-                return True
+                return (True, resp)
         except RequestException:
             log.error('Bitbucket webhook creation failed for project: %s',
                       project, exc_info=True)
         else:
             log.error('Bitbucket webhook creation failed for project: %s',
                       project)
-            return False
+            return (False, resp)

readthedocs/oauth/services/github.py

@@ -163,8 +163,8 @@ class GitHubService(Service):
 
         :param project: project to set up webhook for
         :type project: Project
-        :returns: boolean based on webhook set up success
-        :rtype: bool
+        :returns: boolean based on webhook set up success, and requests Response object
+        :rtype: (Bool, Response)
         """
         session = self.get_session()
         owner, repo = build_utils.get_github_username_repo(url=project.repo)
@@ -181,10 +181,11 @@ class GitHubService(Service):
                 data=data,
                 headers={'content-type': 'application/json'}
             )
-            if resp.status_code == 201:
+            # GitHub will return 200 if already synced
+            if resp.status_code in [200, 201]:
                 log.info('GitHub webhook creation successful for project: %s',
                          project)
-                return True
+                return (True, resp)
         except RequestException:
             log.error('GitHub webhook creation failed for project: %s',
                       project, exc_info=True)
@@ -192,7 +193,7 @@ class GitHubService(Service):
         else:
             log.error('GitHub webhook creation failed for project: %s',
                       project)
-            return False
+            return (False, resp)
 
     @classmethod
     def get_token_for_project(cls, project, force_local=False):

readthedocs/oauth/utils.py

@@ -0,0 +1,42 @@
+import logging
+
+from django.contrib import messages
+from django.utils.translation import ugettext_lazy as _
+
+from readthedocs.oauth.services import registry
+
+log = logging.getLogger(__name__)
+
+
+def attach_webhook(project, request=None):
+    """Add post-commit hook on project import"""
+
+    for service_cls in registry:
+        if service_cls.is_project_service(project):
+            service = service_cls
+            break
+    else:
+        return None
+
+    user_accounts = service.for_user(request.user)
+    for account in user_accounts:
+        success, resp = account.setup_webhook(project)
+        if success:
+            messages.success(request, _('Webhook activated'))
+            project.has_valid_webhook = True
+            project.save()
+            break
+    else:
+        if user_accounts:
+            messages.error(
+                request,
+                _('Webhook activation failed. Make sure you have permissions to set it.')
+            )
+        else:
+            messages.error(
+                request,
+                _('No accounts available to set webhook on. '
+                  'Please connect your {network} account.'.format(
+                      network=service.adapter().get_provider().name
+                  ))
+            )

readthedocs/projects/signals.py

@@ -1,13 +1,9 @@
 """Project signals"""
 
-import logging
-
 import django.dispatch
-from django.contrib import messages
 from django.dispatch import receiver
-from django.utils.translation import ugettext_lazy as _
 
-from readthedocs.oauth.services import registry
+from readthedocs.oauth.utils import attach_webhook
 
 
 before_vcs = django.dispatch.Signal(providing_args=["version"])
@@ -19,29 +15,10 @@ after_build = django.dispatch.Signal(providing_args=["version"])
 project_import = django.dispatch.Signal(providing_args=["project"])
 
 
-log = logging.getLogger(__name__)
-
-
 @receiver(project_import)
 def handle_project_import(sender, **kwargs):
     """Add post-commit hook on project import"""
     project = sender
     request = kwargs.get('request')
-    _set = False
-    _service = None
 
-    for service_cls in registry:
-        if service_cls.is_project_service(project):
-            for service in service_cls.for_user(request.user):
-                _service = service
-                if service.setup_webhook(project):
-                    messages.success(request, _('Webhook activated'))
-                    _set = True
-                else:
-                    messages.error(request, _('Webhook configuration failed'))
-    if not _set and _service:
-        messages.error(
-            request,
-            _('No accounts available to set webhook on. '
-              'Please connect your %s account.' % _service.get_adapter()().get_provider().name)
-        )
+    attach_webhook(project=project, request=request)

readthedocs/projects/views/private.py

@@ -35,8 +35,8 @@ from readthedocs.projects.forms import (
 from readthedocs.projects.models import Project, EmailHook, WebHook, Domain
 from readthedocs.projects.views.base import ProjectAdminMixin, ProjectSpamMixin
 from readthedocs.projects import constants, tasks
-from readthedocs.projects.exceptions import ProjectSpamError
 from readthedocs.oauth.services import registry
+from readthedocs.oauth.utils import attach_webhook
 
 from readthedocs.core.mixins import LoginRequiredMixin
 from readthedocs.projects.signals import project_import
@@ -678,7 +678,7 @@ def project_resync_webhook(request, project_slug):
     project = get_object_or_404(Project.objects.for_admin_user(request.user),
                                 slug=project_slug)
     if request.method == 'POST':
-        project_import.send(sender=project, request=request)
+        attach_webhook(project=project, request=request)
         return HttpResponseRedirect(reverse('projects_detail',
                                             args=[project.slug]))