diff options
author | Paul Slaughter <pslaughter@gitlab.com> | 2018-09-23 16:15:29 -0500 |
---|---|---|
committer | Paul Slaughter <pslaughter@gitlab.com> | 2018-09-23 16:46:24 -0500 |
commit | 26d88c210d7939e6725a206a73a1cf51029d8561 (patch) | |
tree | 70d310bcb5182430c7719ee3a6da3db892df5172 | |
parent | 39afa3c475fdb71531f14b2cb2c7b6a95a27ed2c (diff) | |
download | gitlab-ce-24128-fix-comment-unresolve-discussions.tar.gz |
Auto resolve new notes of resolved discussions24128-fix-comment-unresolve-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
-rw-r--r-- | app/services/notes/build_service.rb | 6 | ||||
-rw-r--r-- | spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb | 14 | ||||
-rw-r--r-- | spec/services/notes/build_service_spec.rb | 15 |
3 files changed, 35 insertions, 0 deletions
diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb index df5fe65de3c..201ec2e775b 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,11 +16,16 @@ 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/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 08d3afbba5a..9a0cfb468f2 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..18a83255676 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 |