Revise return types of EmailService methods

[justinedelson]

Based on @davidjgonzalez 's comments in #169, we need to revise the API methods in EmailService. Currently, the return value is as follows:

• if no addresses are provided - false
• if one or more addresses are provided and at least one email is successful - true
• if one or more addresses are provided and all sends fail - false

It sounds like the following would make more sense:

• if no address are provided - throw IllegalArgumentException
• if one or more addes are provided and all are successful - return empty list
• if one or more addes are provided and not are successful - return list containing failed addresses.

Return types should match the third argument, i.e. List<InternetAddress> vs. List<String>

Requires documentation update as well.

@upasanac / @davidjgonzalez - please let me know your thoughts.

[davidjgonzalez]

@justinedelson/@upsanac - you made an interesting comment in #169 - it might just be simpler and more straightforward, to have the API take a single recipient and require the caller loop over their list of recipients; true for success, false for failure; and its up to the caller to handle accordingly.

Returning a list of failures seems a bit awkward..?

WDYT?

[justinedelson]

I think the idea of having multiple addresses is that it allows the lookup of the template from the repository and the variable substitution to only happen once.

To me, the original behavior made sense :), I'm used to two different use cases:

1. Fire and forget - I have a notification to send to a bunch of users (say, when a new article is published). It'd be swell if they all got it, but if not, I don't really care and even if I did, there isn't a lot I can do about it.
2. Personal notification - I have a notification to send to a single user and I do actually care if the send succeeded or not.

[upasanac]

I think that the APIs extended capability(i.e, a list of recipients vs a single recipient) is for such stated use-cases. Also, it is conventionally more meaningful to send a single email to all the recipients 'at ones' if that was the intention. But the existing API response could be deceiving for 99 failures vs 1 success.

[davidjgonzalez]

Returning boolean for multiple just seems disingenuous; if, for multiple, we don't care if it any succeed or fail, then void makes more sense; and reserve boolean for a single recipient method. I

I'm not sure anyone would use splat syntax for this IRL - and if they do, new String[] { "up@acs.com", "je@acs.com" } isnt terrible to type out.

Returning a failure list isn't bad either; since callers can always ignore it during fire and forget scenarios - just a little odd.

[upasanac]

@davidjgonzalez @justinedelson One of the issues while testing this API I faced was to identify- if any of the email sent was failed- I could only count the invocations made on email.send() but nothing did guarantee a success. Returning a failure list is an answer to this although I agree with David that the meaning of this response return-type is not very implicit :) But if you think, we have reached to a consensus on this issue, I shall work on the suggested code update and will also update Junits and documentation.

[davidjgonzalez]

@upasanac TBH I havent looked very closely at the APIs youre using; what could cause a failure in the sending of an email? I guess, what does a "failure" really mean in the content of our return types?

That the the SMTP server could not be reached? That the SMTP server refused the send request? That the email could not be delivered? etc.. the MailException's description is pretty vague

[upasanac]

The EmailServiceImpl.send() class will return false in following cases :

1) Not able to get the messageGateway object (messageGateway not configured or is null). 2) No recipients(As per Justin's comment's, need to change this to throw IllegalArgumentException rather than returning false) 3) Email template is not configured and hence HtmlEmail object is null. 4) Due to some reason the email sending failed(SMTP server refused to send email, or recipients email address was incorrect), etc

[upasanac]

@justinedelson @davidjgonzalez Can you also please suggest on the below cases? 1) When messageGateway is NULL -> throw an Exception ? 2) When mailTemplate path is null/empty -> throw an IllegalArgumentException ? 3) If mailTemplate = Null (i.e, No template found in the path specified), throw Exception ? 4) If each messageGateway.send(email) invocation fails due to MailingException, the failiure list contains list of all recipients ? Also, kindly let me know if I have missed any case.

[davidjgonzalez]

Here's my thoughts ..

1) When messageGateway is NULL

• Just let it naturally NPE; this is how we handle all (almost?) every other case where an AEM Service does not exist.

2) When mailTemplate path is null/empty -> throw an IllegalArgumentException ?

• Agree

3) If mailTemplate = Null (i.e, No template found in the path specified), throw Exception ?

• IMO same as #2, throw an IllegalArgumentException; 2 and 3 are the same in that the argument doesn't point to a template; which is what we care about.

4) If each messageGateway.send(email) invocation fails due to MailingException, the failiure list contains list of all recipients ?

• Agree. As @justinedelson mentioned above, the return type should match the arg type.

[justinedelson]

@upasanac / @davidjgonzalez - this issue can be closed now, correct?

[davidjgonzalez]

@justinedelson - unless @upasanac has any reservations; looks good to me. Documentation is in the acs-aem-projects/acs-aem-commons-1.6 branch;

Closing; @upasanac re-open as needed.

[upasanac]

@justinedelson @davidjgonzalez I am happy to have it closed. Thank you :)