if (currentSnapshot != null) {
int resourceId = currentSnapshot.getResourceId();
int snapshotId = currentSnapshot.getId();
- Query query = databaseSession.createNativeQuery(generateUpdateOnResourceSqlRequest(resourceId, snapshotId));
+
+ // Close reviews for which violations have been fixed
+ Query query = databaseSession.createNativeQuery(generateUpdateToCloseReviewsForFixedViolation(resourceId, snapshotId));
int rowUpdated = query.executeUpdate();
LOG.debug("- {} reviews set to 'closed' on resource #{}", rowUpdated, resourceId);
+ // Reopen reviews that had been set to resolved but for which the violation is still here
+ query = databaseSession.createNativeQuery(generateUpdateToReopenResolvedReviewsForNonFixedViolation(resourceId));
+ rowUpdated = query.executeUpdate();
+ LOG.debug("- {} reviews set to 'reopened' on resource #{}", rowUpdated, resourceId);
+
+ // And close reviews that relate to resources that have been deleted or renamed
if (ResourceUtils.isRootProject(resource)) {
- query = databaseSession.createNativeQuery(generateUpdateOnProjectSqlRequest(resourceId, currentSnapshot.getId()));
+ query = databaseSession.createNativeQuery(generateUpdateToCloseReviewsForDeletedResources(resourceId, currentSnapshot.getId()));
query.setParameter(1, Boolean.TRUE);
rowUpdated = query.executeUpdate();
LOG.debug("- {} reviews set to 'closed' on project #{}", rowUpdated, resourceId);
}
}
- protected String generateUpdateOnResourceSqlRequest(int resourceId, int snapshotId) {
- return "UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP WHERE status='OPEN' AND resource_id = " + resourceId
+ protected String generateUpdateToCloseReviewsForFixedViolation(int resourceId, int snapshotId) {
+ return "UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP WHERE status!='CLOSED' AND resource_id = " + resourceId
+ " AND rule_failure_permanent_id NOT IN " + "(SELECT permanent_id FROM rule_failures WHERE snapshot_id = " + snapshotId
+ " AND permanent_id IS NOT NULL)";
}
- protected String generateUpdateOnProjectSqlRequest(int projectId, int projectSnapshotId) {
- return "UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP WHERE status='OPEN' AND project_id=" + projectId
+ protected String generateUpdateToReopenResolvedReviewsForNonFixedViolation(int resourceId) {
+ return "UPDATE reviews SET status='REOPENED', updated_at=CURRENT_TIMESTAMP WHERE status='RESOLVED' AND resource_id = " + resourceId;
+ }
+
+ protected String generateUpdateToCloseReviewsForDeletedResources(int projectId, int projectSnapshotId) {
+ return "UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP 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 ) )";
setupData("fixture");
CloseReviewsDecorator reviewsDecorator = new CloseReviewsDecorator(null, null);
- String sqlRequest = reviewsDecorator.generateUpdateOnResourceSqlRequest(666, 222);
+ String sqlRequest = reviewsDecorator.generateUpdateToCloseReviewsForFixedViolation(666, 222);
Statement stmt = getConnection().createStatement();
int count = stmt.executeUpdate(sqlRequest);
- assertThat(count, is(1));
+ assertThat(count, is(3));
assertTables("shouldCloseReviewWithoutCorrespondingViolation", new String[] { "reviews" }, new String[] { "updated_at" });
try {
// "updated_at" column must be different, so the comparison should raise this exception
}
}
+
+ @Test
+ public void shouldReopenResolvedReviewWithNonFixedViolation() throws Exception {
+ setupData("fixture");
+
+ CloseReviewsDecorator reviewsDecorator = new CloseReviewsDecorator(null, null);
+
+ // First we close the reviews for which the violations have been fixed (this is because we use the same "fixture"...)
+ getConnection().createStatement().executeUpdate(reviewsDecorator.generateUpdateToCloseReviewsForFixedViolation(666, 222));
+
+ // And now we reopen the reviews that still have a violation
+ String sqlRequest = reviewsDecorator.generateUpdateToReopenResolvedReviewsForNonFixedViolation(666);
+ Statement stmt = getConnection().createStatement();
+ int count = stmt.executeUpdate(sqlRequest);
+
+ assertThat(count, is(1));
+ assertTables("shouldReopenResolvedReviewWithNonFixedViolation", new String[] { "reviews" }, new String[] { "updated_at" });
+
+ try {
+ assertTables("shouldReopenResolvedReviewWithNonFixedViolation", new String[] { "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
+ }
+ }
}
<dataset>
+ <!-- Component 555 -->
<snapshots
id="11"
project_id="555"
id="111"
project_id="555"
status="P" islast="false"/>
+ <!-- Component 666 -->
<snapshots
id="22"
project_id="666"
project_id="666"
status="P" islast="false"/>
+ <!-- Violations on previous analysis -->
<rule_failures
id="1"
permanent_id="1"
permanent_id="3"
snapshot_id="22"
rule_id="1" failure_level="1"/>
+ <!-- Violations on new analysis -->
+ <!-- Violation #1 still exists -->
<rule_failures
id="4"
permanent_id="1"
snapshot_id="111"
rule_id="1" failure_level="1"/>
+ <!-- Violation #2 has been fixed -->
+ <!-- Violation #3 still exists -->
<rule_failures
id="5"
permanent_id="3"
snapshot_id="222"
rule_id="1" failure_level="1"/>
+ <!-- Existing reviews -->
<reviews
id="1"
status="OPEN"
status="CLOSED"
rule_failure_permanent_id="2"
resource_id="666"/>
+ <reviews
+ id="5"
+ status="REOPENED"
+ rule_failure_permanent_id="3"
+ resource_id="666"/>
+ <reviews
+ id="6"
+ status="RESOLVED"
+ rule_failure_permanent_id="3"
+ resource_id="666"/>
+ <reviews
+ id="7"
+ status="REOPENED"
+ rule_failure_permanent_id="2"
+ resource_id="666"/>
+ <reviews
+ id="8"
+ status="RESOLVED"
+ rule_failure_permanent_id="2"
+ resource_id="666"/>
</dataset>
\ No newline at end of file
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]"/>
+ <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]"/>
+ <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]"/>
+ <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]"/>
+ <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]"/>
</dataset>
\ No newline at end of file
--- /dev/null
+<dataset>
+
+ <reviews
+ id="1"
+ 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]"/>
+ <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]"/>
+ <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]"/>
+ <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]"/>
+ <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]"/>
+ <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]"/>
+ <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]"/>
+ <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]"/>
+
+</dataset>
\ No newline at end of file
# - 'assignee' : login used to create a review directly assigned. Use 'none' to unassign.
# - 'false_positive' (true/false) : in conjunction with 'new_text' (mandatory in this case), changes the
# state 'false_positive' of the review
+ # - 'status' : used to change the status of the review. For the moment, only "RESOLVED" or "REOPENED" are accepted.
#
# Example :
# - PUT "/api/reviews/?id=1&false_positive=true&new_text=Because
# - PUT "/api/reviews/?id=1&assignee=none
# - PUT "/api/reviews/?id=1&new_text=New%20Comment!
# - PUT "/api/reviews/?id=1&text=Modified%20Comment!
+ # - PUT "/api/reviews/?id=1&status=RESOLVED
#
def update
begin
new_text = params[:new_text]
assignee = params[:assignee]
false_positive = params[:false_positive]
+ status = params[:status]
# 2- Get the review or create one
raise "No 'id' parameter has been provided." unless params[:id]
elsif !text.blank?
raise "You don't have the rights to edit the last comment of this review." unless review.comments.last.user == current_user
review.edit_last_comment(text)
+ elsif !status.blank?
+ raise "Only 'RESOLVED' or 'REOPENED' can be used as a new status for a review." unless status=='RESOLVED' || status=='REOPENED'
+ review.status=status
+ review.save!
else
render_error("The given parameter combination is invalid for updating the review.", 403)
return
SECTION=Navigation::SECTION_HOME
verify :method => :post,
- :only => [:assign, :comment_form, :flag_as_false_positive,
- :violation_assign, :violation_flag_as_false_positive,:violation_save_comment, :violation_delete_comment],
+ :only => [:assign, :flag_as_false_positive, :save_comment, :delete_comment, :change_status,
+ :violation_assign, :violation_flag_as_false_positive,:violation_save_comment, :violation_delete_comment, :violation_change_status],
:redirect_to => {:action => :error_not_post}
helper SourceHelper, UsersHelper
render :partial => "reviews/view"
end
+ # POST
+ def change_status
+ @review = Review.find(params[:id], :include => ['project'])
+ unless has_rights_to_modify?(@review.project)
+ render :text => "<b>Cannot create the comment</b> : access denied."
+ return
+ end
+
+ if @review.isResolved?
+ @review.reopen
+ else
+ # for the moment, if a review is not open, it can only be "RESOLVED"
+ @review.resolve
+ end
+
+ render :partial => "reviews/view"
+ end
+
#
#
end
render :partial => "resource/violation", :locals => { :violation => violation }
end
+
+ # POST
+ def violation_change_status
+ violation = RuleFailure.find(params[:id], :include => 'snapshot')
+ unless has_rights_to_modify?(violation.snapshot)
+ render :text => "<b>Cannot delete the comment</b> : access denied."
+ return
+ end
+ sanitize_violation(violation)
+
+ if violation.review
+ review = violation.review
+ if review.isResolved?
+ review.reopen
+ else
+ # for the moment, if a review is not open, it can only be "RESOLVED"
+ review.resolve
+ end
+ end
+
+ render :partial => "resource/violation", :locals => { :violation => violation }
+ end
@assignee_id = params[:assignee_id] || default_user
@author_id = params[:author_id] || ''
@severities = filter_any(params[:severities]) || ['']
- @statuses = filter_any(params[:statuses]) || [Review::STATUS_OPEN]
+ @statuses = filter_any(params[:statuses]) || [Review::STATUS_OPEN, Review::STATUS_REOPENED]
@projects = filter_any(params[:projects]) || ['']
@false_positives = params[:false_positives] || 'without'
@id = params[:review_id] || ''
before_save :assign_project
STATUS_OPEN = 'OPEN'
+ STATUS_RESOLVED = 'RESOLVED'
+ STATUS_REOPENED = 'REOPENED'
STATUS_CLOSED = 'CLOSED'
def on_project?
end
end
+ def isResolved?
+ status == STATUS_RESOLVED
+ end
+
+ def isClosed?
+ status == STATUS_CLOSED
+ end
+
+ def isReopened?
+ status == STATUS_REOPENED
+ end
+
+ def reopen
+ self.status = STATUS_REOPENED
+ self.save!
+ end
+
+ def resolve
+ self.status = STATUS_RESOLVED
+ self.save!
+ end
+
#
<%= image_tag("sep12.png") -%>
- <span class="rulename">
+ <span class="rulename" <%= 'style="text-decoration: line-through"' if violation.review && violation.review.isResolved? %> >
<a onclick="window.open(this.href,'rule','height=800,width=900,scrollbars=1,resizable=1');return false;" href="<%= url_for :controller => 'rules', :action => 'show', :id => violation.rule.key, :layout => 'false' -%>"><%= h(violation.rule.name) -%></a>
</span>
<span class="falsePositive">False-Positive</span>
<% end %>
+ <% if violation.review && violation.review.isReopened? %>
+ <%= image_tag("sep12.png") -%>
+
+ <span class="review-reopened">Reoponed</span>
+
+ <% end %>
<% if violation.review && violation.review.assignee_id %>
<%= image_tag("sep12.png") -%>
unless violation.switched_off?
%>
- <%= link_to_remote (violation.review && violation.review.assignee_id ? "Reassign" : "Assign"),
- :url => { :controller => "reviews", :action => "violation_assign_form", :violation_id => violation.id},
- :update => "vActions" + violation.id.to_s,
- :complete => "$('vActions" + violation.id.to_s + "').show();$('commentActions" + violation.id.to_s + "').hide();$('assignee_id').focus();" -%>
- <%
+ <%= link_to_remote (violation.review.isResolved? ? "Reopen" : "Resolve"),
+ :url => { :controller => "reviews", :action => "violation_change_status", :id => violation.id},
+ :update => "vId" + violation.id.to_s,
+ :confirm => violation.review.isResolved? ? "Do you want to reopen this review?" : "Do you want to resolve this review?" -%>
+
+ <% unless violation.review && violation.review.isResolved? %>
+
+ <%= link_to_remote (violation.review.assignee_id ? "Reassign" : "Assign"),
+ :url => { :controller => "reviews", :action => "violation_assign_form", :violation_id => violation.id},
+ :update => "vActions" + violation.id.to_s,
+ :complete => "$('vActions" + violation.id.to_s + "').show();$('commentActions" + violation.id.to_s + "').hide();$('assignee_id').focus();" -%>
+ <%
+ end
end
else
%>
<% end %>
+ <% unless violation.review && violation.review.isResolved? %>
<%= 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,
:complete => "$('reviewForm" + violation.id.to_s + "').show();$('commentText" + violation.id.to_s + "').focus();$('vActions" + violation.id.to_s + "').hide();$('commentActions" + violation.id.to_s + "').hide();" -%>
+ <% end %>
</span>
<% end %>
<%= link_to_remote "Delete",
:url => { :controller => "reviews", :action => "violation_delete_comment", :comment_id => review_comment.id, :id => violation.id },
:update => "vId" + violation.id.to_s,
- :confirm => "Do you want to delete this comment ?" -%>
+ :confirm => "Do you want to delete this comment?" -%>
<% end %>
<%
end
<% end %>
<%
- if current_user && review.status != "CLOSED" && review.rule_failure
+ if current_user && !review.isClosed? && review.rule_failure
violation_switched_off = review.rule_failure.switched_off?
%>
<span class="actions" id="rActions">
<%
if !violation_switched_off
%>
- <%= link_to_remote (review.assignee_id ? "Reassign" : "Assign"),
- :url => { :controller => "reviews", :action => "assign_form", :review_id => review.id},
- :update => "assignForm",
- :complete => "$('rActions').hide(); $('editActions').hide(); $('assignee_id').focus();" -%>
-
-
+ <%= link_to_remote (review.isResolved? ? "Reopen" : "Resolve"),
+ :url => { :controller => "reviews", :action => "change_status", :id => review.id},
+ :update => "review",
+ :confirm => review.isResolved? ? "Do you want to reopen this review?" : "Do you want to resolve this review?" -%>
+
+ <% unless review.isResolved? %>
+ <%= link_to_remote (review.assignee_id ? "Reassign" : "Assign"),
+ :url => { :controller => "reviews", :action => "assign_form", :review_id => review.id},
+ :update => "assignForm",
+ :complete => "$('rActions').hide(); $('editActions').hide(); $('assignee_id').focus();" -%>
+
+
+ <% end %>
<% end %>
- <%= link_to_remote (violation_switched_off ? "Unflag as false-positive" : "Flag as false-positive"),
+ <% unless review.isResolved? %>
+ <%= 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",
:complete => "$('reviewForm').show(); $('rActions').hide(); $('editActions').hide(); $('commentText').focus();" -%>
+ <% end %>
</span>
<% end %>
<select size="6" name="statuses[]" multiple="multiple" id="statuses" class="withIcons">
<option <%= 'selected' if @statuses.include?('') -%> value="">Any</option>
<option value="<%= Review::STATUS_OPEN -%>" class="status_open" <%= 'selected' if @statuses.include?(Review::STATUS_OPEN) -%>>Open</option>
+ <option value="<%= Review::STATUS_REOPENED -%>" class="status_reopened" <%= 'selected' if @statuses.include?(Review::STATUS_REOPENED) -%>>Reopened</option>
+ <option value="<%= Review::STATUS_RESOLVED -%>" class="status_resolved" <%= 'selected' if @statuses.include?(Review::STATUS_RESOLVED) -%>>Resolved</option>
<option value="<%= Review::STATUS_CLOSED -%>" class="status_closed" <%= 'selected' if @statuses.include?(Review::STATUS_CLOSED) -%>>Closed</option>
</select>
</td>
padding-left: 5px;
padding-right: 5px;
}
+.review-reopened {
+ background-color: #FDC596;
+ padding-left: 5px;
+ padding-right: 5px;
+}
div.vtitle a.action {
color: #777;
}
option.status_open {
background-image: url('../images/status/OPEN.png');
}
+option.status_reopened {
+ background-image: url('../images/status/REOPENED.png');
+}
+option.status_resolved {
+ background-image: url('../images/status/RESOLVED.png');
+}
option.status_closed {
background-image: url('../images/status/CLOSED.png');
}
private String newText;
private String assignee;
private Boolean falsePositive;
+ private String status;
public ReviewUpdateQuery() {
}
return query;
}
+ /**
+ * Builds a request that will change the status of a an existing review.<br/>
+ * <br/>
+ * Currently, only "RESOLVED" and "REOPENED" are supported.
+ *
+ * @param reviewId
+ * The id of the review
+ * @param status
+ * The new status
+ */
+ public static ReviewUpdateQuery changeStatusQuery(Long reviewId, String status) {
+ ReviewUpdateQuery query = new ReviewUpdateQuery();
+ query.setReviewId(reviewId);
+ query.setStatus(status);
+ return query;
+ }
+
public Long getReviewId() {
return reviewId;
}
return this;
}
+ public String getStatus() {
+ return status;
+ }
+
+ public ReviewUpdateQuery setStatus(String status) {
+ this.status = status;
+ return this;
+ }
+
@Override
public String getUrl() {
StringBuilder url = new StringBuilder();
appendUrlParameter(url, "new_text", getNewText());
appendUrlParameter(url, "assignee", getAssignee());
appendUrlParameter(url, "false_positive", getFalsePositive());
+ appendUrlParameter(url, "status", getStatus());
return url.toString();
}
assertThat(query.getUrl(), is("/api/reviews/?id=13&new_text=Hello+World%21&false_positive=true&"));
}
+ @Test
+ public void testChangeStatus() {
+ ReviewUpdateQuery query = ReviewUpdateQuery.changeStatusQuery(13L, "RESOLVED");
+ assertThat(query.getUrl(), is("/api/reviews/?id=13&status=RESOLVED&"));
+ }
+
}
\ No newline at end of file
projects / sonarqube.git / commitdiff
