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

Empty recipients #40

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 17 additions & 9 deletions lib/bamboo/adapters/mandrill_adapter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,22 @@ defmodule Bamboo.MandrillAdapter do

Here is the response:

#{inspect response}
#{inspect response, limit: :infinity}


Here are the params we sent:
Here are the params that were sent:

#{inspect Poison.decode!(params)}
#{inspect Poison.decode!(params), limit: :infinity}
"""
%ApiError{message: message}
end
end

def deliver(email, config) do
api_key = get_key(config)
params = email |> convert_to_mandrill_params(api_key) |> Poison.encode!
case request!(@send_message_path, params) do
%{status_code: status} = response when status > 299 ->
raise(ApiError, %{params: params, response: response})
response -> response
if email.to == [] and email.cc == [] and email.bcc == [] do
{:no_recipients, email}
else
send_email(email, config)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should be {:ok, email} so you can pattern match on the result...? Then I'd need to add deliver! and deliver_async! I think, and not raise here if there is an API error, instead return {:error, mandrill_response}

Copy link
Contributor

Choose a reason for hiding this comment

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

Being able to pattern match on the result seems useful. As you said you would need to add deliver! and deliver_async! but that's probably fine.

end
end

Expand All @@ -38,6 +36,16 @@ defmodule Bamboo.MandrillAdapter do
end)
end

defp send_email(email, config) do
api_key = get_key(config)
params = email |> convert_to_mandrill_params(api_key) |> Poison.encode!
case request!(@send_message_path, params) do
%{status_code: status} = response when status > 299 ->
raise(ApiError, %{params: params, response: response})
response -> response
end
end

defp get_key(config) do
case Keyword.get(config, :api_key) do
nil -> raise_api_key_error(config)
Expand Down
6 changes: 3 additions & 3 deletions lib/bamboo/email.ex
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
defmodule Bamboo.Email do
defstruct from: nil,
to: nil,
cc: nil,
bcc: nil,
to: [],
cc: [],
bcc: [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think this should go back to nil and it should raise it all recipients are nil. That would mean you forgot to set 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.

After thinking about this more, I think this should be how it's handled. If it starts as nil and you set an list then you probably meant to do that. I can see it going either way though

Here are a couple scenarios:

  1. You are sending emails to people subcribed to a post. In your tests in dev you set up a post and have people subscribed and everything works just fine. Then in production you have posts with no subscribers. Would you want it to raise? I think I would expect it to just not send an email

  2. You want to send to just one user, in which case you would not have a list. The only error I can imagine would be accidentally passing in nil. e.g. Emails.successfully_published(article.author) where the articles author is nil. In which case I would expect it to raise.

So I think I'm going to go ahead with keep the default addresses as nil, and raising only if all of them are nil, and nooping if the the recipients are an empty array

subject: nil,
html_body: nil,
text_body: nil,
Expand Down
37 changes: 31 additions & 6 deletions lib/bamboo/mailer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,26 @@ defmodule Bamboo.Mailer do

alias Bamboo.Formatter

defmodule NoRecipientError do
defexception [:message]

def exception(email) do
message = """
There was a recipient accidentally set to nil. If you meant to set the
to, cc or bcc fields to send to no one, set it to an empty list [] instead.

Recipients:

To - #{inspect email.to}
Cc - #{inspect email.cc}
Bcc - #{inspect email.bcc}

