Merge branch 'support-comment-parallel-diff' into 'master'

Support commenting on a diff in side-by-side view

### What does this MR do?

This MR adds support for commenting on a diff in side-by-side (aka parallel) view. It also fixes a JavaScript bug (see !779) when the comment button is clicked on a line that already has a comment.

There is an existing bug where the comment count is not updated when a new comment is added. I'll send a MR for that later.

### Why was this MR needed?

Commenting only worked in "inline" mode. Often the side-by-side view is more conducive to writing comments.

### What are the relevant issue numbers?

Closes https://github.com/gitlabhq/gitlabhq/issues/9283

### Screenshot

![image](https://gitlab.com/stanhu/gitlab-ce/uploads/3d0a3213fde38844a681a826da18139b/image.png)

See merge request !810

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.13.0 (unreleased)
+  - Support commenting on diffs in side-by-side mode (Stan Hu)
+  - Fix JavaScript error when clicking on the comment button on a diff line that has a comment already (Stan Hu)
   - Remove project visibility icons from dashboard projects list
   - Rename "Design" profile settings page to "Preferences".
   - Allow users to customize their default Dashboard page.

app/assets/javascripts/notes.js.coffee

@@ -8,11 +8,12 @@
 class @Notes
   @interval: null
 
-  constructor: (notes_url, note_ids, last_fetched_at) ->
+  constructor: (notes_url, note_ids, last_fetched_at, view) ->
     @notes_url = notes_url
     @notes_url = gon.relative_url_root + @notes_url if gon.relative_url_root?
     @note_ids = note_ids
     @last_fetched_at = last_fetched_at
+    @view = view
     @noteable_url = document.URL
     @initRefresh()
     @setupMainTargetNoteForm()
@@ -131,6 +132,8 @@ class @Notes
   isNewNote: (note) ->
     $.inArray(note.id, @note_ids) == -1
 
+  isParallelView: ->
+    @view == 'parallel'
 
   ###
   Render note in discussion area.
@@ -391,6 +394,7 @@ class @Notes
   setupDiscussionNoteForm: (dataHolder, form) =>
     # setup note target
     form.attr "rel", dataHolder.data("discussionId")
+    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")
     form.find("#note_noteable_type").val dataHolder.data("noteableType")
@@ -411,19 +415,40 @@ class @Notes
     form = $(".js-new-note-form")
     row = $(link).closest("tr")
     nextRow = row.next()
-
-    # does it already have notes?
-    if nextRow.is(".notes_holder")
-      replyButton = nextRow.find(".js-discussion-reply-button")
-      if replyButton.length > 0
-        $.proxy(@replyToDiscussionNote, replyButton).call()
+    hasNotes = nextRow.is(".notes_holder")
+    addForm = false
+    targetContent = ".notes_content"
+    rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"></td></tr>"
+
+    # In parallel view, look inside the correct left/right pane
+    if @isParallelView()
+      lineType = $(link).data("lineType")
+      targetContent += "." + lineType
+      rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\"></td><td class=\"notes_content parallel old\"></td><td class=\"notes_line\"></td><td class=\"notes_content parallel new\"></td></tr>"
+
+    if hasNotes
+      notesContent = nextRow.find(targetContent)
+      if notesContent.length
+        replyButton = notesContent.find(".js-discussion-reply-button:visible")
+        if replyButton.length
+          e.target = replyButton[0]
+          $.proxy(@replyToDiscussionNote, replyButton[0], e).call()
+        else
+          # In parallel view, the form may not be present in one of the panes
+          noteForm = notesContent.find(".js-discussion-note-form")
+          if noteForm.length == 0
+            addForm = true
     else
       # add a notes row and insert the form
-      row.after "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"></td></tr>"
-      form.clone().appendTo row.next().find(".notes_content")
+      row.after rowCssToAdd
+      addForm = true
+
+    if addForm
+      newForm = form.clone()
+      newForm.appendTo row.next().find(targetContent)
 
       # show the form
-      @setupDiscussionNoteForm $(link), row.next().find("form")
+      @setupDiscussionNoteForm $(link), newForm
 
   ###
   Called in response to "cancel" on a diff note form.

app/controllers/projects/notes_controller.rb

@@ -77,11 +77,24 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def note_to_discussion_html(note)
+    if params[:view] == 'parallel'
+      template = "projects/notes/_diff_notes_with_reply_parallel"
+      locals =
+        if params[:line_type] == 'old'
+          { notes_left: [note], notes_right: [] }
+        else
+          { notes_left: [], notes_right: [note] }
+       end
+    else
+      template = "projects/notes/_diff_notes_with_reply"
+      locals = { notes: [note] }
+    end
+
     render_to_string(
-      "projects/notes/_diff_notes_with_reply",
+      template,
       layout: false,
       formats: [:html],
-      locals: { notes: [note] }
+      locals: locals
     )
   end
 

app/helpers/notes_helper.rb

@@ -47,7 +47,7 @@ module NotesHelper
     }.to_json
   end
 
-  def link_to_new_diff_note(line_code)
+  def link_to_new_diff_note(line_code, line_type = nil)
     discussion_id = Note.build_discussion_id(
       @comments_target[:noteable_type],
       @comments_target[:noteable_id] || @comments_target[:commit_id],
@@ -59,7 +59,8 @@ module NotesHelper
       noteable_id:   @comments_target[:noteable_id],
       commit_id:     @comments_target[:commit_id],
       line_code:     line_code,
-      discussion_id: discussion_id
+      discussion_id: discussion_id,
+      line_type:     line_type
     }
 
     button_tag(class: 'btn add-diff-note js-add-diff-note-button',
@@ -69,7 +70,7 @@ module NotesHelper
     end
   end
 
-  def link_to_reply_diff(note)
+  def link_to_reply_diff(note, line_type = nil)
     return unless current_user
 
     data = {
@@ -77,7 +78,8 @@ module NotesHelper
       noteable_id:   note.noteable_id,
       commit_id:     note.commit_id,
       line_code:     note.line_code,
-      discussion_id: note.discussion_id
+      discussion_id: note.discussion_id,
+      line_type:     line_type
     }
 
     button_tag class: 'btn reply-btn js-discussion-reply-button',

app/views/projects/commit/show.html.haml

 - page_title "#{@commit.title} (#{@commit.short_id})", "Commits"
 = render "commit_box"
 = render "projects/diffs/diffs", diffs: @diffs, project: @project
-= render "projects/notes/notes_with_form"
+= render "projects/notes/notes_with_form", view: params[:view]

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

@@ -18,6 +18,8 @@
         - elsif type_left ==  'old' || type_left.nil?
           %td.old_line{id: line_code_left, class: "#{type_left}"}
             = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left
+            - if @comments_allowed && can?(current_user, :write_note, @project)
+              = link_to_new_diff_note(line_code_left, 'old')
             %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'
@@ -29,12 +31,14 @@
 
           %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
+            - if @comments_allowed && can?(current_user, :write_note, @project)
+              = link_to_new_diff_note(line_code_right, 'new')
             %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
+          = render "projects/notes/diff_notes_with_reply_parallel", notes_left: comments_left, notes_right: 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.present? ? notes1.first : nil
-- note2 = notes2.present? ? notes2.first : nil
+- note1 = notes_left.present? ? notes_left.first : nil
+- note2 = notes_right.present? ? notes_right.first : nil
 
 %tr.notes_holder
   - if note1
-    %td.notes_line
+    %td.notes_line.old
       %span.btn.disabled
         %i.fa.fa-comment
-        = notes1.count
-    %td.notes_content.parallel
+        = notes_left.count
+    %td.notes_content.parallel.old
       %ul.notes{ rel: note1.discussion_id }
-        = render notes1
+        = render notes_left
 
       .discussion-reply-holder
-        = link_to_reply_diff(note1)
+        = link_to_reply_diff(note1, 'old')
   - else
-    %td= ""
-    %td= ""
+    %td.notes_line.old= ""
+    %td.notes_content.parallel.old= ""
 
   - if note2
-    %td.notes_line
+    %td.notes_line.new
       %span.btn.disabled
         %i.fa.fa-comment
-        = notes2.count
-    %td.notes_content.parallel
+        = notes_right.count
+    %td.notes_content.parallel.new
       %ul.notes{ rel: note2.discussion_id }
-        = render notes2
+        = render notes_right
 
       .discussion-reply-holder
-        = link_to_reply_diff(note2)
+        = link_to_reply_diff(note2, 'new')
   - else
-    %td= ""
-    %td= ""
+    %td.notes_line.new= ""
+    %td.notes_content.parallel.new= ""
 

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

 = form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new_note js-new-note-form common-note-form gfm-form" }, authenticity_token: true do |f|
+  = hidden_field_tag :view, params[:view]
+  = hidden_field_tag :line_type
   = note_target_fields(@note)
   = f.hidden_field :commit_id
   = f.hidden_field :line_code

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

@@ -4,7 +4,7 @@
 
 .js-main-target-form
 - if can? current_user, :write_note, @project
-  = render "projects/notes/form"
+  = render "projects/notes/form", view: params[:view]
 
 :javascript
-  new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i})
+  new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}, "#{params[:view]}")

features/project/commits/diff_comments.feature

@@ -77,3 +77,17 @@ Feature: Project Commits Diff Comments
     And I submit the diff comment
     Then I should not see the diff comment form
     And I should see a discussion reply button
+
+  @javascript
+  Scenario: I can add a comment on a side-by-side commit diff (left side)
+    Given I open a diff comment form
+    And I click side-by-side diff button
+    When I leave a diff comment in a parallel view on the left side like "Old comment"
+    Then I should see a diff comment on the left side saying "Old comment"
+
+  @javascript
+  Scenario: I can add a comment on a side-by-side commit diff (right side)
+    Given I open a diff comment form
+    And I click side-by-side diff button
+    When I leave a diff comment in a parallel view on the right side like "New comment"
+    Then I should see a diff comment on the right side saying "New comment"

features/steps/project/commits/commits.rb

@@ -2,6 +2,7 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
   include SharedAuthentication
   include SharedProject
   include SharedPaths
+  include SharedDiffNote
   include RepoHelpers
 
   step 'I see project commits' do
@@ -88,14 +89,6 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
     expect(links[1]['href']).to match %r{blob/#{sample_image_commit.new_blob_id}}
   end
 
-  step 'I click side-by-side diff button' do
-    click_link "Side-by-side"
-  end
-
-  step 'I see side-by-side diff button' do
-    expect(page).to have_content "Side-by-side"
-  end
-
   step 'I see inline diff button' do
     expect(page).to have_content "Inline"
   end

features/steps/shared/diff_note.rb

@@ -28,6 +28,22 @@ module SharedDiffNote
     end
   end
 
+  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[rel$='#{sample_commit.line_code}']") do
+      fill_in "note[note]", with: "Old comment"
+      find(".js-comment-button").trigger("click")
+    end
+  end
+
+  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[rel$='#{sample_commit.line_code}']") do
+      fill_in "note[note]", with: "New comment"
+      find(".js-comment-button").trigger("click")
+    end
+  end
+
   step 'I preview a diff comment text like "Should fix it :smile:"' do
     click_diff_line(sample_commit.line_code)
     page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do
@@ -102,6 +118,18 @@ module SharedDiffNote
     end
   end
 
+  step 'I should see a diff comment on the left side saying "Old comment"' do
+    page.within("#{diff_file_selector} .notes_content.parallel.old") do
+      expect(page).to have_content("Old comment")
+    end
+  end
+
+  step 'I should see a diff comment on the right side saying "New comment"' do
+    page.within("#{diff_file_selector} .notes_content.parallel.new") do
+      expect(page).to have_content("New comment")
+    end
+  end
+
   step 'I should see a discussion reply button' do
     page.within(diff_file_selector) do
       expect(page).to have_button('Reply')
@@ -157,6 +185,14 @@ module SharedDiffNote
     end
   end
 
+  step 'I click side-by-side diff button' do
+    click_link "Side-by-side"
+  end
+
+  step 'I see side-by-side diff button' do
+    expect(page).to have_content "Side-by-side"
+  end
+
   def diff_file_selector
     ".diff-file:nth-of-type(1)"
   end
@@ -164,4 +200,8 @@ module SharedDiffNote
   def click_diff_line(code)
     find("button[data-line-code='#{code}']").click
   end
+
+  def click_parallel_diff_line(code, line_type)
+    find("button[data-line-code='#{code}'][data-line-type='#{line_type}']").trigger('click')
+  end
 end