Correctly display multiple separate discussions on the same diff line

14 files changed, 64 insertions, 65 deletions

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