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

Allow to custom content type when setting mailer body #27227

Merged

Conversation

MQuy
Copy link
Contributor

@MQuy MQuy commented Nov 30, 2016

Summary

Allow to custom content-type in case of setting headers[:body] and attachments

attachments[:rails] = xxx
mail(
  body: "<html><body>Hello rails</body></html>",
  content_type: "text/html"
)
# => content_type is now text/hml

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@MQuy MQuy force-pushed the allow-custom-content-type-in-mail-body branch from 270ecea to c4639b7 Compare November 30, 2016 14:18
@sgrif
Copy link
Contributor

sgrif commented Nov 30, 2016

Thank you for the pull request. Can you add documentation and a changelog entry?

@MQuy MQuy force-pushed the allow-custom-content-type-in-mail-body branch 4 times, most recently from ad3ecfa to 97bd115 Compare December 1, 2016 02:07
@MQuy MQuy force-pushed the allow-custom-content-type-in-mail-body branch from 97bd115 to 2263769 Compare December 1, 2016 02:18
@MQuy
Copy link
Contributor Author

MQuy commented Dec 1, 2016

@sgrif thank for your feedback, I added changelog and release note.
About document, I wonder where should I add because it is just the option which we miss to check in case of attachment and header body for mailer

@@ -140,6 +140,11 @@ class BaseTest < ActiveSupport::TestCase
assert_equal("multipart/mixed", email.mime_type)
end

test "set mime type to text/html when attachment is inclued and body is set" do

Choose a reason for hiding this comment

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

You've a typo for 'included' here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ty @benlovell :D

@sgrif
Copy link
Contributor

sgrif commented Dec 5, 2016

I would document it on the mail method

@MQuy MQuy force-pushed the allow-custom-content-type-in-mail-body branch from 0311620 to 40b1f64 Compare December 6, 2016 05:48
@MQuy
Copy link
Contributor Author

MQuy commented Dec 6, 2016

@sgrif thank for your feedback, I just add documentation for mail method :)

def collect_responses_from_text(headers)
[{
body: headers.delete(:body),
content_type: headers[:content_type] || self.class.default[:content_type] || "text/plain"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgrif should we consider to use different name here like body_type instead of content_type, because there are some cases mailer will have many parts like mailer content_type, body content_type

@MQuy
Copy link
Contributor Author

MQuy commented Jan 6, 2017

@sgrif i wonder is there anything lefts should i do?

def collect_responses_from_text(headers)
[{
body: headers.delete(:body),
content_type: headers[:content_type] || self.class.default[:content_type] || "text/plain"
Copy link
Member

Choose a reason for hiding this comment

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

I think that at this point headers[:content_type] already has the value of self.class.default[:content_type].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I try and debug, they are different value, because they comes from mail method and class default

Copy link
Member

Choose a reason for hiding this comment

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

This method is only called after the apply_defaults method is called and the headers passed to this method as argument is the result of apply_defaults, so unless apply_defaults is broken. self.class.default[:content_type] should be already applied on headers[:content_type].

Copy link
Contributor Author

@MQuy MQuy Jan 6, 2017

Choose a reason for hiding this comment

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

I think apply_defaults only applies key into headers when headers doesn't have that key. In this case, we want to use the value from headers in mail method instead default from self.class.default[:content_type] 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If :content_type is passed to mail, apply_defaults will not copy the default value to the headers and headers[:content_type] will be the same as the :content_type passed to mail.

If :content_type is not passed to mail, apply_defaults will copy the default value to the headers and headers[:content_type] will be the same as the self.class.default[:content_type.

So at this point headers[:content_type] behaves exactly as we want.

Copy link
Contributor Author

@MQuy MQuy Jan 6, 2017

Choose a reason for hiding this comment

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

headers[:content_type] will be the same as the :content_type passed to mail

but self.class.default[:content_type] will get value from default method in actionmailer

Copy link
Member

Choose a reason for hiding this comment

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

Is there another case different of those two?

If :content_type is passed to mail, headers[:content_type] will be the value passed to mail. In this case no matter what self.class.default[:content_type] is.

In the case you show the conditional:

headers[:content_type] || self.class.default[:content_type] || "text/plain"

will return the value of :content_type passed to mail, that is the same as headers[:content_type], unless I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hum, so in this case the condition is just headers[:content_type] || "text/plain" because headers[:content_type] always contains the value of self.class.default[:content_type], right? 😄

Copy link
Member

Choose a reason for hiding this comment

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

Right!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will refactor it, thank you so much 🙇

@MQuy
Copy link
Contributor Author

MQuy commented Jan 6, 2017

@rafaelfranca thank for your suggestion 🙇, please let's me know if is there anything else need to be improved :D

@rafaelfranca rafaelfranca merged commit f091bd6 into rails:master Jan 6, 2017
rafaelfranca added a commit that referenced this pull request Jan 6, 2017
…-body

Allow to custom content type when setting mailer body
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants