Extract Issuables System Note Service

Specs are using doubles

Apply constructor changes

Add specs for SystemNotes::IssuablesService

Fix misspelled file name

Add parameter exclude_project to shared sample

So duplicate definition could be removed.

Use let_it_be instead of set

Remove one superfluous redefinition

Move service definition to top of specs

Use let_it_be instead of set

Add new line

Remove obsolete method (moved to SystemNotes::IssuablesService)

Remove obsolete methods from SystemNoteService

(moved to SystemNotes::IssuablesService)

Replace doubles with noteable, project and author

app/models/note.rb

@@ -215,7 +215,7 @@ class Note < ApplicationRecord
     if force_cross_reference_regex_check?
       matches_cross_reference_regex?
     else
-      SystemNoteService.cross_reference?(note)
+      ::SystemNotes::IssuablesService.cross_reference?(note)
     end
   end
   # rubocop: enable CodeReuse/ServiceClass

ee/app/services/ee/system_note_service.rb

@@ -15,36 +15,12 @@ module EE
       extend_if_ee('EE::SystemNoteService') # rubocop: disable Cop/InjectEnterpriseEditionModule
     end
 
-    #
-    # noteable     - Noteable object
-    # noteable_ref - Referenced noteable object
-    # user         - User performing reference
-    #
-    # Example Note text:
-    #
-    #   "marked this issue as related to gitlab-foss#9001"
-    #
-    # Returns the created Note object
     def relate_issue(noteable, noteable_ref, user)
-      body = "marked this issue as related to #{noteable_ref.to_reference(noteable.project)}"
-
-      create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'relate'))
+      ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).relate_issue(noteable_ref)
     end
 
-    #
-    # noteable     - Noteable object
-    # noteable_ref - Referenced noteable object
-    # user         - User performing reference
-    #
-    # Example Note text:
-    #
-    #   "removed the relation with gitlab-foss#9001"
-    #
-    # Returns the created Note object
     def unrelate_issue(noteable, noteable_ref, user)
-      body = "removed the relation with #{noteable_ref.to_reference(noteable.project)}"
-
-      create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'unrelate'))
+      ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).unrelate_issue(noteable_ref)
     end
 
     # Parameters:
@@ -174,8 +150,7 @@ module EE
     #
     # Returns the created Note object
     def change_weight_note(noteable, project, author)
-      body = noteable.weight ? "changed weight to **#{noteable.weight}**" : 'removed the weight'
-      create_note(NoteSummary.new(noteable, project, author, body, action: 'weight'))
+      ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_weight_note
     end
 
     # Called when the start or end date of an Issuable is changed

ee/app/services/ee/system_notes/issuables_service.rb

+# frozen_string_literal: true
+module EE
+  module SystemNotes
+    module IssuablesService
+      #
+      # noteable_ref - Referenced noteable object
+      #
+      # Example Note text:
+      #
+      #   "marked this issue as related to gitlab-foss#9001"
+      #
+      # Returns the created Note object
+      def relate_issue(noteable_ref)
+        body = "marked this issue as related to #{noteable_ref.to_reference(noteable.project)}"
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'relate'))
+      end
+
+      #
+      # noteable_ref - Referenced noteable object
+      #
+      # Example Note text:
+      #
+      #   "removed the relation with gitlab-foss#9001"
+      #
+      # Returns the created Note object
+      def unrelate_issue(noteable_ref)
+        body = "removed the relation with #{noteable_ref.to_reference(noteable.project)}"
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'unrelate'))
+      end
+
+      # Called when the weight of a Noteable is changed
+      #
+      # Example Note text:
+      #
+      #   "removed the weight"
+      #
+      #   "changed weight to 4"
+      #
+      # Returns the created Note object
+      def change_weight_note
+        body = noteable.weight ? "changed weight to **#{noteable.weight}**" : 'removed the weight'
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'weight'))
+      end
+    end
+  end
+end

