From 6fcc40818bc6e92b642e7e3aa36726233da24e95 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Mon, 5 Dec 2011 17:05:21 +0100 Subject: [PATCH] SONAR-1974 automatically close reviews that relate to resolved manual violations --- .../core/sensors/CloseReviewsDecorator.java | 88 +++++++++++------- .../core/sensors/ManualViolationInjector.java | 7 +- .../sensors/CloseReviewsDecoratorTest.java | 45 +++++---- .../shouldCloseResolvedManualViolations.xml | 91 +++++++++++++++++++ .../org/sonar/persistence/model/Review.java | 12 ++- 5 files changed, 186 insertions(+), 57 deletions(-) create mode 100644 plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseResolvedManualViolations.xml 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 c7c12be103e..26c697bf542 100644 --- 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 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 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 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 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 4e9901e3a1d..e85c0bae1e9 100644 --- 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 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 afab33e4d78..4d3351eb431 100644 --- 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 index 00000000000..d5dd2a25938 --- /dev/null +++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseResolvedManualViolations.xml @@ -0,0 +1,91 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + \ 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 54fb8ce2d96..4badd593f87 100644 --- 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); + } } -- 2.39.5