Merge branch 'fix-edit-notes-on-merge-request-diff' into 'master'

Fix editing notes on a merge request diff

Fixes #3910

See merge request !2041

5 files changed, 49 insertions, 8 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 750ee1016d8..b42aea9c3bb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -25,6 +25,7 @@ v 8.3.0 (unreleased)
   - Add languages page to graphs
   - Block LDAP user when they are no longer found in the LDAP server
   - Improve wording on project visibility levels (Zeger-Jan van de Weg)
+  - Fix editing notes on a merge request diff
   - Automatically select default clone protocol based on user preferences (Eirik Lygre)
   - Make Network page as sub tab of Commits
   - Add copy-to-clipboard button for Snippets
diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee
index b0eeb1db536..9e2dc1250c9 100644
--- a/app/assets/javascripts/merge_request_tabs.js.coffee
+++ b/app/assets/javascripts/merge_request_tabs.js.coffee
@@ -77,7 +77,7 @@ class @MergeRequestTabs
 
   scrollToElement: (container) ->
     if window.location.hash
-      $el = $("#{container} #{window.location.hash}")
+      $el = $("div#{container} #{window.location.hash}")
       $('body').scrollTo($el.offset().top) if $el.length
 
   # Activate a tab based on the current action
@@ -133,7 +133,7 @@ class @MergeRequestTabs
     @_get
       url: "#{source}.json"
       success: (data) =>
-        document.getElementById('commits').innerHTML = data.html
+        document.querySelector("div#commits").innerHTML = data.html
         $('.js-timeago').timeago()
         @commitsLoaded = true
         @scrollToElement("#commits")
@@ -144,7 +144,8 @@ class @MergeRequestTabs
     @_get
       url: "#{source}.json" + @_location.search
       success: (data) =>
-        document.getElementById('diffs').innerHTML = data.html
+        document.querySelector("div#diffs").innerHTML = data.html
+        $('div#diffs .js-syntax-highlight').syntaxHighlight()
         @diffsLoaded = true
         @scrollToElement("#diffs")
 
@@ -154,7 +155,7 @@ class @MergeRequestTabs
     @_get
       url: "#{source}.json"
       success: (data) =>
-        document.getElementById('builds').innerHTML = data.html
+        document.querySelector("div#builds").innerHTML = data.html
         $('.js-timeago').timeago()
         @buildsLoaded = true
         @scrollToElement("#builds")
diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee
index 533d00bfb0c..b1df56b24fe 100644
--- a/app/assets/javascripts/notes.js.coffee
+++ b/app/assets/javascripts/notes.js.coffee
@@ -148,6 +148,8 @@ class @Notes
     @note_ids.push(note.id)
     form = $("form[rel='" + note.discussion_id + "']")
     row = form.closest("tr")
+    note_html = $(note.html)
+    note_html.syntaxHighlight()
 
     # is this the first note of discussion?
     if row.is(".js-temp-notes-holder")
@@ -158,14 +160,16 @@ class @Notes
       row.next().find(".note").remove()
 
       # Add note to 'Changes' page discussions
-      $(".notes[rel='" + note.discussion_id + "']").append note.html
+      $(".notes[rel='" + note.discussion_id + "']").append note_html
 
       # Init discussion on 'Discussion' page if it is merge request page
       if $('body').attr('data-page').indexOf('projects:merge_request') == 0
-        $('ul.main-notes-list').append(note.discussion_with_diff_html)
+        discussion_html = $(note.discussion_with_diff_html)
+        discussion_html.syntaxHighlight()
+        $('ul.main-notes-list').append(discussion_html)
     else
       # append new note to all matching discussions
-      $(".notes[rel='" + note.discussion_id + "']").append note.html
+      $(".notes[rel='" + note.discussion_id + "']").append note_html
 
     # cleanup after successfully creating a diff/discussion note
     @removeDiscussionNoteForm(form)
@@ -286,7 +290,7 @@ class @Notes
     $html.find('.js-task-list-container').taskList('enable')
 
     # Find the note's `li` element by ID and replace it with the updated HTML
-    $note_li = $("#note_#{note.id}")
+    $note_li = $('.note-row-' + note.id)
     $note_li.replaceWith($html)
 
   ###
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index 35ace948888..7490ad400b5 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -85,6 +85,16 @@ Feature: Project Merge Requests
     Then I should see a discussion has started on diff
 
   @javascript
+  Scenario: I edit a comment on a merge request diff
+    Given project "Shop" have "Bug NS-05" open merge request with diffs inside
+    And I visit merge request page "Bug NS-05"
+    And I click on the Changes tab
+    And I leave a comment like "Line is wrong" on diff
+    And I change the comment "Line is wrong" to "Typo, please fix" on diff
+    Then I should not see a diff comment saying "Line is wrong"
+    And I should see a diff comment saying "Typo, please fix"
+
+  @javascript
   Scenario: I comment on a line of a commit in merge request
     Given project "Shop" have "Bug NS-05" open merge request with diffs inside
     And I visit merge request page "Bug NS-05"
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index 0107d9d8486..13d9d180dd1 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -186,6 +186,31 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     leave_comment "Line is wrong"
   end
 
+  step 'I change the comment "Line is wrong" to "Typo, please fix" on diff' do
+    page.within('.diff-file:nth-of-type(5) .note') do
+      find('.js-note-edit').click
+
+      page.within('.current-note-edit-form', visible: true) do
+        fill_in 'note_note', with: 'Typo, please fix'
+        click_button 'Save Comment'
+      end
+
+      expect(page).not_to have_button 'Save Comment', disabled: true, visible: true
+    end
+  end
+
+  step 'I should not see a diff comment saying "Line is wrong"' do
+    page.within('.diff-file:nth-of-type(5) .note') do
+      expect(page).not_to have_visible_content 'Line is wrong'
+    end
+  end
+
+  step 'I should see a diff comment saying "Typo, please fix"' do
+    page.within('.diff-file:nth-of-type(5) .note') do
+      expect(page).to have_visible_content 'Typo, please fix'
+    end
+  end
+
   step 'I should see a discussion has started on diff' do
     page.within(".notes .discussion") do
       page.should have_content "#{current_user.name} started a discussion"