Skip to content
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

Add rendered preview in admin. Fixes: #325 #383

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

wolph
Copy link

@wolph wolph commented Nov 30, 2021

As discussed in #325, this adds an easy to use preview in the admin:
image

Copy link
Collaborator

@jrief jrief left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before fixing my code annotations, I'd rather rethink, if it doesn't make more sense to se a WYSIWYG-Editor straight ahead. This would be my preferred long term solution.

Thanks for your work, but this new feature must be approved by @selwin anyway. As I said, this shall only be a temporary solution.

post_office/admin.py Outdated Show resolved Hide resolved
post_office/admin.py Outdated Show resolved Hide resolved
post_office/admin.py Outdated Show resolved Hide resolved
post_office/admin.py Outdated Show resolved Hide resolved
post_office/admin.py Outdated Show resolved Hide resolved
post_office/admin.py Outdated Show resolved Hide resolved
post_office/admin.py Outdated Show resolved Hide resolved
post_office/admin.py Outdated Show resolved Hide resolved
@wolph
Copy link
Author

wolph commented Dec 1, 2021

The pull request definitely needs cleanup, but since I didn't get a reply in the original issue anymore I thought it would be good to create a pull request so it doesn't get lost :)

Personally I think that this addition is useful even if you would have a WYSIWYG editor. The renderings of those editors are almost always different from a regular render. It's not my call however, if the WYSIWYG editor is the preferred solution than I'll close the pull request and wait for that release.

With that in mind... a form to send out a test-mail would be very useful as well.

@selwin
Copy link
Collaborator

selwin commented Dec 9, 2021

Having rendered HTML preview in admin would be great.

Aside from @jrief 's comments, please make sure that all JS are stripped to prevent XSS attacks.

@wolph
Copy link
Author

wolph commented Dec 11, 2021

Having rendered HTML preview in admin would be great.

Aside from @jrief 's comments, please make sure that all JS are stripped to prevent XSS attacks.

The easiest and safest way would be to sandbox the iframe, is that enough or should I also do a few search/replaces?
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-sandbox

@selwin
Copy link
Collaborator

selwin commented Dec 11, 2021

@wolph we have https://bleach.readthedocs.io/ as optional dependency. We can use that to easily strip all JS.

@selwin
Copy link
Collaborator

selwin commented Dec 11, 2021

I think we can even make bleach a required dependency @jrief . This rendered preview is a big quality of life improvement in my opinion.

@wolph wolph force-pushed the master branch 2 times, most recently from 155c969 to db00311 Compare December 12, 2021 02:31
@wolph
Copy link
Author

wolph commented Dec 12, 2021

I've cleaned up the commits and added the clean_html() call.

I should note that there is another similar attack vector in the code-base outside of this addition:

def render_plaintext_body(self, instance):
for message in instance.email_message().message().walk():
if isinstance(message, SafeMIMEText) and message.get_content_type() == 'text/plain':
return format_html('<pre>{}</pre>', message.get_payload())

And possibly this one:

def shortened_subject(self, instance):
if instance.template:
template_cache_key = '_subject_template_' + str(instance.template_id)
template = getattr(self, template_cache_key, None)
if template is None:
# cache compiled template to speed up rendering of list view
template = Template(instance.template.subject)
setattr(self, template_cache_key, template)
subject = template.render(Context(instance.context))
else:
subject = instance.subject
return Truncator(subject).chars(100)
shortened_subject.short_description = _("Subject")
shortened_subject.admin_order_field = 'subject'

@wolph wolph force-pushed the master branch 2 times, most recently from 5929eaa to 0f98a1a Compare December 12, 2021 02:41
@@ -269,6 +269,7 @@ class EmailTemplate(models.Model):
default='', blank=True)
default_template = models.ForeignKey('self', related_name='translated_templates',
null=True, default=None, verbose_name=_('Default template'), on_delete=models.CASCADE)
example_context = context_field_class(_('Context'), blank=True, null=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding a new field to the model? Adding a preview to admin shouldn't necessitate adding a new field.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the variable name is self explanatory, but it's to add a context to the rendered preview so all variables can be rendered with a useful value.

]
inlines = (EmailTemplateInline,) if settings.USE_I18N else ()
formfield_overrides = {
models.CharField: {'widget': SubjectField}
}
change_form_template = 'admin/post_office/EmailTemplate/change_form.html'

def change_view(self, request, object_id, form_url='', extra_context=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this dedicated preview functionality to a new view?

@selwin
Copy link
Collaborator

selwin commented Dec 31, 2021

@wolph gentle reminder to update this PR so we can merge this in.

@gabn88
Copy link
Contributor

gabn88 commented Jul 22, 2022

@wolph @selwin This sounds like a great feature that I might use right away in my project. I have been reading the code and the idea from the beginning. And it makes me ask myself, should we need an extra STATUS_CHOICES on the Email Model? Something like 'draft'? Or am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants