From 35825848da1fc38adede8f19558895529cc04cbd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 19:43:59 -0500 Subject: [PATCH 01/65] Remove unused basic attribute on trigger build --- readthedocs/core/utils/__init__.py | 3 +-- readthedocs/projects/views/private.py | 5 ++--- readthedocs/rtd_tests/tests/test_core_utils.py | 3 --- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 196131992d9..04cfc9b145b 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -75,7 +75,7 @@ def cname_to_slug(host): return slug -def trigger_build(project, version=None, record=True, force=False, basic=False): +def trigger_build(project, version=None, record=True, force=False): """ Trigger build for project and version. @@ -97,7 +97,6 @@ def trigger_build(project, version=None, record=True, force=False, basic=False): version_pk=version.pk, record=record, force=force, - basic=basic, ) build = None diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 50f26e1969b..a41759c043b 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -231,10 +231,9 @@ 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() project_import.send(sender=project, request=self.request) - trigger_build(project, basic=basic_only) + trigger_build(project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug])) @@ -271,7 +270,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: diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index dc3424b93e8..9e7828c4828 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -30,7 +30,6 @@ def test_trigger_build_time_limit(self, update_docs): kwargs={ 'pk': self.project.id, 'force': False, - 'basic': False, 'record': True, 'build_pk': mock.ANY, 'version_pk': self.version.id @@ -51,7 +50,6 @@ def test_trigger_build_invalid_time_limit(self, update_docs): kwargs={ 'pk': self.project.id, 'force': False, - 'basic': False, 'record': True, 'build_pk': mock.ANY, 'version_pk': self.version.id @@ -72,7 +70,6 @@ def test_trigger_build_rounded_time_limit(self, update_docs): kwargs={ 'pk': self.project.id, 'force': False, - 'basic': False, 'record': True, 'build_pk': mock.ANY, 'version_pk': self.version.id From 10def530bf31398c6d7d001706f148a2c9e06b2b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 20:11:34 -0500 Subject: [PATCH 02/65] 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. --- common | 2 +- readthedocs/core/utils/__init__.py | 68 ++++++++++++++++++++------- readthedocs/projects/views/private.py | 8 +++- 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/common b/common index ed81bfc2608..650bf3ad09f 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit ed81bfc2608fe23b82a22a55d8431a01762e2f02 +Subproject commit 650bf3ad09f3536a98306a193b2c57e4ba35a7e4 diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 04cfc9b145b..27de434c172 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,37 +76,48 @@ def cname_to_slug(host): return slug -def trigger_build(project, version=None, record=True, force=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 the task on this build queue. If + project has ``skip=True``, the build is not triggered. - If project has a ``build_queue``, execute task on this build queue. Queue - will be prefixed with ``build-`` to unify build queue names. + :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, - ) + kwargs = { + 'pk': project.pk, + '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 @@ -120,16 +132,38 @@ def trigger_build(project, version=None, record=True, force=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() + return update_docs_task.si(kwargs=kwargs, **options) + - 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/projects/views/private.py b/readthedocs/projects/views/private.py index a41759c043b..41441697023 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -233,10 +233,16 @@ def done(self, form_list, **kwargs): setattr(project, field, value) project.save() project_import.send(sender=project, request=self.request) - trigger_build(project) + self.trigger_initial_build(project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug])) + def trigger_initial_build(self, project): + """ + Trigger initial build. + """ + return trigger_build(project) + def is_advanced(self): """Determine if the user selected the `show advanced` field.""" data = self.get_cleaned_data_for_step('basics') or {} From c2c10af37d6e96a8387d3aed780897418ff5c064 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 26 Apr 2018 19:49:26 -0500 Subject: [PATCH 03/65] 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. --- readthedocs/projects/signals.py | 8 ++++++++ readthedocs/projects/views/private.py | 4 ++++ readthedocs/settings/dev.py | 1 + 3 files changed, 13 insertions(+) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 21cdf88a19d..1744037f155 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -4,6 +4,7 @@ import django.dispatch from django.dispatch import receiver +from readthedocs.core.utils import trigger_build from readthedocs.oauth.utils import attach_webhook @@ -25,3 +26,10 @@ def handle_project_import(sender, **kwargs): request = kwargs.get('request') attach_webhook(project=project, request=request) + + +# TODO: move this to ImportWizardView.trigger_initial_build +# @receiver(project_import) +# def trigger_initial_build(sender, request, **kwargs): +# project = sender +# trigger_build(project) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 41441697023..5e7244e72f3 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -232,7 +232,11 @@ def done(self, form_list, **kwargs): if field in extra_fields: setattr(project, field, value) project.save() + + # TODO: if we want to make the ``attach_webhook`` async, we need to + # consider the message shown to the user when not valid webhook. project_import.send(sender=project, request=self.request) + self.trigger_initial_build(project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug])) 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' From efa2bd3c4ae51b1cd2643112405cc30fe401f19e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 20:19:38 -0500 Subject: [PATCH 04/65] Remove now unused signal --- readthedocs/projects/signals.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 1744037f155..21cdf88a19d 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -4,7 +4,6 @@ import django.dispatch from django.dispatch import receiver -from readthedocs.core.utils import trigger_build from readthedocs.oauth.utils import attach_webhook @@ -26,10 +25,3 @@ def handle_project_import(sender, **kwargs): request = kwargs.get('request') attach_webhook(project=project, request=request) - - -# TODO: move this to ImportWizardView.trigger_initial_build -# @receiver(project_import) -# def trigger_initial_build(sender, request, **kwargs): -# project = sender -# trigger_build(project) From 851e1ff2ad40bd01d39e13148d1deee1cfe3262e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 21:33:22 -0500 Subject: [PATCH 05/65] Fix Celery signature creation --- readthedocs/core/utils/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 27de434c172..8ca3b245652 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -106,7 +106,6 @@ def prepare_build( version = project.versions.get(slug=LATEST) kwargs = { - 'pk': project.pk, 'version_pk': version.pk, 'record': record, 'force': force, @@ -140,7 +139,7 @@ def prepare_build( options['time_limit'] = int(time_limit * 1.2) update_docs_task = UpdateDocsTask() - return update_docs_task.si(kwargs=kwargs, **options) + return update_docs_task.si(project.pk, **kwargs, **options) def trigger_build(project, version=None, record=True, force=False): From 0f8a055fb5ec264bdfd80c8a9f4e2551513bb7fe Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 22:09:54 -0500 Subject: [PATCH 06/65] Fix testcases with basic on trigger_build --- readthedocs/rtd_tests/base.py | 6 +++--- readthedocs/rtd_tests/tests/test_project_views.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) 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_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' From 2fe122021c1835ff8c1bc912259bab7f1acf208b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 22:20:03 -0500 Subject: [PATCH 07/65] Fix mock calls in test cases --- .../rtd_tests/tests/test_core_utils.py | 57 +++++++++---------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index 9e7828c4828..9c5d5c0b46b 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,60 +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, - '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, - '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( - time_limit=3, - soft_time_limit=3, + self.project.pk, + time_limit=720, + soft_time_limit=600, queue=mock.ANY, - kwargs={ - 'pk': self.project.id, - 'force': 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""" From 2d7063bfc4005a77a0b773a91feb44548f9f56b0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 8 May 2018 10:54:56 -0500 Subject: [PATCH 08/65] Fix test mock check --- readthedocs/rtd_tests/tests/test_core_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index 9c5d5c0b46b..a009982b723 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -62,8 +62,8 @@ def test_trigger_build_rounded_time_limit(self, update_docs): update_docs().si.assert_has_calls([ mock.call( self.project.pk, - time_limit=720, - soft_time_limit=600, + time_limit=3, + soft_time_limit=3, queue=mock.ANY, force=False, record=True, From e4412b303b1782f78e1dc300663d10e7e78fdf19 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 16:41:29 -0300 Subject: [PATCH 09/65] Use async task to attach webhook --- readthedocs/oauth/notifications.py | 38 ++++++++++ readthedocs/oauth/tasks.py | 73 +++++++++++++++++++ .../attach_webhook_fail_site.html | 7 ++ .../attach_webhook_success_site.html | 1 + readthedocs/oauth/utils.py | 47 ------------ readthedocs/projects/signals.py | 12 --- readthedocs/projects/views/private.py | 17 +++-- 7 files changed, 131 insertions(+), 64 deletions(-) create mode 100644 readthedocs/oauth/notifications.py create mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html create mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py new file mode 100644 index 00000000000..555559227f5 --- /dev/null +++ b/readthedocs/oauth/notifications.py @@ -0,0 +1,38 @@ +# -*- coding: utf-8 -*- +from __future__ import division, print_function, unicode_literals + +from readthedocs.notifications import Notification +from readthedocs.notifications.constants import HTML, WARNING, INFO + + +class AttachWebhookNotification(Notification): + + name = 'attach_webhook' + context_object_name = 'socialaccount' + subject_success = 'Attach webhook success' + subject_fail = 'Attach webhook failed' + + def __init__(self, context_object, request, user, success, reason=None): + self.success = success + self.reason = reason + if self.success: + self.level = INFO + self.subject = self.subject_success + else: + self.level = WARNING + self.subject = self.subject_fail + + super(AttachWebhookNotification, self).__init__(context_object, request, user) + + def get_context_data(self): + context = super(AttachWebhookNotification, self).get_context_data() + context.update({'reason': self.reason}) + return context + + def get_template_names(self, backend_name, source_format=HTML): + return 'oauth/notifications/{name}_{status}_{backend}.{source_format}'.format( + name=self.name, + status='success' if self.success else 'failed', + backend=backend_name, + source_format=source_format, + ) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 4d0ad6336d4..1e71226e3e5 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -2,10 +2,15 @@ from __future__ import absolute_import from django.contrib.auth.models import User +from django.http import HttpRequest from readthedocs.core.utils.tasks import PublicTask from readthedocs.core.utils.tasks import permission_check from readthedocs.core.utils.tasks import user_id_matches +from readthedocs.oauth.notifications import AttachWebhookNotification +from readthedocs.projects.models import Project +from readthedocs.worker import app + from .services import registry @@ -24,3 +29,71 @@ 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) + for service_cls in registry: + if service_cls.is_project_service(project): + service = service_cls + break + else: + notification = AttachWebhookNotification( + context_object=None, + request=HttpRequest(), + user=user, + success=False, + reason='no_connected_services', + ) + notification.send() + return None + + from celery.contrib import rdb; rdb.set_trace() + + user_accounts = service.for_user(user) + for account in user_accounts: + success, __ = account.setup_webhook(project) + if success: + notification = AttachWebhookNotification( + context_object=None, + request=HttpRequest(), + user=user, + success=True, + reason='no_connected_services', + ) + notification.send() + + project.has_valid_webhook = True + project.save() + return True + + # No valid account found + if user_accounts: + notification = AttachWebhookNotification( + context_object=None, + request=HttpRequest(), + user=user, + success=False, + reason='no_permissions', + ) + notification.send() + else: + notification = AttachWebhookNotification( + context_object=service, + request=HttpRequest(), + user=user, + success=False, + reason='no_accounts', + ) + notification.send() + return False diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html new file mode 100644 index 00000000000..4cbb6ff62cf --- /dev/null +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html @@ -0,0 +1,7 @@ +{% if reason == 'no_connected_projects' %} + Webhook activation failed. There are no connected services for this project. +{% elif reason == 'no_permissions' %} + Webhook activation failed. Make sure you have permissions to set it. +{% elif reason == 'no_accounts' %} + No accounts available to set webhook on. Please connect your {{ provider.name }} account. +{% endif %} diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html new file mode 100644 index 00000000000..3fcc9bdefd8 --- /dev/null +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html @@ -0,0 +1 @@ +Webhook activated. 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..6a6147da871 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -4,7 +4,6 @@ import django.dispatch from django.dispatch import receiver -from readthedocs.oauth.utils import attach_webhook before_vcs = django.dispatch.Signal(providing_args=["version"]) @@ -14,14 +13,3 @@ after_build = django.dispatch.Signal(providing_args=["version"]) 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 5e7244e72f3..524ba2f8b0d 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, @@ -233,8 +235,7 @@ def done(self, form_list, **kwargs): setattr(project, field, value) project.save() - # TODO: if we want to make the ``attach_webhook`` async, we need to - # consider the message shown to the user when not valid webhook. + # TODO: this signal could be removed, or used for sync task project_import.send(sender=project, request=self.request) self.trigger_initial_build(project) @@ -245,7 +246,13 @@ def trigger_initial_build(self, project): """ Trigger initial build. """ - return trigger_build(project) + 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.""" From d5cf6c6d68893517eaaca5d22266d71e28b14f8e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 11:22:39 -0300 Subject: [PATCH 10/65] Use proper context object --- readthedocs/oauth/notifications.py | 4 ++-- readthedocs/oauth/tasks.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 555559227f5..3e82ff3fa1f 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -8,7 +8,7 @@ class AttachWebhookNotification(Notification): name = 'attach_webhook' - context_object_name = 'socialaccount' + context_object_name = 'provider' subject_success = 'Attach webhook success' subject_fail = 'Attach webhook failed' @@ -32,7 +32,7 @@ def get_context_data(self): def get_template_names(self, backend_name, source_format=HTML): return 'oauth/notifications/{name}_{status}_{backend}.{source_format}'.format( name=self.name, - status='success' if self.success else 'failed', + status='success' if self.success else 'fail', backend=backend_name, source_format=source_format, ) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 1e71226e3e5..479505f13b6 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -58,7 +58,6 @@ def attach_webhook(project_pk, user_pk): notification.send() return None - from celery.contrib import rdb; rdb.set_trace() user_accounts = service.for_user(user) for account in user_accounts: @@ -88,8 +87,11 @@ def attach_webhook(project_pk, user_pk): ) notification.send() else: + from allauth.socialaccount.providers import registry as allauth_registry + + provider = allauth_registry.by_id(service.adapter.provider_id) notification = AttachWebhookNotification( - context_object=service, + context_object=provider, request=HttpRequest(), user=user, success=False, From 14c99f3e0e1b54f9f4e59bd4576e0dab17c14d98 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 11:33:35 -0300 Subject: [PATCH 11/65] Make it compatible with newer Django versions --- readthedocs/notifications/notification.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 261b0965b30..21fee20ecaa 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -38,15 +38,15 @@ def __init__(self, context_object, request, user=None): def get_subject(self): template = Template(self.subject) - return template.render(context=Context(self.get_context_data())) + return template.render(context=self.get_context_data()) def get_context_data(self): return { 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 +71,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): From 90f5ea62cdf99139bce1fce7de8f09095734f0ec Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 11:53:46 -0300 Subject: [PATCH 12/65] Py2 and Py3 compatible line --- readthedocs/core/utils/__init__.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 8ca3b245652..3b8b346ac11 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -139,7 +139,13 @@ def prepare_build( options['time_limit'] = int(time_limit * 1.2) update_docs_task = UpdateDocsTask() - return update_docs_task.si(project.pk, **kwargs, **options) + + # 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) def trigger_build(project, version=None, record=True, force=False): From 3cff0a7c9750bf6f10064173a9f112ae5ec1ed2a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:03:41 -0300 Subject: [PATCH 13/65] Dismiss the sticky message and stay in the same page --- .../templates/oauth/notifications/attach_webhook_fail_site.html | 1 + .../oauth/notifications/attach_webhook_success_site.html | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html index 4cbb6ff62cf..e4ddcf1ce6f 100644 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html @@ -5,3 +5,4 @@ {% elif reason == 'no_accounts' %} No accounts available to set webhook on. Please connect your {{ provider.name }} account. {% endif %} +Close. diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html index 3fcc9bdefd8..a92eb8e837d 100644 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html @@ -1 +1 @@ -Webhook activated. +Webhook activated. Close. From da81906892f2a629389614d178e7683691261b6c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:15:29 -0300 Subject: [PATCH 14/65] Use Context to render the template --- readthedocs/notifications/notification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 21fee20ecaa..17badc2d0ed 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -38,7 +38,7 @@ def __init__(self, context_object, request, user=None): def get_subject(self): template = Template(self.subject) - return template.render(context=self.get_context_data()) + return template.render(context=Context(self.get_context_data())) def get_context_data(self): return { From 9cd08f7915d273fba2a43633b89472df340ce9c4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:26:24 -0300 Subject: [PATCH 15/65] Lint errors fixed --- readthedocs/oauth/tasks.py | 1 - readthedocs/projects/signals.py | 1 - readthedocs/projects/views/private.py | 4 +--- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 479505f13b6..56439303ffc 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -58,7 +58,6 @@ def attach_webhook(project_pk, user_pk): notification.send() return None - user_accounts = service.for_user(user) for account in user_accounts: success, __ = account.setup_webhook(project) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 6a6147da871..b45e9e3908e 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -5,7 +5,6 @@ from django.dispatch import receiver - before_vcs = django.dispatch.Signal(providing_args=["version"]) after_vcs = django.dispatch.Signal(providing_args=["version"]) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 524ba2f8b0d..6b1feb5340c 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -243,9 +243,7 @@ def done(self, form_list, **kwargs): reverse('projects_detail', args=[project.slug])) def trigger_initial_build(self, project): - """ - Trigger initial build. - """ + """Trigger initial build.""" update_docs = prepare_build(project) task_promise = chain( attach_webhook.si(project.pk, self.request.user.pk), From 804ef65f056545bd9c4b2d89fc98178326ad8a43 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:26:31 -0300 Subject: [PATCH 16/65] Call the task in a sync way properly --- readthedocs/projects/views/private.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 6b1feb5340c..1dc197096eb 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -791,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): From 8179559c0ce7f947f7c368b7be5fd691a881977d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 14:51:27 -0300 Subject: [PATCH 17/65] Update common submodule to latest --- common | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common b/common index 650bf3ad09f..f9a4b53a4af 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit 650bf3ad09f3536a98306a193b2c57e4ba35a7e4 +Subproject commit f9a4b53a4af21d22e84fa607cd8448e9f68fc8fc From e5fe8f72535936503adeb9d9d67eb46c798f6fd4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 16:30:21 -0300 Subject: [PATCH 18/65] 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. --- media/css/core.css | 4 +- readthedocs/notifications/__init__.py | 3 +- readthedocs/notifications/backends.py | 18 ++++- readthedocs/notifications/constants.py | 17 ++++ readthedocs/notifications/notification.py | 74 +++++++++++++++++ readthedocs/notifications/storages.py | 97 ++++++++++++++++++++++- readthedocs/oauth/notifications.py | 43 +++------- readthedocs/oauth/tasks.py | 16 +--- 8 files changed, 220 insertions(+), 52 deletions(-) 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/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 48bb0dd0556..077a3ac649f 100644 --- a/readthedocs/notifications/backends.py +++ b/readthedocs/notifications/backends.py @@ -29,7 +29,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): @@ -87,11 +91,19 @@ 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 + source_format=HTML, ), extra_tags='', user=notification.user, 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 17badc2d0ed..dae065c6cbd 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Support for templating of notifications.""" from __future__ import absolute_import @@ -6,6 +7,7 @@ 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 @@ -28,6 +30,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 @@ -84,3 +87,74 @@ 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 ``message_key`` 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, message_key=None, context_object=None, + request=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.message_key = message_key + super(SiteNotification, self).__init__(context_object, request, user) + + 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 + + if isinstance(message, dict) and self.message_key: + if self.message_key in message: + msg = message.get(self.message_key) + else: + raise KeyError( + "Notification has no key '{}' for {} messages".format( + self.message_key, + 'success' if self.success else 'failure', + ), + ) + else: + msg = message + return msg.format(**{self.context_object_name: self.object}) + + def render(self, *args, **kwargs): + return self.get_message(self.success) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py index e94f46e4fbe..d779b2158ff 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -1,19 +1,26 @@ +# -*- 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): """ Persistent message fallback storage, but only stores unique notifications. - - This loops through all backends to find messages to store, but will skip this step if the message already exists for the user in the database. Deduplication is important here, as a persistent message may ask a user to @@ -29,17 +36,26 @@ class FallbackUniqueStorage(FallbackStorage): render the text in templates. """ + storages_names = ( + 'readthedocs.notifications.storages.NonPersistentNotification', + '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 +75,76 @@ def add(self, level, message, extra_tags='', *args, **kwargs): # noqa return super(FallbackUniqueStorage, self).add(level, message, extra_tags, *args, **kwargs) + + +class NonPersistentNotification(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 not message.level 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 not message.level 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 index 3e82ff3fa1f..b16869d292c 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,38 +1,19 @@ # -*- coding: utf-8 -*- from __future__ import division, print_function, unicode_literals -from readthedocs.notifications import Notification -from readthedocs.notifications.constants import HTML, WARNING, INFO +from readthedocs.notifications import SiteNotification -class AttachWebhookNotification(Notification): +class AttachWebhookNotification(SiteNotification): - name = 'attach_webhook' - context_object_name = 'provider' - subject_success = 'Attach webhook success' - subject_fail = 'Attach webhook failed' - - def __init__(self, context_object, request, user, success, reason=None): - self.success = success - self.reason = reason - if self.success: - self.level = INFO - self.subject = self.subject_success - else: - self.level = WARNING - self.subject = self.subject_fail - - super(AttachWebhookNotification, self).__init__(context_object, request, user) + NO_CONNECTED_SERVICES = 'no_connected_services' + NO_PERMISSIONS = 'no_permissions' + NO_ACCOUNTS = 'no_accounts' - def get_context_data(self): - context = super(AttachWebhookNotification, self).get_context_data() - context.update({'reason': self.reason}) - return context - - def get_template_names(self, backend_name, source_format=HTML): - return 'oauth/notifications/{name}_{status}_{backend}.{source_format}'.format( - name=self.name, - status='success' if self.success else 'fail', - backend=backend_name, - source_format=source_format, - ) + context_object_name = 'provider' + success_message = 'Webhook activated successfully.' + failure_message = { + NO_CONNECTED_SERVICES: 'Webhook activation failed. There are no connected services for this project.', + NO_PERMISSIONS: 'Webhook activation failed. Make sure you have permissions to set it.', + NO_ACCOUNTS: 'No accounts available to set webhook on. Please connect your {provider.name} account.', + } diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 56439303ffc..98286d9140c 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -1,8 +1,8 @@ +# -*- coding: utf-8 -*- """Tasks for OAuth services""" from __future__ import absolute_import from django.contrib.auth.models import User -from django.http import HttpRequest from readthedocs.core.utils.tasks import PublicTask from readthedocs.core.utils.tasks import permission_check @@ -49,11 +49,9 @@ def attach_webhook(project_pk, user_pk): break else: notification = AttachWebhookNotification( - context_object=None, - request=HttpRequest(), user=user, success=False, - reason='no_connected_services', + message_key=AttachWebhookNotification.NO_CONNECTED_SERVICES, ) notification.send() return None @@ -63,11 +61,8 @@ def attach_webhook(project_pk, user_pk): success, __ = account.setup_webhook(project) if success: notification = AttachWebhookNotification( - context_object=None, - request=HttpRequest(), user=user, success=True, - reason='no_connected_services', ) notification.send() @@ -78,11 +73,9 @@ def attach_webhook(project_pk, user_pk): # No valid account found if user_accounts: notification = AttachWebhookNotification( - context_object=None, - request=HttpRequest(), user=user, success=False, - reason='no_permissions', + message_key=AttachWebhookNotification.NO_PERMISSIONS, ) notification.send() else: @@ -91,10 +84,9 @@ def attach_webhook(project_pk, user_pk): provider = allauth_registry.by_id(service.adapter.provider_id) notification = AttachWebhookNotification( context_object=provider, - request=HttpRequest(), user=user, success=False, - reason='no_accounts', + message_key=AttachWebhookNotification.NO_ACCOUNTS, ) notification.send() return False From fca9947bcc6c0431df85becab57e19111f1459fb Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 16:38:57 -0300 Subject: [PATCH 19/65] Make string translatable --- readthedocs/oauth/notifications.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index b16869d292c..654ad062482 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- from __future__ import division, print_function, unicode_literals +from django.utils.translation import ugettext_lazy as _ from readthedocs.notifications import SiteNotification @@ -11,9 +12,9 @@ class AttachWebhookNotification(SiteNotification): NO_ACCOUNTS = 'no_accounts' context_object_name = 'provider' - success_message = 'Webhook activated successfully.' + success_message = _('Webhook activated successfully.') failure_message = { - NO_CONNECTED_SERVICES: 'Webhook activation failed. There are no connected services for this project.', - NO_PERMISSIONS: 'Webhook activation failed. Make sure you have permissions to set it.', - NO_ACCOUNTS: 'No accounts available to set webhook on. Please connect your {provider.name} account.', + NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for this project.'), + NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), + NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), } From 38a93e373c6c43215b1114e93f5e5f65ef84e88b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 17:02:57 -0300 Subject: [PATCH 20/65] Fix merge conflicts --- readthedocs/projects/signals.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index b45e9e3908e..6ef49f9e67c 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -1,8 +1,8 @@ +# -*- coding: utf-8 -*- """Project signals""" from __future__ import absolute_import import django.dispatch -from django.dispatch import receiver before_vcs = django.dispatch.Signal(providing_args=["version"]) @@ -12,3 +12,5 @@ after_build = django.dispatch.Signal(providing_args=["version"]) project_import = django.dispatch.Signal(providing_args=["project"]) + +files_changed = django.dispatch.Signal(providing_args=["project", "files"]) From 757f57fbe152d12318ed8b7074eaaed16c3890d6 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 17:16:46 -0300 Subject: [PATCH 21/65] Recover accidentally deleted line --- readthedocs/notifications/storages.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py index d779b2158ff..c275d9a7497 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -21,6 +21,8 @@ class FallbackUniqueStorage(FallbackStorage): """ Persistent message fallback storage, but only stores unique notifications. + + This loops through all backends to find messages to store, but will skip this step if the message already exists for the user in the database. Deduplication is important here, as a persistent message may ask a user to From d804b533849ec0378c29496d93ff0eabbecb5048 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 17:19:50 -0300 Subject: [PATCH 22/65] Fixed linting errors --- readthedocs/notifications/notification.py | 2 +- readthedocs/notifications/storages.py | 14 ++++++++------ readthedocs/oauth/notifications.py | 6 +++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index dae065c6cbd..a4b26e7eaa0 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -156,5 +156,5 @@ def get_message(self, success): msg = message return msg.format(**{self.context_object_name: self.object}) - def render(self, *args, **kwargs): + 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 c275d9a7497..ca0ca595ba5 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -93,9 +93,7 @@ class NonPersistentNotification(PersistentStorage): level = 100 def _message_queryset(self, include_read=False): - """ - Return a queryset of non persistent messages for the request user. - """ + """Return a queryset of non persistent messages for the request user.""" expire = timezone.now() qs = PersistentMessage.objects.\ @@ -120,7 +118,10 @@ def _message_queryset(self, include_read=False): def _store(self, messages, response, *args, **kwargs): # There are alredy saved. - return [message for message in messages if not message.level in NON_PERSISTENT_MESSAGE_LEVELS] + return [ + message for message in messages + if message.level not in NON_PERSISTENT_MESSAGE_LEVELS + ] def process_message(self, message, *args, **kwargs): """ @@ -129,7 +130,7 @@ def process_message(self, message, *args, **kwargs): If its level is into non-persistent levels, convert the message to models and save it """ - if not message.level in NON_PERSISTENT_MESSAGE_LEVELS: + if message.level not in NON_PERSISTENT_MESSAGE_LEVELS: return message user = kwargs.get("user") or self.get_user() @@ -139,7 +140,8 @@ def process_message(self, message, *args, **kwargs): except TypeError: anonymous = user.is_anonymous if anonymous: - raise NotImplementedError('Persistent message levels cannot be used for anonymous users.') + raise NotImplementedError( + 'Persistent message levels cannot be used for anonymous users.') message_persistent = PersistentMessage() message_persistent.level = message.level message_persistent.message = message.message diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 654ad062482..483e18bf7df 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -14,7 +14,7 @@ class AttachWebhookNotification(SiteNotification): context_object_name = 'provider' success_message = _('Webhook activated successfully.') failure_message = { - NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for this project.'), - NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), - NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), + NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for ''this project.'), # noqa + NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), # noqa + NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), # noqa } From 2f20aed0c6c92d3ddaf312e1bde6af2e4c491154 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 10:43:02 -0300 Subject: [PATCH 23/65] Replace message_key with reason to be clearer --- readthedocs/notifications/notification.py | 14 +++++++------- readthedocs/oauth/tasks.py | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index a4b26e7eaa0..2aaa678aad9 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -96,7 +96,7 @@ class SiteNotification(Notification): ``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 ``message_key`` to get the proper + 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 @@ -117,7 +117,7 @@ class SiteNotification(Notification): failure_level = constants.ERROR_NON_PERSISTENT def __init__( - self, user, success, message_key=None, context_object=None, + self, user, success, reason=None, context_object=None, request=None): self.object = context_object @@ -128,7 +128,7 @@ def __init__( self.request.user = user self.success = success - self.message_key = message_key + self.reason = reason super(SiteNotification, self).__init__(context_object, request, user) def get_message_level(self): @@ -142,13 +142,13 @@ def get_message(self, success): else: message = self.failure_message - if isinstance(message, dict) and self.message_key: - if self.message_key in message: - msg = message.get(self.message_key) + if isinstance(message, dict) and self.reason: + if self.reason in message: + msg = message.get(self.reason) else: raise KeyError( "Notification has no key '{}' for {} messages".format( - self.message_key, + self.reason, 'success' if self.success else 'failure', ), ) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 98286d9140c..b2f2330ae00 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -51,7 +51,7 @@ def attach_webhook(project_pk, user_pk): notification = AttachWebhookNotification( user=user, success=False, - message_key=AttachWebhookNotification.NO_CONNECTED_SERVICES, + reason=AttachWebhookNotification.NO_CONNECTED_SERVICES, ) notification.send() return None @@ -75,7 +75,7 @@ def attach_webhook(project_pk, user_pk): notification = AttachWebhookNotification( user=user, success=False, - message_key=AttachWebhookNotification.NO_PERMISSIONS, + reason=AttachWebhookNotification.NO_PERMISSIONS, ) notification.send() else: @@ -86,7 +86,7 @@ def attach_webhook(project_pk, user_pk): context_object=provider, user=user, success=False, - message_key=AttachWebhookNotification.NO_ACCOUNTS, + reason=AttachWebhookNotification.NO_ACCOUNTS, ) notification.send() return False From 90892fb652dac42e492f0b2d4f945a8b7f4df421 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 10:44:18 -0300 Subject: [PATCH 24/65] Rename non persistent storage class --- readthedocs/notifications/storages.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py index ca0ca595ba5..004a135284d 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -39,7 +39,7 @@ class FallbackUniqueStorage(FallbackStorage): """ storages_names = ( - 'readthedocs.notifications.storages.NonPersistentNotification', + 'readthedocs.notifications.storages.NonPersistentStorage', 'messages_extends.storages.StickyStorage', 'messages_extends.storages.PersistentStorage', 'django.contrib.messages.storage.cookie.CookieStorage', @@ -79,7 +79,7 @@ def add(self, level, message, extra_tags='', *args, **kwargs): # noqa *args, **kwargs) -class NonPersistentNotification(PersistentStorage): +class NonPersistentStorage(PersistentStorage): """ Save one time (non-pesistent) messages in the database. From f72a1293530e5c223bf982cd0ab25a1720cd1551 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:00:47 -0300 Subject: [PATCH 25/65] Remove old templates --- .../oauth/notifications/attach_webhook_fail_site.html | 8 -------- .../oauth/notifications/attach_webhook_success_site.html | 1 - 2 files changed, 9 deletions(-) delete mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html delete mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html deleted file mode 100644 index e4ddcf1ce6f..00000000000 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html +++ /dev/null @@ -1,8 +0,0 @@ -{% if reason == 'no_connected_projects' %} - Webhook activation failed. There are no connected services for this project. -{% elif reason == 'no_permissions' %} - Webhook activation failed. Make sure you have permissions to set it. -{% elif reason == 'no_accounts' %} - No accounts available to set webhook on. Please connect your {{ provider.name }} account. -{% endif %} -Close. diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html deleted file mode 100644 index a92eb8e837d..00000000000 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html +++ /dev/null @@ -1 +0,0 @@ -Webhook activated. Close. From 8f1f5cddd5969c7a1d4bcd4e688467adebd7a1bd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:47:31 -0300 Subject: [PATCH 26/65] Make SiteNotification more flexible - render strings messages as Django Templates - accept extra_context for the template - do not crash if the reason is incorrect --- readthedocs/notifications/notification.py | 38 ++++++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 2aaa678aad9..bf42f4ba2ff 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -3,6 +3,7 @@ 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 @@ -13,6 +14,9 @@ from . import constants +log = logging.getLogger(__name__) + + class Notification(object): """ @@ -118,7 +122,7 @@ class SiteNotification(Notification): def __init__( self, user, success, reason=None, context_object=None, - request=None): + request=None, extra_context=None): self.object = context_object self.user = user or request.user @@ -129,8 +133,14 @@ def __init__( self.success = success self.reason = reason + self.extra_context = extra_context 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 @@ -142,19 +152,31 @@ def get_message(self, success): else: message = self.failure_message - if isinstance(message, dict) and self.reason: - if self.reason in message: - msg = message.get(self.reason) + 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: - raise KeyError( - "Notification has no key '{}' for {} messages".format( - self.reason, + 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 msg.format(**{self.context_object_name: self.object}) + + 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) From 39237cf55fbecfff57827e6934357911b71f8031 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:47:50 -0300 Subject: [PATCH 27/65] Adapt AttachWebhookNotification to the new features --- readthedocs/oauth/notifications.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 483e18bf7df..03e6d138c98 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,20 +1,32 @@ # -*- 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 readthedocs.notifications import SiteNotification class AttachWebhookNotification(SiteNotification): - NO_CONNECTED_SERVICES = 'no_connected_services' + # Fail reasons NO_PERMISSIONS = 'no_permissions' NO_ACCOUNTS = 'no_accounts' context_object_name = 'provider' - success_message = _('Webhook activated successfully.') + success_message = _('Webhook successfully added.') failure_message = { - NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for ''this project.'), # noqa - NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), # noqa - NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), # noqa + 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 From 58ce2531331caf06e7367b88a27a12f3ca44d080 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:48:13 -0300 Subject: [PATCH 28/65] 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. --- readthedocs/oauth/tasks.py | 47 +++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index b2f2330ae00..344d146cfe9 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -2,7 +2,9 @@ """Tasks for OAuth services""" from __future__ import absolute_import +from allauth.socialaccount.providers import registry as allauth_registry from django.contrib.auth.models import User +import logging from readthedocs.core.utils.tasks import PublicTask from readthedocs.core.utils.tasks import permission_check @@ -14,6 +16,9 @@ from .services import registry +log = logging.getLogger(__name__) + + @permission_check(user_id_matches) class SyncRemoteRepositories(PublicTask): @@ -48,22 +53,22 @@ def attach_webhook(project_pk, user_pk): service = service_cls break else: - notification = AttachWebhookNotification( - user=user, - success=False, - reason=AttachWebhookNotification.NO_CONNECTED_SERVICES, - ) - notification.send() + log.warning('There are no registered services in the application.') 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 = AttachWebhookNotification( - user=user, - success=True, - ) + notification.success = True notification.send() project.has_valid_webhook = True @@ -72,21 +77,11 @@ def attach_webhook(project_pk, user_pk): # No valid account found if user_accounts: - notification = AttachWebhookNotification( - user=user, - success=False, - reason=AttachWebhookNotification.NO_PERMISSIONS, - ) - notification.send() + notification.success = False + notification.reason = AttachWebhookNotification.NO_PERMISSIONS else: - from allauth.socialaccount.providers import registry as allauth_registry - - provider = allauth_registry.by_id(service.adapter.provider_id) - notification = AttachWebhookNotification( - context_object=provider, - user=user, - success=False, - reason=AttachWebhookNotification.NO_ACCOUNTS, - ) - notification.send() + notification.success = False + notification.reason = AttachWebhookNotification.NO_ACCOUNTS + + notification.send() return False From f609a3a7c2b71ad785f9e1760dffc2a08fe5b26c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 13:10:43 -0300 Subject: [PATCH 29/65] Test cases for SiteNotification and NonPersistentStorage --- readthedocs/notifications/notification.py | 2 +- .../rtd_tests/tests/test_notifications.py | 102 +++++++++++++++++- 2 files changed, 99 insertions(+), 5 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index bf42f4ba2ff..cd9a96ed3dd 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -133,7 +133,7 @@ def __init__( self.success = success self.reason = reason - self.extra_context = extra_context + self.extra_context = extra_context or {} super(SiteNotification, self).__init__(context_object, request, user) def get_context_data(self): diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index cdc2fbc5c71..7b3f5e54eef 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,96 @@ 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() + + +class NonPersistentStorageTests(TestCase): + pass From 864d968e3e38344731f0e5e071a3c8991437bfb5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 16:30:56 -0300 Subject: [PATCH 30/65] Remove unnecessary lines --- readthedocs/rtd_tests/tests/test_notifications.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index 7b3f5e54eef..b80035be01f 100644 --- a/readthedocs/rtd_tests/tests/test_notifications.py +++ b/readthedocs/rtd_tests/tests/test_notifications.py @@ -221,7 +221,3 @@ def test_message(self): with mock.patch('readthedocs.notifications.notification.log') as mock_log: self.assertEqual(self.n.get_message(False), '') mock_log.error.assert_called_once() - - -class NonPersistentStorageTests(TestCase): - pass From 9001a9e6cb626e15fbd27f0a4a99dd267faceda0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 16:36:03 -0300 Subject: [PATCH 31/65] 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. --- readthedocs/oauth/notifications.py | 22 ++++++++++++++++++++++ readthedocs/oauth/tasks.py | 26 ++++++++++++++++++-------- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 03e6d138c98..2b52f18ba37 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -3,6 +3,8 @@ 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 @@ -30,3 +32,23 @@ def get_context_data(self): '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 = _( + "You project {{ project.name }} doesn't have a valid webhook set up, " + "commits won't trigger new builds for this project. " + "See your 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 344d146cfe9..2ad30f8e4a6 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -1,21 +1,23 @@ # -*- coding: utf-8 -*- -"""Tasks for OAuth services""" +"""Tasks for OAuth services.""" + +from __future__ import ( + absolute_import, division, print_function, unicode_literals) + +import logging -from __future__ import absolute_import from allauth.socialaccount.providers import registry as allauth_registry from django.contrib.auth.models import User -import logging -from readthedocs.core.utils.tasks import PublicTask -from readthedocs.core.utils.tasks import permission_check -from readthedocs.core.utils.tasks import user_id_matches -from readthedocs.oauth.notifications import AttachWebhookNotification +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__) @@ -48,12 +50,19 @@ def attach_webhook(project_pk, user_pk): """ 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) @@ -83,5 +92,6 @@ def attach_webhook(project_pk, user_pk): notification.success = False notification.reason = AttachWebhookNotification.NO_ACCOUNTS + project_notification.send() notification.send() return False From 1d90204dfd347929dbc39ea5d56e09cf167c75cd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Jun 2018 16:43:22 -0300 Subject: [PATCH 32/65] Improve copy --- readthedocs/oauth/notifications.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 2b52f18ba37..0a457873702 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -39,9 +39,9 @@ class InvalidProjectWebhookNotification(SiteNotification): context_object_name = 'project' failure_level = ERROR_PERSISTENT failure_message = _( - "You project {{ project.name }} doesn't have a valid webhook set up, " + "The project {{ project.name }} doesn't have a valid webhook set up, " "commits won't trigger new builds for this project. " - "See your project integrations for more information.") # noqa + "See the project integrations for more information.") # noqa def get_context_data(self): context = super(InvalidProjectWebhookNotification, self).get_context_data() From 0f16e2495010ae4ce66afbc5849690890d707329 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 19:43:59 -0500 Subject: [PATCH 33/65] Remove unused basic attribute on trigger build --- readthedocs/core/utils/__init__.py | 3 +-- readthedocs/projects/views/private.py | 5 ++--- readthedocs/rtd_tests/tests/test_core_utils.py | 3 --- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 196131992d9..04cfc9b145b 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -75,7 +75,7 @@ def cname_to_slug(host): return slug -def trigger_build(project, version=None, record=True, force=False, basic=False): +def trigger_build(project, version=None, record=True, force=False): """ Trigger build for project and version. @@ -97,7 +97,6 @@ def trigger_build(project, version=None, record=True, force=False, basic=False): version_pk=version.pk, record=record, force=force, - basic=basic, ) build = None diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 50f26e1969b..a41759c043b 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -231,10 +231,9 @@ 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() project_import.send(sender=project, request=self.request) - trigger_build(project, basic=basic_only) + trigger_build(project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug])) @@ -271,7 +270,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: diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index dc3424b93e8..9e7828c4828 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -30,7 +30,6 @@ def test_trigger_build_time_limit(self, update_docs): kwargs={ 'pk': self.project.id, 'force': False, - 'basic': False, 'record': True, 'build_pk': mock.ANY, 'version_pk': self.version.id @@ -51,7 +50,6 @@ def test_trigger_build_invalid_time_limit(self, update_docs): kwargs={ 'pk': self.project.id, 'force': False, - 'basic': False, 'record': True, 'build_pk': mock.ANY, 'version_pk': self.version.id @@ -72,7 +70,6 @@ def test_trigger_build_rounded_time_limit(self, update_docs): kwargs={ 'pk': self.project.id, 'force': False, - 'basic': False, 'record': True, 'build_pk': mock.ANY, 'version_pk': self.version.id From 16cbf0fa94f8e9992d51df456ad834e31eae25ac Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 20:11:34 -0500 Subject: [PATCH 34/65] 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. --- common | 2 +- readthedocs/core/utils/__init__.py | 68 ++++++++++++++++++++------- readthedocs/projects/views/private.py | 8 +++- 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/common b/common index ed81bfc2608..650bf3ad09f 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit ed81bfc2608fe23b82a22a55d8431a01762e2f02 +Subproject commit 650bf3ad09f3536a98306a193b2c57e4ba35a7e4 diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 04cfc9b145b..27de434c172 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,37 +76,48 @@ def cname_to_slug(host): return slug -def trigger_build(project, version=None, record=True, force=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 the task on this build queue. If + project has ``skip=True``, the build is not triggered. - If project has a ``build_queue``, execute task on this build queue. Queue - will be prefixed with ``build-`` to unify build queue names. + :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, - ) + kwargs = { + 'pk': project.pk, + '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 @@ -120,16 +132,38 @@ def trigger_build(project, version=None, record=True, force=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() + return update_docs_task.si(kwargs=kwargs, **options) + - 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/projects/views/private.py b/readthedocs/projects/views/private.py index a41759c043b..41441697023 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -233,10 +233,16 @@ def done(self, form_list, **kwargs): setattr(project, field, value) project.save() project_import.send(sender=project, request=self.request) - trigger_build(project) + self.trigger_initial_build(project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug])) + def trigger_initial_build(self, project): + """ + Trigger initial build. + """ + return trigger_build(project) + def is_advanced(self): """Determine if the user selected the `show advanced` field.""" data = self.get_cleaned_data_for_step('basics') or {} From 2d4b2276ad705afb772128b90056f7a78ca8726c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 26 Apr 2018 19:49:26 -0500 Subject: [PATCH 35/65] 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. --- readthedocs/projects/signals.py | 8 ++++++++ readthedocs/projects/views/private.py | 4 ++++ readthedocs/settings/dev.py | 1 + 3 files changed, 13 insertions(+) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 21cdf88a19d..1744037f155 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -4,6 +4,7 @@ import django.dispatch from django.dispatch import receiver +from readthedocs.core.utils import trigger_build from readthedocs.oauth.utils import attach_webhook @@ -25,3 +26,10 @@ def handle_project_import(sender, **kwargs): request = kwargs.get('request') attach_webhook(project=project, request=request) + + +# TODO: move this to ImportWizardView.trigger_initial_build +# @receiver(project_import) +# def trigger_initial_build(sender, request, **kwargs): +# project = sender +# trigger_build(project) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 41441697023..5e7244e72f3 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -232,7 +232,11 @@ def done(self, form_list, **kwargs): if field in extra_fields: setattr(project, field, value) project.save() + + # TODO: if we want to make the ``attach_webhook`` async, we need to + # consider the message shown to the user when not valid webhook. project_import.send(sender=project, request=self.request) + self.trigger_initial_build(project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug])) 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' From 4022f28d9f94b784c3944b4bdb17d550518b0d71 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 20:19:38 -0500 Subject: [PATCH 36/65] Remove now unused signal --- readthedocs/projects/signals.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 1744037f155..21cdf88a19d 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -4,7 +4,6 @@ import django.dispatch from django.dispatch import receiver -from readthedocs.core.utils import trigger_build from readthedocs.oauth.utils import attach_webhook @@ -26,10 +25,3 @@ def handle_project_import(sender, **kwargs): request = kwargs.get('request') attach_webhook(project=project, request=request) - - -# TODO: move this to ImportWizardView.trigger_initial_build -# @receiver(project_import) -# def trigger_initial_build(sender, request, **kwargs): -# project = sender -# trigger_build(project) From 79fcaa3618b42f9d6103f407622481315dd928ba Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 21:33:22 -0500 Subject: [PATCH 37/65] Fix Celery signature creation --- readthedocs/core/utils/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 27de434c172..8ca3b245652 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -106,7 +106,6 @@ def prepare_build( version = project.versions.get(slug=LATEST) kwargs = { - 'pk': project.pk, 'version_pk': version.pk, 'record': record, 'force': force, @@ -140,7 +139,7 @@ def prepare_build( options['time_limit'] = int(time_limit * 1.2) update_docs_task = UpdateDocsTask() - return update_docs_task.si(kwargs=kwargs, **options) + return update_docs_task.si(project.pk, **kwargs, **options) def trigger_build(project, version=None, record=True, force=False): From 26e45b33747ed2fe5019694b2f70f1a56aeb6292 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 22:09:54 -0500 Subject: [PATCH 38/65] Fix testcases with basic on trigger_build --- readthedocs/rtd_tests/base.py | 6 +++--- readthedocs/rtd_tests/tests/test_project_views.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) 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_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' From cbcf55d6895d8654ec758a86cac21aee27b464b8 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 30 Apr 2018 22:20:03 -0500 Subject: [PATCH 39/65] Fix mock calls in test cases --- .../rtd_tests/tests/test_core_utils.py | 57 +++++++++---------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index 9e7828c4828..9c5d5c0b46b 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,60 +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, - '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, - '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( - time_limit=3, - soft_time_limit=3, + self.project.pk, + time_limit=720, + soft_time_limit=600, queue=mock.ANY, - kwargs={ - 'pk': self.project.id, - 'force': 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""" From ebac6623e5f79f555961ced1bf831e62f8ace7b9 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 8 May 2018 10:54:56 -0500 Subject: [PATCH 40/65] Fix test mock check --- readthedocs/rtd_tests/tests/test_core_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index 9c5d5c0b46b..a009982b723 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -62,8 +62,8 @@ def test_trigger_build_rounded_time_limit(self, update_docs): update_docs().si.assert_has_calls([ mock.call( self.project.pk, - time_limit=720, - soft_time_limit=600, + time_limit=3, + soft_time_limit=3, queue=mock.ANY, force=False, record=True, From bb500ba64b1bfdd0e347fbac00a7dcb433666af0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 16:41:29 -0300 Subject: [PATCH 41/65] Use async task to attach webhook --- readthedocs/oauth/notifications.py | 38 ++++++++++ readthedocs/oauth/tasks.py | 73 +++++++++++++++++++ .../attach_webhook_fail_site.html | 7 ++ .../attach_webhook_success_site.html | 1 + readthedocs/oauth/utils.py | 47 ------------ readthedocs/projects/signals.py | 12 --- readthedocs/projects/views/private.py | 17 +++-- 7 files changed, 131 insertions(+), 64 deletions(-) create mode 100644 readthedocs/oauth/notifications.py create mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html create mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py new file mode 100644 index 00000000000..555559227f5 --- /dev/null +++ b/readthedocs/oauth/notifications.py @@ -0,0 +1,38 @@ +# -*- coding: utf-8 -*- +from __future__ import division, print_function, unicode_literals + +from readthedocs.notifications import Notification +from readthedocs.notifications.constants import HTML, WARNING, INFO + + +class AttachWebhookNotification(Notification): + + name = 'attach_webhook' + context_object_name = 'socialaccount' + subject_success = 'Attach webhook success' + subject_fail = 'Attach webhook failed' + + def __init__(self, context_object, request, user, success, reason=None): + self.success = success + self.reason = reason + if self.success: + self.level = INFO + self.subject = self.subject_success + else: + self.level = WARNING + self.subject = self.subject_fail + + super(AttachWebhookNotification, self).__init__(context_object, request, user) + + def get_context_data(self): + context = super(AttachWebhookNotification, self).get_context_data() + context.update({'reason': self.reason}) + return context + + def get_template_names(self, backend_name, source_format=HTML): + return 'oauth/notifications/{name}_{status}_{backend}.{source_format}'.format( + name=self.name, + status='success' if self.success else 'failed', + backend=backend_name, + source_format=source_format, + ) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 4d0ad6336d4..1e71226e3e5 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -2,10 +2,15 @@ from __future__ import absolute_import from django.contrib.auth.models import User +from django.http import HttpRequest from readthedocs.core.utils.tasks import PublicTask from readthedocs.core.utils.tasks import permission_check from readthedocs.core.utils.tasks import user_id_matches +from readthedocs.oauth.notifications import AttachWebhookNotification +from readthedocs.projects.models import Project +from readthedocs.worker import app + from .services import registry @@ -24,3 +29,71 @@ 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) + for service_cls in registry: + if service_cls.is_project_service(project): + service = service_cls + break + else: + notification = AttachWebhookNotification( + context_object=None, + request=HttpRequest(), + user=user, + success=False, + reason='no_connected_services', + ) + notification.send() + return None + + from celery.contrib import rdb; rdb.set_trace() + + user_accounts = service.for_user(user) + for account in user_accounts: + success, __ = account.setup_webhook(project) + if success: + notification = AttachWebhookNotification( + context_object=None, + request=HttpRequest(), + user=user, + success=True, + reason='no_connected_services', + ) + notification.send() + + project.has_valid_webhook = True + project.save() + return True + + # No valid account found + if user_accounts: + notification = AttachWebhookNotification( + context_object=None, + request=HttpRequest(), + user=user, + success=False, + reason='no_permissions', + ) + notification.send() + else: + notification = AttachWebhookNotification( + context_object=service, + request=HttpRequest(), + user=user, + success=False, + reason='no_accounts', + ) + notification.send() + return False diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html new file mode 100644 index 00000000000..4cbb6ff62cf --- /dev/null +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html @@ -0,0 +1,7 @@ +{% if reason == 'no_connected_projects' %} + Webhook activation failed. There are no connected services for this project. +{% elif reason == 'no_permissions' %} + Webhook activation failed. Make sure you have permissions to set it. +{% elif reason == 'no_accounts' %} + No accounts available to set webhook on. Please connect your {{ provider.name }} account. +{% endif %} diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html new file mode 100644 index 00000000000..3fcc9bdefd8 --- /dev/null +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html @@ -0,0 +1 @@ +Webhook activated. 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..6a6147da871 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -4,7 +4,6 @@ import django.dispatch from django.dispatch import receiver -from readthedocs.oauth.utils import attach_webhook before_vcs = django.dispatch.Signal(providing_args=["version"]) @@ -14,14 +13,3 @@ after_build = django.dispatch.Signal(providing_args=["version"]) 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 5e7244e72f3..524ba2f8b0d 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, @@ -233,8 +235,7 @@ def done(self, form_list, **kwargs): setattr(project, field, value) project.save() - # TODO: if we want to make the ``attach_webhook`` async, we need to - # consider the message shown to the user when not valid webhook. + # TODO: this signal could be removed, or used for sync task project_import.send(sender=project, request=self.request) self.trigger_initial_build(project) @@ -245,7 +246,13 @@ def trigger_initial_build(self, project): """ Trigger initial build. """ - return trigger_build(project) + 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.""" From 1939f9b02d7335d8d5951499a9f5c781f0e875a2 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 11:22:39 -0300 Subject: [PATCH 42/65] Use proper context object --- readthedocs/oauth/notifications.py | 4 ++-- readthedocs/oauth/tasks.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 555559227f5..3e82ff3fa1f 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -8,7 +8,7 @@ class AttachWebhookNotification(Notification): name = 'attach_webhook' - context_object_name = 'socialaccount' + context_object_name = 'provider' subject_success = 'Attach webhook success' subject_fail = 'Attach webhook failed' @@ -32,7 +32,7 @@ def get_context_data(self): def get_template_names(self, backend_name, source_format=HTML): return 'oauth/notifications/{name}_{status}_{backend}.{source_format}'.format( name=self.name, - status='success' if self.success else 'failed', + status='success' if self.success else 'fail', backend=backend_name, source_format=source_format, ) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 1e71226e3e5..479505f13b6 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -58,7 +58,6 @@ def attach_webhook(project_pk, user_pk): notification.send() return None - from celery.contrib import rdb; rdb.set_trace() user_accounts = service.for_user(user) for account in user_accounts: @@ -88,8 +87,11 @@ def attach_webhook(project_pk, user_pk): ) notification.send() else: + from allauth.socialaccount.providers import registry as allauth_registry + + provider = allauth_registry.by_id(service.adapter.provider_id) notification = AttachWebhookNotification( - context_object=service, + context_object=provider, request=HttpRequest(), user=user, success=False, From 671c29ffe917f4b29f8718a905c14897fd755070 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 11:33:35 -0300 Subject: [PATCH 43/65] Make it compatible with newer Django versions --- readthedocs/notifications/notification.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 261b0965b30..21fee20ecaa 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -38,15 +38,15 @@ def __init__(self, context_object, request, user=None): def get_subject(self): template = Template(self.subject) - return template.render(context=Context(self.get_context_data())) + return template.render(context=self.get_context_data()) def get_context_data(self): return { 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 +71,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): From a9a16a6cc06d6fd8ef239071bf6df656868ddd70 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 11:53:46 -0300 Subject: [PATCH 44/65] Py2 and Py3 compatible line --- readthedocs/core/utils/__init__.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 8ca3b245652..3b8b346ac11 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -139,7 +139,13 @@ def prepare_build( options['time_limit'] = int(time_limit * 1.2) update_docs_task = UpdateDocsTask() - return update_docs_task.si(project.pk, **kwargs, **options) + + # 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) def trigger_build(project, version=None, record=True, force=False): From d8cc0925afd5b71597949477ea07ea4e0217d8f9 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:03:41 -0300 Subject: [PATCH 45/65] Dismiss the sticky message and stay in the same page --- .../templates/oauth/notifications/attach_webhook_fail_site.html | 1 + .../oauth/notifications/attach_webhook_success_site.html | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html index 4cbb6ff62cf..e4ddcf1ce6f 100644 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html @@ -5,3 +5,4 @@ {% elif reason == 'no_accounts' %} No accounts available to set webhook on. Please connect your {{ provider.name }} account. {% endif %} +Close. diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html index 3fcc9bdefd8..a92eb8e837d 100644 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html +++ b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html @@ -1 +1 @@ -Webhook activated. +Webhook activated. Close. From f1b20788ffb07d8b64fde3cfd360293c1e5f9250 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:15:29 -0300 Subject: [PATCH 46/65] Use Context to render the template --- readthedocs/notifications/notification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 21fee20ecaa..17badc2d0ed 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -38,7 +38,7 @@ def __init__(self, context_object, request, user=None): def get_subject(self): template = Template(self.subject) - return template.render(context=self.get_context_data()) + return template.render(context=Context(self.get_context_data())) def get_context_data(self): return { From c96fd7266bc51821ad9ac81b8288ed66322287a8 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:26:24 -0300 Subject: [PATCH 47/65] Lint errors fixed --- readthedocs/oauth/tasks.py | 1 - readthedocs/projects/signals.py | 1 - readthedocs/projects/views/private.py | 4 +--- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 479505f13b6..56439303ffc 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -58,7 +58,6 @@ def attach_webhook(project_pk, user_pk): notification.send() return None - user_accounts = service.for_user(user) for account in user_accounts: success, __ = account.setup_webhook(project) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index 6a6147da871..b45e9e3908e 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -5,7 +5,6 @@ from django.dispatch import receiver - before_vcs = django.dispatch.Signal(providing_args=["version"]) after_vcs = django.dispatch.Signal(providing_args=["version"]) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 524ba2f8b0d..6b1feb5340c 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -243,9 +243,7 @@ def done(self, form_list, **kwargs): reverse('projects_detail', args=[project.slug])) def trigger_initial_build(self, project): - """ - Trigger initial build. - """ + """Trigger initial build.""" update_docs = prepare_build(project) task_promise = chain( attach_webhook.si(project.pk, self.request.user.pk), From a2244f7a7d560757c6cebda1c55b9d76d27081dc Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 12:26:31 -0300 Subject: [PATCH 48/65] Call the task in a sync way properly --- readthedocs/projects/views/private.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 6b1feb5340c..1dc197096eb 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -791,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): From a67dde435b5c1ff0aacab8e98dd4eb26e3df06c8 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 5 Jun 2018 14:51:27 -0300 Subject: [PATCH 49/65] Update common submodule to latest --- common | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common b/common index 650bf3ad09f..f9a4b53a4af 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit 650bf3ad09f3536a98306a193b2c57e4ba35a7e4 +Subproject commit f9a4b53a4af21d22e84fa607cd8448e9f68fc8fc From 841650388479339a81252e4e67ec84cd4c5f497a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 16:30:21 -0300 Subject: [PATCH 50/65] 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. --- media/css/core.css | 4 +- readthedocs/notifications/__init__.py | 3 +- readthedocs/notifications/backends.py | 18 ++++- readthedocs/notifications/constants.py | 17 ++++ readthedocs/notifications/notification.py | 74 +++++++++++++++++ readthedocs/notifications/storages.py | 97 ++++++++++++++++++++++- readthedocs/oauth/notifications.py | 43 +++------- readthedocs/oauth/tasks.py | 16 +--- 8 files changed, 220 insertions(+), 52 deletions(-) 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/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 48bb0dd0556..077a3ac649f 100644 --- a/readthedocs/notifications/backends.py +++ b/readthedocs/notifications/backends.py @@ -29,7 +29,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): @@ -87,11 +91,19 @@ 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 + source_format=HTML, ), extra_tags='', user=notification.user, 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 17badc2d0ed..dae065c6cbd 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Support for templating of notifications.""" from __future__ import absolute_import @@ -6,6 +7,7 @@ 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 @@ -28,6 +30,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 @@ -84,3 +87,74 @@ 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 ``message_key`` 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, message_key=None, context_object=None, + request=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.message_key = message_key + super(SiteNotification, self).__init__(context_object, request, user) + + 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 + + if isinstance(message, dict) and self.message_key: + if self.message_key in message: + msg = message.get(self.message_key) + else: + raise KeyError( + "Notification has no key '{}' for {} messages".format( + self.message_key, + 'success' if self.success else 'failure', + ), + ) + else: + msg = message + return msg.format(**{self.context_object_name: self.object}) + + def render(self, *args, **kwargs): + return self.get_message(self.success) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py index e94f46e4fbe..d779b2158ff 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -1,19 +1,26 @@ +# -*- 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): """ Persistent message fallback storage, but only stores unique notifications. - - This loops through all backends to find messages to store, but will skip this step if the message already exists for the user in the database. Deduplication is important here, as a persistent message may ask a user to @@ -29,17 +36,26 @@ class FallbackUniqueStorage(FallbackStorage): render the text in templates. """ + storages_names = ( + 'readthedocs.notifications.storages.NonPersistentNotification', + '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 +75,76 @@ def add(self, level, message, extra_tags='', *args, **kwargs): # noqa return super(FallbackUniqueStorage, self).add(level, message, extra_tags, *args, **kwargs) + + +class NonPersistentNotification(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 not message.level 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 not message.level 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 index 3e82ff3fa1f..b16869d292c 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,38 +1,19 @@ # -*- coding: utf-8 -*- from __future__ import division, print_function, unicode_literals -from readthedocs.notifications import Notification -from readthedocs.notifications.constants import HTML, WARNING, INFO +from readthedocs.notifications import SiteNotification -class AttachWebhookNotification(Notification): +class AttachWebhookNotification(SiteNotification): - name = 'attach_webhook' - context_object_name = 'provider' - subject_success = 'Attach webhook success' - subject_fail = 'Attach webhook failed' - - def __init__(self, context_object, request, user, success, reason=None): - self.success = success - self.reason = reason - if self.success: - self.level = INFO - self.subject = self.subject_success - else: - self.level = WARNING - self.subject = self.subject_fail - - super(AttachWebhookNotification, self).__init__(context_object, request, user) + NO_CONNECTED_SERVICES = 'no_connected_services' + NO_PERMISSIONS = 'no_permissions' + NO_ACCOUNTS = 'no_accounts' - def get_context_data(self): - context = super(AttachWebhookNotification, self).get_context_data() - context.update({'reason': self.reason}) - return context - - def get_template_names(self, backend_name, source_format=HTML): - return 'oauth/notifications/{name}_{status}_{backend}.{source_format}'.format( - name=self.name, - status='success' if self.success else 'fail', - backend=backend_name, - source_format=source_format, - ) + context_object_name = 'provider' + success_message = 'Webhook activated successfully.' + failure_message = { + NO_CONNECTED_SERVICES: 'Webhook activation failed. There are no connected services for this project.', + NO_PERMISSIONS: 'Webhook activation failed. Make sure you have permissions to set it.', + NO_ACCOUNTS: 'No accounts available to set webhook on. Please connect your {provider.name} account.', + } diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 56439303ffc..98286d9140c 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -1,8 +1,8 @@ +# -*- coding: utf-8 -*- """Tasks for OAuth services""" from __future__ import absolute_import from django.contrib.auth.models import User -from django.http import HttpRequest from readthedocs.core.utils.tasks import PublicTask from readthedocs.core.utils.tasks import permission_check @@ -49,11 +49,9 @@ def attach_webhook(project_pk, user_pk): break else: notification = AttachWebhookNotification( - context_object=None, - request=HttpRequest(), user=user, success=False, - reason='no_connected_services', + message_key=AttachWebhookNotification.NO_CONNECTED_SERVICES, ) notification.send() return None @@ -63,11 +61,8 @@ def attach_webhook(project_pk, user_pk): success, __ = account.setup_webhook(project) if success: notification = AttachWebhookNotification( - context_object=None, - request=HttpRequest(), user=user, success=True, - reason='no_connected_services', ) notification.send() @@ -78,11 +73,9 @@ def attach_webhook(project_pk, user_pk): # No valid account found if user_accounts: notification = AttachWebhookNotification( - context_object=None, - request=HttpRequest(), user=user, success=False, - reason='no_permissions', + message_key=AttachWebhookNotification.NO_PERMISSIONS, ) notification.send() else: @@ -91,10 +84,9 @@ def attach_webhook(project_pk, user_pk): provider = allauth_registry.by_id(service.adapter.provider_id) notification = AttachWebhookNotification( context_object=provider, - request=HttpRequest(), user=user, success=False, - reason='no_accounts', + message_key=AttachWebhookNotification.NO_ACCOUNTS, ) notification.send() return False From db4a084caef4b55ccbbef9f342ce36b3d34ae1c3 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 16:38:57 -0300 Subject: [PATCH 51/65] Make string translatable --- readthedocs/oauth/notifications.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index b16869d292c..654ad062482 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- from __future__ import division, print_function, unicode_literals +from django.utils.translation import ugettext_lazy as _ from readthedocs.notifications import SiteNotification @@ -11,9 +12,9 @@ class AttachWebhookNotification(SiteNotification): NO_ACCOUNTS = 'no_accounts' context_object_name = 'provider' - success_message = 'Webhook activated successfully.' + success_message = _('Webhook activated successfully.') failure_message = { - NO_CONNECTED_SERVICES: 'Webhook activation failed. There are no connected services for this project.', - NO_PERMISSIONS: 'Webhook activation failed. Make sure you have permissions to set it.', - NO_ACCOUNTS: 'No accounts available to set webhook on. Please connect your {provider.name} account.', + NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for this project.'), + NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), + NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), } From 9f31115ad7d9b26fa5fd31d68101893df5b41e2d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 17:02:57 -0300 Subject: [PATCH 52/65] Fix merge conflicts --- readthedocs/projects/signals.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/signals.py b/readthedocs/projects/signals.py index b45e9e3908e..6ef49f9e67c 100644 --- a/readthedocs/projects/signals.py +++ b/readthedocs/projects/signals.py @@ -1,8 +1,8 @@ +# -*- coding: utf-8 -*- """Project signals""" from __future__ import absolute_import import django.dispatch -from django.dispatch import receiver before_vcs = django.dispatch.Signal(providing_args=["version"]) @@ -12,3 +12,5 @@ after_build = django.dispatch.Signal(providing_args=["version"]) project_import = django.dispatch.Signal(providing_args=["project"]) + +files_changed = django.dispatch.Signal(providing_args=["project", "files"]) From eee7e719e2ffb9214f7061441a035084e9e26ba6 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 17:16:46 -0300 Subject: [PATCH 53/65] Recover accidentally deleted line --- readthedocs/notifications/storages.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py index d779b2158ff..c275d9a7497 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -21,6 +21,8 @@ class FallbackUniqueStorage(FallbackStorage): """ Persistent message fallback storage, but only stores unique notifications. + + This loops through all backends to find messages to store, but will skip this step if the message already exists for the user in the database. Deduplication is important here, as a persistent message may ask a user to From f8299cc2215caccd087c22cd806c6863c9107117 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 11 Jun 2018 17:19:50 -0300 Subject: [PATCH 54/65] Fixed linting errors --- readthedocs/notifications/notification.py | 2 +- readthedocs/notifications/storages.py | 14 ++++++++------ readthedocs/oauth/notifications.py | 6 +++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index dae065c6cbd..a4b26e7eaa0 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -156,5 +156,5 @@ def get_message(self, success): msg = message return msg.format(**{self.context_object_name: self.object}) - def render(self, *args, **kwargs): + 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 c275d9a7497..ca0ca595ba5 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -93,9 +93,7 @@ class NonPersistentNotification(PersistentStorage): level = 100 def _message_queryset(self, include_read=False): - """ - Return a queryset of non persistent messages for the request user. - """ + """Return a queryset of non persistent messages for the request user.""" expire = timezone.now() qs = PersistentMessage.objects.\ @@ -120,7 +118,10 @@ def _message_queryset(self, include_read=False): def _store(self, messages, response, *args, **kwargs): # There are alredy saved. - return [message for message in messages if not message.level in NON_PERSISTENT_MESSAGE_LEVELS] + return [ + message for message in messages + if message.level not in NON_PERSISTENT_MESSAGE_LEVELS + ] def process_message(self, message, *args, **kwargs): """ @@ -129,7 +130,7 @@ def process_message(self, message, *args, **kwargs): If its level is into non-persistent levels, convert the message to models and save it """ - if not message.level in NON_PERSISTENT_MESSAGE_LEVELS: + if message.level not in NON_PERSISTENT_MESSAGE_LEVELS: return message user = kwargs.get("user") or self.get_user() @@ -139,7 +140,8 @@ def process_message(self, message, *args, **kwargs): except TypeError: anonymous = user.is_anonymous if anonymous: - raise NotImplementedError('Persistent message levels cannot be used for anonymous users.') + raise NotImplementedError( + 'Persistent message levels cannot be used for anonymous users.') message_persistent = PersistentMessage() message_persistent.level = message.level message_persistent.message = message.message diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 654ad062482..483e18bf7df 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -14,7 +14,7 @@ class AttachWebhookNotification(SiteNotification): context_object_name = 'provider' success_message = _('Webhook activated successfully.') failure_message = { - NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for this project.'), - NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), - NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), + NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for ''this project.'), # noqa + NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), # noqa + NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), # noqa } From c96c4fe3556d463a62473a1ea8a847c20d29c34c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 10:43:02 -0300 Subject: [PATCH 55/65] Replace message_key with reason to be clearer --- readthedocs/notifications/notification.py | 14 +++++++------- readthedocs/oauth/tasks.py | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index a4b26e7eaa0..2aaa678aad9 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -96,7 +96,7 @@ class SiteNotification(Notification): ``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 ``message_key`` to get the proper + 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 @@ -117,7 +117,7 @@ class SiteNotification(Notification): failure_level = constants.ERROR_NON_PERSISTENT def __init__( - self, user, success, message_key=None, context_object=None, + self, user, success, reason=None, context_object=None, request=None): self.object = context_object @@ -128,7 +128,7 @@ def __init__( self.request.user = user self.success = success - self.message_key = message_key + self.reason = reason super(SiteNotification, self).__init__(context_object, request, user) def get_message_level(self): @@ -142,13 +142,13 @@ def get_message(self, success): else: message = self.failure_message - if isinstance(message, dict) and self.message_key: - if self.message_key in message: - msg = message.get(self.message_key) + if isinstance(message, dict) and self.reason: + if self.reason in message: + msg = message.get(self.reason) else: raise KeyError( "Notification has no key '{}' for {} messages".format( - self.message_key, + self.reason, 'success' if self.success else 'failure', ), ) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 98286d9140c..b2f2330ae00 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -51,7 +51,7 @@ def attach_webhook(project_pk, user_pk): notification = AttachWebhookNotification( user=user, success=False, - message_key=AttachWebhookNotification.NO_CONNECTED_SERVICES, + reason=AttachWebhookNotification.NO_CONNECTED_SERVICES, ) notification.send() return None @@ -75,7 +75,7 @@ def attach_webhook(project_pk, user_pk): notification = AttachWebhookNotification( user=user, success=False, - message_key=AttachWebhookNotification.NO_PERMISSIONS, + reason=AttachWebhookNotification.NO_PERMISSIONS, ) notification.send() else: @@ -86,7 +86,7 @@ def attach_webhook(project_pk, user_pk): context_object=provider, user=user, success=False, - message_key=AttachWebhookNotification.NO_ACCOUNTS, + reason=AttachWebhookNotification.NO_ACCOUNTS, ) notification.send() return False From 1ac78a1cd7e80e6273f9c3dd94394d499b10a490 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 10:44:18 -0300 Subject: [PATCH 56/65] Rename non persistent storage class --- readthedocs/notifications/storages.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/notifications/storages.py b/readthedocs/notifications/storages.py index ca0ca595ba5..004a135284d 100644 --- a/readthedocs/notifications/storages.py +++ b/readthedocs/notifications/storages.py @@ -39,7 +39,7 @@ class FallbackUniqueStorage(FallbackStorage): """ storages_names = ( - 'readthedocs.notifications.storages.NonPersistentNotification', + 'readthedocs.notifications.storages.NonPersistentStorage', 'messages_extends.storages.StickyStorage', 'messages_extends.storages.PersistentStorage', 'django.contrib.messages.storage.cookie.CookieStorage', @@ -79,7 +79,7 @@ def add(self, level, message, extra_tags='', *args, **kwargs): # noqa *args, **kwargs) -class NonPersistentNotification(PersistentStorage): +class NonPersistentStorage(PersistentStorage): """ Save one time (non-pesistent) messages in the database. From 7e1c7d99d7669d18e8b8330cf720b8067da5c036 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:00:47 -0300 Subject: [PATCH 57/65] Remove old templates --- .../oauth/notifications/attach_webhook_fail_site.html | 8 -------- .../oauth/notifications/attach_webhook_success_site.html | 1 - 2 files changed, 9 deletions(-) delete mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html delete mode 100644 readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html deleted file mode 100644 index e4ddcf1ce6f..00000000000 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html +++ /dev/null @@ -1,8 +0,0 @@ -{% if reason == 'no_connected_projects' %} - Webhook activation failed. There are no connected services for this project. -{% elif reason == 'no_permissions' %} - Webhook activation failed. Make sure you have permissions to set it. -{% elif reason == 'no_accounts' %} - No accounts available to set webhook on. Please connect your {{ provider.name }} account. -{% endif %} -Close. diff --git a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html b/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html deleted file mode 100644 index a92eb8e837d..00000000000 --- a/readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html +++ /dev/null @@ -1 +0,0 @@ -Webhook activated. Close. From 5421b1f0c4f0270c7b2a16b1d3aada8bed317fa7 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:47:31 -0300 Subject: [PATCH 58/65] Make SiteNotification more flexible - render strings messages as Django Templates - accept extra_context for the template - do not crash if the reason is incorrect --- readthedocs/notifications/notification.py | 38 ++++++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index 2aaa678aad9..bf42f4ba2ff 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -3,6 +3,7 @@ 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 @@ -13,6 +14,9 @@ from . import constants +log = logging.getLogger(__name__) + + class Notification(object): """ @@ -118,7 +122,7 @@ class SiteNotification(Notification): def __init__( self, user, success, reason=None, context_object=None, - request=None): + request=None, extra_context=None): self.object = context_object self.user = user or request.user @@ -129,8 +133,14 @@ def __init__( self.success = success self.reason = reason + self.extra_context = extra_context 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 @@ -142,19 +152,31 @@ def get_message(self, success): else: message = self.failure_message - if isinstance(message, dict) and self.reason: - if self.reason in message: - msg = message.get(self.reason) + 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: - raise KeyError( - "Notification has no key '{}' for {} messages".format( - self.reason, + 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 msg.format(**{self.context_object_name: self.object}) + + 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) From c8deddffa98cd936fa211aa0d3394cfddfff40d7 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:47:50 -0300 Subject: [PATCH 59/65] Adapt AttachWebhookNotification to the new features --- readthedocs/oauth/notifications.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 483e18bf7df..03e6d138c98 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -1,20 +1,32 @@ # -*- 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 readthedocs.notifications import SiteNotification class AttachWebhookNotification(SiteNotification): - NO_CONNECTED_SERVICES = 'no_connected_services' + # Fail reasons NO_PERMISSIONS = 'no_permissions' NO_ACCOUNTS = 'no_accounts' context_object_name = 'provider' - success_message = _('Webhook activated successfully.') + success_message = _('Webhook successfully added.') failure_message = { - NO_CONNECTED_SERVICES: _('Webhook activation failed. There are no connected services for ''this project.'), # noqa - NO_PERMISSIONS: _('Webhook activation failed. Make sure you have permissions to set it.'), # noqa - NO_ACCOUNTS: _('No accounts available to set webhook on. Please connect your {provider.name} account.'), # noqa + 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 From c85676ff21c4d9d117a6adb64c7914f4ec75c3a7 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 11:48:13 -0300 Subject: [PATCH 60/65] 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. --- readthedocs/oauth/tasks.py | 47 +++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index b2f2330ae00..344d146cfe9 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -2,7 +2,9 @@ """Tasks for OAuth services""" from __future__ import absolute_import +from allauth.socialaccount.providers import registry as allauth_registry from django.contrib.auth.models import User +import logging from readthedocs.core.utils.tasks import PublicTask from readthedocs.core.utils.tasks import permission_check @@ -14,6 +16,9 @@ from .services import registry +log = logging.getLogger(__name__) + + @permission_check(user_id_matches) class SyncRemoteRepositories(PublicTask): @@ -48,22 +53,22 @@ def attach_webhook(project_pk, user_pk): service = service_cls break else: - notification = AttachWebhookNotification( - user=user, - success=False, - reason=AttachWebhookNotification.NO_CONNECTED_SERVICES, - ) - notification.send() + log.warning('There are no registered services in the application.') 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 = AttachWebhookNotification( - user=user, - success=True, - ) + notification.success = True notification.send() project.has_valid_webhook = True @@ -72,21 +77,11 @@ def attach_webhook(project_pk, user_pk): # No valid account found if user_accounts: - notification = AttachWebhookNotification( - user=user, - success=False, - reason=AttachWebhookNotification.NO_PERMISSIONS, - ) - notification.send() + notification.success = False + notification.reason = AttachWebhookNotification.NO_PERMISSIONS else: - from allauth.socialaccount.providers import registry as allauth_registry - - provider = allauth_registry.by_id(service.adapter.provider_id) - notification = AttachWebhookNotification( - context_object=provider, - user=user, - success=False, - reason=AttachWebhookNotification.NO_ACCOUNTS, - ) - notification.send() + notification.success = False + notification.reason = AttachWebhookNotification.NO_ACCOUNTS + + notification.send() return False From 9a54e7412450caa7b63aaae5eaeaa3508fe08a0a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 13:10:43 -0300 Subject: [PATCH 61/65] Test cases for SiteNotification and NonPersistentStorage --- readthedocs/notifications/notification.py | 2 +- .../rtd_tests/tests/test_notifications.py | 102 +++++++++++++++++- 2 files changed, 99 insertions(+), 5 deletions(-) diff --git a/readthedocs/notifications/notification.py b/readthedocs/notifications/notification.py index bf42f4ba2ff..cd9a96ed3dd 100644 --- a/readthedocs/notifications/notification.py +++ b/readthedocs/notifications/notification.py @@ -133,7 +133,7 @@ def __init__( self.success = success self.reason = reason - self.extra_context = extra_context + self.extra_context = extra_context or {} super(SiteNotification, self).__init__(context_object, request, user) def get_context_data(self): diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index cdc2fbc5c71..7b3f5e54eef 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,96 @@ 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() + + +class NonPersistentStorageTests(TestCase): + pass From 48eed1628500216871409a198791968fee8c0cb0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 16:30:56 -0300 Subject: [PATCH 62/65] Remove unnecessary lines --- readthedocs/rtd_tests/tests/test_notifications.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_notifications.py b/readthedocs/rtd_tests/tests/test_notifications.py index 7b3f5e54eef..b80035be01f 100644 --- a/readthedocs/rtd_tests/tests/test_notifications.py +++ b/readthedocs/rtd_tests/tests/test_notifications.py @@ -221,7 +221,3 @@ def test_message(self): with mock.patch('readthedocs.notifications.notification.log') as mock_log: self.assertEqual(self.n.get_message(False), '') mock_log.error.assert_called_once() - - -class NonPersistentStorageTests(TestCase): - pass From 13ec71c8d5a03b611ba8a32ac6b81c8b591370bd Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Jun 2018 16:36:03 -0300 Subject: [PATCH 63/65] 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. --- readthedocs/oauth/notifications.py | 22 ++++++++++++++++++++++ readthedocs/oauth/tasks.py | 26 ++++++++++++++++++-------- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 03e6d138c98..2b52f18ba37 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -3,6 +3,8 @@ 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 @@ -30,3 +32,23 @@ def get_context_data(self): '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 = _( + "You project {{ project.name }} doesn't have a valid webhook set up, " + "commits won't trigger new builds for this project. " + "See your 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 344d146cfe9..2ad30f8e4a6 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -1,21 +1,23 @@ # -*- coding: utf-8 -*- -"""Tasks for OAuth services""" +"""Tasks for OAuth services.""" + +from __future__ import ( + absolute_import, division, print_function, unicode_literals) + +import logging -from __future__ import absolute_import from allauth.socialaccount.providers import registry as allauth_registry from django.contrib.auth.models import User -import logging -from readthedocs.core.utils.tasks import PublicTask -from readthedocs.core.utils.tasks import permission_check -from readthedocs.core.utils.tasks import user_id_matches -from readthedocs.oauth.notifications import AttachWebhookNotification +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__) @@ -48,12 +50,19 @@ def attach_webhook(project_pk, user_pk): """ 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) @@ -83,5 +92,6 @@ def attach_webhook(project_pk, user_pk): notification.success = False notification.reason = AttachWebhookNotification.NO_ACCOUNTS + project_notification.send() notification.send() return False From 9f8fddfe89b495311c4d02fa8c7b93d9f7aef8a6 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 13 Jun 2018 16:43:22 -0300 Subject: [PATCH 64/65] Improve copy --- readthedocs/oauth/notifications.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/oauth/notifications.py b/readthedocs/oauth/notifications.py index 2b52f18ba37..0a457873702 100644 --- a/readthedocs/oauth/notifications.py +++ b/readthedocs/oauth/notifications.py @@ -39,9 +39,9 @@ class InvalidProjectWebhookNotification(SiteNotification): context_object_name = 'project' failure_level = ERROR_PERSISTENT failure_message = _( - "You project {{ project.name }} doesn't have a valid webhook set up, " + "The project {{ project.name }} doesn't have a valid webhook set up, " "commits won't trigger new builds for this project. " - "See your project integrations for more information.") # noqa + "See the project integrations for more information.") # noqa def get_context_data(self): context = super(InvalidProjectWebhookNotification, self).get_context_data() From 5d1d7c5a08c9858ab0014656e706f6eec6878801 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 14 Jun 2018 14:51:02 -0300 Subject: [PATCH 65/65] Remove fixed template notification about Project.has_valid_webhook --- readthedocs/templates/core/project_bar_base.html | 14 -------------- 1 file changed, 14 deletions(-) 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 %}