Prettified the rendering of merge request status changes in comments and events

9 files changed, 56 insertions, 27 deletions

diff --git a/app/helpers/event_rendering_helper.rb b/app/helpers/event_rendering_helper.rb
index ac49d48..9d4a22c 100644
--- a/app/helpers/event_rendering_helper.rb
+++ b/app/helpers/event_rendering_helper.rb
@@ -245,7 +245,7 @@ module EventRenderingHelper
     action = action_for_event(:event_updated_merge_request) do
       link_to(h(target_repository.url_path) + "/" + h("merge request ##{event.target.to_param}"),
         repo_owner_path(target_repository, :project_repository_merge_request_path, project, target_repository, event.target)) +
-      event.data
+      "<div class=\"meta_body\">&#x2192; " + event.data + "</div>"
     end
     body = truncate(h(event.body), :length => 100) 
     category = "merge_request"
diff --git a/app/models/comment.rb b/app/models/comment.rb
index 4d2ffae..8e64fff 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -58,13 +58,14 @@ class Comment < ActiveRecord::Base
     message.save
   end
   
-  def state=(s)
-    return if s.blank?
+  def state=(new_state)
+    return if new_state.blank?
     result = []
     if applies_to_merge_request?
+      return if target.status_tag == new_state
       result << target.status_tag
     end
-    result << s
+    result << new_state
     self.state_change = result
   end
   
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index ea3203d..5b389a5 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -194,9 +194,10 @@ class MergeRequest < ActiveRecord::Base
   def create_status_change_event(comment)
     if @current_user
       message = "State changed "
-      message << "from #{@previous_state} " if @previous_state
-      message << "to #{status_tag}"
-      target_repository.project.create_event(Action::UPDATE_MERGE_REQUEST, self, @current_user, message, comment)
+      message << "from <span class=\"changed\">#{@previous_state}</span> " if @previous_state
+      message << "to <span class=\"changed\">#{status_tag}</span>"
+      target_repository.project.create_event(Action::UPDATE_MERGE_REQUEST, self,
+        @current_user, message, comment)
     end
   end
   
@@ -491,7 +492,7 @@ class MergeRequest < ActiveRecord::Base
       tracking_repository.full_repository_path, branch_spec)
     create_new_version
     target_repository.project.create_event(Action::UPDATE_MERGE_REQUEST, self,
-      user, "new version #{current_version_number}", "reason")
+      user, "new version #{current_version_number}")
   end
 
   def tracking_repository
diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb
index 15c1c59..6b8518f 100644
--- a/app/views/comments/_comment.html.erb
+++ b/app/views/comments/_comment.html.erb
@@ -22,6 +22,13 @@
 
 <div class="comment">
   <a name="<%= dom_id(comment) -%>"></a>
+  <%- if comment.state_change -%>
+  <p class="meta_body">
+    &#x2192; State changed <%- if comment.state_changed_from -%>from 
+    <span class="changed"><%= comment.state_changed_from %></span><%- end -%> to
+    <span class="changed"><%= comment.state_changed_to %></span>
+  </p>
+  <%- end -%>
   <div class="body"><%= render_markdown(comment.body, :auto_link) -%></div>
   <p class="byline">
     <span class="avatar">
@@ -30,9 +37,4 @@
     <%= link_to(h(comment.user.title), comment.user) -%> | 
     <span class="permalink"><%= link_to time_ago_in_words(comment.created_at) + " ago", "##{dom_id(comment)}" -%></span>
   </p>
-  <%- if comment.state_change -%>
-  <p>
-    State changed <%- if comment.state_changed_from -%>from <strong><%= comment.state_changed_from %></strong><%- end -%> to <strong><%= comment.state_changed_to %></strong>
-  </p>
-  <%- end -%>
 </div>
diff --git a/app/views/comments/_form.html.erb b/app/views/comments/_form.html.erb
index 97df2ab..10bf66e 100644
--- a/app/views/comments/_form.html.erb
+++ b/app/views/comments/_form.html.erb
@@ -49,17 +49,18 @@
     <%- if MergeRequest === parent && (logged_in? && parent.resolvable_by?(current_user)) -%>
     <p>
       <%= f.label :state, 'Status' %><br />
-      <%- if parent.target_repository.project.admin?(current_user) -%>
-        <span class="hint">As an administrator, you may <%= link_to('customize the available statuses', edit_project_path(parent.target_repository.project)) -%> for this project</span><br />
-      <%- else -%>
-        <span class="hint">Administrators for the project can customize the available statuses</span><br />
-      <%- end -%>
-      <%= f.select :state, parent.target_repository.project.merge_request_state_list, :include_blank => true -%>
+      <%= f.select :state, parent.target_repository.project.merge_request_state_list,
+           :selected => parent.status_tag -%>
     </p>
     <%- end -%>
     <p>
       <%= button_to_remote("Preview", :url => repo_owner_path(@repository, [@project, @repository, :comments, :preview]), :method => :post, :with => 'Form.serialize(this.form)') %>
-      <%= f.submit t("views.comments.add") -%>
+      <%- if MergeRequest === parent && (logged_in? &&
+      parent.resolvable_by?(current_user)) -%>
+        <%= f.submit t("views.comments.update_or_add") -%>
+      <% else -%>
+        <%= f.submit t("views.comments.add") -%>
+      <% end -%>
     </p>
   <% end -%>
 <% else -%>
diff --git a/config/locales/en.rb b/config/locales/en.rb
index 7e74806..12cf31b 100644
--- a/config/locales/en.rb
+++ b/config/locales/en.rb
@@ -287,6 +287,7 @@
         :add_title => "Add a new comment",
         :body => "Comment",
         :add => "Add Comment",
+        :update_or_add => "Update / Add Comment",
         :page_title => "Comments in {{repo}}",
         :diff => "Commit diff",
         :total => "Comments ({{total}})",
diff --git a/public/stylesheets/base.css b/public/stylesheets/base.css
index afcdb3f..bf56c16 100644
--- a/public/stylesheets/base.css
+++ b/public/stylesheets/base.css
@@ -2014,6 +2014,15 @@ table.select_commits_from_list tr.commit_comments .commit-details .permalink a {
   margin: 0 100px 20px 105px;
 }
 
+.events .event_instance .event_meta .meta_body, .comments .comment .meta_body {
+    font-size: 12px;
+}
+.events .event_instance .event_meta .meta_body span.changed, 
+.comments .comment .meta_body span.changed {
+    background: #E0EFCF;
+    padding: 1px 2px 1px 2px;
+}
+
 .events.inline .event_instance .event_meta {
   margin: 0 0px 20px 95px;
   text-align: left;
diff --git a/test/unit/comment_test.rb b/test/unit/comment_test.rb
index 9ee4d0b..62c5b67 100644
--- a/test/unit/comment_test.rb
+++ b/test/unit/comment_test.rb
@@ -99,11 +99,23 @@ class CommentTest < ActiveSupport::TestCase
 
     should "not require a body if state changes" do
       @merge_request = merge_requests(:moes_to_johans_open)
-      @comment = @merge_request.comments.new(:project => projects(:johans), :user => users(:moe))
-      assert @comment.body_required?
-      @comment.state = "Foo"
-      assert !@comment.body_required?
-      assert @comment.save, "Should not require a body when state changes"
+      comment = @merge_request.comments.new(:project => projects(:johans),
+        :user => users(:moe))
+      assert comment.body_required?
+      comment.state = "Foo"
+      assert !comment.body_required?
+      assert comment.save, "Should not require a body when state changes"
+    end
+
+    should "not update the state change if the previous state is the same" do
+      @merge_request = merge_requests(:moes_to_johans_open)
+      @merge_request.status_tag = "Foo"
+      comment = @merge_request.comments.new(:project => projects(:johans),
+        :user => users(:moe), :body => "just a comment")
+      comment.state = "Foo"
+      comment.save!
+      assert_nil comment.reload.state_changed_from
+      assert_nil comment.state_changed_to
     end
   end
   
diff --git a/test/unit/merge_request_test.rb b/test/unit/merge_request_test.rb
index 591fd68..038f4a8 100644
--- a/test/unit/merge_request_test.rb
+++ b/test/unit/merge_request_test.rb
@@ -586,7 +586,9 @@ class MergeRequestTest < ActiveSupport::TestCase
         @merge_request.status_tag = 'after'
         @merge_request.create_status_change_event("Foo")
         event = @merge_request.events.reload.last
-        assert_equal "State changed from before to after", event.data
+        exp ="State changed from <span class=\"changed\">before</span> " +
+          "to <span class=\"changed\">after</span>"
+        assert_equal exp, event.data
       end
     end
 
@@ -596,7 +598,7 @@ class MergeRequestTest < ActiveSupport::TestCase
         @merge_request.status_tag = "merged"
         @merge_request.create_status_change_event("Setting this to merged")
         event = @merge_request.events.reload.last
-        assert_equal "State changed to merged", event.data
+        assert_equal "State changed to <span class=\"changed\">merged</span>", event.data
       end
     end