Skip to content

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

Open
wolph wants to merge 5 commits into
ui:masterfrom
wolph:master
Open

Add rendered preview in admin. Fixes: #325#383
wolph wants to merge 5 commits into
ui:masterfrom
wolph:master

Conversation

@wolph

@wolph wolph commented Nov 30, 2021

Copy link
Copy Markdown

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

@jrief jrief left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread post_office/admin.py Outdated
Comment thread post_office/admin.py Outdated
Comment thread post_office/admin.py Outdated
Comment thread post_office/admin.py Outdated
Comment thread post_office/admin.py Outdated
Comment thread post_office/admin.py Outdated
Comment thread post_office/admin.py Outdated
Comment thread post_office/admin.py Outdated
@wolph

wolph commented Dec 1, 2021

Copy link
Copy Markdown
Author

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

selwin commented Dec 9, 2021

Copy link
Copy Markdown
Collaborator

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

wolph commented Dec 11, 2021

Copy link
Copy Markdown
Author

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

selwin commented Dec 11, 2021

Copy link
Copy Markdown
Collaborator

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

@selwin

selwin commented Dec 11, 2021

Copy link
Copy Markdown
Collaborator

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

wolph commented Dec 12, 2021

Copy link
Copy Markdown
Author

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
Comment thread post_office/models.py
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
Copy Markdown
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
Copy Markdown
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.

Comment thread post_office/admin.py
}
change_form_template = 'admin/post_office/EmailTemplate/change_form.html'

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

Copy link
Copy Markdown
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

selwin commented Dec 31, 2021

Copy link
Copy Markdown
Collaborator

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

@gabn88

gabn88 commented Jul 22, 2022

Copy link
Copy Markdown
Contributor

@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