-
Notifications
You must be signed in to change notification settings - Fork 268
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
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. |
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? |
@wolph we have https://bleach.readthedocs.io/ as optional dependency. We can use that to easily strip all JS. |
I think we can even make |
155c969
to
db00311
Compare
I've cleaned up the commits and added the I should note that there is another similar attack vector in the code-base outside of this addition: django-post_office/post_office/admin.py Lines 179 to 182 in 5297f00
And possibly this one: django-post_office/post_office/admin.py Lines 121 to 135 in 5297f00
|
5929eaa
to
0f98a1a
Compare
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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?
@wolph gentle reminder to update this PR so we can merge this in. |
# Conflicts: # post_office/admin.py
As discussed in #325, this adds an easy to use preview in the admin: