Diffs will create button/diff form on demand no on server side

CHANGELOG

@@ -24,6 +24,7 @@ v 8.10.0 (unreleased)
   - Allow importing from Github using Personal Access Tokens. (Eric K Idema)
   - API: Todos !3188 (Robert Schilling)
   - Add "Enabled Git access protocols" to Application Settings
+  - Diffs will create button/diff form on demand no on server side
   - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
   - PipelinesFinder uses git cache data
   - Check for conflicts with existing Project's wiki path when creating a new project.

app/assets/javascripts/diff.js.coffee

@@ -1,6 +1,8 @@
 class @Diff
   UNFOLD_COUNT = 20
   constructor: ->
+    @filesCommentButton = new FilesCommentButton($('.files'))
+
     $(document).off('click', '.js-unfold')
     $(document).on('click', '.js-unfold', (event) =>
       target = $(event.target)
@@ -36,7 +38,7 @@ class @Diff
         # see https://gitlab.com/gitlab-org/gitlab-ce/issues/707
         indent: 1
 
-      $.get(link, params, (response) =>
+      $.get(link, params, (response) ->
         target.parent().replaceWith(response)
       )
     )

app/assets/javascripts/files_comment_button.js.coffee

@@ -0,0 +1,82 @@
+class @FilesCommentButton
+  constructor: (@filesContainerElement) ->
+    return if not @filesContainerElement and not @filesContainerElement.data 'can-create-note'
+
+    @COMMENT_BUTTON_CLASS = '.add-diff-note'
+    @COMMENT_BUTTON_TEMPLATE = _.template("<button name='button' type='submit' class='btn <%- COMMENT_BUTTON_CLASS %> js-add-diff-note-button' title='Add a comment to this line'><i class='fa fa-comment-o'></i></button>")
+
+    @LINE_HOLDER_CLASS = '.line_holder'
+    @LINE_NUMBER_CLASS = 'diff-line-num'
+    @LINE_CONTENT_CLASS = 'line_content'
+    @LINE_COLUMN_CLASSES = ".#{@LINE_NUMBER_CLASS}, .line_content"
+
+    @DEBOUNCE_TIMEOUT_DURATION = 150
+
+    $(document)
+      .on 'mouseover', @LINE_COLUMN_CLASSES, @debounceRender
+      .on 'mouseleave', @LINE_COLUMN_CLASSES, @destroy
+
+  debounceRender: (e) =>
+    clearTimeout @debounceTimeout if @debounceTimeout
+    @debounceTimeout = setTimeout =>
+      @render e
+    , @DEBOUNCE_TIMEOUT_DURATION
+    return
+
+  render: (e) ->
+    lineHolderElement = @getLineHolder($(e.currentTarget))
+    lineContentElement = @getLineContent($(e.currentTarget))
+    lineNumElement = @getLineNum($(e.currentTarget))
+    buttonParentElement = lineNumElement
+
+    return if not @shouldRender e, buttonParentElement
+
+    buttonParentElement.append @buildButton
+      id:
+        noteable: lineHolderElement.attr 'data-noteable-id'
+        commit: lineHolderElement.attr 'data-commit-id'
+        discussion: lineContentElement.attr('data-discussion-id') || lineHolderElement.attr('data-discussion-id')
+      type:
+        noteable: lineHolderElement.attr 'data-noteable-type'
+        note: lineHolderElement.attr 'data-note-type'
+        line: lineContentElement.attr 'data-line-type'
+      code:
+        line: lineContentElement.attr('data-line-code') || lineHolderElement.attr('id')
+    return
+
+  destroy: (e) =>
+    return if @isMovingToSameType e
+    $(@COMMENT_BUTTON_CLASS, @getLineNum $(e.currentTarget)).remove()
+    return
+
+  buildButton: (buttonAttributes) ->
+    $(@COMMENT_BUTTON_TEMPLATE COMMENT_BUTTON_CLASS: @COMMENT_BUTTON_CLASS.substr 1).attr
+      'data-noteable-id': buttonAttributes.id.noteable
+      'data-commit-id': buttonAttributes.id.commit
+      'data-discussion-id': buttonAttributes.id.discussion
+      'data-noteable-type': buttonAttributes.type.noteable
+      'data-line-type': buttonAttributes.type.line
+      'data-note-type': buttonAttributes.type.note
+      'data-line-code': buttonAttributes.code.line
+
+  getLineHolder: (hoveredElement) ->
+    return hoveredElement if hoveredElement.hasClass @LINE_HOLDER_CLASS
+    $(hoveredElement.parent())
+
+  getLineNum: (hoveredElement) ->
+    return hoveredElement if hoveredElement.hasClass @LINE_NUMBER_CLASS
+
+    $(hoveredElement).prev('.' + @LINE_NUMBER_CLASS)
+
+  getLineContent: (hoveredElement) ->
+    return hoveredElement if hoveredElement.hasClass @LINE_CONTENT_CLASS
+
+    $(hoveredElement).next('.' + @LINE_CONTENT_CLASS)
+
+  isMovingToSameType: (e) ->
+    newLineNum = @getLineNum($(e.toElement))
+    return false unless newLineNum
+    (newLineNum).is @getLineNum($(e.currentTarget))
+
+  shouldRender: (e, buttonParentElement) ->
+    (!buttonParentElement.hasClass('empty-cell') and $(@COMMENT_BUTTON_CLASS, buttonParentElement).length is 0)

app/assets/javascripts/merge_request_tabs.js.coffee

@@ -153,7 +153,6 @@ class @MergeRequestTabs
 
   loadDiff: (source) ->
     return if @diffsLoaded
-
     @_get
       url: "#{source}.json" + @_location.search
       success: (data) =>

app/controllers/projects/compare_controller.rb

@@ -24,6 +24,7 @@ class Projects::CompareController < Projects::ApplicationController
       @diff_refs = [@base_commit, @commit]
       @diff_notes_disabled = true
       @grouped_diff_notes = {}
+      @comments_target = {}
     end
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -138,6 +138,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @base_commit = @merge_request.diff_base_commit
     @diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare
     @diff_notes_disabled = true
+    @comments_target = {}
 
     @pipeline = @merge_request.pipeline
     @statuses = @pipeline.statuses if @pipeline

app/helpers/notes_helper.rb

@@ -24,28 +24,12 @@ module NotesHelper
     }.to_json
   end
 
-  def link_to_new_diff_note(line_code, line_type = nil)
+  def discussion_id(line_code)
-    discussion_id = LegacyDiffNote.build_discussion_id(
+    LegacyDiffNote.build_discussion_id(
       @comments_target[:noteable_type],
       @comments_target[:noteable_id] || @comments_target[:commit_id],
       line_code
     )
-
-    data = {
-      noteable_type: @comments_target[:noteable_type],
-      noteable_id:   @comments_target[:noteable_id],
-      commit_id:     @comments_target[:commit_id],
-      line_type:     line_type,
-      line_code:     line_code,
-      note_type:     LegacyDiffNote.name,
-      discussion_id: discussion_id
-    }
-
-    button_tag(class: 'btn add-diff-note js-add-diff-note-button',
-               data: data,
-               title: 'Add a comment to this line') do
-      icon('comment-o')
-    end
   end
 
   def link_to_reply_discussion(note, line_type = nil)

app/mailers/emails/projects.rb

@@ -30,6 +30,7 @@ module Emails
       @target_url = @message.target_url
       @project = Project.find(project_id)
       @diff_notes_disabled = true
+      @comments_target = {}
 
       add_project_headers
       headers['X-GitLab-Author'] = @message.author_username

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

@@ -21,7 +21,7 @@
 - if diff_files.overflow?
   = render 'projects/diffs/warning', diff_files: diff_files
 
-.files
+.files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, @project)).to_s}}
   - diff_files.each_with_index do |diff_file, index|
     - diff_commit = commit_for_diff(diff_file)
     - blob = project.repository.blob_for_diff(diff_commit, diff_file)

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

@@ -1,5 +1,6 @@
 - type = line.type
-%tr.line_holder{ id: line_code, class: type }
+- holder_data = @comments_target.any? ? { data: { noteable_id: @comments_target[:noteable_id], noteable_type: @comments_target[:noteable_type], commit_id: @comments_target[:commit_id], discussion_id: discussion_id(line_code), note_type: LegacyDiffNote.name } } : {}
+%tr.line_holder{ holder_data, id: line_code, class: type }
   - case type
   - when 'match'
     = render "projects/diffs/match_line", { line: line.text,
@@ -10,17 +11,15 @@
     %td.line_content.match= line.text
   - else
     %td.old_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
-      - link_text = type == "new" ? " ".html_safe : line.old_pos
+      - link_text = type == "new" ? " " : line.old_pos
       - if defined?(plain) && plain
         = link_text
       - else
-        = link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text }
+        %a{href: "##{line_code}", data: { linenumber: link_text }}= "  "
-      - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
-        = link_to_new_diff_note(line_code)
     %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
-      - link_text = type == "old" ? " ".html_safe : line.new_pos
+      - link_text = type == "old" ? " " : line.new_pos
       - if defined?(plain) && plain
         = link_text
       - else
-        = link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text }
+        %a{href: "##{line_code}", data: { linenumber: link_text }}= ""
-    %td.line_content{ class: ['noteable_line', type, line_code], data: { line_code: line_code } }= diff_line_content(line.text, type)
+    %td.line_content{ class: ['noteable_line', type], data: { line_code: line_code, line_type: type } }= diff_line_content(line.text, type)

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

@@ -1,4 +1,4 @@
-%td.old_line.diff-line-num
+%td.old_line.diff-line-num.empty-cell
 %td.line_content.parallel.match= line
-%td.new_line.diff-line-num
+%td.new_line.diff-line-num.empty-cell
 %td.line_content.parallel.match= line

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

@@ -4,21 +4,19 @@
     - diff_file.parallel_diff_lines.each do |line|
       - left = line[:left]
       - right = line[:right]
-      %tr.line_holder.parallel
+      - holder_data = @comments_target.any? ? { data: { noteable_id: @comments_target[:noteable_id], noteable_type: @comments_target[:noteable_type], commit_id: @comments_target[:commit_id], note_type: LegacyDiffNote.name } } : {}
+      %tr.line_holder.parallel{ holder_data }
         - if left[:type] == 'match'
-          = render "projects/diffs/match_line_parallel", { line: left[:text],
+          = render "projects/diffs/match_line_parallel", { line: left[:text] }
-          line_old: left[:number], line_new: right[:number] }
         - elsif left[:type] == 'nonewline'
-          %td.old_line.diff-line-num
+          %td.old_line.diff-line-num.empty-cell
           %td.line_content.parallel.match= left[:text]
-          %td.new_line.diff-line-num
+          %td.new_line.diff-line-num.empty-cell
           %td.line_content.parallel.match= left[:text]
         - else
-          %td.old_line.diff-line-num{id: left[:line_code], class: "#{left[:type]} #{'empty-cell' if !left[:number]}"}
+          %td.old_line.diff-line-num{id: left[:line_code], class: "#{left[:type]} #{'empty-cell' if !left[:number]}", data: { linenumber: left[:number] }}
-            = link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code]
+            %a{href: "##{left[:line_code]}" }= raw(left[:number])
-            - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
+          %td.line_content{class: "parallel noteable_line #{left[:type]} #{'empty-cell' if left[:text].empty?}", data: { discussion_id: discussion_id(left[:line_code]), line_type: left[:type], line_code: left[:line_code] }}= diff_line_content(left[:text])
-              = link_to_new_diff_note(left[:line_code], 'old')
-          %td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]} #{'empty-cell' if left[:text].empty?}", data: { line_code: left[:line_code] }}= diff_line_content(left[:text])
 
           - if right[:type] == 'new'
             - new_line_class = 'new'
