SONAR-2453 Update the way "false-positive" reviews are managed

* The column REVIEWS.FALSE-POSITIVE should be renamed to
  REVIEWS.RESOLUTION, the value of this column should be FALSE-POSITIVE
  for false-positive reviews and FIXED for other RESOLVED reviews.

* The status of a false-positive reviews should be RESOLVED.

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 922e3154e2df8b55f2016542f1afc1b2b3a37b20..9bc35bb5f0f3f10a44862071759183383e6d812a 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
@@ -91,7 +91,7 @@ public class CloseReviewsDecorator implements Decorator {
   }
 
   protected String generateUpdateToReopenResolvedReviewsForNonFixedViolation(int resourceId) {
-    return "UPDATE reviews SET status='REOPENED', updated_at=CURRENT_TIMESTAMP WHERE status='RESOLVED' AND resource_id = " + resourceId;
+    return "UPDATE reviews SET status='REOPENED', resolution=NULL, updated_at=CURRENT_TIMESTAMP WHERE status='RESOLVED' AND resource_id = " + resourceId;
   }
 
   protected String generateUpdateToCloseReviewsForDeletedResources(int projectId, int projectSnapshotId) {

diff --git a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/fixture.xml b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/fixture.xml
index 5c1b54ac17392eea68886add6404aee1df2a8890..b72092a662a49b88e17a117f03bdd388addcdae4 100644 (file)
--- a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/fixture.xml
+++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/fixture.xml
@@ -51,10 +51,12 @@
              rule_id="1" failure_level="1"/>
 
   <!-- Existing reviews -->
+  <!-- Note that DbUnit uses the first tag for a table to define the columns to be populated. So that's why "resolution" column here. -->
   <reviews
                        id="1"
                        status="OPEN"
                        rule_failure_permanent_id="1"
+                       resolution="[null]"
                        resource_id="555"/>
   <reviews
                        id="2"
@@ -79,6 +81,7 @@
   <reviews
                        id="6"
                        status="RESOLVED"
+                       resolution="FIXED"
                        rule_failure_permanent_id="3"
                        resource_id="666"/>
   <reviews
@@ -89,6 +92,7 @@
   <reviews
                        id="8"
                        status="RESOLVED"
+                       resolution="FIXED"
                        rule_failure_permanent_id="2"
                        resource_id="666"/>
 

diff --git a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseReviewWithoutCorrespondingViolation-result.xml b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseReviewWithoutCorrespondingViolation-result.xml
index f74c2d97a737242c83306ab91082a883a417afba..5ba0b972ce9a7a37e614b7d707e8668ec06a1dca 100644 (file)
--- a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseReviewWithoutCorrespondingViolation-result.xml
+++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseReviewWithoutCorrespondingViolation-result.xml
@@ -5,48 +5,48 @@
                        status="OPEN"
                        rule_failure_permanent_id="1"
                        resource_id="555"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="2"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="3"
                        status="OPEN"
                        rule_failure_permanent_id="3"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="4"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="5"
                        status="REOPENED"
                        rule_failure_permanent_id="3"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="6"
                        status="RESOLVED"
                        rule_failure_permanent_id="3"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="FIXED" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="7"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="8"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="FIXED" severity="[null]" resource_line="[null]" project_id="[null]"/>
 
 </dataset>
\ No newline at end of file

diff --git a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldReopenResolvedReviewWithNonFixedViolation-result.xml b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldReopenResolvedReviewWithNonFixedViolation-result.xml
index 0397576d6b33ce35f678ce6612cd9411e60928ca..de7ef1fb28cb9a42e2f49c4dbf19186b30a6bb36 100644 (file)
--- a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldReopenResolvedReviewWithNonFixedViolation-result.xml
+++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldReopenResolvedReviewWithNonFixedViolation-result.xml
@@ -5,48 +5,48 @@
                        status="OPEN"
                        rule_failure_permanent_id="1"
                        resource_id="555"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="2"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="3"
                        status="OPEN"
                        rule_failure_permanent_id="3"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="4"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="5"
                        status="REOPENED"
                        rule_failure_permanent_id="3"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="6"
                        status="REOPENED"
                        rule_failure_permanent_id="3"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="7"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
   <reviews
                        id="8"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]" project_id="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="FIXED" severity="[null]" resource_line="[null]" project_id="[null]"/>
 
 </dataset>
