Merge branch 'comment-updated-by' into 'master'

Show who last edited a comment if it wasn't the original author

Fixes #1796.

The `updated_by` user is also tracked for issues and merge requests, but it isn't currently shown, because "edited by Person" would give the idea that Person edited the description, while most updates to those are assignee/milestone/labels, in which case who made that change is already visible in the comments.

See merge request !1075

16 files changed, 52 insertions, 66 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 97d8ef55628..973425295c1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -29,6 +29,7 @@ v 7.14.0 (unreleased)
   - Add project star and fork count, group avatar URL and user/group web URL attributes to API
   - Fix bug causing Bitbucket importer to crash when OAuth application had been removed.
   - Add fetch command to the MR page.
+  - Show who last edited a comment if it wasn't the original author
   - Add ability to manage user email addresses via the API.
   - Show buttons to add license, changelog and contribution guide if they're missing.
   - Tweak project page buttons.
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index c4a87e9dbd8..0f5d82ce133 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -30,13 +30,10 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def update
-    if note.editable?
-      note.update_attributes(note_params)
-      note.reset_events_cache
-    end
+    @note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
 
     respond_to do |format|
-      format.json { render_note_json(note) }
+      format.json { render_note_json(@note) }
       format.html { redirect_to :back }
     end
   end
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index d4c345fe431..6ddb37cd0dc 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -43,21 +43,6 @@ module IssuesHelper
     end
   end
 
