From b202b42cfee6bb8cf0c142c918c545f45464a29c Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Fri, 31 Mar 2017 17:39:14 -0600
Subject: [PATCH] Link to outdated diff in older MR version from outdated diff
 discussion

---
 .../stylesheets/pages/merge_requests.scss     |  2 ++
 .../projects/merge_requests_controller.rb     | 28 ++++++++-----------
 app/models/concerns/note_on_diff.rb           | 10 +++++++
 app/models/concerns/noteable.rb               |  4 +--
 app/models/diff_discussion.rb                 |  1 +
 app/models/diff_note.rb                       | 14 ++++------
 app/models/legacy_diff_note.rb                |  3 +-
 app/models/merge_request.rb                   |  4 +++
 app/models/merge_request_diff.rb              |  1 +
 app/models/note.rb                            |  8 ++++--
 app/views/discussions/_discussion.html.haml   |  7 ++++-
 .../projects/diffs/_parallel_view.html.haml   |  3 +-
 app/views/projects/diffs/_text_file.html.haml |  3 +-
 .../merge_requests/show/_versions.html.haml   | 10 ++++---
 .../dm-link-discussion-to-outdated-diff.yml   |  4 +++
 15 files changed, 63 insertions(+), 39 deletions(-)
 create mode 100644 changelogs/unreleased/dm-link-discussion-to-outdated-diff.yml

diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 2f946ab2f59..cc43d046b54 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -528,6 +528,8 @@
   }
 
   .comments-disabled-notif {
+    line-height: 28px;
+
     .btn {
       margin-left: 5px;
     }
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 5c1f7e69ee8..bba3e007610 100755
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -16,7 +16,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :define_show_vars, only: [:show, :diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines]
   before_action :define_widget_vars, only: [:merge, :cancel_merge_when_pipeline_succeeds, :merge_check]
   before_action :define_commit_vars, only: [:diffs]
-  before_action :define_diff_comment_vars, only: [:diffs]
   before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds, :conflicts, :conflict_for_path, :pipelines]
   before_action :close_merge_request_without_source_project, only: [:show, :diffs, :commits, :builds, :pipelines]
   before_action :apply_diff_view_cookie!, only: [:new_diffs]
@@ -108,6 +107,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
             @merge_request.merge_request_diff
           end
 
+        define_diff_comment_vars
+
         @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff
         @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
 
@@ -123,11 +124,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
         @environment = @merge_request.environments_for(current_user).last
 
-        if @start_sha
-          compared_diff_version
-        else
-          original_diff_version
-        end
+        @diff_notes_disabled = !@merge_request_diff.latest? || @start_sha
+
+        @diffs =
+          if @start_sha
+            @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
+          else
+            @merge_request_diff.diffs(diff_options)
+          end
 
         render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
       end
@@ -594,7 +598,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
 
-    @grouped_diff_discussions = @merge_request.grouped_diff_discussions
+    @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@merge_request_diff.diff_refs)
     @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
   end
 
@@ -678,16 +682,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params.merge(diff_options: diff_options)).execute
   end
 
-  def compared_diff_version
-    @diff_notes_disabled = true
-    @diffs = @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
-  end
-
-  def original_diff_version
-    @diff_notes_disabled = !@merge_request_diff.latest?
-    @diffs = @merge_request_diff.diffs(diff_options)
-  end
-
   def close_merge_request_without_source_project
     if !@merge_request.source_project && @merge_request.open?
       @merge_request.close
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index 1a5a7007a2b..ac4c3099c00 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -25,4 +25,14 @@ module NoteOnDiff
   def diff_attributes
     raise NotImplementedError
   end
+
+  private
+
+  def noteable_diff_refs
+    if noteable.respond_to?(:diff_sha_refs)
+      noteable.diff_sha_refs
+    else
+      noteable.diff_refs
+    end
+  end
 end
diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb
index 772ff6a6d2f..dd1e6630642 100644
--- a/app/models/concerns/noteable.rb
+++ b/app/models/concerns/noteable.rb
@@ -36,10 +36,10 @@ module Noteable
       .discussions(self)
   end
 
-  def grouped_diff_discussions
+  def grouped_diff_discussions(*args)
     # Doesn't use `discussion_notes`, because this may include commit diff notes
     # besides MR diff notes, that we do no want to display on the MR Changes tab.
-    notes.inc_relations_for_view.grouped_diff_discussions
+    notes.inc_relations_for_view.grouped_diff_discussions(*args)
   end
 
   def resolvable_discussions
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index d9b7e484e0f..6a6466b493b 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -10,6 +10,7 @@ class DiffDiscussion < Discussion
 
   delegate  :position,
             :original_position,
+            :latest_merge_request_diff,
 
             to: :first_note
 
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 1523244f8a8..abe4518d62a 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -65,20 +65,18 @@ class DiffNote < Note
     self.position.diff_refs == diff_refs
   end
 
+  def latest_merge_request_diff
+    return unless for_merge_request?
+
+    self.noteable.merge_request_diff_for(self.position.diff_refs)
+  end
+
   private
 
   def supported?
     for_commit? || self.noteable.has_complete_diff_refs?
   end
 
-  def noteable_diff_refs
-    if noteable.respond_to?(:diff_sha_refs)
-      noteable.diff_sha_refs
-    else
-      noteable.diff_refs
-    end
-  end
-
   def set_original_position
     self.original_position = self.position.dup unless self.original_position&.complete?
   end
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index 9a77557ebcd..d7c627432d2 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -56,11 +56,12 @@ class LegacyDiffNote < Note
   #
   # If the note's current diff cannot be matched in the MergeRequest's current
   # diff, it's considered inactive.
-  def active?
+  def active?(diff_refs = nil)
     return @active if defined?(@active)
     return true if for_commit?
     return true unless diff_line
     return false unless noteable
+    return false if diff_refs && diff_refs != noteable_diff_refs
 
     noteable_diff = find_noteable_diff
 
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b71a9e17a93..1b6e898a7fd 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -367,6 +367,10 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff(true)
   end
 
+  def merge_request_diff_for(diff_refs)
+    merge_request_diffs.viewable.select_without_diff.with_diff_refs(diff_refs).take
+  end
+
   def reload_diff_if_branch_changed
     if source_branch_changed? || target_branch_changed?
       reload_diff
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 6ad56b842b2..bf9289086f0 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -26,6 +26,7 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   scope :viewable, -> { without_state(:empty) }
+  scope :with_diff_refs, ->(diff_refs) { where(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha) }
 
   # All diff information is collected from repository after object is created.
   # It allows you to override variables like head_commit_sha before getting diff.
diff --git a/app/models/note.rb b/app/models/note.rb
index 1ea7b946061..c85692c5aec 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -113,11 +113,11 @@ class Note < ActiveRecord::Base
       Discussion.build(notes)
     end
 
-    def grouped_diff_discussions
+    def grouped_diff_discussions(diff_refs = nil)
       diff_notes.
         fresh.
         discussions.
-        select(&:active?).
+        select { |n| n.active?(diff_refs) }.
         group_by(&:line_code)
     end
 
@@ -140,6 +140,10 @@ class Note < ActiveRecord::Base
     true
   end
 
+  def latest_merge_request_diff
+    nil
+  end
+
   def max_attachment_size
     current_application_settings.max_attachment_size.megabytes.to_i
   end
diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml
index e04958817e4..f12778be305 100644
--- a/app/views/discussions/_discussion.html.haml
+++ b/app/views/discussions/_discussion.html.haml
@@ -34,7 +34,12 @@
               - if discussion.active?
                 = link_to 'the diff', discussion_diff_path(discussion)
               - else
-                an outdated diff
+                - merge_request_diff = discussion.latest_merge_request_diff
+                - if merge_request_diff
+                  = link_to diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, diff_id: merge_request_diff, anchor: discussion.line_code) do
+                    an outdated diff
+                - else
+                  an outdated 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/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml
index f920f359de2..45c95f7ab6a 100644
--- a/app/views/projects/diffs/_parallel_view.html.haml
+++ b/app/views/projects/diffs/_parallel_view.html.haml
@@ -5,8 +5,7 @@
       - left = line[:left]
       - right = line[:right]
       - last_line = right.new_pos if right
-      - unless @diff_notes_disabled
-        - discussions_left, discussions_right = parallel_diff_discussions(left, right, diff_file)
+      - discussions_left, discussions_right = parallel_diff_discussions(left, right, diff_file)
       %tr.line_holder.parallel
         - if left
           - case left.type
diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml
index ebd1a914ee7..5f3968b6709 100644
--- a/app/views/projects/diffs/_text_file.html.haml
+++ b/app/views/projects/diffs/_text_file.html.haml
@@ -4,11 +4,10 @@
     %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show.
 
 %table.text-file.code.js-syntax-highlight{ data: diff_view_data, class: too_big ? 'hide' : '' }
-  - discussions = @grouped_diff_discussions unless @diff_notes_disabled
   = render partial: "projects/diffs/line",
     collection: diff_file.highlighted_diff_lines,
     as: :line,
-    locals: { diff_file: diff_file, discussions: discussions }
+    locals: { diff_file: diff_file, discussions: @grouped_diff_discussions }
 
   - if !diff_file.new_file && !diff_file.deleted_file && diff_file.highlighted_diff_lines.any?
     - last_line = diff_file.highlighted_diff_lines.last
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index 74a7b1dc498..47f45e8e061 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/app/views/projects/merge_requests/show/_versions.html.haml
@@ -74,11 +74,13 @@
         from
         %code= @merge_request.target_branch
 
-    - unless @merge_request_diff.latest? && !@start_sha
+    - if @diff_notes_disabled
       .comments-disabled-notif.content-block
         = icon('info-circle')
         - if @start_sha
-          Comments are disabled because you're comparing two versions of this merge request.
+          Comment creation is disabled because you're comparing two versions of this merge request.
         - else
-          Comments are disabled because you're viewing an old version of this merge request.
-        = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'
+          Discussions on this old version of the merge request are displayed but comment creation has been disabled.
+
+        .pull-right
+          = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'
diff --git a/changelogs/unreleased/dm-link-discussion-to-outdated-diff.yml b/changelogs/unreleased/dm-link-discussion-to-outdated-diff.yml
new file mode 100644
index 00000000000..d489bada7ea
--- /dev/null
+++ b/changelogs/unreleased/dm-link-discussion-to-outdated-diff.yml
@@ -0,0 +1,4 @@
+---
+title: Link to outdated diff in older MR version from outdated diff discussion
+merge_request:
+author: