Commit c3acc822 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'refactor-diff-notes' into 'master'

Refactor diff notes

Related to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4101

/cc @rspeicher 

See merge request !4110
parents 61756ecc 876dc7ba
......@@ -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")
......
......@@ -226,8 +226,7 @@ ul.notes {
}
}
.note-action-button,
.discussion-action-button {
.note-action-button {
display: inline-block;
margin-left: 10px;
line-height: 24px;
......
......@@ -17,11 +17,11 @@ 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_target = {
noteable_type: 'Commit',
commit_id: @commit.id
......
......@@ -22,7 +22,8 @@ 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 = []
@diff_notes_disabled = true
@grouped_diff_notes = {}
end
end
......
......@@ -73,12 +73,12 @@ 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_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
......@@ -117,6 +117,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@commit = @merge_request.last_commit
@base_commit = @merge_request.diff_base_commit
@diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare
@diff_notes_disabled = true
@ci_commit = @merge_request.ci_commit
@statuses = @ci_commit.statuses if @ci_commit
......@@ -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
......
......@@ -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
......
......@@ -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"
......
......@@ -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
......
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
......
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 diff_file_hash
line_code.split('_')[0] if line_code
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 diff
@diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
end
def diff_file_path
diff.new_path.presence || diff.old_path
end
def diff_lines
@diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
end
def diff_line
@diff_line ||= diff_lines.find { |line| generate_line_code(line) == self.line_code }
end
def diff_line_text
diff_line.try(:text)
end
def diff_line_type
diff_line.try(:type)
end
def highlighted_diff_lines
Gitlab::Diff::Highlight.new(diff_lines).highlight
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
# 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 @active if defined?(@active)
return true if for_commit?
return true unless self.diff
return false unless noteable
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_text }
else
@active = false
end
@active
end
private
def find_diff
return nil unless noteable
return @diff if defined?(@diff)
@diff = noteable.diffs(Commit.max_diff_options).find do |d|
d.new_path && Digest::SHA1.hexdigest(d.new_path) == diff_file_hash
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_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
def generate_line_code(line)
Gitlab::Diff::LineCode.generate(diff_file_path, line.new_pos, line.old_pos)
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
end
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
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,39 @@ 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
def diff_note?
false
end
def diff
@diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
def legacy_diff_note?
false
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
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
true
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
end
end
prev_lines
self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
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 +145,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 +217,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
def contains_emoji_only?
......
- if @note.diff_file_name
- if @note.legacy_diff_note?
%p.details
New comment on diff for
= link_to @note.diff_file_name, @target_url
= link_to @note.diff_file_path, @target_url
\:
= render 'note_message'
......@@ -15,7 +15,7 @@
= link_text
- else
= link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text }
- if @comments_allowed && can?(current_user, :create_note, @project)
- if !@diff_notes_disabled && can?(current_user, :create_note, @project)
= link_to_new_diff_note(line_code)
%td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
- link_text = type == "old" ? "&nbsp;".html_safe : line.new_pos
......
......@@ -16,7 +16,7 @@
- else
%td.old_line.diff-line-num{id: left[:line_code], class: "#{left[:type]} #{'empty-cell' if !left[:number]}"}
= link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code]
- if @comments_allowed && can?(current_user, :create_note, @project)
- if !@diff_notes_disabled && can?(current_user, :create_note, @project)
= link_to_new_diff_note(left[:line_code], 'old')
%td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]} #{'empty-cell' if left[:text].empty?}", data: { line_code: left[:line_code] }}= diff_line_content(left[:text])
......@@ -29,14 +29,14 @@
%td.new_line.diff-line-num{id: new_line_code, class: "#{new_line_class} #{'empty-cell' if !right[:number]}", data: { linenumber: right[:number] }}
= link_to raw(right[:number]), "##{new_line_code}", id: new_line_code
- if @comments_allowed && can?(current_user, :create_note, @project)
= link_to_new_diff_note(right[:line_code], 'new')
- if !@diff_notes_disabled && can?(current_user, :create_note, @project)
= link_to_new_diff_note(new_line_code, 'new')
%td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code} #{'empty-cell' if right[:text].empty?}", data: { line_code: new_line_code }}= diff_line_content(right[:text])
- if @reply_allowed
- comments_left, comments_right = organize_comments(left[:type], right[:type], left[:line_code], right[:line_code])
- if comments_left.present? || comments_right.present?
= render "projects/notes/diff_notes_with_reply_parallel", notes_left: comments_left, notes_right: comments_right
- unless @diff_notes_disabled
- notes_left, notes_right = organize_comments(left, right)
- if notes_left.present? || notes_right.present?
= render "projects/notes/diff_notes_with_reply_parallel", notes_left: notes_left, notes_right: notes_right
- if diff_file.diff.diff.blank? && diff_file.mode_changed?
.file-mode-changed
......
......@@ -6,16 +6,15 @@
%table.text-file.code.js-syntax-highlight{ class: too_big ? 'hide' : '' }
- last_line = 0
- raw_diff_lines = diff_file.diff_lines.to_a
- diff_file.highlighted_diff_lines.each_with_index do |line, index|
- line_code = generate_line_code(diff_file.file_path, line)
- last_line = line.new_pos
= render "projects/diffs/line", {line: line, diff_file: diff_file, line_code: line_code}
- if @reply_allowed
- comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at)
- unless comments.empty?
= render "projects/notes/diff_notes_with_reply", notes: comments, line: raw_diff_lines[index].text
- unless @diff_notes_disabled
- diff_notes = @grouped_diff_notes[line_code]
- if diff_notes
= render "projects/notes/diff_notes_with_reply", notes: diff_notes
- if last_line > 0
= render "projects/diffs/match_line", { line: "",
......
- note = notes.first # example note
-# Check if line want not changed since comment was left
- if !defined?(line) || line == note.diff_line
%tr.notes_holder
- note = notes.first
%tr.notes_holder
%td.notes_line{ colspan: 2 }
%td.notes_content
%ul.notes{ data: { discussion_id: note.discussion_id } }
= render notes
= render partial: "projects/notes/note", collection: notes, as: :note
.discussion-reply-holder
= link_to_reply_diff(note)
= link_to_reply_discussion(note)
- note1 = notes_left.present? ? notes_left.first : nil
- note2 = notes_right.present? ? notes_right.first : nil
- note_left = notes_left.present? ? notes_left.first : nil
- note_right = notes_right.present? ? notes_right.first : nil
%tr.notes_holder
- if note1
- if note_left
%td.notes_line.old
%td.notes_content.parallel.old
%ul.notes{ data: { discussion_id: note1.discussion_id } }
= render notes_left
%ul.notes{ data: { discussion_id: note_left.discussion_id } }
= render partial: "projects/notes/note", collection: notes_left, as: :note
.discussion-reply-holder
= link_to_reply_diff(note1, 'old')
= link_to_reply_discussion(note_left, 'old')
- else
%td.notes_line.old= ""
%td.notes_content.parallel.old= ""
- if note2
- if note_right
%td.notes_line.new
%td.notes_content.parallel.new
%ul.notes{ data: { discussion_id: note2.discussion_id } }
= render notes_right
%ul.notes{ data: { discussion_id: note_right.discussion_id } }
= render partial: "projects/notes/note", collection: notes_right, as: :note
.discussion-reply-holder
= link_to_reply_diff(note2, 'new')
= link_to_reply_discussion(note_right, 'new')
- else
%td.notes_line.new= ""
%td.notes_content.parallel.new= ""
- note = discussion_notes.first
- expanded = !note.diff_note? || note.active?
%li.note.note-discussion.timeline-entry
.timeline-entry-inner
.timeline-icon
= link_to user_path(note.author) do
= image_tag avatar_icon(note.author_email), class: "avatar s40"
= image_tag avatar_icon(note.author), class: "avatar s40"
.timeline-content
- if note.for_merge_request?
- (active_notes, outdated_notes) = discussion_notes.partition(&:active?)
= render "projects/notes/discussions/active", discussion_notes: active_notes if active_notes.length > 0
= render "projects/notes/discussions/outdated", discussion_notes: outdated_notes if outdated_notes.length > 0
.discussion.js-toggle-container{ class: note.discussion_id }
.discussion-header
= link_to_member(@project, note.author, avatar: false)
.inline.discussion-headline-light
= note.author.to_reference
started a discussion on
- if note.for_commit?
- commit = note.noteable
- if commit
commit
= link_to commit.short_id, namespace_project_commit_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code), class: 'monospace'
- else
= render "projects/notes/discussions/commit", discussion_notes: discussion_notes
a deleted commit
- else
- if note.active?
= link_to diffs_namespace_project_merge_request_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code) do
the diff
- else
an outdated diff
= time_ago_with_tooltip(note.created_at, placement: "bottom", html_class: "note-created-ago")
.discussion-actions
= link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do
- if expanded
= icon("chevron-up")
- else
= icon("chevron-down")
Toggle discussion
.discussion-body.js-toggle-content{ class: ("hide" unless expanded) }
- if note.diff_note?
= render "projects/notes/discussions/diff_with_notes", discussion_notes: discussion_notes
- else
= render "projects/notes/discussions/notes", discussion_notes: discussion_notes
......@@ -6,6 +6,7 @@
= f.hidden_field :line_code
= f.hidden_field :noteable_id
= f.hidden_field :noteable_type
= f.hidden_field :type
= render layout: 'projects/md_preview', locals: { preview_class: "md-preview", referenced_users: true } do
= render 'projects/zen', f: f, attr: :note, classes: 'note-textarea js-note-text', placeholder: "Write a comment or drag your files here..."
......
- return unless note.author
- return if note.cross_reference_not_visible_for?(current_user)
- note_editable = note_editable?(note)
%li.timeline-entry{ id: dom_id(note), class: [dom_class(note), "note-row-#{note.id}", ('system-note' if note.system)], data: {author_id: note.author.id, editable: note_editable} }
%li.timeline-entry{ id: dom_id(note), class: ["note", "note-row-#{note.id}", ('system-note' if note.system)], data: {author_id: note.author.id, editable: note_editable} }
.timeline-entry-inner
.timeline-icon
%a{href: user_path(note.author)}
......@@ -8,8 +11,8 @@
.note-header
= link_to_member(note.project, note.author, avatar: false)
.inline.note-headline-light
= "#{note.author.to_reference}"
- if !note.system
= note.author.to_reference
- unless note.system
commented
%a{ href: "##{dom_id(note)}" }
= time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago')
......
......@@ -2,14 +2,9 @@
- @discussions.each do |discussion_notes|
- note = discussion_notes.first
- if note_for_main_target?(note)
- next if note.cross_reference_not_visible_for?(current_user)
= render discussion_notes
= render partial: "projects/notes/note", object: note, as: :note
- else
= render 'projects/notes/discussion', discussion_notes: discussion_notes
- else
- @notes.each do |note|
- next unless note.author
- next if note.cross_reference_not_visible_for?(current_user)
= render note
= render partial: "projects/notes/note", object: note, as: :note
- note = discussion_notes.first
.discussion.js-toggle-container{ class: note.discussion_id }
.discussion-header
= link_to_member(@project, note.author, avatar: false)
.inline.discussion-headline-light
= "#{note.author.to_reference} started a discussion"
= link_to diffs_namespace_project_merge_request_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code) do
on the diff
= time_ago_with_tooltip(note.created_at, placement: "bottom", html_class: "discussion_updated_ago")
.discussion-actions
= link_to "#", class: "discussion-action-button discussion-toggle-button js-toggle-button" do
%i.fa.fa-chevron-up
Show/hide discussion
.discussion-body.js-toggle-content
= render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
- note = discussion_notes.first
- commit = note.noteable
- commit_description = commit ? 'commit' : 'a deleted commit'
.discussion.js-toggle-container{ class: note.discussion_id }
.discussion-header
= link_to_member(@project, note.author, avatar: false)
.inline.discussion-headline-light
= "#{note.author.to_reference} started a discussion on #{commit_description}"
- if commit
= link_to(commit.short_id, namespace_project_commit_path(note.project.namespace, note.project, note.noteable), class: 'monospace')
= time_ago_with_tooltip(note.created_at, placement: "bottom", html_class: "discussion_updated_ago")
.discussion-actions
= link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do
%i.fa.fa-chevron-up
Show/hide discussion
.discussion-body.js-toggle-content
- if note.for_diff_line?
= render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
- else
.panel.panel-default
.notes{ data: { discussion_id: discussion_notes.first.discussion_id } }
%ul.notes.timeline
= render discussion_notes
.discussion-reply-holder
= link_to_reply_diff(discussion_notes.first)
- diff = note.diff
- if diff
.diff-file
.diff-header
%span
- if diff.deleted_file
= diff.old_path
- else
= diff.new_path
- if diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode
%span.file-mode= "#{diff.a_mode}#{diff.b_mode}"
.diff-content.code.js-syntax-highlight
%table
- note.truncated_diff_lines.each do |line|
- type = line.type
- line_code = generate_line_code(note.file_path, line)
%tr.line_holder{ id: line_code, class: "#{type}" }
- if type == "match"
%td.old_line.diff-line-num= "..."
%td.new_line.diff-line-num= "..."
%td.line_content.match= line.text
- else
%td.old_line.diff-line-num{ data: { linenumber: type == "new" ? "&nbsp;".html_safe : line.old_pos } }
%td.new_line.diff-line-num{ data: { linenumber: type == "old" ? "&nbsp;".html_safe : line.new_pos } }
%td.line_content{ class: ['noteable_line', type, line_code], line_code: line_code }= diff_line_content(line.text, type)
- if line_code == note.line_code
= render "projects/notes/diff_notes_with_reply", notes: discussion_notes
- note = discussion_notes.first
- diff = note.diff
- return unless diff
.diff-file
.diff-header
%span
- if diff.deleted_file
= diff.old_path
- else
= diff.new_path
- if diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode
%span.file-mode= "#{diff.a_mode}#{diff.b_mode}"
.diff-content.code.js-syntax-highlight
%table
- note.truncated_diff_lines.each do |line|
- type = line.type
- line_code = generate_line_code(note.diff_file_path, line)
%tr.line_holder{ id: line_code, class: "#{type}" }
- if type == "match"
%td.old_line.diff-line-num= "..."
%td.new_line.diff-line-num= "..."
%td.line_content.match= line.text
- else
%td.old_line.diff-line-num{ data: { linenumber: type == "new" ? "&nbsp;".html_safe : line.old_pos } }
%td.new_line.diff-line-num{ data: { linenumber: type == "old" ? "&nbsp;".html_safe : line.new_pos } }
%td.line_content{ class: ['noteable_line', type, line_code], line_code: line_code }= diff_line_content(line.text, type)
- if line_code == note.line_code
= render "projects/notes/diff_notes_with_reply", notes: discussion_notes
- note = discussion_notes.first
.panel.panel-default
.notes{ data: { discussion_id: note.discussion_id } }
%ul.notes.timeline
= render partial: "projects/notes/note", collection: discussion_notes, as: :note
.discussion-reply-holder
= link_to_reply_discussion(note)
- note = discussion_notes.first
.discussion.js-toggle-container{ class: note.discussion_id }
.discussion-header
= link_to_member(@project, note.author, avatar: false)
.inline.discussion-headline-light
= "#{note.author.to_reference} started a discussion"
on the outdated diff
= time_ago_with_tooltip(note.created_at, placement: "bottom", html_class: "discussion_updated_ago")
.discussion-actions
= link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do
%i.fa.fa-chevron-down
Show/hide discussion
.discussion-body.js-toggle-content.hide
= render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
class AddTypeToNotes < ActiveRecord::Migration
def change
add_column :notes, :type, :string
end
end
class SetTypeOnLegacyDiffNotes < ActiveRecord::Migration
def change
execute "UPDATE notes SET type = 'LegacyDiffNote' WHERE line_code IS NOT NULL"
end
end
......@@ -636,6 +636,7 @@ ActiveRecord::Schema.define(version: 20160509201028) do
t.text "st_diff"
t.integer "updated_by_id"
t.boolean "is_award", default: false, null: false
t.string "type"
end
add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree
......
......@@ -23,7 +23,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
click_diff_line(sample_commit.line_code)
page.within("form[id$='#{sample_commit.line_code}']") do
page.within("form[id$='#{sample_commit.line_code}-true']") do
fill_in "note[note]", with: "Typo, please fix"
find(".js-comment-button").trigger("click")
sleep 0.05
......@@ -33,7 +33,7 @@ module SharedDiffNote
step 'I leave a diff comment in a parallel view on the left side like "Old comment"' do
click_parallel_diff_line(sample_commit.line_code, 'old')
page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}']") do
page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}-true']") do
fill_in "note[note]", with: "Old comment"
find(".js-comment-button").trigger("click")
end
......@@ -41,7 +41,7 @@ module SharedDiffNote
step 'I leave a diff comment in a parallel view on the right side like "New comment"' do
click_parallel_diff_line(sample_commit.line_code, 'new')
page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}']") do
page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}-true']") do
fill_in "note[note]", with: "New comment"
find(".js-comment-button").trigger("click")
end
......@@ -51,7 +51,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
click_diff_line(sample_commit.line_code)
page.within("form[id$='#{sample_commit.line_code}']") do
page.within("form[id$='#{sample_commit.line_code}-true']") do
fill_in "note[note]", with: "Should fix it :smile:"
find('.js-md-preview-button').click
end
......@@ -62,7 +62,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
click_diff_line(sample_commit.del_line_code)
page.within("form[id$='#{sample_commit.del_line_code}']") do
page.within("form[id$='#{sample_commit.del_line_code}-true']") do
fill_in "note[note]", with: "DRY this up"
find('.js-md-preview-button').click
end
......@@ -91,7 +91,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
click_diff_line(sample_commit.line_code)
page.within("form[id$='#{sample_commit.line_code}']") do
page.within("form[id$='#{sample_commit.line_code}-true']") do
fill_in 'note[note]', with: ':smile:'
click_button('Comment')
end
......
......@@ -107,6 +107,8 @@ module API
break if opts[:line_code]
end
opts[:type] = LegacyDiffNote.name if opts[:line_code]
end
note = ::Notes::CreateService.new(user_project, current_user, opts).execute
......
......@@ -227,9 +227,9 @@ module API
class CommitNote < Grape::Entity
expose :note
expose(:path) { |note| note.diff_file_name }
expose(:line) { |note| note.diff_new_line }
expose(:line_type) { |note| note.diff_line_type }
expose(:path) { |note| note.diff_file_path if note.legacy_diff_note? }
expose(:line) { |note| note.diff_new_line if note.legacy_diff_note? }
expose(:line_type) { |note| note.diff_line_type if note.legacy_diff_note? }
expose :author, using: Entities::UserBasic
expose :created_at
end
......
......@@ -9,10 +9,10 @@ FactoryGirl.define do
author
factory :note_on_commit, traits: [:on_commit]
factory :note_on_commit_diff, traits: [:on_commit, :on_diff]
factory :note_on_commit_diff, traits: [:on_commit, :on_diff], class: LegacyDiffNote
factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
factory :note_on_merge_request, traits: [:on_merge_request]
factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff]
factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff], class: LegacyDiffNote
factory :note_on_project_snippet, traits: [:on_project_snippet]
factory :system_note, traits: [:system]
factory :downvote_note, traits: [:award, :downvote]
......
......@@ -192,7 +192,7 @@ describe 'Comments', feature: true do
end
it 'should be removed when canceled' do
page.within(".diff-file form[id$='#{line_code}']") do
page.within(".diff-file form[id$='#{line_code}-true']") do
find('.js-close-discussion-note-form').trigger('click')
end
......
require 'spec_helper'
describe LegacyDiffNote, models: true do
describe "Commit diff line notes" do
let!(:note) { create(:note_on_commit_diff, note: "+1 from me") }
let!(:commit) { note.noteable }
it "should save a valid note" do
expect(note.commit_id).to eq(commit.id)
expect(note.noteable.id).to eq(commit.id)
end
it "should be recognized by #legacy_diff_note?" do
expect(note).to be_legacy_diff_note
end
end
describe '#active?' do
it 'is always true when the note has no associated diff' do
note = build(:note_on_merge_request_diff)
expect(note).to receive(:diff).and_return(nil)
expect(note).to be_active
end
it 'is never true when the note has no noteable associated' do
note = build(:note_on_merge_request_diff)
expect(note).to receive(:diff).and_return(double)
expect(note).to receive(:noteable).and_return(nil)
expect(note).not_to be_active
end
it 'returns the memoized value if defined' do
note = build(:note_on_merge_request_diff)
note.instance_variable_set(:@active, 'foo')
expect(note).not_to receive(:find_noteable_diff)
expect(note.active?).to eq 'foo'
end
context 'for a merge request noteable' do
it 'is false when noteable has no matching diff' do
merge = build_stubbed(:merge_request, :simple)
note = build(:note_on_merge_request_diff, noteable: merge)
allow(note).to receive(:diff).and_return(double)
expect(note).to receive(:find_noteable_diff).and_return(nil)
expect(note).not_to be_active
end
it 'is true when noteable has a matching diff' do
merge = create(:merge_request, :simple)
# Generate a real line_code value so we know it will match. We use a
# random line from a random diff just for funsies.
diff = merge.diffs.to_a.sample
line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample
code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
# We're persisting in order to trigger the set_diff callback
note = create(:note_on_merge_request_diff, noteable: merge, line_code: code)
# Make sure we don't get a false positive from a guard clause
expect(note).to receive(:find_noteable_diff).and_call_original
expect(note).to be_active
end
end
end
end
......@@ -34,24 +34,6 @@ describe Note, models: true do
end
end
describe "Commit diff line notes" do
let!(:note) { create(:note_on_commit_diff, note: "+1 from me") }
let!(:commit) { note.noteable }
it "should save a valid note" do
expect(note.commit_id).to eq(commit.id)
expect(note.noteable.id).to eq(commit.id)
end
it "should be recognized by #for_diff_line?" do
expect(note).to be_for_diff_line
end
it "should be recognized by #for_commit_diff_line?" do
expect(note).to be_for_commit_diff_line
end
end
describe 'authorization' do
before do
@p1 = create(:project)
......@@ -148,66 +130,6 @@ describe Note, models: true do
end
end
describe '#active?' do
it 'is always true when the note has no associated diff' do
note = build(:note)
expect(note).to receive(:diff).and_return(nil)
expect(note).to be_active
end
it 'is never true when the note has no noteable associated' do
note = build(:note)
expect(note).to receive(:diff).and_return(double)
expect(note).to receive(:noteable).and_return(nil)
expect(note).not_to be_active
end
it 'returns the memoized value if defined' do
note = build(:note)
expect(note).to receive(:diff).and_return(double)
expect(note).to receive(:noteable).and_return(double)
note.instance_variable_set(:@active, 'foo')
expect(note).not_to receive(:find_noteable_diff)
expect(note.active?).to eq 'foo'
end
context 'for a merge request noteable' do
it 'is false when noteable has no matching diff' do
merge = build_stubbed(:merge_request, :simple)
note = build(:note, noteable: merge)
allow(note).to receive(:diff).and_return(double)
expect(note).to receive(:find_noteable_diff).and_return(nil)
expect(note).not_to be_active
end
it 'is true when noteable has a matching diff' do
merge = create(:merge_request, :simple)
# Generate a real line_code value so we know it will match. We use a
# random line from a random diff just for funsies.
diff = merge.diffs.to_a.sample
line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample
code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
# We're persisting in order to trigger the set_diff callback
note = create(:note, noteable: merge, line_code: code)
# Make sure we don't get a false positive from a guard clause
expect(note).to receive(:find_noteable_diff).and_call_original
expect(note).to be_active
end
end
end
describe "editable?" do
it "returns true" do
note = build(:note)
......@@ -258,7 +180,7 @@ describe Note, models: true do
end
it "is not an award emoji when comment is on a diff" do
note = create(:note, note: ":blowfish:", noteable: merge_request, line_code: "11d5d2e667e9da4f7f610f81d86c974b146b13bd_0_2")
note = create(:note_on_merge_request_diff, note: ":blowfish:", noteable: merge_request, line_code: "11d5d2e667e9da4f7f610f81d86c974b146b13bd_0_2")
note = note.reload
expect(note.note).to eq(":blowfish:")
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment