Catch all nil emails

[princemaple]

Fix #146

@paulcsmith

I was tempted to swap the order of validate and normailize_addresses, which would reduce a few lines of code, but then I would have to implement Bamboo.Formatter for nil, which doesn't really make sense.

[paulcsmith]

At first glance this looks good. I should have a bit more time to review it more thoroughly later today. Thanks for working on it!

[princemaple]

@paulcsmith the current validation logic is designed according to the existing error NilRecipientsError. I'd like to propose that before htting 1.0 we change it to NilRecipientError (notice singular) and raise on any nil recipient seen during the validation.

[princemaple]

I was tempted to swap the order of validate and normailize_addresses, which would reduce a few lines of code, but then I would have to implement Bamboo.Formatter for nil, which doesn't really make sense.

On a second thought, it would actually make more sense if we validate after normalizing the email addresses. Because that way, we get to validate everything, instead of letting unknown types go.

Re: nil, we could simply implement it with a raise, just like you did for map, regardless of the tiny bit of awkwardness around implementing the behavior for nil.

If you think it's a good idea, I'll update the PR.

[princemaple]

Ping

[paulcsmith]

Thanks for the ping. Yes I think validating after normalizing is a good idea. That way we can raise if email address is nil for stuff that implements the Bamboo.Formatter protocol

Please ping again when ready for re-review :)

[princemaple]

@paulcsmith so I have this wip commit right now, there is one test failing:
test raises if all receipients are nil (Bamboo.MailerTest) when the email has all to, cc and bcc as nil. Swapping the order of normalization and validation makes one big difference - List.wrap(...) is called beforehand and is turning nil's into []'s.

Hence, I think it's time to ask for clarification. Why did you raise for nil's but not for []'s? Could we simply treat empty lists as the same as nil's so we have a unified case?

[princemaple]

Could we simply treat empty lists as the same as nil's so we have a unified case?

meaning flipping this false to true :)

[paulcsmith]

@princemaple I'll have to take a closer look at the code later, but the reason [] does not raise is in this comment: #40 (comment)

Basically, it's that sometimes you send to a list of users and that list of users happens to be an empty list. I think this is a valid case and shouldn't raise, it just shouldn't send the email. The comment should explain the logic a bit better.

Does that make sense?

[princemaple]

@paulcsmith I've been busy. I probably don't have time to dig further into this refactor any time soon, so I've deleted the wip commit.

[princemaple]

I'm going to leave this PR as pure bug fix / edge case elimination.

[paulcsmith]

@princemaple This is great. Thanks so much and thanks for letting me know where you're at. I'll take a look at this and merge it in soon. Thanks for all your work on this!

[paulcsmith]

I'm going to try this out one some of our projects on Friday and if all works as expected I'll merge it in :D

[paulcsmith]

@princemaple I gave this a try and it seemed to work great! Thanks so much for tackling this :)