Add option to start a new discussion on an MR

app/models/issue.rb

@@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord'
 class Issue < ActiveRecord::Base
   include InternalId
   include Issuable
+  include Noteable
   include Referable
   include Sortable
   include Spammable

app/models/legacy_diff_discussion.rb

+class LegacyDiffDiscussion < DiffDiscussion
+  def self.unique_position_identifier(note)
+    note.line_code
+  end
+
+  def self.build_original_discussion_id(note)
+    Discussion.build_original_discussion_id(note)
+  end
+
+  def legacy_diff_discussion?
+    true
+  end
+
+  def potentially_resolvable?
+    false
+  end
+
+  def collapsed?
+    !active?
+  end
+end

app/models/legacy_diff_note.rb

@@ -7,10 +7,8 @@ class LegacyDiffNote < Note
   before_create :set_diff
-  class << self
-    def build_discussion_id(noteable_type, noteable_id, line_code)
-      [super(noteable_type, noteable_id), line_code].join("-")
-    end
+  def discussion_class(*)
+    LegacyDiffDiscussion
   end
   def legacy_diff_note?
@@ -119,8 +117,4 @@ class LegacyDiffNote < Note
     diffs = noteable.raw_diffs(Commit.max_diff_options)
     diffs.find { |d| d.new_path == self.diff.new_path }
   end
-
-  def build_discussion_id
-    self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
-  end
 end

app/models/merge_request.rb

 class MergeRequest < ActiveRecord::Base
   include InternalId
   include Issuable
+  include Noteable
   include Referable
   include Sortable
@@ -475,44 +476,32 @@ class MergeRequest < ActiveRecord::Base
     )
   end
-  def discussions
-    @discussions ||= self.related_notes.
-      inc_relations_for_view.
-      fresh.
-      discussions
-  end
-
-  def diff_discussions
-    @diff_discussions ||= self.notes.diff_notes.discussions
-  end
+  alias_method :discussion_notes, :related_notes
   def resolvable_discussions
-    @resolvable_discussions ||= diff_discussions.select(&:to_be_resolved?)
-  end
-
-  def discussions_can_be_resolved_by?(user)
-    resolvable_discussions.all? { |discussion| discussion.can_resolve?(user) }
-  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)
+    @resolvable_discussions ||= 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?
     discussions_resolvable? && !discussions_resolved?
   end
+  def discussions_to_be_resolved
+    @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?)
+  end
+
+  def discussions_can_be_resolved_by?(user)
+    discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) }
+  end
+
   def mergeable_discussions_state?
     return true unless project.only_allow_merge_if_all_discussions_are_resolved?

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

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/, allow_nil: true }
   validate :note_valid
   after_save :keep_around_commit
@@ -34,23 +35,20 @@ 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,
+        recipient_id: recipient_id,
+        reply_key: reply_key,
+
+        noteable_type: noteable.class.name,
+        noteable_id: noteable_id,
+        commit_id: commit_id,
       )
       create(attrs)
     end
     def record_note(note, recipient_id, reply_key, attrs = {})
-      if note.diff_note?
-        attrs[:note_type] = note.type
-
-        attrs.merge!(note.diff_attributes)
-      end
+      attrs[:in_reply_to_discussion_id] = note.original_discussion_id
       record(note.noteable, recipient_id, reply_key, attrs)
     end
@@ -89,31 +87,34 @@ 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
+  def note_params
+    attrs = {
+      noteable_type: self.noteable_type,
+      noteable_id: self.noteable_id,
+      commit_id: self.commit_id
     }
-  end
-  def create_note(note)
-    Notes::CreateService.new(
-      self.project,
-      self.recipient,
-      self.note_attributes.merge(note: note)
-    ).execute
+    if self.in_reply_to_discussion_id.present?
+      attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id
+    else
+      attrs.merge!(
+        type: self.note_type,
+
+        # LegacyDiffNote
+        line_code: self.line_code,
+
+        # DiffNote
+        position: self.position.to_json
+      )
+    end
+
+    attrs
   end
   private
   def note_valid
-    Note.new(note_attributes.merge(note: "Test")).valid?
+    Notes::BuildService.new(self.project, self.recipient, note_params.merge(note: 'Test')).execute.valid?
   end
   def keep_around_commit

app/models/simple_discussion.rb

+class SimpleDiscussion < Discussion
+  def self.build_discussion_id(note)
+    [*super(note), SecureRandom.hex]
+  end
+
+  def reply_attributes
+    super.merge(discussion_id: self.id)
+  end
+end

app/models/snippet.rb

@@ -2,6 +2,7 @@ class Snippet < ActiveRecord::Base
   include Gitlab::VisibilityLevel
   include Linguist::BlobHelper
   include CacheMarkdownField
+  include Noteable
   include Participable
   include Referable
   include Sortable

app/services/concerns/issues/resolve_discussions.rb

@@ -25,7 +25,7 @@ module Issues
           Array(discussion_or_nil)
         else
           merge_request_to_resolve_discussions_of
-            .resolvable_discussions
+            .discussions_to_be_resolved
         end
     end
   end

app/services/notes/build_service.rb

+module Notes
+  class BuildService < BaseService
+    def execute
+      # TODO: Remove when we use a selectbox instead of a submit button
+      params[:type] = DiscussionNote.name if params.delete(:new_discussion)
+
+      in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
+      if project && in_reply_to_discussion_id.present?
+        discussion =
+          project.notes.find_original_discussion(in_reply_to_discussion_id) ||
+          project.notes.find_discussion(in_reply_to_discussion_id)
+
+        unless discussion
+          note = Note.new
+          note.errors.add(:base, 'Discussion to reply to cannot be found')
+          return note
+        end
+
+        params.merge!(discussion.reply_attributes)
+      end
+
+      note = Note.new(params)
+      note.project = project
+      note.author = current_user
+
+      note
+    end
+  end
+end

app/services/notes/create_service.rb

@@ -3,10 +3,8 @@ module Notes
     def execute
       merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
-      note = Note.new(params)
-      note.project = project
-      note.author  = current_user
-      note.system  = false
+      note = Notes::BuildService.new(project, current_user, params).execute
+      return note unless note.valid?
       # We execute commands (extracted from `params[:note]`) on the noteable
       # **before** we save the note because if the note consists of commands

app/services/system_note_service.rb

@@ -228,12 +228,10 @@ module SystemNoteService
   def discussion_continued_in_issue(discussion, project, author, issue)
     body = "created #{issue.to_reference} to continue this discussion"
-    note_params = discussion.reply_attributes.merge(project: project, author: author, note: body)
-    note_params[:type] = note_params.delete(:note_type)
-
-    note = Note.create(note_params.merge(system: true))
-    note.system_note_metadata = SystemNoteMetadata.new({ action: 'discussion' })
+    note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
+    note = Note.create(note_attributes.merge(system: true))
+    note.system_note_metadata = SystemNoteMetadata.new(action: 'discussion')
     note
   end

app/views/discussions/_discussion.html.haml

@@ -18,19 +18,21 @@
           .inline.discussion-headline-light
             = discussion.author.to_reference
-            started a discussion on
-            - if discussion.for_commit?
+            started a discussion
+            - if discussion.for_commit? && @noteable != discussion.noteable
+              on
               - commit = discussion.noteable
               - if commit
                 commit
-                = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code), class: 'monospace'
+                - anchor = discussion.line_code if discussion.diff_discussion?
+                = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: anchor), class: 'monospace'
               - else
                 a deleted commit
-            - else
+            - elsif discussion.diff_discussion?
+              on
               - if discussion.active?
-                = link_to diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code) do
-                  the diff
+                = link_to 'the diff', discussion_diff_path(discussion)
               - else
                 an outdated diff

app/views/discussions/_notes.html.haml

-%ul.notes{ data: { discussion_id: discussion.id } }
+%ul.notes{ data: { discussion_id: discussion.id, original_discussion_id: discussion.original_id } }
   = render partial: "projects/notes/note", collection: discussion.notes, as: :note
 - if current_user
   .discussion-reply-holder
-    - if discussion.diff_discussion?
+    - if discussion.potentially_resolvable?
       - line_type = local_assigns.fetch(:line_type, nil)
       .btn-group-justified.discussion-with-resolve-btn{ role: "group" }
         .btn-group{ role: "group" }
           = link_to_reply_discussion(discussion, line_type)
+
         = render "discussions/resolve_all", discussion: discussion
-        - if discussion.for_merge_request?
-          .btn-group.discussion-actions
-            = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable
-            = render "discussions/jump_to_next", discussion: discussion
+        
+        .btn-group.discussion-actions
+          = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable
+          = render "discussions/jump_to_next", discussion: discussion
     - else
       = link_to_reply_discussion(discussion)

app/views/discussions/_resolve_all.html.haml

-- if discussion.for_merge_request?
-  %resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'",
-      ":merge-request-id" => discussion.noteable.iid,
-      ":can-resolve" => discussion.can_resolve?(current_user),
-      "inline-template" => true }
-    .btn-group{ role: "group", "v-if" => "showButton" }
-      %button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading", "v-cloak" => "true" }
-        = icon("spinner spin", "v-show" => "loading")
-        {{ buttonText }}
+%resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'",
+    ":merge-request-id" => discussion.noteable.iid,
+    ":can-resolve" => discussion.can_resolve?(current_user),
+    "inline-template" => true }
+  .btn-group{ role: "group", "v-if" => "showButton" }
+    %button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading", "v-cloak" => "true" }
+      = icon("spinner spin", "v-show" => "loading")
+      {{ buttonText }}

app/views/projects/commit/show.html.haml

@@ -10,6 +10,7 @@
   - else
     .block-connector
   = render "projects/diffs/diffs", diffs: @diffs, environment: @environment
+
   = render "projects/notes/notes_with_form"
   - if can_collaborate_with_project?
     - %w(revert cherry-pick).each do |type|

app/views/projects/notes/_form.html.haml

@@ -4,12 +4,18 @@
   = hidden_field_tag :view, diff_view
   = hidden_field_tag :line_type
   = hidden_field_tag :merge_request_diff_head_sha, @note.noteable.try(:diff_head_sha)
+  = hidden_field_tag :in_reply_to_discussion_id
+
   = note_target_fields(@note)
-  = f.hidden_field :commit_id
-  = f.hidden_field :line_code
-  = f.hidden_field :noteable_id
   = f.hidden_field :noteable_type
+  = f.hidden_field :noteable_id
+  = f.hidden_field :commit_id
   = f.hidden_field :type
+
+  -# LegacyDiffNote
+  = f.hidden_field :line_code
+
+  -# DiffNote
   = f.hidden_field :position
   = render layout: 'projects/md_preview', locals: { preview_class: "md-preview", referenced_users: true } do
@@ -23,6 +29,11 @@
   .note-form-actions.clearfix
     = f.submit 'Comment', class: "btn btn-nr btn-create append-right-10 comment-btn js-comment-button"
+
+    - if @note.can_be_discussion_note?
+      = submit_tag 'Start discussion', name: 'new_discussion', class: "btn btn-nr append-right-10 btn-inverted js-note-new-discussion"
+
     = yield(:note_actions)
+
     %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
       Discard draft

app/views/projects/notes/_note.html.haml

@@ -31,7 +31,7 @@
             - if note.resolvable?
               - can_resolve = can?(current_user, :resolve_note, note)
               %resolve-btn{ "project-path" => project_path(note.project),
-                  "discussion-id" => note.discussion_id,
+                  "discussion-id" => note.discussion_id(@noteable),
                   ":note-id" => note.id,
                   ":resolved" => note.resolved?,
                   ":can-resolve" => can_resolve,

app/views/projects/notes/_notes.html.haml

 - if @discussions.present?
   - @discussions.each do |discussion|
-    - if discussion.for_target?(@noteable)
-      = render partial: "projects/notes/note", object: discussion.first_note, as: :note
+    - if discussion.render_as_individual_notes?
+      = render partial: "projects/notes/note", collection: discussion.notes, as: :note
     - else
       = render 'discussions/discussion', discussion: discussion
 - else

changelogs/unreleased/new-resolvable-discussion.yml

+---
+title: Add option to start a new resolvable discussion in an MR
+merge_request: 
+author: