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.

6 files changed, 71 insertions, 70 deletions

diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb
index 1ffdd285b91..0b33e45473b 100644
--- a/app/models/system_note_metadata.rb
+++ b/app/models/system_note_metadata.rb
@@ -1,8 +1,9 @@
 class SystemNoteMetadata < ActiveRecord::Base
   ICON_TYPES = %w[
     commit description merge confidential visible label assignee cross_reference
-    title time_tracking branch milestone discussion task moved opened closed merged
-    outdated duplicate
+    title time_tracking branch milestone discussion task moved
+    opened closed merged duplicate
+    outdated
   ].freeze
 
   validates :note, presence: true
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index c7e646222bb..f57fbaca836 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -50,10 +50,6 @@ class IssuableBaseService < BaseService
     SystemNoteService.mark_duplicate_issue(issuable, issuable.project, current_user, original_issue)
   end
 
-  def create_cross_reference_note(noteable, mentioner)
-    SystemNoteService.cross_reference(noteable, mentioner, current_user)
-  end
-
   def filter_params(issuable)
     ability_name = :"admin_#{issuable.to_ability_name}"
 
@@ -303,14 +299,18 @@ class IssuableBaseService < BaseService
 
   def change_issue_duplicate(issuable)
     original_issue_id = params.delete(:original_issue_id)
-    return if original_issue_id.nil?
+    return unless original_issue_id
 
-    original_issue = IssuesFinder.new(current_user).find(original_issue_id)
-    if original_issue.present?
-      create_issue_duplicate_note(issuable, original_issue)
-      close_service.new(project, current_user, {}).execute(issuable)
-      create_cross_reference_note(original_issue, issuable)
+    begin
+      original_issue = IssuesFinder.new(current_user).find(original_issue_id)
+    rescue ActiveRecord::RecordNotFound
+      return
     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
 
   def toggle_award(issuable)
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 2e5e904c43d..ed079f0e495 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -552,11 +552,11 @@ module SystemNoteService
     create_note(NoteSummary.new(noteable, project, author, body, action: 'moved'))
   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
-  # project    - Project owning noteable
-  # author     - User performing the change
+  # noteable       - Noteable object
+  # project        - Project owning noteable
+  # author         - User performing the change
   # original_issue - Issue that this is a duplicate of
   #
   # Example Note text:
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