Merge branch '17597-extract-epics-system-note-service-pd' into 'master'

Extract Epics System Note Service

See merge request gitlab-org/gitlab!18161

ee/app/services/ee/system_note_service.rb

@@ -56,67 +56,23 @@ module EE
     end
 
     def epic_issue(epic, issue, user, type)
-      return unless validate_epic_issue_action_type(type)
-
-      action = type == :added ? 'epic_issue_added' : 'epic_issue_removed'
-
-      body = "#{type} issue #{issue.to_reference(epic.group)}"
-
-      create_note(NoteSummary.new(epic, nil, user, body, action: action))
+      EE::SystemNotes::EpicsService.new(noteable: epic, author: user).epic_issue(issue, type)
     end
 
     def epic_issue_moved(from_epic, issue, to_epic, user)
-      epic_issue_moved_act(from_epic, issue, to_epic, user, verb: 'added', direction: 'from')
-      epic_issue_moved_act(to_epic, issue, from_epic, user, verb: 'moved', direction: 'to')
-    end
-
-    def epic_issue_moved_act(subject_epic, issue, object_epic, user, verb:, direction:)
-      action = 'epic_issue_moved'
-
-      body = "#{verb} issue #{issue.to_reference(subject_epic.group)} #{direction}" \
-             " epic #{subject_epic.to_reference(object_epic.group)}"
-
-      create_note(NoteSummary.new(object_epic, nil, user, body, action: action))
+      EE::SystemNotes::EpicsService.new(noteable: from_epic, author: user).epic_issue_moved(issue, to_epic)
     end
 
     def issue_promoted(noteable, noteable_ref, author, direction:)
-      unless [:to, :from].include?(direction)
-        raise ArgumentError, "Invalid direction `#{direction}`"
-      end
-
-      project = noteable.project
-
-      cross_reference = noteable_ref.to_reference(project || noteable.group)
-      body = "promoted #{direction} #{noteable_ref.class.to_s.downcase} #{cross_reference}"
-
-      create_note(NoteSummary.new(noteable, project, author, body, action: 'moved'))
+      EE::SystemNotes::EpicsService.new(noteable: noteable, author: author).issue_promoted(noteable_ref, direction: direction)
     end
 
     def issue_on_epic(issue, epic, user, type)
-      return unless validate_epic_issue_action_type(type)
-
-      if type == :added
-        direction = 'to'
-        action = 'issue_added_to_epic'
-      else
-        direction = 'from'
-        action = 'issue_removed_from_epic'
-      end
-
-      body = "#{type} #{direction} epic #{epic.to_reference(issue.project)}"
-
-      create_note(NoteSummary.new(issue, issue.project, user, body, action: action))
+      EE::SystemNotes::EpicsService.new(noteable: epic, author: user).issue_on_epic(issue, type)
     end
 
     def issue_epic_change(issue, epic, user)
-      body = "changed epic to #{epic.to_reference(issue.project)}"
-      action = 'issue_changed_epic'
-
-      create_note(NoteSummary.new(issue, issue.project, user, body, action: action))
-    end
-
-    def validate_epic_issue_action_type(type)
-      [:added, :removed].include?(type)
+      EE::SystemNotes::EpicsService.new(noteable: epic, author: user).issue_epic_change(issue)
     end
 
     # Called when the merge request is approved by user
@@ -167,30 +123,11 @@ module EE
     #
     # Returns the created Note object
     def change_epic_date_note(noteable, author, date_type, date)
-      body = if date
-               "changed #{date_type} to #{date.strftime('%b %-d, %Y')}"
-             else
-               "removed the #{date_type}"
-             end
-
-      create_note(NoteSummary.new(noteable, nil, author, body, action: 'epic_date_changed'))
+      EE::SystemNotes::EpicsService.new(noteable: noteable, author: author).change_epic_date_note(date_type, date)
     end
 
     def change_epics_relation(epic, child_epic, user, type)
