Extract LegacyDiffNote out of Note

app/assets/javascripts/notes.js.coffee

@@ -285,6 +285,7 @@ class @Notes
     form.addClass "js-main-target-form"
 
     form.find("#note_line_code").remove()
+    form.find("#note_type").remove()
 
   ###
   General note form setup.
@@ -472,6 +473,7 @@ class @Notes
   setupDiscussionNoteForm: (dataHolder, form) =>
     # setup note target
     form.attr 'id', "new-discussion-note-form-#{dataHolder.data("discussionId")}"
+    form.find("#note_type").val dataHolder.data("noteType")
     form.find("#line_type").val dataHolder.data("lineType")
     form.find("#note_commit_id").val dataHolder.data("commitId")
     form.find("#note_line_code").val dataHolder.data("lineCode")

app/assets/stylesheets/pages/notes.scss

@@ -226,8 +226,7 @@ ul.notes {
   }
 }
 
-.note-action-button,
-.discussion-action-button {
+.note-action-button {
   display: inline-block;
   margin-left: 10px;
   line-height: 24px;

app/controllers/projects/commit_controller.rb

@@ -17,11 +17,12 @@ class Projects::CommitController < Projects::ApplicationController
   def show
     apply_diff_view_cookie!
 
-    @line_notes = commit.notes.inline
+    @grouped_diff_notes = commit.notes.grouped_diff_notes
+
     @note = @project.build_commit_note(commit)
-    @notes = commit.notes.not_inline.fresh
+    @notes = commit.notes.non_diff_notes.fresh
     @noteable = @commit
-    @comments_allowed = @reply_allowed = true
+    @comments_allowed = true
     @comments_target  = {
       noteable_type: 'Commit',
       commit_id: @commit.id
@@ -67,10 +68,10 @@ class Projects::CommitController < Projects::ApplicationController
     create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title} has been successfully reverted.",
                                           success_path: successful_change_path, failure_path: failed_change_path)
   end
-  
+
   def cherry_pick
     assign_change_commit_vars(@commit.cherry_pick_branch_name)
-    
+
     return render_404 if @target_branch.blank?
 
     create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title} has been successfully cherry-picked.",

app/controllers/projects/compare_controller.rb

@@ -22,7 +22,7 @@ class Projects::CompareController < Projects::ApplicationController
       @base_commit = @project.merge_base_commit(@base_ref, @head_ref)
       @diffs = compare.diffs(diff_options)
       @diff_refs = [@base_commit, @commit]
-      @line_notes = []
+      @grouped_diff_notes = {}
     end
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -73,12 +73,13 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     # but we need it for the "View file @ ..." link by deleted files
     @base_commit ||= @merge_request.first_commit.parent || @merge_request.first_commit
 
-    @comments_allowed = @reply_allowed = true
+    @comments_allowed = true
     @comments_target = {
       noteable_type: 'MergeRequest',
       noteable_id: @merge_request.id
     }
-    @line_notes = @merge_request.notes.where("line_code is not null")
+
+    @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
 
     respond_to do |format|
       format.html
@@ -300,7 +301,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     # Build a note object for comment form
     @note = @project.notes.new(noteable: @merge_request)
     @notes = @merge_request.mr_and_commit_notes.nonawards.inc_author.fresh
-    @discussions = Note.discussions_from_notes(@notes)
+    @discussions = @notes.discussions
     @noteable = @merge_request
 
     # Get commits from repository

app/controllers/projects/notes_controller.rb

@@ -96,7 +96,7 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def note_to_discussion_html(note)
-    return unless note.for_diff_line?
+    return unless note.diff_note?
 
     if params[:view] == 'parallel'
       template = "projects/notes/_diff_notes_with_reply_parallel"
@@ -120,7 +120,7 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def note_to_discussion_with_diff_html(note)
-    return unless note.for_diff_line?
+    return unless note.diff_note?
 
     render_to_string(
       "projects/notes/_discussion",
@@ -158,7 +158,7 @@ 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
+      :attachment, :line_code, :commit_id, :type
     )
   end
 

app/finders/notes_finder.rb

