Allow to hook the initial build from outside #4033

humitos · 2018-04-27T00:51:25Z

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.

Needed by https://github.com/readthedocs/readthedocs-corporate/issues/99

Also Closes #3888

agjohnson

Still WIP, so just making a note for now on implementation.

agjohnson · 2018-04-27T16:58:21Z

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, basic=False, execute_task=True):


How about we instead move the task creation to it's own method and make trigger_build always execute the task signature returned from this secondary utility method. That way, we don't have a confusing case here where trigger_build doesn't actually "trigger a build".

Yes. Agreed. What about prepare_build that always returns the task signature and trigger_build which call the prepare_build and just apply_async?

What about the rest of the ideas?

One in particular, do you think it's better to use a signal and connect to (*) or call a method (that can be overriden from .com) from ImportWizardView.done?

(*) the problem with this that I hit locally, is that when running the corporate site, I ended up with two receivers for the project_import signal: one from community which failed, and the other one from corporate

humitos · 2018-05-01T02:24:44Z

readthedocs/projects/views/private.py

        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)


I suppose that we can remove the receiver of this signal and use django-async-messages here also.

We can achieve this in another PR, if you agree, after we test a little the new flow.

What's the difference between message extends and async messages? Seems we already have support for this, no?

Two things here:

I didn't know that we were using message extends

it doesn't do exactly the same since async message is a one-time message where the user doesn't have to interact with it (hit the close button) which sometimes could be lost by the user and don't read (it works exactly in the same way as normal django message framework does).

Summarizing, I think that what we were using (message extends) is better. I will adapt my code to use the Sticky storage.

EDIT: the main difference is that async message doesn't need the request as first attribute, which is useful when we want to add a message from a Celery task

Mmm... I just read the code and found that it does depend on the request.

I'm taking a look to see if we can extend the storage to just receive the user instead.

I was thinking on something like this:

from collections import namedtuple from messages_extends.storages import PersistentStorage class CeleryPersistentStorage(PersistentStorage): """ Persistent Message Storage for Celery. This storage doesn't depend on a request as first argument. """ FakeRequest = namedtuple('FakeRequest', ['user']) def __init__(self, user, *args, **kwargs): self.user = user request = self.FakeRequest(user=self.user) return super(CeleryPersistentStorage, self).__init__(request, *args, **kwargs) def get_user(self): return self.user

But I'm not sure if it's what we need and also it's a hack and does't work 😀

I found another issue with message_extends.

In case we want to use it without a request, we can do this:

