Allow commenting on older versions of the diff and comparisons between diff versionsdm-link-discussion-to-outdated-diff

7 files changed, 67 insertions, 23 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index 8ee42875670..a7bdf5587b2 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -11,6 +11,7 @@ module DiscussionOnDiff
               :diff_line,
               :for_line?,
               :active?,
+              :created_at_diff?,
 
               to: :first_note
 
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index 6c27dd5aa5c..6359f7596b1 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -30,6 +30,10 @@ module NoteOnDiff
     raise NotImplementedError
   end
 
+  def created_at_diff?(diff_refs)
+    false
+  end
+
   private
 
   def noteable_diff_refs
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 6a6466b493b..d627fbe327f 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -10,7 +10,6 @@ class DiffDiscussion < Discussion
 
   delegate  :position,
             :original_position,
-            :latest_merge_request_diff,
 
             to: :first_note
 
@@ -18,6 +17,25 @@ class DiffDiscussion < Discussion
     false
   end
 
+  def merge_request_version_params
+    return unless for_merge_request?
+
+    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
+  end
+
   def reply_attributes
     super.merge(
       original_position: original_position.to_json,
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index abe4518d62a..76c59199afd 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -65,10 +65,11 @@ class DiffNote < Note
     self.position.diff_refs == diff_refs
   end
 
-  def latest_merge_request_diff
-    return unless for_merge_request?
+  def created_at_diff?(diff_refs)
+    return false unless supported?
+    return true if for_commit?
 
-    self.noteable.merge_request_diff_for(self.position.diff_refs)
+    self.original_position.diff_refs == diff_refs
   end
 
   private
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index e617ce36f56..3c1d34db5fa 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -9,14 +9,14 @@ class LegacyDiffDiscussion < Discussion
 
   memoized_values << :active
 
-  def legacy_diff_discussion?
-    true
-  end
-
   def self.note_class
     LegacyDiffNote
   end
 
+  def legacy_diff_discussion?
+    true
+  end
+
   def active?(*args)
     return @active if @active.present?
 
@@ -27,6 +27,16 @@ class LegacyDiffDiscussion < Discussion
     !active?
   end
 
+  def merge_request_version_params
+    return unless for_merge_request?
+
+    if active?
+      {}
+    else
+      nil
+    end
+  end
+
   def reply_attributes
     super.merge(line_code: line_code)
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 365fa4f1e70..12c5481cd6d 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -374,12 +374,18 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff(true)
   end
 
-  def merge_request_diff_for(diff_refs)
-    @merge_request_diffs_by_diff_refs ||= Hash.new do |h, diff_refs|
-      h[diff_refs] = merge_request_diffs.viewable.select_without_diff.find_by_diff_refs(diff_refs)
-    end
-
-    @merge_request_diffs_by_diff_refs[diff_refs]
+  def merge_request_diff_for(diff_refs_or_sha)
+    @merge_request_diffs_by_diff_refs_or_sha ||= Hash.new do |h, diff_refs_or_sha|
+      diffs = merge_request_diffs.viewable.select_without_diff
+      h[diff_refs_or_sha] =
+        if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs)
+          diffs.find_by_diff_refs(diff_refs_or_sha)
+        else
+          diffs.find_by(head_commit_sha: diff_refs_or_sha)
+        end
+    end
+
+    @merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha]
   end
 
   def reload_diff_if_branch_changed
diff --git a/app/models/note.rb b/app/models/note.rb
index e720bfba030..b06985b4a6f 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -115,11 +115,19 @@ class Note < ActiveRecord::Base
     end
 
     def grouped_diff_discussions(diff_refs = nil)
-      diff_notes.
-        fresh.
-        discussions.
-        select { |n| n.active?(diff_refs) }.
-        group_by(&:line_code)
+      groups = {}
+
+      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
+      end
+
+      groups
     end
 
     def count_for_collection(ids, type)
@@ -141,10 +149,6 @@ 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