SONAR-3755 Improve IssuesWorkflowDecorator

3 files changed, 126 insertions, 60 deletions

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/IssuesWorkflowDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/IssuesWorkflowDecorator.java
index a10ed742763..c5507e1d54b 100644
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/IssuesWorkflowDecorator.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/IssuesWorkflowDecorator.java
@@ -73,13 +73,15 @@ public class IssuesWorkflowDecorator implements Decorator {
       issueTracking.track(resource, openIssues, (List) newIssues);
       updateIssues(newIssues);
 
-      addManualIssues(openIssues);
+      addManualIssuesAndCloseResolvedOnes(openIssues, resource);
       closeResolvedStandardIssues(openIssues, newIssues, resource);
-      closeResolvedManualIssues(openIssues, resource);
+      keepFalsePositiveIssues(openIssues, resource);
       reopenUnresolvedIssues(openIssues, resource);
-    }
-    if (ResourceUtils.isRootProject(resource)) {
-      closeIssuesOnDeletedResources(openIssues, resource);
+
+      if (ResourceUtils.isRootProject(resource)) {
+        // TODO
+//        closeIssuesOnDeletedResources(openIssues, resource);
+      }
     }
   }
 
@@ -89,38 +91,37 @@ public class IssuesWorkflowDecorator implements Decorator {
     }
   }
 
