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.

11 files changed, 52 insertions, 38 deletions

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 922e3154e2d..9bc35bb5f0f 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
@@ -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 5c1b54ac173..b72092a662a 100644
--- 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 f74c2d97a73..5ba0b972ce9 100644
--- 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 0397576d6b3..de7ef1fb28c 100644
--- 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 50fc18b74c4..fea5c7d00e3 100644
--- 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 6408b5a1947..dca003915bd 100644
--- 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 9b886cfbd79..84726448203 100644
--- 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 114032a6472..15b621fec22 100644
--- 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 45d91944eab..05c7e46353f 100644
--- 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 @@
 	        &nbsp;
           <% 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 d2c2312cf7b..fa0d157e867 100644
--- 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 b90fec13f8e..c8c81e9de40 100644
--- 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)
 );