From 2e69ab5a682768568f24c037480cd7a18d832459 Mon Sep 17 00:00:00 2001 From: Evgeny Mandrikov Date: Wed, 22 Jun 2011 12:58:16 +0400 Subject: [PATCH] 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. --- .../core/sensors/CloseReviewsDecorator.java | 2 +- .../CloseReviewsDecoratorTest/fixture.xml | 4 +++ ...ewWithoutCorrespondingViolation-result.xml | 16 +++++------ ...lvedReviewWithNonFixedViolation-result.xml | 16 +++++------ .../purgeOrphanReviews-result.xml | 4 +-- .../purgeOrphanReviews.xml | 4 +-- .../main/webapp/WEB-INF/app/models/review.rb | 27 ++++++++++++------- .../app/views/resource/_violation.html.erb | 2 +- .../app/views/reviews/_review.html.erb | 2 +- .../201_change_false_positive_on_reviews.rb | 11 +++++--- .../org/sonar/test/persistence/sonar-test.ddl | 2 +- 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"/> + 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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="FIXED" 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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="FIXED" severity="[null]" resource_line="[null]" project_id="[null]"/> \ 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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="FIXED" severity="[null]" resource_line="[null]" project_id="[null]"/> \ 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]"/> + created_at="[null]" updated_at="[null]" user_id="[null]" assignee_id="[null]" title="[null]" resolution="[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]"/> 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 %>   - <% 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 @@   <% 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) ); -- 2.39.5