SONAR-4776 Improve calculation of new technical debt from changelog

4 files changed, 122 insertions, 51 deletions

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecorator.java
index ba1b92337b5..eee453e9560 100644
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecorator.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecorator.java
@@ -20,7 +20,9 @@
 
 package org.sonar.plugins.core.technicaldebt;
 
+import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Ordering;
 import org.apache.commons.lang.time.DateUtils;
 import org.sonar.api.batch.*;
 import org.sonar.api.component.ResourcePerspectives;
@@ -42,10 +44,10 @@ import org.sonar.core.technicaldebt.TechnicalDebtConverter;
 
 import javax.annotation.Nullable;
 
-import java.io.Serializable;
 import java.util.*;
 
 import static com.google.common.collect.Lists.newArrayList;
+import static com.google.common.collect.Maps.newLinkedHashMap;
 
 /**
  * Decorator that computes the technical debt metric
@@ -85,7 +87,7 @@ public final class NewTechnicalDebtDecorator implements Decorator {
   private void saveMeasures(DecoratorContext context, Collection<Issue> issues) {
     Measure measure = new Measure(CoreMetrics.NEW_TECHNICAL_DEBT);
     for (Period period : timeMachineConfiguration.periods()) {
-      Date periodDate = period.getDate() != null ? DateUtils.addSeconds(period.getDate(), 1) : null;
+      Date periodDate = period.getDate();
       double value = calculateNewTechnicalDebtValue(issues, periodDate);
       Collection<Measure> children = context.getChildrenMeasures(measure.getMetric());
       double sum = MeasureUtils.sumOnVariation(true, period.getIndex(), children) + value;
@@ -98,31 +100,23 @@ public final class NewTechnicalDebtDecorator implements Decorator {
     double value = 0;
     for (Issue issue : issues) {
       WorkDayDuration currentTechnicalDebt = ((DefaultIssue) issue).technicalDebt();
-      List<FieldDiffs> technicalDebtChangelog = changesOnField(IssueUpdater.TECHNICAL_DEBT, ((DefaultIssue) issue).changes());
-      if (technicalDebtChangelog.isEmpty()) {
-        if (isAfter(issue.creationDate(), periodDate)) {
-          value += technicalDebtConverter.toDays(currentTechnicalDebt);
-        }
+
+      Date periodDatePlusOneSecond = periodDate != null ? DateUtils.addSeconds(periodDate, 1) : null;
+      if (isAfter(issue.creationDate(), periodDatePlusOneSecond)) {
+        value += technicalDebtConverter.toDays(currentTechnicalDebt);
       } else {
-        value += calculateNewTechnicalDebtValueFromChangelog(currentTechnicalDebt, technicalDebtChangelog, periodDate);
+        value += calculateNewTechnicalDebtValueFromChangelog(currentTechnicalDebt, issue, periodDate);
       }
     }
     return value;
   }
 
-  private double calculateNewTechnicalDebtValueFromChangelog(WorkDayDuration currentTechnicalDebt, List<FieldDiffs> technicalDebtChangelog, Date periodDate) {
+  private double calculateNewTechnicalDebtValueFromChangelog(WorkDayDuration currentTechnicalDebt, Issue issue, Date periodDate) {
     double currentTechnicalDebtValue = technicalDebtConverter.toDays(currentTechnicalDebt);
 
-    // Changelog have to be sorted from oldest to newest to catch oldest value just before the period date
-    Collections.sort(technicalDebtChangelog, new Comparator<FieldDiffs>() {
-      @Override
-      public int compare(FieldDiffs fieldDiffs, FieldDiffs fieldDiffs2) {
-        return fieldDiffs.createdAt().compareTo(fieldDiffs2.createdAt());
-      }
-    });
-    for (FieldDiffs fieldDiffs : technicalDebtChangelog) {
-      if (isAfter(fieldDiffs.createdAt(), periodDate)) {
-        WorkDayDuration pastTechnicalDebt = newValue(IssueUpdater.TECHNICAL_DEBT, fieldDiffs);
+    for (Map.Entry<Date, WorkDayDuration> history : technicalDebtHistory(issue).entrySet()) {
+      if (isAfterOrEqual(history.getKey(), periodDate)) {
+        WorkDayDuration pastTechnicalDebt = history.getValue();
         double pastTechnicalDebtValue = technicalDebtConverter.toDays(pastTechnicalDebt);
         return currentTechnicalDebtValue - pastTechnicalDebtValue;
       }
@@ -130,28 +124,65 @@ public final class NewTechnicalDebtDecorator implements Decorator {
     return 0d;
   }
 
-  private List<FieldDiffs> changesOnField(final String field, Collection<FieldDiffs> fieldDiffs) {
+  private Map<Date, WorkDayDuration> technicalDebtHistory(Issue issue) {
+    Map<Date, WorkDayDuration> technicalDebtHistory = newLinkedHashMap();
+    List<FieldDiffs> technicalDebtChangelog = changesOnField(((DefaultIssue) issue).changes());
+
+    if (!technicalDebtChangelog.isEmpty()) {
+      // Changelog have to be sorted from oldest to newest to catch oldest value just before the period date. Null date should be the latest as this happen
+      // when technical debt has changed since previous analysis.
+      Ordering<FieldDiffs> ordering = Ordering.natural().nullsLast().onResultOf(new Function<FieldDiffs, Date>() {
+        public Date apply(FieldDiffs diff) {
+          return diff.createdAt();
+        }
+      });
+      List<FieldDiffs> technicalDebtChangelogSorted = ordering.immutableSortedCopy(technicalDebtChangelog);
+
+      technicalDebtHistory.put(issue.creationDate(), oldValue(technicalDebtChangelogSorted.iterator().next()));
+      for (FieldDiffs fieldDiffs : technicalDebtChangelogSorted) {
+        technicalDebtHistory.put(fieldDiffs.createdAt(), newValue(fieldDiffs));
+      }
+
+    }
+    return technicalDebtHistory;
+  }
+
+  private List<FieldDiffs> changesOnField(Collection<FieldDiffs> fieldDiffs) {
     List<FieldDiffs> diffs = newArrayList();
     for (FieldDiffs fieldDiff : fieldDiffs) {
-      if (fieldDiff.diffs().containsKey(field)) {
+      if (fieldDiff.diffs().containsKey(IssueUpdater.TECHNICAL_DEBT)) {
         diffs.add(fieldDiff);
       }
     }
     return diffs;
   }
 
-  private WorkDayDuration newValue(final String field, FieldDiffs fieldDiffs) {
+  private WorkDayDuration newValue(FieldDiffs fieldDiffs) {
     for (Map.Entry<String, FieldDiffs.Diff> entry : fieldDiffs.diffs().entrySet()) {
-      if (entry.getKey().equals(field)) {
-        Serializable newValue = entry.getValue().newValue();
-        return newValue != null && !"".equals(newValue) ? (WorkDayDuration.fromLong(Long.parseLong((String) newValue))) : null;
+      if (entry.getKey().equals(IssueUpdater.TECHNICAL_DEBT)) {
+        Long newValue = entry.getValue().newValueLong();
+        return newValue != null ? WorkDayDuration.fromLong(newValue) : null;
       }
     }
     return null;
   }
 
-  private boolean isAfter(Date currentDate, @Nullable Date pastDate) {
-    return pastDate == null || (currentDate != null && DateUtils.truncatedCompareTo(currentDate, pastDate, Calendar.SECOND) > 0);
+  private WorkDayDuration oldValue(FieldDiffs fieldDiffs) {
+    for (Map.Entry<String, FieldDiffs.Diff> entry : fieldDiffs.diffs().entrySet()) {
+      if (entry.getKey().equals(IssueUpdater.TECHNICAL_DEBT)) {
+        Long value = entry.getValue().oldValueLong();
+        return value != null ? WorkDayDuration.fromLong(value) : null;
+      }
+    }
+    return null;
+  }
+
+  private boolean isAfter(@Nullable Date currentDate, @Nullable Date pastDate) {
+    return pastDate == null || (currentDate!= null && DateUtils.truncatedCompareTo(currentDate, pastDate, Calendar.SECOND) > 0);
+  }
+
+  private boolean isAfterOrEqual(@Nullable Date currentDate, @Nullable Date pastDate) {
+    return currentDate == null || pastDate == null || (DateUtils.truncatedCompareTo(currentDate, pastDate, Calendar.SECOND) >= 0);
   }
 
   private boolean shouldSaveNewMetrics(DecoratorContext context) {
diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecoratorTest.java
index 4bd4521b608..f985fa7fbec 100644
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecoratorTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/NewTechnicalDebtDecoratorTest.java
@@ -116,10 +116,11 @@ public class NewTechnicalDebtDecoratorTest {
   }
 
   @Test
-  public void save_on_one_issue_with_one_changelog() {
-    Issue issue = new DefaultIssue().setKey("A").setCreationDate(fiveDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
+  public void save_on_one_issue_with_one_new_changelog() {
+    Issue issue = new DefaultIssue().setKey("A").setCreationDate(tenDaysAgo).setTechnicalDebt(twoDaysDebt).setChanges(
       newArrayList(
-        new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(fourDaysAgo)
+        // changelog created at is null because it has just been created on the current analysis
+        new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(null)
       )
     );
     when(issuable.issues()).thenReturn(newArrayList(issue));
@@ -127,16 +128,15 @@ public class NewTechnicalDebtDecoratorTest {
     decorator.decorate(resource, context);
 
     // remember : period1 is 5daysAgo, period2 is 10daysAgo
-    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 3.0, 3.0)));
+    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 0.0, 1.0)));
   }
 
   @Test
   public void save_on_one_issue_with_changelog() {
-    Issue issue = new DefaultIssue().setKey("A").setCreationDate(fiveDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
+    Issue issue = new DefaultIssue().setKey("A").setCreationDate(tenDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
       newArrayList(
-        new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(twoDaysDebt), fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(rightNow),
-        new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(fourDaysAgo),
-        new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(oneDaysDebt)).setCreatedAt(nineDaysAgo)
+        new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(twoDaysDebt), fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(null),
+        new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(fourDaysAgo)
       )
     );
     when(issuable.issues()).thenReturn(newArrayList(issue));
@@ -149,9 +149,9 @@ public class NewTechnicalDebtDecoratorTest {
 
   @Test
   public void save_on_one_issue_with_changelog_having_null_value() {
-    Issue issue = new DefaultIssue().setKey("A").setCreationDate(fiveDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
+    Issue issue = new DefaultIssue().setKey("A").setCreationDate(tenDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
       newArrayList(
-        new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(rightNow),
+        new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(null),
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), null).setCreatedAt(fourDaysAgo),
         new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(oneDaysDebt)).setCreatedAt(nineDaysAgo)
       )
@@ -161,16 +161,16 @@ public class NewTechnicalDebtDecoratorTest {
     decorator.decorate(resource, context);
 
     // remember : period1 is 5daysAgo, period2 is 10daysAgo
-    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 5.0, 4.0)));
+    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 5.0, 5.0)));
   }
 
   @Test
   public void save_on_one_issue_with_changelog_and_periods_have_no_dates() {
     when(timeMachineConfiguration.periods()).thenReturn(newArrayList(new Period(1, null, null), new Period(2, null, null)));
 
-    Issue issue = new DefaultIssue().setKey("A").setCreationDate(fiveDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
+    Issue issue = new DefaultIssue().setKey("A").setCreationDate(tenDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
       newArrayList(
-        new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(rightNow),
+        new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(null),
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), null).setCreatedAt(fourDaysAgo),
         new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(oneDaysDebt)).setCreatedAt(nineDaysAgo)
       )
@@ -180,12 +180,12 @@ public class NewTechnicalDebtDecoratorTest {
     decorator.decorate(resource, context);
 
     // remember : period1 is 5daysAgo, period2 is 10daysAgo
-    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 4.0, 4.0)));
+    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 5.0, 5.0)));
   }
 
   @Test
   public void save_on_one_issue_with_changelog_having_not_only_technical_debt_changes() {
-    Issue issue = new DefaultIssue().setKey("A").setCreationDate(fiveDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
+    Issue issue = new DefaultIssue().setKey("A").setCreationDate(tenDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
       newArrayList(
         new FieldDiffs()
           .setDiff("actionPlan", "1.0", "1.1").setCreatedAt(fourDaysAgo)
@@ -197,19 +197,19 @@ public class NewTechnicalDebtDecoratorTest {
     decorator.decorate(resource, context);
 
     // remember : period1 is 5daysAgo, period2 is 10daysAgo
-    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 3.0, 3.0)));
+    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 3.0, 4.0)));
   }
 
   @Test
   public void save_on_issues_with_changelog() {
-    Issue issue1 = new DefaultIssue().setKey("A").setCreationDate(fiveDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
+    Issue issue1 = new DefaultIssue().setKey("A").setCreationDate(tenDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
       newArrayList(
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(twoDaysDebt), fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(rightNow),
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(fourDaysAgo),
         new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(oneDaysDebt)).setCreatedAt(nineDaysAgo)
       )
     );
-    Issue issue2 = new DefaultIssue().setKey("B").setCreationDate(fiveDaysAgo).setTechnicalDebt(twoDaysDebt).setChanges(
+    Issue issue2 = new DefaultIssue().setKey("B").setCreationDate(tenDaysAgo).setTechnicalDebt(twoDaysDebt).setChanges(
       newArrayList(
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(rightNow),
         new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(oneDaysDebt)).setCreatedAt(nineDaysAgo)
@@ -220,7 +220,7 @@ public class NewTechnicalDebtDecoratorTest {
     decorator.decorate(resource, context);
 
     // remember : period1 is 5daysAgo, period2 is 10daysAgo
-    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 3.0, 5.0)));
+    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 3.0, 7.0)));
   }
 
   @Test
@@ -277,14 +277,14 @@ public class NewTechnicalDebtDecoratorTest {
   @Test
   public void save_on_issues_with_changelog_and_issues_without_changelog() {
     // issue1 and issue2 have changelog
-    Issue issue1 = new DefaultIssue().setKey("A").setCreationDate(fiveDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
+    Issue issue1 = new DefaultIssue().setKey("A").setCreationDate(tenDaysAgo).setTechnicalDebt(fiveDaysDebt).setChanges(
       newArrayList(
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(twoDaysDebt), fromWorkDayDuration(fiveDaysDebt)).setCreatedAt(rightNow),
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(fourDaysAgo),
         new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(oneDaysDebt)).setCreatedAt(nineDaysAgo)
       )
     );
-    Issue issue2 = new DefaultIssue().setKey("B").setCreationDate(fiveDaysAgo).setTechnicalDebt(twoDaysDebt).setChanges(
+    Issue issue2 = new DefaultIssue().setKey("B").setCreationDate(tenDaysAgo).setTechnicalDebt(twoDaysDebt).setChanges(
       newArrayList(
         new FieldDiffs().setDiff("technicalDebt", fromWorkDayDuration(oneDaysDebt), fromWorkDayDuration(twoDaysDebt)).setCreatedAt(rightNow),
         new FieldDiffs().setDiff("technicalDebt", null, fromWorkDayDuration(oneDaysDebt)).setCreatedAt(nineDaysAgo)
@@ -299,7 +299,7 @@ public class NewTechnicalDebtDecoratorTest {
     decorator.decorate(resource, context);
 
     // remember : period1 is 5daysAgo, period2 is 10daysAgo
-    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 3.0, 12.0)));
+    verify(context).saveMeasure(argThat(new IsVariationMeasure(CoreMetrics.NEW_TECHNICAL_DEBT, 3.0, 14.0)));
   }
 
   @Test
@@ -315,8 +315,8 @@ public class NewTechnicalDebtDecoratorTest {
     verify(context, never()).saveMeasure(argThat(new IsMeasure(CoreMetrics.NEW_TECHNICAL_DEBT)));
   }
 
-  private String fromWorkDayDuration(WorkDayDuration workDayDuration){
-    return Long.toString(workDayDuration.toLong());
+  private Long fromWorkDayDuration(WorkDayDuration workDayDuration){
+    return workDayDuration.toLong();
   }
 
 
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/FieldDiffs.java b/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/FieldDiffs.java
index 7cd29d66d6e..6329f703443 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/FieldDiffs.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/issue/internal/FieldDiffs.java
@@ -157,14 +157,36 @@ public class FieldDiffs implements Serializable {
     }
 
     @CheckForNull
+    public Long oldValueLong() {
+      return toLong(oldValue);
+    }
+
+    @CheckForNull
     public T newValue() {
       return newValue;
     }
 
+    @CheckForNull
+    public Long newValueLong() {
+      return toLong(newValue);
+    }
+
     void setNewValue(T t) {
       this.newValue = t;
     }
 
+    @CheckForNull
+    private Long toLong(Serializable value) {
+      if (value != null && !"".equals(value)) {
+        try {
+          return Long.valueOf((String) value);
+        } catch (ClassCastException e) {
+          return (Long) value;
+        }
+      }
+      return null;
+    }
+
     @Override
     public String toString() {
       //TODO escape , and | characters
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/issue/internal/FieldDiffsTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/issue/internal/FieldDiffsTest.java
index 6a07ac3cffb..2490d88bdcb 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/issue/internal/FieldDiffsTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/issue/internal/FieldDiffsTest.java
@@ -49,6 +49,24 @@ public class FieldDiffsTest {
   }
 
   @Test
+  public void diff_with_long_values() throws Exception {
+    diffs.setDiff("technicalDebt", 50l, "100");
+
+    FieldDiffs.Diff diff = diffs.diffs().get("technicalDebt");
+    assertThat(diff.oldValueLong()).isEqualTo(50l);
+    assertThat(diff.newValueLong()).isEqualTo(100l);
+  }
+
+  @Test
+  public void diff_with_empty_long_values() throws Exception {
+    diffs.setDiff("technicalDebt", null, "");
+
+    FieldDiffs.Diff diff = diffs.diffs().get("technicalDebt");
+    assertThat(diff.oldValueLong()).isNull();
+    assertThat(diff.newValueLong()).isNull();
+  }
+
+  @Test
   public void test_diff_by_key() throws Exception {
     diffs.setDiff("severity", "BLOCKER", "INFO");
     diffs.setDiff("resolution", "OPEN", "FIXED");