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

Optionally pass in 'html' template function to Accounts email templates #1785

Closed
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
8 participants
@zol
Contributor

zol commented Jan 24, 2014

PR against #1784 . Do you guys agree with the approach I've used here, that is making 'text' emails required and 'html' optional?

This is the last piece of functionality on our fork of Meteor that's not yet integrated into mainline, it would be awesome to see it merged.

@glasser

This comment has been minimized.

Member

glasser commented Feb 10, 2014

I closed your issue a dupe of #1545. This PR looks like a good start, but as mentioned on #1545, we would like tests; see accounts-password/email_tests.js.

@markdowney

This comment has been minimized.

markdowney commented Feb 11, 2014

Would love to see this merged. Currently hitting the same issue.

@zol

This comment has been minimized.

Contributor

zol commented Feb 11, 2014

@glasser could you please give me some more guidance - in particular, point me to a specific function somewhere in else in the codebase you'd like me to model tests off. The tests in accounts-password/email_tests.js perform more valuable end-end tests but tests for this PR would just be testing that a parameter is detected and passed through - pretty trivial . I'm hesitant to introduce test bloat.

@charlesvallieres

This comment has been minimized.

charlesvallieres commented Feb 28, 2014

We are waiting for this PR to be merged in, we need html emails. I'd be happy to help if there is anything I can do.

@srounce

This comment has been minimized.

srounce commented Mar 3, 2014

+1 for merge.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Mar 4, 2014

Hrm, I'd kinda like to see tests. This is the sort of thing we can break pretty easily without tests. It would be good to see that the functions run and they have the right arguments, like we do for the default text functions, and I don't think its too hard.

I think this looks like:

  • change hookSend in email_tests_setup.js to save the text and HTML arguments in an object, instead of just text.
  • add at the top level (not per test) an html option to each template.
  • the function puts the user info and url in the html body
  • the test looks at the html output option in addition to the text output

I'm not so worried that the template config is global. No one else is going to be testing this email sending, and if they are they probably shouldn't be sensitive to the presence of the html option to intercepted emails. If we're extra fancy we could have some shared state like var outstandingHtmlTokens = {}, have the template function add a token there and put it in the body of the email, and then the test check that they see a token and it is in the outstanding map. But really, I think just seeing that the function is called and puts the right URL and token (matches the text option) is sufficient.

@zol

This comment has been minimized.

Contributor

zol commented Mar 7, 2014

@n1mmy thanks for writing up your expectations. I've added tests based on that minus the var outstandingHtmlTokens = {} enhancement.

n1mmy added a commit that referenced this pull request Mar 13, 2014

@n1mmy

This comment has been minimized.

Member

n1mmy commented Mar 13, 2014

Merged! Thanks, @zol!

@n1mmy n1mmy closed this Mar 13, 2014

@lorensr

This comment has been minimized.

Collaborator

lorensr commented Dec 21, 2014

Why is text required? When I give an html function, are there cases in which the text version of the email be used?

@Slava

This comment has been minimized.

Member

Slava commented Dec 21, 2014

@lorensr I don't think this is the right place to discuss it, but I believe text is used by email clients that don't support html.

Edit: think I found a great link: http://blog.hubspot.com/blog/tabid/6307/bid/32643/Why-Marketers-Must-Optimize-Emails-for-HTML-AND-Plain-Text-Infographic.aspx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment