From 01c9488f4a559063eba77074ba2d369de87b8018 Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Thu, 30 Mar 2017 10:39:06 +0900 Subject: Added slash command to close an issue as a duplicate. Closes #26372 --- .../issues/user_uses_slash_commands_spec.rb | 41 ++++++++++++++++ spec/services/issues/update_service_spec.rb | 56 ++++++++++++++++++++++ .../quick_actions/interpret_service_spec.rb | 36 ++++++++++++++ spec/services/system_note_service_spec.rb | 25 ++++++++++ 4 files changed, 158 insertions(+) (limited to 'spec') diff --git a/spec/features/issues/user_uses_slash_commands_spec.rb b/spec/features/issues/user_uses_slash_commands_spec.rb index 1cd1f016674..d5de060b033 100644 --- a/spec/features/issues/user_uses_slash_commands_spec.rb +++ b/spec/features/issues/user_uses_slash_commands_spec.rb @@ -134,5 +134,46 @@ feature 'Issues > User uses quick actions', feature: true, js: true do expect(page).not_to have_content '/wip' end end + + describe 'mark issue as duplicate' do + let(:issue) { create(:issue, project: project) } + let(:original_issue) { create(:issue, project: project) } + + context 'when the current user can update issues' do + it 'does not create a note, and marks the issue as a duplicate' do + write_note("/duplicate ##{original_issue.to_reference}") + + expect(page).not_to have_content "/duplicate #{original_issue.to_reference}" + expect(page).to have_content 'Commands applied' + expect(page).to have_content "marked this issue as a duplicate of #{original_issue.to_reference}" + + issue.reload + + expect(issue.closed?).to be_truthy + end + end + + context 'when the current user cannot update the issue' do + let(:guest) { create(:user) } + before do + project.team << [guest, :guest] + logout + login_with(guest) + visit namespace_project_issue_path(project.namespace, project, issue) + end + + it 'does not create a note, and does not mark the issue as a duplicate' do + write_note("/duplicate ##{original_issue.to_reference}") + + expect(page).to have_content "/duplicate ##{original_issue.to_reference}" + expect(page).not_to have_content 'Commands applied' + expect(page).not_to have_content "marked this issue as a duplicate of #{original_issue.to_reference}" + + issue.reload + + expect(issue.closed?).to be_falsey + end + end + end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index d0b991f19ab..3e7abf85106 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -491,6 +491,62 @@ describe Issues::UpdateService, services: true do include_examples 'updating mentions', Issues::UpdateService end + context 'duplicate issue' do + let(:issues_finder) { spy(:issues_finder) } + let(:close_service) { spy(:close_service) } + + before do + allow(IssuesFinder).to receive(:new).and_return(issues_finder) + allow(Issues::CloseService).to receive(:new).and_return(close_service) + allow(SystemNoteService).to receive(:cross_reference) + allow(SystemNoteService).to receive(:mark_duplicate_issue) + end + + context 'invalid original_issue_id' do + let(:original_issue_id) { double } + before { update_issue({ original_issue_id: original_issue_id }) } + + it 'finds the root issue' do + expect(issues_finder).to have_received(:find).with(original_issue_id) + end + + it 'does not close the issue' do + expect(close_service).not_to have_received(:execute) + end + + it 'does not create system notes' do + expect(SystemNoteService).not_to have_received(:cross_reference) + expect(SystemNoteService).not_to have_received(:mark_duplicate_issue) + end + end + + context 'valid original_issue_id' do + let(:original_issue) { create(:issue, project: project) } + let(:original_issue_id) { double } + + before do + allow(issues_finder).to receive(:find).and_return(original_issue) + update_issue({ original_issue_id: original_issue_id }) + end + + it 'finds the root issue' do + expect(issues_finder).to have_received(:find).with(original_issue_id) + end + + it 'closes the issue' do + expect(close_service).to have_received(:execute).with(issue) + end + + it 'creates a system note that this issue is a duplicate' do + expect(SystemNoteService).to have_received(:mark_duplicate_issue).with(issue, project, user, original_issue) + end + + it 'creates a cross reference system note in the other issue' do + expect(SystemNoteService).to have_received(:cross_reference).with(original_issue, issue, user) + end + end + end + include_examples 'issuable update service' do let(:open_issuable) { issue } let(:closed_issuable) { create(:closed_issue, project: project) } diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index a2db3f68ff7..3e4aa66756c 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -261,6 +261,17 @@ describe QuickActions::InterpretService, services: true do end end + shared_examples 'duplicate command' do + let(:issue_duplicate) { create(:issue, project: project) } + + it 'fetches issue and populates original_issue_id if content contains /duplicate issue_reference' do + issue_duplicate # populate the issue + _, updates = service.execute(content, issuable) + + expect(updates).to eq(original_issue_id: issue_duplicate.id) + end + end + it_behaves_like 'reopen command' do let(:content) { '/reopen' } let(:issuable) { issue } @@ -644,6 +655,26 @@ describe QuickActions::InterpretService, services: true do let(:issuable) { issue } end + it_behaves_like 'duplicate command' do + let(:content) { "/duplicate #{issue_duplicate.to_reference}" } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/duplicate #{issue.to_reference}' } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/duplicate' } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/duplicate imaginary#1234' } + let(:issuable) { issue } + end + context 'when current_user cannot :admin_issue' do let(:visitor) { create(:user) } let(:issue) { create(:issue, project: project, author: visitor) } @@ -693,6 +724,11 @@ describe QuickActions::InterpretService, services: true do let(:content) { '/remove_due_date' } let(:issuable) { issue } end + + it_behaves_like 'empty command' do + let(:content) { '/duplicate #{issue.to_reference}' } + let(:issuable) { issue } + end end context '/award command' do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 60477b8e9ba..db120889119 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -1101,4 +1101,29 @@ describe SystemNoteService, services: true do expect(subject.note).to include(diffs_project_merge_request_url(project, merge_request, diff_id: diff_id, anchor: line_code)) end end + + describe '.mark_duplicate_issue' do + subject { described_class.mark_duplicate_issue(noteable, project, author, original_issue) } + + context 'within the same project' do + let(:original_issue) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked this issue as a duplicate of #{original_issue.to_reference}" } + end + + context 'across different projects' do + let(:other_project) { create(:empty_project) } + let(:original_issue) { create(:issue, project: other_project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked this issue as a duplicate of #{original_issue.to_reference(project)}" } + end + end end -- cgit v1.2.3 From 7e3d34595c3e090fe505b4fbd49cde2a303b1b6f Mon Sep 17 00:00:00 2001 From: Ryan Scott Date: Wed, 5 Apr 2017 11:31:48 +0900 Subject: Changes based on MR feedback. Marking an issue as a duplicate will now also add an upvote on behalf of the author on the original issue. --- .../issues/user_uses_slash_commands_spec.rb | 8 +--- spec/services/issues/update_service_spec.rb | 48 +++++++------------- .../quick_actions/interpret_service_spec.rb | 52 +++++++++++++++------- 3 files changed, 54 insertions(+), 54 deletions(-) (limited to 'spec') diff --git a/spec/features/issues/user_uses_slash_commands_spec.rb b/spec/features/issues/user_uses_slash_commands_spec.rb index d5de060b033..28f27c76e35 100644 --- a/spec/features/issues/user_uses_slash_commands_spec.rb +++ b/spec/features/issues/user_uses_slash_commands_spec.rb @@ -147,9 +147,7 @@ feature 'Issues > User uses quick actions', feature: true, js: true do expect(page).to have_content 'Commands applied' expect(page).to have_content "marked this issue as a duplicate of #{original_issue.to_reference}" - issue.reload - - expect(issue.closed?).to be_truthy + expect(issue.reload).to be_closed end end @@ -169,9 +167,7 @@ feature 'Issues > User uses quick actions', feature: true, js: true do expect(page).not_to have_content 'Commands applied' expect(page).not_to have_content "marked this issue as a duplicate of #{original_issue.to_reference}" - issue.reload - - expect(issue.closed?).to be_falsey + expect(issue.reload).to be_open end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 3e7abf85106..e7f3ab93395 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -492,57 +492,43 @@ describe Issues::UpdateService, services: true do end context 'duplicate issue' do - let(:issues_finder) { spy(:issues_finder) } - let(:close_service) { spy(:close_service) } - - before do - allow(IssuesFinder).to receive(:new).and_return(issues_finder) - allow(Issues::CloseService).to receive(:new).and_return(close_service) - allow(SystemNoteService).to receive(:cross_reference) - allow(SystemNoteService).to receive(:mark_duplicate_issue) - end + let(:original_issue) { create(:issue, project: project) } context 'invalid original_issue_id' do - let(:original_issue_id) { double } - before { update_issue({ original_issue_id: original_issue_id }) } - - it 'finds the root issue' do - expect(issues_finder).to have_received(:find).with(original_issue_id) + before do + update_issue(original_issue_id: 123456789) end it 'does not close the issue' do - expect(close_service).not_to have_received(:execute) + expect(issue.reload).not_to be_closed end - it 'does not create system notes' do - expect(SystemNoteService).not_to have_received(:cross_reference) - expect(SystemNoteService).not_to have_received(:mark_duplicate_issue) + it 'does not create a system note' do + note = find_note("marked this issue as a duplicate of #{original_issue.to_reference}") + expect(note).to be_nil + end + + it 'does not upvote the issue on behalf of the author' do + expect(original_issue).not_to be_awarded_emoji(AwardEmoji::UPVOTE_NAME, issue.author) end end context 'valid original_issue_id' do - let(:original_issue) { create(:issue, project: project) } - let(:original_issue_id) { double } - before do - allow(issues_finder).to receive(:find).and_return(original_issue) - update_issue({ original_issue_id: original_issue_id }) - end - - it 'finds the root issue' do - expect(issues_finder).to have_received(:find).with(original_issue_id) + update_issue(original_issue_id: original_issue.id) end it 'closes the issue' do - expect(close_service).to have_received(:execute).with(issue) + expect(issue.reload).to be_closed end it 'creates a system note that this issue is a duplicate' do - expect(SystemNoteService).to have_received(:mark_duplicate_issue).with(issue, project, user, original_issue) + note = find_note("marked this issue as a duplicate of #{original_issue.to_reference}") + expect(note).not_to be_nil end - it 'creates a cross reference system note in the other issue' do - expect(SystemNoteService).to have_received(:cross_reference).with(original_issue, issue, user) + it 'upvotes the issue on behalf of the author' do + expect(original_issue).to be_awarded_emoji(AwardEmoji::UPVOTE_NAME, issue.author) end end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 3e4aa66756c..1d60b74e566 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -262,8 +262,6 @@ describe QuickActions::InterpretService, services: true do end shared_examples 'duplicate command' do - let(:issue_duplicate) { create(:issue, project: project) } - it 'fetches issue and populates original_issue_id if content contains /duplicate issue_reference' do issue_duplicate # populate the issue _, updates = service.execute(content, issuable) @@ -655,24 +653,44 @@ describe QuickActions::InterpretService, services: true do let(:issuable) { issue } end - it_behaves_like 'duplicate command' do - let(:content) { "/duplicate #{issue_duplicate.to_reference}" } - let(:issuable) { issue } - end + context '/duplicate command' do + it_behaves_like 'duplicate command' do + let(:issue_duplicate) { create(:issue, project: project) } + let(:content) { "/duplicate #{issue_duplicate.to_reference}" } + let(:issuable) { issue } + end - it_behaves_like 'empty command' do - let(:content) { '/duplicate #{issue.to_reference}' } - let(:issuable) { issue } - end + it_behaves_like 'empty command' do + let(:content) { "/duplicate #{issue.to_reference}" } + let(:issuable) { issue } + end - it_behaves_like 'empty command' do - let(:content) { '/duplicate' } - let(:issuable) { issue } - end + it_behaves_like 'empty command' do + let(:content) { '/duplicate' } + let(:issuable) { issue } + end - it_behaves_like 'empty command' do - let(:content) { '/duplicate imaginary#1234' } - let(:issuable) { issue } + context 'cross project references' do + it_behaves_like 'duplicate command' do + let(:other_project) { create(:empty_project, :public) } + let(:issue_duplicate) { create(:issue, project: other_project) } + let(:content) { "/duplicate #{issue_duplicate.to_reference(project)}" } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/duplicate imaginary#1234' } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:other_project) { create(:empty_project, :private) } + let(:issue_duplicate) { create(:issue, project: other_project) } + + let(:content) { "/duplicate #{issue_duplicate.to_reference(project)}" } + let(:issuable) { issue } + end + end end context 'when current_user cannot :admin_issue' do -- cgit v1.2.3 From 3498e825d08adb0311d0431d9d15e450f95bfc86 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 18 Jul 2017 15:27:00 +0100 Subject: Fix feature specs --- spec/features/issues/user_uses_slash_commands_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'spec') diff --git a/spec/features/issues/user_uses_slash_commands_spec.rb b/spec/features/issues/user_uses_slash_commands_spec.rb index 28f27c76e35..60b787fdd61 100644 --- a/spec/features/issues/user_uses_slash_commands_spec.rb +++ b/spec/features/issues/user_uses_slash_commands_spec.rb @@ -155,9 +155,9 @@ feature 'Issues > User uses quick actions', feature: true, js: true do let(:guest) { create(:user) } before do project.team << [guest, :guest] - logout - login_with(guest) - visit namespace_project_issue_path(project.namespace, project, issue) + gitlab_sign_out + sign_in(guest) + visit project_issue_path(project, issue) end it 'does not create a note, and does not mark the issue as a duplicate' do -- cgit v1.2.3 From 1df696f5a6836e03a6bf8d5139c2c7ce6d96e727 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 20 Jul 2017 15:42:33 +0100 Subject: Move duplicate issue management to a service --- spec/services/issues/duplicate_service_spec.rb | 80 ++++++++++++++++++++++ spec/services/issues/update_service_spec.rb | 41 +++-------- .../quick_actions/interpret_service_spec.rb | 11 +-- spec/services/system_note_service_spec.rb | 35 ++++++++-- 4 files changed, 123 insertions(+), 44 deletions(-) create mode 100644 spec/services/issues/duplicate_service_spec.rb (limited to 'spec') diff --git a/spec/services/issues/duplicate_service_spec.rb b/spec/services/issues/duplicate_service_spec.rb new file mode 100644 index 00000000000..82daf53b173 --- /dev/null +++ b/spec/services/issues/duplicate_service_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +describe Issues::DuplicateService, services: true do + let(:user) { create(:user) } + let(:canonical_project) { create(:empty_project) } + let(:duplicate_project) { create(:empty_project) } + + let(:canonical_issue) { create(:issue, project: canonical_project) } + let(:duplicate_issue) { create(:issue, project: duplicate_project) } + + subject { described_class.new(duplicate_project, user, {}) } + + describe '#execute' do + context 'when the issues passed are the same' do + it 'does nothing' do + expect(subject).not_to receive(:close_service) + expect(SystemNoteService).not_to receive(:mark_duplicate_issue) + expect(SystemNoteService).not_to receive(:mark_canonical_issue_of_duplicate) + + subject.execute(duplicate_issue, duplicate_issue) + end + end + + context 'when the user cannot update the duplicate issue' do + before do + canonical_project.add_reporter(user) + end + + it 'does nothing' do + expect(subject).not_to receive(:close_service) + expect(SystemNoteService).not_to receive(:mark_duplicate_issue) + expect(SystemNoteService).not_to receive(:mark_canonical_issue_of_duplicate) + + subject.execute(duplicate_issue, canonical_issue) + end + end + + context 'when the user cannot comment on the canonical issue' do + before do + duplicate_project.add_reporter(user) + end + + it 'does nothing' do + expect(subject).not_to receive(:close_service) + expect(SystemNoteService).not_to receive(:mark_duplicate_issue) + expect(SystemNoteService).not_to receive(:mark_canonical_issue_of_duplicate) + + subject.execute(duplicate_issue, canonical_issue) + end + end + + context 'when the user can mark the issue as a duplicate' do + before do + canonical_project.add_reporter(user) + duplicate_project.add_reporter(user) + end + + it 'closes the duplicate issue' do + subject.execute(duplicate_issue, canonical_issue) + + expect(duplicate_issue.reload).to be_closed + expect(canonical_issue.reload).to be_open + end + + it 'adds a system note to the duplicate issue' do + expect(SystemNoteService) + .to receive(:mark_duplicate_issue).with(duplicate_issue, duplicate_project, user, canonical_issue) + + subject.execute(duplicate_issue, canonical_issue) + end + + it 'adds a system note to the canonical issue' do + expect(SystemNoteService) + .to receive(:mark_canonical_issue_of_duplicate).with(canonical_issue, canonical_project, user, duplicate_issue) + + subject.execute(duplicate_issue, canonical_issue) + end + end + end +end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index e7f3ab93395..064be940a1c 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -492,43 +492,22 @@ describe Issues::UpdateService, services: true do end context 'duplicate issue' do - let(:original_issue) { create(:issue, project: project) } + let(:canonical_issue) { create(:issue, project: project) } - context 'invalid original_issue_id' do - before do - update_issue(original_issue_id: 123456789) - end - - it 'does not close the issue' do - expect(issue.reload).not_to be_closed - end + context 'invalid canonical_issue_id' do + it 'does not call the duplicate service' do + expect(Issues::DuplicateService).not_to receive(:new) - it 'does not create a system note' do - note = find_note("marked this issue as a duplicate of #{original_issue.to_reference}") - expect(note).to be_nil - end - - it 'does not upvote the issue on behalf of the author' do - expect(original_issue).not_to be_awarded_emoji(AwardEmoji::UPVOTE_NAME, issue.author) + update_issue(canonical_issue_id: 123456789) end end - context 'valid original_issue_id' do - before do - update_issue(original_issue_id: original_issue.id) - end - - it 'closes the issue' do - expect(issue.reload).to be_closed - end - - it 'creates a system note that this issue is a duplicate' do - note = find_note("marked this issue as a duplicate of #{original_issue.to_reference}") - expect(note).not_to be_nil - end + context 'valid canonical_issue_id' do + it 'calls the duplicate service with both issues' do + expect_any_instance_of(Issues::DuplicateService) + .to receive(:execute).with(issue, canonical_issue) - it 'upvotes the issue on behalf of the author' do - expect(original_issue).to be_awarded_emoji(AwardEmoji::UPVOTE_NAME, issue.author) + update_issue(canonical_issue_id: canonical_issue.id) end end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 1d60b74e566..2a2a5c38e4b 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -262,11 +262,11 @@ describe QuickActions::InterpretService, services: true do end shared_examples 'duplicate command' do - it 'fetches issue and populates original_issue_id if content contains /duplicate issue_reference' do + it 'fetches issue and populates canonical_issue_id if content contains /duplicate issue_reference' do issue_duplicate # populate the issue _, updates = service.execute(content, issuable) - expect(updates).to eq(original_issue_id: issue_duplicate.id) + expect(updates).to eq(canonical_issue_id: issue_duplicate.id) end end @@ -660,11 +660,6 @@ describe QuickActions::InterpretService, services: true do let(:issuable) { issue } end - it_behaves_like 'empty command' do - let(:content) { "/duplicate #{issue.to_reference}" } - let(:issuable) { issue } - end - it_behaves_like 'empty command' do let(:content) { '/duplicate' } let(:issuable) { issue } @@ -679,7 +674,7 @@ describe QuickActions::InterpretService, services: true do end it_behaves_like 'empty command' do - let(:content) { '/duplicate imaginary#1234' } + let(:content) { "/duplicate imaginary#1234" } let(:issuable) { issue } end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index db120889119..681b419aedf 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -1103,27 +1103,52 @@ describe SystemNoteService, services: true do end describe '.mark_duplicate_issue' do - subject { described_class.mark_duplicate_issue(noteable, project, author, original_issue) } + subject { described_class.mark_duplicate_issue(noteable, project, author, canonical_issue) } context 'within the same project' do - let(:original_issue) { create(:issue, project: project) } + let(:canonical_issue) { create(:issue, project: project) } it_behaves_like 'a system note' do let(:action) { 'duplicate' } end - it { expect(subject.note).to eq "marked this issue as a duplicate of #{original_issue.to_reference}" } + it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference}" } end context 'across different projects' do let(:other_project) { create(:empty_project) } - let(:original_issue) { create(:issue, project: other_project) } + let(:canonical_issue) { create(:issue, project: other_project) } it_behaves_like 'a system note' do let(:action) { 'duplicate' } end - it { expect(subject.note).to eq "marked this issue as a duplicate of #{original_issue.to_reference(project)}" } + it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference(project)}" } + end + end + + describe '.mark_canonical_issue_of_duplicate' do + subject { described_class.mark_canonical_issue_of_duplicate(noteable, project, author, duplicate_issue) } + + context 'within the same project' do + let(:duplicate_issue) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference} as a duplicate of this issue" } + end + + context 'across different projects' do + let(:other_project) { create(:empty_project) } + let(:duplicate_issue) { create(:issue, project: other_project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference(project)} as a duplicate of this issue" } end end end -- cgit v1.2.3