From 1afee765024b8ed5bb41af811fd56b7d8f2b1000 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Sun, 2 Jun 2013 10:18:19 +0200 Subject: [PATCH] SONAR-3755 improve email of issue changes --- .../IssueChangesEmailTemplate.java | 67 ++++++++++++------- .../IssueChangesEmailTemplateTest.java | 41 +++++++++--- .../email_with_changes.txt | 10 +++ ...with_issue_message_same_than_rule_name.txt | 6 ++ .../sonar/core/issue/IssueNotifications.java | 2 +- 5 files changed, 91 insertions(+), 35 deletions(-) create mode 100644 plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_changes.txt create mode 100644 plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/issue/notification/IssueChangesEmailTemplateTest/email_with_issue_message_same_than_rule_name.txt 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); -- 2.39.5