Merge branch '11851-design-annotation-system-notes' into 'master'

Create a system note for new design discussions

See merge request gitlab-org/gitlab!19990

app/services/notes/post_process_service.rb

@@ -35,3 +35,5 @@ module Notes
     end
   end
 end
+
+Notes::PostProcessService.prepend_if_ee('EE::Notes::PostProcessService')

ee/app/helpers/ee/system_note_helper.rb

@@ -21,7 +21,8 @@ module EE
       'unrelate_epic' => 'epic',
       'designs_added' => 'doc-image',
       'designs_modified' => 'doc-image',
-      'designs_removed' => 'doc-image'
+      'designs_removed' => 'doc-image',
+      'designs_discussion_added' => 'doc-image'
     }.freeze
 
     override :system_note_icon_name

ee/app/models/ee/system_note_metadata.rb

@@ -8,7 +8,7 @@ module EE
       weight approved unapproved relate unrelate
       epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
       epic_issue_moved issue_changed_epic epic_date_changed relate_epic unrelate_epic
-      designs_added designs_modified designs_removed
+      designs_added designs_modified designs_removed designs_discussion_added
     ].freeze
 
     EE_TYPES_WITH_CROSS_REFERENCES = %w[

ee/app/services/ee/notes/post_process_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Notes
+    module PostProcessService
+      extend ActiveSupport::Concern
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute
+        super
+        return unless create_design_discussion_system_note?
+
+        ::SystemNoteService.design_discussion_added(note)
+      end
+
+      private
+
+      def create_design_discussion_system_note?
+        note && note.for_design? && note.discussion.new_discussion?
+      end
+    end
+  end
+end

ee/app/services/ee/system_note_service.rb

@@ -7,6 +7,7 @@
 module EE
   module SystemNoteService
     extend ActiveSupport::Concern
+    include ActionView::RecordIdentifier
 
     prepended do
       # ::SystemNoteService wants the methods to be available as both class and
@@ -37,7 +38,7 @@ module EE
       issue = version.issue
       project = issue.project
       user = version.author
-      link_href = design_version_path(version)
+      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])
@@ -50,6 +51,31 @@ module EE
       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
+      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))
+    end
+
     def epic_issue(epic, issue, user, type)
       return unless validate_epic_issue_action_type(type)
 
@@ -250,15 +276,8 @@ module EE
 
     private
 
-    # We do not have a named route for DesignManagement::Version, instead
-    # we route to `/designs`, with the version in the query parameters.
-    # This is because this route is not managed by Rails, but Vue:
-    def design_version_path(version)
-      ::Gitlab::Routing.url_helpers.designs_project_issue_path(
-        version.project,
-        version.issue,
-        version: version.id
-      )
+    def designs_path(project, issue, params = {})
+      url_helpers.designs_project_issue_path(project, issue, params)
     end
 
     # Take one of the `DesignManagement::Action.events` and

ee/changelogs/unreleased/11851-design-annotation-system-notes.yml

+---
+title: New discussions on designs will generate a system note on the issue
+merge_request: 19990
+author:
+type: added

ee/spec/factories/notes.rb

@@ -7,6 +7,10 @@ FactoryBot.modify do
       project { nil }
     end
 
+    trait :on_design do
+      noteable { create(:design, :with_file, project: project) }
+    end
+
     trait :with_review do
       review
     end
@@ -16,13 +20,7 @@ end
 FactoryBot.define do
   factory :note_on_epic, parent: :note, traits: [:on_epic]
 
-  factory :diff_note_on_design, class: DiffNote do
-    association :project
-    note { generate(:title) }
-    author { project&.creator || create(:user) }
-
-    noteable { create(:design, :with_file, project: project) }
-
+  factory :diff_note_on_design, parent: :note, traits: [:on_design], class: DiffNote do
     position do
       Gitlab::Diff::Position.new(
         old_path: noteable.full_path,

ee/spec/services/ee/notes/post_process_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Notes::PostProcessService do
+  describe '#execute' do
+    context 'when the noteable is a design' do
+      let_it_be(:noteable) { create(:design, :with_file) }
+      let_it_be(:discussion_note) { create_note }
+
+      subject { described_class.new(note).execute }
+
+      def create_note(in_reply_to: nil)
+        create(:diff_note_on_design, noteable: noteable, project: noteable.project, in_reply_to: in_reply_to)
+      end
+
+      context 'when the note is the start of a new discussion' do
+        let(:note) { discussion_note }
+
+        it 'creates a new system note' do
+          expect { subject }.to change { Note.system.count }.by(1)
+        end
+      end
+
+      context 'when the note is a reply within a discussion' do
+        let_it_be(:note) { create_note(in_reply_to: discussion_note) }
+
+        it 'does not create a new system note' do
+          expect { subject }.not_to change { Note.system.count }
+        end
+      end
+    end
+  end
+end

ee/spec/services/system_note_service_spec.rb

@@ -8,12 +8,12 @@ describe SystemNoteService do
   include RepoHelpers
   include DesignManagementTestHelpers
 
-  set(:group)    { create(:group) }
-  set(:project)  { create(:project, :repository, group: group) }
-  set(:author)   { create(:user) }
-  let(:noteable) { create(:issue, project: project) }
-  let(:issue)    { noteable }
-  let(:epic)     { create(:epic, group: group) }
+  let_it_be(:group)    { create(:group) }
+  let_it_be(:project)  { create(:project, :repository, group: group) }
+  let_it_be(:author)   { create(:user) }
+  let_it_be(:noteable) { create(:issue, project: project) }
+  let_it_be(:issue)    { noteable }
+  let_it_be(:epic)     { create(:epic, group: group) }
 
   describe '.relate_issue' do
     let(:noteable_ref) { double }
@@ -53,7 +53,6 @@ describe SystemNoteService do
     subject { described_class.design_version_added(version) }
 
     # default (valid) parameters:
-    set(:issue) { create(:issue) }
     let(:n_designs) { 3 }
     let(:designs) { create_list(:design, n_designs, issue: issue) }
     let(:user) { build(:user) }
@@ -140,7 +139,7 @@ describe SystemNoteService do
         end
 
         let(:anchor_tag) { %r{ <a[^>]*>#{link}</a>} }
-        let(:href) { described_class.send(:design_version_path, version) }
+        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 }
@@ -160,6 +159,32 @@ describe SystemNoteService do
     end
   end
 
+  describe '.design_discussion_added' do
+    subject { described_class.design_discussion_added(discussion_note) }
+
+    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)
+    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})")
+    end
+  end
+
   describe '.approve_mr' do
     it 'calls MergeRequestsService' do
       expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service|

locale/gitlab.pot

@@ -20993,6 +20993,9 @@ msgstr ""
 msgid "started"
 msgstr ""
 
+msgid "started a discussion on %{design_link}"
+msgstr ""
+
 msgid "started on %{milestone_start_date}"
 msgstr ""
 

spec/workers/new_note_worker_spec.rb

@@ -7,16 +7,17 @@ describe NewNoteWorker do
     let(:note) { create(:note) }
 
     it "calls NotificationService#new_note" do
-      expect_any_instance_of(NotificationService).to receive(:new_note).with(note)
+      expect_next_instance_of(NotificationService) do |service|
+        expect(service).to receive(:new_note).with(note)
+      end
 
       described_class.new.perform(note.id)
     end
 
     it "calls Notes::PostProcessService#execute" do
-      notes_post_process_service = double(Notes::PostProcessService)
-      allow(Notes::PostProcessService).to receive(:new).with(note) { notes_post_process_service }
-
-      expect(notes_post_process_service).to receive(:execute)
+      expect_next_instance_of(Notes::PostProcessService) do |service|
+        expect(service).to receive(:execute)
+      end
 
       described_class.new.perform(note.id)
     end
@@ -36,14 +37,14 @@ describe NewNoteWorker do
       expect { described_class.new.perform(unexistent_note_id) }.not_to raise_error
     end
 
-    it "does not call NotificationService#new_note" do
-      expect_any_instance_of(NotificationService).not_to receive(:new_note)
+    it "does not call NotificationService" do
+      expect(NotificationService).not_to receive(:new)
 
       described_class.new.perform(unexistent_note_id)
     end
 
-    it "does not call Notes::PostProcessService#execute" do
-      expect_any_instance_of(Notes::PostProcessService).not_to receive(:execute)
+    it "does not call Notes::PostProcessService" do
+      expect(Notes::PostProcessService).not_to receive(:new)
 
       described_class.new.perform(unexistent_note_id)
     end