Repository: adobe-consulting-services/acs-aem-commons
Issue: Issue with email service

1. If we have more than one recipients. The second recipient is getting one attachment which is the mail template. Third recipient gets two attachments etc. This is the behaviour in outlook.
2. When we are sending mail to gmail or other. Second recipient getting two mails. Third receipt getting three mails etc.


Changed files:
bundle/src/test/java/com/adobe/acs/commons/email/EmailServiceImplTest.java, bundle/src/main/java/com/adobe/acs/commons/email/impl/EmailServiceImpl.java

diff --git a/bundle/src/main/java/com/adobe/acs/commons/email/impl/EmailServiceImpl.java b/bundle/src/main/java/com/adobe/acs/commons/email/impl/EmailServiceImpl.java
index 0c7a5c4cb..5dfea54d2 100644
--- a/bundle/src/main/java/com/adobe/acs/commons/email/impl/EmailServiceImpl.java
+++ b/bundle/src/main/java/com/adobe/acs/commons/email/impl/EmailServiceImpl.java
@@ -19,33 +19,34 @@
 */
 package com.adobe.acs.commons.email.impl;
 
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
-import java.util.Map;
-
-import javax.jcr.Session;
-import javax.mail.internet.AddressException;
-import javax.mail.internet.InternetAddress;
-
-import org.apache.commons.lang.StringUtils;
+import com.adobe.acs.commons.email.EmailService;
+import com.adobe.acs.commons.email.EmailServiceConstants;
+import com.day.cq.commons.mail.MailTemplate;
+import com.day.cq.mailer.MessageGateway;
+import com.day.cq.mailer.MessageGatewayService;
 import org.apache.commons.lang.text.StrLookup;
 import org.apache.commons.mail.Email;
+import org.apache.commons.mail.EmailException;
 import org.apache.commons.mail.HtmlEmail;
 import org.apache.commons.mail.SimpleEmail;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Reference;
 import org.apache.felix.scr.annotations.Service;
+import org.apache.sling.api.resource.LoginException;
 import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.api.resource.ResourceResolverFactory;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.adobe.acs.commons.email.EmailService;