Full email - #{inspect email, limit: :infinity}
"""
%NoRecipientError{message: message}
end
end

defmacro __using__(opts) do
quote bind_quoted: [opts: opts] do
%{adapter: adapter, config: config} = Bamboo.Mailer.parse_opts(__MODULE__, opts)
Expand All @@ -21,18 +41,23 @@ defmodule Bamboo.Mailer do
end

def deliver(adapter, email, config) do
email = email |> Bamboo.Mailer.normalize_addresses

debug(email)
adapter.deliver(email, config)
email |> validate_and_normalize |> adapter.deliver(config)
end

def deliver_async(adapter, email, config) do
email = email |> Bamboo.Mailer.normalize_addresses
email |> validate_and_normalize |> adapter.deliver_async(config)
end

defp validate_and_normalize(email) do
email = email |> validate_recipients |> normalize_addresses
debug(email)
adapter.deliver_async(email, config)
email
end

defp validate_recipients(%{to: to, cc: cc, bcc: bcc} = email) when is_nil(to) or is_nil(cc) or is_nil(bcc) do
raise NoRecipientError, email
end
defp validate_recipients(email), do: email

defp debug(email) do
Logger.debug """
Expand Down
6 changes: 3 additions & 3 deletions test/lib/bamboo/email_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ defmodule Bamboo.EmailTest do
test "new_email/1 returns an Email struct" do
assert new_email == %Bamboo.Email{
from: nil,
to: nil,
cc: nil,
bcc: nil,
to: [],
cc: [],
bcc: [],
subject: nil,
html_body: nil,
text_body: nil,
Expand Down
17 changes: 17 additions & 0 deletions test/lib/bamboo/mailer_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,23 @@ defmodule Bamboo.MailerTest do
:ok
end

test "deliver/1 raises if any of the recipients are nil" do
assert_raise Bamboo.Mailer.NoRecipientError, fn ->
refute_receive {:deliver, _, _}
new_email(to: nil) |> FooMailer.deliver
end

assert_raise Bamboo.Mailer.NoRecipientError, fn ->
refute_receive {:deliver, _, _}
new_email(bcc: nil) |> FooMailer.deliver
end

assert_raise Bamboo.Mailer.NoRecipientError, fn ->
refute_receive {:deliver, _, _}
new_email(cc: nil) |> FooMailer.deliver
end
end

test "deliver/1 calls the adapter with the email and config" do
FooMailer.deliver(new_email)

Expand Down
25 changes: 19 additions & 6 deletions test/lib/bamboo/mandrill_adapter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,29 @@ defmodule Bamboo.MandrillAdapterTest do

test "raises if the api key is nil" do
assert_raise ArgumentError, ~r/no API key set/, fn ->
new_email |> MailerWithBadKey.deliver
base_email |> MailerWithBadKey.deliver
end
end

test "does not send the email if there are no recipients" do
{:no_recipients, _} = new_email(to: [], cc: [], bcc: []) |> Mailer.deliver
refute_received {:fake_mandrill, _}

email_task = new_email(to: [], cc: [], bcc: []) |> Mailer.deliver_async
{:no_recipients, _} = Task.await(email_task)
refute_received {:fake_mandrill, _}
end

test "deliver/2 sends the to the right url" do
new_email |> Mailer.deliver
base_email |> Mailer.deliver

assert_receive {:fake_mandrill, %{request_path: request_path}}

assert request_path == "/api/1.0/messages/send.json"
end

test "deliver/2 sends from, html and text body, subject, and headers" do
email = new_email(
email = base_email(
from: %{name: "From", address: "from@foo.com"},
subject: "My Subject",
text_body: "TEXT BODY",
Expand All @@ -93,7 +102,7 @@ defmodule Bamboo.MandrillAdapterTest do
end

test "deliver_async sends asynchronously and can be awaited upon" do
email = new_email(
email = base_email(
from: %{name: "From", address: "from@foo.com"},
subject: "My Subject",
text_body: "TEXT BODY",
Expand Down Expand Up @@ -133,7 +142,7 @@ defmodule Bamboo.MandrillAdapterTest do
end

test "deliver/2 adds extra params to the message " do
email = new_email |> MandrillEmail.put_message_param("important", true)
email = base_email |> MandrillEmail.put_message_param("important", true)

email |> Mailer.deliver

Expand All @@ -142,10 +151,14 @@ defmodule Bamboo.MandrillAdapterTest do
end

test "raises if the response is not a success" do
email = new_email(from: "INVALID_EMAIL")
email = base_email(from: "INVALID_EMAIL")

assert_raise Bamboo.MandrillAdapter.ApiError, fn ->
email |> Mailer.deliver
end
end

def base_email(attrs \\ []) do
new_email(to: "foo@bar.com") |> struct(attrs)
end
end