SONAR-8368 restore resolution when reopening closed issues

6 files changed, 227 insertions, 30 deletions

diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java
index 052acd43f9a..95694c99809 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/IssueWorkflow.java
@@ -250,7 +250,7 @@ public class IssueWorkflow implements Startable {
           new PreviousStatusWas(Issue.STATUS_OPEN),
           new HasResolution(Issue.RESOLUTION_REMOVED, Issue.RESOLUTION_FIXED),
           IsNotHotspotNorManualVulnerability.INSTANCE)
-        .functions(UnsetCloseDate.INSTANCE)
+        .functions(RestoreResolutionFunction.INSTANCE, UnsetCloseDate.INSTANCE)
         .automatic()
         .build())
       .transition(Transition.builder("automaticunclosereopen")
@@ -259,7 +259,7 @@ public class IssueWorkflow implements Startable {
           new PreviousStatusWas(Issue.STATUS_REOPENED),
           new HasResolution(Issue.RESOLUTION_REMOVED, Issue.RESOLUTION_FIXED),
           IsNotHotspotNorManualVulnerability.INSTANCE)
-        .functions(UnsetCloseDate.INSTANCE)
+        .functions(RestoreResolutionFunction.INSTANCE, UnsetCloseDate.INSTANCE)
         .automatic()
         .build())
       .transition(Transition.builder("automaticuncloseconfirmed")
@@ -268,7 +268,7 @@ public class IssueWorkflow implements Startable {
           new PreviousStatusWas(Issue.STATUS_CONFIRMED),
           new HasResolution(Issue.RESOLUTION_REMOVED, Issue.RESOLUTION_FIXED),
           IsNotHotspotNorManualVulnerability.INSTANCE)
-        .functions(UnsetCloseDate.INSTANCE)
+        .functions(RestoreResolutionFunction.INSTANCE, UnsetCloseDate.INSTANCE)
         .automatic()
         .build())
       .transition(Transition.builder("automaticuncloseresolved")
@@ -277,7 +277,7 @@ public class IssueWorkflow implements Startable {
           new PreviousStatusWas(Issue.STATUS_RESOLVED),
           new HasResolution(Issue.RESOLUTION_REMOVED, Issue.RESOLUTION_FIXED),
           IsNotHotspotNorManualVulnerability.INSTANCE)
-        .functions(UnsetCloseDate.INSTANCE)
+        .functions(RestoreResolutionFunction.INSTANCE, UnsetCloseDate.INSTANCE)
         .automatic()
         .build())
 
diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/PreviousStatusWas.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/PreviousStatusWas.java
index e3fec3623ae..80cf70cf8e7 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/PreviousStatusWas.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/PreviousStatusWas.java
@@ -39,6 +39,7 @@ class PreviousStatusWas implements Condition {
     Optional<String> lastPreviousStatus = defaultIssue.changes().stream()
       // exclude current change (if any)
       .filter(change -> change != defaultIssue.currentChange())
+      .filter(change -> change.creationDate() != null)
       .sorted(Comparator.comparing(FieldDiffs::creationDate).reversed())
       .map(change -> change.get("status"))
       .filter(Objects::nonNull)
diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/RestoreResolutionFunction.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/RestoreResolutionFunction.java
new file mode 100644
index 00000000000..77196c26cf6
--- /dev/null
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/RestoreResolutionFunction.java
@@ -0,0 +1,86 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program 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.
+ *
+ * This program 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.server.issue.workflow;
+
+import java.util.Comparator;
+import java.util.Objects;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
+import org.sonar.api.issue.Issue;
+import org.sonar.core.issue.DefaultIssue;
+import org.sonar.core.issue.FieldDiffs;
+
+enum RestoreResolutionFunction implements Function {
+  INSTANCE;
+
+  @Override
+  public void execute(Context context) {
+    DefaultIssue defaultIssue = (DefaultIssue) context.issue();
+    String previousResolution = defaultIssue.changes().stream()
+      // exclude current change (if any)
+      .filter(change -> change != defaultIssue.currentChange())
+      .filter(change -> change.creationDate() != null)
+      .sorted(Comparator.comparing(FieldDiffs::creationDate).reversed())
+      .map(this::parse)
+      .filter(Objects::nonNull)
+      .filter(StatusAndResolutionDiffs::hasResolution)
+      .findFirst()
+      .map(t -> t.newStatusClosed ? t.oldResolution : t.newResolution)
+      .orElse(null);
+    context.setResolution(previousResolution);
+  }
+
+  @CheckForNull
+  private StatusAndResolutionDiffs parse(FieldDiffs fieldDiffs) {
+    FieldDiffs.Diff status = fieldDiffs.get("status");
+    if (status == null) {
+      return null;
+    }
+    FieldDiffs.Diff resolution = fieldDiffs.get("resolution");
+    if (resolution == null) {
+      return new StatusAndResolutionDiffs(Issue.STATUS_CLOSED.equals(status.newValue()), null, null);
+    }
+    return new StatusAndResolutionDiffs(Issue.STATUS_CLOSED.equals(status.newValue()), (String) resolution.oldValue(), (String) resolution.newValue());
+  }
+
+  private static class StatusAndResolutionDiffs {
+    private final boolean newStatusClosed;
+    private final String oldResolution;
+    private final String newResolution;
+
+    private StatusAndResolutionDiffs(boolean newStatusClosed, @Nullable String oldResolution, @Nullable String newResolution) {
+      this.newStatusClosed = newStatusClosed;
+      this.oldResolution = emptyToNull(oldResolution);
+      this.newResolution = emptyToNull(newResolution);
+    }
+
+    private static String emptyToNull(@Nullable String str) {
+      if (str == null || str.isEmpty()) {
+        return null;
+      }
+      return str;
+    }
+
+    boolean hasResolution() {
+      return oldResolution != null || newResolution != null;
+    }
+  }
+
+}
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java
index c848d059559..3bc8b6283c8 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java
@@ -29,6 +29,7 @@ import java.util.Calendar;
 import java.util.Collection;
 import java.util.Date;
 import java.util.List;
+import java.util.Random;
 import javax.annotation.Nullable;
 import org.apache.commons.lang.time.DateUtils;
 import org.junit.Test;
@@ -41,6 +42,7 @@ import org.sonar.core.issue.FieldDiffs;
 import org.sonar.core.issue.IssueChangeContext;
 import org.sonar.server.issue.IssueFieldsSetter;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static org.apache.commons.lang.time.DateUtils.addDays;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.fail;
@@ -57,8 +59,8 @@ import static org.sonar.api.issue.Issue.STATUS_RESOLVED;
 @RunWith(DataProviderRunner.class)
 public class IssueWorkflowTest {
 
-  IssueFieldsSetter updater = new IssueFieldsSetter();
-  IssueWorkflow workflow = new IssueWorkflow(new FunctionExecutor(updater), updater);
+  private IssueFieldsSetter updater = new IssueFieldsSetter();
+  private IssueWorkflow workflow = new IssueWorkflow(new FunctionExecutor(updater), updater);
 
   @Test
   public void init_state_machine() {
@@ -189,7 +191,7 @@ public class IssueWorkflowTest {
         Date now = new Date();
         addStatusChange(issue, addDays(now, -60), STATUS_OPEN, STATUS_CONFIRMED);
         addStatusChange(issue, addDays(now, -10), STATUS_CONFIRMED, previousStatus);
-        addStatusChange(issue, now, previousStatus, STATUS_CLOSED);
+        setStatusPreviousToClosed(issue, previousStatus);
         return issue;
       })
       .toArray(DefaultIssue[]::new);
@@ -206,23 +208,81 @@ public class IssueWorkflowTest {
     });
   }
 
-  @DataProvider
-  public static Object[][] allStatusesLeadingToClosed() {
-    return new Object[][] {
-      {STATUS_OPEN},
-      {STATUS_REOPENED},
-      {STATUS_CONFIRMED},
-      {STATUS_RESOLVED}
-    };
+  @Test
+  @UseDataProvider("allResolutionsBeforeClosing")
+  public void automatically_reopen_closed_issue_to_previous_resolution_from_changelog(String resolutionBeforeClosed) {
+    String randomPreviousStatus = ALL_STATUSES_LEADING_TO_CLOSED[new Random().nextInt(ALL_STATUSES_LEADING_TO_CLOSED.length)];
+    DefaultIssue[] issues = Arrays.stream(SUPPORTED_RESOLUTIONS_FOR_UNCLOSING)
+      .map(resolution -> {
+        DefaultIssue issue = newClosedIssue(resolution);
+        addResolutionAndStatusChange(issue, new Date(), randomPreviousStatus, STATUS_CLOSED, resolutionBeforeClosed, resolution);
+        return issue;
+      })
+      .toArray(DefaultIssue[]::new);
+    Date now = new Date();
+    workflow.start();
+
+    Arrays.stream(issues).forEach(issue -> {
+      workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now));
+
+      assertThat(issue.status()).isEqualTo(randomPreviousStatus);
+      assertThat(issue.resolution()).isEqualTo(resolutionBeforeClosed);
+      assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
+      assertThat(issue.closeDate()).isNull();
+      assertThat(issue.isChanged()).isTrue();
+    });
   }
 
-  private static final String[] SUPPORTED_RESOLUTIONS_FOR_UNCLOSING = new String[] {RESOLUTION_FIXED, RESOLUTION_REMOVED};
+  @Test
+  public void automatically_reopen_closed_issue_to_no_resolution_if_no_previous_one_changelog() {
+    String randomPreviousStatus = ALL_STATUSES_LEADING_TO_CLOSED[new Random().nextInt(ALL_STATUSES_LEADING_TO_CLOSED.length)];
+    DefaultIssue[] issues = Arrays.stream(SUPPORTED_RESOLUTIONS_FOR_UNCLOSING)
+      .map(resolution -> {
+        DefaultIssue issue = newClosedIssue(resolution);
+        setStatusPreviousToClosed(issue, randomPreviousStatus);
+        return issue;
+      })
+      .toArray(DefaultIssue[]::new);
+    Date now = new Date();
+    workflow.start();
 
-  @DataProvider
-  public static Object[][] supportedResolutionsForUnClosing() {
-    return Arrays.stream(SUPPORTED_RESOLUTIONS_FOR_UNCLOSING)
-      .map(t -> new Object[] {t})
-      .toArray(Object[][]::new);
+    Arrays.stream(issues).forEach(issue -> {
+      workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now));
+
+      assertThat(issue.status()).isEqualTo(randomPreviousStatus);
+      assertThat(issue.resolution()).isNull();
+      assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
+      assertThat(issue.closeDate()).isNull();
+      assertThat(issue.isChanged()).isTrue();
+    });
+  }
+
+  @Test
+  @UseDataProvider("allResolutionsBeforeClosing")
+  public void automatically_reopen_closed_issue_to_previous_resolution_of_closing_the_issue_if_most_recent_of_all_resolution_changes(String resolutionBeforeClosed) {
+    String randomPreviousStatus = ALL_STATUSES_LEADING_TO_CLOSED[new Random().nextInt(ALL_STATUSES_LEADING_TO_CLOSED.length)];
+    DefaultIssue[] issues = Arrays.stream(SUPPORTED_RESOLUTIONS_FOR_UNCLOSING)
+      .map(resolution -> {
+        DefaultIssue issue = newClosedIssue(resolution);
+        Date now = new Date();
+        addResolutionChange(issue, addDays(now, -60), null, RESOLUTION_FALSE_POSITIVE);
+        addResolutionChange(issue, addDays(now, -10), RESOLUTION_FALSE_POSITIVE, resolutionBeforeClosed);
+        addResolutionAndStatusChange(issue, now, randomPreviousStatus, STATUS_CLOSED, resolutionBeforeClosed, resolution);
+        return issue;
+      })
+      .toArray(DefaultIssue[]::new);
+    Date now = new Date();
+    workflow.start();
+
+    Arrays.stream(issues).forEach(issue -> {
+      workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now));
+
+      assertThat(issue.status()).isEqualTo(randomPreviousStatus);
+      assertThat(issue.resolution()).isEqualTo(resolutionBeforeClosed);
+      assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND));
+      assertThat(issue.closeDate()).isNull();
+      assertThat(issue.isChanged()).isTrue();
+    });
   }
 
   @Test
@@ -285,15 +345,37 @@ public class IssueWorkflowTest {
     });
   }
 
+  private static final String[] ALL_STATUSES_LEADING_TO_CLOSED = new String[] {STATUS_OPEN, STATUS_REOPENED, STATUS_CONFIRMED, STATUS_RESOLVED};
+  private static final String[] ALL_RESOLUTIONS_BEFORE_CLOSING = new String[] {
+    null,
+    RESOLUTION_FIXED,
+    RESOLUTION_WONT_FIX,
+    RESOLUTION_FALSE_POSITIVE
+  };
+  private static final String[] SUPPORTED_RESOLUTIONS_FOR_UNCLOSING = new String[] {RESOLUTION_FIXED, RESOLUTION_REMOVED};
+
+  @DataProvider
+  public static Object[][] allStatusesLeadingToClosed() {
+    return Arrays.stream(ALL_STATUSES_LEADING_TO_CLOSED)
+      .map(t -> new Object[] {t})
+      .toArray(Object[][]::new);
+  }
+
+  @DataProvider
+  public static Object[][] allResolutionsBeforeClosing() {
+    return Arrays.stream(ALL_RESOLUTIONS_BEFORE_CLOSING)
+      .map(t -> new Object[] {t})
+      .toArray(Object[][]::new);
+  }
+
   private static DefaultIssue newClosedIssue(String resolution) {
-    DefaultIssue res = new DefaultIssue()
+    return new DefaultIssue()
       .setKey("ABCDE")
       .setRuleKey(RuleKey.of("js", "S001"))
       .setResolution(resolution)
       .setStatus(STATUS_CLOSED)
       .setNew(false)
       .setCloseDate(new Date(5_999_999L));
-    return res;
   }
 
   private static void setStatusPreviousToClosed(DefaultIssue issue, String previousStatus) {
@@ -304,6 +386,30 @@ public class IssueWorkflowTest {
     issue.addChange(new FieldDiffs().setCreationDate(date).setDiff("status", previousStatus, newStatus));
   }
 
+  private void addResolutionChange(DefaultIssue issue, Date creationDate,
+    @Nullable String previousResolution, @Nullable String newResolution) {
+    checkArgument(previousResolution != null || newResolution != null, "At least one resolution must be non null");
+
+    FieldDiffs fieldDiffs = new FieldDiffs().setCreationDate(creationDate)
+      .setDiff("resolution", emptyIfNull(previousResolution), emptyIfNull(newResolution));
+    issue.addChange(fieldDiffs);
+  }
+
+  private void addResolutionAndStatusChange(DefaultIssue issue, Date creationDate,
+    String previousStatus, String newStatus,
+    @Nullable String previousResolution, @Nullable String newResolution) {
+    checkArgument(previousResolution != null || newResolution != null, "At least one resolution must be non null");
+
+    FieldDiffs fieldDiffs = new FieldDiffs().setCreationDate(creationDate)
+      .setDiff("status", previousStatus, newStatus)
+      .setDiff("resolution", emptyIfNull(previousResolution), emptyIfNull(newResolution));
+    issue.addChange(fieldDiffs);
+  }
+
+  private static String emptyIfNull(@Nullable String newResolution) {
+    return newResolution == null ? "" : newResolution;
+  }
+
   @Test
   public void close_open_dead_issue() {
     workflow.start();
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java b/sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java
index 47b6f194dd1..34e764aa298 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java
@@ -183,11 +183,11 @@ public class FieldDiffs implements Serializable {
     public String toString() {
       // TODO escape , and | characters
       StringBuilder sb = new StringBuilder();
+      if (oldValue != null) {
+        sb.append(oldValue.toString());
+        sb.append('|');
+      }
       if (newValue != null) {
-        if (oldValue != null) {
-          sb.append(oldValue.toString());
-          sb.append('|');
-        }
         sb.append(newValue.toString());
       }
       return sb.toString();
diff --git a/sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java b/sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java
index 962dcae300e..b4bab8d0491 100644
--- a/sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java
@@ -104,13 +104,13 @@ public class FieldDiffsTest {
     diffs.setDiff("severity", null, "INFO");
     diffs.setDiff("assignee", "user1", null);
 
-    assertThat(diffs.toString()).isEqualTo("severity=INFO,assignee=");
+    assertThat(diffs.toString()).isEqualTo("severity=INFO,assignee=user1|");
   }
 
   @Test
   public void test_parse() {
-    diffs = FieldDiffs.parse("severity=BLOCKER|INFO,resolution=OPEN|FIXED,donut=|new,acme=old|");
-    assertThat(diffs.diffs()).hasSize(4);
+    diffs = FieldDiffs.parse("severity=BLOCKER|INFO,resolution=OPEN|FIXED,donut=|new,gambas=miam,acme=old|");
+    assertThat(diffs.diffs()).hasSize(5);
 
     FieldDiffs.Diff diff = diffs.diffs().get("severity");
     assertThat(diff.oldValue()).isEqualTo("BLOCKER");
@@ -124,6 +124,10 @@ public class FieldDiffsTest {
     assertThat(diff.oldValue()).isEqualTo("");
     assertThat(diff.newValue()).isEqualTo("new");
 
+    diff = diffs.diffs().get("gambas");
+    assertThat(diff.oldValue()).isEqualTo("");
+    assertThat(diff.newValue()).isEqualTo("miam");
+
     diff = diffs.diffs().get("acme");
     assertThat(diff.oldValue()).isEqualTo("old");
     assertThat(diff.newValue()).isEqualTo("");