-import com.adobe.acs.commons.email.EmailServiceConstants;
-import com.day.cq.commons.mail.MailTemplate;
-import com.day.cq.mailer.MessageGateway;
-import com.day.cq.mailer.MessageGatewayService;
+import javax.jcr.Session;
+import javax.mail.MessagingException;
+import javax.mail.internet.AddressException;
+import javax.mail.internet.InternetAddress;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
 
 /**
  * ACS AEM Commons - E-mail Service
@@ -64,9 +65,12 @@ public final class EmailServiceImpl implements EmailService {
     private ResourceResolverFactory resourceResolverFactory;
 
     @Override
-    public List<String> sendEmail(final String templatePath, final Map<String, String> emailParams,
-        final String... recipients) {
+    public List<String> sendEmail(final String templatePath,
+                                  final Map<String, String> emailParams,
+                                  final String... recipients) {
+
         List<String> failureList = new ArrayList<String>();
+
         if (recipients == null || recipients.length <= 0) {
             throw new IllegalArgumentException("Invalid Recipients");
         }
@@ -80,7 +84,7 @@ public final class EmailServiceImpl implements EmailService {
             }
         }
         InternetAddress[] iAddressRecipients = addresses.toArray(new InternetAddress[addresses.size()]);
-        List<InternetAddress> failureInternetAddresses =  sendEmail(templatePath, emailParams, iAddressRecipients);
+        List<InternetAddress> failureInternetAddresses = sendEmail(templatePath, emailParams, iAddressRecipients);
 
         for (InternetAddress address : failureInternetAddresses) {
             failureList.add(address.toString());
@@ -92,7 +96,7 @@ public final class EmailServiceImpl implements EmailService {
 
     @Override
     public List<InternetAddress> sendEmail(final String templatePath, final Map<String, String> emailParams,
-            final InternetAddress... recipients) {
+                                           final InternetAddress... recipients) {
 
         List<InternetAddress> failureList = new ArrayList<InternetAddress>();
 
@@ -100,65 +104,69 @@ public final class EmailServiceImpl implements EmailService {
             throw new IllegalArgumentException("Invalid Recipients");
         }
 
-        if (StringUtils.isBlank(templatePath)) {
-            throw new IllegalArgumentException("Template path is null or empty");
-        }
-        Email email = getEmail(templatePath, emailParams);
+        final MailTemplate mailTemplate = this.getMailTemplate(templatePath);
+        final Class<? extends Email> mailType = this.getMailType(templatePath);
+        final MessageGateway<Email> messageGateway = messageGatewayService.getGateway(mailType);
 
-        if (email == null) {
-            throw new IllegalArgumentException("Error while creating template");
-        }
-
-        MessageGateway<Email> messageGateway = messageGatewayService.getGateway(email.getClass());
-
-        for (InternetAddress address : recipients) {
+        for (final InternetAddress address : recipients) {
             try {
+                // Get a new email per recipient to avoid duplicate attachments
+                final Email email = getEmail(mailTemplate, mailType, emailParams);
                 email.setTo(Collections.singleton(address));
                 messageGateway.send(email);
             } catch (Exception e) {
                 failureList.add(address);
-                log.error("Exception sending email to " + address, e);
+                log.error("Error sending email to [ " + address + " ]", e);
             }
-         }
+        }
 
         return failureList;
     }
 
-    private Email getEmail(String templatePath, Map<String, String> emailParams) {
-        ResourceResolver resourceResolver = null;
-        try {
-            resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(null);
+    private Email getEmail(final MailTemplate mailTemplate,
+                           final Class<? extends Email> mailType,
+                           final Map<String, String> params) throws EmailException, MessagingException, IOException {
 
-           final MailTemplate mailTemplate = MailTemplate.create(templatePath, resourceResolver.adaptTo(Session.class));
+        final Email email = mailTemplate.getEmail(StrLookup.mapLookup(params), mailType);
 
-           if (mailTemplate == null) {
-               log.warn("Email template at {} could not be created.", templatePath);
-               return null;
-           }
+        if (params.containsKey(EmailServiceConstants.SENDER_EMAIL_ADDRESS)
+                && params.containsKey(EmailServiceConstants.SENDER_NAME)) {
 
-           Class<? extends Email> emailClass = templatePath.endsWith(".html") ? HtmlEmail.class : SimpleEmail.class;
+            email.setFrom(
+                    params.get(EmailServiceConstants.SENDER_EMAIL_ADDRESS),
+                    params.get(EmailServiceConstants.SENDER_NAME));
 
-           final Email email = mailTemplate.getEmail(StrLookup.mapLookup(emailParams), emailClass);
+        } else if (params.containsKey(EmailServiceConstants.SENDER_EMAIL_ADDRESS)) {
+            email.setFrom(params.get(EmailServiceConstants.SENDER_EMAIL_ADDRESS));
+        }
 
-           if (emailParams.containsKey(EmailServiceConstants.SENDER_EMAIL_ADDRESS)
-                    && emailParams.containsKey(EmailServiceConstants.SENDER_NAME)) {
-                email.setFrom(emailParams.get(EmailServiceConstants.SENDER_EMAIL_ADDRESS),
-                        emailParams.get(EmailServiceConstants.SENDER_NAME));
-           } else if (emailParams.containsKey(EmailServiceConstants.SENDER_EMAIL_ADDRESS)) {
-                email.setFrom(emailParams.get(EmailServiceConstants.SENDER_EMAIL_ADDRESS));
-           }
+        return email;
+    }
 
-           return email;
+    private Class<? extends Email> getMailType(String templatePath) {
+        return templatePath.endsWith(".html") ? HtmlEmail.class : SimpleEmail.class;
+    }
 
-        } catch (Exception e) {
-            log.error("Unable to construct email from template " + templatePath, e);
+    private MailTemplate getMailTemplate(String templatePath) throws IllegalArgumentException {
+        MailTemplate mailTemplate = null;
+        ResourceResolver resourceResolver = null;
+        try {
+            resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(null);
+            mailTemplate = MailTemplate.create(templatePath, resourceResolver.adaptTo(Session.class));
+
+            if (mailTemplate == null) {
+                throw new IllegalArgumentException("Mail template path [ "
+                        + templatePath + " ] could not resolve to a valid template");
+            }
+        } catch (LoginException e) {
+            log.error("Unable to obtain an administrative resource resolver to get the Mail Template at [ "
+                            + templatePath + " ]", e);
         } finally {
             if (resourceResolver != null) {
                 resourceResolver.close();
             }
         }
 
-        return null;
+        return mailTemplate;
     }
-
 }
diff --git a/bundle/src/test/java/com/adobe/acs/commons/email/EmailServiceImplTest.java b/bundle/src/test/java/com/adobe/acs/commons/email/EmailServiceImplTest.java
index 2ce06ba75..904c390c7 100644
--- a/bundle/src/test/java/com/adobe/acs/commons/email/EmailServiceImplTest.java
+++ b/bundle/src/test/java/com/adobe/acs/commons/email/EmailServiceImplTest.java
@@ -19,19 +19,10 @@
  */
 package com.adobe.acs.commons.email;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
-
-import java.io.FileInputStream;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
-import javax.jcr.Session;
-
+import com.adobe.acs.commons.email.impl.EmailServiceImpl;
+import com.day.cq.commons.mail.MailTemplate;
+import com.day.cq.mailer.MessageGateway;
+import com.day.cq.mailer.MessageGatewayService;
 import org.apache.commons.mail.SimpleEmail;
 import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.api.resource.ResourceResolverFactory;
@@ -50,10 +41,17 @@ import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
-import com.adobe.acs.commons.email.impl.EmailServiceImpl;
-import com.day.cq.commons.mail.MailTemplate;
-import com.day.cq.mailer.MessageGateway;
-import com.day.cq.mailer.MessageGatewayService;
+import javax.jcr.Session;
+import java.io.FileInputStream;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 @RunWith(PowerMockRunner.class)
 @PrepareForTest(MailTemplate.class)
@@ -185,7 +183,7 @@ public class EmailServiceImplTest {
         final Map<String, String> params = new HashMap<String, String>();
         final String recipient =  "upasanac@acs.com";
         thrown.expect(IllegalArgumentException.class);
-        thrown.expectMessage("Template path is null or empty");
+        thrown.expectMessage("Mail template path [ null ] could not resolve to a valid template");
 
         emailService.sendEmail(templatePath, params, recipient);
      }
@@ -196,7 +194,7 @@ public class EmailServiceImplTest {
         final Map<String, String> params = new HashMap<String, String>();
         final String recipient =  "upasanac@acs.com";
         thrown.expect(IllegalArgumentException.class);
-        thrown.expectMessage("Error while creating template");
+        thrown.expectMessage("Mail template path [ /invalidTemplatePath.txt ] could not resolve to a valid template");
 
         emailService.sendEmail(templatePath, params, recipient);
      }