Refactor GitHub web hook logic.

This fixes an error where we would say it failed
when the web hook was already set (status 200).
Also passes the response along to the attach_webhook caller,
so it can be smarter about error messages/logic.

readthedocs/oauth/services/bitbucket.py

@@ -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

@@ -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,32 @@
+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):
+            user_accounts = service_cls.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 %s account.' % service_cls.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]))