Add system note with link to diff comparison when MR discussion becomes outdated

app/assets/stylesheets/framework/timeline.scss

@@ -5,7 +5,7 @@
 
   .note-text {
     p:last-child {
-      margin-bottom: 0;
+      margin-bottom: 0 !important;
     }
   }
 

app/assets/stylesheets/pages/note_form.scss

@@ -164,10 +164,6 @@
 
 .discussion-body,
 .diff-file {
-  .notes .note {
-    padding: 10px 15px;
-  }
-
   .discussion-reply-holder {
     background-color: $white-light;
     padding: 10px 16px;

app/assets/stylesheets/pages/notes.scss

@@ -80,10 +80,6 @@ ul.notes {
       &.timeline-entry {
         padding: 14px 10px;
       }
-
-      .system-note {
-        padding: 0;
-      }
     }
 
     &.is-editing {
@@ -380,6 +376,10 @@ ul.notes {
   padding-bottom: 5px;
 }
 
+.system-note .note-header-info {
+  padding-bottom: 0;
+}
+
 .note-headline-light {
   display: inline;
 
@@ -582,6 +582,17 @@ ul.notes {
   }
 }
 
+.discussion-body,
+.diff-file {
+  .notes .note {
+    padding: 10px 15px;
+
+    &.system-note {
+      padding: 0;
+    }
+  }
+}
+
 .diff-file {
   .is-over {
     .add-diff-note {

app/helpers/system_note_helper.rb

@@ -17,7 +17,8 @@ module SystemNoteHelper
     'visible' => 'icon_eye',
     'milestone' => 'icon_clock_o',
     'discussion' => 'icon_comment_o',
-    'moved' => 'icon_arrow_circle_o_right'
+    'moved' => 'icon_arrow_circle_o_right',
+    'outdated' => 'icon_edit'
   }.freeze
 
   def icon_for_system_note(note)

app/models/diff_discussion.rb

@@ -19,21 +19,9 @@ class DiffDiscussion < Discussion
 
   def merge_request_version_params
     return unless for_merge_request?
+    return {} if active?
 
-    if active?
-      {}
-    else
-      diff_refs = position.diff_refs
-
-      if diff = noteable.merge_request_diff_for(diff_refs)
-        { diff_id: diff.id }
-      elsif diff = noteable.merge_request_diff_for(diff_refs.head_sha)
-        {
-          diff_id: diff.id,
-          start_sha: diff_refs.start_sha
-        }
-      end
-    end
+    noteable.version_params_for(position.diff_refs)
   end
 
   def reply_attributes

app/models/diff_note.rb

@@ -8,6 +8,7 @@ class DiffNote < Note
 
   serialize :original_position, Gitlab::Diff::Position
   serialize :position, Gitlab::Diff::Position
+  serialize :change_position, Gitlab::Diff::Position
 
   validates :original_position, presence: true
   validates :position, presence: true
@@ -25,7 +26,7 @@ class DiffNote < Note
     DiffDiscussion
   end
 
-  %i(original_position position).each do |meth|
+  %i(original_position position change_position).each do |meth|
     define_method "#{meth}=" do |new_position|
       if new_position.is_a?(String)
         new_position = JSON.parse(new_position) rescue nil
@@ -36,6 +37,8 @@ class DiffNote < Note
         new_position = Gitlab::Diff::Position.new(new_position)
       end
 
+      return if new_position == read_attribute(meth)
+
       super(new_position)
     end
   end
@@ -45,7 +48,7 @@ class DiffNote < Note
   end
 
   def diff_line
-    @diff_line ||= diff_file.line_for_position(self.original_position) if diff_file
+    @diff_line ||= diff_file&.line_for_position(self.original_position)
   end
 
   def for_line?(line)

app/models/merge_request.rb

@@ -416,13 +416,24 @@ class MergeRequest < ActiveRecord::Base
     @merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha]
   end
 
+  def version_params_for(diff_refs)
+    if diff = merge_request_diff_for(diff_refs)
+      { diff_id: diff.id }
+    elsif diff = merge_request_diff_for(diff_refs.head_sha)
+      {
+        diff_id: diff.id,
+        start_sha: diff_refs.start_sha
+      }
+    end
+  end
+
   def reload_diff_if_branch_changed
     if source_branch_changed? || target_branch_changed?
       reload_diff
     end
   end
 
-  def reload_diff
+  def reload_diff(current_user = nil)
     return unless open?
 
     old_diff_refs = self.diff_refs
@@ -432,7 +443,8 @@ class MergeRequest < ActiveRecord::Base
 
     update_diff_notes_positions(
       old_diff_refs: old_diff_refs,
-      new_diff_refs: new_diff_refs
+      new_diff_refs: new_diff_refs,
+      current_user: current_user
     )
   end
 
@@ -861,7 +873,7 @@ class MergeRequest < ActiveRecord::Base
     diff_sha_refs && diff_sha_refs.complete?
   end
 
-  def update_diff_notes_positions(old_diff_refs:, new_diff_refs:)
+  def update_diff_notes_positions(old_diff_refs:, new_diff_refs:, current_user: nil)
     return unless has_complete_diff_refs?
     return if new_diff_refs == old_diff_refs
 
@@ -875,7 +887,7 @@ class MergeRequest < ActiveRecord::Base
 
     service = Notes::DiffPositionUpdateService.new(
       self.project,
-      nil,
+      current_user,
       old_diff_refs: old_diff_refs,
       new_diff_refs: new_diff_refs,
       paths: paths

app/models/merge_request_diff.rb

@@ -175,12 +175,11 @@ class MergeRequestDiff < ActiveRecord::Base
     self == merge_request.merge_request_diff
   end
 
-  def compare_with(sha, straight: true)
+  def compare_with(sha)
     # When compare merge request versions we want diff A..B instead of A...B
     # so we handle cases when user does squash and rebase of the commits between versions.
     # For this reason we set straight to true by default.
-    CompareService.new(project, head_commit_sha)
-      .execute(project, sha, straight: straight)
+    CompareService.new(project, head_commit_sha).execute(project, sha, straight: true)
   end
 
   def commits_count

app/models/note.rb

@@ -121,16 +121,17 @@ class Note < ActiveRecord::Base
     end
 
     def grouped_diff_discussions(diff_refs = nil)
-      groups = {}
+      groups = Hash.new { |h, k| h[k] = [] }
 
       diff_notes.fresh.discussions.each do |discussion|
+        line_code =
           if discussion.active?(diff_refs)
-          discussions = groups[discussion.line_code] ||= []
+            discussion.line_code
           elsif diff_refs && discussion.created_at_diff?(diff_refs)
-          discussions = groups[discussion.original_line_code] ||= []
+            discussion.original_line_code
           end
 
-        discussions << discussion if discussions
+        groups[line_code] << discussion if line_code
       end
 
       groups

app/models/system_note_metadata.rb

@@ -2,6 +2,7 @@ class SystemNoteMetadata < ActiveRecord::Base
   ICON_TYPES = %w[
     commit description merge confidential visible label assignee cross_reference
     title time_tracking branch milestone discussion task moved opened closed merged
+    outdated
   ].freeze
 
   validates :note, presence: true

app/services/merge_requests/refresh_service.rb

@@ -66,12 +66,12 @@ module MergeRequests
 
       filter_merge_requests(merge_requests).each do |merge_request|
         if merge_request.source_branch == @branch_name || force_push?
-          merge_request.reload_diff
+          merge_request.reload_diff(current_user)
         else
           mr_commit_ids = merge_request.commits_sha
           push_commit_ids = @commits.map(&:id)
           matches = mr_commit_ids & push_commit_ids
-          merge_request.reload_diff if matches.any?
+          merge_request.reload_diff(current_user) if matches.any?
         end
 
         merge_request.mark_as_unchecked

app/services/merge_requests/reopen_service.rb

@@ -8,7 +8,7 @@ module MergeRequests
         create_note(merge_request)
         notification_service.reopen_mr(merge_request, current_user)
         execute_hooks(merge_request, 'reopen')
-        merge_request.reload_diff
+        merge_request.reload_diff(current_user)
         merge_request.mark_as_unchecked
       end
 

app/services/notes/diff_position_update_service.rb

 module Notes
   class DiffPositionUpdateService < BaseService
     def execute(note)
-      new_position = tracer.trace(note.position)
+      results = tracer.trace(note.position)
+      return unless results
 
-      # Don't update the position if the type doesn't match, since that means
-      # the diff line commented on was changed, and the comment is now outdated
-      old_position = note.position
-      if new_position &&
-          new_position != old_position &&
-          new_position.type == old_position.type
+      position = results[:position]
+      outdated = results[:outdated]
 
-        note.position = new_position
-      end
+      if outdated
+        note.change_position = position
 
-      note
+        if note.persisted? && current_user
+          SystemNoteService.diff_discussion_outdated(note.to_discussion, project, current_user, position)
+        end
+      else
+        note.position = position
+        note.change_position = nil
+      end
     end
 
     private
 
     def tracer
       @tracer ||= Gitlab::Diff::PositionTracer.new(
-        repository: project.repository,
+        project: project,
         old_diff_refs: params[:old_diff_refs],
         new_diff_refs: params[:new_diff_refs],
         paths: params[:paths]

app/services/system_note_service.rb

@@ -258,7 +258,7 @@ module SystemNoteService
     create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
   end
 
-  def self.resolve_all_discussions(merge_request, project, author)
+  def resolve_all_discussions(merge_request, project, author)
     body = "resolved all discussions"
 
     create_note(NoteSummary.new(merge_request, project, author, body, action: 'discussion'))
@@ -274,6 +274,28 @@ module SystemNoteService
     note
   end
 
+  def diff_discussion_outdated(discussion, project, author, change_position)
+    merge_request = discussion.noteable
+    diff_refs = change_position.diff_refs
+    version_index = merge_request.merge_request_diffs.viewable.count
+
+    body = "changed this line in"
+    if version_params = merge_request.version_params_for(diff_refs)
+      line_code = change_position.line_code(project.repository)
+      url = url_helpers.diffs_namespace_project_merge_request_url(project.namespace, project, merge_request, version_params.merge(anchor: line_code))
+
+      body << " [version #{version_index} of the diff](#{url})"
+    else
+      body << " version #{version_index} of the diff"
+    end
+
+    note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
+    note = Note.create(note_attributes.merge(system: true))
+    note.system_note_metadata = SystemNoteMetadata.new(action: 'outdated')
+
+    note
+  end
+
   # Called when the title of a Noteable is changed
   #
   # noteable  - Noteable object that responds to `title`

app/views/discussions/_discussion.html.haml

@@ -32,10 +32,9 @@
             - elsif discussion.diff_discussion?
               on
               = conditional_link_to url.present?, url do
-                - if discussion.active?
+                - unless discussion.active?
+                  an old version of
                 the diff
-                - else
-                  an outdated diff
 
             = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
           = render "discussions/headline", discussion: discussion

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

@@ -91,7 +91,7 @@
           comparing two versions
         - else
           viewing an old version
-        of this merge request.
+        of the diff.
 
         .pull-right
           = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'

db/migrate/20170521184006_add_change_position_to_notes.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddChangePositionToNotes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :notes, :change_position, :text
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170518231126) do
+ActiveRecord::Schema.define(version: 20170521184006) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -794,6 +794,7 @@ ActiveRecord::Schema.define(version: 20170518231126) do
     t.string "discussion_id"
     t.text "note_html"
     t.integer "cached_markdown_version"
+    t.text "change_position"
   end
 
   add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree

lib/gitlab/diff/file_collection/base.rb

@@ -24,6 +24,14 @@ module Gitlab
           @diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) }
         end
 
+        def diff_file_with_old_path(old_path)
+          diff_files.find { |diff_file| diff_file.old_path == old_path }
+        end
+
+        def diff_file_with_new_path(new_path)
+          diff_files.find { |diff_file| diff_file.new_path == new_path }
+        end
+
         private
 
         def decorate_diff!(diff)

lib/gitlab/diff/position.rb

@@ -12,20 +12,26 @@ module Gitlab
       attr_reader :head_sha
 
       def initialize(attrs = {})
+        if diff_file = attrs[:diff_file]
+          attrs[:diff_refs] = diff_file.diff_refs
+          attrs[:old_path] = diff_file.old_path
+          attrs[:new_path] = diff_file.new_path
+        end
+
+        if diff_refs = attrs[:diff_refs]
+          attrs[:base_sha]  = diff_refs.base_sha
+          attrs[:start_sha] = diff_refs.start_sha
+          attrs[:head_sha]  = diff_refs.head_sha
+        end
+
         @old_path = attrs[:old_path]
         @new_path = attrs[:new_path]
-        @old_line = attrs[:old_line]
-        @new_line = attrs[:new_line]
-
-        if attrs[:diff_refs]
-          @base_sha  = attrs[:diff_refs].base_sha
-          @start_sha = attrs[:diff_refs].start_sha
-          @head_sha  = attrs[:diff_refs].head_sha
-        else
         @base_sha  = attrs[:base_sha]
         @start_sha = attrs[:start_sha]
         @head_sha  = attrs[:head_sha]
-        end
+
+        @old_line = attrs[:old_line]
+        @new_line = attrs[:new_line]
       end
 
       # `Gitlab::Diff::Position` objects are stored as serialized attributes in
@@ -129,11 +135,11 @@ module Gitlab
       end
 
       def diff_line(repository)
-        @diff_line ||= diff_file(repository).line_for_position(self)
+        @diff_line ||= diff_file(repository)&.line_for_position(self)
       end
 
       def line_code(repository)
-        @line_code ||= diff_file(repository).line_code_for_position(self)
+        @line_code ||= diff_file(repository)&.line_code_for_position(self)
       end
 
       private

spec/features/merge_requests/versions_spec.rb

@@ -124,6 +124,8 @@ feature 'Merge Request versions', js: true, feature: true do
         diff_refs: merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs
       )
       outdated_diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
+      outdated_diff_note.position = outdated_diff_note.original_position
+      outdated_diff_note.save!
 
       visit current_url
       wait_for_ajax

spec/models/diff_discussion_spec.rb

@@ -48,7 +48,7 @@ describe DiffDiscussion, model: true do
       end
 
       it 'returns the diff ID for the version to show' do
-        expect(diff_id: merge_request_diff1.id)
+        expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff1.id)
       end
     end
 
@@ -65,6 +65,11 @@ describe DiffDiscussion, model: true do
 
       let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position) }
 
+      before do
+        diff_note.position = diff_note.original_position
+        diff_note.save!
+      end
+
       it 'returns the diff ID and start sha of the versions to compare' do
         expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
       end

spec/models/merge_request_spec.rb

@@ -1213,7 +1213,7 @@ describe MergeRequest, models: true do
 
       expect(Notes::DiffPositionUpdateService).to receive(:new).with(
         subject.project,
-        nil,
+        subject.author,
         old_diff_refs: old_diff_refs,
         new_diff_refs: commit.diff_refs,
         paths: note.position.paths
@@ -1222,7 +1222,7 @@ describe MergeRequest, models: true do
       expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
       expect_any_instance_of(DiffNote).to receive(:save).once
 
-      subject.reload_diff
+      subject.reload_diff(subject.author)
     end
   end
 
@@ -1534,4 +1534,36 @@ describe MergeRequest, models: true do
       end
     end
   end
+
+  describe '#version_params_for' do
+    subject { create(:merge_request, importing: true) }
+    let(:project) { subject.project }
+    let!(:merge_request_diff1) { subject.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
+    let!(:merge_request_diff2) { subject.merge_request_diffs.create(head_commit_sha: nil) }
+    let!(:merge_request_diff3) { subject.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
+
+    context 'when the diff refs are for an older merge request version' do
+      let(:diff_refs) { merge_request_diff1.diff_refs }
+
+      it 'returns the diff ID for the version to show' do
+        expect(subject.version_params_for(diff_refs)).to eq(diff_id: merge_request_diff1.id)
+      end
+    end
+
+    context 'when the diff refs are for a comparison between merge request versions' do
+      let(:diff_refs) { merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs }
+
+      it 'returns the diff ID and start sha of the versions to compare' do
+        expect(subject.version_params_for(diff_refs)).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
+      end
+    end
+
+    context 'when the diff refs are not for a merge request version' do
+      let(:diff_refs) { project.commit(sample_commit.id).diff_refs }
+
+      it 'returns nil' do
+        expect(subject.version_params_for(diff_refs)).to be_nil
+      end
+    end
+  end
 end

spec/services/notes/diff_position_update_service_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Notes::DiffPositionUpdateService, services: true do
   let(:project) { create(:project, :repository) }
+  let(:current_user) { project.owner }
   let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") }
   let(:modify_commit) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") }
   let(:edit_commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") }
@@ -25,7 +26,7 @@ describe Notes::DiffPositionUpdateService, services: true do
   subject do
     described_class.new(
       project,
-      nil,
+      current_user,
       old_diff_refs: old_diff_refs,
       new_diff_refs: new_diff_refs,
       paths: [path]
@@ -170,6 +171,23 @@ describe Notes::DiffPositionUpdateService, services: true do
         expect(note.original_position).to eq(old_position)
         expect(note.position).to eq(old_position)
       end
+
+      it 'sets the change position' do
+        subject.execute(note)
+
+        change_position = note.change_position
+        expect(change_position.start_sha).to eq(old_diff_refs.head_sha)
+        expect(change_position.head_sha).to eq(new_diff_refs.head_sha)
+        expect(change_position.old_line).to eq(9)
+        expect(change_position.new_line).to be_nil
+      end
+
+      it 'creates a system note' do
+        expect(SystemNoteService).to receive(:diff_discussion_outdated).with(
+          note.to_discussion, project, current_user, instance_of(Gitlab::Diff::Position))
+
+        subject.execute(note)
+      end
     end
   end
 end

spec/services/system_note_service_spec.rb

@@ -1034,4 +1034,35 @@ describe SystemNoteService, services: true do
       expect(subject.note).to eq 'resolved all discussions'
     end
   end
+
+  describe '.diff_discussion_outdated' do
+    let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
+    let(:merge_request) { discussion.noteable }
+    let(:project) { merge_request.source_project }
+    let(:change_position) { discussion.position }
+
+    def reloaded_merge_request
+      MergeRequest.find(merge_request.id)
+    end
+
+    subject { described_class.diff_discussion_outdated(discussion, project, author, change_position) }
+
+    it_behaves_like 'a system note' do
+      let(:expected_noteable) { discussion.first_note.noteable }
+      let(:action)              { 'outdated' }
+    end
+
+    it 'creates a new note in the discussion' do
+      # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
+      expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+    end
+
+    it 'links to the diff in the system note' do
+      expect(subject.note).to include('version 1')
+
+      diff_id = merge_request.merge_request_diff.id
+      line_code = change_position.line_code(project.repository)
+      expect(subject.note).to include(diffs_namespace_project_merge_request_url(project.namespace, project, merge_request, diff_id: diff_id, anchor: line_code))
+    end
+  end
 end