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

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.

app/controllers/projects/notes_controller.rb

@@ -30,13 +30,10 @@ def create
   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

app/helpers/issues_helper.rb

@@ -43,21 +43,6 @@ def url_for_issue(issue_iid, project = @project, options = {})
     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',

app/helpers/notes_helper.rb

@@ -23,21 +23,6 @@ def link_to_commit_diff_line_note(note)
     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,

app/helpers/projects_helper.rb

@@ -21,7 +21,7 @@ def link_to_project(project)
   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 @@ def link_to_member(project, author, opts = {})
     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

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

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

app/services/issues/update_service.rb

@@ -14,7 +14,7 @@ def execute(issue)
       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

app/services/merge_requests/update_service.rb

@@ -24,7 +24,7 @@ def execute(merge_request)
       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

app/services/notes/update_service.rb

 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)
-        # Skip system notes, like status changes and cross-references.
-        unless note.system
-          event_service.leave_note(note, note.author)
-          # 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
+    def execute(note)
+      return note unless note.editable?
+      note.update_attributes(params.merge(updated_by: current_user))
+      note.reset_events_cache
       note
     end

app/views/projects/issues/show.html.haml

@@ -9,7 +9,13 @@
           Open
       Issue ##{@issue.iid}
       %small.creator
-        · created by #{link_to_member(@project, @issue.author)} #{issue_timestamp(@issue)}
+        · 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
+            ·
+            = 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)

app/views/projects/merge_requests/show/_mr_title.html.haml

 %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
     ·
-    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
+        ·
+        = 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)

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
+              ·
+              = 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?

db/migrate/20150730122406_add_updated_by_to_issuables_and_notes.rb

+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

db/schema.rb

@@ -136,12 +136,13 @@
     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 @@
     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 @@
     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

lib/api/notes.rb

@@ -78,17 +78,15 @@ class Notes < Grape::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