ee/spec/services/ee/system_notes/issuables_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::SystemNotes::IssuablesService do
+  let_it_be(:group)    { create(:group) }
+  let_it_be(:project)  { create(:project, :repository, group: group) }
+  let_it_be(:author)   { create(:user) }
+
+  let(:noteable) { create(:issue, project: project) }
+  let(:issue)    { noteable }
+  let(:epic)     { create(:epic, group: group) }
+
+  let(:service) { described_class.new(noteable: noteable, project: project, author: author) }
+
+  describe '#relate_issue' do
+    let(:noteable_ref) { create(:issue) }
+
+    subject { service.relate_issue(noteable_ref) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'relate' }
+    end
+
+    context 'when issue marks another as related' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "marked this issue as related to #{noteable_ref.to_reference(project)}"
+      end
+    end
+  end
+
+  describe '#unrelate_issue' do
+    let(:noteable_ref) { create(:issue) }
+
+    subject { service.unrelate_issue(noteable_ref) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'unrelate' }
+    end
+
+    context 'when issue relation is removed' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "removed the relation with #{noteable_ref.to_reference(project)}"
+      end
+    end
+  end
+
+  describe '#change_weight_note' do
+    context 'when weight changed' do
+      let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum', weight: 4) }
+
+      subject { service.change_weight_note }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'weight' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq "changed weight to **4**"
+      end
+    end
+
+    context 'when weight removed' do
+      let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum', weight: nil) }
+
+      subject { service.change_weight_note }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'weight' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq 'removed the weight'
+      end
+    end
+  end
+end

ee/spec/services/system_note_service_spec.rb

@@ -15,59 +15,37 @@ describe SystemNoteService do
   let(:issue)    { noteable }
   let(:epic)     { create(:epic, group: group) }
 
-  shared_examples_for 'a system note' do
-    let(:expected_noteable) { noteable }
-    let(:commit_count)      { nil }
-
-    it 'has the correct attributes', :aggregate_failures do
-      expect(subject).to be_valid
-      expect(subject).to be_system
-
-      expect(subject.noteable).to eq expected_noteable
-      expect(subject.author).to eq author
-
-      expect(subject.system_note_metadata.action).to eq(action)
-      expect(subject.system_note_metadata.commit_count).to eq(commit_count)
-    end
-  end
-
-  shared_examples_for 'a project system note' do
-    it 'has the project attribute set' do
-      expect(subject.project).to eq project
-    end
-
-    it_behaves_like 'a system note'
-  end
-
   describe '.relate_issue' do
-    let(:noteable_ref) { create(:issue) }
+    let(:noteable_ref) { double }
+    let(:noteable) { double }
 
-    subject { described_class.relate_issue(noteable, noteable_ref, author) }
-
-    it_behaves_like 'a system note' do
-      let(:action) { 'relate' }
+    before do
+      allow(noteable).to receive(:project).and_return(double)
     end
 
-    context 'when issue marks another as related' do
-      it 'sets the note text' do
-        expect(subject.note).to eq "marked this issue as related to #{noteable_ref.to_reference(project)}"
+    it 'calls IssuableService' do
+      expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
+        expect(service).to receive(:relate_issue).with(noteable_ref)
       end
+
+      described_class.relate_issue(noteable, noteable_ref, double)
     end
   end
 
   describe '.unrelate_issue' do
-    let(:noteable_ref) { create(:issue) }
+    let(:noteable_ref) { double }
+    let(:noteable) { double }
 
-    subject { described_class.unrelate_issue(noteable, noteable_ref, author) }
-
-    it_behaves_like 'a system note' do
-      let(:action) { 'unrelate' }
+    before do
+      allow(noteable).to receive(:project).and_return(double)
     end
 
-    context 'when issue relation is removed' do
-      it 'sets the note text' do
-        expect(subject.note).to eq "removed the relation with #{noteable_ref.to_reference(project)}"
+    it 'calls IssuableService' do
+      expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
+        expect(service).to receive(:unrelate_issue).with(noteable_ref)
       end
+
+      described_class.unrelate_issue(noteable, noteable_ref, double)
     end
   end
 
@@ -175,7 +153,7 @@ describe SystemNoteService do
     let(:noteable) { create(:merge_request, source_project: project) }
     subject { described_class.unapprove_mr(noteable, author) }
 
-    it_behaves_like 'a system note' do
+    it_behaves_like 'a system note', exclude_project: true do
       let(:action) { 'unapproved' }
     end
 
@@ -187,32 +165,12 @@ describe SystemNoteService do
   end
 
   describe '.change_weight_note' do