-      note_body = if type == 'relate_epic'
-                    "added epic %{target_epic_ref} as %{direction} epic"
-                  else
-                    "removed %{direction} epic %{target_epic_ref}"
-                  end
-
-      change_epics_relation_act(epic, user, type, note_body,
-                                { direction: 'child', target_epic_ref: child_epic.to_reference(epic.group) })
-      change_epics_relation_act(child_epic, user, type, note_body,
-                                { direction: 'parent', target_epic_ref: epic.to_reference(child_epic.group) })
-    end
-
-    def change_epics_relation_act(subject_epic, user, action, text, text_params)
-      create_note(NoteSummary.new(subject_epic, nil, user, text % text_params, action: action))
+      EE::SystemNotes::EpicsService.new(noteable: epic, author: user).change_epics_relation(child_epic, type)
     end
 
     # Called when 'merge train' is executed

ee/app/services/ee/system_notes/epics_service.rb

+# frozen_string_literal: true
+
+module EE
+  module SystemNotes
+    class EpicsService < ::SystemNotes::BaseService
+      def epic_issue(issue, type)
+        return unless validate_epic_issue_action_type(type)
+
+        action = type == :added ? 'epic_issue_added' : 'epic_issue_removed'
+
+        body = "#{type} issue #{issue.to_reference(noteable.group)}"
+
+        create_note(NoteSummary.new(noteable, nil, author, body, action: action))
+      end
+
+      def epic_issue_moved(issue, to_epic)
+        epic_issue_moved_act(noteable, issue, to_epic, author, verb: 'added', direction: 'from')
+        epic_issue_moved_act(to_epic, issue, noteable, author, verb: 'moved', direction: 'to')
+      end
+
+      def issue_promoted(noteable_ref, direction:)
+        unless [:to, :from].include?(direction)
+          raise ArgumentError, "Invalid direction `#{direction}`"
+        end
+
+        project = noteable.project
+
+        cross_reference = noteable_ref.to_reference(project || noteable.group)
+        body = "promoted #{direction} #{noteable_ref.class.to_s.downcase} #{cross_reference}"
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'moved'))
+      end
+
+      def issue_on_epic(issue, type)
+        return unless validate_epic_issue_action_type(type)
+
+        if type == :added
+          direction = 'to'
+          action = 'issue_added_to_epic'
+        else
+          direction = 'from'
+          action = 'issue_removed_from_epic'
+        end
+
+        body = "#{type} #{direction} epic #{noteable.to_reference(issue.project)}"
+
+        create_note(NoteSummary.new(issue, issue.project, author, body, action: action))
+      end
+
+      def issue_epic_change(issue)
+        body = "changed epic to #{noteable.to_reference(issue.project)}"
+        action = 'issue_changed_epic'
+
+        create_note(NoteSummary.new(issue, issue.project, author, body, action: action))
+      end
+
+      # Called when the start or end date of an Issuable is changed
+      #
+      # date_type  - 'start date' or 'finish date'
+      # date       - New date
+      #
+      # Example Note text:
+      #
+      #   "changed start date to FIXME"
+      #
+      # Returns the created Note object
+      def change_epic_date_note(date_type, date)
+        body = if date
+                 "changed #{date_type} to #{date.strftime('%b %-d, %Y')}"
+               else
+                 "removed the #{date_type}"
+               end
+
+        create_note(NoteSummary.new(noteable, nil, author, body, action: 'epic_date_changed'))
+      end
+
+      def change_epics_relation(child_epic, type)
+        note_body = if type == 'relate_epic'
+                      "added epic %{target_epic_ref} as %{direction} epic"
+                    else
+                      "removed %{direction} epic %{target_epic_ref}"
+                    end
+
+        note_body_params = { direction: 'child', target_epic_ref: child_epic.to_reference(noteable.group) }
+        create_note(NoteSummary.new(noteable, nil, author, note_body % note_body_params, action: type))
+
+        note_body_params = { direction: 'parent', target_epic_ref: noteable.to_reference(child_epic.group) }
+        create_note(NoteSummary.new(child_epic, nil, author, note_body % note_body_params, action: type))
+      end
+
+      private
+
+      def epic_issue_moved_act(subject_epic, issue, object_epic, user, verb:, direction:)
+        action = 'epic_issue_moved'
+
+        body = "#{verb} issue #{issue.to_reference(subject_epic.group)} #{direction}" \
+               " epic #{subject_epic.to_reference(object_epic.group)}"
+
+        create_note(NoteSummary.new(object_epic, nil, user, body, action: action))
+      end
+
+      def validate_epic_issue_action_type(type)
+        [:added, :removed].include?(type)
+      end
+    end
+  end
+end

