Add Discussion model to represent MR/diff discussion

Author: DouweM

app/assets/javascripts/notes.js.coffee

@@ -162,7 +162,7 @@ class @Notes
         @last_fetched_at = data.last_fetched_at
         @setPollingInterval(data.notes.length)
         $.each notes, (i, note) =>
-          if note.discussion_with_diff_html?
+          if note.discussion_html?
             @renderDiscussionNote(note)
           else
             @renderNote(note)
@@ -251,7 +251,7 @@ class @Notes
       discussionContainer = $(".notes[data-discussion-id='" + note.original_discussion_id + "']")
     if discussionContainer.length is 0
       # insert the note and the reply button after the temp row
-      row.after note.discussion_html
+      row.after note.diff_discussion_html
 
       # remove the note (will be added again below)
       row.next().find(".note").remove()
@@ -265,7 +265,7 @@ class @Notes
       # Init discussion on 'Discussion' page if it is merge request page
       if $('body').attr('data-page').indexOf('projects:merge_request') is 0
         $('ul.main-notes-list')
-          .append(note.discussion_with_diff_html)
+          .append(note.discussion_html)
           .syntaxHighlight()
     else
       # append new note to all matching discussions

app/controllers/projects/commit_controller.rb

@@ -115,11 +115,11 @@ def define_commit_vars
   end
 
   def define_note_vars
-    @grouped_diff_notes = commit.notes.grouped_diff_notes
+    @grouped_diff_discussions = commit.notes.grouped_diff_discussions
     @notes = commit.notes.non_diff_notes.fresh
 
     Banzai::NoteRenderer.render(
-      @grouped_diff_notes.values.flatten + @notes,
+      @grouped_diff_discussions.values.flat_map(&:notes) + @notes,
       @project,
       current_user,
     )

app/controllers/projects/compare_controller.rb

@@ -54,7 +54,7 @@ def define_diff_vars
       )
 
       @diff_notes_disabled = true
-      @grouped_diff_notes = {}
+      @grouped_diff_discussions = {}
     end
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -97,7 +97,7 @@ def diff_for_path
     else
       build_merge_request
       @diff_notes_disabled = true
-      @grouped_diff_notes = {}
+      @grouped_diff_discussions = {}
     end
 
     define_commit_vars
@@ -378,7 +378,7 @@ def define_discussion_vars
 
     # This is not executed lazily
     @notes = Banzai::NoteRenderer.render(
-      @discussions.flatten,
+      @discussions.flat_map(&:notes),
       @project,
       current_user,
       @path,
@@ -404,10 +404,10 @@ def define_diff_comment_vars
     }
 
     @use_legacy_diff_notes = !@merge_request.support_new_diff_notes?
-    @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
+    @grouped_diff_discussions = @merge_request.notes.grouped_diff_discussions
 
     Banzai::NoteRenderer.render(
-      @grouped_diff_notes.values.flatten,
+      @grouped_diff_discussions.values.flat_map(&:notes),
       @project,
       current_user,
       @path,

app/controllers/projects/notes_controller.rb

@@ -73,7 +73,7 @@ def note
   end
   alias_method :awardable, :note
 
-  def note_to_html(note)
+  def note_html(note)
     render_to_string(
       "projects/notes/_note",
       layout: false,
@@ -82,20 +82,20 @@ def note_to_html(note)
     )
   end
 
-  def note_to_discussion_html(note)
-    return unless note.diff_note?
+  def diff_discussion_html(discussion)
+    return unless discussion.diff_discussion?
 
     if params[:view] == 'parallel'
-      template = "projects/notes/_diff_notes_with_reply_parallel"
+      template = "discussions/_parallel_diff_discussion"
       locals =
         if params[:line_type] == 'old'
-          { notes_left: [note], notes_right: [] }
+          { discussion_left: discussion, discussion_right: nil }
         else
-          { notes_left: [], notes_right: [note] }
+          { discussion_left: nil, discussion_right: discussion }
         end
     else
-      template = "projects/notes/_diff_notes_with_reply"
-      locals = { notes: [note] }
+      template = "discussions/_diff_discussion"
+      locals = { discussion: discussion }
     end
 
     render_to_string(
@@ -106,14 +106,14 @@ def note_to_discussion_html(note)
     )
   end
 
-  def note_to_discussion_with_diff_html(note)
-    return unless note.diff_note?
+  def discussion_html(discussion)
+    return unless discussion.diff_discussion?
 
     render_to_string(
-      "projects/notes/_discussion",
+      "discussions/_discussion",
       layout: false,
       formats: [:html],
-      locals: { discussion_notes: [note] }
+      locals: { discussion: discussion }
     )
   end
 
@@ -132,26 +132,33 @@ def note_json(note)
         valid: true,
         id: note.id,
         discussion_id: note.discussion_id,
-        html: note_to_html(note),
+        html: note_html(note),
         award: false,
-        note: note.note,
-        discussion_html: note_to_discussion_html(note),
-        discussion_with_diff_html: note_to_discussion_with_diff_html(note)
+        note: note.note
       }
 
-      # 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
+      if note.diff_note?
+        discussion = Discussion.new([note])
+
+        attrs.merge!(
+          diff_discussion_html: diff_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
       end
 
       attrs

app/helpers/diff_helper.rb

@@ -54,18 +54,20 @@ def diff_line_content(line, line_type = nil)
     end
   end
 
-  def organize_comments(left, right)
-    notes_left = notes_right = nil
+  def parallel_diff_discussions(left, right, diff_file)
+    discussion_left = discussion_right = nil
 
-    unless left[:type].nil? && right[:type] == 'new'
-      notes_left = @grouped_diff_notes[left[:line_code]]
+    if left && (left.unchanged? || left.removed?)
+      line_code = diff_file.line_code(left)
+      discussion_left = @grouped_diff_discussions[line_code]
     end
 
-    unless left[:type].nil? && right[:type].nil?
-      notes_right = @grouped_diff_notes[right[:line_code]]
+    if right && right.added?
+      line_code = diff_file.line_code(right)
+      discussion_right = @grouped_diff_discussions[line_code]
     end
 
-    [notes_left, notes_right]
+    [discussion_left, discussion_right]
   end
 
   def inline_diff_btn

app/helpers/notes_helper.rb

@@ -1,9 +1,4 @@
 module NotesHelper
-  # Helps to distinguish e.g. commit notes in mr notes list
-  def note_for_main_target?(note)
-    @noteable.class.name == note.noteable_type && !note.diff_note?
-  end
-
   def note_target_fields(note)
     if note.noteable
       hidden_field_tag(:target_type, note.noteable.class.name.underscore) +
@@ -44,8 +39,8 @@ def diff_view_line_data(line_code, position, line_type)
     # If we didn't, diff notes that would show for the same line on the changes
     # tab, would show in different discussions on the discussion tab.
     use_legacy_diff_note ||= begin
-      line_diff_notes = @grouped_diff_notes[line_code]
-      line_diff_notes && line_diff_notes.any?(&:legacy_diff_note?)
+      discussion = @grouped_diff_discussions[line_code]
+      discussion && discussion.legacy_diff_discussion?
     end
 
     data = {
@@ -81,22 +76,10 @@ def diff_view_line_data(line_code, position, line_type)
     data
   end
 
-  def link_to_reply_discussion(note, line_type = nil)
+  def link_to_reply_discussion(discussion, line_type = nil)
     return unless current_user
 
-    data = {
-      noteable_type: note.noteable_type,
-      noteable_id:   note.noteable_id,
-      commit_id:     note.commit_id,
-      discussion_id: note.discussion_id,
-      line_type:     line_type
-    }
-
-    if note.diff_note?
-      data[:note_type] = note.type
-
-      data.merge!(note.diff_attributes)
-    end
+    data = discussion.reply_attributes.merge(line_type: line_type)
 
     content_tag(:div, class: "discussion-reply-holder") do
       button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
@@ -114,13 +97,13 @@ def note_max_access_for_user(note)
     @max_access_by_user_id[full_key]
   end
 
-  def diff_note_path(note)
-    return unless note.diff_note?
+  def discussion_diff_path(discussion)
+    return unless discussion.diff_discussion?
 
-    if note.for_merge_request? && note.active?
-      diffs_namespace_project_merge_request_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code)
-    elsif note.for_commit?
-      namespace_project_commit_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code)
+    if discussion.for_merge_request? && discussion.active?
+      diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code)
+    elsif discussion.for_commit?
+      namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code)
     end
   end
 end

app/models/concerns/note_on_diff.rb

@@ -1,12 +1,6 @@
 module NoteOnDiff
   extend ActiveSupport::Concern
 
-  NUMBER_OF_TRUNCATED_DIFF_LINES = 16
-
-  included do
-    delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true
-  end
-
   def diff_note?
     true
   end
@@ -30,23 +24,4 @@ def diff_attributes
   def can_be_award_emoji?
     false
   end
-
-  # Returns an array of at most 16 highlighted lines above a diff note
-  def truncated_diff_lines
-    prev_lines = []
-
-    highlighted_diff_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