@@ -10,7 +10,7 @@ class NotesFinder
     notes =
       case target_type
       when "commit"
-        project.notes.for_commit_id(target_id).not_inline
+        project.notes.for_commit_id(target_id).non_diff_notes
       when "issue"
         project.issues.find(target_id).notes.nonawards.inc_author
       when "merge_request"

app/helpers/diff_helper.rb

@@ -55,22 +55,18 @@ module DiffHelper
     end
   end
 
-  def line_comments
-    @line_comments ||= @line_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code)
-  end
-
-  def organize_comments(type_left, type_right, line_code_left, line_code_right)
-    comments_left = comments_right = nil
+  def organize_comments(left, right)
+    notes_left = notes_right = nil
 
-    unless type_left.nil? && type_right == 'new'
-      comments_left = line_comments[line_code_left]
+    unless left[:type].nil? && right[:type] == 'new'
+      notes_left = @grouped_diff_notes[left[:line_code]]
     end
 
-    unless type_left.nil? && type_right.nil?
-      comments_right = line_comments[line_code_right]
+    unless left[:type].nil? && right[:type].nil?
+      notes_right = @grouped_diff_notes[right[:line_code]]
     end
 
-    [comments_left, comments_right]
+    [notes_left, notes_right]
   end
 
   def inline_diff_btn
@@ -96,8 +92,8 @@ module DiffHelper
     ].join(' ').html_safe
   end
 
-  def commit_for_diff(diff)
-    if diff.deleted_file
+  def commit_for_diff(diff_file)
+    if diff_file.deleted_file
       @base_commit || @commit.parent || @commit
     else
       @commit

app/helpers/notes_helper.rb

 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.for_diff_line?)
+    @noteable.class.name == note.noteable_type && !note.diff_note?
   end
 
   def note_target_fields(note)
@@ -15,16 +15,6 @@ module NotesHelper
     note.editable? && can?(current_user, :admin_note, note)
   end
 
