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

Extract Design Management System Note Service

See merge request gitlab-org/gitlab!18162

ee/app/services/ee/system_note_service.rb

@@ -34,21 +34,9 @@ module EE
     #
     # Returns [Array<Note>]: the created Note objects
     def design_version_added(version)
-      events = DesignManagement::Action.events
-      issue = version.issue
-      project = issue.project
-      user = version.author
-      link_href = designs_path(project, issue, version: version.id)
-
-      version.designs_by_event.map do |(event_name, designs)|
-        note_data = design_event_note_data(events[event_name])
-        icon_name = note_data[:icon]
-        n = designs.size
-
-        body = "%s [%d %s](%s)" % [note_data[:past_tense], n, 'design'.pluralize(n), link_href]
-
-        create_note(NoteSummary.new(issue, project, user, body, action: icon_name))
-      end
+      EE::SystemNotes::DesignManagementService.new(noteable: version.issue,
+                                                   project: version.issue.project,
+                                                   author: version.author).design_version_added(version)
     end
 
     # Called when a new discussion is created on a design
@@ -62,18 +50,9 @@ module EE
     # Returns the created Note object
     def design_discussion_added(discussion_note)
       design = discussion_note.noteable
-      issue = design.issue
-      project = design.project
-      user = discussion_note.author
-      body = _('started a discussion on %{design_link}') % {
-        design_link: '[%s](%s)' % [
-          design.filename,
-          designs_path(project, issue, vueroute: design.filename, anchor: dom_id(discussion_note))
-        ]
-      }
-      action = :designs_discussion_added
-
-      create_note(NoteSummary.new(issue, project, user, body, action: action))
+      EE::SystemNotes::DesignManagementService.new(noteable: design.issue,
+                                                   project: design.project,
+                                                   author: discussion_note.author).design_discussion_added(discussion_note)
     end
 
     def epic_issue(epic, issue, user, type)
@@ -273,33 +252,5 @@ module EE
 
       create_note(NoteSummary.new(noteable, project, author, body, action: 'closed'))
     end
-
-    private
-
-    def designs_path(project, issue, params = {})
-      url_helpers.designs_project_issue_path(project, issue, params)
-    end
-
-    # Take one of the `DesignManagement::Action.events` and
-    # return:
-    #   * an English past-tense verb.
-    #   * the name of an icon used in renderin a system note
-    #
-    # We do not currently internationalize our system notes,
-    # instead we just produce English-language descriptions.
-    # See: https://gitlab.com/gitlab-org/gitlab/issues/30408
-    # See: https://gitlab.com/gitlab-org/gitlab/issues/14056
-    def design_event_note_data(event)
-      case event
-      when DesignManagement::Action.events[:creation]
-        { icon: 'designs_added', past_tense: 'added' }
-      when DesignManagement::Action.events[:modification]
-        { icon: 'designs_modified', past_tense: 'updated' }
-      when DesignManagement::Action.events[:deletion]
-        { icon: 'designs_removed', past_tense: 'removed' }
-      else
-        raise "Unknown event: #{event}"
-      end
-    end
   end
 end

ee/app/services/ee/system_notes/design_management_service.rb

+# frozen_string_literal: true
+
+module EE
+  module SystemNotes
+    class DesignManagementService < ::SystemNotes::BaseService
+      include ActionView::RecordIdentifier
+
+      # Parameters:
+      #   - version [DesignManagement::Version]
+      #
+      # Example Note text:
+      #
+      #   "added [1 designs](link-to-version)"
+      #   "changed [2 designs](link-to-version)"
+      #
+      # Returns [Array<Note>]: the created Note objects
+      def design_version_added(version)
+        events = DesignManagement::Action.events
+        link_href = designs_path(version: version.id)
+
+        version.designs_by_event.map do |(event_name, designs)|
+          note_data = self.class.design_event_note_data(events[event_name])
+          icon_name = note_data[:icon]
+          n = designs.size
+
+          body = "%s [%d %s](%s)" % [note_data[:past_tense], n, 'design'.pluralize(n), link_href]
+
+          create_note(NoteSummary.new(noteable, project, author, body, action: icon_name))
+        end
+      end
+
+      # Called when a new discussion is created on a design
+      #
+      # discussion_note - DiscussionNote
+      #
+      # Example Note text:
+      #
+      #   "started a discussion on screen.png"
+      #
+      # Returns the created Note object
+      def design_discussion_added(discussion_note)
+        design = discussion_note.noteable
+
+        body = _('started a discussion on %{design_link}') % {
+          design_link: '[%s](%s)' % [
+            design.filename,
+            designs_path(vueroute: design.filename, anchor: dom_id(discussion_note))
+          ]
+        }
+
+        action = :designs_discussion_added
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: action))
+      end
+
+      # Take one of the `DesignManagement::Action.events` and
+      # return:
+      #   * an English past-tense verb.
+      #   * the name of an icon used in renderin a system note
+      #
+      # We do not currently internationalize our system notes,
+      # instead we just produce English-language descriptions.
+      # See: https://gitlab.com/gitlab-org/gitlab/issues/30408
+      # See: https://gitlab.com/gitlab-org/gitlab/issues/14056
+      def self.design_event_note_data(event)
+        case event
+        when DesignManagement::Action.events[:creation]
+          { icon: 'designs_added', past_tense: 'added' }
+        when DesignManagement::Action.events[:modification]
+          { icon: 'designs_modified', past_tense: 'updated' }
+        when DesignManagement::Action.events[:deletion]
+          { icon: 'designs_removed', past_tense: 'removed' }
+        else
+          raise "Unknown event: #{event}"
+        end
+      end
+
+      private
+
+      def designs_path(params = {})
+        url_helpers.designs_project_issue_path(project, noteable, params)
+      end
+    end
+  end
+end

