Merge branch 'blackst0ne/gitlab-ce-add_system_note_for_editing_issuable'

app/helpers/application_helper.rb

@@ -180,16 +180,16 @@ module ApplicationHelper
     element
   end
-  def edited_time_ago_with_tooltip(object, placement: 'top', html_class: 'time_ago', include_author: false)
-    return if object.updated_at == object.created_at
-    content_tag :small, class: "edited-text" do
-      output = content_tag(:span, "Edited ")
-      output << time_ago_with_tooltip(object.updated_at, placement: placement, html_class: html_class)
-      if include_author && object.updated_by && object.updated_by != object.author
-        output << content_tag(:span, " by ")
-        output << link_to_member(object.project, object.updated_by, avatar: false, author_class: nil)
+  def edited_time_ago_with_tooltip(object, placement: 'top', html_class: 'time_ago', exclude_author: false)
+    return if object.last_edited_at == object.created_at || object.last_edited_at.blank?
+    content_tag :small, class: 'edited-text' do
+      output = content_tag(:span, 'Edited ')
+      output << time_ago_with_tooltip(object.last_edited_at, placement: placement, html_class: html_class)
+      if !exclude_author && object.last_edited_by
+        output << content_tag(:span, ' by ')
+        output << link_to_member(object.project, object.last_edited_by, avatar: false, author_class: nil)
       end
       output

app/helpers/system_note_helper.rb

 module SystemNoteHelper
   ICON_NAMES_BY_ACTION = {
     'commit' => 'icon_commit',
+    'description' => 'icon_edit',
     'merge' => 'icon_merge',
     'merged' => 'icon_merged',
     'opened' => 'icon_status_open',

app/models/concerns/issuable.rb

@@ -27,6 +27,7 @@ module Issuable
     belongs_to :author, class_name: "User"
     belongs_to :updated_by, class_name: "User"
+    belongs_to :last_edited_by, class_name: 'User'
     belongs_to :milestone
     has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :destroy do
       def authors_loaded?

app/models/note.rb

@@ -18,6 +18,11 @@ class Note < ActiveRecord::Base
   cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
+  # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes.
+  # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10392/diffs#note_28719102
+  alias_attribute :last_edited_at, :updated_at
+  alias_attribute :last_edited_by, :updated_by
+
   # Attribute containing rendered and redacted Markdown as generated by
   # Banzai::ObjectRenderer.
   attr_accessor :redacted_note_html
@@ -38,6 +43,7 @@ class Note < ActiveRecord::Base
   belongs_to :noteable, polymorphic: true, touch: true
   belongs_to :author, class_name: "User"
   belongs_to :updated_by, class_name: "User"
+  belongs_to :last_edited_by, class_name: 'User'
   has_many :todos, dependent: :destroy
   has_many :events, as: :target, dependent: :destroy

app/models/snippet.rb

@@ -12,6 +12,11 @@ class Snippet < ActiveRecord::Base
   cache_markdown_field :title, pipeline: :single_line
   cache_markdown_field :content
+  # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with snippets.
+  # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10392/diffs#note_28719102
+  alias_attribute :last_edited_at, :updated_at
+  alias_attribute :last_edited_by, :updated_by
+
   # If file_name changes, it invalidates content
   alias_method :default_content_html_invalidator, :content_html_invalidated?
   def content_html_invalidated?

app/models/system_note_metadata.rb

 class SystemNoteMetadata < ActiveRecord::Base
   ICON_TYPES = %w[
-    commit merge confidential visible label assignee cross_reference
+    commit description merge confidential visible label assignee cross_reference
     title time_tracking branch milestone discussion task moved opened closed merged
   ].freeze

app/services/issuable_base_service.rb

@@ -19,6 +19,10 @@ class IssuableBaseService < BaseService
       issuable, issuable.project, current_user, old_title)
   end
+  def create_description_change_note(issuable)
+    SystemNoteService.change_description(issuable, issuable.project, current_user)
+  end
+
   def create_branch_change_note(issuable, branch_type, old_branch, new_branch)
     SystemNoteService.change_branch(
       issuable, issuable.project, current_user, branch_type,
@@ -211,6 +215,10 @@ class IssuableBaseService < BaseService
     if issuable.changed? || params.present?
       issuable.assign_attributes(params.merge(updated_by: current_user))
+      if has_title_or_description_changed?(issuable)
+        issuable.assign_attributes(last_edited_at: Time.now, last_edited_by: current_user)
+      end
+
       before_update(issuable)
       if issuable.with_transaction_returning_status { issuable.save }
@@ -239,6 +247,10 @@ class IssuableBaseService < BaseService
     old_label_ids.sort != new_label_ids.sort
   end
+  def has_title_or_description_changed?(issuable)
+    issuable.title_changed? || issuable.description_changed?
+  end
+
   def change_state(issuable)
     case params.delete(:state_event)
     when 'reopen'
@@ -294,6 +306,10 @@ class IssuableBaseService < BaseService
       create_title_change_note(issuable, issuable.previous_changes['title'].first)
     end
+    if issuable.previous_changes.include?('description')
+      create_description_change_note(issuable)
+    end
+
     if issuable.previous_changes.include?('description') && issuable.tasks?
       create_task_status_note(issuable)
     end

app/services/system_note_service.rb

@@ -299,6 +299,23 @@ module SystemNoteService
     create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
   end
+  # Called when the description of a Noteable is changed
+  #
+  # noteable  - Noteable object that responds to `description`
+  # project   - Project owning noteable
+  # author    - User performing the change
+  #
+  # Example Note text:
+  #
+  #   "changed the description"
+  #
+  # Returns the created Note object
+  def change_description(noteable, project, author)
+    body = 'changed the description'
+
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'description'))
+  end
+
   # Called when the confidentiality changes
   #
   # issue   - Issue object

app/views/shared/notes/_note.html.haml

@@ -40,7 +40,7 @@
       .note-body{ class: note_editable ? 'js-task-list-container' : '' }
         .note-text.md
           = note.redacted_note_html
-        = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago', include_author: true)
+        = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago')
         - if note_editable
           = render 'shared/notes/edit', note: note
         .note-awards

app/views/shared/snippets/_header.html.haml

@@ -21,4 +21,4 @@
     = markdown_field(@snippet, :title)
   - if @snippet.updated_at != @snippet.created_at
-    = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago')
+    = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago', exclude_author: true)

changelogs/unreleased/add_system_note_for_editing_issuable.yml

+---
+title: Add system note on description change of issue/merge request
+merge_request: 10392
+author: blackst0ne

db/migrate/20170503021915_add_last_edited_at_and_last_edited_by_id_to_issues.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLastEditedAtAndLastEditedByIdToIssues < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def change
+    add_column :issues, :last_edited_at, :timestamp
+    add_column :issues, :last_edited_by_id, :integer
+  end
+end

db/migrate/20170503022548_add_last_edited_at_and_last_edited_by_id_to_merge_requests.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLastEditedAtAndLastEditedByIdToMergeRequests < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def change
+    add_column :merge_requests, :last_edited_at, :timestamp
+    add_column :merge_requests, :last_edited_by_id, :integer
+  end
+end

db/schema.rb

@@ -498,6 +498,8 @@ ActiveRecord::Schema.define(version: 20170504102911) do
     t.integer "relative_position"
     t.datetime "closed_at"
     t.integer "cached_markdown_version"
+    t.datetime "last_edited_at"
+    t.integer "last_edited_by_id"
   end
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -684,6 +686,8 @@ ActiveRecord::Schema.define(version: 20170504102911) do
     t.text "description_html"
     t.integer "time_estimate"
     t.integer "cached_markdown_version"
+    t.datetime "last_edited_at"
+    t.integer "last_edited_by_id"
   end
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -1425,4 +1429,4 @@ ActiveRecord::Schema.define(version: 20170504102911) do
   add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
-end
\ No newline at end of file
+end

lib/gitlab/import_export/relation_factory.rb

@@ -15,7 +15,7 @@ module Gitlab
                     priorities: :label_priorities,
                     label: :project_label }.freeze
-      USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id created_by_id merge_user_id resolved_by_id].freeze
+      USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id created_by_id last_edited_by_id merge_user_id resolved_by_id].freeze
       PROJECT_REFERENCES = %w[project_id source_project_id target_project_id].freeze

spec/lib/gitlab/import_export/all_models.yml

@@ -9,6 +9,7 @@ issues:
 - notes
 - label_links
 - labels
+- last_edited_by
 - todos
 - user_agent_detail
 - moved_to
@@ -27,6 +28,7 @@ notes:
 - noteable
 - author
 - updated_by
+- last_edited_by
 - resolved_by
 - todos
 - events
@@ -72,6 +74,7 @@ merge_requests:
 - notes
 - label_links
 - labels
+- last_edited_by
 - todos
 - target_project
 - source_project

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -23,6 +23,8 @@ Issue:
 - weight
 - time_estimate
 - relative_position
+- last_edited_at
+- last_edited_by_id
 Event:
 - id
 - target_type
@@ -154,6 +156,8 @@ MergeRequest:
 - approvals_before_merge
 - rebase_commit_sha
 - time_estimate
+- last_edited_at
+- last_edited_by_id
 MergeRequestDiff:
 - id
 - state

spec/services/issues/update_service_spec.rb

@@ -132,6 +132,17 @@ describe Issues::UpdateService, services: true do
       end
     end
+    context 'when description changed' do
+      it 'creates system note about description change' do
+        update_issue(description: 'Changed description')
+
+        note = find_note('changed the description')
+
+        expect(note).not_to be_nil
+        expect(note.note).to eq('changed the description')
+      end
+    end
+
     context 'when issue turns confidential' do
       let(:opts) do
         {

spec/services/merge_requests/update_service_spec.rb

@@ -102,6 +102,13 @@ describe MergeRequests::UpdateService, services: true do
         expect(note.note).to eq 'changed title from **{-Old-} title** to **{+New+} title**'
       end
+      it 'creates system note about description change' do
+        note = find_note('changed the description')
+
+        expect(note).not_to be_nil
+        expect(note.note).to eq('changed the description')
+      end
+
       it 'creates system note about branch change' do
         note = find_note('changed target')

spec/services/system_note_service_spec.rb

@@ -339,6 +339,20 @@ describe SystemNoteService, services: true do
     end
   end
+  describe '.change_description' do
+    subject { described_class.change_description(noteable, project, author) }
+
+    context 'when noteable responds to `description`' do
+      it_behaves_like 'a system note' do
+        let(:action) { 'description' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq('changed the description')
+      end
+    end
+  end
+
   describe '.change_issue_confidentiality' do
     subject { described_class.change_issue_confidentiality(noteable, project, author) }