-
Notifications
You must be signed in to change notification settings - Fork 345
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
Add support for replyto parameter with SendGrid #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few very small comments. Thanks so much for this. It will be very helpful! I'll merge this in as soon as the comments are addressed
@@ -156,6 +157,14 @@ defmodule Bamboo.SendgridAdapter do | |||
defp put_text_body(body, %Email{text_body: nil}), do: body | |||
defp put_text_body(body, %Email{text_body: text_body}), do: Map.put(body, :text, text_body) | |||
|
|||
# If you would like to add a replyto header to your email, then simply pass it in | |||
# using the header property or put_header function like so: | |||
# put_header("reply-to", "foo@bar.com") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment would be better in the moduledoc at the top. Right above # Example Config
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good call 👍
# or you can use functions to build it up step by step | ||
base_email | ||
|> to(user) | ||
|> subject("Welcome!") | ||
|> text_body("Welcome to the app") | ||
|> html_body("<strong>Welcome to the app</strong>") | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catches :)
@@ -144,6 +144,17 @@ defmodule Bamboo.SendgridAdapterTest do | |||
}} | |||
end | |||
|
|||
test "deliver/2 correctly formats sendto from headers" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe sendto
should be reply-to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 👍
test "deliver/2 correctly formats sendto from headers" do | ||
email = new_email( | ||
headers: %{"reply-to" => "foo@bar.com"} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would look a bit better all on one line. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely
@cjpwrs If you are busy I'm happy to finish this off for you. LMK if you need any help :D |
@paulcsmith Hey sorry I just saw these comments! Thanks for the review! I'll make those changes real quick and let you know once they are pushed up. |
@paulcsmith Alright those changes have been made and merge conflicts resolved. Let me know if you find anything else you want me to change. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny comment. The rest looks great. I'll merge as soon as you get to that last comment. Thanks a ton!
@@ -5,6 +5,10 @@ defmodule Bamboo.SendGridAdapter do | |||
Use this adapter to send emails through SendGrid's API. Requires that an API | |||
key is set in the config. | |||
|
|||
If you would like to add a replyto header to your email, then simply pass it in | |||
using the header property or put_header function like so: | |||
put_header("reply-to", "foo@bar.com") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you put this one more line down and indent it 4 spaces it will be highlighted as code, which I think would look nice and draw the reader's attention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulcsmith Oh nice that's good to know! K, that change is pushed 👍
I user can pass in a
reply-to
header, and the SendGrid adapter will format that so that a replyto parameter can be set on an email. This does not change any current functionality, simply adds a new feature to the SendGrid adapter.