[Emails] Template context variables documentation links #2491
Conversation
pbanaszkiewicz
force-pushed
the
feature/2482-template-context-variables
branch
from
717c754 to
5542caa
Compare
pbanaszkiewicz
force-pushed
the
feature/2482-template-context-variables
branch
from
5dd4ee7 to
449e668
Compare
|
@elichad ready for your review! Thanks. |
| <strong>Template variables</strong> | ||
| </div> | ||
| <div class="col-12 col-lg-10"> | ||
| Visible after saving the template. |
There was a problem hiding this comment.
Hmm, it would be nice to show the variables as soon as a signal is selected, but I know that would increase the complexity a lot (is it even possible for this case?)
I think we can manage with this and make it part of the workflow to save before entering the full body text, since we shouldn't be creating tons of templates.
There was a problem hiding this comment.
I think it's not impossible, but since I have a ton of other features to implement I didn't focus on this case.
One option I can see is to load the variables and their definitions to a JSON object, then switch display in this depending on what user is choosing in the dropdown.
There was a problem hiding this comment.
That sounds like a nice idea – could you make an issue for it and we can come back to it later in the project?
| <div class="col-12 col-lg-2"> | ||
| <strong>Template variables</strong> | ||
| </div> | ||
| <div class="col-12 col-lg-10"> |
There was a problem hiding this comment.
Could you add some help text either here or on the "Email body" field to make explicit that these are variables that can be referenced in the text box?
There was a problem hiding this comment.
Also, is there any validation we can do to check that the referenced fields exist when these variables are used in the template? e.g. if I tried to use {{ person.not_a_field }}
Right now it just renders as XXX-unset-variable-XXX when an email is scheduled, which at least doesn't cause a server error, but it happens quite quietly.
There was a problem hiding this comment.
I'm going to update the wording so that it's clearer what they refer to.
About template validation: yes, I think I could build that in here. The previous system contained similar validation (for proper Django template format).
There was a problem hiding this comment.
@elichad Actually the validation is not possible at this point.
We'd get the best results if we used Django Template Engine behavior for checking if all variables are accessible. However, to do that we have to have context with real data. At this point we're working with email templates without any context, so proper validation is not possible.
| </div> | ||
| <div class="col-12 col-lg-10"> | ||
| <ul> | ||
| {% for key, value in body_context_annotations.items %} |
There was a problem hiding this comment.
Can these template variables still be used when editing a scheduled email? I had assumed the templates were rendered once and weren't re-rendered before sending.
If they can't be used, this row should be removed from here and scheduled_email_detail.html.
There was a problem hiding this comment.
@elichad You're correct.
There are 2 renderings:
- raw user input --(django template enigne)-> Markdown format
- Markdown format --(markdown compiler)-> HTML
Currently first rendering occurs when an email is scheduled, and the second should be performed by the worker (at the moment it isn't - I will get back to this).
The advantage of this approach is simplicity, but the clear disadvantage is the final form of the email which can't be updated with newer versions of the objects included. So potentially we need to shift the rendering more towards the worker - it would have to render Django template and then Markdown.
I'll remove these lines, however they may be brought back in future.
… and scheduled email
Using mocks was necessary because with metaprogramming Django changes Model.__class__ to ModelBase and therefore (only in these tests!) it wasn't possible to use Person or other models. A trick was to use MagicMock with `.__class__.__name__` attribute overwritten.
…cheduled email update views
They aren't used in this layer.
Updated sections containing variables available in template body.
pbanaszkiewicz
force-pushed
the
feature/2482-template-context-variables
branch
from
3d019ad to
cb98109
Compare
|
@elichad Please take a look again and consider my responses to your questions. Thanks! |
This fixes #2482 by providing links to documentation for every variable used in email template context (e.g. person, event, award, etc.).
It builds on top of #2481.
WIP. Needs testing and perhaps some code improvements / refactoring.