ee/spec/services/ee/system_notes/epics_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::SystemNotes::EpicsService 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) }
+
+  describe '#epic_issue' do
+    let(:noteable) { epic }
+
+    context 'issue added to an epic' do
+      subject { described_class.new(noteable: noteable, author: author).epic_issue(issue, :added) }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'epic_issue_added' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("added issue #{issue.to_reference(epic.group)}")
+      end
+    end
+
+    context 'issue removed from an epic' do
+      subject { described_class.new(noteable: epic, author: author).epic_issue(issue, :removed) }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'epic_issue_removed' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("removed issue #{issue.to_reference(epic.group)}")
+      end
+    end
+  end
+
+  describe '#issue_on_epic' do
+    context 'issue added to an epic' do
+      subject { described_class.new(noteable: epic, author: author).issue_on_epic(issue, :added) }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'issue_added_to_epic' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("added to epic #{epic.to_reference(issue.project)}")
+      end
+    end
+
+    context 'issue removed from an epic' do
+      subject { described_class.new(noteable: epic, author: author).issue_on_epic(issue, :removed) }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'issue_removed_from_epic' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("removed from epic #{epic.to_reference(issue.project)}")
+      end
+    end
+
+    context 'invalid type' do
+      it 'raises an error' do
+        expect { described_class.new(noteable: epic, author: author).issue_on_epic(issue, :invalid) }
+          .not_to change { Note.count }
+      end
+    end
+  end
+
+  describe '#change_epic_date_note' do
+    let(:timestamp) { Time.now }
+
+    context 'when start date was changed' do
+      let(:noteable) { create(:epic) }
+
+      subject { described_class.new(noteable: noteable, author: author).change_epic_date_note('start date', timestamp) }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'epic_date_changed' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq "changed start date to #{timestamp.strftime('%b %-d, %Y')}"
+      end
+    end
+
+    context 'when start date was removed' do
+      let(:noteable) { create(:epic, start_date: timestamp) }
+
+      subject { described_class.new(noteable: noteable, author: author).change_epic_date_note('start date', nil) }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'epic_date_changed' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq 'removed the start date'
+      end
+    end
+  end
+
+  describe '#issue_promoted' do
+    context 'note on the epic' do
+      subject { described_class.new(noteable: epic, author: author).issue_promoted(issue, direction: :from) }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'moved' }
+        let(:expected_noteable) { epic }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq("promoted from issue #{issue.to_reference(group)}")
+      end
+    end
+
+    context 'note on the issue' do
+      subject { described_class.new(noteable: issue, author: author).issue_promoted(epic, direction: :to) }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'moved' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq("promoted to epic #{epic.to_reference(project)}")
+      end
+    end
+  end
+
+  describe '#change_epics_relation' do
+    context 'relate epic' do
+      let(:child_epic) { create(:epic, parent: epic, group: group) }
+      let(:noteable) { child_epic }
+
+      subject { described_class.new(noteable: epic, author: author).change_epics_relation(child_epic, 'relate_epic') }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'relate_epic' }
+      end
+
+      context 'when epic is added as child to a parent epic' do
+        it 'sets the note text' do
+          expect { subject }.to change { Note.system.count }.from(0).to(2)
+          expect(Note.first.note).to eq("added epic &#{child_epic.iid} as child epic")
+          expect(Note.last.note).to eq("added epic &#{epic.iid} as parent epic")
+        end
+      end
+
+      context 'when added epic is from a subgroup' do
+        let(:subgroup) {create(:group, parent: group)}
+
+        before do
+          child_epic.update!({ group: subgroup })
+        end
+
+        it 'sets the note text' do
+          expect { subject }.to change { Note.system.count }.from(0).to(2)
+          expect(Note.first.note).to eq("added epic #{group.path}/#{subgroup.path}&#{child_epic.iid} as child epic")
+          expect(Note.last.note).to eq("added epic #{group.path}&#{epic.iid} as parent epic")
+        end
+      end
+    end
+
+    context 'unrelate epic' do
+      let(:child_epic) { create(:epic, parent: epic, group: group) }
+      let(:noteable) { child_epic }
+
+      subject { described_class.new(noteable: epic, author: author).change_epics_relation(child_epic, 'unrelate_epic') }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'unrelate_epic' }
+      end
+
+      context 'when child epic is removed from a parent epic' do
+        it 'sets the note text' do
+          expect { subject }.to change { Note.system.count }.from(0).to(2)
+          expect(Note.first.note).to eq("removed child epic &#{child_epic.iid}")
+          expect(Note.last.note).to eq("removed parent epic &#{epic.iid}")
+        end
+      end
+
+      context 'when removed epic is from a subgroup' do
+        let(:subgroup) {create(:group, parent: group)}
+
+        before do
+          child_epic.update!({ group: subgroup })
+        end
+
+        it 'sets the note text' do
+          expect { subject }.to change { Note.system.count }.from(0).to(2)
+          expect(Note.first.note).to eq("removed child epic #{group.path}/#{subgroup.path}&#{child_epic.iid}")
+          expect(Note.last.note).to eq("removed parent epic #{group.path}&#{epic.iid}")
+        end
+      end
+    end
+  end
+end

