From cedda9bf538e41122f075ba00bee268dcff67a60 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 14 Jun 2018 15:29:08 -0300 Subject: [PATCH] Allow to hook the initial build from outside (#4033) * Allow to hook the initial build from outside Add a new attribute to `trigger_build` to do not execute the task but just return the immutable signature of it, so it can be chained into a bigger task from outside when it's needed to do something else before/after it's execution. * Remove unused basic attribute on trigger build * Split trigger_build to be able to prepare_build first Added a ``ImportWizard.trigger_initial_build`` method to be override from corporate site. Also, instead of using ``trigger_build`` to create the Celery task and call it immediately, split it to use ``prepare_build`` first to create the task and use ``trigger_build`` to effectively triggers it. * Remove now unused signal * Fix Celery signature creation * Fix testcases with basic on trigger_build * Fix mock calls in test cases * Fix test mock check * Use async task to attach webhook * Use proper context object * Make it compatible with newer Django versions * Py2 and Py3 compatible line * Dismiss the sticky message and stay in the same page * Use Context to render the template * Lint errors fixed * Call the task in a sync way properly * Update common submodule to latest * Define NonPersistentStorage for one-time notifications NonPersistentNotification backed together with SiteNotification class helps us to create one-time site notifications and attach it to a user without the needs of a request/session object. The message/notification is saved into the database as a PersistentMessage and it's marked as read while it's retrived the first time. This system is useful for notifications that need to be attached from a Celery task where we know the user but we don't have the request. * Make string translatable * Fix merge conflicts * Recover accidentally deleted line * Fixed linting errors * Replace message_key with reason to be clearer * Rename non persistent storage class * Remove old templates * Make SiteNotification more flexible - render strings messages as Django Templates - accept extra_context for the template - do not crash if the reason is incorrect * Adapt AttachWebhookNotification to the new features * Refactor the task to attach a webhook Instantiate only once the notification and adapt it depending the context. Also, if there are no connected services into our application do not show a message to the user, but log it as a warning. * Test cases for SiteNotification and NonPersistentStorage * Remove unnecessary lines * Show a persistent message for invalid project webhook If we can setup a valid webhook, we show a persistent message with an actionable link using our notifications abstraction. At this point, the message is duplicated because we have a "fixed template message" also which is planned to be removed soon. * Improve copy * Allow to hook the initial build from outside Add a new attribute to `trigger_build` to do not execute the task but just return the immutable signature of it, so it can be chained into a bigger task from outside when it's needed to do something else before/after it's execution. * Remove unused basic attribute on trigger build * Split trigger_build to be able to prepare_build first Added a ``ImportWizard.trigger_initial_build`` method to be override from corporate site. Also, instead of using ``trigger_build`` to create the Celery task and call it immediately, split it to use ``prepare_build`` first to create the task and use ``trigger_build`` to effectively triggers it. * Remove now unused signal * Fix Celery signature creation * Fix testcases with basic on trigger_build * Fix mock calls in test cases * Fix test mock check * Use proper context object * Make it compatible with newer Django versions * Py2 and Py3 compatible line * Dismiss the sticky message and stay in the same page * Use Context to render the template * Lint errors fixed * Call the task in a sync way properly * Update common submodule to latest * Define NonPersistentStorage for one-time notifications NonPersistentNotification backed together with SiteNotification class helps us to create one-time site notifications and attach it to a user without the needs of a request/session object. The message/notification is saved into the database as a PersistentMessage and it's marked as read while it's retrived the first time. This system is useful for notifications that need to be attached from a Celery task where we know the user but we don't have the request. * Make string translatable * Use async task to attach webhook * Fix merge conflicts * Recover accidentally deleted line * Fixed linting errors * Replace message_key with reason to be clearer * Rename non persistent storage class * Remove old templates * Make SiteNotification more flexible - render strings messages as Django Templates - accept extra_context for the template - do not crash if the reason is incorrect * Adapt AttachWebhookNotification to the new features * Refactor the task to attach a webhook Instantiate only once the notification and adapt it depending the context. Also, if there are no connected services into our application do not show a message to the user, but log it as a warning. * Test cases for SiteNotification and NonPersistentStorage * Remove unnecessary lines * Show a persistent message for invalid project webhook If we can setup a valid webhook, we show a persistent message with an actionable link using our notifications abstraction. At this point, the message is duplicated because we have a "fixed template message" also which is planned to be removed soon. * Improve copy * Remove fixed template notification about Project.has_valid_webhook --- common | 2 +- media/css/core.css | 4 +- readthedocs/core/utils/__init__.py | 74 +++++++++---- readthedocs/notifications/__init__.py | 3 +- readthedocs/notifications/backends.py | 16 ++- readthedocs/notifications/constants.py | 17 +++ readthedocs/notifications/notification.py | 102 +++++++++++++++++- readthedocs/notifications/storages.py | 97 ++++++++++++++++- readthedocs/oauth/notifications.py | 54 ++++++++++ readthedocs/oauth/tasks.py | 68 ++++++++++++ readthedocs/oauth/utils.py | 47 -------- readthedocs/projects/signals.py | 13 +-- readthedocs/projects/views/private.py | 29 +++-- readthedocs/rtd_tests/base.py | 6 +- .../rtd_tests/tests/test_core_utils.py | 56 +++++----- .../rtd_tests/tests/test_notifications.py | 98 ++++++++++++++++- .../rtd_tests/tests/test_project_views.py | 2 +- readthedocs/settings/dev.py | 1 + .../templates/core/project_bar_base.html | 14 --- 19 files changed, 557 insertions(+), 146 deletions(-) create mode 100644 readthedocs/oauth/notifications.py diff --git a/common b/common index ed81bfc2608..f9a4b53a4af 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit ed81bfc2608fe23b82a22a55d8431a01762e2f02 +Subproject commit f9a4b53a4af21d22e84fa607cd8448e9f68fc8fc diff --git a/media/css/core.css b/media/css/core.css index 91bddf4036d..1c71374ed72 100644 --- a/media/css/core.css +++ b/media/css/core.css @@ -610,7 +610,9 @@ p.build-missing { font-size: .8em; color: #9d9a55; margin: 0 0 3px; } /* notification box */ .notification { padding: 5px 0; color: #a55; } .notification-20, -.notification-25 { +.notification-25, +.notification-101, +.notification-102 { color: #5a5; } diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 196131992d9..3b8b346ac11 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Common utilty functions.""" from __future__ import absolute_import @@ -16,7 +17,7 @@ from future.backports.urllib.parse import urlparse from celery import group, chord -from readthedocs.builds.constants import LATEST +from readthedocs.builds.constants import LATEST, BUILD_STATE_TRIGGERED from readthedocs.doc_builder.constants import DOCKER_LIMITS @@ -75,38 +76,47 @@ def cname_to_slug(host): return slug -def trigger_build(project, version=None, record=True, force=False, basic=False): +def prepare_build( + project, version=None, record=True, force=False, immutable=True): """ - Trigger build for project and version. + Prepare a build in a Celery task for project and version. - If project has a ``build_queue``, execute task on this build queue. Queue - will be prefixed with ``build-`` to unify build queue names. + If project has a ``build_queue``, execute the task on this build queue. If + project has ``skip=True``, the build is not triggered. + + :param project: project's documentation to be built + :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 + :param immutable: whether or not create an immutable Celery signature + :returns: Celery signature of UpdateDocsTask to be executed """ # Avoid circular import from readthedocs.projects.tasks import UpdateDocsTask from readthedocs.builds.models import Build if project.skip: + log.info( + 'Build not triggered because Project.skip=True: project=%s', + project.slug, + ) return None if not version: version = project.versions.get(slug=LATEST) - kwargs = dict( - pk=project.pk, - version_pk=version.pk, - record=record, - force=force, - basic=basic, - ) + kwargs = { + 'version_pk': version.pk, + 'record': record, + 'force': force, + } - build = None if record: build = Build.objects.create( project=project, version=version, type='html', - state='triggered', + state=BUILD_STATE_TRIGGERED, success=True, ) kwargs['build_pk'] = build.pk @@ -121,16 +131,44 @@ def trigger_build(project, version=None, record=True, force=False, basic=False): if project.container_time_limit: time_limit = int(project.container_time_limit) except ValueError: - pass + log.warning('Invalid time_limit for project: %s', project.slug) + # Add 20% overhead to task, to ensure the build can timeout and the task # will cleanly finish. options['soft_time_limit'] = time_limit options['time_limit'] = int(time_limit * 1.2) - update_docs = UpdateDocsTask() - update_docs.apply_async(kwargs=kwargs, **options) + update_docs_task = UpdateDocsTask() + + # Py 2.7 doesn't support ``**`` expand syntax twice. We create just one big + # kwargs (including the options) for this and expand it just once. + # return update_docs_task.si(project.pk, **kwargs, **options) + kwargs.update(options) + + return update_docs_task.si(project.pk, **kwargs) - return build + +def trigger_build(project, version=None, record=True, force=False): + """ + Trigger a Build. + + Helper that calls ``prepare_build`` and just effectively trigger the Celery + task to be executed by a worker. + + :param project: project's documentation to be built + :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: Celery AsyncResult promise + """ + update_docs_task = prepare_build( + project, + version, + record, + force, + immutable=True, + ) + return update_docs_task.apply_async() def send_email(recipient, subject, template, template_html, context=None, diff --git a/readthedocs/notifications/__init__.py b/readthedocs/notifications/__init__.py index 518d5db8410..c1860cbc8d1 100644 --- a/readthedocs/notifications/__init__.py +++ b/readthedocs/notifications/__init__.py @@ -12,11 +12,12 @@ .. _`django-messages-extends`: https://github.com /AliLozano/django-messages-extends/ """ -from .notification import Notification +from .notification import Notification, SiteNotification from .backends import send_notification __all__ = ( 'Notification', + 'SiteNotification', 'send_notification' ) diff --git a/readthedocs/notifications/backends.py b/readthedocs/notifications/backends.py index bb7f6ce51f5..28529f1f1a6 100644 --- a/readthedocs/notifications/backends.py +++ b/readthedocs/notifications/backends.py @@ -32,7 +32,11 @@ def send_notification(request, notification): backends = getattr(settings, 'NOTIFICATION_BACKENDS', []) for cls_name in backends: backend = import_string(cls_name)(request) - backend.send(notification) + # Do not send email notification if defined explicitly + if backend.name == EmailBackend.name and not notification.send_email: + pass + else: + backend.send(notification) class Backend(object): @@ -96,8 +100,16 @@ def send(self, notification): req = HttpRequest() setattr(req, 'session', '') storage = cls(req) + + # Use the method defined by the notification or map a simple level to a + # persistent one otherwise + if hasattr(notification, 'get_message_level'): + level = notification.get_message_level() + else: + level = LEVEL_MAPPING.get(notification.level, INFO_PERSISTENT) + storage.add( - level=LEVEL_MAPPING.get(notification.level, INFO_PERSISTENT), + level=level, message=notification.render( backend_name=self.name, source_format=HTML, diff --git a/readthedocs/notifications/constants.py b/readthedocs/notifications/constants.py index d1c77412faf..d15efa98448 100644 --- a/readthedocs/notifications/constants.py +++ b/readthedocs/notifications/constants.py @@ -18,3 +18,20 @@ REQUIREMENT: message_constants.WARNING_PERSISTENT, ERROR: message_constants.ERROR_PERSISTENT, } + + +# Message levels to save the message into the database and mark as read +# immediately after retrieved (one-time shown message) +DEBUG_NON_PERSISTENT = 100 +INFO_NON_PERSISTENT = 101 +SUCCESS_NON_PERSISTENT = 102 +WARNING_NON_PERSISTENT = 103 +ERROR_NON_PERSISTENT = 104 + +NON_PERSISTENT_MESSAGE_LEVELS = ( + DEBUG_NON_PERSISTENT, + INFO_NON_PERSISTENT, + SUCCESS_NON_PERSISTENT, + WARNING_NON_PERSISTENT, + ERROR_NON_PERSISTENT, +) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 261b0965b30..cd9a96ed3dd 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -1,16 +1,22 @@ +# -*- coding: utf-8 -*- """Support for templating of notifications.""" from __future__ import absolute_import from builtins import object +import logging from django.conf import settings from django.template import Template, Context from django.template.loader import render_to_string from django.db import models +from django.http import HttpRequest from .backends import send_notification from . import constants +log = logging.getLogger(__name__) + + class Notification(object): """ @@ -28,6 +34,7 @@ class Notification(object): level = constants.INFO subject = None user = None + send_email = True def __init__(self, context_object, request, user=None): self.object = context_object @@ -45,8 +52,8 @@ def get_context_data(self): self.context_object_name: self.object, 'request': self.request, 'production_uri': '{scheme}://{host}'.format( - scheme='https', host=settings.PRODUCTION_DOMAIN - ) + scheme='https', host=settings.PRODUCTION_DOMAIN, + ), } def get_template_names(self, backend_name, source_format=constants.HTML): @@ -71,7 +78,7 @@ def render(self, backend_name, source_format=constants.HTML): backend_name=backend_name, source_format=source_format, ), - context=Context(self.get_context_data()), + context=self.get_context_data(), ) def send(self): @@ -84,3 +91,92 @@ def send(self): avoided. """ send_notification(self.request, self) + + +class SiteNotification(Notification): + + """ + Simple notification to show *only* on site messages. + + ``success_message`` and ``failure_message`` can be a simple string or a + dictionary with different messages depending on the reason of the failure / + success. The message is selected by using ``reason`` to get the proper + value. + + The notification is tied to the ``user`` and it could be sticky, persistent + or normal --this depends on the ``success_level`` and ``failure_level``. + + .. note:: + + ``send_email`` is forced to False to not send accidental emails when + only a simple site notification is needed. + """ + + send_email = False + + success_message = None + failure_message = None + + success_level = constants.SUCCESS_NON_PERSISTENT + failure_level = constants.ERROR_NON_PERSISTENT + + def __init__( + self, user, success, reason=None, context_object=None, + request=None, extra_context=None): + self.object = context_object + + self.user = user or request.user + # Fake the request in case the notification is instantiated from a place + # without access to the request object (Celery task, for example) + self.request = request or HttpRequest() + self.request.user = user + + self.success = success + self.reason = reason + self.extra_context = extra_context or {} + super(SiteNotification, self).__init__(context_object, request, user) + + def get_context_data(self): + context = super(SiteNotification, self).get_context_data() + context.update(self.extra_context) + return context + + def get_message_level(self): + if self.success: + return self.success_level + return self.failure_level + + def get_message(self, success): + if success: + message = self.success_message + else: + message = self.failure_message + + msg = '' # default message in case of error + if isinstance(message, dict): + if self.reason: + if self.reason in message: + msg = message.get(self.reason) + else: + # log the error but not crash + log.error( + "Notification {} has no key '{}' for {} messages".format( + self.__class__.__name__, + self.reason, + 'success' if self.success else 'failure', + ), + ) + else: + log.error( + '{}.{}_message is a dictionary but no reason was provided'.format( + self.__class__.__name__, + 'success' if self.success else 'failure', + ), + ) + else: + msg = message + + return Template(msg).render(context=Context(self.get_context_data())) + + def render(self, *args, **kwargs): # pylint: disable=arguments-differ + return self.get_message(self.success) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py index e94f46e4fbe..004a135284d 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -1,12 +1,21 @@ +# -*- coding: utf-8 -*- """Customised storage for notifications.""" from __future__ import absolute_import from django.contrib.messages.storage.base import Message +from django.db.models import Q from django.utils.safestring import mark_safe -from messages_extends.storages import FallbackStorage +from messages_extends.storages import FallbackStorage, PersistentStorage from messages_extends.models import Message as PersistentMessage from messages_extends.constants import PERSISTENT_MESSAGE_LEVELS +try: + from django.utils import timezone +except ImportError: + from datetime import datetime as timezone + +from .constants import NON_PERSISTENT_MESSAGE_LEVELS + class FallbackUniqueStorage(FallbackStorage): @@ -29,17 +38,26 @@ class FallbackUniqueStorage(FallbackStorage): render the text in templates. """ + storages_names = ( + 'readthedocs.notifications.storages.NonPersistentStorage', + 'messages_extends.storages.StickyStorage', + 'messages_extends.storages.PersistentStorage', + 'django.contrib.messages.storage.cookie.CookieStorage', + 'django.contrib.messages.storage.session.SessionStorage', + ) + def _get(self, *args, **kwargs): # The database backend for persistent messages doesn't support setting # messages with ``mark_safe``, therefore, we need to do it broadly here. messages, all_ret = (super(FallbackUniqueStorage, self) ._get(self, *args, **kwargs)) + safe_messages = [] for message in messages: # Handle all message types, if the message is persistent, take # special action. As the default message handler, this will also # process ephemeral messages - if message.level in PERSISTENT_MESSAGE_LEVELS: + if message.level in PERSISTENT_MESSAGE_LEVELS + NON_PERSISTENT_MESSAGE_LEVELS: message_pk = message.pk message = Message(message.level, mark_safe(message.message), @@ -59,3 +77,78 @@ def add(self, level, message, extra_tags='', *args, **kwargs): # noqa return super(FallbackUniqueStorage, self).add(level, message, extra_tags, *args, **kwargs) + + +class NonPersistentStorage(PersistentStorage): + + """ + Save one time (non-pesistent) messages in the database. + + Messages are saved into the database. ``user`` object is mandatory but + ``request`` is needed. + """ + + # Non-persistent level numbers start at 100 and it's used to check if it's + # an actionable message or not + level = 100 + + def _message_queryset(self, include_read=False): + """Return a queryset of non persistent messages for the request user.""" + expire = timezone.now() + + qs = PersistentMessage.objects.\ + filter(user=self.get_user()).\ + filter(Q(expires=None) | Q(expires__gt=expire)).\ + filter(level__in=NON_PERSISTENT_MESSAGE_LEVELS) + if not include_read: + qs = qs.exclude(read=True) + + # We need to save the objects returned by the query before updating it, + # otherwise they are marked as ``read`` and not returned when executed + result = list(qs) + + # Mark non-persistent messages as read when show them + qs.update(read=True) + + return result + + # These methods (_store, process_message) are copied from + # https://github.com/AliLozano/django-messages-extends/blob/master/messages_extends/storages.py + # and replaced PERSISTENT_MESSAGE_LEVELS by NON_PERSISTENT_MESSAGE_LEVELS + + def _store(self, messages, response, *args, **kwargs): + # There are alredy saved. + return [ + message for message in messages + if message.level not in NON_PERSISTENT_MESSAGE_LEVELS + ] + + def process_message(self, message, *args, **kwargs): + """ + Convert messages to models and save them. + + If its level is into non-persistent levels, convert the message to + models and save it + """ + if message.level not in NON_PERSISTENT_MESSAGE_LEVELS: + return message + + user = kwargs.get("user") or self.get_user() + + try: + anonymous = user.is_anonymous() + except TypeError: + anonymous = user.is_anonymous + if anonymous: + raise NotImplementedError( + 'Persistent message levels cannot be used for anonymous users.') + message_persistent = PersistentMessage() + message_persistent.level = message.level + message_persistent.message = message.message + message_persistent.extra_tags = message.extra_tags + message_persistent.user = user + + if "expires" in kwargs: + message_persistent.expires = kwargs["expires"] + message_persistent.save() + return None diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py new file mode 100644 index 00000000000..0a457873702 --- /dev/null +++ b/readthedocs/oauth/notifications.py @@ -0,0 +1,54 @@ +# -*- coding: utf-8 -*- +from __future__ import division, print_function, unicode_literals + +from django.core.urlresolvers import reverse +from django.utils.translation import ugettext_lazy as _ +from messages_extends.constants import ERROR_PERSISTENT + +from readthedocs.notifications import SiteNotification + + +class AttachWebhookNotification(SiteNotification): + + # Fail reasons + NO_PERMISSIONS = 'no_permissions' + NO_ACCOUNTS = 'no_accounts' + + context_object_name = 'provider' + success_message = _('Webhook successfully added.') + failure_message = { + NO_PERMISSIONS: _('Could not add webhook for {{ project.name }}. Make sure you have the correct {{ provider.name }} permissions.'), # noqa + NO_ACCOUNTS: _('Could not add webhook for {{ project.name }}. Please connect your {{ provider.name }} account.'), # noqa + } + + def get_context_data(self): + context = super(AttachWebhookNotification, self).get_context_data() + project = self.extra_context.get('project') + context.update({ + 'url_connect_account': reverse( + 'projects_integrations', + args=[project.slug], + ), + 'url_docs_webhook': 'http://docs.readthedocs.io/en/latest/webhooks.html', # noqa + }) + return context + + +class InvalidProjectWebhookNotification(SiteNotification): + + context_object_name = 'project' + failure_level = ERROR_PERSISTENT + failure_message = _( + "The project {{ project.name }} doesn't have a valid webhook set up, " + "commits won't trigger new builds for this project. " + "See the project integrations for more information.") # noqa + + def get_context_data(self): + context = super(InvalidProjectWebhookNotification, self).get_context_data() + context.update({ + 'url_integrations': reverse( + 'projects_integrations', + args=[self.object.slug], + ), + }) + return context diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 7c300f947c4..2ad30f8e4a6 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -4,13 +4,22 @@ from __future__ import ( absolute_import, division, print_function, unicode_literals) +import logging + +from allauth.socialaccount.providers import registry as allauth_registry from django.contrib.auth.models import User from readthedocs.core.utils.tasks import ( PublicTask, permission_check, user_id_matches) +from readthedocs.oauth.notifications import ( + AttachWebhookNotification, InvalidProjectWebhookNotification) +from readthedocs.projects.models import Project +from readthedocs.worker import app from .services import registry +log = logging.getLogger(__name__) + @permission_check(user_id_matches) class SyncRemoteRepositories(PublicTask): @@ -27,3 +36,62 @@ def run_public(self, user_id): sync_remote_repositories = SyncRemoteRepositories() + + +@app.task(queue='web') +def attach_webhook(project_pk, user_pk): + """ + Add post-commit hook on project import. + + This is a brute force approach to add a webhook to a repository. We try + all accounts until we set up a webhook. This should remain around for legacy + connections -- that is, projects that do not have a remote repository them + and were not set up with a VCS provider. + """ + project = Project.objects.get(pk=project_pk) + user = User.objects.get(pk=user_pk) + project_notification = InvalidProjectWebhookNotification( + context_object=project, + user=user, + success=False, + ) + + for service_cls in registry: + if service_cls.is_project_service(project): + service = service_cls + break + else: + log.warning('There are no registered services in the application.') + project_notification.send() + return None + + provider = allauth_registry.by_id(service.adapter.provider_id) + notification = AttachWebhookNotification( + context_object=provider, + extra_context={'project': project}, + user=user, + success=None, + ) + + user_accounts = service.for_user(user) + for account in user_accounts: + success, __ = account.setup_webhook(project) + if success: + notification.success = True + notification.send() + + project.has_valid_webhook = True + project.save() + return True + + # No valid account found + if user_accounts: + notification.success = False + notification.reason = AttachWebhookNotification.NO_PERMISSIONS + else: + notification.success = False + notification.reason = AttachWebhookNotification.NO_ACCOUNTS + + project_notification.send() + notification.send() + return False diff --git a/readthedocs/oauth/utils.py b/readthedocs/oauth/utils.py index fbb47916b5e..64e7dc7ed07 100644 --- a/readthedocs/oauth/utils.py +++ b/readthedocs/oauth/utils.py @@ -23,53 +23,6 @@ } -def attach_webhook(project, request=None): - """ - Add post-commit hook on project import. - - This is a brute force approach to adding a webhook to a repository. We try - all accounts until we set up a webhook. This should remain around for legacy - connections -- that is, projects that do not have a remote repository them - and were not set up with a VCS provider. - """ - for service_cls in registry: - if service_cls.is_project_service(project): - service = service_cls - break - else: - messages.error( - request, - _( - 'Webhook activation failed. ' - 'There are no connected services for this project.')) - return None - - user_accounts = service.for_user(request.user) - for account in user_accounts: - success, __ = account.setup_webhook(project) - if success: - messages.success(request, _('Webhook activated')) - project.has_valid_webhook = True - project.save() - return True - # No valid account found - 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(request).get_provider().name))) - return False - - def update_webhook(project, integration, request=None): """Update a specific project integration instead of brute forcing.""" service_cls = SERVICE_MAP.get(integration.integration_type) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 21cdf88a19d..6ef49f9e67c 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -1,10 +1,8 @@ +# -*- coding: utf-8 -*- """Project signals""" from __future__ import absolute_import import django.dispatch -from django.dispatch import receiver - -from readthedocs.oauth.utils import attach_webhook before_vcs = django.dispatch.Signal(providing_args=["version"]) @@ -16,12 +14,3 @@ project_import = django.dispatch.Signal(providing_args=["project"]) files_changed = django.dispatch.Signal(providing_args=["project", "files"]) - - -@receiver(project_import) -def handle_project_import(sender, **kwargs): - """Add post-commit hook on project import""" - project = sender - request = kwargs.get('request') - - attach_webhook(project=project, request=request) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 50f26e1969b..1dc197096eb 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -7,6 +7,7 @@ import logging from allauth.socialaccount.models import SocialAccount +from celery import chain from django.contrib import messages from django.contrib.auth.decorators import login_required from django.contrib.auth.models import User @@ -24,10 +25,11 @@ from readthedocs.builds.forms import AliasForm, VersionForm from readthedocs.builds.models import Version, VersionAlias from readthedocs.core.mixins import ListViewWithForm, LoginRequiredMixin -from readthedocs.core.utils import broadcast, trigger_build +from readthedocs.core.utils import broadcast, trigger_build, prepare_build from readthedocs.integrations.models import HttpExchange, Integration from readthedocs.oauth.services import registry -from readthedocs.oauth.utils import attach_webhook, update_webhook +from readthedocs.oauth.utils import update_webhook +from readthedocs.oauth.tasks import attach_webhook from readthedocs.projects import tasks from readthedocs.projects.forms import ( DomainForm, EmailHookForm, IntegrationForm, ProjectAdvancedForm, @@ -231,13 +233,25 @@ def done(self, form_list, **kwargs): for field, value in list(form_data.items()): if field in extra_fields: setattr(project, field, value) - basic_only = True project.save() + + # TODO: this signal could be removed, or used for sync task project_import.send(sender=project, request=self.request) - trigger_build(project, basic=basic_only) + + self.trigger_initial_build(project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug])) + def trigger_initial_build(self, project): + """Trigger initial build.""" + update_docs = prepare_build(project) + task_promise = chain( + attach_webhook.si(project.pk, self.request.user.pk), + update_docs, + ) + async_result = task_promise.apply_async() + return async_result + def is_advanced(self): """Determine if the user selected the `show advanced` field.""" data = self.get_cleaned_data_for_step('basics') or {} @@ -271,7 +285,7 @@ def get(self, request, *args, **kwargs): if form.is_valid(): project = form.save() project.save() - trigger_build(project, basic=True) + trigger_build(project) messages.success( request, _('Your demo project is currently being imported')) else: @@ -777,7 +791,10 @@ def post(self, request, *args, **kwargs): # This is a brute force form of the webhook sync, if a project has a # webhook or a remote repository object, the user should be using # the per-integration sync instead. - attach_webhook(project=self.get_project(), request=request) + attach_webhook( + project_pk=self.get_project().pk, + user_pk=request.user.pk, + ) return HttpResponseRedirect(self.get_success_url()) def get_success_url(self): diff --git a/readthedocs/rtd_tests/base.py b/readthedocs/rtd_tests/base.py index 2027fd35146..e226ea69884 100644 --- a/readthedocs/rtd_tests/base.py +++ b/readthedocs/rtd_tests/base.py @@ -33,8 +33,8 @@ def tearDown(self): settings.DOCROOT = self.original_DOCROOT -@patch('readthedocs.projects.views.private.trigger_build', lambda x, basic: None) -@patch('readthedocs.projects.views.private.trigger_build', lambda x, basic: None) +@patch('readthedocs.projects.views.private.trigger_build', lambda x: None) +@patch('readthedocs.projects.views.private.trigger_build', lambda x: None) class MockBuildTestCase(TestCase): """Mock build triggers for test cases.""" @@ -97,7 +97,7 @@ class WizardTestCase(RequestFactoryTestMixin, TestCase): wizard_class_slug = None wizard_class = None - @patch('readthedocs.projects.views.private.trigger_build', lambda x, basic: None) + @patch('readthedocs.projects.views.private.trigger_build', lambda x: None) def post_step(self, step, **kwargs): """ Post step form data to `url`, using supplementary `kwargs` diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index dc3424b93e8..a009982b723 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -5,7 +5,6 @@ from django_dynamic_fixture import get from django.test import TestCase -from django.test.utils import override_settings from readthedocs.projects.models import Project from readthedocs.builds.models import Version @@ -22,63 +21,58 @@ def setUp(self): def test_trigger_build_time_limit(self, update_docs): """Pass of time limit""" trigger_build(project=self.project, version=self.version) - update_docs().apply_async.assert_has_calls([ + update_docs().si.assert_has_calls([ mock.call( + self.project.pk, time_limit=720, soft_time_limit=600, queue=mock.ANY, - kwargs={ - 'pk': self.project.id, - 'force': False, - 'basic': False, - 'record': True, - 'build_pk': mock.ANY, - 'version_pk': self.version.id - } - ) + force=False, + record=True, + build_pk=mock.ANY, + version_pk=self.version.id, + ), ]) + update_docs().si().apply_async.assert_called() @mock.patch('readthedocs.projects.tasks.UpdateDocsTask') def test_trigger_build_invalid_time_limit(self, update_docs): """Time limit as string""" self.project.container_time_limit = '200s' trigger_build(project=self.project, version=self.version) - update_docs().apply_async.assert_has_calls([ + update_docs().si.assert_has_calls([ mock.call( + self.project.pk, time_limit=720, soft_time_limit=600, queue=mock.ANY, - kwargs={ - 'pk': self.project.id, - 'force': False, - 'basic': False, - 'record': True, - 'build_pk': mock.ANY, - 'version_pk': self.version.id - } - ) + force=False, + record=True, + build_pk=mock.ANY, + version_pk=self.version.id, + ), ]) + update_docs().si().apply_async.assert_called() @mock.patch('readthedocs.projects.tasks.UpdateDocsTask') def test_trigger_build_rounded_time_limit(self, update_docs): """Time limit should round down""" self.project.container_time_limit = 3 trigger_build(project=self.project, version=self.version) - update_docs().apply_async.assert_has_calls([ + update_docs().si.assert_has_calls([ mock.call( + self.project.pk, time_limit=3, soft_time_limit=3, queue=mock.ANY, - kwargs={ - 'pk': self.project.id, - 'force': False, - 'basic': False, - 'record': True, - 'build_pk': mock.ANY, - 'version_pk': self.version.id - } - ) + force=False, + record=True, + build_pk=mock.ANY, + version_pk=self.version.id, + ), ]) + update_docs().si().apply_async.assert_called() + def test_slugify(self): """Test additional slugify""" diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index cdc2fbc5c71..b80035be01f 100644 --- a/readthedocs/rtd_tests/tests/test_notifications.py +++ b/readthedocs/rtd_tests/tests/test_notifications.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Notification tests""" from __future__ import absolute_import @@ -8,17 +9,17 @@ from django.contrib.auth.models import User, AnonymousUser from messages_extends.models import Message as PersistentMessage -from readthedocs.notifications import Notification +from readthedocs.notifications import Notification, SiteNotification from readthedocs.notifications.backends import EmailBackend, SiteBackend -from readthedocs.notifications.constants import ERROR +from readthedocs.notifications.constants import ERROR, INFO_NON_PERSISTENT, WARNING_NON_PERSISTENT from readthedocs.builds.models import Build @override_settings( NOTIFICATION_BACKENDS=[ 'readthedocs.notifications.backends.EmailBackend', - 'readthedocs.notifications.backends.SiteBackend' - ] + 'readthedocs.notifications.backends.SiteBackend', + ], ) @mock.patch('readthedocs.notifications.notification.render_to_string') @mock.patch.object(Notification, 'send') @@ -131,3 +132,92 @@ class TestNotification(Notification): self.assertEqual(render_to_string.call_count, 1) self.assertEqual(PersistentMessage.objects.count(), 0) + + @mock.patch('readthedocs.notifications.backends.send_email') + def test_non_persistent_message(self, send_email, render_to_string): + render_to_string.return_value = 'Test' + + class TestNotification(SiteNotification): + name = 'foo' + success_message = 'Test success message' + success_level = INFO_NON_PERSISTENT + + user = fixture.get(User) + n = TestNotification(user, True) + backend = SiteBackend(request=None) + + self.assertEqual(PersistentMessage.objects.count(), 0) + backend.send(n) + # No email is sent for non persistent messages + send_email.assert_not_called() + self.assertEqual(PersistentMessage.objects.count(), 1) + self.assertEqual(PersistentMessage.objects.filter(read=False).count(), 1) + + self.client.force_login(user) + response = self.client.get('/') + self.assertContains(response, 'Test success message') + self.assertEqual(PersistentMessage.objects.count(), 1) + self.assertEqual(PersistentMessage.objects.filter(read=True).count(), 1) + + response = self.client.get('/') + self.assertNotContains(response, 'Test success message') + + +@override_settings(PRODUCTION_DOMAIN='readthedocs.org') +class SiteNotificationTests(TestCase): + + class TestSiteNotification(SiteNotification): + name = 'foo' + success_message = 'simple success message' + failure_message = { + 1: 'simple failure message', + 2: '{{ object.name }} object name', + 'three': '{{ object.name }} and {{ other.name }} render', + } + success_level = INFO_NON_PERSISTENT + failure_level = WARNING_NON_PERSISTENT + + def setUp(self): + self.user = fixture.get(User) + self.context = {'other': {'name': 'other name'}} + self.n = self.TestSiteNotification( + self.user, + True, + context_object={'name': 'object name'}, + extra_context=self.context, + ) + + def test_context_data(self): + context = { + 'object': {'name': 'object name'}, + 'request': None, + 'production_uri': 'https://readthedocs.org', + 'other': {'name': 'other name'}, + } + self.assertEqual(self.n.get_context_data(), context) + + def test_message_level(self): + self.n.success = True + self.assertEqual(self.n.get_message_level(), INFO_NON_PERSISTENT) + + self.n.success = False + self.assertEqual(self.n.get_message_level(), WARNING_NON_PERSISTENT) + + def test_message(self): + self.n.reason = 1 + self.assertEqual(self.n.get_message(True), 'simple success message') + self.n.reason = 'three' + self.assertEqual(self.n.get_message(True), 'simple success message') + + self.n.reason = 1 + self.assertEqual(self.n.get_message(False), 'simple failure message') + self.n.reason = 2 + self.assertEqual(self.n.get_message(False), 'object name object name') + self.n.reason = 'three' + self.assertEqual(self.n.get_message(False), 'object name and other name render') + + # Invalid reason + self.n.reason = None + with mock.patch('readthedocs.notifications.notification.log') as mock_log: + self.assertEqual(self.n.get_message(False), '') + mock_log.error.assert_called_once() diff --git a/readthedocs/rtd_tests/tests/test_project_views.py b/readthedocs/rtd_tests/tests/test_project_views.py index 313a38e6343..a4ca3f02a87 100644 --- a/readthedocs/rtd_tests/tests/test_project_views.py +++ b/readthedocs/rtd_tests/tests/test_project_views.py @@ -25,7 +25,7 @@ from readthedocs.projects import tasks -@patch('readthedocs.projects.views.private.trigger_build', lambda x, basic: None) +@patch('readthedocs.projects.views.private.trigger_build', lambda x: None) class TestProfileMiddleware(RequestFactoryTestMixin, TestCase): wizard_class_slug = 'import_wizard_view' diff --git a/readthedocs/settings/dev.py b/readthedocs/settings/dev.py index 6ba56f5df8b..7fa4dafe959 100644 --- a/readthedocs/settings/dev.py +++ b/readthedocs/settings/dev.py @@ -36,6 +36,7 @@ def DATABASES(self): # noqa CELERY_RESULT_BACKEND = 'redis://localhost:6379/0' CELERY_RESULT_SERIALIZER = 'json' CELERY_ALWAYS_EAGER = True + CELERY_TASK_IGNORE_RESULT = False EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend' FILE_SYNCER = 'readthedocs.builds.syncers.LocalSyncer' diff --git a/readthedocs/templates/core/project_bar_base.html b/readthedocs/templates/core/project_bar_base.html index c7063a8dc53..aad9c0fecac 100644 --- a/readthedocs/templates/core/project_bar_base.html +++ b/readthedocs/templates/core/project_bar_base.html @@ -26,20 +26,6 @@

- {% if not project.has_valid_webhook and request.user|is_admin:project %} -

- {% url "projects_integrations" project.slug as integrations_url %} - {% blocktrans %} - This repository doesn't have a valid webhook set up, - commits won't trigger new builds for this project. - {% endblocktrans %} -
- {% blocktrans %} - See your project integrations for more information. - {% endblocktrans %} -

- {% endif %} - {% if project.skip %}

{% blocktrans %}