Merge pull request #4991 from rtfd/humitos/projects/skip

Skip builds when project is not active

Author: humitos

common

@@ -12,14 +12,15 @@
 import logging
 
 from builtins import object
+from django.contrib import messages
 from django.contrib.auth.decorators import login_required
-from django.urls import reverse
 from django.http import (
     HttpResponseForbidden,
     HttpResponsePermanentRedirect,
     HttpResponseRedirect,
 )
 from django.shortcuts import get_object_or_404
+from django.urls import reverse
 from django.utils.decorators import method_decorator
 from django.views.generic import DetailView, ListView
 
@@ -28,6 +29,7 @@
 from readthedocs.core.utils import trigger_build
 from readthedocs.projects.models import Project
 
+
 log = logging.getLogger(__name__)
 
 
@@ -63,7 +65,18 @@ def post(self, request, project_slug):
             slug=version_slug,
         )
 
-        _, build = trigger_build(project=project, version=version)
+        update_docs_task, build = trigger_build(project=project, version=version)
+        if (update_docs_task, build) == (None, None):
+            # Build was skipped
+            messages.add_message(
+                request,
+                messages.WARNING,
+                "This project is currently disabled and can't trigger new builds.",
+            )
+            return HttpResponseRedirect(
+                reverse('builds_project_list', args=[project.slug]),
+            )
+
         return HttpResponseRedirect(
             reverse('builds_detail', args=[project.slug, build.pk]),
         )

readthedocs/builds/views.py

@@ -20,6 +20,7 @@
 from readthedocs.builds.constants import LATEST, BUILD_STATE_TRIGGERED
 from readthedocs.doc_builder.constants import DOCKER_LIMITS
 
+
 log = logging.getLogger(__name__)
 
 SYNC_USER = getattr(settings, 'SYNC_USER', getpass.getuser())