from messages_extends import WARNING_PERSISTENT from messages_extends.models import Message Message.objects.create( user=self.user, message='Persistent message here', level=WARNING_PERSISTENT, expires=datetime, # optional )

This message will be displayed until it expires (if it's set). After it's expires it will be deleted from the database with a task that we have wrote.

The problem here is that we don't show the X to the user to close the message and be marked as read in the db (there are some messages/ urls that we are not defining).

I also found that we have an already coded hack here: https://github.com/rtfd/readthedocs.org/blob/31799754e46e7b51d8176686da0c34c4bded3917/readthedocs/notifications/backends.py#L81-L84

I will give it a try tomorrow. It seems that all I need is to call notifications.backends.send_notification with None as request and a level lower than REQUIREMENT to avoid sending the email and just show it in the site... (sounds tricky)

humitos · 2018-05-01T02:25:32Z

readthedocs/projects/views/private.py

@@ -271,7 +280,7 @@ def get(self, request, *args, **kwargs):
            if form.is_valid():
                project = form.save()
                project.save()
-                trigger_build(project, basic=True)


I removed basic since it's not used at all.

agjohnson

Changes here look great! I think the question of how do we message a user without a request is already solved by messages-extend, which we use more on the commercial hosting side. I think we just need to pass around a user instead of a request object.

humitos · 2018-06-05T14:48:45Z

This is pretty close to what we want: all the initial project import is handled with chained async calls.

The only problem/issue that I'm experimenting now is that as has_valid_webhook is default to False, after importing the project I'm immediately seeing a message like:

This repository doesn't have a valid webhook set up, commits won't trigger new builds for this project.

from https://github.com/rtfd/readthedocs.org/blob/3a78b848c205399e16bb270d331be4a85c9f3843/readthedocs/templates/core/project_bar_base.html#L29-L42

This message dissapears once the Celery task completes and I refresh the page.

humitos · 2018-06-05T17:19:10Z

There is a linting error because it seems that we are requiring django-dynamic-fixture for some strange reason and it's not in the requirements.

stsewd · 2018-06-05T17:23:36Z

There is a linting error because it seems that we are requiring django-dynamic-fixture for some strange reason and it's not in the requirements.

I think this is because you updated the common submodule, you only need to rebase (the requirements were moved in other PR a long time ago).

agjohnson

Looks great! I raised question on the use of our notification backend, though I don't think it's a bad application -- just looking for some quick discussion there.

The rest of the PR makes sense though. I think we're in good shape as long as:

When a webhook attachment fails, the requesting user (and only the requesting user) receives an on-site notification (no email) and the notification has a link to something actionable in the notification
When a webhook attachment succeeds, the user gets a normal notification (non-sticky, doesn't need to be dismissed, no email)

agjohnson · 2018-06-06T16:40:45Z

readthedocs/oauth/notifications.py

+from readthedocs.notifications.constants import HTML, WARNING, INFO
+
+
+class AttachWebhookNotification(Notification):


What is the reason for using our notification backend (as opposed to using message_extends directly)?

Using the notification backend is not required to send a message without a request, but it is meant to be a way to make maintaining long-lived notifications easier to manage. I'd consider this a short-lived notification.

Also, I'm not implying this is incorrect, just considering the choice for this implementation.

As I mentioned in the chat, I thought that we didn't use the message_extends directly, but through our Notification backend.

We can't create short-lived (non persistent) notifications from a Celery task since we don't have the request nor the session, which is the important part since the message is saved in the session.

The package that I was using before, django-async-messages, saves the message in the cache instead of in the session and that's why we can save/load them without a request/session.

So, I'm still not sure what's the best approach here, but it looks like these are the main points:

attaching the webhook fails: we want a persistent message here with an actionable link to solve the issue and mention the project's name because the user can moves out the project's page after importing it

attaching the webhook succeeds: we want an async message saved in the cache

We could generalize this and create a celery notification helper that does one thing or the other depending on the status of the message (fail / success) so we don't have to deal with it every time we want a notification from a task.

I think this needs some clarification, as it sounds like the notification system, and usage of message_extends, is confusing. We don't need to switch to a different package to do what you ultimately want to do.

message_extends is a) a databased backed method of sending messages, and is also b) a way to make persistent/sticky messages. It doesn't have to be both, it can accept message levels that are basic django message levels.

The message levels from django.contrib.messages correspond to the notification style you are used to -- messages that will display on one request only.

message_extends adds new levels that correspond to the same warning/info/etc levels, but provide a sticky/persistent message. This is a message that will display on multiple requests.

All of this is separate from how message_extends stores messages. You can pass any message level to message_extends and it will display, but it comes from the database, not the user's cookie/session.

Using our notification system isn't wrong, but it isn't required just to use message_extends. If you were to write this again, you don't need the bulk of our notification system. You would just submit a notification through message_extends -- and use a message level from django.contrib.messages to get a single request notification (non-sticky/non-persistent).

However, this is maybe a chance to improve a pattern for on-site messaging

I don't necessarily see a reason why we shouldn't use the notification system. My problem is that it is overkill for this case, but it does provide isolation between the notification pieces, which I like as pattern. Perhaps we need an extension to the class that makes it really easy for basic on-site messages like this. For example, a subclass that:

Doesn't require files on disk for templates, it uses methods

Doesn't send via the email backend, or maybe make this a class var that is default False

Has success/failure messages in a single class -- normally this would be two separate classes, one for each notification and notification template

Has a CBV like get_success_url() and get_success_message() or something, so we're not doing django templates in string vars on the class.

Just a thought. Hopefully that makes more sense. I'm happy to pair on this more if you're still unsure.

We could generalize this and create a celery notification

We probably don't need this level of optimization yet, but the generalized on-site message notification helper class above would be close to this. We might be able to use that class to standardize all our on-site messaging eventually, celery task or not.

agjohnson · 2018-06-06T16:47:48Z

readthedocs/oauth/tasks.py

+            request=HttpRequest(),
+            user=user,
+            success=False,
+            reason='no_connected_services',


Perhaps there is a better place to define these constants? I like to avoid using magic strings like this. I'd probably suggestion class-level constants on AttachWebhookNotification with the constants passed in through context data.

agjohnson · 2018-06-06T16:49:00Z

readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html

+{% elif reason == 'no_accounts' %}
+  No accounts available to set webhook on. Please connect your {{ provider.name }} account.
+{% endif %}
+<a href="{{ request.path }}">Close.</a>


The UX here could be better. We should probably link the user to a page where they can take an action to resolve these issues. Some example links for the above:

<a href="{{ ... }}">Connect your account</a> <a href="https://docs.readthedocs.io/en/latest/something.html">Check your permissions</a> <a href="{{ ... }}">Connect your account</a>

Can also be removed

agjohnson · 2018-06-06T16:54:45Z

readthedocs/oauth/templates/oauth/notifications/attach_webhook_success_site.html

@@ -0,0 +1 @@
+Webhook activated. <a href="{{ request.path }}">Close.</a>


A sticky notification for a temporary message should be avoided, and is part of why using the notification system is a little overkill. You can maybe instead make the notification non-sticky by using a standard django messages message level:
https://docs.djangoproject.com/en/2.0/ref/contrib/messages/#message-levels

These differ from the message levels in notifications/constants.py -- you'll have to look at the message extends constants for more background here.

You'll likely need to subclass the Notification into multiple classes (pass and fail classes) so that you can set the notification level. It might still be possible to override on the class __init__ though.

Also, localization required here

agjohnson · 2018-06-06T16:55:41Z

readthedocs/oauth/templates/oauth/notifications/attach_webhook_fail_site.html

@@ -0,0 +1,8 @@
+{% if reason == 'no_connected_projects' %}
+  Webhook activation failed. There are no connected services for this project.


These should be translated

Looks like this can just be removed now

agjohnson · 2018-06-06T16:58:28Z

readthedocs/oauth/notifications.py

+        self.success = success
+        self.reason = reason
+        if self.success:
+            self.level = INFO


Raised below, I see now you've already sort of accomplished setting the level per-notification. As per feedback below, I'd use a standard django messages level to avoid making the notification sticky.

I believe the docs for messages_extends goes over some of the notification levels, but you might need to refer to code for more background.

agjohnson · 2018-06-06T17:12:04Z

readthedocs/oauth/notifications.py

+    name = 'attach_webhook'
+    context_object_name = 'provider'
+    subject_success = 'Attach webhook success'
+    subject_fail = 'Attach webhook failed'


I'm mostly certain that subject is used for email only, so these should be required. Double check that I'm not incorrect here.

Also, for purposes of maintaining good UX, copy for this email subject (if we needed to send one) should be:
Attach webhook success -> Webhook successfully configured
Attach webhook failed -> Webhook could not be configured

These would be clearer to the user -- they are speaking in core developer language currently.

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.

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.

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.

- render strings messages as Django Templates - accept extra_context for the template - do not crash if the reason is incorrect

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.

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.

…/async/initial-build

…docs.org into humitos/async/initial-build

humitos · 2018-06-14T18:26:49Z

3 notifications were shown, yes. We decided in the chat to remove the one from the template.

Doing QA, the only issue that I found is that importing the project, it failed to add the webhook, and you just delete the project... Since the notification is persistent you will end with an invalid notification that to be removed you have to click in the link from the notification which will lead you to a 404 page (project deleted) :/

agjohnson · 2018-06-14T18:29:01Z

That's fine, I'd expect a 404 as a user.

In #4033, I introduce a bug when using Celery signatures. From Celery's docs at http://docs.celeryproject.org/en/latest/reference/celery.html#celery.signature > the .s() shortcut does not allow you to specify execution options but there’s a chaning .set method that returns the signature So, instead of dealing with multiple `.set()`, I'm just using the `.signature()` method of the task which is more explicit.

humitos added the PR: work in progress Pull request is not ready for full review label Apr 27, 2018

agjohnson reviewed Apr 27, 2018

View reviewed changes

humitos force-pushed the humitos/async/initial-build branch 2 times, most recently from afd027e to c561296 Compare May 1, 2018 01:25

humitos commented May 1, 2018

View reviewed changes

humitos force-pushed the humitos/async/initial-build branch from 0daf963 to cd3f73e Compare May 1, 2018 02:53

humitos added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels May 1, 2018

agjohnson reviewed May 3, 2018

View reviewed changes

agjohnson added PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels May 3, 2018

agjohnson added this to the 2.4 milestone May 3, 2018

agjohnson modified the milestones: 2.4, 2.5 May 31, 2018

humitos force-pushed the humitos/async/initial-build branch from 2d7f6e9 to efd40ac Compare June 5, 2018 13:11

humitos added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Jun 5, 2018

agjohnson requested changes Jun 6, 2018

View reviewed changes

agjohnson modified the milestones: 2.5, 2.6 Jun 7, 2018

agjohnson removed the PR: ready for review label Jun 8, 2018

humitos added 5 commits June 11, 2018 16:39

Remove unused basic attribute on trigger build

3582584

Allow to hook the initial build from outside

c2c10af

Add a new attribute to `trigger_build` to do not execute the task but just return the immutable signature of it, so it can be chained into a bigger task from outside when it's needed to do something else before/after it's execution.

Remove now unused signal

efa2bd3

Fix Celery signature creation

851e1ff

humitos added 22 commits June 14, 2018 14:42

Use Context to render the template

f1b2078

Lint errors fixed

c96fd72

Call the task in a sync way properly

a2244f7

Update common submodule to latest

a67dde4

Make string translatable

db4a084

Fix merge conflicts

9f31115

Recover accidentally deleted line

eee7e71

Fixed linting errors

f8299cc

Replace message_key with reason to be clearer

c96c4fe

Rename non persistent storage class

1ac78a1

Remove old templates

7e1c7d9

Make SiteNotification more flexible

5421b1f

- render strings messages as Django Templates - accept extra_context for the template - do not crash if the reason is incorrect

Adapt AttachWebhookNotification to the new features

c8deddf

Refactor the task to attach a webhook

c85676f

Instantiate only once the notification and adapt it depending the context. Also, if there are no connected services into our application do not show a message to the user, but log it as a warning.

Test cases for SiteNotification and NonPersistentStorage

9a54e74

Remove unnecessary lines

48eed16

Improve copy

9f8fddf

Merge branch 'master' of github.com:rtfd/readthedocs.org into humitos…

5bfd8df

…/async/initial-build

Remove fixed template notification about Project.has_valid_webhook

5d1d7c5

Merge branch 'humitos/async/initial-build' of github.com:rtfd/readthe…

c87e646

…docs.org into humitos/async/initial-build

agjohnson merged commit cedda9b into master Jun 14, 2018

agjohnson deleted the humitos/async/initial-build branch June 14, 2018 18:29

humitos mentioned this pull request Aug 21, 2018

Route task to proper queue #4553

Merged

humitos mentioned this pull request Nov 2, 2018

RTD wants me to set up webhooks on a now non-maintaining project #4684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to hook the initial build from outside #4033

Allow to hook the initial build from outside #4033

humitos commented Apr 27, 2018 •

edited

Loading

agjohnson left a comment

agjohnson Apr 27, 2018

humitos Apr 27, 2018

humitos May 1, 2018

humitos May 1, 2018

agjohnson May 3, 2018

humitos May 8, 2018 •

edited

Loading

humitos May 8, 2018

humitos May 8, 2018 •

edited

Loading

humitos May 9, 2018

humitos May 9, 2018

humitos May 1, 2018

agjohnson left a comment

humitos commented Jun 5, 2018

humitos commented Jun 5, 2018

stsewd commented Jun 5, 2018

agjohnson left a comment

agjohnson Jun 6, 2018

humitos Jun 6, 2018

agjohnson Jun 7, 2018

agjohnson Jun 6, 2018

agjohnson Jun 6, 2018

agjohnson Jun 12, 2018

agjohnson Jun 6, 2018

agjohnson Jun 6, 2018

agjohnson Jun 6, 2018

agjohnson Jun 12, 2018

agjohnson Jun 6, 2018

agjohnson Jun 6, 2018

humitos commented Jun 14, 2018

agjohnson commented Jun 14, 2018

		from readthedocs.notifications.constants import HTML, WARNING, INFO


		class AttachWebhookNotification(Notification):

		@@ -0,0 +1 @@
		Webhook activated. <a href="{{ request.path }}">Close.</a>

		@@ -0,0 +1,8 @@
		{% if reason == 'no_connected_projects' %}
		Webhook activation failed. There are no connected services for this project.

Allow to hook the initial build from outside #4033

Allow to hook the initial build from outside #4033

Conversation

humitos commented Apr 27, 2018 • edited Loading

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson left a comment

Choose a reason for hiding this comment

humitos commented Jun 5, 2018

humitos commented Jun 5, 2018

stsewd commented Jun 5, 2018

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Jun 14, 2018

agjohnson commented Jun 14, 2018

humitos commented Apr 27, 2018 •

edited

Loading

humitos May 8, 2018 •

edited

Loading

humitos May 8, 2018 •

edited

Loading