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

Fix delete notes on merge request diff

Fixes #3971

See merge request !2061

4 files changed, 46 insertions, 8 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 61068d3efb1..4c8d8dcd4df 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -45,6 +45,7 @@ v 8.3.0 (unreleased)
   - Accept COPYING,COPYING.lesser, and licence as license file (Zeger-Jan van de Weg)
   - Fix emoji aliases problem
   - Fix award-emojis Flash alert's width
+  - Fix deleting notes on a merge request diff
 
 v 8.2.3
   - Fix application settings cache not expiring after changes (Stan Hu)
diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee
index a7d720fff4b..35dc7829da2 100644
--- a/app/assets/javascripts/notes.js.coffee
+++ b/app/assets/javascripts/notes.js.coffee
@@ -350,18 +350,26 @@ class @Notes
   ###
   removeNote: ->
     note = $(this).closest(".note")
-    notes = note.closest(".notes")
+    note_id = note.attr('id')
 
-    # check if this is the last note for this line
-    if notes.find(".note").length is 1
+    $('.note[id="' + note_id + '"]').each ->
+      note = $(this)
+      notes = note.closest(".notes")
+      count = notes.closest(".notes_holder").find(".discussion-notes-count")
 
-      # for discussions
-      notes.closest(".discussion").remove()
+      # check if this is the last note for this line
+      if notes.find(".note").length is 1
 
-      # for diff lines
-      notes.closest("tr").remove()
+        # for discussions
+        notes.closest(".discussion").remove()
 
-    note.remove()
+        # for diff lines
+        notes.closest("tr").remove()
+      else
+        # update notes count
+        count.get(0).lastChild.nodeValue = " #{notes.children().length - 1}"
+
+      note.remove()
 
   ###
   Called in response to clicking the delete attachment link
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index 7490ad400b5..aa9078b878f 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -95,6 +95,16 @@ Feature: Project Merge Requests
     And I should see a diff comment saying "Typo, please fix"
 
   @javascript
+  Scenario: I delete 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 delete the comment "Line is wrong" on diff
+    And I click on the Discussion tab
+    Then I should not see any discussion
+
+  @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 13d9d180dd1..f0cf4818d0c 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -211,6 +211,25 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     end
   end
 
+  step 'I delete the comment "Line is wrong" on diff' do
+    page.within('.diff-file:nth-of-type(5) .note') do
+      find('.js-note-delete').click
+    end
+  end
+
+  step 'I click on the Discussion tab' do
+    page.within '.merge-request-tabs' do
+      click_link 'Discussion'
+    end
+
+    # Waits for load
+    expect(page).to have_css('.tab-content #notes.active')
+  end
+
+  step 'I should not see any discussion' do
+    expect(page).not_to have_css('.notes .discussion')
+  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"