Add option to start a new discussion on an MR

6 files changed, 70 insertions, 83 deletions

diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index cc67f688d51..98cd4ce344a 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -2,6 +2,7 @@
 #
 # Not to be confused with CommitsController, plural.
 class Projects::CommitController < Projects::ApplicationController
+  include NotesHelper
   include CreatesCommit
   include DiffForPath
   include DiffHelper
@@ -111,22 +112,19 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def define_note_vars
-    @grouped_diff_discussions = commit.notes.grouped_diff_discussions
-    @notes = commit.notes.non_diff_notes.fresh
-
-    Banzai::NoteRenderer.render(
-      @grouped_diff_discussions.values.flat_map(&:notes) + @notes,
-      @project,
-      current_user,
-    )
-
+    @noteable = @commit
     @note = @project.build_commit_note(commit)
 
-    @noteable = @commit
-    @comments_target = {
+    @new_diff_note_attrs = {
       noteable_type: 'Commit',
       commit_id: @commit.id
     }
+
+    @grouped_diff_discussions = commit.grouped_diff_discussions
+    @discussions = commit.discussions
+
+    @notes = (@grouped_diff_discussions.values + @discussions).flat_map(&:notes)
+    @notes = prepare_notes_for_rendering(@notes)
   end
 
   def assign_change_commit_vars
diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb
index 1349b015a63..f4a18a5e8f7 100644
--- a/app/controllers/projects/discussions_controller.rb
+++ b/app/controllers/projects/discussions_controller.rb
@@ -28,7 +28,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
   end
 
   def discussion
-    @discussion ||= @merge_request.find_diff_discussion(params[:id]) || render_404
+    @discussion ||= @merge_request.find_discussion(params[:id]) || render_404
   end
 
   def authorize_resolve_discussion!
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index d984e6d3918..0c6267f065b 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -84,15 +84,11 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def show
-    raw_notes = @issue.notes.inc_relations_for_view.fresh
-
-    @notes = Banzai::NoteRenderer.
-      render(raw_notes, @project, current_user, @path, @project_wiki, @ref)
-
-    @note     = @project.notes.new(noteable: @issue)
     @noteable = @issue
+    @note     = @project.notes.new(noteable: @issue)
 
-    preload_max_access_for_authors(@notes, @project)
+    @discussions = @issue.discussions
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
 
     respond_to do |format|
       format.html
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 37e3ac05916..7863fe18f32 100755
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -570,20 +570,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @note = @project.notes.new(noteable: @merge_request)
 
     @discussions = @merge_request.discussions
-
-    preload_noteable_for_regular_notes(@discussions.flat_map(&:notes))
-
-    # This is not executed lazily
-    @notes = Banzai::NoteRenderer.render(
-      @discussions.flat_map(&:notes),
-      @project,
-      current_user,
-      @path,
-      @project_wiki,
-      @ref
-    )
-
-    preload_max_access_for_authors(@notes, @project)
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
   end
 
   def define_widget_vars
@@ -596,22 +583,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def define_diff_comment_vars
-    @comments_target = {
+    @new_diff_note_attrs = {
       noteable_type: 'MergeRequest',
       noteable_id: @merge_request.id
     }
 
     @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
-    @grouped_diff_discussions = @merge_request.notes.inc_relations_for_view.grouped_diff_discussions
-
-    Banzai::NoteRenderer.render(
-      @grouped_diff_discussions.values.flat_map(&:notes),
-      @project,
-      current_user,
-      @path,
-      @project_wiki,
-      @ref
-    )
+
+    @grouped_diff_discussions = @merge_request.grouped_diff_discussions
+    @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flat_map(&:notes))
   end
 
   def define_pipelines_vars
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index d00177e7612..f80313bbee0 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -6,13 +6,14 @@ class Projects::NotesController < Projects::ApplicationController
   before_action :authorize_create_note!, only: [:create]
   before_action :authorize_admin_note!, only: [:update, :destroy]
   before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
-  before_action :find_current_user_notes, only: [:index]
 
   def index
     current_fetched_at = Time.now.to_i
 
     notes_json = { notes: [], last_fetched_at: current_fetched_at }
 
+    @notes = notes_finder.execute.inc_author
+
     @notes.each do |note|
       next if note.cross_reference_not_visible_for?(current_user)
 
@@ -23,7 +24,11 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def create
-    create_params = note_params.merge(merge_request_diff_head_sha: params[:merge_request_diff_head_sha])
+    create_params = note_params.merge(
+      merge_request_diff_head_sha: params[:merge_request_diff_head_sha],
+      in_reply_to_discussion_id: params[:in_reply_to_discussion_id],
+      new_discussion: params[:new_discussion],
+    )
     @note = Notes::CreateService.new(project, current_user, create_params).execute
 
     if @note.is_a?(Note)
@@ -111,6 +116,17 @@ class Projects::NotesController < Projects::ApplicationController
     )
   end
 
+  def discussion_html(discussion)
+    return if discussion.render_as_individual_notes?
+
+    render_to_string(
+      "discussions/_discussion",
+      layout: false,
+      formats: [:html],
+      locals: { discussion: discussion }
+    )
+  end
+
   def diff_discussion_html(discussion)
     return unless discussion.diff_discussion?
 
@@ -135,17 +151,6 @@ class Projects::NotesController < Projects::ApplicationController
     )
   end
 
-  def discussion_html(discussion)
-    return unless discussion.diff_discussion?
-
-    render_to_string(
-      "discussions/_discussion",
-      layout: false,
-      formats: [:html],
-      locals: { discussion: discussion }
-    )
-  end
-
   def note_json(note)
     attrs = {
       id: note.id
@@ -156,33 +161,22 @@ class Projects::NotesController < Projects::ApplicationController
 
       attrs.merge!(
         valid: true,
-        discussion_id: note.discussion_id,
+        discussion_id: note.discussion_id(noteable),
         html: note_html(note),
         note: note.note
       )
 
-      if note.diff_note?
-        discussion = note.to_discussion
-
+      discussion = note.to_discussion(noteable)
+      unless discussion.render_as_individual_notes?
         attrs.merge!(
           diff_discussion_html: diff_discussion_html(discussion),
-          discussion_html: discussion_html(discussion)
-        )
+          discussion_html: discussion_html(discussion),
 
-        # The discussion_id is used to add the comment to the correct discussion
-        # element on the merge request page. Among other things, the discussion_id
-        # contains the sha of head commit of the merge request.
-        # When new commits are pushed into the merge request after the initial
-        # load of the merge request page, the discussion elements will still have
-        # the old discussion_ids, with the old head commit sha. The new comment,
-        # however, will have the new discussion_id with the new commit sha.
-        # To ensure that these new comments will still end up in the correct
-        # discussion element, we also send the original discussion_id, with the
-        # old commit sha, along, and fall back on this value when no discussion
-        # element with the new discussion_id could be found.
-        if note.new_diff_note? && note.position != note.original_position
-          attrs[:original_discussion_id] = note.original_discussion_id
-        end
+          # Since the `discussion_id` can change, for example when new commits are pushed into an MR,
+          # the never-changing `original_discussion_id` is used as a fallback to the find the relevant
+          # discussion container to add this note to.
+          original_discussion_id: note.original_discussion_id
+        )
       end
     else
       attrs.merge!(
@@ -205,14 +199,30 @@ class Projects::NotesController < Projects::ApplicationController
 
   def note_params
     params.require(:note).permit(
-      :note, :noteable, :noteable_id, :noteable_type, :project_id,
-      :attachment, :line_code, :commit_id, :type, :position
+      :project_id,
+      :noteable_type,
+      :noteable_id,
+      :commit_id,
+      :noteable,
+      :type,
+
+      :note,
+      :attachment,
+
+      # LegacyDiffNote
+      :line_code,
+
+      # DiffNote
+      :position
     )
   end
 
-  def find_current_user_notes
-    @notes = NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at))
-      .execute.inc_author
+  def notes_finder
+    @notes_finder ||= NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at))
+  end
+
+  def noteable
+    @noteable ||= notes_finder.target
   end
 
   def last_fetched_at
diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index ea1a97b7cf0..2600a9d7256 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -1,4 +1,5 @@
 class Projects::SnippetsController < Projects::ApplicationController
+  include NotesHelper
   include ToggleAwardEmoji
   include SpammableActions
   include SnippetsActions
@@ -55,8 +56,10 @@ class Projects::SnippetsController < Projects::ApplicationController
 
   def show
     @note = @project.notes.new(noteable: @snippet)
-    @notes = Banzai::NoteRenderer.render(@snippet.notes.fresh, @project, current_user)
     @noteable = @snippet
+
+    @discussions = @snippet.discussions
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
   end
 
   def destroy