ee/spec/services/system_note_service_spec.rb

@@ -104,195 +104,52 @@ describe SystemNoteService do
   end
 
   describe '.change_epic_date_note' do
-    let(:timestamp) { Time.now }
+    let(:date_type) { double }
+    let(:date) { double }
 
-    context 'when start date was changed' do
-      let(:noteable) { create(:epic) }
-
-      subject { described_class.change_epic_date_note(noteable, author, 'start date', timestamp) }
-
-      it_behaves_like 'a system note', exclude_project: true do
-        let(:action) { 'epic_date_changed' }
-      end
-
-      it 'sets the note text' do
-        expect(subject.note).to eq "changed start date to #{timestamp.strftime('%b %-d, %Y')}"
-      end
-    end
-
-    context 'when start date was removed' do
-      let(:noteable) { create(:epic, start_date: timestamp) }
-
-      subject { described_class.change_epic_date_note(noteable, author, 'start date', nil) }
-
-      it_behaves_like 'a system note', exclude_project: true do
-        let(:action) { 'epic_date_changed' }
+    it 'calls EpicsService' do
+      expect_next_instance_of(EE::SystemNotes::EpicsService) do |service|
+        expect(service).to receive(:change_epic_date_note).with(date_type, date)
       end
 
-      it 'sets the note text' do
-        expect(subject.note).to eq 'removed the start date'
-      end
-    end
-
-    context '.issue_promoted' do
-      context 'note on the epic' do
-        subject { described_class.issue_promoted(epic, issue, author, direction: :from) }
-
-        it_behaves_like 'a system note', exclude_project: true do
-          let(:action) { 'moved' }
-          let(:expected_noteable) { epic }
-        end
-
-        it 'sets the note text' do
-          expect(subject.note).to eq("promoted from issue #{issue.to_reference(group)}")
-        end
-      end
-
-      context 'note on the issue' do
-        subject { described_class.issue_promoted(issue, epic, author, direction: :to) }
-
-        it_behaves_like 'a system note' do
-          let(:action) { 'moved' }
-        end
-
-        it 'sets the note text' do
-          expect(subject.note).to eq("promoted to epic #{epic.to_reference(project)}")
-        end
-      end
+      described_class.change_epic_date_note(noteable, author, date_type, date)
     end
   end
 
   describe '.epic_issue' do
-    let(:noteable) { epic }
-
-    context 'issue added to an epic' do
-      subject { described_class.epic_issue(epic, issue, author, :added) }
+    let(:type) { double }
 
-      it_behaves_like 'a system note', exclude_project: true do
-        let(:action) { 'epic_issue_added' }
+    it 'calls EpicsService' do
+      expect_next_instance_of(EE::SystemNotes::EpicsService) do |service|
+        expect(service).to receive(:epic_issue).with(noteable, type)
       end
 
-      it 'creates the note text correctly' do
-        expect(subject.note).to eq("added issue #{issue.to_reference(epic.group)}")
-      end
-    end
-
-    context 'issue removed from an epic' do
-      subject { described_class.epic_issue(epic, issue, author, :removed) }
-
-      it_behaves_like 'a system note', exclude_project: true do
-        let(:action) { 'epic_issue_removed' }
-      end
-
-      it 'creates the note text correctly' do
-        expect(subject.note).to eq("removed issue #{issue.to_reference(epic.group)}")
-      end
-    end
-
-    context 'invalid type' do
-      it 'raises an error' do
-        expect { described_class.issue_on_epic(issue, epic, author, :invalid) }
-          .not_to change { Note.count }
-      end
+      described_class.epic_issue(epic, noteable, author, type)
     end
   end
 
   describe '.issue_on_epic' do
