Fix closing of resovled manual issue

4 files changed, 204 insertions, 69 deletions

diff --git a/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java b/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java
index 5d5275f8502..cbd2c0661e0 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java
@@ -91,7 +91,7 @@ public class IssueWorkflow implements Startable {
         .functions(new SetResolution(null), new SetCloseDate(false))
         .build())
 
-      // resolve as false-positive
+    // resolve as false-positive
       .transition(Transition.builder(DefaultTransitions.FALSE_POSITIVE)
         .from(Issue.STATUS_OPEN).to(Issue.STATUS_RESOLVED)
         .functions(new SetResolution(Issue.RESOLUTION_FALSE_POSITIVE), UnsetAssignee.INSTANCE)
@@ -108,7 +108,7 @@ public class IssueWorkflow implements Startable {
         .requiredProjectPermission(UserRole.ISSUE_ADMIN)
         .build())
 
-      // resolve as won't fix
+    // resolve as won't fix
       .transition(Transition.builder(DefaultTransitions.WONT_FIX)
         .from(Issue.STATUS_OPEN).to(Issue.STATUS_RESOLVED)
         .functions(new SetResolution(Issue.RESOLUTION_WONT_FIX), UnsetAssignee.INSTANCE)
@@ -123,8 +123,7 @@ public class IssueWorkflow implements Startable {
         .from(Issue.STATUS_CONFIRMED).to(Issue.STATUS_RESOLVED)
         .functions(new SetResolution(Issue.RESOLUTION_WONT_FIX), UnsetAssignee.INSTANCE)
         .requiredProjectPermission(UserRole.ISSUE_ADMIN)
-        .build()
-      );
+        .build());
 
   }
 
@@ -151,26 +150,19 @@ public class IssueWorkflow implements Startable {
         .build())
       .transition(Transition.builder("automaticclose")
         .from(Issue.STATUS_RESOLVED).to(Issue.STATUS_CLOSED)
-        .conditions(IsBeingClosed.INSTANCE)
-        .functions(SetClosed.INSTANCE, new SetCloseDate(true))
-        .automatic()
-        .build())
-      .transition(Transition.builder("automaticclosemanual")
-        .from(Issue.STATUS_RESOLVED).to(Issue.STATUS_CLOSED)
-        .conditions(IsManual.INSTANCE)
+        .conditions(new OrCondition(IsBeingClosed.INSTANCE, IsManual.INSTANCE))
         .functions(SetClosed.INSTANCE, new SetCloseDate(true))
         .automatic()
         .build())
 
-      // Reopen issues that are marked as resolved but that are still alive.
-      // Manual issues are kept resolved.
+    // Reopen issues that are marked as resolved but that are still alive.
+    // Manual issues are kept resolved.
       .transition(Transition.builder("automaticreopen")
         .from(Issue.STATUS_RESOLVED).to(Issue.STATUS_REOPENED)
         .conditions(new NotCondition(IsBeingClosed.INSTANCE), new HasResolution(Issue.RESOLUTION_FIXED), new NotCondition(IsManual.INSTANCE))
         .functions(new SetResolution(null), new SetCloseDate(false))
         .automatic()
-        .build()
-      );
+        .build());
   }
 
   @Override
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/workflow/OrCondition.java b/sonar-core/src/main/java/org/sonar/core/issue/workflow/OrCondition.java
new file mode 100644
index 00000000000..8047892d71d
--- /dev/null
+++ b/sonar-core/src/main/java/org/sonar/core/issue/workflow/OrCondition.java
@@ -0,0 +1,42 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2014 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.core.issue.workflow;
+
+import org.sonar.api.issue.Issue;
+import org.sonar.api.issue.condition.Condition;
+
+public class OrCondition implements Condition {
+  private final Condition[] conditions;
+
+  public OrCondition(Condition... conditions) {
+    this.conditions = conditions;
+  }
+
+  @Override
+  public boolean matches(Issue issue) {
+    for (Condition condition : conditions) {
+      if (condition.matches(issue)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+}
diff --git a/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java b/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java
index 57a1d934b9b..1b59f3569b3 100644
--- a/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java
@@ -27,7 +27,6 @@ import java.util.Date;
 import java.util.List;
 import javax.annotation.Nullable;
 import org.apache.commons.lang.time.DateUtils;
-import org.assertj.core.api.Assertions;
 import org.junit.Test;
 import org.sonar.api.issue.DefaultTransitions;
 import org.sonar.api.issue.Issue;
@@ -39,6 +38,9 @@ import org.sonar.core.issue.IssueUpdater;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
+import static org.sonar.api.issue.Issue.RESOLUTION_FIXED;
+import static org.sonar.api.issue.Issue.STATUS_CLOSED;
+import static org.sonar.api.rule.RuleKey.MANUAL_REPOSITORY_KEY;
 
 public class IssueWorkflowTest {
 
@@ -52,7 +54,7 @@ public class IssueWorkflowTest {
     assertThat(workflow.machine()).isNotNull();
     assertThat(workflow.machine().state(Issue.STATUS_OPEN)).isNotNull();
     assertThat(workflow.machine().state(Issue.STATUS_CONFIRMED)).isNotNull();
-    assertThat(workflow.machine().state(Issue.STATUS_CLOSED)).isNotNull();
+    assertThat(workflow.machine().state(STATUS_CLOSED)).isNotNull();
     assertThat(workflow.machine().state(Issue.STATUS_REOPENED)).isNotNull();
     assertThat(workflow.machine().state(Issue.STATUS_RESOLVED)).isNotNull();
     workflow.stop();
@@ -62,7 +64,7 @@ public class IssueWorkflowTest {
   public void list_statuses() {
     workflow.start();
     // order is important for UI
-    assertThat(workflow.statusKeys()).containsSequence(Issue.STATUS_OPEN, Issue.STATUS_CONFIRMED, Issue.STATUS_REOPENED, Issue.STATUS_RESOLVED, Issue.STATUS_CLOSED);
+    assertThat(workflow.statusKeys()).containsSequence(Issue.STATUS_OPEN, Issue.STATUS_CONFIRMED, Issue.STATUS_REOPENED, Issue.STATUS_RESOLVED, STATUS_CLOSED);
   }
 
   @Test
@@ -105,7 +107,7 @@ public class IssueWorkflowTest {
   public void list_no_out_transition_from_status_closed() {
     workflow.start();
 
-    DefaultIssue issue = new DefaultIssue().setStatus(Issue.STATUS_CLOSED).setRuleKey(RuleKey.of("java", "R1  "));
+    DefaultIssue issue = new DefaultIssue().setStatus(STATUS_CLOSED).setRuleKey(RuleKey.of("java", "R1  "));
     List<Transition> transitions = workflow.outTransitions(issue);
     assertThat(transitions).isEmpty();
   }
@@ -117,7 +119,7 @@ public class IssueWorkflowTest {
     // Manual issue because of reporter
     DefaultIssue issue = new DefaultIssue()
       .setKey("ABCDE")
-      .setStatus(Issue.STATUS_CLOSED)
+      .setStatus(STATUS_CLOSED)
       .setRuleKey(RuleKey.of("manual", "Performance"))
       .setReporter("simon");
 
@@ -139,22 +141,22 @@ public class IssueWorkflowTest {
   }
 
   @Test
-  public void do_automatic_transition() {
+  public void automatically_close_resolved_issue() {
     workflow.start();
 
     DefaultIssue issue = new DefaultIssue()
       .setKey("ABCDE")
       .setRuleKey(RuleKey.of("js", "S001"))
-      .setResolution(Issue.RESOLUTION_FIXED)
+      .setResolution(RESOLUTION_FIXED)
       .setStatus(Issue.STATUS_RESOLVED)
       .setNew(false)
       .setBeingClosed(true);
     Date now = new Date();
     workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now));
-    Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED);
-    Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED);
-    Assertions.assertThat(issue.closeDate()).isNotNull();
-    Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
+    assertThat(issue.closeDate()).isNotNull();
+    assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
   }
 
   @Test
@@ -169,10 +171,10 @@ public class IssueWorkflowTest {
       .setBeingClosed(true);
     Date now = new Date();
     workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now));
-    Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED);
-    Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED);
-    Assertions.assertThat(issue.closeDate()).isNotNull();
-    Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
+    assertThat(issue.closeDate()).isNotNull();
+    assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
   }
 
   @Test
@@ -187,10 +189,10 @@ public class IssueWorkflowTest {
       .setBeingClosed(true);
     Date now = new Date();
     workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now));
-    Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED);
-    Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED);
-    Assertions.assertThat(issue.closeDate()).isNotNull();
-    Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
+    assertThat(issue.closeDate()).isNotNull();
+    assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
   }
 
   @Test
@@ -205,10 +207,10 @@ public class IssueWorkflowTest {
       .setBeingClosed(true);
     Date now = new Date();
     workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now));
-    Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED);
-    Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED);
-    Assertions.assertThat(issue.closeDate()).isNotNull();
-    Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
+    assertThat(issue.closeDate()).isNotNull();
+    assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
   }
 
   @Test
@@ -217,7 +219,7 @@ public class IssueWorkflowTest {
 
     DefaultIssue issue = new DefaultIssue()
       .setKey("ABCDE")
-      .setResolution(Issue.RESOLUTION_FIXED)
+      .setResolution(RESOLUTION_FIXED)
       .setStatus("xxx")
       .setNew(false)
       .setBeingClosed(true);
@@ -240,11 +242,11 @@ public class IssueWorkflowTest {
     workflow.start();
     workflow.doTransition(issue, DefaultTransitions.FALSE_POSITIVE, IssueChangeContext.createScan(new Date()));
 
-    Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FALSE_POSITIVE);
-    Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED);
+    assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FALSE_POSITIVE);
+    assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED);
 
     // should remove assignee
-    Assertions.assertThat(issue.assignee()).isNull();
+    assertThat(issue.assignee()).isNull();
   }
 
   @Test
@@ -258,19 +260,23 @@ public class IssueWorkflowTest {
     workflow.start();
     workflow.doTransition(issue, DefaultTransitions.WONT_FIX, IssueChangeContext.createScan(new Date()));
 
-    Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_WONT_FIX);
-    Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED);
+    assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_WONT_FIX);
+    assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED);
 
     // should remove assignee
-    Assertions.assertThat(issue.assignee()).isNull();
+    assertThat(issue.assignee()).isNull();
   }
 
+  /**
+   * User marks the manual issue as resolved -> issue is automatically
+   * closed.
+   */
   @Test
-  public void manual_issues_be_resolved_then_closed() {
+  public void automatically_close_resolved_manual_issue() {
     DefaultIssue issue = new DefaultIssue()
       .setKey("ABCDE")
       .setStatus(Issue.STATUS_OPEN)
-      .setRuleKey(RuleKey.of(RuleKey.MANUAL_REPOSITORY_KEY, "Performance"));
+      .setRuleKey(RuleKey.of(MANUAL_REPOSITORY_KEY, "Performance"));
 
     workflow.start();
 
@@ -278,20 +284,62 @@ public class IssueWorkflowTest {
       Transition.create("confirm", "OPEN", "CONFIRMED"),
       Transition.create("resolve", "OPEN", "RESOLVED"),
       Transition.create("falsepositive", "OPEN", "RESOLVED"),
-      Transition.create("wontfix", "OPEN", "RESOLVED")
-      );
+      Transition.create("wontfix", "OPEN", "RESOLVED"));
 
     workflow.doTransition(issue, "resolve", mock(IssueChangeContext.class));
-    Assertions.assertThat(issue.resolution()).isEqualTo("FIXED");
-    Assertions.assertThat(issue.status()).isEqualTo("RESOLVED");
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo("RESOLVED");
 
     assertThat(workflow.outTransitions(issue)).containsOnly(
-      Transition.create("reopen", "RESOLVED", "REOPENED")
-      );
+      Transition.create("reopen", "RESOLVED", "REOPENED"));
 
     workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class));
-    Assertions.assertThat(issue.resolution()).isEqualTo("FIXED");
-    Assertions.assertThat(issue.status()).isEqualTo("CLOSED");
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
+  }
+
+  /**
+   * Manual issue is fixed because the file does not exist anymore
+   * or the tracking engine did not find the associated code
+   * -> the issue is closed
+   */
+  @Test
+  public void automatically_close_manual_issue_on_deleted_code() {
+    DefaultIssue issue = new DefaultIssue()
+      .setKey("ABCDE")
+      .setStatus(Issue.STATUS_OPEN)
+      .setRuleKey(RuleKey.of(MANUAL_REPOSITORY_KEY, "Performance"))
+      .setBeingClosed(true);
+
+    workflow.start();
+
+    workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class));
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
+  }
+
+  /**
+   * Corner-case : the manual issue was marked as resolved by user but at the same 
+   * time the file or the associated line was deleted.
+   */
+  @Test
+  public void automatically_close_resolved_manual_issue_on_deleted_code() {
+    DefaultIssue issue = new DefaultIssue()
+      .setKey("ABCDE")
+      .setRuleKey(RuleKey.of(MANUAL_REPOSITORY_KEY, "Performance"))
+
+    // resolved by user
+      .setResolution(RESOLUTION_FIXED)
+      .setStatus(Issue.STATUS_RESOLVED)
+
+    // but unmatched by tracking engine
+      .setBeingClosed(true);
+
+    workflow.start();
+
+    workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class));
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
   }
 
   @Test
