Merge branch 'implement_comments_on_parallel_diff' into 'master'

Implement comments on parallel diff

Fixes #1565

See merge request !1090

app/assets/stylesheets/sections/notes.scss

@@ -90,6 +90,9 @@ ul.notes {
       border-width: 1px 0;
       padding-top: 0;
       vertical-align: top;
+      &.parallel{
+        border-width: 1px;
+      }
     }
   }
 }

app/helpers/diff_helper.rb

@@ -36,7 +36,10 @@ module DiffHelper
     # Building array of lines
     #
-    # [left_type, left_line_number, left_line_content, line_code, right_line_type, right_line_number, right_line_content]
+    # [
+    # left_type, left_line_number, left_line_content, left_line_code,
+    # right_line_type, right_line_number, right_line_content, right_line_code
+    # ]
     #
     diff_file.diff_lines.each do |line|
@@ -49,26 +52,25 @@ module DiffHelper
       next_line = diff_file.next_line(line.index)
       if next_line
+        next_line_code = generate_line_code(diff_file.file_path, next_line)
         next_type = next_line.type
         next_line = next_line.text
       end
-      line = [type, line_old, full_line, line_code, next_type, line_new]
       if type == 'match' || type.nil?
         # line in the right panel is the same as in the left one
-        line = [type, line_old, full_line, line_code, type, line_new, full_line]
+        line = [type, line_old, full_line, line_code, type, line_new, full_line, line_code]
         lines.push(line)
       elsif type == 'old'
         if next_type == 'new'
           # Left side has text removed, right side has text added
-          line.push(next_line)
+          line = [type, line_old, full_line, line_code, next_type, line_new, next_line, next_line_code]
           lines.push(line)
           skip_next = true
         elsif next_type == 'old' || next_type.nil?
           # Left side has text removed, right side doesn't have any change
-          line.pop # remove the newline
-          line.push(nil) # no line number on the right panel
-          line.push(" ") # empty line on the right panel
+          # No next line code, no new line number, no new line text
+          line = [type, line_old, full_line, line_code, next_type, nil, " ", nil]
           lines.push(line)
         end
       elsif type == 'new'
@@ -78,7 +80,7 @@ module DiffHelper
           next
         else
           # Change is only on the right side, left side has no change
-          line = [nil, nil, " ", line_code, type, line_new, full_line]
+          line = [nil, nil, " ", line_code, type, line_new, full_line, line_code]
           lines.push(line)
         end
       end
@@ -97,4 +99,22 @@ module DiffHelper
       line
     end
   end
+
+  def line_comments
+    @line_comments ||= @line_notes.group_by(&:line_code)
+  end
+
+  def organize_comments(type_left, type_right, line_code_left, line_code_right)
+    comments_left = comments_right = nil
+
+    unless type_left.nil? && type_right == 'new'
+      comments_left = line_comments[line_code_left]
+    end
+
+    unless type_left.nil? && type_right.nil?
+      comments_right = line_comments[line_code_right]
+    end
+
+    [comments_left, comments_right]
+  end
 end

app/views/projects/diffs/_parallel_view.html.haml

@@ -5,22 +5,36 @@
       - type_left = line[0]
       - line_number_left = line[1]
       - line_content_left = line[2]
-      - line_code = line[3]
+      - line_code_left = line[3]
       - type_right = line[4]
       - line_number_right = line[5]
       - line_content_right = line[6]
-      %tr.line_holder.parallel{id: line_code}
+      - line_code_right = line[7]
+      %tr.line_holder.parallel
         - if type_left == 'match'
           = render "projects/diffs/match_line_parallel", { line: line_content_left,
           line_old: line_number_left, line_new: line_number_right }
         - elsif type_left ==  'old' || type_left.nil?
-          %td.old_line{class: "#{type_left}"}
-            = link_to raw(line_number_left), "##{line_code}", id: line_code
-            %td.line_content{class: "parallel noteable_line #{type_left} #{line_code}", "line_code" => line_code }= raw line_content_left
-          %td.new_line{ class: "#{type_right == 'new' ? 'new' : nil}", data: { linenumber: line_number_right }}
-            = link_to raw(line_number_right), "##{line_code}", id: line_code
-            %td.line_content.parallel{class: "noteable_line #{type_right == 'new' ? 'new' : nil} #{line_code}", "line_code" => line_code}= raw line_content_right
+          %td.old_line{id: line_code_left, class: "#{type_left}"}
+            = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left
+            %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw line_content_left
+
+          - if type_right == 'new'
+            - new_line_class = 'new'
+            - new_line_code = line_code_right
+          - else
+            - new_line_class = nil
+            - new_line_code = line_code_left
+
+          %td.new_line{id: new_line_code, class: "#{new_line_class}", data: { linenumber: line_number_right }}
+            = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code
+            %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw line_content_right
+
+      - if @reply_allowed
+        - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right)
+        - if comments_left.present? || comments_right.present?
+          = render "projects/notes/diff_notes_with_reply_parallel", notes1: comments_left, notes2: comments_right
 - if diff_file.diff.diff.blank? && diff_file.mode_changed?
   .file-mode-changed

app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml

-- note1 = notes1.first # example note
-- note2 = notes2.first # example note
--# Check if line want not changed since comment was left
-/- if !defined?(line) || line == note.diff_line
+- note1 = notes1.present? ? notes1.first : nil
+- note2 = notes2.present? ? notes2.first : nil
+
 %tr.notes_holder
   - if note1
     %td.notes_line
       %span.btn.disabled
         %i.icon-comment
         = notes1.count
-    %td.notes_content
+    %td.notes_content.parallel
       %ul.notes{ rel: note1.discussion_id }
         = render notes1
@@ -23,7 +22,7 @@
       %span.btn.disabled
         %i.icon-comment
         = notes2.count
-    %td.notes_content
+    %td.notes_content.parallel
       %ul.notes{ rel: note2.discussion_id }
         = render notes2

features/project/merge_requests.feature

@@ -147,3 +147,13 @@ Feature: Project Merge Requests
     And I switch to the diff tab
     And I unfold diff
     Then I should see additional file lines
+
+  @javascript
+  Scenario: I show comments on a merge request side-by-side diff with comments in multiple files
+    Given project "Shop" have "Bug NS-05" open merge request with diffs inside
+    And I visit merge request page "Bug NS-05"
+    And I switch to the diff tab
+    And I leave a comment like "Line is correct" on line 12 of the first file
+    And I leave a comment like "Line is wrong" on line 39 of the second file
+    And I click Side-by-side Diff tab
+    Then I should see comments on the side-by-side diff page

features/steps/project/merge_requests.rb

@@ -250,6 +250,16 @@ class ProjectMergeRequests < Spinach::FeatureSteps
     expect(first('.text-file')).to have_content('.bundle')
   end
+  step 'I click Side-by-side Diff tab' do
+    click_link 'Side-by-side Diff'
+  end
+
+  step 'I should see comments on the side-by-side diff page' do
+    within '.files [id^=diff]:nth-child(1) .note-text' do
+      page.should have_visible_content "Line is correct"
+    end
+  end
+
   def project
     @project ||= Project.find_by!(name: "Shop")
   end

spec/helpers/diff_helper_spec.rb

@@ -67,32 +67,33 @@ describe DiffHelper do
   def parallel_diff_result_array
     [
-      ["match", 6, "@@ -6,12 +6,18 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", "match", 6, "@@ -6,12 +6,18 @@ module Popen"],
-      [nil, 6, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", nil, 6, " "],
-      [nil, 7, "   def popen(cmd, path=nil)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7", nil, 7, "   def popen(cmd, path=nil)"],
-      [nil, 8, "     unless cmd.is_a?(Array)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8", nil, 8, "     unless cmd.is_a?(Array)"],
-      ["old", 9, "-      raise <span class='idiff'></span>&quot;System commands must be given as an array of strings&quot;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9", "new", 9, "+      raise <span class='idiff'>RuntimeError, </span>&quot;System commands must be given as an array of strings&quot;"],
-      [nil, 10, "     end", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10", nil, 10, "     end"], [nil, 11, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11", nil, 11, " "],
-      [nil, 12, "     path ||= Dir.pwd", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12", nil, 12, "     path ||= Dir.pwd"],
-      ["old", 13, "-    vars = { &quot;PWD&quot; =&gt; path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13", "old", nil, "&nbsp;"],
-      ["old", 14, "-    options = { chdir: path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13", "new", 13, "+"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14", "new", 14, "+    vars = {"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15", "new", 15, "+      &quot;PWD&quot; =&gt; path"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16", "new", 16, "+    }"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17", "new", 17, "+"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18", "new", 18, "+    options = {"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19", "new", 19, "+      chdir: path"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20", "new", 20, "+    }"],
-      [nil, 15, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21", nil, 21, " "],
-      [nil, 16, "     unless File.directory?(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22", nil, 22, "     unless File.directory?(path)"],
-      [nil, 17, "       FileUtils.mkdir_p(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23", nil, 23, "       FileUtils.mkdir_p(path)"],
-      ["match", 19, "@@ -19,6 +25,7 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", "match", 25, "@@ -19,6 +25,7 @@ module Popen"],
-      [nil, 19, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", nil, 25, " "], [nil, 20, "     @cmd_output = &quot;&quot;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26", nil, 26, "     @cmd_output = &quot;&quot;"],
-      [nil, 21, "     @cmd_status = 0", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27", nil, 27, "     @cmd_status = 0"],
-      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28", "new", 28, "+"],
-      [nil, 22, "     Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29", nil, 29, "     Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|"],
-      [nil, 23, "       @cmd_output &lt;&lt; stdout.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30", nil, 30, "       @cmd_output &lt;&lt; stdout.read"],
-      [nil, 24, "       @cmd_output &lt;&lt; stderr.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31", nil, 31, "       @cmd_output &lt;&lt; stderr.read"]
+      ["match", 6, "@@ -6,12 +6,18 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", "match", 6, "@@ -6,12 +6,18 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6"],
+      [nil, 6, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", nil, 6, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6"], [nil, 7, "   def popen(cmd, path=nil)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7", nil, 7, "   def popen(cmd, path=nil)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"],
+      [nil, 8, "     unless cmd.is_a?(Array)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8", nil, 8, "     unless cmd.is_a?(Array)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8"],
+      ["old", 9, "-      raise <span class='idiff'></span>&quot;System commands must be given as an array of strings&quot;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9", "new", 9, "+      raise <span class='idiff'>RuntimeError, </span>&quot;System commands must be given as an array of strings&quot;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"],
+      [nil, 10, "     end", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10", nil, 10, "     end", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10"],
+      [nil, 11, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11", nil, 11, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11"],
+      [nil, 12, "     path ||= Dir.pwd", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12", nil, 12, "     path ||= Dir.pwd", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12"],
+      ["old", 13, "-    vars = { &quot;PWD&quot; =&gt; path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13", "old", nil, "&nbsp;", nil],
+      ["old", 14, "-    options = { chdir: path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13", "new", 13, "+", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14", "new", 14, "+    vars = {", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15", "new", 15, "+      &quot;PWD&quot; =&gt; path", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16", "new", 16, "+    }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17", "new", 17, "+", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18", "new", 18, "+    options = {", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19", "new", 19, "+      chdir: path", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20", "new", 20, "+    }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20"],
+      [nil, 15, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21", nil, 21, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21"],
+      [nil, 16, "     unless File.directory?(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22", nil, 22, "     unless File.directory?(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22"],
+      [nil, 17, "       FileUtils.mkdir_p(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23", nil, 23, "       FileUtils.mkdir_p(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23"],
+      ["match", 19, "@@ -19,6 +25,7 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", "match", 25, "@@ -19,6 +25,7 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25"],
+      [nil, 19, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", nil, 25, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25"],
+      [nil, 20, "     @cmd_output = &quot;&quot;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26", nil, 26, "     @cmd_output = &quot;&quot;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26"],
+      [nil, 21, "     @cmd_status = 0", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27", nil, 27, "     @cmd_status = 0", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27"],
+      [nil, nil, "&nbsp;", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28", "new", 28, "+", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28"],
+      [nil, 22, "     Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29", nil, 29, "     Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29"],
+      [nil, 23, "       @cmd_output &lt;&lt; stdout.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30", nil, 30, "       @cmd_output &lt;&lt; stdout.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30"],
+      [nil, 24, "       @cmd_output &lt;&lt; stderr.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31", nil, 31, "       @cmd_output &lt;&lt; stderr.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31"]
     ]
   end
 end