From fcae413fe38ec35d9f12664b6b644c34626f4d92 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 30 Jul 2013 16:38:00 +0200 Subject: [PATCH] SONAR-4540 Notifications should not be sent on changes of issue line, author or effort to fix --- .../SendIssueNotificationsPostJob.java | 2 +- .../SendIssueNotificationsPostJobTest.java | 23 ++++++++- .../sonar/core/issue/IssueNotifications.java | 48 +++++++++---------- .../org/sonar/core/issue/IssueUpdater.java | 8 ++++ .../core/issue/IssueNotificationsTest.java | 1 + .../sonar/core/issue/IssueUpdaterTest.java | 28 ++++++++++- .../api/issue/internal/DefaultIssue.java | 12 +++++ 7 files changed, 95 insertions(+), 27 deletions(-) diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java index 73d41db228a..0a3658a34a0 100644 --- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java +++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java @@ -58,7 +58,7 @@ public class SendIssueNotificationsPostJob implements PostJob { if (issue.isNew() && issue.resolution() == null) { newIssues++; } - if (!issue.isNew() && issue.isChanged()) { + if (!issue.isNew() && issue.isChanged() && issue.mustSendNotifications()) { Rule rule = ruleFinder.findByKey(issue.ruleKey()); // TODO warning - rules with status REMOVED are currently ignored, but should not if (rule != null) { diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java index fc61ced163a..736049d7ae8 100644 --- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java +++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java @@ -84,13 +84,14 @@ public class SendIssueNotificationsPostJobTest { } @Test - public void should_send_notification_if_issue_change() throws Exception { + public void should_send_notification() throws Exception { when(project.getAnalysisDate()).thenReturn(DateUtils.parseDate("2013-05-18")); RuleKey ruleKey = RuleKey.of("squid", "AvoidCycles"); Rule rule = new Rule("squid", "AvoidCycles"); DefaultIssue issue = new DefaultIssue() .setNew(false) .setChanged(true) + .setSendNotifications(true) .setFieldChange(mock(IssueChangeContext.class), "severity", "MINOR", "BLOCKER") .setRuleKey(ruleKey); when(issueCache.all()).thenReturn(Arrays.asList(issue)); @@ -120,4 +121,24 @@ public class SendIssueNotificationsPostJobTest { verify(notifications, never()).sendChanges(eq(issue), eq(changeContext), any(Rule.class), any(Component.class), any(Component.class)); } + + @Test + public void should_not_send_notification_on_any_change() throws Exception { + IssueChangeContext changeContext = mock(IssueChangeContext.class); + + when(project.getAnalysisDate()).thenReturn(DateUtils.parseDate("2013-05-18")); + RuleKey ruleKey = RuleKey.of("squid", "AvoidCycles"); + DefaultIssue issue = new DefaultIssue() + .setChanged(true) + .setSendNotifications(false) + .setFieldChange(changeContext, "severity", "MINOR", "BLOCKER") + .setRuleKey(ruleKey); + when(issueCache.all()).thenReturn(Arrays.asList(issue)); + when(ruleFinder.findByKey(ruleKey)).thenReturn(null); + + SendIssueNotificationsPostJob job = new SendIssueNotificationsPostJob(issueCache, notifications, ruleFinder); + job.executeOn(project, sensorContext); + + verify(notifications, never()).sendChanges(eq(issue), eq(changeContext), any(Rule.class), any(Component.class), any(Component.class)); + } } 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 93d67e5c62d..1a96d48446d 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 @@ -88,31 +88,31 @@ public class IssueNotifications implements BatchComponent, ServerComponent { @CheckForNull private Notification createChangeNotification(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project, @Nullable Component component, @Nullable String comment) { - FieldDiffs currentChange = issue.currentChange(); - if (comment == null && (currentChange == null || currentChange.diffs().isEmpty())) { - return 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.longName()); - } - if (comment != null) { - notification.setFieldValue("comment", comment); - } + Notification notification = null; + if (comment != null || issue.mustSendNotifications()) { + FieldDiffs currentChange = issue.currentChange(); + 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.longName()); + } + if (comment != null) { + notification.setFieldValue("comment", comment); + } - if (currentChange != null) { - for (Map.Entry entry : currentChange.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); + if (currentChange != null) { + for (Map.Entry entry : currentChange.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); + } } } return notification; diff --git a/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java b/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java index a91d401e0df..3c5a80c53a0 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java +++ b/sonar-core/src/main/java/org/sonar/core/issue/IssueUpdater.java @@ -63,6 +63,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent { issue.setManualSeverity(true); issue.setUpdateDate(context.date()); issue.setChanged(true); + issue.setSendNotifications(true); return true; } return false; @@ -75,6 +76,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent { issue.setAssignee(sanitizedAssignee); issue.setUpdateDate(context.date()); issue.setChanged(true); + issue.setSendNotifications(true); return true; } return false; @@ -101,6 +103,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent { issue.setResolution(resolution); issue.setUpdateDate(context.date()); issue.setChanged(true); + issue.setSendNotifications(true); return true; } return false; @@ -112,6 +115,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent { issue.setStatus(status); issue.setUpdateDate(context.date()); issue.setChanged(true); + issue.setSendNotifications(true); return true; } return false; @@ -123,6 +127,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent { issue.setAuthorLogin(authorLogin); issue.setUpdateDate(context.date()); issue.setChanged(true); + // do not send notifications to prevent spam when installing the developer cockpit plugin return true; } return false; @@ -163,6 +168,8 @@ public class IssueUpdater implements BatchComponent, ServerComponent { issue.setEffortToFix(d); issue.setUpdateDate(context.date()); issue.setChanged(true); + // Do not send notifications to prevent spam when installing the SQALE plugin, + // and do not complete the changelog (for the moment) return true; } return false; @@ -193,6 +200,7 @@ public class IssueUpdater implements BatchComponent, ServerComponent { issue.setActionPlanKey(sanitizedActionPlanKey); issue.setUpdateDate(context.date()); issue.setChanged(true); + issue.setSendNotifications(true); return true; } return false; 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 9f85faa3be9..f0b464fb987 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 @@ -76,6 +76,7 @@ public class IssueNotificationsTest { .setAssignee("freddy") .setFieldChange(context, "resolution", null, "FIXED") .setFieldChange(context, "status", "OPEN", "RESOLVED") + .setSendNotifications(true) .setComponentKey("struts:Action") .setProjectKey("struts"); DefaultIssueQueryResult queryResult = new DefaultIssueQueryResult(Arrays.asList(issue)); diff --git a/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java b/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java index ade4e76cbd2..63f7ac1483e 100644 --- a/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java +++ b/sonar-core/src/test/java/org/sonar/core/issue/IssueUpdaterTest.java @@ -39,6 +39,7 @@ public class IssueUpdaterTest { boolean updated = updater.assign(issue, "emmerik", context); assertThat(updated).isTrue(); assertThat(issue.assignee()).isEqualTo("emmerik"); + assertThat(issue.mustSendNotifications()).isTrue(); FieldDiffs.Diff diff = issue.currentChange().get("assignee"); assertThat(diff.oldValue()).isNull(); assertThat(diff.newValue()).isEqualTo("emmerik"); @@ -50,6 +51,7 @@ public class IssueUpdaterTest { boolean updated = updater.assign(issue, null, context); assertThat(updated).isTrue(); assertThat(issue.assignee()).isNull(); + assertThat(issue.mustSendNotifications()).isTrue(); FieldDiffs.Diff diff = issue.currentChange().get("assignee"); assertThat(diff.oldValue()).isEqualTo("morgan"); assertThat(diff.newValue()).isNull(); @@ -61,6 +63,7 @@ public class IssueUpdaterTest { boolean updated = updater.assign(issue, "emmerik", context); assertThat(updated).isTrue(); assertThat(issue.assignee()).isEqualTo("emmerik"); + assertThat(issue.mustSendNotifications()).isTrue(); FieldDiffs.Diff diff = issue.currentChange().get("assignee"); assertThat(diff.oldValue()).isEqualTo("morgan"); assertThat(diff.newValue()).isEqualTo("emmerik"); @@ -72,6 +75,7 @@ public class IssueUpdaterTest { boolean updated = updater.assign(issue, "morgan", context); assertThat(updated).isFalse(); assertThat(issue.currentChange()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @@ -81,6 +85,7 @@ public class IssueUpdaterTest { assertThat(updated).isTrue(); assertThat(issue.severity()).isEqualTo("BLOCKER"); assertThat(issue.manualSeverity()).isFalse(); + assertThat(issue.mustSendNotifications()).isFalse(); FieldDiffs.Diff diff = issue.currentChange().get("severity"); assertThat(diff.oldValue()).isNull(); @@ -93,6 +98,7 @@ public class IssueUpdaterTest { boolean updated = updater.setPastSeverity(issue, "INFO", context); assertThat(updated).isTrue(); assertThat(issue.severity()).isEqualTo("BLOCKER"); + assertThat(issue.mustSendNotifications()).isFalse(); FieldDiffs.Diff diff = issue.currentChange().get("severity"); assertThat(diff.oldValue()).isEqualTo("INFO"); @@ -106,6 +112,7 @@ public class IssueUpdaterTest { assertThat(updated).isTrue(); assertThat(issue.severity()).isEqualTo("MINOR"); + assertThat(issue.mustSendNotifications()).isFalse(); FieldDiffs.Diff diff = issue.currentChange().get("severity"); assertThat(diff.oldValue()).isEqualTo("BLOCKER"); assertThat(diff.newValue()).isEqualTo("MINOR"); @@ -116,6 +123,7 @@ public class IssueUpdaterTest { issue.setSeverity("MINOR"); boolean updated = updater.setSeverity(issue, "MINOR", context); assertThat(updated).isFalse(); + assertThat(issue.mustSendNotifications()).isFalse(); assertThat(issue.currentChange()).isNull(); } @@ -137,6 +145,7 @@ public class IssueUpdaterTest { assertThat(updated).isTrue(); assertThat(issue.severity()).isEqualTo("MINOR"); assertThat(issue.manualSeverity()).isTrue(); + assertThat(issue.mustSendNotifications()).isTrue(); FieldDiffs.Diff diff = issue.currentChange().get("severity"); assertThat(diff.oldValue()).isEqualTo("BLOCKER"); assertThat(diff.newValue()).isEqualTo("MINOR"); @@ -148,6 +157,7 @@ public class IssueUpdaterTest { boolean updated = updater.setManualSeverity(issue, "MINOR", context); assertThat(updated).isFalse(); assertThat(issue.currentChange()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -155,7 +165,7 @@ public class IssueUpdaterTest { boolean updated = updater.setLine(issue, 123); assertThat(updated).isTrue(); assertThat(issue.line()).isEqualTo(123); - + assertThat(issue.mustSendNotifications()).isFalse(); // do not save change assertThat(issue.currentChange()).isNull(); } @@ -166,6 +176,7 @@ public class IssueUpdaterTest { boolean updated = updater.setPastLine(issue, 123); assertThat(updated).isTrue(); assertThat(issue.line()).isEqualTo(42); + assertThat(issue.mustSendNotifications()).isFalse(); // do not save change assertThat(issue.currentChange()).isNull(); @@ -178,6 +189,7 @@ public class IssueUpdaterTest { assertThat(updated).isFalse(); assertThat(issue.line()).isEqualTo(123); assertThat(issue.currentChange()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -189,6 +201,7 @@ public class IssueUpdaterTest { FieldDiffs.Diff diff = issue.currentChange().get("resolution"); assertThat(diff.oldValue()).isNull(); assertThat(diff.newValue()).isEqualTo("OPEN"); + assertThat(issue.mustSendNotifications()).isTrue(); } @Test @@ -198,6 +211,7 @@ public class IssueUpdaterTest { assertThat(updated).isFalse(); assertThat(issue.resolution()).isEqualTo("FIXED"); assertThat(issue.currentChange()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -209,6 +223,7 @@ public class IssueUpdaterTest { FieldDiffs.Diff diff = issue.currentChange().get("status"); assertThat(diff.oldValue()).isNull(); assertThat(diff.newValue()).isEqualTo("OPEN"); + assertThat(issue.mustSendNotifications()).isTrue(); } @Test @@ -218,6 +233,7 @@ public class IssueUpdaterTest { assertThat(updated).isFalse(); assertThat(issue.status()).isEqualTo("CLOSED"); assertThat(issue.currentChange()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -228,6 +244,7 @@ public class IssueUpdaterTest { assertThat(issue.currentChange().diffs()).hasSize(1); assertThat(issue.currentChange().get("JIRA").oldValue()).isNull(); assertThat(issue.currentChange().get("JIRA").newValue()).isEqualTo("FOO-123"); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -239,6 +256,7 @@ public class IssueUpdaterTest { assertThat(issue.currentChange().diffs()).hasSize(1); assertThat(issue.currentChange().get("JIRA").oldValue()).isEqualTo("FOO-123"); assertThat(issue.currentChange().get("JIRA").newValue()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -246,6 +264,7 @@ public class IssueUpdaterTest { issue.setAttribute("JIRA", "FOO-123"); boolean updated = updater.setAttribute(issue, "JIRA", "FOO-123", context); assertThat(updated).isFalse(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -257,6 +276,7 @@ public class IssueUpdaterTest { FieldDiffs.Diff diff = issue.currentChange().get("actionPlanKey"); assertThat(diff.oldValue()).isNull(); assertThat(diff.newValue()).isEqualTo("ABCD"); + assertThat(issue.mustSendNotifications()).isTrue(); } @Test @@ -265,6 +285,7 @@ public class IssueUpdaterTest { assertThat(updated).isTrue(); assertThat(issue.isChanged()).isTrue(); assertThat(issue.effortToFix()).isEqualTo(3.14); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -274,6 +295,7 @@ public class IssueUpdaterTest { assertThat(updated).isFalse(); assertThat(issue.isChanged()).isFalse(); assertThat(issue.effortToFix()).isEqualTo(3.14); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -285,6 +307,7 @@ public class IssueUpdaterTest { // do not save change assertThat(issue.currentChange()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -293,6 +316,7 @@ public class IssueUpdaterTest { assertThat(updated).isTrue(); assertThat(issue.isChanged()).isTrue(); assertThat(issue.message()).isEqualTo("the message"); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -304,6 +328,7 @@ public class IssueUpdaterTest { // do not save change assertThat(issue.currentChange()).isNull(); + assertThat(issue.mustSendNotifications()).isFalse(); } @Test @@ -315,5 +340,6 @@ public class IssueUpdaterTest { FieldDiffs.Diff diff = issue.currentChange().get("author"); assertThat(diff.oldValue()).isNull(); assertThat(diff.newValue()).isEqualTo("eric"); + assertThat(issue.mustSendNotifications()).isFalse(); } } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java b/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java index 0a4ba0394e8..739ec339918 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/DefaultIssue.java @@ -91,6 +91,9 @@ public class DefaultIssue implements Issue { // true if some fields have been changed since the previous scan private boolean isChanged = false; + // true if notifications have to be sent + private boolean sendNotifications = false; + // Date when issue was loaded from db (only when isNew=false) private Date selectedAt; @@ -309,6 +312,15 @@ public class DefaultIssue implements Issue { return this; } + public boolean mustSendNotifications() { + return sendNotifications; + } + + public DefaultIssue setSendNotifications(boolean b) { + sendNotifications = b; + return this; + } + @CheckForNull public String attribute(String key) { return attributes == null ? null : attributes.get(key); -- 2.39.5