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

Breaking changes coming to Bamboo v2.0.0 #176

Closed
germsvel opened this issue Feb 18, 2021 · 5 comments
Closed

Breaking changes coming to Bamboo v2.0.0 #176

germsvel opened this issue Feb 18, 2021 · 5 comments
Assignees

Comments

@germsvel
Copy link

👋 Hi,

Thanks for maintaining this Bamboo adapter. 🥳

There will be a breaking change coming to Bamboo in v2.0.0 where the deliver_now function will no longer raise an error on failure to deliver an email. Instead, it will return :ok or :error tuples. That way, users of the library can decide how to handle the errors. I've completed that work in beam-community/bamboo#571.

I've also added a deliver_now! function that is meant to behave exactly as deliver_now currently behaves. So, there's an easy upgrade path for users who don't want to handle their own errors. They can change deliver_now -> deliver_now!.

(There are also deliver_later corresponding changes, but from the adapter's perspective, I don't know that it matters).

What that means for adapters

In order for people to upgrade to Bamboo v2.0.0, the adapters will need to stop raising errors when they fail to deliver emails. In order to accommodate that, the adapter's callbacks are changing like this:

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

In other words, adapters should now return an {:ok, response} tuple or an {:error, error} tuple, where the error is either an exception struct that can later be raised or an error message.

To facilitate that work, PR 571 also introduces a Bamboo.ApiError.build_api_error function that can be used by adapters — if you want to easily build an error to return in the {:error, error} response.

Sample changes to other adapters

I have already done some changes to the Sendgrid, Mailgun, and Mandrill adapters. They might serve as a guide:

Why open this issue?

I wanted to open this issue for two reasons:

  1. To let you know about the changes that are coming up (all of the above), and
  2. To ask you if there's anything I can do to help with the transition.

Unfortunately, I can't offer to open PRs to make the changes in all the adapter repos, but if it's helpful, I'd be happy to review PRs.

And if there's some change to Bamboo that would make it easier to work with your adapter, or if you have suggestions or comments on PR 571, I'd love to know.

Thanks again for maintaining this adapter, and please feel free to close this issue whenever you've read it (if it's of no further use). It just seemed like the easiest way to communicate about the upcoming changes with you.

@Aduril
Copy link

Aduril commented Feb 22, 2021

As far as I see the the major change is towards the API of Bamboo.Mailer (please do not trust my word, check for yourself).

So the change should be an easy version upgrade, right?

@jarvisjohnson
Copy link
Contributor

I think @Aduril is right, bumping the bamboo version seems to be all that is needed.

Have created a PR for this - #177

@germsvel
Copy link
Author

Hi @Aduril and @jarvisjohnson, thanks so much for being quick to look through this. You're right that the main changes in Bamboo 2.0 are to the Bamboo.Mailer API. But in order for that change to work — for bamboo and bamboo_smtp users to be able to handle errors when emails fail to deliver — we also need to change what the adapter's deliver/2 function returns (See the changes to the adapter deliver callback):

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

+  @callback deliver(%Bamboo.Email{}, %{}) :: {:ok, any} | {:error, error}

That means adapters should no longer raise errors in the adapter code when failing to deliver emails. Instead they should return an {:error, error} tuple, so that Bamboo can handle that. Looking at the smtp_adapter.ex file, my guess is that these are the lines that should now return an error instead of raising one:

defp handle_response({:error, :no_credentials = reason}) do
raise SMTPError, {reason, "Username and password were not provided for authentication."}
end
defp handle_response({:error, reason, detail}) do
raise SMTPError, {reason, detail}
end
defp handle_response({:error, detail}) do
raise SMTPError, {:not_specified, detail}
end

If instead of raise SMTPError, bamboo_smtp returns {:error, %SMTPError{}}, then users who call Mailer.deliver_now will get that error tuple back. The users can then handle that in whatever way they see fit (e.g. retrying to send, or keeping a log of what failed, etc.)

For users who call Mailer.deliver_now!, Bamboo will raise the error the adapter passes back, so it'll do an raise %SMTPError{}.

As for the successful case, it seems that bamboo_smtp already returns an {:ok, response}, so that's perfect.

Does all that make sense?

@jarvisjohnson
Copy link
Contributor

Totally, thanks - I'll make some updates for it to return an :error tuple

@MatheusBueno782
Copy link
Contributor

Since the pull request #177 that address to this issue was merged I am closing it.

thanks all for the contribution 🙂.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants