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

Text-only email templates #5034

Open
Medvezo opened this issue Feb 26, 2019 · 7 comments
Open

Text-only email templates #5034

Medvezo opened this issue Feb 26, 2019 · 7 comments

Comments

@Medvezo
Copy link

Medvezo commented Feb 26, 2019

Problem

By default Devise comes with html templates. The problem with that is that some spam catching systems may consider that as a negative signal and route email to spam folder. That is worrisome because we are talking about transactional email. Think of all extra customer support emails that will generate in the long run.

Solution

Ship with text-only templates for the next major release (5.0.0).

References

@tegon
Copy link
Member

tegon commented Aug 1, 2019

Hi @Medvezo, thanks for bringing this up.

I agree with you that is important to include text versions of the emails, but do you think we need to send only them, without the HTML versions?

By reading the references you pointed out, all of them say that it is safe to send both, as long as the text and links are similar to HTML version. I've also worked on applications that did something similiar in the past and it worked well.

Thoughts?

@JanBussieck
Copy link

I was also going to bring this up, I solved this by overwriting the default devise templates. Sending both works well and only sending html email might be linked to being sent to gmails notorious promotion tab :)

I could start work on this, drawing on what we have done so far and open a PR if you think this is interesting to the broader user base.

@tegon
Copy link
Member

tegon commented Sep 19, 2019

@JanBussieck That would be great!

@tegon tegon removed the Discussion label Sep 27, 2019
@colinross
Copy link
Contributor

colinross commented Oct 10, 2019

If we supplied the text formats alongside the html (in app/views/devise/mailer) wouldn't this automagically send multi-part via ActionMailer?

ref: https://guides.rubyonrails.org/action_mailer_basics.html#sending-multipart-emails

You could then essentially configure which format (or both by default) get sent via action mailer's config options. It wouldn't be specific to devise, but I would expect the domain requirement would be consistent anyways, right?

default
config.action_mailer.default_options = { parts_order: ["text/plain", "text/enriched", "text/html"] }
text-only
config.action_mailer.default_options = { parts_order: ["text/plain"] }
html-only
config.action_mailer.default_options = { parts_order: ["text/html"] }

Edit:
For backwards compatibility, we could also override the devise_mail call to only include html format as a default (current behavior) by appending parts_order: ["text/html"] in the options passed off in headers_for`.

ref: https://github.com/plataformatec/devise/blob/master/lib/devise/mailers/helpers.rb#L31

@tegon
Copy link
Member

tegon commented Oct 14, 2019

@colinross

If we supplied the text formats alongside the html (in app/views/devise/mailer) wouldn't this automagically send multi-part via ActionMailer?

I think so 👍

@colinross
Copy link
Contributor

I've put a bit of time into this and wanted to report back a bit on my findings so far.

If the goal is just to send multipart emails, it is a small code change (just adding the templates). Where things get messy is dealing with backwards compatibility and not wanting to release a change that essentially locks people into a multipart-emails as a requirement. I think this needs to be an opt-in/explicit configuration feature. There are way too many deployments of devise with very customized emails to just rollout this change otherwise, imho.

Due to the way that rails (actionmailer specifically) handles formats (similar to other types of views) if the formats exist, they will be used. This means that if we add them to the gem-based views ( app/views/devise/mailer) they get picked up implicitly and are used. This also means that if the deployment is using custom mailers, they may end up with a mix of custom-html and gem-supplied plain text. We can't make assumptions the contents are rationally similar. I've worked on projects that used password reset emails as the 'welcome' email since the accounts were automatically created either on demand or before hand in non-public applications for instance.

So, what is the only 'safe' way of rolling this out?
We could either do nothing (just beef up the documentation around mailer views saying 'if you want multipart, make a confirmation_instructions.text.erb view template).
Alternatively, we could only have the plain text views populated via the views rake task, but not in the gem's view path.

Other than that, I think we have painted ourselves into a box a bit as far as a graceful upgrade path here.

More technically, we loose a lot of control in preventing formats from going out since devise_mail accepts a block. This isn't bad persay (devise is meant to be highly customizable), but it does mean that we can either support custom blocks, or we can support configuration of what formats go out (not solely based on the formats available in the view_paths due to the above issues). You control the format by what formats get called in the block. Please open my eyes, but i think ruby runs into void concerning some concept of a super block. We also run into issues when trying to use the new multipart emails with scoped views.

I want to chew on this a bit more with some concrete tests to see what happens in the various scenarios, but overall – I don't think we are ready to implement this 'as a small change' and as such, I want to see if there is a better approach, maybe even shipping a Devise::Mailer vs Devise::MultipartMailer that would allow for breaking changes and a more graceful upgrade path.

@colinross
Copy link
Contributor

I circled back a bit, it seems that even if you override the parts_order and omit plain text, it is still included, confirming for me a bit that parts_order is really just about order and isn't an array of the parts to include.

I know this is a bit to get through, but before moving forward, I'd want a bit more buy-in from the group and team as to the direction they'd like to see this move.

In the meantime (time permitting) I was going to dig more into the ActionMailer::Collector part of the mail generation to see if I can find a legit way to better control the formats produced.

phinze added a commit to chicago-tool-library/circulate that referenced this issue Jan 20, 2024
Hopefully this addresses the problem in #1245 - we'll need to test
deliverability in production to know if it fixes it or not.

See heartcombo/devise#5034 for some good
background on this approach.
phinze added a commit to chicago-tool-library/circulate that referenced this issue Jan 20, 2024
# What it does

Hopefully this addresses the problem in #1245 - we'll need to test
deliverability in production to know if it fixes it or not.

# Why it is important

We don't want any emails to be marked as spam - especially password
reset emails!

# Implementation notes

* See heartcombo/devise#5034 for some good
background on this approach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants