Merge branch '24128-fix-comment-unresolve-discussions' into 'master'

Resolve "Resolved discussions automatically get unresolved when commented on"

Closes #24128

See merge request gitlab-org/gitlab-ce!21881

4 files changed, 75 insertions, 30 deletions

diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb
index df5fe65de3c..7b92fe6fe14 100644
--- a/app/services/notes/build_service.rb
+++ b/app/services/notes/build_service.rb
@@ -3,6 +3,7 @@
 module Notes
   class BuildService < ::BaseService
     def execute
+      should_resolve = false
       in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
 
       if in_reply_to_discussion_id.present?
@@ -15,12 +16,17 @@ module Notes
         end
 
         params.merge!(discussion.reply_attributes)
+        should_resolve = discussion.resolved?
       end
 
       note = Note.new(params)
       note.project = project
       note.author = current_user
 
+      if should_resolve
+        note.resolve_without_save(current_user)
+      end
+
       note
     end
 
diff --git a/changelogs/unreleased/24128-fix-comment-unresolve-discussions.yml b/changelogs/unreleased/24128-fix-comment-unresolve-discussions.yml
new file mode 100644
index 00000000000..d835d25f39b
--- /dev/null
+++ b/changelogs/unreleased/24128-fix-comment-unresolve-discussions.yml
@@ -0,0 +1,5 @@
+---
+title: Fix resolved discussions being unresolved when commented on
+merge_request: 21881
+author:
+type: fixed
diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
index 2d268ecab58..8a16c011067 100644
--- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
+++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
@@ -139,44 +139,64 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
             expect(find('.diffs .diff-file .notes_holder')).to be_visible
           end
         end
-      end
 
-      it 'allows user to resolve from reply form without a comment' do
-        page.within '.diff-content' do
-          click_button 'Reply...'
+        describe 'reply form' do
+          before do
+            click_button 'Toggle discussion'
 
-          click_button 'Resolve discussion'
-        end
+            page.within '.diff-content' do
+              click_button 'Reply...'
+            end
+          end
 
-        page.within '.line-resolve-all-container' do
-          expect(page).to have_content('1/1 discussion resolved')
-          expect(page).to have_selector('.line-resolve-btn.is-active')
-        end
-      end
+          it 'allows user to comment' do
+            page.within '.diff-content' do
+              find('.js-note-text').set 'testing'
 
-      it 'allows user to unresolve from reply form without a comment' do
-        page.within '.diff-content' do
-          click_button 'Resolve discussion'
-          sleep 1
+              click_button 'Comment'
 
-          click_button 'Reply...'
+              wait_for_requests
+            end
 
-          click_button 'Unresolve discussion'
-        end
+            page.within '.line-resolve-all-container' do
+              expect(page).to have_content('1/1 discussion resolved')
+            end
+          end
 
-        page.within '.line-resolve-all-container' do
-          expect(page).to have_content('0/1 discussion resolved')
-          expect(page).not_to have_selector('.line-resolve-btn.is-active')
+          it 'allows user to unresolve from reply form without a comment' do
+            page.within '.diff-content' do
+              click_button 'Unresolve discussion'
+
+              wait_for_requests
+            end
+
+            page.within '.line-resolve-all-container' do
+              expect(page).to have_content('0/1 discussion resolved')
+              expect(page).not_to have_selector('.line-resolve-btn.is-active')
+            end
+          end
+
+          it 'allows user to comment & unresolve discussion' do
+            page.within '.diff-content' do
+              find('.js-note-text').set 'testing'
+
+              click_button 'Comment & unresolve discussion'
+
+              wait_for_requests
+            end
+
+            page.within '.line-resolve-all-container' do
+              expect(page).to have_content('0/1 discussion resolved')
+            end
+          end
         end
       end
 
-      it 'allows user to comment & resolve discussion' do
+      it 'allows user to resolve from reply form without a comment' do
         page.within '.diff-content' do
           click_button 'Reply...'
 
-          find('.js-note-text').set 'testing'
-
-          click_button 'Comment & resolve discussion'
+          click_button 'Resolve discussion'
         end
 
         page.within '.line-resolve-all-container' do
@@ -185,19 +205,18 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
         end
       end
 
-      it 'allows user to comment & unresolve discussion' do
+      it 'allows user to comment & resolve discussion' do
         page.within '.diff-content' do
-          click_button 'Resolve discussion'
-
           click_button 'Reply...'
 
           find('.js-note-text').set 'testing'
 
-          click_button 'Comment & unresolve discussion'
+          click_button 'Comment & resolve discussion'
         end
 
         page.within '.line-resolve-all-container' do
-          expect(page).to have_content('0/1 discussion resolved')
+          expect(page).to have_content('1/1 discussion resolved')
+          expect(page).to have_selector('.line-resolve-btn.is-active')
         end
       end
 
diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb
index 6e1c1fe6c02..ff85c261cd4 100644
--- a/spec/services/notes/build_service_spec.rb
+++ b/spec/services/notes/build_service_spec.rb
@@ -4,6 +4,8 @@ describe Notes::BuildService do
   let(:note) { create(:discussion_note_on_issue) }
   let(:project) { note.project }
   let(:author) { note.author }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+  let(:mr_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, author: author) }
 
   describe '#execute' do
     context 'when in_reply_to_discussion_id is specified' do
@@ -12,6 +14,19 @@ describe Notes::BuildService do
           new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
           expect(new_note).to be_valid
           expect(new_note.in_reply_to?(note)).to be_truthy
+          expect(new_note.resolved?).to be_falsey
+        end
+
+        context 'when discussion is resolved' do
+          before do
+            mr_note.resolve!(author)
+          end
+
+          it 'resolves the note' do
+            new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: mr_note.discussion_id).execute
+            expect(new_note).to be_valid
+            expect(new_note.resolved?).to be_truthy
+          end
         end
       end