Email: render template before sending it to the task

[stsewd]

The txt formats are the same as the html ones, but with tags removed and small changes to include links along the text.

Closes https://github.com/readthedocs/readthedocs.org/issues/9466

---

:books: Documentation previews :books:

• User's documentation (docs): https://docs--9538.org.readthedocs.build/en/9538/

• Developer's documentation (dev): https://dev--9538.org.readthedocs.build/en/9538/

[humitos]

@agjohnson

These emails were all templated and sent from a task to avoid synchronous delays with web requests.

This is a good point that I didn't think about. I don't think it will be a problem since these templates are small and I don't think they will take too much time to render that could produce a timeout on the web request.

@stsewd One thing that we should test before deploying this is if the Celery message inserted in the queue is not too big due the whole HTML + txt files are going to be on the message itself. Does Celery have a setting for the max size of the message that we can check?

[stsewd]

Does Celery have a setting for the max size of the message that we can check?

Didn't find a setting related to that

[agjohnson]

I don't think it will be a problem since these templates are small and I don't think they will take too much time to render that could produce a timeout on the web request.

Aye, I feel this is safe as well, though don't quote me on this either. At some point we must have had some variable that required an external lookup or something that would block the web request during template render.

[humitos]

@stsewd @agjohnson are we happy moving forward with this PR?

@stsewd have you tested this locally to check the rendered template does fit on the Celery queue without problems?

[stsewd]

@stsewd have you tested this locally to check the rendered template does fit on the Celery queue without problems?

It works locally.

[stsewd]

Another approach would be to change the notifications classes to pass the attributes of the object to be used in the context instead of the object itself, but that feels more error-prone than just passing the object.

[agjohnson]

I don't have any objections, I was just raising a note about the performance. If you all feel this is safe, I'd agree this shouldn't be an issue.

@humitos brings up a good point on the Celery internals. I'm not aware of a Celery specific limit here, but we are using AWS Elasticache in production (as opposed to Redis in development). Elasticache, or Elasticache + Celery, could have different limits here.

[benjaoming]

Re: queue object and size limits

The rendered email content is likely smaller than a Django context object, since it for instance contains request from django.template.context_processors.request.

If there is a known limit to celery task data, it would be nice of Celery to raise that exception. Moreover, Redis and RabbitMQ both have a maximum value size of 512 MB: https://stackoverflow.com/a/5624569/405682

I was reading a bit about Kombu, and I think there's some "Quality of Service" involved when a worker receives messages as well. For all I can tell, these message brokers and task queues are designed to handle significantly larger task data than some text email.

I don't think this is an issue -- and if it is, then we might find more issues.