-    context 'issue added to an epic' do
-      subject { described_class.issue_on_epic(issue, epic, author, :added) }
-
-      it_behaves_like 'a system note' do
-        let(:action) { 'issue_added_to_epic' }
-      end
-
-      it 'creates the note text correctly' do
-        expect(subject.note).to eq("added to epic #{epic.to_reference(issue.project)}")
-      end
-    end
+    let(:type) { double }
 
-    context 'issue removed from an epic' do
-      subject { described_class.issue_on_epic(issue, epic, author, :removed) }
-
-      it_behaves_like 'a system note' do
-        let(:action) { 'issue_removed_from_epic' }
-      end
-
-      it 'creates the note text correctly' do
-        expect(subject.note).to eq("removed from epic #{epic.to_reference(issue.project)}")
+    it 'calls EpicsService' do
+      expect_next_instance_of(EE::SystemNotes::EpicsService) do |service|
+        expect(service).to receive(:issue_on_epic).with(noteable, type)
       end
-    end
 
-    context 'invalid type' do
-      it 'does not create a new note' do
-        expect { described_class.issue_on_epic(issue, epic, author, :invalid) }
-          .not_to change { Note.count }
-      end
+      described_class.issue_on_epic(noteable, epic, author, type)
     end
   end
 
-  describe '.relate_epic' do
-    let(:child_epic) { create(:epic, parent: epic, group: group) }
-    let(:noteable) { child_epic }
-
-    subject { described_class.change_epics_relation(epic, child_epic, author, 'relate_epic') }
-
-    it_behaves_like 'a system note', exclude_project: true do
-      let(:action) { 'relate_epic' }
-    end
-
-    context 'when epic is added as child to a parent epic' do
-      it 'sets the note text' do
-        expect { subject }.to change { Note.system.count }.from(0).to(2)
-        expect(Note.first.note).to eq("added epic &#{child_epic.iid} as child epic")
-        expect(Note.last.note).to eq("added epic &#{epic.iid} as parent epic")
-      end
-    end
-
-    context 'when added epic is from a subgroup' do
-      let(:subgroup) {create(:group, parent: group)}
+  describe '.change_epics_relation' do
+    let(:child_epic) { double }
+    let(:type) { double }
 
-      before do
-        child_epic.update!({ group: subgroup })
+    it 'calls EpicsService' do
+      expect_next_instance_of(EE::SystemNotes::EpicsService) do |service|
+        expect(service).to receive(:change_epics_relation).with(child_epic, type)
       end
 
-      it 'sets the note text' do
-        expect { subject }.to change { Note.system.count }.from(0).to(2)
-        expect(Note.first.note).to eq("added epic #{group.path}/#{subgroup.path}&#{child_epic.iid} as child epic")
-        expect(Note.last.note).to eq("added epic #{group.path}&#{epic.iid} as parent epic")
-      end
-    end
-  end
-
-  describe '.unrelate_epic' do
-    let(:child_epic) { create(:epic, parent: epic, group: group) }
-    let(:noteable) { child_epic }
-
-    subject { described_class.change_epics_relation(epic, child_epic, author, 'unrelate_epic') }
-
-    it_behaves_like 'a system note', exclude_project: true do
-      let(:action) { 'unrelate_epic' }
-    end
-
-    context 'when child epic is removed from a parent epic' do
-      it 'sets the note text' do
-        expect { subject }.to change { Note.system.count }.from(0).to(2)
-        expect(Note.first.note).to eq("removed child epic &#{child_epic.iid}")
-        expect(Note.last.note).to eq("removed parent epic &#{epic.iid}")
-      end
-    end
-
-    context 'when removed epic is from a subgroup' do
-      let(:subgroup) {create(:group, parent: group)}
-
-      before do
-        child_epic.update!({ group: subgroup })
-      end
-
-      it 'sets the note text' do
-        expect { subject }.to change { Note.system.count }.from(0).to(2)
-        expect(Note.first.note).to eq("removed child epic #{group.path}/#{subgroup.path}&#{child_epic.iid}")
-        expect(Note.last.note).to eq("removed parent epic #{group.path}&#{epic.iid}")
-      end
+      described_class.change_epics_relation(epic, child_epic, author, type)
     end
   end