-  def link_to_commit_diff_line_note(note)
-    if note.for_commit_diff_line?
-      link_to(
-        "#{note.diff_file_name}:L#{note.diff_new_line}",
-        namespace_project_commit_path(@project.namespace, @project,
-                                      note.noteable, anchor: note.line_code)
-      )
-    end
-  end
-
   def noteable_json(noteable)
     {
       id: noteable.id,
@@ -35,7 +25,7 @@ module NotesHelper
   end
 
   def link_to_new_diff_note(line_code, line_type = nil)
-    discussion_id = Note.build_discussion_id(
+    discussion_id = LegacyDiffNote.build_discussion_id(
       @comments_target[:noteable_type],
       @comments_target[:noteable_id] || @comments_target[:commit_id],
       line_code
@@ -45,9 +35,10 @@ module NotesHelper
       noteable_type: @comments_target[:noteable_type],
       noteable_id:   @comments_target[:noteable_id],
       commit_id:     @comments_target[:commit_id],
+      line_type:     line_type,
       line_code:     line_code,
-      discussion_id: discussion_id,
-      line_type:     line_type
+      note_type:     LegacyDiffNote.name,
+      discussion_id: discussion_id
     }
 
     button_tag(class: 'btn add-diff-note js-add-diff-note-button',
@@ -57,18 +48,24 @@ module NotesHelper
     end
   end
 
-  def link_to_reply_diff(note, line_type = nil)
+  def link_to_reply_discussion(note, line_type = nil)
     return unless current_user
 
     data = {
       noteable_type: note.noteable_type,
       noteable_id:   note.noteable_id,
       commit_id:     note.commit_id,
-      line_code:     note.line_code,
       discussion_id: note.discussion_id,
       line_type:     line_type
     }
 
+    if note.diff_note?
+      data.merge!(
+        line_code: note.line_code,
+        note_type: LegacyDiffNote.name
+      )
+    end
+
     button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
                            data: data, title: 'Add a reply'
   end

app/models/legacy_diff_note.rb

+class LegacyDiffNote < Note
+  serialize :st_diff
+
+  validates :line_code, presence: true, line_code: true
+
+  before_create :set_diff
+
+  class << self
+    def build_discussion_id(noteable_type, noteable_id, line_code, active = true)
+      [super(noteable_type, noteable_id), line_code, active].join("-")
+    end
+  end
+
+  def diff_note?
+    true
+  end
+
+  def legacy_diff_note?
+    true
+  end
+
+  def discussion_id
+    @discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code, active?)
+  end
+
+  def find_diff
+    return nil unless noteable
+    return @diff if defined?(@diff)
+
+    # Don't use ||= because nil is a valid value for @diff
+    @diff = noteable.diffs(Commit.max_diff_options).find do |d|
+      Digest::SHA1.hexdigest(d.new_path) == diff_file_index if d.new_path
+    end
+  end
+
+  def set_diff
+    # First lets find notes with same diff
+    # before iterating over all mr diffs
+    diff = diff_for_line_code unless for_merge_request?
+    diff ||= find_diff
+
+    self.st_diff = diff.to_hash if diff
+  end
+
+  def diff
+    @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
+  end
+
+  def diff_for_line_code
+    attributes = {
+      noteable_type: noteable_type,
+      line_code: line_code
+    }
+
+    if for_commit?
+      attributes[:commit_id] = commit_id
+    else
+      attributes[:noteable_id] = noteable_id
+    end
+
+    self.class.where(attributes).last.try(:diff)
+  end
+
+  # Check if this note is part of an "active" discussion
+  #
+  # This will always return true for anything except MergeRequest noteables,
+  # which have special logic.
+  #
+  # If the note's current diff cannot be matched in the MergeRequest's current
+  # diff, it's considered inactive.
+  def active?
+    return true if for_commit?
+    return true unless self.diff
+    return false unless noteable
+    return @active if defined?(@active)
+
+    noteable_diff = find_noteable_diff
+
+    if noteable_diff
+      parsed_lines = Gitlab::Diff::Parser.new.parse(noteable_diff.diff.each_line)
+
+      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line }
+    else
+      @active = false
+    end
+
+    @active
+  end
+
+  def diff_file_index
+    line_code.split('_')[0] if line_code
+  end
+
+  def diff_file_name
+    diff.new_path if diff
+  end
+
+  def file_path
+    if diff.new_path.present?
+      diff.new_path
+    elsif diff.old_path.present?
+      diff.old_path
+    end
+  end
+
+  def diff_old_line
+    line_code.split('_')[1].to_i if line_code
+  end
+
+  def diff_new_line
+    line_code.split('_')[2].to_i if line_code
+  end
+
+  def generate_line_code(line)
+    Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
+  end
+
+  def diff_line
+    return @diff_line if @diff_line
+
+    if diff
+      diff_lines.each do |line|
+        if generate_line_code(line) == self.line_code
+          @diff_line = line.text
+        end
+      end
+    end
+
+    @diff_line
+  end
+
+  def diff_line_type
+    return @diff_line_type if @diff_line_type
+
+    if diff
+      diff_lines.each do |line|
+        if generate_line_code(line) == self.line_code
+          @diff_line_type = line.type
+        end
+      end
+    end
+
+    @diff_line_type
+  end
+
+  def truncated_diff_lines
+    max_number_of_lines = 16
+    prev_match_line = nil
+    prev_lines = []
+
+    highlighted_diff_lines.each do |line|
+      if line.type == "match"
+        prev_lines.clear
+        prev_match_line = line
+      else
+        prev_lines << line
+
+        break if generate_line_code(line) == self.line_code
+
+        prev_lines.shift if prev_lines.length >= max_number_of_lines
+      end
+    end
+
+    prev_lines
+  end
+
+  def diff_lines
+    @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
+  end
+
+  def highlighted_diff_lines
+    Gitlab::Diff::Highlight.new(diff_lines).highlight
+  end
+
+  private
+
+  # Find the diff on noteable that matches our own
+  def find_noteable_diff
+    diffs = noteable.diffs(Commit.max_diff_options)
+    diffs.find { |d| d.new_path == self.diff.new_path }
+  end
+end

app/models/note.rb

-require 'carrierwave/orm/activerecord'
-
 class Note < ActiveRecord::Base
+  extend ActiveModel::Naming
   include Gitlab::CurrentSettings
   include Participable
   include Mentionable
@@ -22,12 +21,10 @@ class Note < ActiveRecord::Base
   delegate :name, :email, to: :author, prefix: true
 
   before_validation :set_award!
-  before_validation :clear_blank_line_code!
 
   validates :note, :project, presence: true
   validates :note, uniqueness: { scope: [:author, :noteable_type, :noteable_id] }, if: ->(n) { n.is_award }
   validates :note, inclusion: { in: Emoji.emojis_names }, if: ->(n) { n.is_award }
-  validates :line_code, line_code: true, allow_blank: true
   # Attachments are deprecated and are handled by Markdown uploader
   validates :attachment, file_size: { maximum: :max_attachment_size }
 
@@ -41,8 +38,6 @@ class Note < ActiveRecord::Base
   scope :awards, ->{ where(is_award: true) }
   scope :nonawards, ->{ where(is_award: false) }
   scope :for_commit_id, ->(commit_id) { where(noteable_type: "Commit", commit_id: commit_id) }
-  scope :inline, ->{ where("line_code IS NOT NULL") }
-  scope :not_inline, ->{ where(line_code: nil) }
   scope :system, ->{ where(system: true) }
   scope :user, ->{ where(system: false) }
   scope :common, ->{ where(noteable_type: ["", nil]) }
@@ -50,38 +45,31 @@ class Note < ActiveRecord::Base
   scope :inc_author_project, ->{ includes(:project, :author) }
   scope :inc_author, ->{ includes(:author) }
 
+  scope :legacy_diff_notes, ->{ where(type: 'LegacyDiffNote') }
+  scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
+
   scope :with_associations, -> do
     includes(:author, :noteable, :updated_by,
              project: [:project_members, { group: [:group_members] }])
   end
 
-  serialize :st_diff
-  before_create :set_diff, if: ->(n) { n.line_code.present? }
+  before_validation :clear_blank_line_code!
 
   class << self
-    def discussions_from_notes(notes)
-      discussion_ids = []
-      discussions = []
-
-      notes.each do |note|
-        next if discussion_ids.include?(note.discussion_id)
-
-        # don't group notes for the main target
-        if !note.for_diff_line? && note.for_merge_request?
-          discussions << [note]
-        else
-          discussions << notes.select do |other_note|
-            note.discussion_id == other_note.discussion_id
-          end
-          discussion_ids << note.discussion_id
-        end
-      end
+    def model_name
+      ActiveModel::Name.new(self, nil, 'note')
+    end
+
+    def build_discussion_id(noteable_type, noteable_id)
+      [:discussion, noteable_type.try(:underscore), noteable_id].join("-")
+    end
 
-      discussions
+    def discussions
+      all.group_by(&:discussion_id).values
     end
 
-    def build_discussion_id(type, id, line_code)
-      [:discussion, type.try(:underscore), id, line_code].join("-").to_sym
+    def grouped_diff_notes
+      legacy_diff_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code)
     end
 
     # Searches for notes matching the given query.
@@ -116,167 +104,35 @@ class Note < ActiveRecord::Base
     system && SystemNoteService.cross_reference?(note)
   end
 
-  def max_attachment_size
-    current_application_settings.max_attachment_size.megabytes.to_i
-  end
-
-  def find_diff
-    return nil unless noteable
-    return @diff if defined?(@diff)
-
-    # Don't use ||= because nil is a valid value for @diff
-    @diff = noteable.diffs(Commit.max_diff_options).find do |d|
-      Digest::SHA1.hexdigest(d.new_path) == diff_file_index if d.new_path
-    end
-  end
-
-  def hook_attrs
-    attributes
-  end
-
-  def set_diff
-    # First lets find notes with same diff
-    # before iterating over all mr diffs
-    diff = diff_for_line_code unless for_merge_request?
-    diff ||= find_diff
-
-    self.st_diff = diff.to_hash if diff
-  end
-
-  def diff
-    @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
-  end
-
-  def diff_for_line_code
-    Note.where(noteable_id: noteable_id, noteable_type: noteable_type, line_code: line_code).last.try(:diff)
-  end
-
-  # Check if this note is part of an "active" discussion
-  #
-  # This will always return true for anything except MergeRequest noteables,
-  # which have special logic.
-  #
-  # If the note's current diff cannot be matched in the MergeRequest's current
-  # diff, it's considered inactive.
-  def active?
-    return true unless self.diff
-    return false unless noteable
-    return @active if defined?(@active)
-
-    noteable_diff = find_noteable_diff
-
-    if noteable_diff
-      parsed_lines = Gitlab::Diff::Parser.new.parse(noteable_diff.diff.each_line)
-
-      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line }
-    else
-      @active = false
-    end
-
-    @active
+  def diff_note?
+    false
   end
 
-  def diff_file_index
-    line_code.split('_')[0] if line_code
-  end
-
-  def diff_file_name
-    diff.new_path if diff
-  end
-
-  def file_path
-    if diff.new_path.present?
-      diff.new_path
-    elsif diff.old_path.present?
-      diff.old_path
-    end
+  def legacy_diff_note?
+    false
   end
 
-  def diff_old_line
-    line_code.split('_')[1].to_i if line_code
-  end
-
-  def diff_new_line
-    line_code.split('_')[2].to_i if line_code
-  end
-
-  def generate_line_code(line)
-    Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
-  end
-
-  def diff_line
-    return @diff_line if @diff_line
-
-    if diff
-      diff_lines.each do |line|
-        if generate_line_code(line) == self.line_code
-          @diff_line = line.text
-        end
-      end
-    end
-
-    @diff_line
-  end
-
-  def diff_line_type
-    return @diff_line_type if @diff_line_type
-
-    if diff
-      diff_lines.each do |line|
-        if generate_line_code(line) == self.line_code
-          @diff_line_type = line.type
-        end
-      end
-    end
-
-    @diff_line_type
-  end
-
-  def truncated_diff_lines
-    max_number_of_lines = 16
-    prev_match_line = nil
-    prev_lines = []
-
-    highlighted_diff_lines.each do |line|
-      if line.type == "match"
-        prev_lines.clear
-        prev_match_line = line
+  def discussion_id
+    @discussion_id ||=
+      if for_merge_request?
+        [:discussion, :note, id].join("-")
       else
-        prev_lines << line
-
-        break if generate_line_code(line) == self.line_code
-
-        prev_lines.shift if prev_lines.length >= max_number_of_lines
+        self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
       end
-    end
-
-    prev_lines
-  end
-
-  def diff_lines
-    @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
   end
 
-  def highlighted_diff_lines
-    Gitlab::Diff::Highlight.new(diff_lines).highlight
+  def max_attachment_size
+    current_application_settings.max_attachment_size.megabytes.to_i
   end
 
-  def discussion_id
-    @discussion_id ||= Note.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
+  def hook_attrs
+    attributes
   end
 
   def for_commit?
     noteable_type == "Commit"
   end
 
-  def for_commit_diff_line?
-    for_commit? && for_diff_line?
-  end
-
-  def for_diff_line?
-    line_code.present?
-  end
-
   def for_issue?
     noteable_type == "Issue"
   end
@@ -285,10 +141,6 @@ class Note < ActiveRecord::Base
     noteable_type == "MergeRequest"
   end
 
-  def for_merge_request_diff_line?
-    for_merge_request? && for_diff_line?
-  end
-
   def for_snippet?
     noteable_type == "Snippet"
   end
@@ -361,14 +213,8 @@ class Note < ActiveRecord::Base
     self.line_code = nil if self.line_code.blank?
   end
 
-  # Find the diff on noteable that matches our own
-  def find_noteable_diff
-    diffs = noteable.diffs(Commit.max_diff_options)
-    diffs.find { |d| d.new_path == self.diff.new_path }
-  end
-
   def awards_supported?
-    (for_issue? || for_merge_request?) && !for_diff_line?
+    (for_issue? || for_merge_request?) && !diff_note?
   end