@@ -309,29 +357,27 @@ public class IssueWorkflowTest {
       Transition.create("confirm", "OPEN", "CONFIRMED"),
       Transition.create("resolve", "OPEN", "RESOLVED"),
       Transition.create("falsepositive", "OPEN", "RESOLVED"),
-      Transition.create("wontfix", "OPEN", "RESOLVED")
-      );
+      Transition.create("wontfix", "OPEN", "RESOLVED"));
 
     workflow.doTransition(issue, "confirm", mock(IssueChangeContext.class));
-    Assertions.assertThat(issue.resolution()).isNull();
-    Assertions.assertThat(issue.status()).isEqualTo("CONFIRMED");
+    assertThat(issue.resolution()).isNull();
+    assertThat(issue.status()).isEqualTo("CONFIRMED");
 
     assertThat(workflow.outTransitions(issue)).containsOnly(
       Transition.create("unconfirm", "CONFIRMED", "REOPENED"),
       Transition.create("resolve", "CONFIRMED", "RESOLVED"),
       Transition.create("falsepositive", "CONFIRMED", "RESOLVED"),
-      Transition.create("wontfix", "CONFIRMED", "RESOLVED")
-      );
+      Transition.create("wontfix", "CONFIRMED", "RESOLVED"));
 
     // keep confirmed and unresolved
     workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class));
-    Assertions.assertThat(issue.resolution()).isNull();
-    Assertions.assertThat(issue.status()).isEqualTo("CONFIRMED");
+    assertThat(issue.resolution()).isNull();
+    assertThat(issue.status()).isEqualTo("CONFIRMED");
 
     // unconfirm
     workflow.doTransition(issue, "unconfirm", mock(IssueChangeContext.class));
-    Assertions.assertThat(issue.resolution()).isNull();
-    Assertions.assertThat(issue.status()).isEqualTo("REOPENED");
+    assertThat(issue.resolution()).isNull();
+    assertThat(issue.status()).isEqualTo("REOPENED");
   }
 
   @Test
@@ -348,8 +394,8 @@ public class IssueWorkflowTest {
     workflow.start();
 
     workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class));
-    Assertions.assertThat(issue.resolution()).isEqualTo("REMOVED");
-    Assertions.assertThat(issue.status()).isEqualTo("CLOSED");
+    assertThat(issue.resolution()).isEqualTo("REMOVED");
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
   }
 
   @Test
@@ -366,8 +412,8 @@ public class IssueWorkflowTest {
     workflow.start();
 
     workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class));
-    Assertions.assertThat(issue.resolution()).isEqualTo("FIXED");
-    Assertions.assertThat(issue.status()).isEqualTo("CLOSED");
+    assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED);
+    assertThat(issue.status()).isEqualTo(STATUS_CLOSED);
   }
 
   private Collection<String> keys(List<Transition> transitions) {
diff --git a/sonar-core/src/test/java/org/sonar/core/issue/workflow/OrConditionTest.java b/sonar-core/src/test/java/org/sonar/core/issue/workflow/OrConditionTest.java
new file mode 100644
index 00000000000..83763cbb098
--- /dev/null
+++ b/sonar-core/src/test/java/org/sonar/core/issue/workflow/OrConditionTest.java
@@ -0,0 +1,55 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2014 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.core.issue.workflow;
+
+import org.junit.Test;
+import org.sonar.api.issue.Issue;
+import org.sonar.api.issue.condition.Condition;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+
+public class OrConditionTest {
+
+  private static final Condition TRUE_CONDITION = new BooleanCondition(true);
+  private static final Condition FALSE_CONDITION = new BooleanCondition(false);
+  Issue issue = mock(Issue.class);
+
+  @Test
+  public void match() {
+    assertThat(new OrCondition(TRUE_CONDITION).matches(issue)).isTrue();
+    assertThat(new OrCondition(FALSE_CONDITION).matches(issue)).isFalse();
+    assertThat(new OrCondition(FALSE_CONDITION, TRUE_CONDITION).matches(issue)).isTrue();
+    assertThat(new OrCondition(FALSE_CONDITION, FALSE_CONDITION).matches(issue)).isFalse();
+  }
+
+  private static class BooleanCondition implements Condition {
+    private final boolean b;
+
+    public BooleanCondition(boolean b) {
+      this.b = b;
+    }
+
+    @Override
+    public boolean matches(Issue issue) {
+      return b;
+    }
+  }
+}