Commit 7e3d3459 authored by Ryan Scott's avatar Ryan Scott Committed by Sean McGivern

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.
parent 01c9488f
class SystemNoteMetadata < ActiveRecord::Base class SystemNoteMetadata < ActiveRecord::Base
ICON_TYPES = %w[ ICON_TYPES = %w[
commit description merge confidential visible label assignee cross_reference commit description merge confidential visible label assignee cross_reference
title time_tracking branch milestone discussion task moved opened closed merged title time_tracking branch milestone discussion task moved
outdated duplicate opened closed merged duplicate
outdated
].freeze ].freeze
validates :note, presence: true validates :note, presence: true
......
...@@ -50,10 +50,6 @@ class IssuableBaseService < BaseService ...@@ -50,10 +50,6 @@ class IssuableBaseService < BaseService
SystemNoteService.mark_duplicate_issue(issuable, issuable.project, current_user, original_issue) SystemNoteService.mark_duplicate_issue(issuable, issuable.project, current_user, original_issue)
end end
def create_cross_reference_note(noteable, mentioner)
SystemNoteService.cross_reference(noteable, mentioner, current_user)
end
def filter_params(issuable) def filter_params(issuable)
ability_name = :"admin_#{issuable.to_ability_name}" ability_name = :"admin_#{issuable.to_ability_name}"
...@@ -303,14 +299,18 @@ class IssuableBaseService < BaseService ...@@ -303,14 +299,18 @@ class IssuableBaseService < BaseService
def change_issue_duplicate(issuable) def change_issue_duplicate(issuable)
original_issue_id = params.delete(:original_issue_id) original_issue_id = params.delete(:original_issue_id)
return if original_issue_id.nil? return unless original_issue_id
begin
original_issue = IssuesFinder.new(current_user).find(original_issue_id) original_issue = IssuesFinder.new(current_user).find(original_issue_id)
if original_issue.present? rescue ActiveRecord::RecordNotFound
create_issue_duplicate_note(issuable, original_issue) return
close_service.new(project, current_user, {}).execute(issuable)
create_cross_reference_note(original_issue, issuable)
end end
note = create_issue_duplicate_note(issuable, original_issue)
note.create_cross_references!
close_service.new(project, current_user, {}).execute(issuable)
original_issue.create_award_emoji(AwardEmoji::UPVOTE_NAME, issuable.author)
end end
def toggle_award(issuable) def toggle_award(issuable)
......
...@@ -552,7 +552,7 @@ module SystemNoteService ...@@ -552,7 +552,7 @@ module SystemNoteService
create_note(NoteSummary.new(noteable, project, author, body, action: 'moved')) create_note(NoteSummary.new(noteable, project, author, body, action: 'moved'))
end end
# Called when a Notable has been marked as a duplicate of another Issue # Called when a Noteable has been marked as a duplicate of another Issue
# #
# noteable - Noteable object # noteable - Noteable object
# project - Project owning noteable # project - Project owning noteable
......
...@@ -147,9 +147,7 @@ feature 'Issues > User uses quick actions', feature: true, js: true do ...@@ -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 'Commands applied'
expect(page).to have_content "marked this issue as a duplicate of #{original_issue.to_reference}" expect(page).to have_content "marked this issue as a duplicate of #{original_issue.to_reference}"
issue.reload expect(issue.reload).to be_closed
expect(issue.closed?).to be_truthy
end end
end end
...@@ -169,9 +167,7 @@ feature 'Issues > User uses quick actions', feature: true, js: true do ...@@ -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 'Commands applied'
expect(page).not_to have_content "marked this issue as a duplicate of #{original_issue.to_reference}" expect(page).not_to have_content "marked this issue as a duplicate of #{original_issue.to_reference}"
issue.reload expect(issue.reload).to be_open
expect(issue.closed?).to be_falsey
end end
end end
end end
......
...@@ -492,57 +492,43 @@ describe Issues::UpdateService, services: true do ...@@ -492,57 +492,43 @@ describe Issues::UpdateService, services: true do
end end
context 'duplicate issue' do context 'duplicate issue' do
let(:issues_finder) { spy(:issues_finder) } let(:original_issue) { create(:issue, project: project) }
let(:close_service) { spy(:close_service) }
context 'invalid original_issue_id' do
before do before do
allow(IssuesFinder).to receive(:new).and_return(issues_finder) update_issue(original_issue_id: 123456789)
allow(Issues::CloseService).to receive(:new).and_return(close_service)
allow(SystemNoteService).to receive(:cross_reference)
allow(SystemNoteService).to receive(:mark_duplicate_issue)
end end
context 'invalid original_issue_id' do it 'does not close the issue' do
let(:original_issue_id) { double } expect(issue.reload).not_to be_closed
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 end
it 'does not close the issue' do it 'does not create a system note' do
expect(close_service).not_to have_received(:execute) note = find_note("marked this issue as a duplicate of #{original_issue.to_reference}")
expect(note).to be_nil
end end
it 'does not create system notes' do it 'does not upvote the issue on behalf of the author' do
expect(SystemNoteService).not_to have_received(:cross_reference) expect(original_issue).not_to be_awarded_emoji(AwardEmoji::UPVOTE_NAME, issue.author)
expect(SystemNoteService).not_to have_received(:mark_duplicate_issue)
end end
end end
context 'valid original_issue_id' do context 'valid original_issue_id' do
let(:original_issue) { create(:issue, project: project) }
let(:original_issue_id) { double }
before do before do
allow(issues_finder).to receive(:find).and_return(original_issue) update_issue(original_issue_id: original_issue.id)
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 end
it 'closes the issue' do it 'closes the issue' do
expect(close_service).to have_received(:execute).with(issue) expect(issue.reload).to be_closed
end end
it 'creates a system note that this issue is a duplicate' do 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 end
it 'creates a cross reference system note in the other issue' do it 'upvotes the issue on behalf of the author' do
expect(SystemNoteService).to have_received(:cross_reference).with(original_issue, issue, user) expect(original_issue).to be_awarded_emoji(AwardEmoji::UPVOTE_NAME, issue.author)
end end
end end
end end
......
...@@ -262,8 +262,6 @@ describe QuickActions::InterpretService, services: true do ...@@ -262,8 +262,6 @@ describe QuickActions::InterpretService, services: true do
end end
shared_examples 'duplicate command' do 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 it 'fetches issue and populates original_issue_id if content contains /duplicate issue_reference' do
issue_duplicate # populate the issue issue_duplicate # populate the issue
_, updates = service.execute(content, issuable) _, updates = service.execute(content, issuable)
...@@ -655,13 +653,15 @@ describe QuickActions::InterpretService, services: true do ...@@ -655,13 +653,15 @@ describe QuickActions::InterpretService, services: true do
let(:issuable) { issue } let(:issuable) { issue }
end end
context '/duplicate command' do
it_behaves_like 'duplicate command' do it_behaves_like 'duplicate command' do
let(:issue_duplicate) { create(:issue, project: project) }
let(:content) { "/duplicate #{issue_duplicate.to_reference}" } let(:content) { "/duplicate #{issue_duplicate.to_reference}" }
let(:issuable) { issue } let(:issuable) { issue }
end end
it_behaves_like 'empty command' do it_behaves_like 'empty command' do
let(:content) { '/duplicate #{issue.to_reference}' } let(:content) { "/duplicate #{issue.to_reference}" }
let(:issuable) { issue } let(:issuable) { issue }
end end
...@@ -670,11 +670,29 @@ describe QuickActions::InterpretService, services: true do ...@@ -670,11 +670,29 @@ describe QuickActions::InterpretService, services: true do
let(:issuable) { issue } let(:issuable) { issue }
end end
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 it_behaves_like 'empty command' do
let(:content) { '/duplicate imaginary#1234' } let(:content) { '/duplicate imaginary#1234' }
let(:issuable) { issue } let(:issuable) { issue }
end 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 context 'when current_user cannot :admin_issue' do
let(:visitor) { create(:user) } let(:visitor) { create(:user) }
let(:issue) { create(:issue, project: project, author: visitor) } let(:issue) { create(:issue, project: project, author: visitor) }
......
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