Skip to content

Commit

Permalink
SONAR-8672 Return issue types in issue change notifications
Browse files Browse the repository at this point in the history
  • Loading branch information
@julienlancelot
julienlancelot committed Feb 1, 2017
1 parent c4f0200 commit d639362
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 61 deletions.
4 changes: 1 addition & 3 deletions server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java
View file
Expand Up @@ -79,9 +79,7 @@ public boolean setType(DefaultIssue issue, RuleType type, IssueChangeContext con
} }


public boolean setSeverity(DefaultIssue issue, String severity, IssueChangeContext context) { public boolean setSeverity(DefaultIssue issue, String severity, IssueChangeContext context) {
if (issue.manualSeverity()) { checkState(!issue.manualSeverity(), "Severity can't be changed");
throw new IllegalStateException("Severity can't be changed");
}
if (!Objects.equals(severity, issue.severity())) { if (!Objects.equals(severity, issue.severity())) {
issue.setFieldChange(context, SEVERITY, issue.severity(), severity); issue.setFieldChange(context, SEVERITY, issue.severity(), severity);
issue.setSeverity(severity); issue.setSeverity(severity);
Expand Down
39 changes: 21 additions & 18 deletions ...r-server/src/main/java/org/sonar/server/issue/notification/IssueChangesEmailTemplate.java
View file
Expand Up @@ -20,29 +20,29 @@
package org.sonar.server.issue.notification; package org.sonar.server.issue.notification;


import com.google.common.base.Strings; import com.google.common.base.Strings;
import javax.annotation.CheckForNull;
import javax.annotation.Nullable;
import org.apache.commons.lang.StringUtils; import org.apache.commons.lang.StringUtils;
import org.sonar.api.config.EmailSettings; import org.sonar.api.config.EmailSettings;
import org.sonar.api.notifications.Notification; import org.sonar.api.notifications.Notification;
import org.sonar.api.user.User; import org.sonar.db.DbClient;
import org.sonar.api.user.UserFinder; import org.sonar.db.DbSession;
import org.sonar.db.user.UserDto;
import org.sonar.plugins.emailnotifications.api.EmailMessage; import org.sonar.plugins.emailnotifications.api.EmailMessage;
import org.sonar.plugins.emailnotifications.api.EmailTemplate; import org.sonar.plugins.emailnotifications.api.EmailTemplate;


import javax.annotation.CheckForNull;
import javax.annotation.Nullable;

/** /**
* Creates email message for notification "issue-changes". * Creates email message for notification "issue-changes".
*/ */
public class IssueChangesEmailTemplate extends EmailTemplate { public class IssueChangesEmailTemplate extends EmailTemplate {


private static final char NEW_LINE = '\n'; private static final char NEW_LINE = '\n';
private final DbClient dbClient;
private final EmailSettings settings; 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.settings = settings;
this.userFinder = userFinder;
} }


@Override @Override
Expand Down Expand Up @@ -72,10 +72,11 @@ public EmailMessage format(Notification notif) {
return message; return message;
} }


private void appendChanges(Notification notif, StringBuilder sb) { private static void appendChanges(Notification notif, StringBuilder sb) {
appendField(sb, "Comment", null, notif.getFieldValue("comment")); appendField(sb, "Comment", null, notif.getFieldValue("comment"));
appendFieldWithoutHistory(sb, "Assignee", notif.getFieldValue("old.assignee"), notif.getFieldValue("new.assignee")); appendFieldWithoutHistory(sb, "Assignee", notif.getFieldValue("old.assignee"), notif.getFieldValue("new.assignee"));
appendField(sb, "Severity", notif.getFieldValue("old.severity"), notif.getFieldValue("new.severity")); 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, "Resolution", notif.getFieldValue("old.resolution"), notif.getFieldValue("new.resolution"));
appendField(sb, "Status", notif.getFieldValue("old.status"), notif.getFieldValue("new.status")); appendField(sb, "Status", notif.getFieldValue("old.status"), notif.getFieldValue("new.status"));
appendField(sb, "Message", notif.getFieldValue("old.message"), notif.getFieldValue("new.message")); appendField(sb, "Message", notif.getFieldValue("old.message"), notif.getFieldValue("new.message"));
Expand All @@ -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"))); appendLine(sb, StringUtils.defaultString(notif.getFieldValue("componentName"), notif.getFieldValue("componentKey")));
appendField(sb, "Rule", null, notif.getFieldValue("ruleName")); appendField(sb, "Rule", null, notif.getFieldValue("ruleName"));
appendField(sb, "Message", null, notif.getFieldValue("message")); appendField(sb, "Message", null, notif.getFieldValue("message"));
Expand All @@ -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); 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)) { if (!Strings.isNullOrEmpty(line)) {
sb.append(line).append(NEW_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) { if (oldValue != null || newValue != null) {
sb.append(name).append(": "); sb.append(name).append(": ");
if (newValue != null) { if (newValue != null) {
Expand All @@ -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) { if (oldValue != null || newValue != null) {
sb.append(name); sb.append(name);
if (newValue != null) { if (newValue != null) {
Expand All @@ -140,12 +141,14 @@ private String getUserFullName(@Nullable String login) {
if (login == null) { if (login == null) {
return null; return null;
} }
User user = userFinder.findByLogin(login); try (DbSession dbSession = dbClient.openSession(false)) {
if (user == null) { UserDto userDto = dbClient.userDao().selectByLogin(dbSession, login);
// most probably user was deleted if (userDto == null || !userDto.isActive()) {
return login; // most probably user was deleted
return login;
}
return StringUtils.defaultIfBlank(userDto.getName(), login);
} }
return StringUtils.defaultIfBlank(user.name(), login);
} }


} }
77 changes: 37 additions & 40 deletions ...rver/src/test/java/org/sonar/server/issue/notification/IssueChangesEmailTemplateTest.java
View file
Expand Up @@ -20,43 +20,34 @@
package org.sonar.server.issue.notification; package org.sonar.server.issue.notification;


import com.google.common.io.Resources; import com.google.common.io.Resources;
import java.nio.charset.StandardCharsets;
import org.apache.commons.lang.StringUtils; import org.apache.commons.lang.StringUtils;
import org.junit.Before; import org.junit.Rule;
import org.junit.Test; 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.EmailSettings;
import org.sonar.api.config.MapSettings;
import org.sonar.api.config.Settings;
import org.sonar.api.notifications.Notification; import org.sonar.api.notifications.Notification;
import org.sonar.api.user.User; import org.sonar.db.DbTester;
import org.sonar.api.user.UserFinder;
import org.sonar.plugins.emailnotifications.api.EmailMessage; import org.sonar.plugins.emailnotifications.api.EmailMessage;


import java.nio.charset.StandardCharsets;

import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock; import static org.sonar.api.CoreProperties.SERVER_BASE_URL;
import static org.mockito.Mockito.when; import static org.sonar.db.user.UserTesting.newUserDto;


@RunWith(MockitoJUnitRunner.class)
public class IssueChangesEmailTemplateTest { public class IssueChangesEmailTemplateTest {


@Mock @Rule
UserFinder userFinder; public DbTester db = DbTester.create();


IssueChangesEmailTemplate template; private Settings settings = new MapSettings().setProperty(SERVER_BASE_URL, "http://nemo.sonarsource.org");


@Before private IssueChangesEmailTemplate underTest = new IssueChangesEmailTemplate(db.getDbClient(), new EmailSettings(settings));
public void setUp() {
EmailSettings settings = mock(EmailSettings.class);
when(settings.getServerBaseURL()).thenReturn("http://nemo.sonarsource.org");
template = new IssueChangesEmailTemplate(settings, userFinder);
}


@Test @Test
public void should_ignore_non_issue_changes() { public void should_ignore_non_issue_changes() {
Notification notification = new Notification("other"); Notification notification = new Notification("other");
EmailMessage message = template.format(notification); EmailMessage message = underTest.format(notification);
assertThat(message).isNull(); assertThat(message).isNull();
} }


Expand All @@ -66,15 +57,14 @@ public void email_should_display_assignee_change() throws Exception {
.setFieldValue("old.assignee", "simon") .setFieldValue("old.assignee", "simon")
.setFieldValue("new.assignee", "louis"); .setFieldValue("new.assignee", "louis");


EmailMessage email = template.format(notification); EmailMessage email = underTest.format(notification);
assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE");
assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE");


String message = email.getMessage(); String message = email.getMessage();
String expected = Resources.toString(Resources.getResource( String expected = Resources.toString(Resources.getResource(
"org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_assignee_change.txt"), "org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_assignee_change.txt"),
StandardCharsets.UTF_8 StandardCharsets.UTF_8);
);
expected = StringUtils.remove(expected, '\r'); expected = StringUtils.remove(expected, '\r');
assertThat(message).isEqualTo(expected); assertThat(message).isEqualTo(expected);
assertThat(email.getFrom()).isNull(); assertThat(email.getFrom()).isNull();
Expand All @@ -86,15 +76,14 @@ public void email_should_display_plan_change() throws Exception {
.setFieldValue("old.actionPlan", null) .setFieldValue("old.actionPlan", null)
.setFieldValue("new.actionPlan", "ABC 1.0"); .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.getMessageId()).isEqualTo("issue-changes/ABCDE");
assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE");


String message = email.getMessage(); String message = email.getMessage();
String expected = Resources.toString(Resources.getResource( String expected = Resources.toString(Resources.getResource(
"org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_action_plan_change.txt"), "org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_with_action_plan_change.txt"),
StandardCharsets.UTF_8 StandardCharsets.UTF_8);
);
expected = StringUtils.remove(expected, '\r'); expected = StringUtils.remove(expected, '\r');
assertThat(message).isEqualTo(expected); assertThat(message).isEqualTo(expected);
assertThat(email.getFrom()).isNull(); assertThat(email.getFrom()).isNull();
Expand All @@ -106,15 +95,14 @@ public void email_should_display_resolution_change() throws Exception {
.setFieldValue("old.resolution", "FALSE-POSITIVE") .setFieldValue("old.resolution", "FALSE-POSITIVE")
.setFieldValue("new.resolution", "FIXED"); .setFieldValue("new.resolution", "FIXED");


EmailMessage email = template.format(notification); EmailMessage email = underTest.format(notification);
assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE");
assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE");


String message = email.getMessage(); String message = email.getMessage();
String expected = Resources.toString(Resources.getResource( String expected = Resources.toString(Resources.getResource(
"org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_should_display_resolution_change.txt"), "org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/email_should_display_resolution_change.txt"),
StandardCharsets.UTF_8 StandardCharsets.UTF_8);
);
expected = StringUtils.remove(expected, '\r'); expected = StringUtils.remove(expected, '\r');
assertThat(message).isEqualTo(expected); assertThat(message).isEqualTo(expected);
assertThat(email.getFrom()).isNull(); assertThat(email.getFrom()).isNull();
Expand All @@ -125,15 +113,14 @@ public void display_component_key_if_no_component_name() throws Exception {
Notification notification = generateNotification() Notification notification = generateNotification()
.setFieldValue("componentName", null); .setFieldValue("componentName", null);


EmailMessage email = template.format(notification); EmailMessage email = underTest.format(notification);
assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE");
assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE");


String message = email.getMessage(); String message = email.getMessage();
String expected = Resources.toString(Resources.getResource( String expected = Resources.toString(Resources.getResource(
"org/sonar/server/issue/notification/IssueChangesEmailTemplateTest/display_component_key_if_no_component_name.txt"), "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'); expected = StringUtils.remove(expected, '\r');
assertThat(message).isEqualTo(expected); assertThat(message).isEqualTo(expected);
} }
Expand All @@ -146,9 +133,10 @@ public void test_email_with_multiple_changes() throws Exception {
.setFieldValue("new.assignee", "louis") .setFieldValue("new.assignee", "louis")
.setFieldValue("new.resolution", "FALSE-POSITIVE") .setFieldValue("new.resolution", "FALSE-POSITIVE")
.setFieldValue("new.status", "RESOLVED") .setFieldValue("new.status", "RESOLVED")
.setFieldValue("new.type", "BUG")
.setFieldValue("new.tags", "bug performance"); .setFieldValue("new.tags", "bug performance");


EmailMessage email = template.format(notification); EmailMessage email = underTest.format(notification);
assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE"); assertThat(email.getMessageId()).isEqualTo("issue-changes/ABCDE");
assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE"); assertThat(email.getSubject()).isEqualTo("Struts, change on issue #ABCDE");


Expand All @@ -162,27 +150,36 @@ public void test_email_with_multiple_changes() throws Exception {


@Test @Test
public void notification_sender_should_be_the_author_of_change() { public void notification_sender_should_be_the_author_of_change() {
User user = mock(User.class); db.users().insertUser(newUserDto().setLogin("simon").setName("Simon"));
when(user.name()).thenReturn("Simon");
when(userFinder.findByLogin("simon")).thenReturn(user);


Notification notification = new IssueChangeNotification() Notification notification = new IssueChangeNotification()
.setChangeAuthorLogin("simon") .setChangeAuthorLogin("simon")
.setProject("Struts", "org.apache:struts"); .setProject("Struts", "org.apache:struts");


EmailMessage message = template.format(notification); EmailMessage message = underTest.format(notification);
assertThat(message.getFrom()).isEqualTo("Simon"); 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() 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("projectName", "Struts")
.setFieldValue("projectKey", "org.apache:struts") .setFieldValue("projectKey", "org.apache:struts")
.setFieldValue("componentName", "Action") .setFieldValue("componentName", "Action")
.setFieldValue("componentKey", "org.apache.struts.Action") .setFieldValue("componentKey", "org.apache.struts.Action")
.setFieldValue("key", "ABCDE") .setFieldValue("key", "ABCDE")
.setFieldValue("ruleName", "Avoid Cycles") .setFieldValue("ruleName", "Avoid Cycles")
.setFieldValue("message", "Has 3 cycles"); .setFieldValue("message", "Has 3 cycles");
return notification;
} }
} }
1 change: 1 addition & 0 deletions ...r/server/issue/notification/IssueChangesEmailTemplateTest/email_with_multiple_changes.txt
View file
Expand Up @@ -4,6 +4,7 @@ Message: Has 3 cycles


Comment: How to fix it? Comment: How to fix it?
Assignee changed to louis Assignee changed to louis
Type: BUG
Resolution: FALSE-POSITIVE Resolution: FALSE-POSITIVE
Status: RESOLVED Status: RESOLVED
Tags: [bug performance] Tags: [bug performance]
Expand Down

0 comments on commit d639362

Please sign in to comment.