Commit 40074568 authored by Douwe Maan's avatar Douwe Maan

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
parents 842ed384 370f0736
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
module Notes module Notes
class BuildService < ::BaseService class BuildService < ::BaseService
def execute def execute
should_resolve = false
in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id) in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
if in_reply_to_discussion_id.present? if in_reply_to_discussion_id.present?
...@@ -15,12 +16,17 @@ module Notes ...@@ -15,12 +16,17 @@ module Notes
end end
params.merge!(discussion.reply_attributes) params.merge!(discussion.reply_attributes)
should_resolve = discussion.resolved?
end end
note = Note.new(params) note = Note.new(params)
note.project = project note.project = project
note.author = current_user note.author = current_user
if should_resolve
note.resolve_without_save(current_user)
end
note note
end end
......
---
title: Fix resolved discussions being unresolved when commented on
merge_request: 21881
author:
type: fixed
...@@ -139,29 +139,35 @@ describe 'Merge request > User resolves diff notes and discussions', :js do ...@@ -139,29 +139,35 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
expect(find('.diffs .diff-file .notes_holder')).to be_visible expect(find('.diffs .diff-file .notes_holder')).to be_visible
end end
end end
end
it 'allows user to resolve from reply form without a comment' do describe 'reply form' do
before do
click_button 'Toggle discussion'
page.within '.diff-content' do page.within '.diff-content' do
click_button 'Reply...' click_button 'Reply...'
end
end
click_button 'Resolve discussion' it 'allows user to comment' do
page.within '.diff-content' do
find('.js-note-text').set 'testing'
click_button 'Comment'
wait_for_requests
end end
page.within '.line-resolve-all-container' do page.within '.line-resolve-all-container' do
expect(page).to have_content('1/1 discussion resolved') expect(page).to have_content('1/1 discussion resolved')
expect(page).to have_selector('.line-resolve-btn.is-active')
end end
end end
it 'allows user to unresolve from reply form without a comment' do it 'allows user to unresolve from reply form without a comment' do
page.within '.diff-content' do page.within '.diff-content' do
click_button 'Resolve discussion'
sleep 1
click_button 'Reply...'
click_button 'Unresolve discussion' click_button 'Unresolve discussion'
wait_for_requests
end end
page.within '.line-resolve-all-container' do page.within '.line-resolve-all-container' do
...@@ -170,34 +176,47 @@ describe 'Merge request > User resolves diff notes and discussions', :js do ...@@ -170,34 +176,47 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
end end
end end
it 'allows user to comment & resolve discussion' do it 'allows user to comment & unresolve discussion' do
page.within '.diff-content' do page.within '.diff-content' do
click_button 'Reply...'
find('.js-note-text').set 'testing' find('.js-note-text').set 'testing'
click_button 'Comment & resolve discussion' click_button 'Comment & unresolve discussion'
wait_for_requests
end end
page.within '.line-resolve-all-container' do page.within '.line-resolve-all-container' do
expect(page).to have_content('1/1 discussion resolved') expect(page).to have_content('0/1 discussion resolved')
expect(page).to have_selector('.line-resolve-btn.is-active') end
end
end end
end end
it 'allows user to comment & unresolve discussion' do it 'allows user to resolve from reply form without a comment' do
page.within '.diff-content' do page.within '.diff-content' do
click_button 'Reply...'
click_button 'Resolve discussion' click_button 'Resolve discussion'
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 & resolve discussion' do
page.within '.diff-content' do
click_button 'Reply...' click_button 'Reply...'
find('.js-note-text').set 'testing' find('.js-note-text').set 'testing'
click_button 'Comment & unresolve discussion' click_button 'Comment & resolve discussion'
end end
page.within '.line-resolve-all-container' do 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
end end
......
...@@ -4,6 +4,8 @@ describe Notes::BuildService do ...@@ -4,6 +4,8 @@ describe Notes::BuildService do
let(:note) { create(:discussion_note_on_issue) } let(:note) { create(:discussion_note_on_issue) }
let(:project) { note.project } let(:project) { note.project }
let(:author) { note.author } 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 describe '#execute' do
context 'when in_reply_to_discussion_id is specified' do context 'when in_reply_to_discussion_id is specified' do
...@@ -12,6 +14,19 @@ describe Notes::BuildService 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 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).to be_valid
expect(new_note.in_reply_to?(note)).to be_truthy 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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment