From 73b1299333b3e0707a4d73e4c4fad1874a158eac Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 30 May 2013 15:46:13 +0200 Subject: [PATCH] SONAR-4283 Add notification on new comment, and change emails subject --- .../IssueChangesEmailTemplate.java | 6 ++- .../notification/NewIssuesEmailTemplate.java | 2 +- .../resources/org/sonar/l10n/core.properties | 1 + .../IssueChangesEmailTemplateTest.java | 4 +- .../NewIssuesEmailTemplateTest.java | 4 +- .../sonar/core/issue/IssueNotifications.java | 49 +++++++++++++------ .../core/issue/IssueNotificationsTest.java | 20 ++++++++ .../server/issue/IssueCommentService.java | 22 ++++++--- .../account/_global_notifications.html.erb | 46 ++++++++--------- 9 files changed, 105 insertions(+), 49 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 a24c2439b6d..7750c8947ba 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 @@ -53,10 +53,12 @@ public class IssueChangesEmailTemplate extends EmailTemplate { String author = notif.getFieldValue("changeAuthor"); StringBuilder sb = new StringBuilder(); - appendField(sb, "Project", null, notif.getFieldValue("projectName")); + 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'); appendField(sb, "Assignee", notif.getFieldValue("old.assignee"), notif.getFieldValue("new.assignee")); @@ -69,7 +71,7 @@ public class IssueChangesEmailTemplate extends EmailTemplate { EmailMessage message = new EmailMessage() .setMessageId("issue-changes/" + issueKey) - .setSubject("Issue #" + issueKey) + .setSubject("Project "+ projectName +", change on issue #" + issueKey) .setMessage(sb.toString()); if (author != null) { message.setFrom(getUserFullName(author)); diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplate.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplate.java index 9678c5d542f..1199da9ff11 100644 --- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplate.java +++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplate.java @@ -56,7 +56,7 @@ public class NewIssuesEmailTemplate extends EmailTemplate { EmailMessage message = new EmailMessage() .setMessageId("new-issues/" + notification.getFieldValue("projectKey")) - .setSubject("New issues for project " + projectName) + .setSubject("Project " + projectName + ", new issues") .setMessage(sb.toString()); return message; diff --git a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties index 5f27346a6ae..72291d3a6be 100644 --- a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties +++ b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties @@ -1591,6 +1591,7 @@ server_id_configuration.does_not_match_organisation_pattern=Organisation does no # #------------------------------------------------------------------------------ notification.channel.EmailNotificationChannel=Email +notification.dispatcher.information=Subscribe to following channels to be notified when the related events occur. A notification is never sent to the author of the event. notification.dispatcher.ChangesOnMyIssue=Changes in issues assigned to me or reported by me notification.dispatcher.NewIssues=New issues notification.dispatcher.NewAlerts=New alerts 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 c84b80cf789..a3452e48283 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 @@ -64,6 +64,7 @@ public class IssueChangesEmailTemplateTest { .setFieldValue("key", "ABCDE") .setFieldValue("ruleName", "Avoid Cycles") .setFieldValue("message", "Has 3 cycles") + .setFieldValue("comment", "How to fix it?") .setFieldValue("old.assignee", "simon") .setFieldValue("new.assignee", "louis") .setFieldValue("new.resolution", "FALSE-POSITIVE") @@ -71,9 +72,10 @@ public class IssueChangesEmailTemplateTest { EmailMessage message = template.format(notification); assertThat(message.getMessageId()).isEqualTo("issue-changes/ABCDE"); - assertThat(message.getSubject()).isEqualTo("Issue #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"); diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplateTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplateTest.java index 0f862b13259..af3cc7e4430 100644 --- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplateTest.java +++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/NewIssuesEmailTemplateTest.java @@ -49,7 +49,7 @@ public class NewIssuesEmailTemplateTest { /** * 
-   * Subject: New issues for project Foo
+   * Subject: Project Struts, new issues
    * From: Sonar
    *
    * Project: Foo
@@ -68,7 +68,7 @@ public class NewIssuesEmailTemplateTest {
 
     EmailMessage message = template.format(notification);
     assertThat(message.getMessageId()).isEqualTo("new-issues/org.apache:struts");
-    assertThat(message.getSubject()).isEqualTo("New issues for project Struts");
+    assertThat(message.getSubject()).isEqualTo("Project Struts, new issues");
     assertThat(message.getMessage()).isEqualTo("" +
       "Project: Struts\n" +
       "32 new issues\n" +
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 0fc2debfc09..b7cc1fc5504 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
@@ -32,6 +32,7 @@ import org.sonar.core.i18n.RuleI18nManager;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
+
 import java.util.Locale;
 import java.util.Map;
 
@@ -67,28 +68,46 @@ public class IssueNotifications implements BatchComponent, ServerComponent {
   @CheckForNull
   public Notification sendChanges(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project, @Nullable Component component) {
     if (issue.diffs() != 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.name());
-      }
+      Notification notification = createNotification(issue, context, rule, project, component, null);
+      notificationsManager.scheduleForSending(notification);
+      return notification;
+    }
+    return null;
+  }
 
-      for (Map.Entry entry : issue.diffs().diffs().entrySet()) {
+  @CheckForNull
+  public Notification sendChanges(DefaultIssue issue, IssueChangeContext context, IssueQueryResult queryResult, @Nullable String comment) {
+    Notification notification = createNotification(issue, context, queryResult.rule(issue), queryResult.project(issue), queryResult.component(issue), comment);
+    notificationsManager.scheduleForSending(notification);
+    return notification;
+  }
+
+  private Notification createNotification(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project, @Nullable Component component, @Nullable String comment){
+    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.name());
+    }
+    if (comment != null) {
+      notification.setFieldValue("comment", comment);
+    }
+
+    FieldDiffs diffs = issue.diffs();
+    if (diffs != null) {
+      for (Map.Entry entry : diffs.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);
       }
-      notificationsManager.scheduleForSending(notification);
-      return notification;
     }
-    return null;
+    return notification;
   }
 
   private String ruleName(@Nullable Rule rule) {
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 34789bb72ce..af263e33b44 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
@@ -89,4 +89,24 @@ public class IssueNotificationsTest {
     assertThat(notification.getFieldValue("new.status")).isEqualTo("RESOLVED");
     Mockito.verify(manager).scheduleForSending(notification);
   }
+
+  @Test
+  public void sendChangesWithComment() throws Exception {
+    IssueChangeContext context = IssueChangeContext.createScan(new Date());
+    DefaultIssue issue = new DefaultIssue()
+      .setMessage("the message")
+      .setKey("ABCDE")
+      .setAssignee("freddy")
+      .setComponentKey("struts:Action")
+      .setProjectKey("struts");
+    DefaultIssueQueryResult queryResult = new DefaultIssueQueryResult(Arrays.asList(issue));
+    queryResult.addProjects(Arrays.asList(new Project("struts")));
+
+    Notification notification = issueNotifications.sendChanges(issue, context, queryResult, "I don't know how to fix it?");
+
+    assertThat(notification.getFieldValue("message")).isEqualTo("the message");
+    assertThat(notification.getFieldValue("key")).isEqualTo("ABCDE");
+    assertThat(notification.getFieldValue("comment")).isEqualTo("I don't know how to fix it?");
+    Mockito.verify(manager).scheduleForSending(notification);
+  }
 }
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueCommentService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueCommentService.java
index 8a518028514..684817d0cff 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueCommentService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueCommentService.java
@@ -23,16 +23,16 @@ import com.google.common.base.Objects;
 import com.google.common.base.Strings;
 import org.sonar.api.ServerComponent;
 import org.sonar.api.issue.IssueComment;