-  private void addManualIssues(Collection<IssueDto> openIssues) {
+  private void addManualIssuesAndCloseResolvedOnes(Collection<IssueDto> openIssues, Resource resource) {
     for (IssueDto openIssue : openIssues) {
       if (openIssue.isManualIssue()) {
-        DefaultIssue newIssue = new DefaultIssue();
-        moduleIssues.addOrUpdate(newIssue);
+        DefaultIssue issue = toIssue(openIssue, resource);
+        if (Issue.STATUS_RESOLVED.equals(issue.status())) {
+          close(issue);
+        }
+        moduleIssues.addOrUpdate(issue);
       }
     }
   }
 
-  /**
-   * Close issues that relate to resources that have been deleted or renamed.
-   */
-  private void closeIssuesOnDeletedResources(Collection<IssueDto> openIssues, Resource resource) {
-    for (IssueDto openIssue : openIssues) {
-      close(openIssue, resource);
-    }
-  }
+  private void closeResolvedStandardIssues(Collection<IssueDto> openIssues, Collection<Issue> issues, Resource resource) {
+    Set<String> issueKeys = Sets.newHashSet(Collections2.transform(issues, new IssueToKeyfunction()));
 
-  private void closeResolvedManualIssues(Collection<IssueDto> openIssues, Resource resource) {
     for (IssueDto openIssue : openIssues) {
-      if (openIssue.isManualIssue() && Issue.STATUS_RESOLVED.equals(openIssue.getStatus())) {
-        close(openIssue, resource);
+      if (!openIssue.isManualIssue() && !issueKeys.contains(openIssue.getUuid())) {
+        closeAndSave(openIssue, resource);
       }
     }
   }
 
-  private void closeResolvedStandardIssues(Collection<IssueDto> openIssues, Collection<Issue> issues, Resource resource) {
-    Set<String> issueKeys = Sets.newHashSet(Collections2.transform(issues, new IssueToKeyfunction()));
 
+  private void keepFalsePositiveIssues(Collection<IssueDto> openIssues, Resource resource) {
     for (IssueDto openIssue : openIssues) {
-      if (!openIssue.isManualIssue() && !issueKeys.contains(openIssue.getUuid())) {
-        close(openIssue, resource);
+      if (!openIssue.isManualIssue() && Issue.RESOLUTION_FALSE_POSITIVE.equals(openIssue.getResolution())) {
+        DefaultIssue issue = toIssue(openIssue, resource);
+        issue.setResolution(openIssue.getResolution());
+        issue.setStatus(openIssue.getStatus());
+        issue.setUpdatedAt(new Date());
+        moduleIssues.addOrUpdate(issue);
       }
     }
   }
@@ -129,19 +130,32 @@ public class IssuesWorkflowDecorator implements Decorator {
     for (IssueDto openIssue : openIssues) {
       if (Issue.STATUS_RESOLVED.equals(openIssue.getStatus()) && !Issue.RESOLUTION_FALSE_POSITIVE.equals(openIssue.getResolution())
           && !openIssue.isManualIssue()) {
-        reopen(openIssue, resource);
+        reopenAndSave(openIssue, resource);
       }
     }
   }
 
-  private void close(IssueDto openIssue, Resource resource) {
-    DefaultIssue issue = toIssue(openIssue, resource);
+  /**
+   * Close issues that relate to resources that have been deleted or renamed.
+   */
+  private void closeIssuesOnDeletedResources(Collection<IssueDto> openIssues, Resource resource) {
+    for (IssueDto openIssue : openIssues) {
+      closeAndSave(openIssue, resource);
+    }
+  }
+
+  private void close(DefaultIssue issue) {
     issue.setStatus(Issue.STATUS_CLOSED);
     issue.setUpdatedAt(new Date());
+  }
+
+  private void closeAndSave(IssueDto openIssue, Resource resource){
+    DefaultIssue issue = toIssue(openIssue, resource);
+    close(issue);
     moduleIssues.addOrUpdate(issue);
   }
 
-  private void reopen(IssueDto openIssue, Resource resource) {
+  private void reopenAndSave(IssueDto openIssue, Resource resource) {
     DefaultIssue issue = toIssue(openIssue, resource);
     issue.setStatus(Issue.STATUS_REOPENED);
     issue.setResolution(null);
@@ -166,6 +180,10 @@ public class IssuesWorkflowDecorator implements Decorator {
     issue.setClosedAt(dto.getClosedAt());
     issue.setAttributes(KeyValueFormat.parse(dto.getData()));
     issue.setComponentKey(resource.getKey());
+    issue.setManual(dto.isManualIssue());
+    issue.setManualSeverity(dto.isManualSeverity());
+
+    // TODO add person
 
     Rule rule = ruleFinder.findById(dto.getRuleId());
     issue.setRuleKey(rule.getKey());
diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/IssuesWorkflowDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/IssuesWorkflowDecoratorTest.java
index d6eb183d690..d73b79e7ecc 100644
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/IssuesWorkflowDecoratorTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/IssuesWorkflowDecoratorTest.java
@@ -19,18 +19,17 @@
  */
 package org.sonar.plugins.core.issue;
 
+import com.google.common.collect.Lists;
 import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
-import org.sonar.api.batch.DecoratorContext;
+import org.mockito.ArgumentCaptor;
 import org.sonar.api.issue.Issue;
-import org.sonar.api.resources.JavaFile;
 import org.sonar.api.resources.Project;
+import org.sonar.api.resources.Qualifiers;
 import org.sonar.api.resources.Resource;
 import org.sonar.api.rules.Rule;
 import org.sonar.api.rules.RuleFinder;
-import org.sonar.api.rules.Violation;
-import org.sonar.api.violations.ViolationQuery;
 import org.sonar.batch.issue.InitialOpenIssuesStack;
 import org.sonar.batch.issue.ModuleIssues;
 import org.sonar.core.issue.DefaultIssue;
@@ -38,13 +37,15 @@ import org.sonar.core.issue.IssueDto;
 import org.sonar.core.persistence.AbstractDaoTestCase;
 
 import java.util.Collections;
+import java.util.List;
 
 import static com.google.common.collect.Lists.newArrayList;
+import static org.fest.assertions.Assertions.assertThat;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Matchers.*;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
+import static org.mockito.Matchers.anyInt;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.*;
 
 public class IssuesWorkflowDecoratorTest extends AbstractDaoTestCase {
 
@@ -61,6 +62,7 @@ public class IssuesWorkflowDecoratorTest extends AbstractDaoTestCase {
     initialOpenIssuesStack = mock(InitialOpenIssuesStack.class);
     issueTracking = mock(IssueTracking.class);
     ruleFinder = mock(RuleFinder.class);
+    when(ruleFinder.findById(anyInt())).thenReturn(Rule.create());
 
     decorator = new IssuesWorkflowDecorator(moduleIssues, initialOpenIssuesStack, issueTracking, ruleFinder);
   }
@@ -80,45 +82,88 @@ public class IssuesWorkflowDecoratorTest extends AbstractDaoTestCase {
   }
 
   @Test
-  @Ignore
-  public void shouldCloseIssuesOnResolvedViolations() {
-    //setupData("shouldCloseReviewsOnResolvedViolations");
-
-    when(moduleIssues.issues(anyString())).thenReturn(newArrayList((Issue) new DefaultIssue().setKey("111").setComponentKey("key")));
-    when(initialOpenIssuesStack.selectAndRemove(anyInt())).thenReturn(newArrayList(new IssueDto().setUuid("111").setResourceId(1)));
+  public void should_close_resolved_issue() {
+    when(moduleIssues.issues(anyString())).thenReturn(Collections.<Issue>emptyList());
+    when(initialOpenIssuesStack.selectAndRemove(anyInt())).thenReturn(newArrayList(
+        new IssueDto().setUuid("100").setRuleId(10)));
 
-    Resource resource = new JavaFile("key");
-    decorator.decorate(resource, null);
+    decorator.decorate(mock(Resource.class), null);
 
-    //checkTables("shouldCloseReviewsOnResolvedViolations", new String[] {"updated_at"}, "reviews");
+    ArgumentCaptor<DefaultIssue> argument = ArgumentCaptor.forClass(DefaultIssue.class);
+    verify(moduleIssues).addOrUpdate(argument.capture());
+    assertThat(argument.getValue().status()).isEqualTo(Issue.STATUS_CLOSED);
+    assertThat(argument.getValue().updatedAt()).isNotNull();
   }
 
   @Test
-  @Ignore
-  public void shouldCloseResolvedManualViolations() {
-    setupData("shouldCloseResolvedManualViolations");
-    DecoratorContext context = mock(DecoratorContext.class);
-    when(context.getViolations(any(ViolationQuery.class))).thenReturn(Collections.<Violation>emptyList());
+  public void should_close_resolved_manual_issue() {
+    when(moduleIssues.issues(anyString())).thenReturn(Collections.<Issue>emptyList());
+    when(initialOpenIssuesStack.selectAndRemove(anyInt())).thenReturn(newArrayList(
+        new IssueDto().setUuid("100").setRuleId(1).setManualIssue(true).setStatus(Issue.STATUS_RESOLVED)));
 
-    Resource resource = new JavaFile("org.foo.Bar");
-    decorator.decorate(resource, context);
+    decorator.decorate(mock(Resource.class), null);
 
-    checkTables("shouldCloseResolvedManualViolations", new String[] {"updated_at"}, "reviews");
+    ArgumentCaptor<DefaultIssue> argument = ArgumentCaptor.forClass(DefaultIssue.class);
+    verify(moduleIssues).addOrUpdate(argument.capture());
+    assertThat(argument.getValue().status()).isEqualTo(Issue.STATUS_CLOSED);
+    assertThat(argument.getValue().updatedAt()).isNotNull();
+  }
+
+  @Test
+  public void should_reopen_unresolved_issue() {
+    when(moduleIssues.issues(anyString())).thenReturn(Lists.<Issue>newArrayList(
+        new DefaultIssue().setKey("100")));
+    when(initialOpenIssuesStack.selectAndRemove(anyInt())).thenReturn(newArrayList(
+        new IssueDto().setUuid("100").setRuleId(1).setStatus(Issue.STATUS_RESOLVED).setResolution(Issue.RESOLUTION_FIXED)));
+
+    decorator.decorate(mock(Resource.class), null);
+
+    ArgumentCaptor<DefaultIssue> argument = ArgumentCaptor.forClass(DefaultIssue.class);
+    verify(moduleIssues, times(2)).addOrUpdate(argument.capture());
+
+    List<DefaultIssue> capturedDefaultIssues = argument.getAllValues();
+    // First call is done when updating issues after calling issue tracking and we don't care
+    DefaultIssue defaultIssue = capturedDefaultIssues.get(1);
+    assertThat(defaultIssue.status()).isEqualTo(Issue.STATUS_REOPENED);
+    assertThat(defaultIssue.resolution()).isNull();
+    assertThat(defaultIssue.updatedAt()).isNotNull();
+  }
+
+  @Test
+  public void should_keep_false_positive_issue() {
+    when(moduleIssues.issues(anyString())).thenReturn(Lists.<Issue>newArrayList(
+        new DefaultIssue().setKey("100")));
+    when(initialOpenIssuesStack.selectAndRemove(anyInt())).thenReturn(newArrayList(
+        new IssueDto().setUuid("100").setRuleId(1).setStatus(Issue.STATUS_RESOLVED).setResolution(Issue.RESOLUTION_FALSE_POSITIVE)));
+
+    decorator.decorate(mock(Resource.class), null);
+
+    ArgumentCaptor<DefaultIssue> argument = ArgumentCaptor.forClass(DefaultIssue.class);
+    verify(moduleIssues, times(2)).addOrUpdate(argument.capture());
+
+    List<DefaultIssue> capturedDefaultIssues = argument.getAllValues();
+    // First call is done when updating issues after calling issue tracking and we don't care
+    DefaultIssue defaultIssue = capturedDefaultIssues.get(1);
+    assertThat(defaultIssue.status()).isEqualTo(Issue.STATUS_RESOLVED);
+    assertThat(defaultIssue.resolution()).isEqualTo(Issue.RESOLUTION_FALSE_POSITIVE);
+    assertThat(defaultIssue.updatedAt()).isNotNull();
   }
 
   @Test
   @Ignore
-  public void shouldReopenViolations() {
-    setupData("shouldReopenViolations");
-    DecoratorContext context = mock(DecoratorContext.class);
-    Violation violation = new Violation(new Rule());
-    violation.setPermanentId(1000);
-    when(context.getViolations(any(ViolationQuery.class))).thenReturn(newArrayList(violation));
+  public void should_close_remaining_open_issue_on_root_project() {
+    when(moduleIssues.issues(anyString())).thenReturn(Collections.<Issue>emptyList());
+    when(initialOpenIssuesStack.selectAndRemove(anyInt())).thenReturn(newArrayList(
+        new IssueDto().setUuid("100").setRuleId(1)));
 
-    Resource resource = new JavaFile("org.foo.Bar");
-    decorator.decorate(resource, context);
+    Resource resource = mock(Resource.class);
+    when(resource.getQualifier()).thenReturn(Qualifiers.PROJECT);
+    decorator.decorate(resource, null);
 
-    checkTables("shouldReopenViolations", new String[] {"updated_at"}, "reviews");
+    ArgumentCaptor<DefaultIssue> argument = ArgumentCaptor.forClass(DefaultIssue.class);
+    verify(moduleIssues, times(2)).addOrUpdate(argument.capture());
+    assertThat(argument.getValue().status()).isEqualTo(Issue.STATUS_CLOSED);
+    assertThat(argument.getValue().updatedAt()).isNotNull();
   }
 
 }
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFinder.java b/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFinder.java
index 12d965cfdef..3cc612cd3b0 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFinder.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFinder.java
@@ -145,6 +145,9 @@ public class DefaultIssueFinder implements IssueFinder {
     issue.setUpdatedAt(dto.getUpdatedAt());
     issue.setClosedAt(dto.getClosedAt());
     issue.setAttributes(KeyValueFormat.parse(dto.getData()));
+    issue.setManual(dto.isManualIssue());
+    issue.setManualSeverity(dto.isManualSeverity());
+
     if (resource != null) {
       issue.setComponentKey(resource.getKey());
     }