@@ -27,11 +25,9 @@
             - new_line_class = nil
             - new_line_code = left[:line_code]
 
-          %td.new_line.diff-line-num{id: new_line_code, class: "#{new_line_class} #{'empty-cell' if !right[:number]}", data: { linenumber: right[:number] }}
+          %td.new_line.diff-line-num{id: new_line_code, class: "#{new_line_class} #{'empty-cell' if !right[:number]}", data: { linenumber: right[:number] } }
-            = link_to raw(right[:number]), "##{new_line_code}", id: new_line_code
+            %a{href: "##{new_line_code}" }= raw(right[:number])
-            - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
+          %td.line_content.parallel{class: "noteable_line #{new_line_class} #{'empty-cell' if right[:text].empty?}", data: { discussion_id: discussion_id(new_line_code), line_type: new_line_class, line_code: new_line_code }}= diff_line_content(right[:text])
-              = link_to_new_diff_note(new_line_code, 'new')
-          %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code} #{'empty-cell' if right[:text].empty?}", data: { line_code: new_line_code }}= diff_line_content(right[:text])
 
       - unless @diff_notes_disabled
         - notes_left, notes_right = organize_comments(left, right)

features/project/commits/diff_comments.feature

@@ -5,10 +5,6 @@ Feature: Project Commits Diff Comments
     And I own project "Shop"
     And I visit project commit page
 
-  @javascript
-  Scenario: I can access add diff comment buttons
-    Then I should see add a diff comment button
-
   @javascript
   Scenario: I can comment on a commit diff
     Given I leave a diff comment like "Typo, please fix"

features/steps/shared/diff_note.rb

@@ -32,8 +32,8 @@ module SharedDiffNote
   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')
+    click_parallel_diff_line(sample_commit.del_line_code, 'old')
-    page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}-true']") do
+    page.within("#{diff_file_selector} form[id$='#{sample_commit.del_line_code}-true']") do
       fill_in "note[note]", with: "Old comment"
       find(".js-comment-button").trigger("click")
     end
@@ -165,10 +165,6 @@ module SharedDiffNote
     end
   end
 
-  step 'I should see add a diff comment button' do
-    expect(page).to have_css('.js-add-diff-note-button')
-  end
-
   step 'I should see an empty diff comment form' do
     page.within(diff_file_selector) do
       expect(page).to have_field("note[note]", with: "")
@@ -227,10 +223,12 @@ module SharedDiffNote
   end
 
   def click_diff_line(code)
-    find("button[data-line-code='#{code}']").trigger('click')
+    find(".line_holder[id='#{code}'] td:nth-of-type(1)").hover
+    find(".line_holder[id='#{code}'] button").trigger('click')
   end
 
   def click_parallel_diff_line(code, line_type)
-    find("button[data-line-code='#{code}'][data-line-type='#{line_type}']").trigger('click')
+    find(".line_content.parallel.#{line_type}[data-line-code='#{code}']").trigger('mouseover')
+    find(".line_holder.parallel button[data-line-code='#{code}']").trigger('click')
   end
 end

spec/features/notes_on_merge_requests_spec.rb

@@ -229,6 +229,7 @@ describe 'Comments', feature: true do
   end
 
   def click_diff_line(data = line_code)
-    execute_script("$('button[data-line-code=\"#{data}\"]').click()")
+    find(".line_holder[id='#{data}'] td:nth-of-type(1)").hover
+    find(".line_holder[id='#{data}'] button").trigger('click')
   end
 end