Conditionally include the adapter response in deliver_now

[maymillerricci]

Resolves #451 and #161.

The email adapters return data about the results from sending the email when .deliver is called, some of which can be valuable but is not currently being captured.

This introduces a change to merge that response in to the Email struct that is returned from .deliver_now if the response conforms to our norm of returning a status_code, headers, and body. (See the return for the success case from the adapters' deliver function for SendGrid, Mandrill, and Mailgun.) If the return value does not conform to that pattern, nothing additional happens, so for example the results from .deliver_now for the LocalAdapter and TestAdapter do not change.

In looking at the custom adapters, it looks like some of them conform to this pattern, so they would get the response included now, and others do not and some have separate custom handling for their responses, so there would be no change for them.

My goal was to include this data but to change as little as possible. I took inspiration from #308 and #450, but was opting not to change the type of what is returned from deliver or introduce a separate function for this.

[maymillerricci]

Upon reflection, I'm on the fence about this approach. It does solve the current issue without introducing any breaking changes which is great. But, it feels like response doesn't belong as part of Email which is meant to be all about creating/composing the email. But, I also don't love the other potential solutions, so I'm not sure what the best path forward with this is...

[Ch4s3]

I think it would make sense to go with this approach and make cleaner but breaking changes as part of a major version bump.

[lancejjohnson]

@maymillerricci I think the code looks good. It seems the conversation at this point, though, is largely one of strategies for handling this issue without introducing a breaking change. I'm of mixed opinions on the best way to go forward. On the one hand, I sympathize with your thoughts about adding a response field to the Email struct. On the other hand, I'm curious if there are parallels here to the way that Plug handles a Plug.Conn. The Plug.Conn is everything the system knows about the incoming request and the outgoing response. Is there a parallel here? Would that be a good pattern to emulate in this case? I hesitate only because Plug.Conn exists within the boundaries of the Plug system whereas the response in our case means something has left the system and come back.

Adding to the struct seems like a non-breaking change, I'm curious if it would better to add functions that return some data structure that has the email and the response as separate entities (e.g. {email, response}). Perhaps something like,

def deliver_now(email, response: true) do
  # ... snip ...
end

def deliver_later(email, response: true) do
  # ... snip ...
end

I tend to agree with comments in another issue that it's surprising the return value from our deliver functions is not in the pattern {:ok, thing} | {:error, thing}. Would adding functions move us closer to that direction rather than adding to the email struct?

Just thinking aloud here. Any thoughts on all that?

[maymillerricci]

@lancejjohnson Interesting! Thanks for your thoughts on this. I was initially resistant to the idea of creating a separate function for this, but that was for a function with a totally different name, and I hadn't considered the idea of adding something like a response: true option to the existing function. That might be a really good solution, at least for the short term.

Ultimately doing a major version bump with breaking changes might make sense, and following the {:ok, thing} | {:error, thing} pattern seems like a natural fit.

But for now, I like your idea, and it does feel better than merging the response onto the Email where it doesn't totally belong. The Plug.Conn parallel is interesting, but like you said in our case it feels different since something has left the system and come back, and also one of the explicit goals here seems to be for email creation and email delivery to be totally separate, so as much as I was wanting this to be the solution, it doesn't seem quite right.

I will take a pass at implementing your suggestion of having a separate function with a response option, and we can see what we think of it. Thanks! 🙏

[maymillerricci]

@lancejjohnson See #466 for my pass at your suggestion! Is something like that what you had in mind?

[maymillerricci]

Closing this in favor of #466.