+import org.sonar.api.issue.IssueQuery;
+import org.sonar.api.issue.IssueQueryResult;
 import org.sonar.api.web.UserRole;
-import org.sonar.core.issue.DefaultIssue;
-import org.sonar.core.issue.DefaultIssueComment;
-import org.sonar.core.issue.IssueChangeContext;
-import org.sonar.core.issue.IssueUpdater;
+import org.sonar.core.issue.*;
 import org.sonar.core.issue.db.IssueChangeDao;
 import org.sonar.core.issue.db.IssueChangeDto;
 import org.sonar.core.issue.db.IssueStorage;
 import org.sonar.server.user.UserSession;
 
+import java.util.Arrays;
 import java.util.Date;
 
 public class IssueCommentService implements ServerComponent {
@@ -41,12 +41,14 @@ public class IssueCommentService implements ServerComponent {
   private final IssueChangeDao changeDao;
   private final IssueStorage storage;
   private final DefaultIssueFinder finder;
+  private final IssueNotifications issueNotifications;
 
-  public IssueCommentService(IssueUpdater updater, IssueChangeDao changeDao, IssueStorage storage, DefaultIssueFinder finder) {
+  public IssueCommentService(IssueUpdater updater, IssueChangeDao changeDao, IssueStorage storage, DefaultIssueFinder finder, IssueNotifications issueNotifications) {
     this.updater = updater;
     this.changeDao = changeDao;
     this.storage = storage;
     this.finder = finder;
+    this.issueNotifications = issueNotifications;
   }
 
   public IssueComment findComment(String commentKey) {
@@ -55,11 +57,14 @@ public class IssueCommentService implements ServerComponent {
 
   public IssueComment addComment(String issueKey, String text, UserSession userSession) {
     verifyLoggedIn(userSession);
-    DefaultIssue issue = finder.findByKey(issueKey, UserRole.USER);
+
+    IssueQueryResult queryResult = loadIssue(issueKey);
+    DefaultIssue issue = (DefaultIssue) queryResult.first();
 
     IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login());
     updater.addComment(issue, text, context);
     storage.save(issue);
+    issueNotifications.sendChanges(issue, context, queryResult, text);
     return issue.comments().get(issue.comments().size() - 1);
   }
 
@@ -109,4 +114,9 @@ public class IssueCommentService implements ServerComponent {
       throw new IllegalStateException("User is not logged in");
     }
   }
+
+  public IssueQueryResult loadIssue(String issueKey) {
+    IssueQuery query = IssueQuery.builder().issueKeys(Arrays.asList(issueKey)).requiredRole(UserRole.USER).build();
+    return finder.find(query);
+  }
 }
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/account/_global_notifications.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/account/_global_notifications.html.erb
index dbad8b052d3..8be9b588467 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/account/_global_notifications.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/account/_global_notifications.html.erb
@@ -1,25 +1,27 @@
 
<%= message('my_profile.overall_notifications.title') -%>

 
-    
-      
-        
-        <% for channel in @channels %>
-          
-        <% end %>
-      
-      <% for dispatcher in @global_dispatchers %>
-        
-          
-          <%
-               for channel in @channels
-                 notification_id = dispatcher + '.' + channel.getKey()
-                 check_box_id = 'global_notifs[' + notification_id + ']'
-                 check_box_checked = @global_notifications[notification_id]
-          %>
-          
-          <% end %>
-        
+<%= message('notification.dispatcher.information') -%>
+


+
<%= message('notification.channel.' + channel.getKey()) -%>
<%= message('notification.dispatcher.' + dispatcher) -%>
-              <%= check_box_tag check_box_id, 'true', check_box_checked %>
-          

+  
+    
+    <% for channel in @channels %>
+      
+    <% end %>
+  
+  <% for dispatcher in @global_dispatchers %>
+    
+      
+      <%
+           for channel in @channels
+             notification_id = dispatcher + '.' + channel.getKey()
+             check_box_id = 'global_notifs[' + notification_id + ']'
+             check_box_checked = @global_notifications[notification_id]
+      %>
+      
       <% end %>
-    
<%= message('notification.channel.' + channel.getKey()) -%>
<%= message('notification.dispatcher.' + dispatcher) -%>
+          <%= check_box_tag check_box_id, 'true', check_box_checked %>
+      

+    
+  <% end %>
+
-- 
2.39.5