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

16 files changed, 97 insertions, 61 deletions

diff --git a/app/assets/stylesheets/framework/timeline.scss b/app/assets/stylesheets/framework/timeline.scss
index aa0c512a277..515e6f0b2b1 100644
--- a/app/assets/stylesheets/framework/timeline.scss
+++ b/app/assets/stylesheets/framework/timeline.scss
@@ -5,7 +5,7 @@
 
   .note-text {
     p:last-child {
-      margin-bottom: 0;
+      margin-bottom: 0 !important;
     }
   }
 
diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss
index 9db26f99a75..49e453c7dbc 100644
--- a/app/assets/stylesheets/pages/note_form.scss
+++ b/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;
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index 4b15fc2bd82..5550c0d5b47 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/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 {
diff --git a/app/helpers/system_note_helper.rb b/app/helpers/system_note_helper.rb
index d889d141101..209bd56b78a 100644
--- a/app/helpers/system_note_helper.rb
+++ b/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)
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 14ddd2fcc88..800574d8be0 100644
--- a/app/models/diff_discussion.rb
+++ b/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
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 76c59199afd..1764004078e 100644
--- a/app/models/diff_note.rb
+++ b/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)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 9be00880438..2eec013fa9d 100644
--- a/app/models/merge_request.rb
+++ b/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
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index f0a3c30ea74..6e3917a10a3 100644
--- a/app/models/merge_request_diff.rb
+++ b/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
diff --git a/app/models/note.rb b/app/models/note.rb
index 46d0a4f159f..b4228cc6492 100644
--- a/app/models/note.rb
+++ b/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|
-        if discussion.active?(diff_refs)
-          discussions = groups[discussion.line_code] ||= []
-        elsif diff_refs && discussion.created_at_diff?(diff_refs)
-          discussions = groups[discussion.original_line_code] ||= []
-        end
-
-        discussions << discussion if discussions
+        line_code =
+          if discussion.active?(diff_refs)
+            discussion.line_code
+          elsif diff_refs && discussion.created_at_diff?(diff_refs)
+            discussion.original_line_code
+          end
+
+        groups[line_code] << discussion if line_code
       end
 
       groups
diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb
index b44f4fe000c..414c95f7705 100644
--- a/app/models/system_note_metadata.rb
+++ b/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
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 1131d6f4913..81d217929d5 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/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
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index fadcce5d9b6..54b19e6d651 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/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
 
diff --git a/app/services/notes/diff_position_update_service.rb b/app/services/notes/diff_position_update_service.rb
index 0cb731f5bc3..eff7b287269 100644
--- a/app/services/notes/diff_position_update_service.rb
+++ b/app/services/notes/diff_position_update_service.rb
@@ -1,26 +1,29 @@
 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]
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 93bf1fb1615..0837c07e6aa 100644
--- a/app/services/system_note_service.rb
+++ b/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`
diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml
index 74992e439f3..578e751ab47 100644
--- a/app/views/discussions/_discussion.html.haml
+++ b/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?
-                  the diff
-                - else
-                  an outdated diff
+                - unless discussion.active?
+                  an old version of
+                the diff
 
             = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
           = render "discussions/headline", discussion: discussion
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index 37117bc64a3..0999b95c9c9 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/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'