@@ -87,18 +88,22 @@ def prepare_build(
     :param record: whether or not record the build in a new Build object
     :param force: build the HTML documentation even if the files haven't changed
     :param immutable: whether or not create an immutable Celery signature
-    :returns: Celery signature of update_docs_task to be executed
+    :returns: Celery signature of update_docs_task and Build instance
+    :rtype: tuple
     """
     # Avoid circular import
-    from readthedocs.projects.tasks import update_docs_task
     from readthedocs.builds.models import Build
+    from readthedocs.projects.models import Project
+    from readthedocs.projects.tasks import update_docs_task
+
+    build = None
 
-    if project.skip:
-        log.info(
-            'Build not triggered because Project.skip=True: project=%s',
+    if not Project.objects.is_active(project):
+        log.warning(
+            'Build not triggered because Project is not active: project=%s',
             project.slug,
         )
-        return None
+        return (None, None)
 
     if not version:
         version = project.versions.get(slug=LATEST)
@@ -158,7 +163,8 @@ def trigger_build(project, version=None, record=True, force=False):
     :param version: version of the project to be built. Default: ``latest``
     :param record: whether or not record the build in a new Build object
     :param force: build the HTML documentation even if the files haven't changed
-    :returns: A tuple (Celery AsyncResult promise, Task Signature from ``prepare_build``)
+    :returns: Celery AsyncResult promise and Build instance
+    :rtype: tuple
     """
     update_docs_task, build = prepare_build(
         project,
@@ -168,9 +174,9 @@ def trigger_build(project, version=None, record=True, force=False):
         immutable=True,
     )
 
-    if update_docs_task is None:
-        # Current project is skipped
-        return None
+    if (update_docs_task, build) == (None, None):
+        # Build was skipped
+        return (None, None)
 
     return (update_docs_task.apply_async(), build)
 

readthedocs/core/utils/__init__.py

@@ -1,14 +1,16 @@
-"""Project model QuerySet classes"""
+# -*- coding: utf-8 -*-
+"""Project model QuerySet classes."""
 
 from __future__ import absolute_import
 
 from django.db import models
 from django.db.models import Q
 from guardian.shortcuts import get_objects_for_user
 
-from . import constants
 from readthedocs.core.utils.extend import SettingsOverrideObject
 
+from . import constants
+
 
 class ProjectQuerySetBase(models.QuerySet):
 
@@ -54,6 +56,24 @@ def private(self, user=None):
             return self._add_user_repos(queryset, user)
         return queryset
 
+    def is_active(self, project):
+        """
+        Check if the project is active.
+
+        The check consists on,
+          * the Project shouldn't be marked as skipped.
+
+        :param project: project to be checked
+        :type project: readthedocs.projects.models.Project
+
+        :returns: whether or not the project is active
+        :rtype: bool
+        """
+        if project.skip:
+            return False
+
+        return True
+
     # Aliases
 
     def dashboard(self, user=None):

readthedocs/projects/querysets.py

@@ -16,7 +16,6 @@
 from django.contrib import messages
 from django.contrib.auth.decorators import login_required
 from django.contrib.auth.models import User
-from django.urls import reverse
 from django.http import (
     Http404,
     HttpResponseBadRequest,
@@ -25,6 +24,7 @@
 )
 from django.middleware.csrf import get_token
 from django.shortcuts import get_object_or_404, render
+from django.urls import reverse
 from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext_lazy as _
 from django.views.generic import ListView, TemplateView, View
@@ -287,6 +287,9 @@ def done(self, form_list, **kwargs):
     def trigger_initial_build(self, project):
         """Trigger initial build."""
         update_docs, build = prepare_build(project)
+        if (update_docs, build) == (None, None):
+            return None
+
         task_promise = chain(
             attach_webhook.si(project.pk, self.request.user.pk),
             update_docs,

readthedocs/projects/views/private.py

@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
 """Endpoints integrating with Github, Bitbucket, and other webhooks."""
 
 from __future__ import (
@@ -13,7 +14,7 @@
 
 import six
 from django.shortcuts import get_object_or_404
-from rest_framework import permissions
+from rest_framework import permissions, status
 from rest_framework.exceptions import NotFound, ParseError
 from rest_framework.renderers import JSONRenderer
 from rest_framework.response import Response
@@ -55,11 +56,14 @@ def post(self, request, project_slug):
         """Set up webhook post view with request and project objects."""
         self.request = request
         self.project = None
+        self.data = self.get_data()
         try:
             self.project = self.get_project(slug=project_slug)
+            if not Project.objects.is_active(self.project):
+                resp = {'detail': 'This project is currently disabled'}
+                return Response(resp, status=status.HTTP_406_NOT_ACCEPTABLE)
         except Project.DoesNotExist:
             raise NotFound('Project not found')
-        self.data = self.get_data()
         resp = self.handle_webhook()
         if resp is None:
             log.info('Unhandled webhook event')

readthedocs/restapi/views/integrations.py

@@ -862,6 +862,22 @@ def setUp(self):
             },
         }
 
+    def test_webhook_skipped_project(self, trigger_build):
+        client = APIClient()
+        self.project.skip = True
+        self.project.save()
+
+        response = client.post(
+            '/api/v2/webhook/github/{0}/'.format(
+                self.project.slug,
+            ),
+            self.github_payload,
+            format='json',
+        )
+        self.assertDictEqual(response.data, {'detail': 'This project is currently disabled'})
+        self.assertEqual(response.status_code, status.HTTP_406_NOT_ACCEPTABLE)
+        self.assertFalse(trigger_build.called)
+
     def test_github_webhook_for_branches(self, trigger_build):
         """GitHub webhook API."""
         client = APIClient()

readthedocs/rtd_tests/tests/test_api.py

@@ -18,6 +18,18 @@ def setUp(self):
         self.project = get(Project, container_time_limit=None)
         self.version = get(Version, project=self.project)
 
+    @mock.patch('readthedocs.projects.tasks.update_docs_task')
+    def test_trigger_skipped_project(self, update_docs_task):
+        self.project.skip = True
+        self.project.save()
+        result = trigger_build(
+            project=self.project,
+            version=self.version,
+        )
+        self.assertEqual(result, (None, None))
+        self.assertFalse(update_docs_task.signature.called)
+        self.assertFalse(update_docs_task.signature().apply_async.called)
+
     @mock.patch('readthedocs.projects.tasks.update_docs_task')
     def test_trigger_custom_queue(self, update_docs):
         """Use a custom queue when routing the task"""

readthedocs/rtd_tests/tests/test_core_utils.py

@@ -30,6 +30,13 @@ def test_subproject_queryset_as_manager_gets_correct_class(self):
             'ManagerFromParentRelatedProjectQuerySetBase'
         )
 
+    def test_is_active(self):
+        project = fixture.get(Project, skip=False)
+        self.assertTrue(Project.objects.is_active(project))
+
+        project = fixture.get(Project, skip=True)
+        self.assertFalse(Project.objects.is_active(project))
+
 
 class FeatureQuerySetTests(TestCase):