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

Email HTML templates #1673

Merged
merged 35 commits into from Feb 2, 2018
Merged

Email HTML templates #1673

merged 35 commits into from Feb 2, 2018

Conversation

mad-anne
Copy link
Contributor

@mad-anne mad-anne commented Jan 24, 2018

I want to merge this change because it adds compiled HTML templates for e-mails and improves its' content. It introduces MJML as a markup language used for structuring e-mails.

Closes #1662

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. The changes are tested.
  6. The code is documented (docstrings, project documentation).
  7. Python code quality checks pass: pycodestyle, pydocstyle, pylint.
  8. JavaScript code quality checks pass: eslint.

[Edit]

Below are some compiled MJML email templates rendered by Django.

Order confirmation e-mail.

confirm-order

Staff member password set up e-mail.

set-up-password

@patrys
Copy link
Member

patrys commented Jan 24, 2018

We specifically wanted to get rid of premailer as it's not very effective to reprocess templates each time an email is sent.

@mad-anne mad-anne force-pushed the html-emails branch 2 times, most recently from f0f9b3f to d6b177b Compare January 29, 2018 09:13
@codecov-io
Copy link

codecov-io commented Jan 29, 2018

Codecov Report

Merging #1673 into master will increase coverage by 0.08%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1673      +/-   ##
==========================================
+ Coverage      83%   83.08%   +0.08%     
==========================================
  Files         142      142              
  Lines        6071     6077       +6     
  Branches      660      661       +1     
==========================================
+ Hits         5039     5049      +10     
+ Misses        861      858       -3     
+ Partials      171      170       -1
Impacted Files Coverage Δ
saleor/dashboard/emails.py 100% <100%> (ø) ⬆️
saleor/order/emails.py 100% <100%> (ø) ⬆️
saleor/registration/views.py 97.61% <100%> (-0.06%) ⬇️
saleor/order/models.py 88.51% <100%> (ø) ⬆️
saleor/registration/forms.py 82.5% <66.66%> (+5%) ⬆️
saleor/userprofile/models.py 93.84% <0%> (+3.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fc38b9...bee6a90. Read the comment docs.

@mad-anne mad-anne force-pushed the html-emails branch 2 times, most recently from 957023f to 4a99296 Compare January 29, 2018 10:11
@patrys
Copy link
Member

patrys commented Jan 29, 2018

We've discussed two potential solutions in the past. Both would mean keeping a "source" template and a "compiled" template with its styles inlined:

@maarcingebala
Copy link
Member

@patrys There is some wrapper for MJML in Django, we'll check if we could use it.

@patrys
Copy link
Member

patrys commented Jan 29, 2018

@elwoodxblues I was thinking about putting compiled templates (including Django variable placeholders) where Django can find them, not running another server.

Copy link
Member

@patrys patrys left a comment

Choose a reason for hiding this comment

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

Code looks good! I'd love to see a screenshot.

<mj-include path="../../shared/header" />
<mj-section>
<mj-column>
<mj-text font-size="16px">
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like extra space left after attr's deletion? This repeats everywhere that <mj-text font-size="16px"> was copied to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll fix it.

</mj-text>
<mj-text>
{% blocktrans context "Promote customer to staff member confirmation e-mail text" %}
You're receiving this e-mail because you have been promoted to staff member at {{ domain }}.
Copy link
Member

Choose a reason for hiding this comment

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

This template (as well as it's plain text version) could also contain a link to the dashboard, so that receiver of this email could quickly log in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll add it.

@maarcingebala
Copy link
Member

I think we could compile all templates to a single directory, similarly to how webpack compiles the assets to /static/assets/. It would be great if we had simple command in package.json that would do that.

@patrys
Copy link
Member

patrys commented Feb 1, 2018

I think it's worth considering passing the trimmed flag to blocktrans in HTML context.

Copy link
Contributor

@akjanik akjanik left a comment

Choose a reason for hiding this comment

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

LGTM

@maarcingebala maarcingebala merged commit eeb73b7 into saleor:master Feb 2, 2018
@mad-anne mad-anne deleted the html-emails branch February 6, 2018 10:15
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

7 participants