WIPnew-resolvable-discussion

13 files changed, 324 insertions, 184 deletions

diff --git a/app/models/commit_discussion.rb b/app/models/commit_discussion.rb
new file mode 100644
index 00000000000..c6e026334bb
--- /dev/null
+++ b/app/models/commit_discussion.rb
@@ -0,0 +1,5 @@
+class CommitDiscussion < Discussion
+  def potentially_resolvable?
+    false
+  end
+end
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index b8dd27a7afe..abf8ba1b1d8 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -5,6 +5,10 @@ module NoteOnDiff
     true
   end
 
+  def part_of_discussion?
+    true
+  end
+
   def diff_file
     raise NotImplementedError
   end
@@ -24,12 +28,4 @@ module NoteOnDiff
   def diff_attributes
     raise NotImplementedError
   end
-
-  def can_be_award_emoji?
-    false
-  end
-
-  def to_discussion
-    Discussion.new([self])
-  end
 end
diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb
new file mode 100644
index 00000000000..d34d6e2b203
--- /dev/null
+++ b/app/models/concerns/resolvable_note.rb
@@ -0,0 +1,66 @@
+module ResolvableNote
+  extend ActiveSupport::Concern
+
+  included do
+    belongs_to :resolved_by, class_name: "User"
+
+    validates :resolved_by, presence: true, if: :resolved?
+
+    # Keep this scope in sync with the logic in `#resolvable?` in `Note` subclasses that are resolvable
+    scope :resolvable, -> { where(type: ['DiffNote', 'DiscussionNote']).user.where(noteable_type: 'MergeRequest') }
+    scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
+    scope :unresolved, -> { resolvable.where(resolved_at: nil) }
+  end
+
+  module ClassMethods
+    # To be defined in any Note subclasses that are actually resolvable
+    # def resolvable?
+    #   true
+    # end
+
+    # This method must be kept in sync with `#resolve!`
+    def resolve!(current_user)
+      unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
+    end
+
+    # This method must be kept in sync with `#unresolve!`
+    def unresolve!
+      resolved.update_all(resolved_at: nil, resolved_by_id: nil)
+    end
+  end
+
+  # If you update this method remember to also update the scope `resolvable`
+  def resolvable?
+    self.class.resolvable? && !system?
+  end
+
+  def resolved?
+    return false unless resolvable?
+
+    self.resolved_at.present?
+  end
+
+  def to_be_resolved?
+    resolvable? && !resolved?
+  end
+
+  # If you update this method remember to also update `.resolve!`
+  def resolve!(current_user)
+    return unless resolvable?
+    return if resolved?
+
+    self.resolved_at = Time.now
+    self.resolved_by = current_user
+    save!
+  end
+
+  # If you update this method remember to also update `.unresolve!`
+  def unresolve!
+    return unless resolvable?
+    return unless resolved?
+
+    self.resolved_at = nil
+    self.resolved_by = nil
+    save!
+  end
+end
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
new file mode 100644
index 00000000000..1b24b5d1faa
--- /dev/null
+++ b/app/models/diff_discussion.rb
@@ -0,0 +1,61 @@
+class DiffDiscussion < Discussion
+  NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+
+  delegate  :line_code,
+            :original_line_code,
+            :diff_file,
+            :for_line?,
+            :active?,
+
+            to: :first_note
+
+  delegate  :blob,
+            :highlighted_diff_lines,
+            :diff_lines,
+
+            to: :diff_file,
+            allow_nil: true
+
+  def diff_discussion?
+    true
+  end
+
+  def legacy_diff_discussion?
+    false
+  end
+
+  def potentially_resolvable?
+    first_note.for_merge_request?
+  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
+  end
+end
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 559b3075905..829c8516a5d 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -9,15 +9,9 @@ class DiffNote < Note
   validates :diff_line, presence: true
   validates :line_code, presence: true, line_code: true
   validates :noteable_type, inclusion: { in: ['Commit', 'MergeRequest'] }
-  validates :resolved_by, presence: true, if: :resolved?
   validate :positions_complete
   validate :verify_supported
 
-  # Keep this scope in sync with the logic in `#resolvable?`
-  scope :resolvable, -> { user.where(noteable_type: 'MergeRequest') }
-  scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
-  scope :unresolved, -> { resolvable.where(resolved_at: nil) }
-
   after_initialize :ensure_original_discussion_id
   before_validation :set_original_position, :update_position, on: :create
   before_validation :set_line_code, :set_original_discussion_id
@@ -27,18 +21,12 @@ class DiffNote < Note
   after_save :keep_around_commits
 
   class << self
-    def build_discussion_id(noteable_type, noteable_id, position)
-      [super(noteable_type, noteable_id), *position.key].join("-")
+    def resolvable?
+      true
     end
 
-    # This method must be kept in sync with `#resolve!`
-    def resolve!(current_user)
-      unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
-    end
-
-    # This method must be kept in sync with `#unresolve!`
-    def unresolve!
-      resolved.update_all(resolved_at: nil, resolved_by_id: nil)
+    def build_discussion_id(noteable_type, noteable_id, position)
+      [super(noteable_type, noteable_id), *position.key].join("-")
     end
   end
 
@@ -46,6 +34,10 @@ class DiffNote < Note
     true
   end
 
+  def discussion_class
+    DiffDiscussion
+  end
+
   def diff_attributes
     { position: position.to_json }
   end
@@ -88,41 +80,8 @@ class DiffNote < Note
     self.position.diff_refs == diff_refs
   end
 
-  # If you update this method remember to also update the scope `resolvable`
   def resolvable?
-    !system? && for_merge_request?
-  end
-
-  def resolved?
-    return false unless resolvable?
-
-    self.resolved_at.present?
-  end
-
-  # If you update this method remember to also update `.resolve!`
-  def resolve!(current_user)
-    return unless resolvable?
-    return if resolved?
-
-    self.resolved_at = Time.now
-    self.resolved_by = current_user
-    save!
-  end
-
-  # If you update this method remember to also update `.unresolve!`
-  def unresolve!
-    return unless resolvable?
-    return unless resolved?
-
-    self.resolved_at = nil
-    self.resolved_by = nil
-    save!
-  end
-
-  def discussion
-    return unless resolvable?
-
-    self.noteable.find_diff_discussion(self.discussion_id)
+    super && for_merge_request?
   end
 
   private
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 75a85563235..ceadaebe996 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -1,5 +1,5 @@
 class Discussion
-  NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+  MEMOIZED_VALUES = []
 
   attr_reader :notes
 
@@ -11,12 +11,6 @@ class Discussion
             :for_commit?,
             :for_merge_request?,
 
-            :line_code,
-            :original_line_code,
-            :diff_file,
-            :for_line?,
-            :active?,
-
             to: :first_note
 
   delegate  :resolved_at,
@@ -25,19 +19,12 @@ class Discussion
             to: :last_resolved_note,
             allow_nil: true
 
-  delegate  :blob,
-            :highlighted_diff_lines,
-            :diff_lines,
-
-            to: :diff_file,
-            allow_nil: true
-
-  def self.for_notes(notes)
-    notes.group_by(&:discussion_id).values.map { |notes| new(notes) }
+  def self.build(notes)
+    notes.first.discussion_class.new(notes)
   end
 
-  def self.for_diff_notes(notes)
-    notes.group_by(&:line_code).values.map { |notes| new(notes) }
+  def self.build_collection(notes)
+    notes.group_by(&:discussion_id).values.map { |notes| build(notes) }
   end
 
   def initialize(notes)
@@ -49,6 +36,7 @@ class Discussion
 
     @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
   end
+  MEMOIZED_VALUES << :last_resolved_note
 
   def last_updated_at
     last_note.created_at
@@ -65,32 +53,40 @@ class Discussion
   alias_method :to_param, :id
 
   def diff_discussion?
-    first_note.diff_note?
+    false
   end
 
-  def legacy_diff_discussion?
-    notes.any?(&:legacy_diff_note?)
+  def potentially_resolvable?
+    true
+  end
+
+  def single_note?
+    false
   end
 
   def resolvable?
     return @resolvable if @resolvable.present?
 
-    @resolvable = diff_discussion? && notes.any?(&:resolvable?)
+    @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 last_note
     @last_note ||= @notes.last
   end
+  MEMOIZED_VALUES << :last_note
 
   def resolved_notes
     notes.select(&:resolved?)
@@ -120,25 +116,12 @@ class Discussion
     update { |notes| notes.unresolve! }
   end
 
-  def for_target?(target)
-    self.noteable == target && !diff_discussion?
-  end
-
-  def active?
-    return @active if @active.present?
-
-    @active = first_note.active?
-  end
-
   def collapsed?
-    return false unless diff_discussion?
-
     if resolvable?
       # New diff discussions only disappear once they are marked resolved
       resolved?
     else
-      # Old diff discussions disappear once they become outdated
-      !active?
+      false
     end
   end
 
@@ -147,52 +130,28 @@ class Discussion
   end
 
   def reply_attributes
-    data = {
+    {
       noteable_type: first_note.noteable_type,
       noteable_id:   first_note.noteable_id,
       commit_id:     first_note.commit_id,
       discussion_id: self.id,
+      note_type:     first_note.type
     }
-
-    if diff_discussion?
-      data[:note_type] = first_note.type
-
-      data.merge!(first_note.diff_attributes)
-    end
-
-    data
-  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
 
   private
 
   def update
-    notes_relation = DiffNote.where(id: notes.map(&:id)).fresh
+    # Do not select `Note.resolvable`, so that system notes remain in the collection
+    notes_relation = Note.where(id: notes.map(&:id))
+
     yield(notes_relation)
 
     # Set the notes array to the updated notes
-    @notes = notes_relation.to_a
+    @notes = notes_relation.fresh.to_a
 
-    # Reset the memoized values
-    @last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil
+    MEMOIZED_VALUES.each do |var|
+      instance_variable_set(:"@#{var}", nil)
+    end
   end
 end
diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb
new file mode 100644
index 00000000000..b1b4905a512
--- /dev/null
+++ b/app/models/discussion_note.rb
@@ -0,0 +1,27 @@
+class DiscussionNote < Note
+  validates :noteable_type, inclusion: { in: ['MergeRequest'] }
+
+  class << self
+    def resolvable?
+      true
+    end
+  end
+
+  def part_of_discussion?
+    true
+  end
+
+  def discussion_class
+    Discussion
+  end
+
+  private
+
+  def set_discussion_id
+    if in_reply_to_discussion_id
+      self.discussion_id = in_reply_to_discussion_id
+    else
+      super
+    end
+  end
+end
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
new file mode 100644
index 00000000000..59bc68da052
--- /dev/null
+++ b/app/models/legacy_diff_discussion.rb
@@ -0,0 +1,13 @@
+class LegacyDiffDiscussion < DiffDiscussion
+  def legacy_diff_discussion?
+    true
+  end
+
+  def potentially_resolvable?
+    false
+  end
+
+  def collapsed?
+    !active?
+  end
+end
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index 40277a9b139..b71aa71b8fe 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -13,6 +13,10 @@ class LegacyDiffNote < Note
     end
   end
 
+  def discussion_class
+    LegacyDiffDiscussion
+  end
+
   def legacy_diff_note?
     true
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 38d8c15e6b0..4b64894f182 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -453,7 +453,7 @@ class MergeRequest < ActiveRecord::Base
     should_remove_source_branch? || force_remove_source_branch?
   end
 
-  def mr_and_commit_notes
+  def related_notes
     # Fetch comments only from last 100 commits
     commits_for_notes_limit = 100
     commit_ids = commits.last(commits_for_notes_limit).map(&:id)
@@ -469,29 +469,25 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def discussions
-    @discussions ||= self.mr_and_commit_notes.
+    @discussions ||= self.related_notes.
       inc_relations_for_view.
-      fresh.
       discussions
   end
 
-  def diff_discussions
-    @diff_discussions ||= self.notes.diff_notes.discussions
+  def find_discussion(discussion_id)
+    self.related_notes.find_discussion(discussion_id)
   end
 
-  def find_diff_discussion(discussion_id)
-    notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a
-    return if notes.empty?
-
-    Discussion.new(notes)
+  def resolvable_discussions
+    @resolvable_discussions ||= self.notes.resolvable.discussions
   end
 
   def discussions_resolvable?
-    diff_discussions.any?(&:resolvable?)
+    resolvable_discussions.any?(&:resolvable?)
   end
 
   def discussions_resolved?
-    discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?)
+    discussions_resolvable? && resolvable_discussions.none?(&:to_be_resolved?)
   end
 
   def discussions_to_be_resolved?
diff --git a/app/models/note.rb b/app/models/note.rb
index 5b50ca285c3..3e8aa49a601 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -8,6 +8,7 @@ class Note < ActiveRecord::Base
   include FasterCacheKeys
   include CacheMarkdownField
   include AfterCommitQueue
+  include ResolvableNote
 
   cache_markdown_field :note, pipeline: :note
 
@@ -22,6 +23,9 @@ class Note < ActiveRecord::Base
   # Attribute used to store the attributes that have ben changed by slash commands.
   attr_accessor :commands_changes
 
+  # The discussion ID for the `DiscussionNote` thread being replied to
+  attr_accessor :in_reply_to_discussion_id
+
   default_value_for :system, false
 
   attr_mentionable :note, pipeline: :note
@@ -32,9 +36,6 @@ class Note < ActiveRecord::Base
   belongs_to :author, class_name: "User"
   belongs_to :updated_by, class_name: "User"
 
-  # Only used by DiffNote, but defined here so that it can be used in `Note.includes`
-  belongs_to :resolved_by, class_name: "User"
-
   has_many :todos, dependent: :destroy
   has_many :events, as: :target, dependent: :destroy
 
@@ -52,6 +53,7 @@ class Note < ActiveRecord::Base
   validates :noteable_id, presence: true, unless: [:for_commit?, :importing?]
   validates :commit_id, presence: true, if: :for_commit?
   validates :author, presence: true
+  validates :discussion_id, presence: true, format: { with: /\A\h{40}\z/ }
 
   validate unless: [:for_commit?, :importing?] do |note|
     unless note.noteable.try(:project) == note.project
@@ -71,8 +73,9 @@ class Note < ActiveRecord::Base
   scope :inc_author, ->{ includes(:author) }
   scope :inc_relations_for_view, ->{ includes(:project, :author, :updated_by, :resolved_by, :award_emoji) }
 
+  scope :discussion_notes, ->{ where(type: 'DiscussionNote') }
   scope :diff_notes, ->{ where(type: ['LegacyDiffNote', 'DiffNote']) }
-  scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
+  scope :non_diff_notes, ->{ where(type: ['Note', 'DiscussionNote', nil]) }
 
   scope :with_associations, -> do
     # FYI noteable cannot be loaded for LegacyDiffNote for commits
@@ -98,14 +101,28 @@ class Note < ActiveRecord::Base
       Digest::SHA1.hexdigest(build_discussion_id(*args))
     end
 
+    def resolvable?
+      false
+    end
+
     def discussions
-      Discussion.for_notes(all)
+      Discussion.build_collection(all.fresh)
+    end
+
+    def find_discussion(discussion_id)
+      notes = where(discussion_id: discussion_id).fresh.to_a
+      return if notes.empty?
+
+      Discussion.build(notes)
     end
 
     def grouped_diff_discussions
-      active_notes = diff_notes.fresh.select(&:active?)
-      Discussion.for_diff_notes(active_notes).
-        map { |d| [d.line_code, d] }.to_h
+      diff_notes.
+        fresh.
+        select(&:active?).
+        group_by(&:line_code).
+        map { |line_code, notes| [line_code, DiffDiscussion.build(notes)] }.
+        to_h
     end
 
     # Searches for notes matching the given query.
@@ -146,18 +163,10 @@ class Note < ActiveRecord::Base
     true
   end
 
-  def resolvable?
-    false
-  end
-
-  def resolved?
+  def part_of_discussion?
     false
   end
 
-  def to_be_resolved?
-    resolvable? && !resolved?
-  end
-
   def max_attachment_size
     current_application_settings.max_attachment_size.megabytes.to_i
   end
@@ -226,7 +235,7 @@ class Note < ActiveRecord::Base
   end
 
   def can_be_award_emoji?
-    noteable.is_a?(Awardable)
+    noteable.is_a?(Awardable) && !part_of_discussion?
   end
 
   def contains_emoji_only?
@@ -237,6 +246,29 @@ class Note < ActiveRecord::Base
     note.match(Banzai::Filter::EmojiFilter.emoji_pattern)[1]
   end
 
+  def discussion_class
+    if for_merge_request?
+      # Notes on merge requests are always in a discussion of their own
+      SingleNoteDiscussion
+    else
+      CommitDiscussion
+    end
+  end
+
+  # Returns a discussion containing just this note
+  def to_discussion
+    Discussion.build([self])
+  end
+
+  # Returns the entire discussion this note is part of
+  def discussion
+    if part_of_discussion?
+      self.noteable.notes.find_discussion(self.discussion_id)
+    else
+      to_discussion
+    end
+  end
+
   private
 
   def keep_around_commit
diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb
index f4bcb49b34d..558e4c1a7eb 100644
--- a/app/models/sent_notification.rb
+++ b/app/models/sent_notification.rb
@@ -5,10 +5,11 @@ class SentNotification < ActiveRecord::Base
   belongs_to :noteable, polymorphic: true
   belongs_to :recipient, class_name: "User"
 
-  validates :project, :recipient, :reply_key, presence: true
-  validates :reply_key, uniqueness: true
+  validates :project, :recipient, presence: true
+  validates :reply_key, presence: true, uniqueness: true
   validates :noteable_id, presence: true, unless: :for_commit?
   validates :commit_id, presence: true, if: :for_commit?
+  validates :in_reply_to_discussion_id, format: { with: /\A\h{40}\z/, if: :discussion_note? }
   validate :note_valid
 
   after_save :keep_around_commit
@@ -34,23 +35,24 @@ class SentNotification < ActiveRecord::Base
       end
 
       attrs.reverse_merge!(
-        project:        noteable.project,
-        noteable_type:  noteable.class.name,
-        noteable_id:    noteable_id,
-        commit_id:      commit_id,
-        recipient_id:   recipient_id,
-        reply_key:      reply_key
+        project: noteable.project,
+        noteable_type: noteable.class.name,
+        noteable_id: noteable_id,
+        commit_id: commit_id,
+        recipient_id: recipient_id,
+        reply_key: reply_key
       )
 
       create(attrs)
     end
 
     def record_note(note, recipient_id, reply_key, attrs = {})
-      if note.diff_note?
-        attrs[:note_type] = note.type
+      attrs.merge!(
+        note_type: note.type,
+        in_reply_to_discussion_id: note.discussion_id
+      )
 
-        attrs.merge!(note.diff_attributes)
-      end
+      attrs.merge!(note.diff_attributes) if note.diff_note?
 
       record(note.noteable, recipient_id, reply_key, attrs)
     end
@@ -64,6 +66,10 @@ class SentNotification < ActiveRecord::Base
     noteable_type == "Commit"
   end
 
+  def discussion_note?
+    note_type == DiscussionNote.name
+  end
+
   def noteable
     if for_commit?
       project.commit(commit_id) rescue nil
@@ -89,31 +95,38 @@ class SentNotification < ActiveRecord::Base
     self.reply_key
   end
 
-  def note_attributes
-    {
-      project:        self.project,
-      author:         self.recipient,
-      type:           self.note_type,
-      noteable_type:  self.noteable_type,
-      noteable_id:    self.noteable_id,
-      commit_id:      self.commit_id,
-      line_code:      self.line_code,
-      position:       self.position.to_json
-    }
-  end
-
   def create_note(note)
     Notes::CreateService.new(
       self.project,
       self.recipient,
-      self.note_attributes.merge(note: note)
+      self.note_params.merge(note: note)
     ).execute
   end
 
   private
 
+  def note_params
+    {
+      project: self.project,
+      author: self.recipient,
+      type: self.note_type,
+      noteable_type: self.noteable_type,
+      noteable_id: self.noteable_id,
+      commit_id: self.commit_id,
+
+      # LegacyDiffNote
+      line_code: self.line_code,
+
+      # DiffNote
+      position: self.position.to_json,
+
+      # DiscussionNote
+      in_reply_to_discussion_id: self.in_reply_to_discussion_id
+    }
+  end
+
   def note_valid
-    Note.new(note_attributes.merge(note: "Test")).valid?
+    Note.new(note_params.merge(note: "Test")).valid?
   end
 
   def keep_around_commit
diff --git a/app/models/single_note_discussion.rb b/app/models/single_note_discussion.rb
new file mode 100644
index 00000000000..ee1aecb6b96
--- /dev/null
+++ b/app/models/single_note_discussion.rb
@@ -0,0 +1,9 @@
+class SingleNoteDiscussion < Discussion
+  def potentially_resolvable?
+    false
+  end
+
+  def single_note?
+    true
+  end
+end