SONAR-3755 improve email of issue changes

5 files changed, 91 insertions, 35 deletions

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplate.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplate.java
index 7750c8947ba..43dff3e911f 100644
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplate.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplate.java
@@ -19,6 +19,8 @@
  */
 package org.sonar.plugins.core.issue.notification;
 
+import com.google.common.base.Objects;
+import com.google.common.base.Strings;
 import org.apache.commons.lang.StringUtils;
 import org.sonar.api.config.EmailSettings;
 import org.sonar.api.notifications.Notification;
@@ -36,6 +38,8 @@ import javax.annotation.Nullable;
  */
 public class IssueChangesEmailTemplate extends EmailTemplate {
 
+  private static final char NEW_LINE = '\n';
+
   private final EmailSettings settings;
   private final UserFinder userFinder;
 
@@ -49,34 +53,57 @@ public class IssueChangesEmailTemplate extends EmailTemplate {
     if (!"issue-changes".equals(notif.getType())) {
       return null;
     }
-    String issueKey = notif.getFieldValue("key");
-    String author = notif.getFieldValue("changeAuthor");
 
     StringBuilder sb = new StringBuilder();
+    appendHeader(notif, sb);
+    sb.append(NEW_LINE);
+    appendChanges(notif, sb);
+    sb.append(NEW_LINE);
+    appendFooter(sb, notif);
+
     String projectName = notif.getFieldValue("projectName");
-    appendField(sb, "Project", null, projectName);
-    appendField(sb, "Component", null, StringUtils.defaultString(notif.getFieldValue("componentName"), notif.getFieldValue("componentKey")));
-    appendField(sb, "Rule", null, notif.getFieldValue("ruleName"));
-    appendField(sb, "Message", null, notif.getFieldValue("message"));
-    appendField(sb, "Comment", null, notif.getFieldValue("comment"));
-    sb.append('\n');
+    String issueKey = notif.getFieldValue("key");
+    String author = notif.getFieldValue("changeAuthor");
 
+    EmailMessage message = new EmailMessage()
+      .setMessageId("issue-changes/" + issueKey)
+      .setSubject(projectName + ", change on issue #" + issueKey)
+      .setMessage(sb.toString());
+    if (author != null) {
+      message.setFrom(getUserFullName(author));
+    }
+    return message;
+  }
+
+  private void appendChanges(Notification notif, StringBuilder sb) {
+    appendField(sb, "Comment", null, notif.getFieldValue("comment"));
     appendField(sb, "Assignee", notif.getFieldValue("old.assignee"), notif.getFieldValue("new.assignee"));
     appendField(sb, "Severity", notif.getFieldValue("old.severity"), notif.getFieldValue("new.severity"));
     appendField(sb, "Resolution", notif.getFieldValue("old.resolution"), notif.getFieldValue("new.resolution"));
     appendField(sb, "Status", notif.getFieldValue("old.status"), notif.getFieldValue("new.status"));
     appendField(sb, "Message", notif.getFieldValue("old.message"), notif.getFieldValue("new.message"));
+  }
 
-    appendFooter(sb, notif);
+  private void appendHeader(Notification notif, StringBuilder sb) {
+    String ruleName = notif.getFieldValue("ruleName");
+    String issueMessage = notif.getFieldValue("message");
 
-    EmailMessage message = new EmailMessage()
-      .setMessageId("issue-changes/" + issueKey)
-      .setSubject("Project "+ projectName +", change on issue #" + issueKey)
-      .setMessage(sb.toString());
-    if (author != null) {
-      message.setFrom(getUserFullName(author));
+    appendLine(sb, StringUtils.defaultString(notif.getFieldValue("componentName"), notif.getFieldValue("componentKey")));
+    appendLine(sb, ruleName);
+    if (!Objects.equal(ruleName, issueMessage)) {
+      appendLine(sb, issueMessage);
+    }
+  }
+
+  private void appendFooter(StringBuilder sb, Notification notification) {
+    String issueKey = notification.getFieldValue("key");
+    sb.append("See it in SonarQube: ").append(settings.getServerBaseURL()).append("/issue/show/").append(issueKey).append(NEW_LINE);
+  }
+
+  private void appendLine(StringBuilder sb, @Nullable String line) {
+    if (!Strings.isNullOrEmpty(line)) {
+      sb.append(line).append(NEW_LINE);
     }
-    return message;
   }
 
   private void appendField(StringBuilder sb, String name, @Nullable String oldValue, @Nullable String newValue) {
@@ -88,16 +115,10 @@ public class IssueChangesEmailTemplate extends EmailTemplate {
       if (oldValue != null) {
         sb.append(" (was ").append(oldValue).append(")");
       }
-      sb.append('\n');
+      sb.append(NEW_LINE);
     }
   }
 
-  private void appendFooter(StringBuilder sb, Notification notification) {
-    String issueKey = notification.getFieldValue("key");
-    sb.append("\n")
-      .append("See it in SonarQube: ").append(settings.getServerBaseURL()).append("/issue/show/").append(issueKey).append('\n');
-  }
-
   private String getUserFullName(@Nullable String login) {
     if (login == null) {
       return null;
diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest.java
index a3452e48283..ec198929714 100644
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest.java
@@ -29,6 +29,7 @@ import org.sonar.api.notifications.Notification;
 import org.sonar.api.user.User;
 import org.sonar.api.user.UserFinder;
 import org.sonar.plugins.emailnotifications.api.EmailMessage;
+import org.sonar.test.TestUtils;
 
 import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
@@ -57,10 +58,11 @@ public class IssueChangesEmailTemplateTest {
   }
 
   @Test
-  public void should_format_change() {
+  public void test_email_with_changes() {
     Notification notification = new Notification("issue-changes")
       .setFieldValue("projectName", "Struts")
       .setFieldValue("projectKey", "org.apache:struts")
+      .setFieldValue("componentName", "org.apache.struts.Action")
       .setFieldValue("key", "ABCDE")
       .setFieldValue("ruleName", "Avoid Cycles")
       .setFieldValue("message", "Has 3 cycles")
@@ -70,16 +72,33 @@ public class IssueChangesEmailTemplateTest {
       .setFieldValue("new.resolution", "FALSE-POSITIVE")
       .setFieldValue("new.status", "RESOLVED");
 
-    EmailMessage message = template.format(notification);
-    assertThat(message.getMessageId()).isEqualTo("issue-changes/ABCDE");
-    assertThat(message.getSubject()).isEqualTo("Project Struts, change on issue #ABCDE");
-    assertThat(message.getMessage()).contains("Rule: Avoid Cycles");
-    assertThat(message.getMessage()).contains("Message: Has 3 cycles");
-    assertThat(message.getMessage()).contains("Comment: How to fix it?");
-    assertThat(message.getMessage()).contains("Assignee: louis (was simon)");
-    assertThat(message.getMessage()).contains("Resolution: FALSE-POSITIVE");
-    assertThat(message.getMessage()).contains("See it in SonarQube: http://nemo.sonarsource.org/issue/show/ABCDE");
-    assertThat(message.getFrom()).isNull();
+    EmailMessage email = template.format(notification);
+    assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE");
+    assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE");
+
+    String message = email.getMessage();
+    String expectedMessage = TestUtils.getResourceContent("/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_changes.txt");
+    assertThat(message).isEqualTo(expectedMessage);
+    assertThat(email.getFrom()).isNull();
+  }
+
+  @Test
+  public void test_email_with_issue_message_same_than_rule_name() {
+    Notification notification = new Notification("issue-changes")
+      .setFieldValue("projectName", "Struts")
+      .setFieldValue("projectKey", "org.apache:struts")
+      .setFieldValue("componentName", "org.apache.struts.Action")
+      .setFieldValue("key", "ABCDE")
+      .setFieldValue("new.assignee", "louis")
+
+      // same rule name and issue msg -> display once
+      .setFieldValue("ruleName", "Avoid Cycles")
+      .setFieldValue("message", "Avoid Cycles");
+
+    EmailMessage email = template.format(notification);
+    String message = email.getMessage();
+    String expectedMessage = TestUtils.getResourceContent("/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_issue_message_same_than_rule_name.txt");
+    assertThat(message).isEqualTo(expectedMessage);
   }
 
   @Test
diff --git a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_changes.txt b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_changes.txt
new file mode 100644
index 00000000000..113d0ba6abb
--- /dev/null
+++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_changes.txt
@@ -0,0 +1,10 @@
+org.apache.struts.Action
+Avoid Cycles
+Has 3 cycles
+
+Comment: How to fix it?
+Assignee: louis (was simon)
+Resolution: FALSE-POSITIVE
+Status: RESOLVED
+
+See it in SonarQube: http://nemo.sonarsource.org/issue/show/ABCDE
diff --git a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_issue_message_same_than_rule_name.txt b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_issue_message_same_than_rule_name.txt
new file mode 100644
index 00000000000..f97b5d17be4
--- /dev/null
+++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_issue_message_same_than_rule_name.txt
@@ -0,0 +1,6 @@
+org.apache.struts.Action
+Avoid Cycles
+
+Assignee: louis
+
+See it in SonarQube: http://nemo.sonarsource.org/issue/show/ABCDE
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 f220fc821a5..51c08844168 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
@@ -95,7 +95,7 @@ public class IssueNotifications implements BatchComponent, ServerComponent {
     notification.setFieldValue("ruleName", ruleName(rule));
     notification.setFieldValue("componentKey", issue.componentKey());
     if (component != null) {
-      notification.setFieldValue("componentName", component.name());
+      notification.setFieldValue("componentName", component.longName());
     }
     if (comment != null) {
       notification.setFieldValue("comment", comment);