-  def issue_timestamp(issue)
-    # Shows the created at time and the updated at time if different
-    ts = time_ago_with_tooltip(issue.created_at, placement: 'bottom', html_class: 'note_created_ago')
-    if issue.updated_at != issue.created_at
-      ts << capture_haml do
-        haml_tag :span do
-          haml_concat '&middot;'
-          haml_concat icon('edit', title: 'edited')
-          haml_concat time_ago_with_tooltip(issue.updated_at, placement: 'bottom', html_class: 'issue_edited_ago')
-        end
-      end
-    end
-    ts.html_safe
-  end
-
   def bulk_update_milestone_options
     options_for_select([['None (backlog)', -1]]) +
         options_from_collection_for_select(project_active_milestones, 'id',
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index dda9b17d61d..5f0c921413a 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -23,21 +23,6 @@ module NotesHelper
     end
   end
 
-  def note_timestamp(note)
-    # Shows the created at time and the updated at time if different
-    ts = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note_created_ago')
-    if note.updated_at != note.created_at
-      ts << capture_haml do
-        haml_tag :span do
-          haml_concat '&middot;'
-          haml_concat icon('edit', title: 'edited')
-          haml_concat time_ago_with_tooltip(note.updated_at, placement: 'bottom', html_class: 'note_edited_ago')
-        end
-      end
-    end
-    ts.html_safe
-  end
-
   def noteable_json(noteable)
     {
       id: noteable.id,
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index f5b78533896..525a46291f6 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -21,7 +21,7 @@ module ProjectsHelper
   end
 
   def link_to_member(project, author, opts = {})
-    default_opts = { avatar: true, name: true, size: 16 }
+    default_opts = { avatar: true, name: true, size: 16, author_class: 'author' }
     opts = default_opts.merge(opts)
 
     return "(deleted)" unless author
@@ -32,7 +32,7 @@ module ProjectsHelper
     author_html << image_tag(avatar_icon(author.try(:email), opts[:size]), width: opts[:size], class: "avatar avatar-inline #{"s#{opts[:size]}" if opts[:size]}", alt:'') if opts[:avatar]
 
     # Build name span tag
-    author_html << content_tag(:span, sanitize(author.name), class: 'author') if opts[:name]
+    author_html << content_tag(:span, sanitize(author.name), class: opts[:author_class]) if opts[:name]
 
     author_html = author_html.html_safe
 
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index c21e7fd0e3b..40642dc63ba 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -12,6 +12,7 @@ module Issuable
   included do
     belongs_to :author, class_name: "User"
     belongs_to :assignee, class_name: "User"
+    belongs_to :updated_by, class_name: "User"
     belongs_to :milestone
     has_many :notes, as: :noteable, dependent: :destroy
     has_many :label_links, as: :target, dependent: :destroy
diff --git a/app/models/note.rb b/app/models/note.rb
index 2362e50276e..a99d428b02d 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -33,6 +33,7 @@ class Note < ActiveRecord::Base
   belongs_to :project
   belongs_to :noteable, polymorphic: true
   belongs_to :author, class_name: "User"
+  belongs_to :updated_by, class_name: "User"
 
   delegate :name, to: :project, prefix: true
   delegate :name, :email, to: :author, prefix: true
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index eabab65c9b0..2fc6ef7f356 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -14,7 +14,7 @@ module Issues
       filter_params
       old_labels = issue.labels.to_a
 
-      if params.present? && issue.update_attributes(params)
+      if params.present? && issue.update_attributes(params.merge(updated_by: current_user))
         issue.reset_events_cache
 
         if issue.labels != old_labels
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 589fad16165..25d79e22e39 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -24,7 +24,7 @@ module MergeRequests
       filter_params
       old_labels = merge_request.labels.to_a
 
-      if params.present? && merge_request.update_attributes(params)
+      if params.present? && merge_request.update_attributes(params.merge(updated_by: current_user))
         merge_request.reset_events_cache
 
         if merge_request.labels != old_labels
diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb
index b5611d46257..c22a9333ef6 100644
--- a/app/services/notes/update_service.rb
+++ b/app/services/notes/update_service.rb
@@ -1,22 +1,11 @@
 module Notes
   class UpdateService < BaseService
-    def execute
-      note = project.notes.find(params[:note_id])
-      note.note = params[:note]
-      if note.save
-        notification_service.new_note(note)
+    def execute(note)
+      return note unless note.editable?
 
-        # Skip system notes, like status changes and cross-references.
-        unless note.system
-          event_service.leave_note(note, note.author)
+      note.update_attributes(params.merge(updated_by: current_user))
 
-          # Create a cross-reference note if this Note contains GFM that
-          # names an issue, merge request, or commit.
-          note.references.each do |mentioned|
-            SystemNoteService.cross_reference(mentioned, note.noteable, note.author)
-          end
-        end
-      end
+      note.reset_events_cache
 
       note
     end
diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml
index 54d33a5ddd1..e7b14e7582c 100644
--- a/app/views/projects/issues/show.html.haml
+++ b/app/views/projects/issues/show.html.haml
@@ -9,7 +9,13 @@
           Open
       Issue ##{@issue.iid}
       %small.creator
-        &middot; created by #{link_to_member(@project, @issue.author)} #{issue_timestamp(@issue)}
+        &middot; created by #{link_to_member(@project, @issue.author)}
+        = time_ago_with_tooltip(@issue.created_at, placement: 'bottom', html_class: 'issue_created_ago')
+        - if @issue.updated_at != @issue.created_at
+          %span
+            &middot;
+            = icon('edit', title: 'edited')
+            = time_ago_with_tooltip(@issue.updated_at, placement: 'bottom', html_class: 'issue_edited_ago')
 
       .pull-right
         - if can?(current_user, :create_issue, @project)
diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml
index 4e8144b4de2..9a1eb36fc88 100644
--- a/app/views/projects/merge_requests/show/_mr_title.html.haml
+++ b/app/views/projects/merge_requests/show/_mr_title.html.haml
@@ -1,10 +1,16 @@
 %h4.page-title
   .issue-box{ class: issue_box_class(@merge_request) }
     = @merge_request.state_human_name
-  = "Merge Request ##{@merge_request.iid}"
+  Merge Request ##{@merge_request.iid}
   %small.creator
     &middot;
-    created by #{link_to_member(@project, @merge_request.author)} #{time_ago_with_tooltip(@merge_request.created_at)}
+    created by #{link_to_member(@project, @merge_request.author)}
+    = time_ago_with_tooltip(@merge_request.created_at)
+    - if @merge_request.updated_at != @merge_request.created_at
+      %span
+        &middot;
+        = icon('edit', title: 'edited')
+        = time_ago_with_tooltip(@merge_request.updated_at, placement: 'bottom')
 
   .issue-btn-group.pull-right
     - if can?(current_user, :update_merge_request, @merge_request)
diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml
index 4a1009686c6..de75d44fc41 100644
--- a/app/views/projects/notes/_note.html.haml
+++ b/app/views/projects/notes/_note.html.haml
@@ -33,7 +33,14 @@
 
         %span.note-last-update
           = link_to "##{dom_id(note)}", name: dom_id(note), title: "Link here" do
-            = note_timestamp(note)
+            = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note_created_ago')
+          - if note.updated_at != note.created_at
+            %span
+              &middot;
+              = icon('edit', title: 'edited')
+              = time_ago_with_tooltip(note.updated_at, placement: 'bottom', html_class: 'note_edited_ago')
+              - if note.updated_by && note.updated_by != note.author
+                by #{link_to_member(note.project, note.updated_by, avatar: false, author_class: nil)}
 
         - if note.superceded?(@notes)
           - if note.upvote?
diff --git a/db/migrate/20150730122406_add_updated_by_to_issuables_and_notes.rb b/db/migrate/20150730122406_add_updated_by_to_issuables_and_notes.rb
new file mode 100644
index 00000000000..78d45c7f96b
--- /dev/null
+++ b/db/migrate/20150730122406_add_updated_by_to_issuables_and_notes.rb
@@ -0,0 +1,7 @@
+class AddUpdatedByToIssuablesAndNotes < ActiveRecord::Migration
+  def change
+    add_column :notes, :updated_by_id, :integer
+    add_column :issues, :updated_by_id, :integer
+    add_column :merge_requests, :updated_by_id, :integer
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index af10a2ff7cd..6e919f2883b 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -136,12 +136,13 @@ ActiveRecord::Schema.define(version: 20150806104937) do
     t.integer  "project_id"
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.integer  "position",     default: 0
+    t.integer  "position",      default: 0
     t.string   "branch_name"
     t.text     "description"
     t.integer  "milestone_id"
     t.string   "state"
     t.integer  "iid"
+    t.integer  "updated_by_id"
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -238,6 +239,7 @@ ActiveRecord::Schema.define(version: 20150806104937) do
     t.text     "description"
     t.integer  "position",          default: 0
     t.datetime "locked_at"
+    t.integer  "updated_by_id"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -297,6 +299,7 @@ ActiveRecord::Schema.define(version: 20150806104937) do
     t.integer  "noteable_id"
     t.boolean  "system",        default: false, null: false
     t.text     "st_diff"
+    t.integer  "updated_by_id"
   end
 
   add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 3726be7c537..3efdfe2d46e 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -78,17 +78,15 @@ module API
         put ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do
           required_attributes! [:body]
 
-          authorize! :admin_note, user_project.notes.find(params[:note_id])
+          note = user_project.notes.find(params[:note_id])
+
+          authorize! :admin_note, note
 
           opts = {
-            note: params[:body],
-            note_id: params[:note_id],
-            noteable_type: noteables_str.classify,
-            noteable_id: params[noteable_id_str]
+            note: params[:body]
           }
 
-          @note = ::Notes::UpdateService.new(user_project, current_user,
-                                             opts).execute
+          @note = ::Notes::UpdateService.new(user_project, current_user, opts).execute(note)
 
           if @note.valid?
             present @note, with: Entities::Note