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

Add Mailer.deliver_now! and deliver_later! #571

Merged
merged 3 commits into from
Feb 19, 2021

Conversation

germsvel
Copy link
Collaborator

@germsvel germsvel commented Dec 28, 2020

Closes: #355, #409, #505

Possibly helps: #403, #346

Supersedes PR: #393

What changed?

We update deliver_now and deliver_later to not raise on errors. They now return an {:ok, email} (or {:ok, email, response} with response: true) or {:error, error}.

We also introduce deliver_now! and deliver_later! for those who liked the previous behavior. Thus, for those who don't want to change the behavior of Bamboo, it should be possible to simply replace what used to be deliver_now and deliver_later with the ! equivalents.

Those who want to handle errors can receive ok/error tuples with deliver_now and deliver_later.

Note on deliver_later and deliver_later!

deliver_later now returns an {:ok, email_to_send} or {:error, error} if there is an error in the email validations before we schedule the email sending.

If you want to keep the existing behavior, use deliver_later!. deliver_later! will raise an error if we have email validation failures before we schedule the email.

Regardless of whether you choose deliver_later or deliver_later!, TaskSupervisorStrategy raises an error because the delivery happens in a separate process. Thus the behavior of the TaskSupervisorStrategy does not change with this commit.

Note on empty recipients (not nil)

While working on this update, I noticed we do not raise an error when to, cc, bcc are all empty lists (after formatting). We decide to keep in line with this behavior, so we do not return an error in those cases either.

We do not want to raise on scenarios the recipient list is empty because that means the user of bamboo interacted with those lists. If all of those fields are nil, we do return an error or raise an error because it means the user of bamboo might not have set anything accidentally (nil is the default value for all of those). That has always been the case, and will continue to be.

Bamboo.ApiError.build_api_error for adapters

We also add a Bamboo.ApiError.build_api_error function that is the counterpart to the raise_api_error. That way, adapters can use the build_api_error function to return {:error, %Bamboo.ApiError{}} tuples instead of raising errors.

Adapter-specific PRs:

@@ -145,7 +145,7 @@ email in fitting places within your application.
defmodule MyApp.SomeControllerPerhaps do
def send_welcome_email do
Email.welcome_email() # Create your email
|> Mailer.deliver_now() # Send your email
|> Mailer.deliver_now!() # Send your email

Choose a reason for hiding this comment

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

Do you think we should include documentation of deliver_now as well in the README?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, maybe it'd be good to add a little more docs in the README about using deliver_now vs deliver_now!. I'll try to take a stab at that.

@@ -35,7 +35,9 @@ defmodule Bamboo.Adapter do
end
"""

@callback deliver(%Bamboo.Email{}, %{}) :: any
@type error :: Exception.t() | String.t()

Choose a reason for hiding this comment

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

🎉

end

@doc false
def deliver_later!(adapter, email, config) do

Choose a reason for hiding this comment

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

I like how the functions with the bangs build on their plain counterparts here. Makes what is changing in this PR clear, too.

end

defp validate_from_address(%{from: {_, nil}}) do
raise Bamboo.EmptyFromAddressError, nil
{:error, %Bamboo.EmptyFromAddressError{}}

Choose a reason for hiding this comment

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

I like that these changes result in more idiomatic elixir, returning the error tuple

Copy link

@lady3bean lady3bean left a comment

Choose a reason for hiding this comment

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

This looks great! The tests look good and made me feel more confident that these changes make sense.

Do you think that since this is a breaking change, there should be more documentation or process when it comes to releasing this?

@germsvel
Copy link
Collaborator Author

germsvel commented Jan 4, 2021

@lady3bean

Do you think that since this is a breaking change, there should be more documentation or process when it comes to releasing this?

Yes, the plan is to do a major bump (to 2.0). Hopefully that causes people to check the changelog. My plan right now (though happy to hear alternatives) was to write a little migration guide from 1.x to 2.0, include that guide in hex docs (just like we include the readme), and link to that guide from the changelog. That way, the migration guide lives with the docs and is also discoverable from the changelog. What do you think?

@lady3bean
Copy link

@lady3bean

Do you think that since this is a breaking change, there should be more documentation or process when it comes to releasing this?

Yes, the plan is to do a major bump (to 2.0). Hopefully that causes people to check the changelog. My plan right now (though happy to hear alternatives) was to write a little migration guide from 1.x to 2.0, include that guide in hex docs (just like we include the readme), and link to that guide from the changelog. That way, the migration guide lives with the docs and is also discoverable from the changelog. What do you think?

That sounds great!

What changed?
=============

We update `deliver_now` and `deliver_later` to not raise on errors. They
now return an `{:ok, email}` (or `{:ok, email, response}` with
`response: true`) or `{:error, error}`.

We introduce `deliver_now!` and `deliver_later!` to replace what used to
be `deliver_now` and `deliver_later` -- in other words, they raise
because that used to be the implementation.

Hopefully, that makes for an easy upgrade path. Those who wish to keep
the current behavior can switch to `deliver_now!` and `deliver_later!`
and keep moving forward.

Those who want to handle errors can receive ok/error tuples.

Note on `deliver_later` and `!`
--------------------------------

`deliver_later` now returns an `{:ok, email_to_send}` or `{:error,
error}` if there is an error in the email validations _before_ we
schedule the email sending. But if an error occurs during the delayed
task, we raise an error.

`deliver_later!` will raise an error if we have email validation
failures before we schedule the email. This is the existing behavior of
`deliver_later`, so it's an easy upgrade for those uninterested in the
new change: `deliver_later` -> `deliver_later!`.

Note on empty recipients (not nil)
-------------------------------

While working on this update, I noticed we did not raise an error when
`to, cc, bcc` are all empty lists (after formatting). We decide to keep
in line with this behavior, so we do not return an error in those cases
either.

We do not want to raise on scenarios the recipient list is empty because
that means the user of bamboo interacted with those lists. If all of
those fields are `nil`, we do return or raise an error because it means
the user of bamboo might not have set anything accidentally (`nil` is
the default value for all of those). That has always been the case, and
will continue to be.
What changed?
============

Since `adapter.deliver` should no longer raise errors, we now raise them
from the supervisor strategy. This keeps the behavior of the
TaskSupervisorStrategy the same as it was before. It'll raise and kill
the child process that was spawned to send the email.
What changed?
=============

This commit adds a `Bamboo.ApiError.build_api_error` function that is a
counterpart to the `raise_api_error`. That way, adapters can use the
`build_api_error` function to return `{:error, %Bamboo.ApiError{}}`
tuples instead of raising errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Potentially breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deliver/2 shouldn't raise exception
2 participants