Merge branch 'fix-side-by-side-comment-widget' into 'master'

Fix side by side comment widget

Fixes comment button for empty lines in side by side diff view.

Fixes #19824 
Fixes #19957

See merge request !5262

3 files changed, 103 insertions, 7 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 0535ef95f45..49b994638d4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -127,6 +127,7 @@ v 8.10.0 (unreleased)
   - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention)
   - Redesign Builds and Pipelines pages
   - Change status color and icon for running builds
+  - Fix commenting issue in side by side diff view for unchanged lines
   - Fix markdown rendering for: consecutive labels references, label references that begin with a digit or contains `.`
   - Project export filename now includes the project and namespace path
   - Fix last update timestamp on issues not preserved on gitlab.com and project imports
diff --git a/app/assets/javascripts/files_comment_button.js.coffee b/app/assets/javascripts/files_comment_button.js.coffee
index db0bf7082a9..5ab82c39fcd 100644
--- a/app/assets/javascripts/files_comment_button.js.coffee
+++ b/app/assets/javascripts/files_comment_button.js.coffee
@@ -7,7 +7,6 @@ class @FilesCommentButton
   UNFOLDABLE_LINE_CLASS = 'js-unfold'
   EMPTY_CELL_CLASS = 'empty-cell'
   OLD_LINE_CLASS = 'old_line'
-  NEW_CLASS = 'new'
   LINE_COLUMN_CLASSES = ".#{LINE_NUMBER_CLASS}, .line_content"
   TEXT_FILE_SELECTOR = '.text-file'
   DEBOUNCE_TIMEOUT_DURATION = 100
@@ -18,6 +17,8 @@ class @FilesCommentButton
     debounce = _.debounce @render, DEBOUNCE_TIMEOUT_DURATION
 
     $(document)
+      .off 'mouseover', LINE_COLUMN_CLASSES
+      .off 'mouseleave', LINE_COLUMN_CLASSES
       .on 'mouseover', LINE_COLUMN_CLASSES, debounce
       .on 'mouseleave', LINE_COLUMN_CLASSES, @destroy
 
@@ -64,20 +65,20 @@ class @FilesCommentButton
   getLineContent: (hoveredElement) ->
     return hoveredElement if hoveredElement.hasClass LINE_CONTENT_CLASS
 
-    $(".#{LINE_CONTENT_CLASS + @diffTypeClass hoveredElement}", hoveredElement.parent())
+    if @VIEW_TYPE is 'inline'
+      return $(hoveredElement).closest(LINE_HOLDER_CLASS).find ".#{LINE_CONTENT_CLASS}"
+    else
+      return $(hoveredElement).next ".#{LINE_CONTENT_CLASS}"
 
   getButtonParent: (hoveredElement) ->
     if @VIEW_TYPE is 'inline'
       return hoveredElement if hoveredElement.hasClass OLD_LINE_CLASS
 
-      $(".#{OLD_LINE_CLASS}", hoveredElement.parent())
+      hoveredElement.parent().find ".#{OLD_LINE_CLASS}"
     else
       return hoveredElement if hoveredElement.hasClass LINE_NUMBER_CLASS
 
-      $(".#{LINE_NUMBER_CLASS + @diffTypeClass hoveredElement}", hoveredElement.parent())
-
-  diffTypeClass: (hoveredElement) ->
-    if hoveredElement.hasClass(NEW_CLASS) then '.new' else '.old'
+      $(hoveredElement).prev ".#{LINE_NUMBER_CLASS}"
 
   isMovingToSameType: (e) ->
     newButtonParent = @getButtonParent $(e.toElement)
diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb
index c9a0059645d..35f5bfe46be 100644
--- a/spec/features/merge_requests/diffs_spec.rb
+++ b/spec/features/merge_requests/diffs_spec.rb
@@ -22,4 +22,98 @@ feature 'Diffs URL', js: true, feature: true do
       expect(page).to have_css('.diffs.tab-pane.active')
     end
   end
+
+  context 'when hovering over the parallel view diff file' do
+    let(:comment_button_class) { '.add-diff-note' }
+
+    before(:each) do
+      visit diffs_namespace_project_merge_request_path @project.namespace, @project, @merge_request
+      click_link 'Side-by-side'
+      @old_line_number = first '.diff-line-num.old_line:not(.empty-cell)'
+      @new_line_number = first '.diff-line-num.new_line:not(.empty-cell)'
+      @old_line = first '.line_content[data-line-type="old"]'
+      @new_line = first '.line_content[data-line-type="new"]'
+    end
+
+    it 'shows a comment button on the old side when hovering over an old line number' do
+      @old_line_number.hover
+      expect(@old_line_number).to have_css comment_button_class
+      expect(@new_line_number).not_to have_css comment_button_class
+    end
+
+    it 'shows a comment button on the old side when hovering over an old line' do
+      @old_line.hover
+      expect(@old_line_number).to have_css comment_button_class
+      expect(@new_line_number).not_to have_css comment_button_class
+    end
+
+    it 'shows a comment button on the new side when hovering over a new line number' do
+      @new_line_number.hover
+      expect(@new_line_number).to have_css comment_button_class
+      expect(@old_line_number).not_to have_css comment_button_class
+    end
+
+    it 'shows a comment button on the new side when hovering over a new line' do
+      @new_line.hover
+      expect(@new_line_number).to have_css comment_button_class
+      expect(@old_line_number).not_to have_css comment_button_class
+    end
+  end
+
+  context 'when hovering over the inline view diff file' do
+    let(:comment_button_class) { '.add-diff-note' }
+
+    before(:each) do
+      visit diffs_namespace_project_merge_request_path @project.namespace, @project, @merge_request
+      click_link 'Inline'
+      @old_line_number = first '.diff-line-num.old_line:not(.unfold)'
+      @new_line_number = first '.diff-line-num.new_line:not(.unfold)'
+      @new_line = first '.line_content:not(.match)'
+    end
+
+    it 'shows a comment button on the old side when hovering over an old line number' do
+      @old_line_number.hover
+      expect(@old_line_number).to have_css comment_button_class
+      expect(@new_line_number).not_to have_css comment_button_class
+    end
+
+    it 'shows a comment button on the new side when hovering over a new line number' do
+      @new_line_number.hover
+      expect(@old_line_number).to have_css comment_button_class
+      expect(@new_line_number).not_to have_css comment_button_class
+    end
+
+    it 'shows a comment button on the new side when hovering over a new line' do
+      @new_line.hover
+      expect(@old_line_number).to have_css comment_button_class
+      expect(@new_line_number).not_to have_css comment_button_class
+    end
+  end
+
+  context 'when clicking a comment button' do
+    let(:test_note_comment) { 'this is a test note!' }
+    let(:note_class) { '.new-note' }
+
+    before(:each) do
+      visit diffs_namespace_project_merge_request_path @project.namespace, @project, @merge_request
+      click_link 'Inline'
+      first('.diff-line-num.old_line:not(.unfold)').hover
+      find('.add-diff-note').click
+    end
+
+    it 'shows a note form' do
+      expect(page).to have_css note_class
+    end
+
+    it 'can be submitted and viewed' do
+      fill_in 'note[note]', with: :test_note_comment
+      click_button 'Comment'
+      expect(page).to have_content :test_note_comment
+    end
+
+    it 'can be closed' do
+      find('.note-form-actions .btn-cancel').click
+      expect(page).not_to have_css note_class
+    end
+  end
 end