New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to hook the initial build from outside #4033

Merged
merged 67 commits into from Jun 14, 2018

Conversation

Projects
None yet
3 participants
@humitos
Member

humitos commented Apr 27, 2018

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 rtfd/readthedocs-corporate#99

Also Closes #3888

@agjohnson

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

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

This comment has been minimized.

@agjohnson

agjohnson Apr 27, 2018

Contributor

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".

This comment has been minimized.

@humitos

humitos Apr 27, 2018

Member

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

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)

This comment has been minimized.

@humitos

humitos May 1, 2018

Member

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

This comment has been minimized.

@humitos

humitos May 1, 2018

Member

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

This comment has been minimized.

@agjohnson

agjohnson May 3, 2018

Contributor

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

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

Two things here:

  1. I didn't know that we were using message extends
  2. 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

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

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.

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

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 😀

This comment has been minimized.

@humitos

humitos May 9, 2018

Member

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).

This comment has been minimized.

@humitos

humitos May 9, 2018

Member

I also found that we have an already coded hack here:

# Instead of calling the standard messages.add method, this instead
# manipulates the storage directly. This is because we don't have a
# request object and need to mock one out to fool the message storage
# into saving a message for a separate user.

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)

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

This comment has been minimized.

@humitos

humitos May 1, 2018

Member

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.

@agjohnson agjohnson added this to the 2.4 milestone May 3, 2018

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

@humitos

This comment has been minimized.

Member

humitos commented Jun 5, 2018

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

{% if not project.has_valid_webhook and request.user|is_admin:project %}
<p class="build-failure">
{% 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 %}
<br>
{% blocktrans %}
See <a href='{{ integrations_url }}'>your project integrations</a> for more information.
{% endblocktrans %}
</p>
{% endif %}

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

@humitos

This comment has been minimized.

Member

humitos commented Jun 5, 2018

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

This comment has been minimized.

Member

stsewd commented Jun 5, 2018

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)
from readthedocs.notifications.constants import HTML, WARNING, INFO
class AttachWebhookNotification(Notification):

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

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.

This comment has been minimized.

@humitos

humitos Jun 6, 2018

Member

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.

This comment has been minimized.

@agjohnson

agjohnson Jun 7, 2018

Contributor

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.

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

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

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.

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

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

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>

This comment has been minimized.

@agjohnson

agjohnson Jun 12, 2018

Contributor

Can also be removed

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

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

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.

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

Also, localization required here

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

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

These should be translated

This comment has been minimized.

@agjohnson

agjohnson Jun 12, 2018

Contributor

Looks like this can just be removed now

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

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

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.

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

This comment has been minimized.

@agjohnson

agjohnson Jun 6, 2018

Contributor

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.

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

humitos added some commits Apr 27, 2018

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

humitos added some commits Jun 5, 2018

Define NonPersistentStorage for one-time notifications
NonPersistentNotification backed together with SiteNotification class
helps us to create one-time site notifications and attach it to a user
without the needs of a request/session object.

The message/notification is saved into the database as a
PersistentMessage and it's marked as read while it's retrived the
first time.

This system is useful for notifications that need to be attached from
a Celery task where we know the user but we don't have the request.
Make SiteNotification more flexible
- render strings messages as Django Templates
- accept extra_context for the template
- do not crash if the reason is incorrect
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.
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.
@humitos

This comment has been minimized.

Member

humitos commented Jun 14, 2018

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) :/

captura de pantalla_2018-06-14_15-26-32

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Jun 14, 2018

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

@agjohnson agjohnson merged commit cedda9b into master Jun 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agjohnson agjohnson deleted the humitos/async/initial-build branch Jun 14, 2018

humitos added a commit that referenced this pull request Aug 21, 2018

Route task to proper queue
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 a commit that referenced this pull request Aug 21, 2018

Route task to proper queue
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.

dojutsu-user added a commit to dojutsu-user/readthedocs.org that referenced this pull request Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment