Add option to start a new discussion on an MR

1 files changed, 74 insertions, 38 deletions

diff --git a/app/models/note.rb b/app/models/note.rb
index 16d66cb1427..6385747b571 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
 
@@ -32,9 +33,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
   has_one :system_note_metadata
@@ -54,6 +52,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, :original_discussion_id, presence: true, format: { with: /\A\h{40}\z/ }
 
   validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note|
     unless note.noteable.try(:project) == note.project
@@ -76,7 +75,7 @@ class Note < ActiveRecord::Base
   end
 
   scope :diff_notes, ->{ where(type: %w(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
@@ -84,9 +83,9 @@ class Note < ActiveRecord::Base
              project: [:project_members, { group: [:group_members] }])
   end
 
-  after_initialize :ensure_discussion_id
+  after_initialize :ensure_discussion_id, :ensure_original_discussion_id
   before_validation :nullify_blank_type, :nullify_blank_line_code
-  before_validation :set_discussion_id
+  before_validation :set_discussion_id, :set_original_discussion_id, on: :create
   after_save :keep_around_commit, unless: :for_personal_snippet?
   after_save :expire_etag_cache
 
@@ -95,22 +94,31 @@ class Note < ActiveRecord::Base
       ActiveModel::Name.new(self, nil, 'note')
     end
 
-    def build_discussion_id(noteable_type, noteable_id)
-      [:discussion, noteable_type.try(:underscore), noteable_id].join("-")
+    def discussions(noteable = nil)
+      Discussion.build_collection(fresh, noteable)
     end
 
-    def discussion_id(*args)
-      Digest::SHA1.hexdigest(build_discussion_id(*args))
+    def find_original_discussion(discussion_id)
+      note = find_by(original_discussion_id: discussion_id)
+      return unless note
+
+      note.to_discussion
     end
 
-    def discussions
-      Discussion.for_notes(fresh)
+    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
 
     def count_for_collection(ids, type)
@@ -121,7 +129,7 @@ class Note < ActiveRecord::Base
   end
 
   def cross_reference?
-    system && SystemNoteService.cross_reference?(note)
+    system? && SystemNoteService.cross_reference?(note)
   end
 
   def diff_note?
@@ -140,18 +148,6 @@ class Note < ActiveRecord::Base
     true
   end
 
-  def resolvable?
-    false
-  end
-
-  def resolved?
-    false
-  end
-
-  def to_be_resolved?
-    resolvable? && !resolved?
-  end
-
   def max_attachment_size
     current_application_settings.max_attachment_size.megabytes.to_i
   end
@@ -228,7 +224,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?
@@ -239,6 +235,42 @@ class Note < ActiveRecord::Base
     for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore
   end
 
+  def can_be_discussion_note?
+    DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type)
+  end
+
+  def discussion_class(noteable = nil)
+    # When commit notes are rendered on an MR's Discussion page, they are
+    # displayed in one discussion instead of individually
+    if noteable && noteable != self.noteable && for_commit?
+      CommitDiscussion
+    else
+      IndividualNoteDiscussion
+    end
+  end
+
+  def discussion_id(noteable = nil)
+    discussion_class(noteable).override_discussion_id(self) || super()
+  end
+
+  # Returns a discussion containing just this note
+  def to_discussion(noteable = nil)
+    Discussion.build([self], noteable)
+  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
+
+  def part_of_discussion?
+    !to_discussion.render_as_individual_notes?
+  end
+
   private
 
   def keep_around_commit
@@ -264,17 +296,21 @@ class Note < ActiveRecord::Base
   end
 
   def set_discussion_id
-    self.discussion_id = Digest::SHA1.hexdigest(build_discussion_id)
+    self.discussion_id ||= discussion_class.discussion_id(self)
   end
 
-  def build_discussion_id
-    if for_merge_request?
-      # Notes on merge requests are always in a discussion of their own,
-      # so we generate a unique discussion ID.
-      [:discussion, :note, SecureRandom.hex].join("-")
-    else
-      self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
-    end
+  def ensure_original_discussion_id
+    return unless self.persisted?
+    # Needed in case the SELECT statement doesn't ask for `original_discussion_id`
+    return unless self.has_attribute?(:original_discussion_id)
+    return if self.original_discussion_id
+
+    set_original_discussion_id
+    update_column(:original_discussion_id, self.original_discussion_id)
+  end
+
+  def set_original_discussion_id
+    self.original_discussion_id = discussion_class.original_discussion_id(self)
   end
 
   def expire_etag_cache