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

Inlined images template engine #260

Merged
merged 25 commits into from Apr 21, 2019
Merged

Conversation

jrief
Copy link
Collaborator

@jrief jrief commented Mar 24, 2019

This is an extension of pull request #251. It allows users to create emails using inlined images.
There is a special template engine, which shall be used in combination with a special templatetag. It keeps track on the attached images and adds them as inline-attachments.

@jrief
Copy link
Collaborator Author

jrief commented Mar 24, 2019

Some unit tests fail because either Python is too old (<3.5), or Django is too old (<1.10).
I would either have to backport some methods to Django-1.8/1.9 and Python-3.4 or drop these versions. Since it is very unlikely that anyone requires these new features, but still wants to stick with a deprecated version of Python or Django, I would opt for the latter. What do you think?

@selwin
Copy link
Collaborator

selwin commented Mar 29, 2019

I'm ok with dropping support for Django < 1.11. Would it be troublesome to support Python 3.4 (it's supported by Django 1.11)?

@jrief
Copy link
Collaborator Author

jrief commented Mar 29, 2019

I'm ok with dropping support for Django < 1.11.

OK, then I will change this in this pull request.

Would it be troublesome to support Python 3.4 (it's supported by Django 1.11)?

No, it's very simple to backport this.

Give me some time to change it.

@jrief
Copy link
Collaborator Author

jrief commented Mar 29, 2019

OK, dropped support for Django<1.10 and Python-3.3.

post_office/models.py Outdated Show resolved Hide resolved
@@ -0,0 +1,54 @@
from __future__ import unicode_literals
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename this file to template/backends/__init__.py and the engine to PostOffice? This way we can refer to this engine as 'post_office.template.backends.PostOffice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can rename it to template/backends/post_office.py if you want, but not to __init__.py. Reason is that this name is used in the settings.py for POST_OFFICE['TEMPLATE_ENGINE'] and therefore we need a meaningful name here.
This btw. was the main reason, to use html_email rather than post_office as the template's engine name, because I wanted to emphasize that it is intended for HTML email rendering.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about moving this to /post_office/template/backends.py then? Name the engine HTMLEmailEngine or HTMLEmail so it can be specified as 'post_office.template.backends.HTMLEmailEngine.

The built in Jinja2 engine can be imported as django.template.backends.jinja2.Jinja2. I think this is a better naming convention than DjangoTemplates (it's not a template).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem are these functions: django.template.loader.get_template, django.template.loader.select_template and django.template.loader.render_to_string. They all take a parameter using, which in our case would be get_template('/path/to/message.html', using='html_email'). Here html_email is the name of the module in backends. If we move it to backends.py, one would have to call get_template('/path/to/message.html', using='backends') – not a very good name.

As said, I have no problem using post_office here.

Copy link
Collaborator Author

@jrief jrief Apr 7, 2019

Choose a reason for hiding this comment

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

I just renamed the template engine to post_office.PostOfficeTemplates. I would really like to keep "Templates" inside the name, because that is what Django does with its own template engine: django.template.backends.django.DjangoTemplates.

…ngo-post_office into inlined-images-template-engine
@jrief
Copy link
Collaborator Author

jrief commented Apr 7, 2019

Please answer me on the debatable review annotations, so that I can fix them as soon as possible.

@@ -20,7 +20,7 @@ def attach_related(self, email_message):
email_message.attach(attachment)


Copy link
Collaborator

@selwin selwin Apr 8, 2019

Choose a reason for hiding this comment

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

Can we rename this file from post_office/template/backends/post_office.py to post_office/template/backends.py?

EDIT: never mind this comment, I didn't see your reply here: #260 (comment)

@jrief
Copy link
Collaborator Author

jrief commented Apr 8, 2019

Can I expect any time soon a new release? I'm asking because some of my apps depend on django-post_office and I wanted to know, if I shall handle the project's requirements through PyPI or GitHub.

@selwin
Copy link
Collaborator

selwin commented Apr 8, 2019 via email

@jrief
Copy link
Collaborator Author

jrief commented Apr 16, 2019

@selwin if you want, I can take over the job of releasing new versions.

I already do this for other Django apps maintained by other people, for instance easy-thumbnails and django-treebeard.

I then would also ensure that the Changelog is always up to date, and that version numbers are tagged correctly to the code in the repository. In the past there unfortunately have been various mismatches: #252 , #151 and #131 .

@selwin selwin merged commit f8403b6 into ui:master Apr 21, 2019
@selwin
Copy link
Collaborator

selwin commented Apr 21, 2019

@jrief sorry for the delay. Would be glad to have your help in coordinating releases. How do other projects usually do this? Do they just invite you as a collaborator in their respective repos?

@jrief
Copy link
Collaborator Author

jrief commented Apr 22, 2019

Yes, on GitHub you can invite someone as collaborator.
On PyPI you can add me as Maintainer.

Be assured that I will never merge new features without your review.
In projects I do not own, I usually only merge bug-fixes.

@jrief jrief deleted the inlined-images-template-engine branch April 23, 2019 08:17
@selwin
Copy link
Collaborator

selwin commented Apr 28, 2019

@jrief I have added you on both Github and PyPI. Please check your inbox :).

@jrief
Copy link
Collaborator Author

jrief commented Apr 29, 2019

@selwin Thanks, I've seen and accepted.
Shall I now bundle the next minor release, ie. 3.2?

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.

None yet

2 participants