summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPaul Slaughter <pslaughter@gitlab.com>2018-09-23 16:15:29 -0500
committerPaul Slaughter <pslaughter@gitlab.com>2018-09-24 08:30:58 -0500
commit370f07361c6d0fb1565003ae9427f69ad6004009 (patch)
tree6427e055c38f72c753cada853f40a6879b431836
parentce27f74afa4ff16eccf353b5ce1bf430c6eb64b3 (diff)
downloadgitlab-ce-370f07361c6d0fb1565003ae9427f69ad6004009.tar.gz
Auto resolve new notes of resolved discussions
**Why?** The previous behavior had resolved discussions being unresolved when commented on. This was strange UX, especially since there is a separate button for "Comment & unresolve discussion". https://gitlab.com/gitlab-org/gitlab-ce/issues/24128 **Note:** - Also adds changelog
-rw-r--r--app/services/notes/build_service.rb6
-rw-r--r--changelogs/unreleased/24128-fix-comment-unresolve-discussions.yml5
-rw-r--r--spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb14
-rw-r--r--spec/services/notes/build_service_spec.rb15
4 files changed, 40 insertions, 0 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 8f595a2b095..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
@@ -149,6 +149,20 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
end
end
+ it 'allows user to comment' do
+ page.within '.diff-content' do
+ find('.js-note-text').set 'testing'
+
+ click_button 'Comment'
+
+ wait_for_requests
+ end
+
+ page.within '.line-resolve-all-container' do
+ expect(page).to have_content('1/1 discussion resolved')
+ end
+ end
+
it 'allows user to unresolve from reply form without a comment' do
page.within '.diff-content' do
click_button 'Unresolve discussion'
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