Quick thoughts on returning {:ok, response} | {:error, response}

[Ivor]

Hi :) It's been interesting reading through the evolution of the response: true feature.

I want to match on the response from deliver_now to determine whether the email was successfully sent or not. Looking at the tests in mailer_test I see that your stubbed response contains this %{status_code: status, headers: headers, body: response} which seems to be what the Mandrill and SendGrid adapters also return, but only if the status code is < 299

(could formalise that as a struct that the implementation of the adapter behaviour needs to return -> %Bamboo.Response{})

The other status codes for that adapter raise an exception, so the response: true option will only return a response when the request was successful. If the implementations of the adapters returned {:error, %{status_code: status, headers: headers, body: response} instead of raise_api_exception then it would be possible to match on that.

Renaming deliver_now to deliver_now! with the current behaviour of raising exceptions and then perhaps changing deliver_now to return {:ok, %Response{}} | {:error, %Response{}}

For the initial need to handle email delivery failures -> can we assume that if the email was not successfully delivered an exception will be raised? Is there somewhere where the expected range of exceptions is documented?

I don't know very much about emails and email delivery and I am probably not understanding al of the issues involved and apologise if my comments are short-sighted. I also suspect that there are perhaps changes planned for a future breaking version update that could include this.

If someone can perhaps document what the expected behaviour is for failures more explicitly it will already help a lot. Thanks for all the great work on this. Bamboo is absolutely great!

[joshchernoff]

Renaming deliver_now to deliver_now! with the current behaviour of raising exceptions and then
perhaps changing deliver_now to return {:ok, %Response{}} | {:error, %Response{}}

I +1 this. Beyond having deliver_now return a tuple I think any method whos default is to raise if not completed normally really should be named with a bang.

[germsvel]

Thanks for opening this issue @Ivor. I really like the idea of returning an ok/error tuple for deliver_now and adding a ! for the raising errors. As I'm thinking this, I wonder if it makes sense to use ok/error tuples in regular scenarios and let response: true still be about the response that comes from the adapter. So we could change the api to something like this:

# success deliver_now
{:ok, email} = deliver_now(email)
{:ok, email, response} = deliver_now(email, response: true)

# error deliver_now
{:error, email} = deliver_now(email)
{:error, email, response} = deliver_now(email, response: true)

# success deliver_now!
email = deliver_now!(email)
{email, response} = deliver_now!(email, response: true)

# error deliver_now!
deliver_now!(email) # raises
deliver_now!(email, response: true) #raises

What do you think?

Since it's a breaking change, we'd need to do it whenever we're ready to bump a major version for Bamboo. I just started maintaining Bamboo, so I'm not sure when that would be (I'd like to get a sense of all the breaking changes so we only bump major version once for them).

[Ivor]

Hi @germsvel

Your suggested change above looks great to me. Looking forward to seeing it in a future version.
Thanks!

[germsvel]

@Ivor I'm currently working on implementing a version of returning tuples. I'm curious to hear your thoughts on a particular scenario.

The strange scenario

Currently, when an email has to, cc, bcc set to empty lists (not nil but empty lists), we simply log that the email wasn't sent. This is in contrast to what we do if those three are nil: we raise an error.

The reasoning for that difference (that I gleam from #40) seems to be that you could somehow programmatically filter out all recipients so you end up with an empty list. In that case, we simply don't send the message. Setting a recipient as nil, however, likely indicates that you had an issue somewhere so we raise an error.

The question for us

So this is the question. How should we handle the case when to, cc, bcc are all empty lists now that we'll return ok/error tuples?

This would be the extension of the current behavior but with ok tuple as the return value:

# this would more closely match the existing behavior
new_email(to: [], cc: [], bcc: []) |> deliver_now() # {:ok, email} 

new_email(to: [], cc: [], bcc: []) |> deliver_now(response: true) # {:ok, email, :no_recipients} <- not even sure what the third element would be here. We currently just return the email. 

Or should we mark them as errors because now that we return error tuples, people can ignore them if they desire?

new_email(to: [], cc: [], bcc: []) |> deliver_now() # {:error, %Bamboo.EmptyRecipientsError{}}
new_email(to: [], cc: [], bcc: []) |> deliver_now(response: true) # {:error, %Bamboo.EmptyRecipientsError{}} 

At this point, I tend to like the latter approach. But when you consider introducing the ! version of the functions, then we have a strange breaking change for people who wanted to keep the previous behavior:

# before
new_email(to: [], cc: [], bcc: []) |> deliver_now() # no error

# you upgrade and decide to keep the raising errors version
new_email(to: [], cc: [], bcc: []) |> deliver_now!() # you get an error you didn't get before

What do you think?

[Ivor]

@germsvel I definitely like the latter approach as well. That is exactly why imho the tuple response is cool because you can handle these kinds of cases.

The bang methods do introduce a problem. I guess a config option to raise_on_empty: true could set whether the bang methods raise or not if there are no recipients? The default could be false to make it backwards compatible. That said, I agree that it would be weird if the bang method doesn't error but the no-bang version returns an error tuple.

I have to admit I don't see the obvious solution for that. The config is a way to get around it. Only alternative would be a breaking change.

[maartenvanvliet]

I think this issue can be closed with #571

[germsvel]

Very true. Thank you!