SONAR-1974 automatically close reviews that relate to resolved manual violations

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java
index c7c12be103e8e58c09d544b8e244ce6acf815ee7..26c697bf5427834737c6686fb6f6f568710e1250 100644 (file)
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java
@@ -73,8 +73,9 @@ public class CloseReviewsDecorator implements Decorator {
       int resourceId = currentSnapshot.getResourceId();
       int snapshotId = currentSnapshot.getId();
 
-      closeReviews(resource, resourceId, snapshotId);
-      reopenReviews(resource, resourceId);
+      closeReviewsOnResolvedViolations(resource, resourceId, snapshotId);
+      reopenReviewsOnUnresolvedViolations(resource, resourceId);
+
       if (ResourceUtils.isRootProject(resource)) {
         closeReviewsForDeletedResources(resourceId, currentSnapshot.getId());
       }
@@ -86,34 +87,51 @@ public class CloseReviewsDecorator implements Decorator {
   /**
    * Close reviews for which violations have been fixed.
    */
-  protected int closeReviews(Resource resource, int resourceId, int snapshotId) {
-    String conditions = " WHERE status!='CLOSED' AND resource_id=" + resourceId
-        + " AND manual_violation=:manualViolation AND rule_failure_permanent_id NOT IN " + "(SELECT permanent_id FROM rule_failures WHERE snapshot_id=" + snapshotId
-        + " AND permanent_id IS NOT NULL)";
+  protected int closeReviewsOnResolvedViolations(Resource resource, int resourceId, int snapshotId) {
+    String conditions = " WHERE resource_id=" + resourceId + " AND "
+      + "( "
+      + "  (manual_violation=:automaticViolation AND status!='CLOSED' AND rule_failure_permanent_id NOT IN " + "(SELECT permanent_id FROM rule_failures WHERE snapshot_id=" + snapshotId + " AND permanent_id IS NOT NULL))"
+      + "  OR "
+      + "  (manual_violation=:manualViolation AND status='RESOLVED')"
+      + ")";
     List<Review> reviews = databaseSession.getEntityManager().createNativeQuery("SELECT * FROM reviews " + conditions, Review.class)
-        .setParameter("manualViolation", false)
-        .getResultList();
+      .setParameter("automaticViolation", false)
+      .setParameter("manualViolation", true)
+      .getResultList();
+
     for (Review review : reviews) {
       notifyClosed(resource, review);
     }
+
     int rowUpdated = databaseSession.createNativeQuery("UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP" + conditions)
-        .setParameter("manualViolation", false)
-        .executeUpdate();
-    LOG.debug("- {} reviews set to 'closed' on resource #{}", rowUpdated, resourceId);
+      .setParameter("automaticViolation", false)
+      .setParameter("manualViolation", true)
+      .executeUpdate();
+    if (rowUpdated > 0) {
+      LOG.debug("- {} reviews closed on #{}", rowUpdated, resourceId);
+    }
     return rowUpdated;
   }
 
   /**
    * Reopen reviews that had been set to resolved but for which the violation is still here.
+   * Manual violations are ignored.
    */
-  protected int reopenReviews(Resource resource, int resourceId) {
-    String conditions = " WHERE status='RESOLVED' AND resolution<>'FALSE-POSITIVE' AND resource_id=" + resourceId;
-    List<Review> reviews = databaseSession.getEntityManager().createNativeQuery("SELECT * FROM reviews " + conditions, Review.class).getResultList();
+  protected int reopenReviewsOnUnresolvedViolations(Resource resource, int resourceId) {
+    String conditions = " WHERE status='RESOLVED' AND resolution<>'FALSE-POSITIVE' AND manual_violation=:manualViolation AND resource_id=" + resourceId;
+    List<Review> reviews = databaseSession.getEntityManager().createNativeQuery("SELECT * FROM reviews " + conditions, Review.class)
+      .setParameter("manualViolation", false)
+      .getResultList();
     for (Review review : reviews) {
       notifyReopened(resource, review);
     }
-    int rowUpdated = databaseSession.createNativeQuery("UPDATE reviews SET status='REOPENED', resolution=NULL, updated_at=CURRENT_TIMESTAMP" + conditions).executeUpdate();
-    LOG.debug("- {} reviews set to 'reopened' on resource #{}", rowUpdated, resourceId);
+
+    int rowUpdated = databaseSession.createNativeQuery("UPDATE reviews SET status='REOPENED', resolution=NULL, updated_at=CURRENT_TIMESTAMP" + conditions)
+      .setParameter("manualViolation", false)
+      .executeUpdate();
+    if (rowUpdated > 0) {
+      LOG.debug("- {} reviews reopened on #{}", rowUpdated, resourceId);
+    }
     return rowUpdated;
   }
 
@@ -122,18 +140,18 @@ public class CloseReviewsDecorator implements Decorator {
    */
   protected int closeReviewsForDeletedResources(int projectId, int projectSnapshotId) {
     String conditions = " WHERE status!='CLOSED' AND project_id=" + projectId
-        + " AND resource_id IN ( SELECT prev.project_id FROM snapshots prev  WHERE prev.root_project_id=" + projectId
-        + " AND prev.islast=? AND NOT EXISTS ( SELECT cur.id FROM snapshots cur WHERE cur.root_snapshot_id=" + projectSnapshotId
-        + " AND cur.created_at > prev.created_at AND cur.root_project_id=" + projectId + " AND cur.project_id=prev.project_id ) )";
+      + " AND resource_id IN ( SELECT prev.project_id FROM snapshots prev  WHERE prev.root_project_id=" + projectId
+      + " AND prev.islast=? AND NOT EXISTS ( SELECT cur.id FROM snapshots cur WHERE cur.root_snapshot_id=" + projectSnapshotId
+      + " AND cur.created_at > prev.created_at AND cur.root_project_id=" + projectId + " AND cur.project_id=prev.project_id ) )";
     List<Review> reviews = databaseSession.getEntityManager().createNativeQuery("SELECT * FROM reviews " + conditions, Review.class)
-        .setParameter(1, Boolean.TRUE)
-        .getResultList();
+      .setParameter(1, Boolean.TRUE)
+      .getResultList();
     for (Review review : reviews) {
       notifyClosed(null, review);
     }
     int rowUpdated = databaseSession.createNativeQuery("UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP" + conditions)
-        .setParameter(1, Boolean.TRUE)
-        .executeUpdate();
+      .setParameter(1, Boolean.TRUE)
+      .executeUpdate();
     LOG.debug("- {} reviews set to 'closed' on project #{}", rowUpdated, projectSnapshotId);
     return rowUpdated;
   }
@@ -156,28 +174,28 @@ public class CloseReviewsDecorator implements Decorator {
 
   void notifyReopened(Resource resource, Review review) {
     Notification notification = createReviewNotification(resource, review)
-        .setFieldValue("old.status", review.getStatus())
-        .setFieldValue("new.status", "REOPENED")
-        .setFieldValue("old.resolution", review.getResolution())
-        .setFieldValue("new.resolution", null);
+      .setFieldValue("old.status", review.getStatus())
+      .setFieldValue("new.status", "REOPENED")
+      .setFieldValue("old.resolution", review.getResolution())
+      .setFieldValue("new.resolution", null);
     notificationManager.scheduleForSending(notification);
   }
 
   void notifyClosed(Resource resource, Review review) {
     Notification notification = createReviewNotification(resource, review)
-        .setFieldValue("old.status", review.getStatus())
-        .setFieldValue("new.status", "CLOSED");
+      .setFieldValue("old.status", review.getStatus())
+      .setFieldValue("new.status", "CLOSED");
     notificationManager.scheduleForSending(notification);
   }
 
   private Notification createReviewNotification(Resource resource, Review review) {
     return new Notification("review-changed")
-        .setFieldValue("reviewId", String.valueOf(review.getId()))
-        .setFieldValue("project", project.getRoot().getLongName())
-        .setFieldValue("resource", resource != null ? resource.getLongName() : null)
-        .setFieldValue("title", review.getTitle())
-        .setFieldValue("creator", getCreator(review))
-        .setFieldValue("assignee", getAssignee(review));
+      .setFieldValue("reviewId", String.valueOf(review.getId()))
+      .setFieldValue("project", project.getRoot().getLongName())
+      .setFieldValue("resource", resource != null ? resource.getLongName() : null)
+      .setFieldValue("title", review.getTitle())
+      .setFieldValue("creator", getCreator(review))
+      .setFieldValue("assignee", getAssignee(review));
   }
 
 }

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/ManualViolationInjector.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/ManualViolationInjector.java
index 4e9901e3a1d1e858251e3a6f56399556f8288a1b..e85c0bae1e9c85b908e7406dcc5ab280952341a6 100644 (file)
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/ManualViolationInjector.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/ManualViolationInjector.java
@@ -32,7 +32,6 @@ import org.sonar.persistence.dao.ReviewDao;
 import org.sonar.persistence.model.Review;
 import org.sonar.persistence.model.ReviewQuery;
 
-
 import java.util.List;
 
 @Phase(name = Phase.Name.PRE)
@@ -52,13 +51,13 @@ public class ManualViolationInjector implements Decorator {
 
   public void decorate(Resource resource, DecoratorContext context) {
     if (resource.getId() != null) {
-      ReviewQuery query = ReviewQuery.create().setManualViolation(true).setResourceId(resource.getId()).setStatus(Review.STATUS_OPEN);
+      ReviewQuery query = ReviewQuery.create().setManualViolation(true).setResourceId(resource.getId()).setStatus(Review.STATUS_OPENED);
       List<Review> reviews = reviewDao.selectByQuery(query);
       for (Review review : reviews) {
-        if (review.getRuleId()==null) {
+        if (review.getRuleId() == null) {
           LoggerFactory.getLogger(getClass()).warn("No rule is defined on the review with id: " + review.getId());
         }
-        if (review.getViolationPermanentId()==null) {
+        if (review.getViolationPermanentId() == null) {
           LoggerFactory.getLogger(getClass()).warn("Permanent id of manual violation is missing on the review with id: " + review.getId());
         }
         Violation violation = Violation.create(ruleFinder.findById(review.getRuleId()), resource);

diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java
index afab33e4d7871f0be100bca1268560fdb85dd024..4d3351eb4317e2b77e475edf4535fb4f2a3df72b 100644 (file)
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java
@@ -19,25 +19,21 @@
  */
 package org.sonar.plugins.core.sensors;
 
-import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 import junit.framework.ComparisonFailure;
-
 import org.junit.Before;
 import org.junit.Test;
 import org.sonar.api.notifications.Notification;
 import org.sonar.api.notifications.NotificationManager;
 import org.sonar.api.resources.Project;
 import org.sonar.api.security.UserFinder;
+import org.sonar.jpa.entity.Review;
 import org.sonar.jpa.test.AbstractDbUnitTestCase;
 
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.*;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.*;
+
 public class CloseReviewsDecoratorTest extends AbstractDbUnitTestCase {
 
   private NotificationManager notificationManager;
@@ -62,14 +58,14 @@ public class CloseReviewsDecoratorTest extends AbstractDbUnitTestCase {
   public void shouldCloseReviewWithoutCorrespondingViolation() throws Exception {
     setupData("fixture");
 
-    int count = reviewsDecorator.closeReviews(null, 666, 222);
+    int count = reviewsDecorator.closeReviewsOnResolvedViolations(null, 666, 222);
 
     assertThat(count, is(3));
     verify(notificationManager, times(3)).scheduleForSending(any(Notification.class));
-    checkTables("shouldCloseReviewWithoutCorrespondingViolation", new String[] { "updated_at" }, new String[] { "reviews" });
+    checkTables("shouldCloseReviewWithoutCorrespondingViolation", new String[]{"updated_at"}, "reviews");
 
     try {
-      checkTables("shouldCloseReviewWithoutCorrespondingViolation", new String[] { "reviews" });
+      checkTables("shouldCloseReviewWithoutCorrespondingViolation", "reviews");
       fail("'updated_at' columns are identical whereas they should be different.");
     } catch (ComparisonFailure e) {
       // "updated_at" column must be different, so the comparison should raise this exception
@@ -81,20 +77,35 @@ public class CloseReviewsDecoratorTest extends AbstractDbUnitTestCase {
     setupData("fixture");
 
     // First we close the reviews for which the violations have been fixed (this is because we use the same "fixture"...)
-    reviewsDecorator.closeReviews(null, 666, 222);
+    reviewsDecorator.closeReviewsOnResolvedViolations(null, 666, 222);
 
     // And now we reopen the reviews that still have a violation
-    int count = reviewsDecorator.reopenReviews(null, 666);
+    int count = reviewsDecorator.reopenReviewsOnUnresolvedViolations(null, 666);
 
     assertThat(count, is(1));
     verify(notificationManager, times(4)).scheduleForSending(any(Notification.class));
-    checkTables("shouldReopenResolvedReviewWithNonFixedViolation", new String[] { "updated_at" }, new String[] { "reviews" });
+    checkTables("shouldReopenResolvedReviewWithNonFixedViolation", new String[]{"updated_at"}, "reviews");
 
     try {
-      checkTables("shouldReopenResolvedReviewWithNonFixedViolation", new String[] { "reviews" });
+      checkTables("shouldReopenResolvedReviewWithNonFixedViolation", "reviews");
       fail("'updated_at' columns are identical whereas they should be different.");
     } catch (ComparisonFailure e) {
       // "updated_at" column must be different, so the comparison should raise this exception
     }
   }
+
+  @Test
+  public void shouldCloseResolvedManualViolations() throws Exception {
+    setupData("shouldCloseResolvedManualViolations");
+
+    int count = reviewsDecorator.closeReviewsOnResolvedViolations(null, 555, 11);
+    assertThat(count, is(1));
+
+    verify(notificationManager, times(1)).scheduleForSending(any(Notification.class));
+
+    getSession().commit();
+    assertThat(getSession().getSingleResult(Review.class, "id", 1L).getStatus(), is("CLOSED")); // automatic violation not changed
+    assertThat(getSession().getSingleResult(Review.class, "id", 2L).getStatus(), is("CLOSED")); // manual violation resolved -> closed
+    assertThat(getSession().getSingleResult(Review.class, "id", 3L).getStatus(), is("OPEN"));   // manual violation not changed
+  }
 }

diff --git a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseResolvedManualViolations.xml b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseResolvedManualViolations.xml
new file mode 100644 (file)
index 0000000..d5dd2a2
--- /dev/null
+++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseResolvedManualViolations.xml
@@ -0,0 +1,91 @@
+<dataset>
+
+  <snapshots
+    id="11"
+    project_id="555"
+    status="P" islast="false"/>
+
+  <!-- Automatic violations -->
+  <rule_failures
+    id="1"
+    permanent_id="1"
+    snapshot_id="11"
+    rule_id="1"
+    failure_level="1"/>
+
+
+  <!-- Manual violations -->
+  <rule_failures
+    id="2"
+    permanent_id="2"
+    snapshot_id="22"
+    rule_id="2"
+    failure_level="4"/>
+
+  <rule_failures
+    id="3"
+    permanent_id="3"
+    snapshot_id="22"
+    rule_id="2"
+    failure_level="4"/>
+
+
+  <!--
+
+  Reviews on automatic violation
+
+  -->
+  <reviews
+    id="1"
+    status="CLOSED"
+    rule_failure_permanent_id="1"
+    resolution="[null]"
+    created_at="[null]"
+    updated_at="[null]"
+    project_id="[null]"
+    resource_line="[null]"
+    severity="MINOR"
+    user_id="[null]"
+    resource_id="555"
+    rule_id="1"
+    manual_violation="false"/>
+
+  <!--
+
+  Reviews on manual violations
+
+  -->
+
+  <!-- to be closed -->
+  <reviews
+    id="2"
+    status="RESOLVED"
+    rule_failure_permanent_id="2"
+    resolution="FIXED"
+    created_at="[null]"
+    updated_at="[null]"
+    project_id="[null]"
+    resource_line="18"
+    severity="BLOCKER"
+    user_id="[null]"
+    resource_id="555"
+    rule_id="2"
+    manual_violation="true"/>
+
+  <!-- to keep opened -->
+  <reviews
+    id="3"
+    status="OPEN"
+    rule_failure_permanent_id="3"
+    resolution="[null]"
+    created_at="[null]"
+    updated_at="[null]"
+    project_id="[null]"
+    resource_line="18"
+    severity="BLOCKER"
+    user_id="[null]"
+    resource_id="555"
+    rule_id="2"
+    manual_violation="true"/>
+
+</dataset>
\ No newline at end of file

diff --git a/sonar-core/src/main/java/org/sonar/persistence/model/Review.java b/sonar-core/src/main/java/org/sonar/persistence/model/Review.java
index 54fb8ce2d9617b3eb941068a290008b0caec533e..4badd593f87176600184adcb420bdae0acc6bf77 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/persistence/model/Review.java
+++ b/sonar-core/src/main/java/org/sonar/persistence/model/Review.java
@@ -19,6 +19,9 @@
  */
 package org.sonar.persistence.model;
 
+import org.apache.commons.lang.builder.ToStringBuilder;
+import org.apache.commons.lang.builder.ToStringStyle;
+
 import java.util.Date;
 
 /**
@@ -26,7 +29,9 @@ import java.util.Date;
  */
 public class Review {
 
-  public static final String STATUS_OPEN = "OPEN";
+  public static final String STATUS_OPENED = "OPEN";
+  public static final String STATUS_REOPENED = "REOPENED";
+  public static final String STATUS_RESOLVED = "RESOLVED";
   public static final String STATUS_CLOSED = "CLOSED";
 
   private Long id;
@@ -179,4 +184,9 @@ public class Review {
     this.manualViolation = b;
     return this;
   }
+
+  @Override
+  public String toString() {
+    return ToStringBuilder.reflectionToString(this, ToStringStyle.SHORT_PREFIX_STYLE);
+  }
 }