readthedocs.org icon indicating copy to clipboard operation
readthedocs.org copied to clipboard

Email: render template before sending it to the task

Open stsewd opened this issue 1 year ago • 1 comments

Currently, we send the email templates and the context to a task, and inside that task they are rendered

https://github.com/readthedocs/readthedocs.org/blob/26d60be80f9f27d381441a838a6b4ba46688d079/readthedocs/core/utils/init.py#L248-L251

https://github.com/readthedocs/readthedocs.org/blob/26d60be80f9f27d381441a838a6b4ba46688d079/readthedocs/core/tasks.py#L50 https://github.com/readthedocs/readthedocs.org/blob/26d60be80f9f27d381441a838a6b4ba46688d079/readthedocs/core/tasks.py#L56

Since the context is passed to a task, it needs to be serialized to json, this means that we can't pass an object as a context, and instead we need to pass each value of the object explicitly (and using primitive types), we have been bitten by this in the past.

If we render the template before passing it to the task, then we won't need to worry about passing an object.

For email notifications, we are passing an object in the context

https://github.com/readthedocs/readthedocs.org/blob/26d60be80f9f27d381441a838a6b4ba46688d079/readthedocs/notifications/notification.py#L55

I think that's the reason why we are sending html for both, the txt and html versions of email notifications (the templates are fine, but the content is always html, which is weird and broken).

https://github.com/readthedocs/readthedocs.org/blob/26d60be80f9f27d381441a838a6b4ba46688d079/readthedocs/notifications/backends.py#L64-L76

Was there a reason why we are rendering the template inside the task?

stsewd avatar Aug 02 '22 17:08 stsewd

I think this makes sense. I don't have a reason in mind that stop us from rendering the template before triggering the task. The only thing I can think of is in case the resulting HTML string is too big, but I don't think that's our case, tho.

humitos avatar Aug 03 '22 17:08 humitos