diff options
author | Simon Brandhof <simon.brandhof@gmail.com> | 2011-12-14 16:53:02 +0100 |
---|---|---|
committer | Simon Brandhof <simon.brandhof@gmail.com> | 2011-12-14 16:53:41 +0100 |
commit | f9ff8517ceda7d8e6cd7bcff46b45f5bd262367f (patch) | |
tree | 54beb38ec79fdbbb2f1d127794b4ee5d6900af93 /sonar-server | |
parent | 6d4cab7c2d50b4498b871996cf600f1000ee95e2 (diff) | |
download | sonarqube-f9ff8517ceda7d8e6cd7bcff46b45f5bd262367f.tar.gz sonarqube-f9ff8517ceda7d8e6cd7bcff46b45f5bd262367f.zip |
SONAR-3086 improve the detail of a review
Diffstat (limited to 'sonar-server')
12 files changed, 182 insertions, 89 deletions
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb index 3b00af877f3..a52fd202bcf 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb @@ -62,6 +62,7 @@ class ReviewsController < ApplicationController # GET def assign_form + @review = Review.find(params[:id]) render :partial => "assign_form" end @@ -73,9 +74,15 @@ class ReviewsController < ApplicationController return end - assignee = findUserByLogin(params[:assignee_login]) unless params[:assignee_login].blank? - @review.reassign(current_user, assignee) + assignee = nil + if params[:me]=='true' + assignee = current_user + elsif params[:assignee_login].present? + assignee = findUserByLogin(params[:assignee_login]) + end + + @review.reassign(current_user, assignee, params) render :partial => 'reviews/view' end @@ -109,6 +116,7 @@ class ReviewsController < ApplicationController # GET def false_positive_form + @review = Review.find(params[:id]) render :partial => 'reviews/false_positive_form' end @@ -120,10 +128,7 @@ class ReviewsController < ApplicationController return end - unless params[:comment].blank? - @review.set_false_positive(params[:false_positive]=='true', current_user, params) - end - + @review.set_false_positive(params[:false_positive]=='true', current_user, params) render :partial => "reviews/view" end @@ -141,24 +146,46 @@ class ReviewsController < ApplicationController render :partial => "reviews/view" end + def change_status_form + @review = Review.find(params[:id]) + render :partial => 'reviews/change_status_form' + 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." + render :text => "<b>Cannot change the status</b> : access denied." return end if @review.resolved? - @review.reopen(current_user) + @review.reopen(current_user, params) else # for the moment, if a review is not open, it can only be "RESOLVED" - @review.resolve(current_user) + @review.resolve(current_user, params) end render :partial => "reviews/view" end + # GET + def change_severity_form + render :partial => 'reviews/change_severity_form' + end + + # POST + def change_severity + @review=Review.find(params[:id], :include => 'project') + unless has_rights_to_modify?(@review.project) + render :text => "<b>Cannot change severity</b> : access denied." + return + end + + @review.set_severity(params[:severity], current_user, params) + render :partial => "reviews/review" + end + # # @@ -318,14 +345,15 @@ class ReviewsController < ApplicationController end sanitize_violation(violation) - if violation.review - review = violation.review - if review.resolved? - review.reopen(current_user) - else - # for the moment, if a review is not open, it can only be "RESOLVED" - review.resolve(current_user) - end + if violation.review.nil? + violation.build_review(:user_id => current_user.id) + end + + if violation.review.resolved? + violation.review.reopen(current_user, params) + else + # for the moment, if a review is not open, it can only be "RESOLVED" + violation.review.resolve(current_user, params) end render :partial => "resource/violation", :locals => {:violation => violation} 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 db13179e545..d7153e5c122 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 @@ -28,6 +28,7 @@ class Review < ActiveRecord::Base validates_presence_of :user, :message => "can't be empty" validates_presence_of :status, :message => "can't be empty" + validates_inclusion_of :severity, :in => Severity::KEYS before_save :assign_project @@ -139,16 +140,22 @@ class Review < ActiveRecord::Base notification_manager.notifyChanged(id.to_i, current_user.login.to_java, old, to_java_map) end - def reopen(current_user) + def reopen(current_user, options={}) old = self.to_java_map + if options[:text].present? + comments.create!(:user => current_user, :text => options[:text]) + end self.status = STATUS_REOPENED self.resolution = nil self.save! notification_manager.notifyChanged(id.to_i, current_user.login.to_java, old, to_java_map) end - def resolve(current_user) + def resolve(current_user, options={}) old = self.to_java_map + if options[:text].present? + comments.create!(:user => current_user, :text => options[:text]) + end self.status = STATUS_RESOLVED self.resolution = RESOLUTION_FIXED self.save! diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/severity.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/severity.rb index 1df514e7091..f1ecf4ed1d8 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/models/severity.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/models/severity.rb @@ -27,4 +27,9 @@ module Severity BLOCKER = "BLOCKER" KEYS=[BLOCKER, CRITICAL, MAJOR, MINOR, INFO] + + def self.valid?(value) + KEYS.include?(value) + end end + 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 e29ac2a6cd4..049a2876e68 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 @@ -1,4 +1,6 @@ -<div id="vId<%= violation.id -%>" onmouseover="sVA(<%= violation.id -%>)" onmouseout="hVA(<%= violation.id -%>)"> +<div id="vId<%= violation.id -%>" + <% if current_user -%>onmouseover="sVA(<%= violation.id -%>)" onmouseout="hVA(<%= violation.id -%>)" +<% end -%>> <div class="violation"> <div class="vtitle"> <% if violation.review %> diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_assign_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_assign_form.html.erb index 06cf4dadea1..0c61e721d68 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_assign_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_assign_form.html.erb @@ -1,21 +1,43 @@ <% - assignee_check_script = "if ($('autocompleteText-assignee_login').value != '' && $('assignee_login').value == '') { alert($('autocompleteText-assignee_login').value + '" + message('reviews.user_does_not_exist') + "'); return false;}" + assignee_check_script = "if ($('autocompleteText-assignee_login').value != '' && $('assignee_login').value == '') { alert($('autocompleteText-assignee_login').value + '" + message('reviews.user_does_not_exist') + "'); return false;}" %> <form method="post" onsubmit="<%= assignee_check_script -%> new Ajax.Updater('review', '<%= url_for :action => 'assign' -%>', {asynchronous:true, evalScripts:true, parameters:Form.serialize(this)}); return false;"> - <%= hidden_field_tag :id, params[:review_id] -%> + <%= hidden_field_tag :id, params[:id] -%> + + <table class="width100"> + <tr> + <td style="vertical-align:top"> + <textarea id="actionText" rows="4" name="text" style="width: 100%"></textarea> + </td> + <td class="sep"></td> + <td style="vertical-align:top;width: 90px"> + <%= render :partial => 'markdown/help' -%> + </td> + </tr> + </table> + <%= user_autocomplete_field "assignee_login", "" -%> - - <%= submit_to_remote "submit_btn", message('assign'), - :url => { :action => 'assign' }, - :update => "review", - :before => assignee_check_script -%> - - <%= link_to_remote message('cancel'), - :url => { :action => 'show', :id => params[:review_id] }, - :update => "review" %> + <%= submit_to_remote "submit_btn", message('reviews.assign_submit'), + :url => {:action => 'assign'}, + :update => "review", + :before => assignee_check_script -%> + + <img src="<%= ApplicationController.root_context -%>/images/sep12.png"> + + <%= button_to_remote message('reviews.assign_to_me_submit'), + { + :url => {:action => 'assign', :id => params[:id], :me => true}, + :update => "review" + }, + :disabled => (@review.assignee_id==current_user.id) -%> + + + <%= link_to_remote message('cancel'), + :url => {:action => 'show', :id => params[:id]}, + :update => "review" %> <script> - $('autocompleteText-assignee_login').focus() + $('autocompleteText-assignee_login').focus(); </script> </form>
\ No newline at end of file diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_severity_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_severity_form.html.erb new file mode 100644 index 00000000000..12c378d1b1b --- /dev/null +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_severity_form.html.erb @@ -0,0 +1,15 @@ +<form method="POST"> + <input type="hidden" name="id" value="<%= params[:id] -%>"/> + + <%= message('reviews.new_severity_label') -%> + <select name="severity" class="withIcons" id="selectSeverity"> + <% Severity::KEYS.each do |severity| %> + <option class="sev_<%= severity -%>" value="<%= severity -%>" <%= 'selected' if severity==Severity::MAJOR -%>><%= message("severity.#{severity}") -%></option> + <% end %> + </select> + + <textarea id="actionText" rows="4" name="text" style="width: 100%"></textarea> + <%= submit_to_remote "submit_btn", message('reviews.change_severity_submit'), :url => {:action => 'change_severity'}, :html => {:id => "submit_btn"}, :update => 'review' -%> + + <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%> +</form> diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_status_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_status_form.html.erb new file mode 100644 index 00000000000..50658fb7369 --- /dev/null +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_status_form.html.erb @@ -0,0 +1,12 @@ +<form method="POST"> + <input type="hidden" name="id" value="<%= @review.id -%>"/> + + <textarea id="actionText" rows="4" name="text" style="width: 100%"></textarea> + <%= submit_to_remote "submit_btn", + message(@review.resolved? ? 'reviews.reopen' : 'reviews.resolve'), + :url => {:action => 'change_status'}, + :html => {:id => "submit_btn"}, + :update => 'review' -%> + + <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%> +</form> diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_comment_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_comment_form.html.erb index 92067818d04..c470bfecc5a 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_comment_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_comment_form.html.erb @@ -1,5 +1,5 @@ <% - button=(@comment ? message('update_comment') : message('add_comment')) + button=(@comment ? message('reviews.update_comment_submit') : message('reviews.comment_submit')) %> <form method="POST" action="save_comment"> <input type="hidden" name="id" value="<%= params[:id] -%>"/> @@ -10,11 +10,7 @@ <table class="width100"> <tr> <td style="vertical-align:top"> - <textarea id="commentText" rows="8" name="text" style="width: 100%" onkeyup="if (this.value=='') $('submit_btn').disabled='true'; else $('submit_btn').disabled='';"><%= @comment.text if @comment -%></textarea> - <br/> - <%= submit_to_remote "submit_btn", button, :url => { :action => 'save_comment'}, :html => { :id => "submit_btn", :disabled => "true" }, :update => 'review' -%> - - <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%> + <textarea id="commentText" rows="4" name="text" style="width: 100%" onkeyup="if (this.value=='') $('submit_btn').disabled='true'; else $('submit_btn').disabled='';"><%= @comment.text if @comment -%></textarea> </td> <td class="sep"></td> <td style="vertical-align:top;width: 90px"> @@ -22,4 +18,8 @@ </td> </tr> </table> + + <%= submit_to_remote "submit_btn", button, :url => {:action => 'save_comment'}, :html => {:id => "submit_btn", :disabled => "true"}, :update => 'review' -%> + + <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%> </form> diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_false_positive_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_false_positive_form.html.erb index 3a2befa2df4..0fa542e5a95 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_false_positive_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_false_positive_form.html.erb @@ -1,20 +1,19 @@ <% - if params[:false_positive]=='true' - title = message('reviews.why_false_positive') - button = message('reviews.flag_as_false_positive') - else + if @review.violation.switched_off? title = message('reviews.why_not_false_positive') button = message('reviews.unflag_as_false_positive') + else + title = message('reviews.why_false_positive') + button = message('reviews.flag_as_false_positive') end %> -<form method="POST" action="violation_flag_as_false_positive"> +<form method="POST"> <input type="hidden" name="id" value="<%= params[:id] -%>"/> - <% if params[:false_positive]=='true' -%> - <input type="hidden" name="false_positive" value="true"/> - <% end %> + <input type="hidden" name="false_positive" value="<%= @review.violation.switched_off? ? 'false' : 'true' -%>"/> + <h3><%= title -%></h3> - <textarea id="commentText" rows="8" name="comment" style="width: 100%" onkeyup="if (this.value=='') $('submit_btn').disabled='true'; else $('submit_btn').disabled='';"></textarea> - <%= submit_to_remote "submit_btn", button, :url => { :action => 'flag_as_false_positive' }, :html => { :id => "submit_btn", :disabled => "true" }, :update => 'review' -%> + <textarea id="actionText" rows="4" name="text" style="width: 100%"></textarea> + <%= submit_to_remote "submit_btn", button, :url => {:action => 'flag_as_false_positive'}, :html => {:id => "submit_btn"}, :update => 'review' -%> <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%> </form> 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 0996dce19d8..0288d74b0cb 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 @@ -1,58 +1,65 @@ <div id="rev_<%= review.id -%>"> <div class="reviewTitle"> <h2><%= message('reviews.review_number', :params => h(review.id.to_s)) -%> - <%= h(review.title) -%></h2> - <% if review.false_positive %> - <%= image_tag("sep12.png") -%> - - <span class="falsePositive"><%= message('false_positive') -%></span> - <% end %> - </div> <% if current_user && !review.closed? && review.rule_failure violation_switched_off = review.rule_failure.switched_off? %> - <div class="reviewActions" id="rActions"> + <div class="marginbottom10" id="actionButtons"> + <% unless review.resolved? %> + <%= button_to_remote message('reviews.assign'), + :url => {:controller => "reviews", :action => "assign_form", :id => review.id}, + :update => "actionForm", + :complete => "$('actionButtons').remove();$('actionForm').show();" -%> + + <% end %> + <% if !violation_switched_off %> - <%= button_to_remote (review.resolved? ? message('reopen') : message('resolve')), - :url => {:controller => "reviews", :action => "change_status", :id => review.id}, - :update => "review", - :confirm => review.resolved? ? message('reviews.do_you_want_to_reopen') : message('reviews.do_you_want_to_resolve') -%> + <%= button_to_remote (review.resolved? ? message('reviews.reopen') : message('reviews.resolve')), + :url => {:controller => "reviews", :action => "change_status_form", :id => review.id}, + :update => "actionForm", + :complete => "$('actionButtons').remove();$('actionForm').show();$('actionText').focus();" -%> - <% unless review.resolved? %> - <%= button_to_remote message('assign'), - :url => {:controller => "reviews", :action => "assign_form", :review_id => review.id}, - :update => "assignForm", - :complete => "$('rActions').hide(); $('editActions').hide(); $('assignee_login').focus();" -%> - <% end %> <% end %> <% if review.can_change_false_positive_flag? %> <%= button_to_remote (violation_switched_off ? message('reviews.unflag_as_false_positive') : message('reviews.flag_as_false_positive')), :url => {:controller => "reviews", :action => "false_positive_form", :id => review.id, :false_positive => !violation_switched_off}, - :update => "reviewForm", - :complete => "$('reviewForm').show();$('commentText').focus();" -%> + :update => "actionForm", + :complete => "$('actionButtons').remove();$('actionForm').show();$('actionText').focus();" -%> + <% end %> + + <% unless review.resolved? %> + <%= button_to_remote message('reviews.change_severity'), + :url => {:controller => "reviews", :action => "change_severity_form", :id => review.id}, + :update => "actionForm", + :complete => "$('actionButtons').remove();$('actionForm').show();$('selectSeverity').focus();" -%> <% end %> </div> <% end %> + <div class="discussionComment" id="actionForm" style="border: 1px solid #DDD;display:none"></div> - <table class="reviewDetails"> + <table class="reviewDetails marginbottom10"> <tr> <td class="key"> <%= message('status') -%>: </td> <td class="val"> <%= image_tag "status/#{review.status}.png" -%> <%= message(review.status.downcase).capitalize -%> + <% if @review.violation.switched_off? %> + <span class="falsePositive">(<%= message('false_positive') -%>)</span> + <% end %> </td> <td class="key"> <%= message('severity') -%>: </td> <td class="val"> - <%= image_tag "priority/#{review.severity}.png" -%> <%= message(review.severity.downcase).capitalize -%> + <%= image_tag "priority/#{review.severity}.png" -%> <%= message("severity.#{review.severity}") -%> </td> </tr> <tr> @@ -60,9 +67,7 @@ <%= message('assignee') -%>: </td> <td class="val"> - <span id="assignForm"> - <%= review.assignee ? h(review.assignee.name) : '-' -%> - </span> + <%= review.assignee ? h(review.assignee.name) : '-' -%> </td> <td class="key"> <%= message('author') -%>: @@ -114,7 +119,7 @@ </div> <% end %> - <div class="discussion"> + <div class="discussion marginbottom10"> <% if review.rule_failure %> <div class="discussionComment first"> <%= h(review.rule_failure.message) -%> @@ -138,7 +143,7 @@ <%= link_to_remote message('edit'), :url => {:controller => "reviews", :action => "comment_form", :comment_id => comment.id, :id => review.id}, :update => "lastComment", - :complete => "$('rActions').hide();$('editActions').hide();$('commentText').focus();" -%> + :complete => "$('commentAction').remove();$('editActions').hide();$('commentText').focus();" -%> <% unless comment_index == 0 %> <%= link_to_remote message('delete'), @@ -163,12 +168,15 @@ <% end %> </div> + <% if current_user %> + <%= button_to_remote message('reviews.comment'), + { + :url => {:controller => "reviews", :action => "comment_form", :id => review.id}, + :update => "commentForm", + :complete => "$('commentAction').remove();$('commentForm').show();$('commentText').focus();" + }, + :id => 'commentAction' -%> - <%= button_to_remote message('add_comment'), - :url => {:controller => "reviews", :action => "comment_form", :id => review.id}, - :update => "reviewForm", - :complete => "$('rActions').hide();$('editActions').hide();$('reviewForm').show();$('commentText').focus();" -%> - - <div class="discussionComment" id="reviewForm" style="display:none"></div> - + <div class="discussionComment" id="commentForm" style="border: 1px solid #DDD;display:none"></div> + <% end %> </div>
\ No newline at end of file diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_violation_false_positive_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_violation_false_positive_form.html.erb index 534e69ad0d3..2cebbd3ffad 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_violation_false_positive_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_violation_false_positive_form.html.erb @@ -1,10 +1,10 @@ <% if @violation.switched_off? title = message('reviews.why_not_false_positive') - button = message('reviews.unflag_as_false_positive') + button = message('reviews.unflag_as_false_positive_submit') else title = message('reviews.why_false_positive') - button = message('reviews.flag_as_false_positive') + button = message('reviews.flag_as_false_positive_submit') end %> <form method="POST" action="violation_flag_as_false_positive"> diff --git a/sonar-server/src/main/webapp/stylesheets/style.css b/sonar-server/src/main/webapp/stylesheets/style.css index 2ede539bbab..30c3e7f6cb5 100644 --- a/sonar-server/src/main/webapp/stylesheets/style.css +++ b/sonar-server/src/main/webapp/stylesheets/style.css @@ -1137,6 +1137,7 @@ div.reviewTitle { color: #4B9FD5; line-height: 2.2em; margin: 0; + margin-bottom: 10px; padding: 0 10px; border: 1px solid #4B9FD5; } @@ -1144,7 +1145,6 @@ div.reviewTitle { div.reviewTitle h2 { color: #4183C4; font-weight: bold; - display: inline; font-size: 100%; text-shadow: 0 1px 0 #FFFFFF; } @@ -1156,7 +1156,6 @@ div.reviewTitle span.actions { table.reviewDetails { width: 100%; border: 0; - margin: 10px 0; } table.reviewDetails td { @@ -1175,10 +1174,6 @@ table.reviewDetails td img { vertical-align: bottom; } -div.reviewActions { - padding: 10px; -} - div.discussion { width: 100%; border: 1px solid #DDDDDD; |