-    context 'when weight changed' do
-      let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum', weight: 4) }
-
-      subject { described_class.change_weight_note(noteable, project, author) }
-
-      it_behaves_like 'a project system note' do
-        let(:action) { 'weight' }
-      end
-
-      it 'sets the note text' do
-        expect(subject.note).to eq "changed weight to **4**"
-      end
-    end
-
-    context 'when weight removed' do
-      let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum', weight: nil) }
-
-      subject { described_class.change_weight_note(noteable, project, author) }
-
-      it_behaves_like 'a project system note' do
-        let(:action) { 'weight' }
+    it 'calls IssuableService' do
+      expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
+        expect(service).to receive(:change_weight_note)
       end
 
-      it 'sets the note text' do
-        expect(subject.note).to eq 'removed the weight'
-      end
+      described_class.change_weight_note(noteable, project, author)
     end
   end
 
@@ -224,7 +182,7 @@ describe SystemNoteService do
 
       subject { described_class.change_epic_date_note(noteable, author, 'start date', timestamp) }
 
-      it_behaves_like 'a system note' do
+      it_behaves_like 'a system note', exclude_project: true do
         let(:action) { 'epic_date_changed' }
       end
 
@@ -238,7 +196,7 @@ describe SystemNoteService do
 
       subject { described_class.change_epic_date_note(noteable, author, 'start date', nil) }
 
-      it_behaves_like 'a system note' do
+      it_behaves_like 'a system note', exclude_project: true do
         let(:action) { 'epic_date_changed' }
       end
 
@@ -251,7 +209,7 @@ describe SystemNoteService do
       context 'note on the epic' do
         subject { described_class.issue_promoted(epic, issue, author, direction: :from) }
 
-        it_behaves_like 'a system note' do
+        it_behaves_like 'a system note', exclude_project: true do
           let(:action) { 'moved' }
           let(:expected_noteable) { epic }
         end
@@ -281,7 +239,7 @@ describe SystemNoteService do
     context 'issue added to an epic' do
       subject { described_class.epic_issue(epic, issue, author, :added) }
 
-      it_behaves_like 'a system note' do
+      it_behaves_like 'a system note', exclude_project: true do
         let(:action) { 'epic_issue_added' }
       end
 
@@ -293,7 +251,7 @@ describe SystemNoteService do
     context 'issue removed from an epic' do
       subject { described_class.epic_issue(epic, issue, author, :removed) }
 
-      it_behaves_like 'a system note' do
+      it_behaves_like 'a system note', exclude_project: true do
         let(:action) { 'epic_issue_removed' }
       end
 
@@ -349,7 +307,7 @@ describe SystemNoteService do
 
     subject { described_class.change_epics_relation(epic, child_epic, author, 'relate_epic') }
 
-    it_behaves_like 'a system note' do
+    it_behaves_like 'a system note', exclude_project: true do
       let(:action) { 'relate_epic' }
     end
 
@@ -382,7 +340,7 @@ describe SystemNoteService do
 
     subject { described_class.change_epics_relation(epic, child_epic, author, 'unrelate_epic') }
 
-    it_behaves_like 'a system note' do
+    it_behaves_like 'a system note', exclude_project: true do
       let(:action) { 'unrelate_epic' }
     end
 

ee/spec/support/shared_examples/common_system_notes_examples.rb

+# frozen_string_literal: true
+
+shared_examples_for 'a project system note' do
+  it 'has the project attribute set' do
+    expect(subject.project).to eq project
+  end
+
+  it_behaves_like 'a system note', exclude_project: true
+end

spec/support/shared_examples/common_system_notes_examples.rb

@@ -36,16 +36,18 @@ shared_examples_for 'a note with overridable created_at' do
   end
 end
 
-shared_examples_for 'a system note' do
+shared_examples_for 'a system note' do |params|
   let(:expected_noteable) { noteable }
   let(:commit_count)      { nil }
 
   it 'has the correct attributes', :aggregate_failures do
+    exclude_project = !params.nil? && params[:exclude_project]
+
     expect(subject).to be_valid
     expect(subject).to be_system
 
     expect(subject.noteable).to eq expected_noteable
-    expect(subject.project).to eq project
+    expect(subject.project).to eq project unless exclude_project
     expect(subject.author).to eq author
 
     expect(subject.system_note_metadata.action).to eq(action)