\ No newline at end of file

diff --git a/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews-result.xml b/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews-result.xml
index 50fc18b74c42bf8e901ed238640d3bee67d24968..fea5c7d00e3ceaad6d2d86ccb69b45e7f7d36ba6 100644 (file)
--- a/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews-result.xml
+++ b/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews-result.xml
@@ -16,7 +16,7 @@
                        rule_failure_permanent_id="1"
                        resource_id="555"
                        project_id="1"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]"/>
                        
   <!--  Following must have been deleted
   <reviews
@@ -25,7 +25,7 @@
                        rule_failure_permanent_id="2"
                        resource_id="666"
                        project_id="2"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]"/>
   -->
                        
   <review_comments

diff --git a/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews.xml b/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews.xml
index 6408b5a19477a0324d10726209ad204ab2ed8085..dca003915bd0e7a0bef1b105e8df1b4a951b1f9d 100644 (file)
--- a/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews.xml
+++ b/plugins/sonar-dbcleaner-plugin/src/test/resources/org/sonar/plugins/dbcleaner/purges/PurgeOrphanReviewsTest/purgeOrphanReviews.xml
@@ -16,14 +16,14 @@
                        rule_failure_permanent_id="1"
                        resource_id="555"
                        project_id="1"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]"/>
   <reviews
                        id="2"
                        status="CLOSED"
                        rule_failure_permanent_id="2"
                        resource_id="666"
                        project_id="2"
-                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" false_positive="[null]" severity="[null]" resource_line="[null]"/>
+                       created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[null]" severity="[null]" resource_line="[null]"/>
                        
   <review_comments
                        id="1"

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/review.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/review.rb
index 9b886cfbd79cbd4216712a4e73a2cf4ab10b03b2..847264482037d39c53e646c30dde78f8ce38fb80 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/review.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/review.rb
@@ -110,13 +110,21 @@ class Review < ActiveRecord::Base
         end
       end
       create_comment(:user => params[:user], :text => params[:text])
-      self.false_positive = is_false_positive
       self.assignee = nil
-      self.status = STATUS_OPEN
-      self.save!      
+      self.status = is_false_positive ? STATUS_RESOLVED : STATUS_REOPENED
+      self.resolution = is_false_positive ? 'FALSE-POSITIVE' : nil
+      self.save!
     end
   end
   
+  def false_positive
+    resolution == 'FALSE-POSITIVE'
+  end
+  
+  def can_change_false_positive_flag?
+    (status == STATUS_RESOLVED && resolution == 'FALSE-POSITIVE') || status == STATUS_OPEN || status == STATUS_REOPENED
+  end
+
   def isResolved?
     status == STATUS_RESOLVED
   end
@@ -131,11 +139,13 @@ class Review < ActiveRecord::Base
   
   def reopen
     self.status = STATUS_REOPENED
+    self.resolution = nil
     self.save!
   end
   
   def resolve
     self.status = STATUS_RESOLVED
+    self.resolution = 'FIXED'
     self.save!
   end
   
@@ -155,22 +165,19 @@ class Review < ActiveRecord::Base
     # Following code just for backward compatibility
     review_type = options['review_type']
     if review_type
-      conditions << 'false_positive=:false_positive'
       if review_type == 'FALSE_POSITIVE'
-        values[:false_positive]=true
+        conditions << "resolution='FALSE-POSITIVE'"
       else
-        values[:false_positive]=false
+        conditions << "(resolution<>'FALSE-POSITIVE' OR resolution IS NULL)"
       end
     end
     # --- End of code for backward compatibility code ---
       
     false_positives = options['false_positives']
     if false_positives == "only"
