Extract commonalities between DiffDiscussion and LegacyDiffDiscussion

9 files changed, 162 insertions, 164 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
new file mode 100644
index 00000000000..28abed967f0
--- /dev/null
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -0,0 +1,56 @@
+module DiscussionOnDiff
+  extend ActiveSupport::Concern
+
+  included do
+    NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+
+    memoized_values << :active
+
+    delegate  :line_code,
+              :original_line_code,
+              :diff_file,
+              :diff_line,
+              :for_line?,
+              :active?,
+
+              to: :first_note
+
+    delegate  :file_path,
+              :blob,
+              :highlighted_diff_lines,
+              :diff_lines,
+
+              to: :diff_file,
+              allow_nil: true
+  end
+
+  def diff_discussion?
+    true
+  end
+
+  def active?
+    return @active if @active.present?
+
+    @active = first_note.active?
+  end
+
+  # Returns an array of at most 16 highlighted lines above a diff note
+  def truncated_diff_lines(highlight: true)
+    lines = highlight ? highlighted_diff_lines : diff_lines
+    prev_lines = []
+
+    lines.each do |line|
+      if line.meta?
+        prev_lines.clear
+      else
+        prev_lines << line
+
+        break if for_line?(line)
+
+        prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
+      end
+    end
+
+    prev_lines
+  end
+end
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
new file mode 100644
index 00000000000..3141c150ac9
--- /dev/null
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -0,0 +1,79 @@
+module ResolvableDiscussion
+  extend ActiveSupport::Concern
+
+  included do
+    memoized_values << :resolvable
+    memoized_values << :resolved
+    memoized_values << :first_note
+    memoized_values << :first_note_to_resolve
+    memoized_values << :last_resolved_note
+    memoized_values << :last_note
+
+    delegate  :resolved_at,
+              :resolved_by,
+
+              to: :last_resolved_note,
+              allow_nil: true
+  end
+
+  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
+  def potentially_resolvable?
+    for_merge_request?
+  end
+
+  def resolvable?
+    return @resolvable if @resolvable.present?
+
+    @resolvable = potentially_resolvable? && notes.any?(&:resolvable?)
+  end
+
+  def resolved?
+    return @resolved if @resolved.present?
+
+    @resolved = resolvable? && notes.none?(&:to_be_resolved?)
+  end
+
+  def first_note
+    @first_note ||= notes.first
+  end
+
+  def first_note_to_resolve
+    return unless resolvable?
+
+    @first_note_to_resolve ||= notes.find(&:to_be_resolved?)
+  end
+
+  def last_resolved_note
+    return unless resolved?
+
+    @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
+  end
+
+  def resolved_notes
+    notes.select(&:resolved?)
+  end
+
+  def to_be_resolved?
+    resolvable? && !resolved?
+  end
+
+  def can_resolve?(current_user)
+    return false unless current_user
+    return false unless resolvable?
+
+    current_user == self.noteable.author ||
+      current_user.can?(:resolve_note, self.project)
+  end
+
+  def resolve!(current_user)
+    return unless resolvable?
+
+    update { |notes| notes.resolve!(current_user) }
+  end
+
+  def unresolve!
+    return unless resolvable?
+
+    update { |notes| notes.unresolve! }
+  end
+end
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 9c80b190b74..e349f743fdd 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -1,71 +1,27 @@
 class DiffDiscussion < Discussion
-  NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+  include DiscussionOnDiff
 
-  delegate  :line_code,
-            :original_line_code,
-            :diff_file,
-            :diff_line,
-            :for_line?,
-            :active?,
+  delegate  :position,
+            :original_position,
 
             to: :first_note
 
-  delegate  :file_path,
-            :blob,
-            :highlighted_diff_lines,
-            :diff_lines,
-
-            to: :diff_file,
-            allow_nil: true
-
   def self.build_discussion_id(note)
-    [*super(note), *unique_position_identifier(note)]
+    [*super(note), *note.position.key]
   end
 
   def self.build_original_discussion_id(note)
     [*Discussion.build_discussion_id(note), *note.original_position.key]
   end
 
-  def self.unique_position_identifier(note)
-    note.position.key
-  end
-
-  def diff_discussion?
-    true
-  end
-
   def legacy_diff_discussion?
     false
   end
 
-  def active?
-    return @active if @active.present?
-
-    @active = first_note.active?
-  end
-  MEMOIZED_VALUES << :active
-
   def reply_attributes
-    super.merge(first_note.diff_attributes)
-  end
-
-  # Returns an array of at most 16 highlighted lines above a diff note
-  def truncated_diff_lines(highlight: true)
-    lines = highlight ? highlighted_diff_lines : diff_lines
-    prev_lines = []
-
-    lines.each do |line|
-      if line.meta?
-        prev_lines.clear
-      else
-        prev_lines << line
-
-        break if for_line?(line)
-
-        prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
-      end
-    end
-
-    prev_lines
+    super.merge(
+      original_position: original_position.to_json,
+      position: position.to_json,
+    )
   end
 end
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index d24491b44e7..38477528279 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -21,23 +21,12 @@ class DiffNote < Note
   before_validation :set_discussion_id, if: :position_changed?
   after_save :keep_around_commits
 
-  def new_diff_note?
-    true
-  end
-
   def discussion_class(*)
     DiffDiscussion
   end
 
-  def diff_attributes
-    {
-      original_position: original_position.to_json,
-      position: position.to_json,
-    }
-  end
-
-  %i(original_position= position=).each do |meth|
-    define_method meth do |new_position|
+  %i(original_position position).each do |meth|
+    define_method "#{meth}=" do |new_position|
       if new_position.is_a?(String)
         new_position = JSON.parse(new_position) rescue nil
       end
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 6e97a4862ed..27ed0480e6d 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -1,5 +1,9 @@
 class Discussion
-  MEMOIZED_VALUES = [] # rubocop:disable Style/MutableConstant
+  cattr_accessor :memoized_values, instance_accessor: false do
+    []
+  end
+
+  include ResolvableDiscussion
 
   attr_reader :notes, :noteable
 
@@ -13,12 +17,6 @@ class Discussion
 
             to: :first_note
 
-  delegate  :resolved_at,
-            :resolved_by,
-
-            to: :last_resolved_note,
-            allow_nil: true
-
   def self.build(notes, noteable = nil)
     notes.first.discussion_class(noteable).new(notes, noteable)
   end
@@ -98,76 +96,9 @@ class Discussion
     notes.length == 1
   end
 
-  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
-  def potentially_resolvable?
-    for_merge_request?
-  end
-
-  def resolvable?
-    return @resolvable if @resolvable.present?
-
-    @resolvable = potentially_resolvable? && notes.any?(&:resolvable?)
-  end
-  MEMOIZED_VALUES << :resolvable
-
-  def resolved?
-    return @resolved if @resolved.present?
-
-    @resolved = resolvable? && notes.none?(&:to_be_resolved?)
-  end
-  MEMOIZED_VALUES << :resolved
-
-  def first_note
-    @first_note ||= notes.first
-  end
-  MEMOIZED_VALUES << :first_note
-
-  def first_note_to_resolve
-    return unless resolvable?
-
-    @first_note_to_resolve ||= notes.find(&:to_be_resolved?)
-  end
-  MEMOIZED_VALUES << :first_note_to_resolve
-
-  def last_resolved_note
-    return unless resolved?
-
-    @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
-  end
-  MEMOIZED_VALUES << :last_resolved_note
-
   def last_note
     @last_note ||= notes.last
   end
-  MEMOIZED_VALUES << :last_note
-
-  def resolved_notes
-    notes.select(&:resolved?)
-  end
-
-  def to_be_resolved?
-    resolvable? && !resolved?
-  end
-
-  def can_resolve?(current_user)
-    return false unless current_user
-    return false unless resolvable?
-
-    current_user == self.noteable.author ||
-      current_user.can?(:resolve_note, self.project)
-  end
-
-  def resolve!(current_user)
-    return unless resolvable?
-
-    update { |notes| notes.resolve!(current_user) }
-  end
-
-  def unresolve!
-    return unless resolvable?
-
-    update { |notes| notes.unresolve! }
-  end
 
   def collapsed?
     resolved?
@@ -192,7 +123,7 @@ class Discussion
     # Set the notes array to the updated notes
     @notes = notes_relation.fresh.to_a
 
-    MEMOIZED_VALUES.each do |var|
+    self.class.memoized_values.each do |var|
       instance_variable_set(:"@#{var}", nil)
     end
   end
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index eb9766a9ffe..53fe9fbab06 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -1,10 +1,8 @@
-class LegacyDiffDiscussion < DiffDiscussion
-  def self.unique_position_identifier(note)
-    note.line_code
-  end
+class LegacyDiffDiscussion < Discussion
+  include DiscussionOnDiff
 
-  def self.build_original_discussion_id(note)
-    Discussion.build_original_discussion_id(note)
+  def self.build_discussion_id(note)
+    [*super(note), note.line_code]
   end
 
   def legacy_diff_discussion?
@@ -19,4 +17,8 @@ class LegacyDiffDiscussion < DiffDiscussion
   def collapsed?
     !active?
   end
+
+  def reply_attributes
+    super.merge(line_code: line_code)
+  end
 end
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index 4952a1ce2ca..0d86610e473 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -11,14 +11,6 @@ class LegacyDiffNote < Note
     LegacyDiffDiscussion
   end
 
-  def legacy_diff_note?
-    true
-  end
-
-  def diff_attributes
-    { line_code: line_code }
-  end
-
   def project_repository
     if RequestStore.active?
       RequestStore.fetch("project:#{project_id}:repository") { self.project.repository }
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 546fd2b8e35..a1efa17180e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -846,8 +846,8 @@ class MergeRequest < ActiveRecord::Base
     return unless has_complete_diff_refs?
     return if new_diff_refs == old_diff_refs
 
-    active_diff_notes = self.notes.diff_notes.select do |note|
-      note.new_diff_note? && note.active?(old_diff_refs)
+    active_diff_notes = self.notes.new_diff_notes.select do |note|
+      note.active?(old_diff_refs)
     end
 
     return if active_diff_notes.empty?
diff --git a/app/models/note.rb b/app/models/note.rb
index 00a58afd2b6..eef868a05d6 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -75,6 +75,7 @@ class Note < ActiveRecord::Base
   end
 
   scope :diff_notes, ->{ where(type: %w(LegacyDiffNote DiffNote)) }
+  scope :new_diff_notes, ->{ where(type: 'DiffNote') }
   scope :non_diff_notes, ->{ where(type: ['Note', 'DiscussionNote', nil]) }
 
   scope :with_associations, -> do
@@ -136,14 +137,6 @@ class Note < ActiveRecord::Base
     false
   end
 
-  def legacy_diff_note?
-    false
-  end
-
-  def new_diff_note?
-    false
-  end
-
   def active?
     true
   end