diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 57335c77e40..edfcda7c214 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -668,7 +668,7 @@ require('./task_list'); return function(i, el) { var note, notes; note = $(el); - notes = note.closest(".notes"); + notes = note.closest(".discussion-notes"); if (typeof gl.diffNotesCompileComponents !== 'undefined') { if (gl.diffNoteApps[noteElId]) { @@ -685,14 +685,13 @@ require('./task_list'); // "Discussions" tab notes.closest(".timeline-entry").remove(); - if (!_this.isParallelView() || notesTr.find('.note').length === 0) { - // "Changes" tab / commit view - notesTr.remove(); + // The notes tr can contain multiple lists of notes, like on the parallel diff + if (notesTr.find('.discussion-notes').length > 1) { + notes.remove(); } else { - notes.closest('.content').empty(); + notesTr.remove(); } } - return note.remove(); }; })(this)); // Decrement the "Discussions" counter only once diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 57cf8e136e2..33efba923b0 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -294,6 +294,18 @@ ul.notes { 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 { background-color: $white-light; } diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 98cd4ce344a..d1558bdfba8 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -123,7 +123,7 @@ class Projects::CommitController < Projects::ApplicationController @grouped_diff_discussions = commit.grouped_diff_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) end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7863fe18f32..48a10fbe9cc 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -591,7 +591,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs? @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 def define_pipelines_vars diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index fb9195bb08d..7ae590d3de1 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -136,13 +136,13 @@ class Projects::NotesController < Projects::ApplicationController template = "discussions/_parallel_diff_discussion" locals = if params[:line_type] == 'old' - { discussion_left: discussion, discussion_right: nil } + { discussions_left: [discussion], discussions_right: nil } else - { discussion_left: nil, discussion_right: discussion } + { discussions_left: nil, discussions_right: [discussion] } end else template = "discussions/_diff_discussion" - locals = { discussion: discussion } + locals = { discussions: [discussion] } end render_to_string( diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index aed1d7c839f..5e0886cc599 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -62,19 +62,19 @@ module DiffHelper end def parallel_diff_discussions(left, right, diff_file) - discussion_left = discussion_right = nil + discussions_left = discussions_right = nil if left && (left.unchanged? || left.removed?) line_code = diff_file.line_code(left) - discussion_left = @grouped_diff_discussions[line_code] + discussions_left = @grouped_diff_discussions[line_code] end if right && right.added? line_code = diff_file.line_code(right) - discussion_right = @grouped_diff_discussions[line_code] + discussions_right = @grouped_diff_discussions[line_code] end - [discussion_left, discussion_right] + [discussions_left, discussions_right] end def inline_diff_btn diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 61805021b39..8ec72d5f43a 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -32,27 +32,12 @@ module NotesHelper def diff_view_line_data(line_code, position, line_type) 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 = { line_code: line_code, line_type: line_type, } - if use_legacy_diff_note + if @use_legacy_diff_notes data[:note_type] = LegacyDiffNote.name else data[:note_type] = DiffNote.name diff --git a/app/models/note.rb b/app/models/note.rb index 3db1656ba57..9d4f99ac9c8 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -109,10 +109,9 @@ class Note < ActiveRecord::Base def grouped_diff_discussions diff_notes. fresh. + discussions. select(&:active?). - group_by(&:line_code). - map { |line_code, notes| [line_code, DiffDiscussion.build(notes)] }. - to_h + group_by(&:line_code) end def count_for_collection(ids, type) diff --git a/app/views/discussions/_diff_discussion.html.haml b/app/views/discussions/_diff_discussion.html.haml index ee452add394..e6d307e5568 100644 --- a/app/views/discussions/_diff_discussion.html.haml +++ b/app/views/discussions/_diff_discussion.html.haml @@ -3,4 +3,4 @@ %td.notes_line{ colspan: 2 } %td.notes_content .content{ class: ('hide' unless expanded) } - = render "discussions/notes", discussion: discussion + = render partial: "discussions/notes", collection: discussions, as: :discussion diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 94408b92374..549364761e6 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -7,7 +7,7 @@ .diff-content.code.js-syntax-highlight %table - - discussions = { discussion.original_line_code => discussion } + - discussions = { discussion.original_line_code => [discussion] } = render partial: "projects/diffs/line", collection: discussion.truncated_diff_lines, as: :line, diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml index ad778612f92..34789808f10 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -1,19 +1,20 @@ -%ul.notes{ data: { discussion_id: discussion.id } } - = render partial: "projects/notes/note", collection: discussion.notes, as: :note +.discussion-notes + %ul.notes{ data: { discussion_id: discussion.id } } + = render partial: "projects/notes/note", collection: discussion.notes, as: :note -- if current_user - .discussion-reply-holder - - if discussion.potentially_resolvable? - - line_type = local_assigns.fetch(:line_type, nil) + - if current_user + .discussion-reply-holder + - if discussion.potentially_resolvable? + - line_type = local_assigns.fetch(:line_type, nil) - .btn-group-justified.discussion-with-resolve-btn{ role: "group" } - .btn-group{ role: "group" } - = link_to_reply_discussion(discussion, line_type) + .btn-group-justified.discussion-with-resolve-btn{ role: "group" } + .btn-group{ role: "group" } + = link_to_reply_discussion(discussion, line_type) - = render "discussions/resolve_all", discussion: discussion + = render "discussions/resolve_all", discussion: discussion - .btn-group.discussion-actions - = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable - = render "discussions/jump_to_next", discussion: discussion - - else - = link_to_reply_discussion(discussion) + .btn-group.discussion-actions + = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable + = render "discussions/jump_to_next", discussion: discussion + - else + = link_to_reply_discussion(discussion) diff --git a/app/views/discussions/_parallel_diff_discussion.html.haml b/app/views/discussions/_parallel_diff_discussion.html.haml index 3a19e021643..253cd336882 100644 --- a/app/views/discussions/_parallel_diff_discussion.html.haml +++ b/app/views/discussions/_parallel_diff_discussion.html.haml @@ -1,20 +1,20 @@ -- expanded = discussion_left.try(:expanded?) || discussion_right.try(:expanded?) +- expanded = [*discussions_left, *discussions_right].any?(&:expanded?) %tr.notes_holder{ class: ('hide' unless expanded) } - - if discussion_left + - if discussions_left %td.notes_line.old %td.notes_content.parallel.old - .content{ class: ('hide' unless discussion_left.expanded?) } - = render "discussions/notes", discussion: discussion_left, line_type: 'old' + .content{ class: ('hide' unless discussions_left.any?(&:expanded?)) } + = render partial: "discussions/notes", collection: discussions_left, as: :discussion, line_type: 'old' - else %td.notes_line.old= ("") %td.notes_content.parallel.old .content - - if discussion_right + - if discussions_right %td.notes_line.new %td.notes_content.parallel.new - .content{ class: ('hide' unless discussion_right.expanded?) } - = render "discussions/notes", discussion: discussion_right, line_type: 'new' + .content{ class: ('hide' unless discussions_right.any?(&:expanded?)) } + = render partial: "discussions/notes", collection: discussions_right, as: :discussion, line_type: 'new' - else %td.notes_line.new= ("") %td.notes_content.parallel.new diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index c09c7b87e24..3e426ee9e7d 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -4,7 +4,7 @@ - type = line.type - line_code = diff_file.line_code(line) - if discussions && !line.meta? - - discussion = discussions[line_code] + - line_discussions = discussions[line_code] %tr.line_holder{ class: type, id: (line_code unless plain) } - case type - when 'match' @@ -20,6 +20,7 @@ = link_text - else %a{ href: "##{line_code}", data: { linenumber: link_text } } + - discussion = line_discussions.try(:first) - if discussion && discussion.resolvable? && !plain %diff-note-avatars{ "discussion-id" => discussion.id } %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } } @@ -34,6 +35,6 @@ - else = diff_line_content(line.text) -- if discussion - - discussion_expanded = local_assigns.fetch(:discussion_expanded, discussion.expanded?) - = render "discussions/diff_discussion", discussion: discussion, expanded: discussion_expanded +- if line_discussions + - discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?)) + = render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index b7346f27ddb..f920f359de2 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -6,7 +6,7 @@ - right = line[:right] - last_line = right.new_pos if right - 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 - if left - case left.type @@ -20,6 +20,7 @@ - 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 } } %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } } + - discussion_left = discussions_left.try(:first) - if discussion_left && discussion_left.resolvable? %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) @@ -39,6 +40,7 @@ - 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 } } %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } } + - discussion_right = discussions_right.try(:first) - if discussion_right && discussion_right.resolvable? %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) @@ -46,8 +48,8 @@ %td.old_line.diff-line-num.empty-cell %td.line_content.parallel - - if discussion_left || discussion_right - = render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right + - if discussions_left || discussions_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? - last_line = diff_file.diff_lines.last - if last_line.new_pos < total_lines