]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-11753 EmailNotificationChannel ignore empty recipient requests
authorSébastien Lesaint <sebastien.lesaint@sonarsource.com>
Mon, 8 Apr 2019 13:28:20 +0000 (15:28 +0200)
committersonartech <sonartech@sonarsource.com>
Tue, 23 Apr 2019 08:37:56 +0000 (10:37 +0200)
server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/notification/ReportAnalysisFailureNotificationHandlerTest.java
server/sonar-server-common/src/main/java/org/sonar/server/notification/EmailNotificationHandler.java
server/sonar-server-common/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java
server/sonar-server-common/src/test/java/org/sonar/server/issue/notification/ChangesOnMyIssueNotificationHandlerTest.java
server/sonar-server-common/src/test/java/org/sonar/server/issue/notification/DoNotFixNotificationHandlerTest.java
server/sonar-server-common/src/test/java/org/sonar/server/issue/notification/MyNewIssuesNotificationHandlerTest.java
server/sonar-server-common/src/test/java/org/sonar/server/issue/notification/NewIssuesNotificationHandlerTest.java
server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/notification/QGChangeNotificationHandlerTest.java
server/sonar-server/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java
server/sonar-server/src/test/java/org/sonar/server/qualityprofile/BuiltInQPChangeNotificationHandlerTest.java

index b8dbdc8c1dfe270198ccc8f36dd8fcd3da8e8f90..89727bf2dcaaa188345c10f65ce577d6362db6a5 100644 (file)
@@ -176,7 +176,7 @@ public class ReportAnalysisFailureNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(REPORT_FAILURE_DISPATCHER_KEY, projectKey, REQUIRED_SUBSCRIBER_PERMISSIONS);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
@@ -214,7 +214,7 @@ public class ReportAnalysisFailureNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(REPORT_FAILURE_DISPATCHER_KEY, projectKey2, REQUIRED_SUBSCRIBER_PERMISSIONS);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
index bcf8b4dd8f865ad93d3c6bae5778203330c36c72..7e95e3da5442740fdff8c07baee09fe0ae5c3303 100644 (file)
@@ -42,7 +42,7 @@ public abstract class EmailNotificationHandler<T extends Notification> implement
     if (requests.isEmpty()) {
       return 0;
     }
-    return emailChannel.deliver(requests);
+    return emailChannel.deliverAll(requests);
   }
 
   protected abstract Set<EmailDeliveryRequest> toEmailDeliveryRequests(Collection<T> notifications);
index 896cca90e7897967f3c6d570956c4b1c5ce8c958..224164d68d038ababadc41b46e9a64951d9c42ee 100644 (file)
@@ -167,22 +167,23 @@ public class EmailNotificationChannel extends NotificationChannel {
     }
   }
 