-      conditions << 'false_positive=:false_positive'
-      values[:false_positive]=true
+      conditions << "resolution='FALSE-POSITIVE'"
     elsif false_positives == "without"
-      conditions << 'false_positive=:false_positive'
-      values[:false_positive]=false    
+      conditions << "(resolution<>'FALSE-POSITIVE' OR resolution IS NULL)"
     end
     
     ids=options['ids'].split(',') if options['ids']

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb
index 114032a6472f86475e21f6c76cdf8e6ed91fb1d2..15b621fec2248559af09198a84b2e992e96a08fe 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb
@@ -70,7 +70,7 @@
         <% end %>
       
         &nbsp;
-        <% unless violation.review && violation.review.isResolved? %>
+        <% if violation.review && violation.review.can_change_false_positive_flag? %>
         <%= link_to_remote (violation.switched_off? ? "Unflag as false-positive" : "Flag as false-positive"),
                        :url => { :controller => "reviews", :action => "violation_false_positive_form", :id => violation.id, :false_positive => !violation.switched_off? },
                        :update => "reviewForm" + violation.id.to_s,

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_review.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_review.html.erb
index 45d91944eaba778ac43d7da735b55f0f6ebfa315..05c7e46353fcdc315ccbb8b3f8ec385701b97513 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_review.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_review.html.erb
@@ -32,7 +32,7 @@
                 
           <% end %>
         <% end %>
-        <% unless review.isResolved? %>
+        <% if review.can_change_false_positive_flag? %>
           <%= link_to_remote (violation_switched_off ? "Unflag as false-positive" : "Flag as false-positive"),
                        :url => { :controller => "reviews", :action => "false_positive_form", :id => review.id, :false_positive => !violation_switched_off },
                        :update => "reviewForm",

diff --git a/sonar-server/src/main/webapp/WEB-INF/db/migrate/201_change_false_positive_on_reviews.rb b/sonar-server/src/main/webapp/WEB-INF/db/migrate/201_change_false_positive_on_reviews.rb
index d2c2312cf7b718773f501acce1e59e6da9e05059..fa0d157e867b656c6d89dc1844682a693d7a0a9c 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/db/migrate/201_change_false_positive_on_reviews.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/db/migrate/201_change_false_positive_on_reviews.rb
@@ -24,14 +24,17 @@
 class ChangeFalsePositiveOnReviews < ActiveRecord::Migration
 
   def self.up
-    add_column 'reviews', 'false_positive', :boolean, :null => true, :default => false
+    add_column 'reviews', 'resolution', :string, :limit => 200, :null => true
     Review.reset_column_information
-    
+
     Review.find(:all).each do |review|
-      review.false_positive= (review.review_type == 'FALSE_POSITIVE')
+      if review.review_type == 'FALSE_POSITIVE'
+        review.status = 'RESOLVED'
+        review.resolution = 'FALSE-POSITIVE'
+      end
       review.save!
     end
-    
+
     remove_column 'reviews', 'review_type'
     Review.reset_column_information
   end

diff --git a/sonar-testing-harness/src/main/resources/org/sonar/test/persistence/sonar-test.ddl b/sonar-testing-harness/src/main/resources/org/sonar/test/persistence/sonar-test.ddl
index b90fec13f8e73a78edf1b01ef8dc1bf04ca65bbc..c8c81e9de40a232f4adf41569d4e248f37424f34 100644 (file)
--- a/sonar-testing-harness/src/main/resources/org/sonar/test/persistence/sonar-test.ddl
+++ b/sonar-testing-harness/src/main/resources/org/sonar/test/persistence/sonar-test.ddl
@@ -496,7 +496,7 @@ CREATE TABLE REVIEWS (
   PROJECT_ID INTEGER,
   RESOURCE_ID INTEGER,
   RESOURCE_LINE INTEGER,
-  FALSE_POSITIVE SMALLINT,
+  RESOLUTION VARCHAR(200),
   primary key (id)
 );