django-wiki / django-nyt

Notification system for Django with batteries included: Email digests, user settings, JSON API
Apache License 2.0
144 stars 47 forks source link

Use Celery for email notifications and digests #13

Closed benjaoming closed 8 years ago

benjaoming commented 9 years ago

Currently, all kinds of notifications, instant or not, can only be sent by email through a management command.

By adding Celery support, we can have emails sent out when new notifications have been created and send out daily digests etc. through Celery as well...

VCAMP commented 8 years ago

Hey Ben. I could do some work on this if you're still interested - do you have any thoughts/suggestions on how to proceed? (i.e. should Celery become a required dependency, etc.)

benjaoming commented 8 years ago

@VCAMP thanks! I've had some unfortunate encounters with Celery since when I wrote the above. Not that I discourage using it, but more that I don't think it's suitable as a dependency due to it's big size and hard setup process. It's very certain that Celery cannot become a dependency. But it might be a good idea to ensure that django-nyt's utility functions are easy to hook into Celery and that it's documented...

So in brief: :+1: for PRs that remove obstacles for Celery integration! :)

VCAMP commented 8 years ago

What I think I could do:

benjaoming commented 8 years ago

@VCAMP I think Celery automatically searches for a tasks.py ? You should be able to use shared_task decorator to make a generic task definition.

Possibly, you could define a sort of django model signal to call the Celery task when there's a Notification post_save signal.

But the trick is not to import Celery anywhere in the existing codebase. If you can keep it in a tasks.py, that would signal to Celery users where to look because it's conventional.

Agreeing on the setting stuff, perhaps something like this:

models.py


if settings.CELERY:
    import .tasks
    post_save.connect(..., partial(tasks.notify.delay, 0))

I've never tried Celery with scheduling stuff, but perhaps leave some of the implementation for people to finalize the configuration. A new Celery is coming out and the API is changing dramatically so it wouldn't be nice to have to change too many things.

benjaoming commented 8 years ago

At this point, having integrated channels in #21, I'd much rather cut Celery out of the loop in terms of trying to achieve some auto-configured utopia :)

Django-channels (arriving in Django 1.10) will support asynchronous workers dispatching notifications through websockets and sending emails as background tasks.

However, it won't support scheduling within the first release, but it's stated as an intention to have this. To achieve scheduling, I'd just add the notifymail command as a cronjob.

benjaoming commented 8 years ago

A few words added on Celery integration here, feel free to pitch in :)

http://django-nyt.readthedocs.org/en/latest/emails.html