-  public int deliver(Set<EmailDeliveryRequest> deliveries) {
-    if (!isActivated()) {
+  public int deliverAll(Set<EmailDeliveryRequest> deliveries) {
+    if (deliveries.isEmpty() || !isActivated()) {
       LOG.debug(SMTP_HOST_NOT_CONFIGURED_DEBUG_MSG);
       return 0;
     }
 
     return (int) deliveries.stream()
+      .filter(t -> !t.getRecipientEmail().trim().isEmpty())
       .map(t -> {
         EmailMessage emailMessage = format(t.getNotification());
         if (emailMessage != null) {
           emailMessage.setTo(t.getRecipientEmail());
           return deliver(emailMessage);
         }
-        return null;
+        return false;
       })
-      .filter(Objects::nonNull)
+      .filter(Boolean::booleanValue)
       .count();
   }
 
index 60e859a8f5f865f2a4f73242cffbfc03e21cbcd2..b6fae117865f647ad019522ec96f9164df398a54 100644 (file)
@@ -218,7 +218,7 @@ public class ChangesOnMyIssueNotificationHandlerTest {
       .map(t -> new EmailDeliveryRequest(emailOf(t.getAssignee()), t))
       .collect(toSet());
     int deliveredCount = new Random().nextInt(expectedRequests.size());
-    when(emailNotificationChannel.deliver(expectedRequests)).thenReturn(deliveredCount);
+    when(emailNotificationChannel.deliverAll(expectedRequests)).thenReturn(deliveredCount);
 
     int deliver = underTest.deliver(Stream.concat(assignee1Notifications.stream(), assignee2Notifications.stream()).collect(toSet()));
 
@@ -226,7 +226,7 @@ public class ChangesOnMyIssueNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(CHANGE_ON_MY_ISSUES_DISPATCHER_KEY, projectKey, ImmutableSet.of(assignee1, assignee2), ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
@@ -257,7 +257,7 @@ public class ChangesOnMyIssueNotificationHandlerTest {
       .map(t -> new EmailDeliveryRequest(emailOf(t.getAssignee()), t))
       .collect(toSet());
     int deliveredCount = new Random().nextInt(expectedRequests.size());
-    when(emailNotificationChannel.deliver(expectedRequests)).thenReturn(deliveredCount);
+    when(emailNotificationChannel.deliverAll(expectedRequests)).thenReturn(deliveredCount);
 
     Set<IssueChangeNotification> notifications = Stream.of(
       assignee1ChangeAuthor.stream(),
@@ -270,7 +270,7 @@ public class ChangesOnMyIssueNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(CHANGE_ON_MY_ISSUES_DISPATCHER_KEY, projectKey, assigneesChangeAuthor, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
index 9994f7d3d4c1db844c3b29424216f9f5f126344d..c7990f33d7bc41c4a39fecb9e51042a091455c4e 100644 (file)
@@ -242,7 +242,7 @@ public class DoNotFixNotificationHandlerTest {
       .flatMap(t -> t)
       .collect(toSet());
     int deliveredCount = new Random().nextInt(expectedRequests.size());
-    when(emailNotificationChannel.deliver(expectedRequests)).thenReturn(deliveredCount);
+    when(emailNotificationChannel.deliverAll(expectedRequests)).thenReturn(deliveredCount);
 
     Set<IssueChangeNotification> notifications = Stream.of(
       subscriber1Notifications.stream(),
@@ -256,7 +256,7 @@ public class DoNotFixNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(DO_NOT_FIX_ISSUE_CHANGE_DISPATCHER_KEY, projectKey, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
index 365d3e867f9c6e3188dc4f649916b888a2da194b..bfe5f9b8f38535fb6753b7d6f60e84bc23df7fd3 100644 (file)
@@ -198,7 +198,7 @@ public class MyNewIssuesNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(MY_NEW_ISSUES_DISPATCHER_KEY, projectKey, assignees, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
@@ -230,7 +230,7 @@ public class MyNewIssuesNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(MY_NEW_ISSUES_DISPATCHER_KEY, projectKey, assignees, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
@@ -275,7 +275,7 @@ public class MyNewIssuesNotificationHandlerTest {
       .map(t -> new EmailDeliveryRequest(emailOf(t.getAssignee()), t))
       .collect(toSet());
     int deliveredCount = new Random().nextInt(expectedRequests.size());
-    when(emailNotificationChannel.deliver(expectedRequests)).thenReturn(deliveredCount);
+    when(emailNotificationChannel.deliverAll(expectedRequests)).thenReturn(deliveredCount);
 
     int deliver = underTest.deliver(Stream.concat(assignee1Notifications.stream(), assignee2Notifications.stream()).collect(toSet()));
 
@@ -283,7 +283,7 @@ public class MyNewIssuesNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(MY_NEW_ISSUES_DISPATCHER_KEY, projectKey, assignees, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
@@ -318,7 +318,7 @@ public class MyNewIssuesNotificationHandlerTest {
       .map(t -> new EmailDeliveryRequest(emailOf(t.getAssignee()), t))
       .collect(toSet());
     int deliveredCount = new Random().nextInt(expectedRequests.size());
-    when(emailNotificationChannel.deliver(expectedRequests)).thenReturn(deliveredCount);
+    when(emailNotificationChannel.deliverAll(expectedRequests)).thenReturn(deliveredCount);
 
     Set<MyNewIssuesNotification> notifications = Stream.of(
       assignee1Project1.stream(), assignee1Project2.stream(),
@@ -334,7 +334,7 @@ public class MyNewIssuesNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(MY_NEW_ISSUES_DISPATCHER_KEY, projectKey3, of(assignee2, assignee3), ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
index 8898251494d8e68cd5d1edf8911e1f002f06f971..6f6f78cac95bd7d98a8db750cfec1033ab607e11 100644 (file)
@@ -174,7 +174,7 @@ public class NewIssuesNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(NEW_ISSUES_DISPATCHER_KEY, projectKey, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
@@ -212,7 +212,7 @@ public class NewIssuesNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(NEW_ISSUES_DISPATCHER_KEY, projectKey2, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
index e4e94c12a94d9c7a36051791837cc148c4fa365b..bf7046c8409790a64aa900c9233c0447945c8917 100644 (file)
@@ -172,7 +172,7 @@ public class QGChangeNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(QG_CHANGE_DISPATCHER_KEY, projectKey, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
@@ -210,7 +210,7 @@ public class QGChangeNotificationHandlerTest {
     verify(notificationManager).findSubscribedEmailRecipients(QG_CHANGE_DISPATCHER_KEY, projectKey2, ALL_MUST_HAVE_ROLE_USER);
     verifyNoMoreInteractions(notificationManager);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
   }
 
index d1cecb1d1bac4e60bb9bc122223aa04d6e1fd06d..deed583b73e4564afd3aff821d5ab590f4a2d484 100644 (file)
  */
 package org.sonar.server.notification.email;
 
+import com.tngtech.java.junit.dataprovider.DataProvider;
+import com.tngtech.java.junit.dataprovider.DataProviderRunner;
+import com.tngtech.java.junit.dataprovider.UseDataProvider;
+import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.stream.IntStream;
+import java.util.stream.Stream;
+import javax.mail.MessagingException;
 import javax.mail.internet.MimeMessage;
 import org.apache.commons.mail.EmailException;
 import org.junit.After;
@@ -27,19 +38,31 @@ import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
 import org.sonar.api.config.EmailSettings;
+import org.sonar.api.notifications.Notification;
 import org.sonar.server.issue.notification.EmailMessage;
+import org.sonar.server.issue.notification.EmailTemplate;
+import org.sonar.server.notification.email.EmailNotificationChannel.EmailDeliveryRequest;
 import org.subethamail.wiser.Wiser;
 import org.subethamail.wiser.WiserMessage;
 
+import static java.util.stream.Collectors.toMap;
+import static java.util.stream.Collectors.toSet;
 import static junit.framework.Assert.fail;
 import static org.apache.commons.lang.RandomStringUtils.random;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
 import static org.mockito.Mockito.when;
 
+@RunWith(DataProviderRunner.class)
 public class EmailNotificationChannelTest {
 
+  private static final String SUBJECT_PREFIX = "[SONARQUBE]";
+
   @Rule
   public ExpectedException thrown = ExpectedException.none();
 
@@ -212,12 +235,134 @@ public class EmailNotificationChannelTest {
     }
   }
 
+  @Test
+  public void deliverAll_has_no_effect_if_set_is_empty() {
+    EmailSettings emailSettings = mock(EmailSettings.class);
+    EmailNotificationChannel underTest = new EmailNotificationChannel(emailSettings, null, null);
+
+    int count = underTest.deliverAll(Collections.emptySet());
+
+    assertThat(count).isZero();
+    verifyZeroInteractions(emailSettings);
+    assertThat(smtpServer.getMessages()).isEmpty();
+  }
+
+  @Test
+  public void deliverAll_has_no_effect_if_smtp_host_is_null() {
+    EmailSettings emailSettings = mock(EmailSettings.class);
+    when(emailSettings.getSmtpHost()).thenReturn(null);
+    Set<EmailDeliveryRequest> requests = IntStream.range(0, 1 + new Random().nextInt(10))
+      .mapToObj(i -> new EmailDeliveryRequest("foo" + i + "@moo", mock(Notification.class)))
+      .collect(toSet());
+    EmailNotificationChannel underTest = new EmailNotificationChannel(emailSettings, null, null);
+
+    int count = underTest.deliverAll(requests);
+
+    assertThat(count).isZero();
+    verify(emailSettings).getSmtpHost();
+    verifyNoMoreInteractions(emailSettings);
+    assertThat(smtpServer.getMessages()).isEmpty();
+  }
+
+  @Test
+  @UseDataProvider("emptyStrings")
+  public void deliverAll_ignores_requests_which_recipient_is_empty(String emptyString) {
+    EmailSettings emailSettings = mock(EmailSettings.class);
+    when(emailSettings.getSmtpHost()).thenReturn(null);
+    Set<EmailDeliveryRequest> requests = IntStream.range(0, 1 + new Random().nextInt(10))
+      .mapToObj(i -> new EmailDeliveryRequest(emptyString, mock(Notification.class)))
+      .collect(toSet());
+    EmailNotificationChannel underTest = new EmailNotificationChannel(emailSettings, null, null);
+
+    int count = underTest.deliverAll(requests);
+
+    assertThat(count).isZero();
+    verify(emailSettings).getSmtpHost();
+    verifyNoMoreInteractions(emailSettings);
+    assertThat(smtpServer.getMessages()).isEmpty();
+  }
+
+  @Test
+  public void deliverAll_returns_count_of_request_for_which_at_least_one_formatter_accept_it() throws MessagingException, IOException {
+    String recipientEmail = "foo@donut";
+    configure();
+    Notification notification1 = mock(Notification.class);
+    Notification notification2 = mock(Notification.class);
+    Notification notification3 = mock(Notification.class);
+    EmailTemplate template1 = mock(EmailTemplate.class);
+    EmailTemplate template3 = mock(EmailTemplate.class);
+    EmailMessage emailMessage1 = new EmailMessage().setTo(recipientEmail).setSubject("sub11").setMessage("msg11");
+    EmailMessage emailMessage3 = new EmailMessage().setTo(recipientEmail).setSubject("sub3").setMessage("msg3");
+    when(template1.format(notification1)).thenReturn(emailMessage1);
+    when(template3.format(notification3)).thenReturn(emailMessage3);
+    Set<EmailDeliveryRequest> requests = Stream.of(notification1, notification2, notification3)
+      .map(t -> new EmailDeliveryRequest(recipientEmail, t))
+      .collect(toSet());
+    EmailNotificationChannel underTest = new EmailNotificationChannel(configuration, new EmailTemplate[] {template1, template3}, null);
+
+    int count = underTest.deliverAll(requests);
+
+    assertThat(count).isEqualTo(2);
+    assertThat(smtpServer.getMessages()).hasSize(2);
+    Map<String, MimeMessage> messagesBySubject = smtpServer.getMessages().stream()
+      .map(t -> {
+        try {
+          return t.getMimeMessage();
+        } catch (MessagingException e) {
+          throw new RuntimeException(e);
+        }
+      })
+      .collect(toMap(t -> {
+        try {
+          return t.getSubject();
+        } catch (MessagingException e) {
+          throw new RuntimeException(e);
+        }
+      }, t -> t));
+
+    assertThat((String) messagesBySubject.get(SUBJECT_PREFIX + " " + emailMessage1.getSubject()).getContent())
+      .contains(emailMessage1.getMessage());
+    assertThat((String) messagesBySubject.get(SUBJECT_PREFIX + " " + emailMessage3.getSubject()).getContent())
+      .contains(emailMessage3.getMessage());
+  }
+
+  @Test
+  public void deliverAll_ignores_multiple_templates_by_notification_and_takes_the_first_one_only() throws MessagingException, IOException {
+    String recipientEmail = "foo@donut";
+    configure();
+    Notification notification1 = mock(Notification.class);
+    EmailTemplate template11 = mock(EmailTemplate.class);
+    EmailTemplate template12 = mock(EmailTemplate.class);
+    EmailMessage emailMessage11 = new EmailMessage().setTo(recipientEmail).setSubject("sub11").setMessage("msg11");
+    EmailMessage emailMessage12 = new EmailMessage().setTo(recipientEmail).setSubject("sub12").setMessage("msg12");
+    when(template11.format(notification1)).thenReturn(emailMessage11);
+    when(template12.format(notification1)).thenReturn(emailMessage12);
+    EmailDeliveryRequest request = new EmailDeliveryRequest(recipientEmail, notification1);
+    EmailNotificationChannel underTest = new EmailNotificationChannel(configuration, new EmailTemplate[] {template11, template12}, null);
+
+    int count = underTest.deliverAll(Collections.singleton(request));
+
+    assertThat(count).isEqualTo(1);
+    assertThat(smtpServer.getMessages()).hasSize(1);
+    assertThat((String) smtpServer.getMessages().iterator().next().getMimeMessage().getContent())
+      .contains(emailMessage11.getMessage());
+  }
+
+  @DataProvider
+  public static Object[][] emptyStrings() {
+    return new Object[][] {
+      {""},
+      {"  "},
+      {" \n "}
+    };
+  }
+
   private void configure() {
     when(configuration.getSmtpHost()).thenReturn("localhost");
     when(configuration.getSmtpPort()).thenReturn(smtpServer.getServer().getPort());
     when(configuration.getFrom()).thenReturn("server@nowhere");
     when(configuration.getFromName()).thenReturn("SonarQube from NoWhere");
-    when(configuration.getPrefix()).thenReturn("[SONARQUBE]");
+    when(configuration.getPrefix()).thenReturn(SUBJECT_PREFIX);
     when(configuration.getServerBaseURL()).thenReturn("http://nemo.sonarsource.org");
   }
 
index 6145f79a9a904223ecbb82c1e1d6d9406fe09537..361f291edb4cbe34bc765a3d0aa1cb4ca8224702 100644 (file)
@@ -117,13 +117,13 @@ public class BuiltInQPChangeNotificationHandlerTest {
       .flatMap(notification -> emailSubscribers.stream().map(subscriber -> new EmailNotificationChannel.EmailDeliveryRequest(subscriber.getEmail(), notification)))
       .collect(toSet());
     int deliveries = new Random().nextInt(expectedRequests.size());
-    when(emailNotificationChannel.deliver(expectedRequests)).thenReturn(deliveries);
+    when(emailNotificationChannel.deliverAll(expectedRequests)).thenReturn(deliveries);
 
     int deliver = underTest.deliver(notifications);
 
     assertThat(deliver).isEqualTo(deliveries);
     verify(emailNotificationChannel).isActivated();
-    verify(emailNotificationChannel).deliver(expectedRequests);
+    verify(emailNotificationChannel).deliverAll(expectedRequests);
     verifyNoMoreInteractions(emailNotificationChannel);
     verify(dbClient).openSession(false);
     verify(dbClient).authorizationDao();