From d6393623490fb38fa14c7e3a4a72bbcdbc47b835 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Tue, 31 Jan 2017 15:33:05 +0100 Subject: [PATCH] SONAR-8672 Return issue types in issue change notifications --- .../sonar/server/issue/IssueFieldsSetter.java | 4 +- .../IssueChangesEmailTemplate.java | 39 +++++----- .../IssueChangesEmailTemplateTest.java | 77 +++++++++---------- .../email_with_multiple_changes.txt | 1 + 4 files changed, 60 insertions(+), 61 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java index d604a124a56..40a35124acb 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java @@ -79,9 +79,7 @@ public boolean setType(DefaultIssue issue, RuleType type, IssueChangeContext con } public boolean setSeverity(DefaultIssue issue, String severity, IssueChangeContext context) { - if (issue.manualSeverity()) { - throw new IllegalStateException("Severity can't be changed"); - } + checkState(!issue.manualSeverity(), "Severity can't be changed"); if (!Objects.equals(severity, issue.severity())) { issue.setFieldChange(context, SEVERITY, issue.severity(), severity); issue.setSeverity(severity); diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/notification/IssueChangesEmailTemplate.java b/server/sonar-server/src/main/java/org/sonar/server/issue/notification/IssueChangesEmailTemplate.java index 23914040db4..c90d8d235a3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/notification/IssueChangesEmailTemplate.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/notification/IssueChangesEmailTemplate.java @@ -20,29 +20,29 @@ package org.sonar.server.issue.notification; import com.google.common.base.Strings; +import javax.annotation.CheckForNull; +import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.sonar.api.config.EmailSettings; import org.sonar.api.notifications.Notification; -import org.sonar.api.user.User; -import org.sonar.api.user.UserFinder; +import org.sonar.db.DbClient; +import org.sonar.db.DbSession; +import org.sonar.db.user.UserDto; import org.sonar.plugins.emailnotifications.api.EmailMessage; import org.sonar.plugins.emailnotifications.api.EmailTemplate; -import javax.annotation.CheckForNull; -import javax.annotation.Nullable; - /** * Creates email message for notification "issue-changes". */ public class IssueChangesEmailTemplate extends EmailTemplate { private static final char NEW_LINE = '\n'; + private final DbClient dbClient; private final EmailSettings settings; - private final UserFinder userFinder; - public IssueChangesEmailTemplate(EmailSettings settings, UserFinder userFinder) { + public IssueChangesEmailTemplate(DbClient dbClient, EmailSettings settings) { + this.dbClient = dbClient; this.settings = settings; - this.userFinder = userFinder; } @Override @@ -72,10 +72,11 @@ public EmailMessage format(Notification notif) { return message; } - private void appendChanges(Notification notif, StringBuilder sb) { + private static void appendChanges(Notification notif, StringBuilder sb) { appendField(sb, "Comment", null, notif.getFieldValue("comment")); appendFieldWithoutHistory(sb, "Assignee", notif.getFieldValue("old.assignee"), notif.getFieldValue("new.assignee")); appendField(sb, "Severity", notif.getFieldValue("old.severity"), notif.getFieldValue("new.severity")); + appendField(sb, "Type", notif.getFieldValue("old.type"), notif.getFieldValue("new.type")); 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")); @@ -93,7 +94,7 @@ private static String formatTagChange(@Nullable String tags) { } } - private void appendHeader(Notification notif, StringBuilder sb) { + private static void appendHeader(Notification notif, StringBuilder sb) { appendLine(sb, StringUtils.defaultString(notif.getFieldValue("componentName"), notif.getFieldValue("componentKey"))); appendField(sb, "Rule", null, notif.getFieldValue("ruleName")); appendField(sb, "Message", null, notif.getFieldValue("message")); @@ -104,13 +105,13 @@ private void appendFooter(StringBuilder sb, Notification notification) { sb.append("See it in SonarQube: ").append(settings.getServerBaseURL()).append("/issues/search#issues=").append(issueKey).append(NEW_LINE); } - private void appendLine(StringBuilder sb, @Nullable String line) { + private static void appendLine(StringBuilder sb, @Nullable String line) { if (!Strings.isNullOrEmpty(line)) { sb.append(line).append(NEW_LINE); } } - private void appendField(StringBuilder sb, String name, @Nullable String oldValue, @Nullable String newValue) { + private static void appendField(StringBuilder sb, String name, @Nullable String oldValue, @Nullable String newValue) { if (oldValue != null || newValue != null) { sb.append(name).append(": "); if (newValue != null) { @@ -123,7 +124,7 @@ private void appendField(StringBuilder sb, String name, @Nullable String oldValu } } - private void appendFieldWithoutHistory(StringBuilder sb, String name, @Nullable String oldValue, @Nullable String newValue) { + private static void appendFieldWithoutHistory(StringBuilder sb, String name, @Nullable String oldValue, @Nullable String newValue) { if (oldValue != null || newValue != null) { sb.append(name); if (newValue != null) { @@ -140,12 +141,14 @@ private String getUserFullName(@Nullable String login) { if (login == null) { return null; } - User user = userFinder.findByLogin(login); - if (user == null) { - // most probably user was deleted - return login; + try (DbSession dbSession = dbClient.openSession(false)) { + UserDto userDto = dbClient.userDao().selectByLogin(dbSession, login); + if (userDto == null || !userDto.isActive()) { + // most probably user was deleted + return login; + } + return StringUtils.defaultIfBlank(userDto.getName(), login); } - return StringUtils.defaultIfBlank(user.name(), login); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest.java index ee8789673c6..99a2ced0bda 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest.java @@ -20,43 +20,34 @@ package org.sonar.server.issue.notification; import com.google.common.io.Resources; +import java.nio.charset.StandardCharsets; import org.apache.commons.lang.StringUtils; -import org.junit.Before; +import org.junit.Rule; import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.runners.MockitoJUnitRunner; import org.sonar.api.config.EmailSettings; +import org.sonar.api.config.MapSettings; +import org.sonar.api.config.Settings; import org.sonar.api.notifications.Notification; -import org.sonar.api.user.User; -import org.sonar.api.user.UserFinder; +import org.sonar.db.DbTester; import org.sonar.plugins.emailnotifications.api.EmailMessage; -import java.nio.charset.StandardCharsets; - import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.sonar.api.CoreProperties.SERVER_BASE_URL; +import static org.sonar.db.user.UserTesting.newUserDto; -@RunWith(MockitoJUnitRunner.class) public class IssueChangesEmailTemplateTest { - @Mock - UserFinder userFinder; + @Rule + public DbTester db = DbTester.create(); - IssueChangesEmailTemplate template; + private Settings settings = new MapSettings().setProperty(SERVER_BASE_URL, "http://nemo.sonarsource.org"); - @Before - public void setUp() { - EmailSettings settings = mock(EmailSettings.class); - when(settings.getServerBaseURL()).thenReturn("http://nemo.sonarsource.org"); - template = new IssueChangesEmailTemplate(settings, userFinder); - } + private IssueChangesEmailTemplate underTest = new IssueChangesEmailTemplate(db.getDbClient(), new EmailSettings(settings)); @Test public void should_ignore_non_issue_changes() { Notification notification = new Notification("other"); - EmailMessage message = template.format(notification); + EmailMessage message = underTest.format(notification); assertThat(message).isNull(); } @@ -66,15 +57,14 @@ public void email_should_display_assignee_change() throws Exception { .setFieldValue("old.assignee", "simon") .setFieldValue("new.assignee", "louis"); - EmailMessage email = template.format(notification); + EmailMessage email = underTest.format(notification); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); String message = email.getMessage(); String expected = Resources.toString(Resources.getResource( "org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_assignee_change.txt"), - StandardCharsets.UTF_8 - ); + StandardCharsets.UTF_8); expected = StringUtils.remove(expected, '\r'); assertThat(message).isEqualTo(expected); assertThat(email.getFrom()).isNull(); @@ -86,15 +76,14 @@ public void email_should_display_plan_change() throws Exception { .setFieldValue("old.actionPlan", null) .setFieldValue("new.actionPlan", "ABC 1.0"); - EmailMessage email = template.format(notification); + EmailMessage email = underTest.format(notification); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); String message = email.getMessage(); String expected = Resources.toString(Resources.getResource( "org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_action_plan_change.txt"), - StandardCharsets.UTF_8 - ); + StandardCharsets.UTF_8); expected = StringUtils.remove(expected, '\r'); assertThat(message).isEqualTo(expected); assertThat(email.getFrom()).isNull(); @@ -106,15 +95,14 @@ public void email_should_display_resolution_change() throws Exception { .setFieldValue("old.resolution", "FALSE-POSITIVE") .setFieldValue("new.resolution", "FIXED"); - EmailMessage email = template.format(notification); + EmailMessage email = underTest.format(notification); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); String message = email.getMessage(); String expected = Resources.toString(Resources.getResource( "org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_should_display_resolution_change.txt"), - StandardCharsets.UTF_8 - ); + StandardCharsets.UTF_8); expected = StringUtils.remove(expected, '\r'); assertThat(message).isEqualTo(expected); assertThat(email.getFrom()).isNull(); @@ -125,15 +113,14 @@ public void display_component_key_if_no_component_name() throws Exception { Notification notification = generateNotification() .setFieldValue("componentName", null); - EmailMessage email = template.format(notification); + EmailMessage email = underTest.format(notification); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); String message = email.getMessage(); String expected = Resources.toString(Resources.getResource( "org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/display_component_key_if_no_component_name.txt"), - StandardCharsets.UTF_8 - ); + StandardCharsets.UTF_8); expected = StringUtils.remove(expected, '\r'); assertThat(message).isEqualTo(expected); } @@ -146,9 +133,10 @@ public void test_email_with_multiple_changes() throws Exception { .setFieldValue("new.assignee", "louis") .setFieldValue("new.resolution", "FALSE-POSITIVE") .setFieldValue("new.status", "RESOLVED") + .setFieldValue("new.type", "BUG") .setFieldValue("new.tags", "bug performance"); - EmailMessage email = template.format(notification); + EmailMessage email = underTest.format(notification); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); @@ -162,20 +150,30 @@ public void test_email_with_multiple_changes() throws Exception { @Test public void notification_sender_should_be_the_author_of_change() { - User user = mock(User.class); - when(user.name()).thenReturn("Simon"); - when(userFinder.findByLogin("simon")).thenReturn(user); + db.users().insertUser(newUserDto().setLogin("simon").setName("Simon")); Notification notification = new IssueChangeNotification() .setChangeAuthorLogin("simon") .setProject("Struts", "org.apache:struts"); - EmailMessage message = template.format(notification); + EmailMessage message = underTest.format(notification); assertThat(message.getFrom()).isEqualTo("Simon"); } - private Notification generateNotification() { + @Test + public void notification_contains_user_login_when_user_is_removed() { + db.users().insertUser(newUserDto().setLogin("simon").setName("Simon").setActive(false)); + Notification notification = new IssueChangeNotification() + .setChangeAuthorLogin("simon") + .setProject("Struts", "org.apache:struts"); + + EmailMessage message = underTest.format(notification); + assertThat(message.getFrom()).isEqualTo("simon"); + } + + private static Notification generateNotification() { + return new IssueChangeNotification() .setFieldValue("projectName", "Struts") .setFieldValue("projectKey", "org.apache:struts") .setFieldValue("componentName", "Action") @@ -183,6 +181,5 @@ private Notification generateNotification() { .setFieldValue("key", "ABCDE") .setFieldValue("ruleName", "Avoid Cycles") .setFieldValue("message", "Has 3 cycles"); - return notification; } } diff --git a/server/sonar-server/src/test/resources/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_multiple_changes.txt b/server/sonar-server/src/test/resources/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_multiple_changes.txt index 6f4b09018ea..48de7c38585 100644 --- a/server/sonar-server/src/test/resources/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_multiple_changes.txt +++ b/server/sonar-server/src/test/resources/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_multiple_changes.txt @@ -4,6 +4,7 @@ Message: Has 3 cycles Comment: How to fix it? Assignee changed to louis +Type: BUG Resolution: FALSE-POSITIVE Status: RESOLVED Tags: [bug performance]