]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-3086 improve the detail of a review
authorSimon Brandhof <simon.brandhof@gmail.com>
Wed, 14 Dec 2011 15:53:02 +0000 (16:53 +0100)
committerSimon Brandhof <simon.brandhof@gmail.com>
Wed, 14 Dec 2011 15:53:41 +0000 (16:53 +0100)
13 files changed:
plugins/sonar-l10n-en-plugin/src/main/resources/org/sonar/l10n/core.properties
sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb
sonar-server/src/main/webapp/WEB-INF/app/models/review.rb
sonar-server/src/main/webapp/WEB-INF/app/models/severity.rb
sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_assign_form.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_severity_form.html.erb [new file with mode: 0644]
sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_change_status_form.html.erb [new file with mode: 0644]
sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_comment_form.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_false_positive_form.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_review.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/reviews/_violation_false_positive_form.html.erb
sonar-server/src/main/webapp/stylesheets/style.css

index c9da7b551aa3c4b1354f6e3b6db69321ae6e5d99..27f39d567fc190ee0d66d64350fc73f88bcf5bcc 100644 (file)
@@ -369,10 +369,10 @@ filters.col.key=Key
 #------------------------------------------------------------------------------
 
 reviews.review_number=Review #{0}
-reviews.do_you_want_to_reopen=Do you want to reopen this review?
-reviews.do_you_want_to_resolve=Do you want to resolve this review?
 reviews.flag_as_false_positive=False-positive
 reviews.unflag_as_false_positive=Not false-positive
+reviews.flag_as_false_positive_submit=False-positive
+reviews.unflag_as_false_positive_submit=Not false-positive
 reviews.do_you_want_to_delete_comment=Do you want to delete this comment?
 reviews.only_false_positives=Only false positives
 reviews.without_false_positives=Without false positives
index 3b00af877f326b109b0eeeddc161d7cb360dd031..a52fd202bcff04224899250276785bb5b9ad7039 100644 (file)
@@ -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}
index db13179e54554b73df0056bfe77e268d86374c2d..d7153e5c122559f7cea85c3d342e9ff9b271b183 100644 (file)
@@ -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!
index 1df514e7091d2f1fa9839f74faf8f1b4a13ae98f..f1ecf4ed1d8ff81925a1370e88f2049ffa0cb39f 100644 (file)
@@ -27,4 +27,9 @@ module Severity
   BLOCKER = "BLOCKER"
 
   KEYS=[BLOCKER, CRITICAL, MAJOR, MINOR, INFO]
+
+  def self.valid?(value)
+    KEYS.include?(value)
+  end
 end
+
index e29ac2a6cd4dd58acbe7042bbc6e52cfd1773a6d..049a2876e6882214d640222c6184b9f8f1ff2cb1 100644 (file)
@@ -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 %>
index 06cf4dadea1b7f083bc2f02c3c1ddf9c8d94b04a..0c61e721d68cab7b0040610674d6b8b1f07eee91 100644 (file)
@@ -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", "" -%>
-  &nbsp;&nbsp;
-  <%= submit_to_remote "submit_btn", message('assign'), 
-         :url => { :action => 'assign' },
-         :update => "review",
-         :before => assignee_check_script -%>
-  &nbsp;&nbsp;
-  <%= 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 -%>
+  &nbsp;
+  <img src="<%= ApplicationController.root_context -%>/images/sep12.png">
+  &nbsp;
+  <%= 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) -%>
+
+  &nbsp;
+  <%= 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 (file)
index 0000000..12c378d
--- /dev/null
@@ -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' -%>
+  &nbsp;
+  <%= 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 (file)
index 0000000..50658fb
--- /dev/null
@@ -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' -%>
+  &nbsp;
+  <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%>
+</form>
index 92067818d041859f9263a7430cfe026334dac04f..c470bfecc5a3f7d38e5dba3e06176245735a4eae 100644 (file)
@@ -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] -%>"/>
   <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' -%>
-        &nbsp;
-        <%= 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' -%>
+  &nbsp;
+  <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%>
 </form>
index 3a2befa2df4d3ca9dea17aea3a430fb48e653c30..0fa542e5a9594e4c63712b0b80e3f4e6f0f76867 100644 (file)
@@ -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' -%>
   &nbsp;
   <%= link_to_remote message('cancel'), :url => {:action => 'show', :id => params[:id]}, :update => 'review' -%>
 </form>
index 0996dce19d8bed49111644ba5388f5eb193b0d1c..0288d74b0cb6815c7d6f55383d74c2d907bfc24f 100644 (file)
@@ -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") -%>
-      &nbsp;
-      <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') -%>:
     </div>
   <% end %>
 
-  <div class="discussion">
+  <div class="discussion marginbottom10">
     <% if review.rule_failure %>
       <div class="discussionComment first">
         <%= h(review.rule_failure.message) -%>
                <%= 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 %>
                &nbsp;
                  <%= link_to_remote message('delete'),
     <% 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
index 534e69ad0d3dd442ca4ca4cfb2d640ff176f292f..2cebbd3ffad3e35a0f96ec880b9990130e5ea75b 100644 (file)
@@ -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">
index 2ede539bbabf4b96d56a639cc1b6d0420a083751..30c3e7f6cb5c8b8fa4581707366c8b63be49d633 100644 (file)
@@ -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;