ee/spec/services/ee/system_notes/design_management_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::SystemNotes::DesignManagementService do
+  let(:project) { create(:project) }
+  let(:issue) { create(:issue, project: project) }
+
+  let(:instance) { described_class.new(noteable: instance_noteable, project: instance_project, author: instance_author) }
+
+  describe '#design_version_added' do
+    let(:instance_noteable) { version.issue }
+    let(:instance_project) { version.issue.project }
+    let(:instance_author) { version.author }
+
+    subject { instance.design_version_added(version) }
+
+    # default (valid) parameters:
+    let(:n_designs) { 3 }
+    let(:designs) { create_list(:design, n_designs, issue: issue) }
+    let(:user) { build(:user) }
+    let(:version) do
+      create(:design_version, issue: issue, designs: designs)
+    end
+
+    before do
+      # Avoid needing to call into gitaly
+      allow(version).to receive(:author).and_return(user)
+    end
+
+    context 'with one kind of event' do
+      before do
+        DesignManagement::Action
+          .where(design: designs).update_all(event: :modification)
+      end
+
+      it 'makes just one note' do
+        expect(subject).to contain_exactly(Note)
+      end
+
+      it 'adds a new system note' do
+        expect { subject }.to change { Note.system.count }.by(1)
+      end
+    end
+
+    context 'with a mixture of events' do
+      let(:n_designs) { DesignManagement::Action.events.size }
+
+      before do
+        designs.each_with_index do |design, i|
+          design.actions.update_all(event: i)
+        end
+      end
+
+      it 'makes one note for each kind of event' do
+        expect(subject).to have_attributes(size: n_designs)
+      end
+
+      it 'adds a system note for each kind of event' do
+        expect { subject }.to change { Note.system.count }.by(n_designs)
+      end
+    end
+
+    describe 'icons' do
+      where(:action) do
+        [
+          [:creation],
+          [:modification],
+          [:deletion]
+        ]
+      end
+
+      with_them do
+        before do
+          version.actions.update_all(event: action)
+        end
+
+        subject(:metadata) do
+          instance.design_version_added(version)
+            .first.system_note_metadata
+        end
+
+        it 'has a valid action' do
+          expect(EE::SystemNoteHelper::EE_ICON_NAMES_BY_ACTION)
+            .to include(metadata.action)
+        end
+      end
+    end
+
+    context 'it succeeds' do
+      where(:action, :icon, :human_description) do
+        [
+          [:creation,     'designs_added',    'added'],
+          [:modification, 'designs_modified', 'updated'],
+          [:deletion,     'designs_removed',  'removed']
+        ]
+      end
+
+      with_them do
+        before do
+          version.actions.update_all(event: action)
+        end
+
+        let(:anchor_tag) { %r{ <a[^>]*>#{link}</a>} }
+        let(:href) { instance.send(:designs_path, { version: version.id }) }
+        let(:link) { "#{n_designs} designs" }
+
+        subject(:note) { instance.design_version_added(version).first }
+
+        it 'has the correct data' do
+          expect(note)
+            .to be_system
+            .and have_attributes(
+              system_note_metadata: have_attributes(action: icon),
+              note: include(human_description)
+                      .and(include link)
+                      .and(include href),
+              note_html: a_string_matching(anchor_tag)
+            )
+        end
+      end
+    end
+  end
+
+  describe '#design_discussion_added' do
+    let(:instance_noteable) { design.issue }
+    let(:instance_project) { design.issue.project }
+    let(:instance_author) { discussion_note.author }
+
+    subject { instance.design_discussion_added(discussion_note) }
+
+    let(:design) { create(:design, :with_file, issue: issue) }
+    let(:author) { create(:user) }
+    let(:discussion_note) do
+      create(:diff_note_on_design, noteable: design, author: author, project: project)
+    end
+    let(:action) { 'designs_discussion_added' }
+
+    it_behaves_like 'a system note' do
+      let(:noteable) { discussion_note.noteable.issue }
+    end
+
+    it 'adds a new system note' do
+      expect { subject }.to change { Note.system.count }.by(1)
+    end
+
+    it 'has the correct note text' do
+      href = instance.send(:designs_path,
+        { vueroute: design.filename, anchor: ActionView::RecordIdentifier.dom_id(discussion_note) }
+      )
+
+      expect(subject.note).to eq("started a discussion on [#{design.filename}](#{href})")
+    end
+  end
+end

ee/spec/services/system_note_service_spec.rb

@@ -50,132 +50,26 @@ describe SystemNoteService do
   end
 
   describe '.design_version_added' do
-    subject { described_class.design_version_added(version) }
+    let(:version) { create(:design_version) }
 
-    # default (valid) parameters:
-    let_it_be(:n_designs) { 3 }
-    let_it_be(:designs) { create_list(:design, n_designs, issue: issue) }
-    let_it_be(:version) do
-      create(:design_version, issue: issue, designs: designs)
+    it 'calls DesignManagementService' do
+      expect_next_instance_of(EE::SystemNotes::DesignManagementService) do |service|
+        expect(service).to receive(:design_version_added).with(version)
       end
 
-    context 'with one kind of event' do
-      before do
-        DesignManagement::Action
-          .where(design: designs).update_all(event: :modification)
-      end
-
-      it 'makes just one note' do
-        expect(subject).to contain_exactly(Note)
-      end
-
-      it 'adds a new system note' do
-        expect { subject }.to change { Note.system.count }.by(1)
-      end
-    end
-
-    context 'with a mixture of events' do
-      let_it_be(:n_designs) { DesignManagement::Action.events.size }
-
-      before do
-        designs.each_with_index do |design, i|
-          design.actions.update_all(event: i)
-        end
-      end
-
-      it 'makes one note for each kind of event' do
-        expect(subject).to have_attributes(size: n_designs)
-      end
-
-      it 'adds a system note for each kind of event' do
-        expect { subject }.to change { Note.system.count }.by(n_designs)
-      end
-    end
-
-    describe 'icons' do
-      where(:action) do
-        [
-          [:creation],
-          [:modification],
-          [:deletion]
-        ]
-      end
-
-      with_them do
-        before do
-          version.actions.update_all(event: action)
-        end
-
-        subject(:metadata) do
       described_class.design_version_added(version)
-            .first.system_note_metadata
-        end
-
-        it 'has a valid action' do
-          expect(EE::SystemNoteHelper::EE_ICON_NAMES_BY_ACTION)
-            .to include(metadata.action)
-        end
-      end
-    end
-
-    context 'it succeeds' do
-      where(:action, :icon, :human_description) do
-        [
-          [:creation,     'designs_added',    'added'],
-          [:modification, 'designs_modified', 'updated'],
-          [:deletion,     'designs_removed',  'removed']
-        ]
-      end
-
-      with_them do
-        before do
-          version.actions.update_all(event: action)
-        end
-
-        let(:anchor_tag) { %r{ <a[^>]*>#{link}</a>} }
-        let(:href) { described_class.send(:designs_path, project, issue, { version: version.id }) }
-        let(:link) { "#{n_designs} designs" }
-
-        subject(:note) { described_class.design_version_added(version).first }
-
-        it 'has the correct data' do
-          expect(note)
-            .to be_system
-            .and have_attributes(
-              system_note_metadata: have_attributes(action: icon),
-              note: include(human_description)
-                      .and(include link)
-                      .and(include href),
-              note_html: a_string_matching(anchor_tag)
-            )
-        end
-      end
     end
   end
 
   describe '.design_discussion_added' do
-    subject { described_class.design_discussion_added(discussion_note) }
+    let(:discussion_note) { create(:diff_note_on_design) }
 
-    let_it_be(:design) { create(:design, :with_file, issue: issue) }
-    let_it_be(:discussion_note) do
-      create(:diff_note_on_design, noteable: design, author: author, project: project)
+    it 'calls DesignManagementService' do
+      expect_next_instance_of(EE::SystemNotes::DesignManagementService) do |service|
+        expect(service).to receive(:design_discussion_added).with(discussion_note)
       end
-    let(:action) { 'designs_discussion_added' }
-
-    it_behaves_like 'a system note' do
-      let_it_be(:noteable) { discussion_note.noteable.issue }
-    end
-
-    it 'adds a new system note' do
-      expect { subject }.to change { Note.system.count }.by(1)
-    end
-
-    it 'has the correct note text' do
-      href = described_class.send(:designs_path, project, issue,
-        { vueroute: design.filename, anchor: ActionView::RecordIdentifier.dom_id(discussion_note) }
-      )
 
-      expect(subject.note).to eq("started a discussion on [#{design.filename}](#{href})")
+      described_class.design_discussion_added(discussion_note)
     end
   end