Commit fe26b8af authored by Douwe Maan's avatar Douwe Maan Committed by Luke "Jared" Bennett

Correctly display multiple separate discussions on the same diff line

parent bb8cc946
...@@ -668,7 +668,7 @@ require('./task_list'); ...@@ -668,7 +668,7 @@ require('./task_list');
return function(i, el) { return function(i, el) {
var note, notes; var note, notes;
note = $(el); note = $(el);
notes = note.closest(".notes"); notes = note.closest(".discussion-notes");
if (typeof gl.diffNotesCompileComponents !== 'undefined') { if (typeof gl.diffNotesCompileComponents !== 'undefined') {
if (gl.diffNoteApps[noteElId]) { if (gl.diffNoteApps[noteElId]) {
...@@ -685,14 +685,13 @@ require('./task_list'); ...@@ -685,14 +685,13 @@ require('./task_list');
// "Discussions" tab // "Discussions" tab
notes.closest(".timeline-entry").remove(); notes.closest(".timeline-entry").remove();
if (!_this.isParallelView() || notesTr.find('.note').length === 0) { // The notes tr can contain multiple lists of notes, like on the parallel diff
// "Changes" tab / commit view if (notesTr.find('.discussion-notes').length > 1) {
notesTr.remove(); notes.remove();
} else { } else {
notes.closest('.content').empty(); notesTr.remove();
} }
} }
return note.remove();
}; };
})(this)); })(this));
// Decrement the "Discussions" counter only once // Decrement the "Discussions" counter only once
......
...@@ -294,6 +294,18 @@ ul.notes { ...@@ -294,6 +294,18 @@ ul.notes {
border-width: 1px; border-width: 1px;
} }
.discussion-notes {
&:not(:first-child) {
border-top: 1px solid $white-normal;
margin-top: 20px;
}
&:not(:last-child) {
border-bottom: 1px solid $white-normal;
margin-bottom: 20px;
}
}
.notes { .notes {
background-color: $white-light; background-color: $white-light;
} }
......
...@@ -123,7 +123,7 @@ class Projects::CommitController < Projects::ApplicationController ...@@ -123,7 +123,7 @@ class Projects::CommitController < Projects::ApplicationController
@grouped_diff_discussions = commit.grouped_diff_discussions @grouped_diff_discussions = commit.grouped_diff_discussions
@discussions = commit.discussions @discussions = commit.discussions
@notes = (@grouped_diff_discussions.values + @discussions).flat_map(&:notes) @notes = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes)
@notes = prepare_notes_for_rendering(@notes) @notes = prepare_notes_for_rendering(@notes)
end end
......
...@@ -591,7 +591,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -591,7 +591,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@use_legacy_diff_notes = !@merge_request.has_complete_diff_refs? @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
@grouped_diff_discussions = @merge_request.grouped_diff_discussions @grouped_diff_discussions = @merge_request.grouped_diff_discussions
@notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flat_map(&:notes)) @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
end end
def define_pipelines_vars def define_pipelines_vars
......
...@@ -136,13 +136,13 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -136,13 +136,13 @@ class Projects::NotesController < Projects::ApplicationController
template = "discussions/_parallel_diff_discussion" template = "discussions/_parallel_diff_discussion"
locals = locals =
if params[:line_type] == 'old' if params[:line_type] == 'old'
{ discussion_left: discussion, discussion_right: nil } { discussions_left: [discussion], discussions_right: nil }
else else
{ discussion_left: nil, discussion_right: discussion } { discussions_left: nil, discussions_right: [discussion] }
end end
else else
template = "discussions/_diff_discussion" template = "discussions/_diff_discussion"
locals = { discussion: discussion } locals = { discussions: [discussion] }
end end
render_to_string( render_to_string(
......
...@@ -62,19 +62,19 @@ module DiffHelper ...@@ -62,19 +62,19 @@ module DiffHelper
end end
def parallel_diff_discussions(left, right, diff_file) def parallel_diff_discussions(left, right, diff_file)
discussion_left = discussion_right = nil discussions_left = discussions_right = nil
if left && (left.unchanged? || left.removed?) if left && (left.unchanged? || left.removed?)
line_code = diff_file.line_code(left) line_code = diff_file.line_code(left)
discussion_left = @grouped_diff_discussions[line_code] discussions_left = @grouped_diff_discussions[line_code]
end end
if right && right.added? if right && right.added?
line_code = diff_file.line_code(right) line_code = diff_file.line_code(right)
discussion_right = @grouped_diff_discussions[line_code] discussions_right = @grouped_diff_discussions[line_code]
end end
[discussion_left, discussion_right] [discussions_left, discussions_right]
end end
def inline_diff_btn def inline_diff_btn
......
...@@ -32,27 +32,12 @@ module NotesHelper ...@@ -32,27 +32,12 @@ module NotesHelper
def diff_view_line_data(line_code, position, line_type) def diff_view_line_data(line_code, position, line_type)
return if @diff_notes_disabled return if @diff_notes_disabled
use_legacy_diff_note = @use_legacy_diff_notes
# If the controller doesn't force the use of legacy diff notes, we
# determine this on a line-by-line basis by seeing if there already exist
# active legacy diff notes at this line, in which case newly created notes
# will use the legacy technology as well.
# We do this because the discussion_id values of legacy and "new" diff
# notes, which are used to group notes on the merge request discussion tab,
# are incompatible.
# 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
discussion = @grouped_diff_discussions[line_code]
discussion && discussion.legacy_diff_discussion?
end
data = { data = {
line_code: line_code, line_code: line_code,
line_type: line_type, line_type: line_type,
} }
if use_legacy_diff_note if @use_legacy_diff_notes
data[:note_type] = LegacyDiffNote.name data[:note_type] = LegacyDiffNote.name
else else
data[:note_type] = DiffNote.name data[:note_type] = DiffNote.name
......
...@@ -109,10 +109,9 @@ class Note < ActiveRecord::Base ...@@ -109,10 +109,9 @@ class Note < ActiveRecord::Base
def grouped_diff_discussions def grouped_diff_discussions
diff_notes. diff_notes.
fresh. fresh.
discussions.
select(&:active?). select(&:active?).
group_by(&:line_code). group_by(&:line_code)
map { |line_code, notes| [line_code, DiffDiscussion.build(notes)] }.
to_h
end end
def count_for_collection(ids, type) def count_for_collection(ids, type)
......
...@@ -3,4 +3,4 @@ ...@@ -3,4 +3,4 @@
%td.notes_line{ colspan: 2 } %td.notes_line{ colspan: 2 }
%td.notes_content %td.notes_content
.content{ class: ('hide' unless expanded) } .content{ class: ('hide' unless expanded) }
= render "discussions/notes", discussion: discussion = render partial: "discussions/notes", collection: discussions, as: :discussion
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
.diff-content.code.js-syntax-highlight .diff-content.code.js-syntax-highlight
%table %table
- discussions = { discussion.original_line_code => discussion } - discussions = { discussion.original_line_code => [discussion] }
= render partial: "projects/diffs/line", = render partial: "projects/diffs/line",
collection: discussion.truncated_diff_lines, collection: discussion.truncated_diff_lines,
as: :line, as: :line,
......
%ul.notes{ data: { discussion_id: discussion.id } } .discussion-notes
= render partial: "projects/notes/note", collection: discussion.notes, as: :note %ul.notes{ data: { discussion_id: discussion.id } }
= render partial: "projects/notes/note", collection: discussion.notes, as: :note
- if current_user - if current_user
.discussion-reply-holder .discussion-reply-holder
- if discussion.potentially_resolvable? - if discussion.potentially_resolvable?
- line_type = local_assigns.fetch(:line_type, nil) - line_type = local_assigns.fetch(:line_type, nil)
.btn-group-justified.discussion-with-resolve-btn{ role: "group" } .btn-group-justified.discussion-with-resolve-btn{ role: "group" }
.btn-group{ role: "group" } .btn-group{ role: "group" }
= link_to_reply_discussion(discussion, line_type) = link_to_reply_discussion(discussion, line_type)
= render "discussions/resolve_all", discussion: discussion = render "discussions/resolve_all", discussion: discussion
.btn-group.discussion-actions .btn-group.discussion-actions
= render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable
= render "discussions/jump_to_next", discussion: discussion = render "discussions/jump_to_next", discussion: discussion
- else - else
= link_to_reply_discussion(discussion) = link_to_reply_discussion(discussion)
- expanded = discussion_left.try(:expanded?) || discussion_right.try(:expanded?) - expanded = [*discussions_left, *discussions_right].any?(&:expanded?)
%tr.notes_holder{ class: ('hide' unless expanded) } %tr.notes_holder{ class: ('hide' unless expanded) }
- if discussion_left - if discussions_left
%td.notes_line.old %td.notes_line.old
%td.notes_content.parallel.old %td.notes_content.parallel.old
.content{ class: ('hide' unless discussion_left.expanded?) } .content{ class: ('hide' unless discussions_left.any?(&:expanded?)) }
= render "discussions/notes", discussion: discussion_left, line_type: 'old' = render partial: "discussions/notes", collection: discussions_left, as: :discussion, line_type: 'old'
- else - else
%td.notes_line.old= ("") %td.notes_line.old= ("")
%td.notes_content.parallel.old %td.notes_content.parallel.old
.content .content
- if discussion_right - if discussions_right
%td.notes_line.new %td.notes_line.new
%td.notes_content.parallel.new %td.notes_content.parallel.new
.content{ class: ('hide' unless discussion_right.expanded?) } .content{ class: ('hide' unless discussions_right.any?(&:expanded?)) }
= render "discussions/notes", discussion: discussion_right, line_type: 'new' = render partial: "discussions/notes", collection: discussions_right, as: :discussion, line_type: 'new'
- else - else
%td.notes_line.new= ("") %td.notes_line.new= ("")
%td.notes_content.parallel.new %td.notes_content.parallel.new
......
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
- type = line.type - type = line.type
- line_code = diff_file.line_code(line) - line_code = diff_file.line_code(line)
- if discussions && !line.meta? - if discussions && !line.meta?
- discussion = discussions[line_code] - line_discussions = discussions[line_code]
%tr.line_holder{ class: type, id: (line_code unless plain) } %tr.line_holder{ class: type, id: (line_code unless plain) }
- case type - case type
- when 'match' - when 'match'
...@@ -20,6 +20,7 @@ ...@@ -20,6 +20,7 @@
= link_text = link_text
- else - else
%a{ href: "##{line_code}", data: { linenumber: link_text } } %a{ href: "##{line_code}", data: { linenumber: link_text } }
- discussion = line_discussions.try(:first)
- if discussion && discussion.resolvable? && !plain - if discussion && discussion.resolvable? && !plain
%diff-note-avatars{ "discussion-id" => discussion.id } %diff-note-avatars{ "discussion-id" => discussion.id }
%td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } } %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
...@@ -34,6 +35,6 @@ ...@@ -34,6 +35,6 @@
- else - else
= diff_line_content(line.text) = diff_line_content(line.text)
- if discussion - if line_discussions
- discussion_expanded = local_assigns.fetch(:discussion_expanded, discussion.expanded?) - discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?))
= render "discussions/diff_discussion", discussion: discussion, expanded: discussion_expanded = render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded
...@@ -6,7 +6,7 @@ ...@@ -6,7 +6,7 @@
- right = line[:right] - right = line[:right]
- last_line = right.new_pos if right - last_line = right.new_pos if right
- unless @diff_notes_disabled - unless @diff_notes_disabled
- discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file) - discussions_left, discussions_right = parallel_diff_discussions(left, right, diff_file)
%tr.line_holder.parallel %tr.line_holder.parallel
- if left - if left
- case left.type - case left.type
...@@ -20,6 +20,7 @@ ...@@ -20,6 +20,7 @@
- left_position = diff_file.position(left) - left_position = diff_file.position(left)
%td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } } %td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } }
%a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } } %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } }
- discussion_left = discussions_left.try(:first)
- if discussion_left && discussion_left.resolvable? - if discussion_left && discussion_left.resolvable?
%diff-note-avatars{ "discussion-id" => discussion_left.id } %diff-note-avatars{ "discussion-id" => discussion_left.id }
%td.line_content.parallel.noteable_line{ class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old') }= diff_line_content(left.text) %td.line_content.parallel.noteable_line{ class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old') }= diff_line_content(left.text)
...@@ -39,6 +40,7 @@ ...@@ -39,6 +40,7 @@
- right_position = diff_file.position(right) - right_position = diff_file.position(right)
%td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } } %td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } }
%a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } } %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } }
- discussion_right = discussions_right.try(:first)
- if discussion_right && discussion_right.resolvable? - if discussion_right && discussion_right.resolvable?
%diff-note-avatars{ "discussion-id" => discussion_right.id } %diff-note-avatars{ "discussion-id" => discussion_right.id }
%td.line_content.parallel.noteable_line{ class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new') }= diff_line_content(right.text) %td.line_content.parallel.noteable_line{ class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new') }= diff_line_content(right.text)
...@@ -46,8 +48,8 @@ ...@@ -46,8 +48,8 @@
%td.old_line.diff-line-num.empty-cell %td.old_line.diff-line-num.empty-cell
%td.line_content.parallel %td.line_content.parallel
- if discussion_left || discussion_right - if discussions_left || discussions_right
= render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right = render "discussions/parallel_diff_discussion", discussions_left: discussions_left, discussions_right: discussions_right
- if !diff_file.new_file && !diff_file.deleted_file && diff_file.diff_lines.any? - if !diff_file.new_file && !diff_file.deleted_file && diff_file.diff_lines.any?
- last_line = diff_file.diff_lines.last - last_line = diff_file.diff_lines.last
- if last_line.new_pos < total_lines - if last_line.new_pos < total_lines
......
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