SONAR-4540 Notifications should not be sent on changes of issue line, author or effort to fix

7 files changed, 95 insertions, 27 deletions

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java
index 73d41db228a..0a3658a34a0 100644
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java
@@ -58,7 +58,7 @@ public class SendIssueNotificationsPostJob implements PostJob {
       if (issue.isNew() && issue.resolution() == null) {
         newIssues++;
       }
-      if (!issue.isNew() && issue.isChanged()) {
+      if (!issue.isNew() && issue.isChanged() && issue.mustSendNotifications()) {
         Rule rule = ruleFinder.findByKey(issue.ruleKey());
         // TODO warning - rules with status REMOVED are currently ignored, but should not
         if (rule != null) {
diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java
index fc61ced163a..736049d7ae8 100644
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java
@@ -84,13 +84,14 @@ public class SendIssueNotificationsPostJobTest {
   }
 
   @Test
-  public void should_send_notification_if_issue_change() throws Exception {
+  public void should_send_notification() throws Exception {
     when(project.getAnalysisDate()).thenReturn(DateUtils.parseDate("2013-05-18"));
     RuleKey ruleKey = RuleKey.of("squid", "AvoidCycles");
     Rule rule = new Rule("squid", "AvoidCycles");
     DefaultIssue issue = new DefaultIssue()
       .setNew(false)
       .setChanged(true)
+      .setSendNotifications(true)
       .setFieldChange(mock(IssueChangeContext.class), "severity", "MINOR", "BLOCKER")
       .setRuleKey(ruleKey);
     when(issueCache.all()).thenReturn(Arrays.asList(issue));
@@ -120,4 +121,24 @@ public class SendIssueNotificationsPostJobTest {
 
     verify(notifications, never()).sendChanges(eq(issue), eq(changeContext), any(Rule.class), any(Component.class), any(Component.class));
   }
+
+  @Test
+  public void should_not_send_notification_on_any_change() throws Exception {
+    IssueChangeContext changeContext = mock(IssueChangeContext.class);
+
+    when(project.getAnalysisDate()).thenReturn(DateUtils.parseDate("2013-05-18"));
+    RuleKey ruleKey = RuleKey.of("squid", "AvoidCycles");
+    DefaultIssue issue = new DefaultIssue()
+      .setChanged(true)
+      .setSendNotifications(false)
+      .setFieldChange(changeContext, "severity", "MINOR", "BLOCKER")
+      .setRuleKey(ruleKey);
+    when(issueCache.all()).thenReturn(Arrays.asList(issue));
+    when(ruleFinder.findByKey(ruleKey)).thenReturn(null);
+
+    SendIssueNotificationsPostJob job = new SendIssueNotificationsPostJob(issueCache, notifications, ruleFinder);
+    job.executeOn(project, sensorContext);
+
+    verify(notifications, never()).sendChanges(eq(issue), eq(changeContext), any(Rule.class), any(Component.class), any(Component.class));
+  }
 }
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java b/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java
index 93d67e5c62d..1a96d48446d 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java
@@ -88,31 +88,31 @@ public class IssueNotifications implements BatchComponent, ServerComponent {
   @CheckForNull
   private Notification createChangeNotification(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project,
                                                 @Nullable Component component, @Nullable String comment) {
-    FieldDiffs currentChange = issue.currentChange();
-    if (comment == null && (currentChange == null || currentChange.diffs().isEmpty())) {
-      return null;
-    }
-    Notification notification = newNotification(project, "issue-changes");
-    notification.setFieldValue("key", issue.key());
-    notification.setFieldValue("changeAuthor", context.login());
-    notification.setFieldValue("reporter", issue.reporter());
-    notification.setFieldValue("assignee", issue.assignee());
-    notification.setFieldValue("message", issue.message());
-    notification.setFieldValue("ruleName", ruleName(rule));
-    notification.setFieldValue("componentKey", issue.componentKey());
-    if (component != null) {
-      notification.setFieldValue("componentName", component.longName());
-    }
-    if (comment != null) {
-      notification.setFieldValue("comment", comment);
-    }
+    Notification notification = null;
+    if (comment != null || issue.mustSendNotifications()) {
+      FieldDiffs currentChange = issue.currentChange();
+      notification = newNotification(project, "issue-changes");
+      notification.setFieldValue("key", issue.key());
+      notification.setFieldValue("changeAuthor", context.login());
+      notification.setFieldValue("reporter", issue.reporter());
+      notification.setFieldValue("assignee", issue.assignee());
+      notification.setFieldValue("message", issue.message());
+      notification.setFieldValue("ruleName", ruleName(rule));
+      notification.setFieldValue("componentKey", issue.componentKey());
+      if (component != null) {
+        notification.setFieldValue("componentName", component.longName());
+      }
+      if (comment != null) {
+        notification.setFieldValue("comment", comment);
+      }
 
-    if (currentChange != null) {
-      for (Map.Entry<String, FieldDiffs.Diff> entry : currentChange.diffs().entrySet()) {
-        String type = entry.getKey();
-        FieldDiffs.Diff diff = entry.getValue();
-        notification.setFieldValue("old." + type, diff.oldValue() != null ? diff.oldValue().toString() : null);
-        notification.setFieldValue("new." + type, diff.newValue() != null ? diff.newValue().toString() : null);
+      if (currentChange != null) {
+        for (Map.Entry<String, FieldDiffs.Diff> entry : currentChange.diffs().entrySet()) {
+          String type = entry.getKey();
+          FieldDiffs.Diff diff = entry.getValue();
+          notification.setFieldValue("old." + type, diff.oldValue() != null ? diff.oldValue().toString() : null);
+          notification.setFieldValue("new." + type, diff.newValue() != null ? diff.newValue().toString() : null);
+        }
       }
     }
     return notification;
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java b/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java
index a91d401e0df..3c5a80c53a0 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java
@@ -63,6 +63,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent {
       issue.setManualSeverity(true);
       issue.setUpdateDate(context.date());
       issue.setChanged(true);
+      issue.setSendNotifications(true);
       return true;
     }
     return false;
@@ -75,6 +76,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent {
       issue.setAssignee(sanitizedAssignee);
       issue.setUpdateDate(context.date());
       issue.setChanged(true);
+      issue.setSendNotifications(true);
       return true;
     }
     return false;
@@ -101,6 +103,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent {
       issue.setResolution(resolution);
       issue.setUpdateDate(context.date());
       issue.setChanged(true);
+      issue.setSendNotifications(true);
       return true;
     }
     return false;
@@ -112,6 +115,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent {
       issue.setStatus(status);
       issue.setUpdateDate(context.date());
       issue.setChanged(true);
+      issue.setSendNotifications(true);
       return true;
     }
     return false;
@@ -123,6 +127,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent {
       issue.setAuthorLogin(authorLogin);
       issue.setUpdateDate(context.date());
       issue.setChanged(true);
+      // do not send notifications to prevent spam when installing the developer cockpit plugin
       return true;
     }
     return false;
@@ -163,6 +168,8 @@ public class IssueUpdater implements BatchComponent, ServerComponent {
       issue.setEffortToFix(d);
       issue.setUpdateDate(context.date());
       issue.setChanged(true);
+      // Do not send notifications to prevent spam when installing the SQALE plugin,
+      // and do not complete the changelog (for the moment)
       return true;
     }
     return false;
@@ -193,6 +200,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent {
       issue.setActionPlanKey(sanitizedActionPlanKey);
       issue.setUpdateDate(context.date());
       issue.setChanged(true);
+      issue.setSendNotifications(true);
       return true;
     }
     return false;
diff --git a/sonar-core/src/test/java/org/sonar/core/issue/IssueNotificationsTest.java b/sonar-core/src/test/java/org/sonar/core/issue/IssueNotificationsTest.java
index 9f85faa3be9..f0b464fb987 100644
--- a/sonar-core/src/test/java/org/sonar/core/issue/IssueNotificationsTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/issue/IssueNotificationsTest.java
@@ -76,6 +76,7 @@ public class IssueNotificationsTest {
       .setAssignee("freddy")
       .setFieldChange(context, "resolution", null, "FIXED")
       .setFieldChange(context, "status", "OPEN", "RESOLVED")
+      .setSendNotifications(true)
       .setComponentKey("struts:Action")
       .setProjectKey("struts");
     DefaultIssueQueryResult queryResult = new DefaultIssueQueryResult(Arrays.<Issue>asList(issue));
diff --git a/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java b/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java
index ade4e76cbd2..63f7ac1483e 100644
--- a/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java
@@ -39,6 +39,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.assign(issue, "emmerik", context);
     assertThat(updated).isTrue();
     assertThat(issue.assignee()).isEqualTo("emmerik");
+    assertThat(issue.mustSendNotifications()).isTrue();
     FieldDiffs.Diff diff = issue.currentChange().get("assignee");
     assertThat(diff.oldValue()).isNull();
     assertThat(diff.newValue()).isEqualTo("emmerik");
@@ -50,6 +51,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.assign(issue, null, context);
     assertThat(updated).isTrue();
     assertThat(issue.assignee()).isNull();
+    assertThat(issue.mustSendNotifications()).isTrue();
     FieldDiffs.Diff diff = issue.currentChange().get("assignee");
     assertThat(diff.oldValue()).isEqualTo("morgan");
     assertThat(diff.newValue()).isNull();
@@ -61,6 +63,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.assign(issue, "emmerik", context);
     assertThat(updated).isTrue();
     assertThat(issue.assignee()).isEqualTo("emmerik");
+    assertThat(issue.mustSendNotifications()).isTrue();
     FieldDiffs.Diff diff = issue.currentChange().get("assignee");
     assertThat(diff.oldValue()).isEqualTo("morgan");
     assertThat(diff.newValue()).isEqualTo("emmerik");
@@ -72,6 +75,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.assign(issue, "morgan", context);
     assertThat(updated).isFalse();
     assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
 
@@ -81,6 +85,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isTrue();
     assertThat(issue.severity()).isEqualTo("BLOCKER");
     assertThat(issue.manualSeverity()).isFalse();
+    assertThat(issue.mustSendNotifications()).isFalse();
 
     FieldDiffs.Diff diff = issue.currentChange().get("severity");
     assertThat(diff.oldValue()).isNull();
@@ -93,6 +98,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.setPastSeverity(issue, "INFO", context);
     assertThat(updated).isTrue();
     assertThat(issue.severity()).isEqualTo("BLOCKER");
+    assertThat(issue.mustSendNotifications()).isFalse();
 
     FieldDiffs.Diff diff = issue.currentChange().get("severity");
     assertThat(diff.oldValue()).isEqualTo("INFO");
@@ -106,6 +112,7 @@ public class IssueUpdaterTest {
 
     assertThat(updated).isTrue();
     assertThat(issue.severity()).isEqualTo("MINOR");
+    assertThat(issue.mustSendNotifications()).isFalse();
     FieldDiffs.Diff diff = issue.currentChange().get("severity");
     assertThat(diff.oldValue()).isEqualTo("BLOCKER");
     assertThat(diff.newValue()).isEqualTo("MINOR");
@@ -116,6 +123,7 @@ public class IssueUpdaterTest {
     issue.setSeverity("MINOR");
     boolean updated = updater.setSeverity(issue, "MINOR", context);
     assertThat(updated).isFalse();
+    assertThat(issue.mustSendNotifications()).isFalse();
     assertThat(issue.currentChange()).isNull();
   }
 
@@ -137,6 +145,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isTrue();
     assertThat(issue.severity()).isEqualTo("MINOR");
     assertThat(issue.manualSeverity()).isTrue();
+    assertThat(issue.mustSendNotifications()).isTrue();
     FieldDiffs.Diff diff = issue.currentChange().get("severity");
     assertThat(diff.oldValue()).isEqualTo("BLOCKER");
     assertThat(diff.newValue()).isEqualTo("MINOR");
@@ -148,6 +157,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.setManualSeverity(issue, "MINOR", context);
     assertThat(updated).isFalse();
     assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -155,7 +165,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.setLine(issue, 123);
     assertThat(updated).isTrue();
     assertThat(issue.line()).isEqualTo(123);
-
+    assertThat(issue.mustSendNotifications()).isFalse();
     // do not save change
     assertThat(issue.currentChange()).isNull();
   }
@@ -166,6 +176,7 @@ public class IssueUpdaterTest {
     boolean updated = updater.setPastLine(issue, 123);
     assertThat(updated).isTrue();
     assertThat(issue.line()).isEqualTo(42);
+    assertThat(issue.mustSendNotifications()).isFalse();
 
     // do not save change
     assertThat(issue.currentChange()).isNull();
@@ -178,6 +189,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isFalse();
     assertThat(issue.line()).isEqualTo(123);
     assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -189,6 +201,7 @@ public class IssueUpdaterTest {
     FieldDiffs.Diff diff = issue.currentChange().get("resolution");
     assertThat(diff.oldValue()).isNull();
     assertThat(diff.newValue()).isEqualTo("OPEN");
+    assertThat(issue.mustSendNotifications()).isTrue();
   }
 
   @Test
@@ -198,6 +211,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isFalse();
     assertThat(issue.resolution()).isEqualTo("FIXED");
     assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -209,6 +223,7 @@ public class IssueUpdaterTest {
     FieldDiffs.Diff diff = issue.currentChange().get("status");
     assertThat(diff.oldValue()).isNull();
     assertThat(diff.newValue()).isEqualTo("OPEN");
+    assertThat(issue.mustSendNotifications()).isTrue();
   }
 
   @Test
@@ -218,6 +233,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isFalse();
     assertThat(issue.status()).isEqualTo("CLOSED");
     assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -228,6 +244,7 @@ public class IssueUpdaterTest {
     assertThat(issue.currentChange().diffs()).hasSize(1);
     assertThat(issue.currentChange().get("JIRA").oldValue()).isNull();
     assertThat(issue.currentChange().get("JIRA").newValue()).isEqualTo("FOO-123");
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -239,6 +256,7 @@ public class IssueUpdaterTest {
     assertThat(issue.currentChange().diffs()).hasSize(1);
     assertThat(issue.currentChange().get("JIRA").oldValue()).isEqualTo("FOO-123");
     assertThat(issue.currentChange().get("JIRA").newValue()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -246,6 +264,7 @@ public class IssueUpdaterTest {
     issue.setAttribute("JIRA", "FOO-123");
     boolean updated = updater.setAttribute(issue, "JIRA", "FOO-123", context);
     assertThat(updated).isFalse();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -257,6 +276,7 @@ public class IssueUpdaterTest {
     FieldDiffs.Diff diff = issue.currentChange().get("actionPlanKey");
     assertThat(diff.oldValue()).isNull();
     assertThat(diff.newValue()).isEqualTo("ABCD");
+    assertThat(issue.mustSendNotifications()).isTrue();
   }
 
   @Test
@@ -265,6 +285,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isTrue();
     assertThat(issue.isChanged()).isTrue();
     assertThat(issue.effortToFix()).isEqualTo(3.14);
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -274,6 +295,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isFalse();
     assertThat(issue.isChanged()).isFalse();
     assertThat(issue.effortToFix()).isEqualTo(3.14);
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -285,6 +307,7 @@ public class IssueUpdaterTest {
 
     // do not save change
     assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -293,6 +316,7 @@ public class IssueUpdaterTest {
     assertThat(updated).isTrue();
     assertThat(issue.isChanged()).isTrue();
     assertThat(issue.message()).isEqualTo("the message");
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -304,6 +328,7 @@ public class IssueUpdaterTest {
 
     // do not save change
     assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
@@ -315,5 +340,6 @@ public class IssueUpdaterTest {
     FieldDiffs.Diff diff = issue.currentChange().get("author");
     assertThat(diff.oldValue()).isNull();
     assertThat(diff.newValue()).isEqualTo("eric");
+    assertThat(issue.mustSendNotifications()).isFalse();
   }
 }
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java b/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java
index 0a4ba0394e8..739ec339918 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java
@@ -91,6 +91,9 @@ public class DefaultIssue implements Issue {
   // true if some fields have been changed since the previous scan
   private boolean isChanged = false;
 
+  // true if notifications have to be sent
+  private boolean sendNotifications = false;
+
   // Date when issue was loaded from db (only when isNew=false)
   private Date selectedAt;
 
@@ -309,6 +312,15 @@ public class DefaultIssue implements Issue {
     return this;
   }
 
+  public boolean mustSendNotifications() {
+    return sendNotifications;
+  }
+
+  public DefaultIssue setSendNotifications(boolean b) {
+    sendNotifications = b;
+    return this;
+  }
+
   @CheckForNull
   public String attribute(String key) {
     return attributes == null ? null : attributes.get(key);