Merge branch 'ajk-designs-system-notes' into 'master'

Create system notes for design events

See merge request gitlab-org/gitlab!14791

app/services/boards/issues/list_service.rb

@@ -11,9 +11,11 @@ module Boards
 
       # rubocop: disable CodeReuse/ActiveRecord
       def metadata
+        issues = Issue.arel_table
         keys = metadata_fields.keys
-        columns = metadata_fields.values_at(*keys).join(', ')
-        results = Issue.where(id: fetch_issues.select('issues.id')).pluck(columns)
+        # TODO: eliminate need for SQL literal fragment
+        columns = Arel.sql(metadata_fields.values_at(*keys).join(', '))
+        results = Issue.where(id: fetch_issues.select(issues[:id])).pluck(columns)
 
         Hash[keys.zip(results.flatten)]
       end

config/sidekiq_queues.yml

@@ -117,3 +117,4 @@
   - [jira_connect, 1]
   - [update_external_pull_requests, 3]
   - [refresh_license_compliance_checks, 2]
+  - [design_management_new_version, 1]

ee/app/helpers/ee/system_note_helper.rb

@@ -18,7 +18,10 @@ module EE
       'epic_date_changed' => 'calendar',
       'weight' => 'weight',
       'relate_epic' => 'epic',
-      'unrelate_epic' => 'epic'
+      'unrelate_epic' => 'epic',
+      'design_added' => 'doc-image',
+      'design_modified' => 'doc-image',
+      'design_removed' => 'doc-image'
     }.freeze
 
     override :system_note_icon_name

ee/app/models/design_management/version.rb

@@ -44,6 +44,8 @@ module DesignManagement
 
     sha_attribute :sha
 
+    delegate :project, to: :issue
+
     scope :for_designs, -> (designs) do
       where(id: Action.where(design_id: designs).select(:version_id)).distinct
     end
@@ -83,5 +85,22 @@ module DesignManagement
     rescue
       raise CouldNotCreateVersion.new(sha, issue_id, design_actions)
     end
+
+    def designs_by_event
+      actions
+        .includes(:design)
+        .group_by(&:event)
+        .transform_values { |group| group.map(&:design) }
+    end
+
+    def author
+      commit&.author
+    end
+
+    private
+
+    def commit
+      @commit ||= issue.project.design_repository.commit(sha)
+    end
   end
 end

ee/app/models/ee/system_note_metadata.rb

@@ -8,6 +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
     ].freeze
 
     EE_TYPES_WITH_CROSS_REFERENCES = %w[

ee/app/services/design_management/save_designs_service.rb

@@ -33,7 +33,10 @@ module DesignManagement
 
     def upload_designs!
       actions = build_actions
-      run_actions(actions) unless actions.empty?
+      return [] if actions.empty?
+
+      version = run_actions(actions)
+      ::DesignManagement::NewVersionWorker.perform_async(version.id)
 
       actions.map(&:design)
     end

ee/app/services/ee/boards/list_service.rb

@@ -25,7 +25,7 @@ module EE
       override :boards
       # rubocop: disable CodeReuse/ActiveRecord
       def boards
-        super.order('LOWER(name) ASC')
+        super.order(Arel.sql('LOWER(name) ASC'))
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

ee/app/services/ee/system_note_service.rb

@@ -47,6 +47,33 @@ module EE
       create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'unrelate'))
     end
 
+    # 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
+      issue = version.issue
+      project = issue.project
+      user = version.author
+      link_href = design_version_path(version)
+
+      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
+    end
+
     def epic_issue(epic, issue, user, type)
       return unless validate_epic_issue_action_type(type)
 
@@ -243,5 +270,40 @@ module EE
       # See https://gitlab.com/gitlab-org/gitlab-foss/issues/63187.
       create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
     end
+
+    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
+      )
+    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-ce/issues/65076
+    # See: https://gitlab.com/gitlab-org/gitlab-ee/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/workers/all_queues.yml

@@ -57,6 +57,7 @@
 
 - admin_emails
 - create_github_webhook
+- design_management_new_version
 - elastic_batch_project_indexer
 - elastic_namespace_indexer
 - elastic_commit_indexer

ee/app/workers/design_management/new_version_worker.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  class NewVersionWorker
+    include ApplicationWorker
+
+    def perform(version_id)
+      version = DesignManagement::Version.find(version_id)
+
+      add_system_note(version)
+    rescue ActiveRecord::RecordNotFound => e
+      Sidekiq.logger.warn(e)
+    end
+
+    private
+
+    def add_system_note(version)
+      SystemNoteService.design_version_added(version)
+    end
+  end
+end

ee/changelogs/unreleased/ajk-designs-system-notes.yml

+---
+title: Create system notes for design events
+merge_request: 14791
+author:
+type: added

ee/spec/factories/design_management/versions.rb

@@ -5,17 +5,109 @@ FactoryBot.define do
     sequence(:sha) { |n| Digest::SHA1.hexdigest("commit-like-#{n}") }
     issue { designs.first&.issue || create(:issue) }
 
+    transient do
+      designs_count 1
+      created_designs []
+      modified_designs []
+      deleted_designs []
+    end
+
     # Warning: this will intentionally result in an invalid version!
     trait :empty do
-      transient do
-        no_designs true
-      end
+      designs_count 0
     end
 
     after(:build) do |version, evaluator|
-      unless evaluator.try(:no_designs) || version.designs.present?
+      # By default all designs are created_designs, so just add them.
+      specific_designs = [].concat(
+        evaluator.created_designs,
+        evaluator.modified_designs,
+        evaluator.deleted_designs
+      )
+      version.designs += specific_designs
+
+      unless evaluator.designs_count.zero? || version.designs.present?
         version.designs << create(:design, issue: version.issue)
       end
     end
+
+    after :create do |version, evaluator|
+      # FactoryBot does not like methods, so we use lambdas instead
+      events = DesignManagement::Action.events
+
+      version.actions
+        .where(design_id: evaluator.modified_designs.map(&:id))
+        .update_all(event: events[:modification])
+
+      version.actions
+        .where(design_id: evaluator.deleted_designs.map(&:id))
+        .update_all(event: events[:deletion])
+
+      version.designs.reload
+      # Ensure version.issue == design.issue for all version.designs
+      version.designs.update_all(issue_id: version.issue_id)
+
+      needed = evaluator.designs_count
+      have = version.designs.size
+
+      create_list(:design, [0, needed - have].max, issue: version.issue).each do |d|
+        version.designs << d
+      end
+
+      version.actions.reset
+    end
+
+    # This trait is for versions that must be present in the git repository.
+    # This might be needed if, for instance, you need to make use of Version#author
+    trait :committed do
+      transient do
+        author { create(:user) }
+        file File.join(Rails.root, 'spec/fixtures/dk.png')
+      end
+
+      after :create do |version, evaluator|
+        project = version.issue.project
+        repository = project.design_repository
+        repository.create_if_not_exists
+
+        designs = version.designs_by_event
+        base_change = { content: evaluator.file }
+
+        actions = %w[modification deletion].flat_map { |k| designs.fetch(k, []) }.map do |design|
+          base_change.merge(action: :create, file_path: design.full_path)
+        end
+
+        if actions.present?
+          repository.multi_action(
+            evaluator.author,
+            branch_name: 'master',
+            message: "created #{actions.size} files",
+            actions: actions
+          )
+        end
+
+        mapping = {
+          'creation' => :create,
+          'modification' => :update,
+          'deletion' => :delete
+        }
+
+        version_actions = designs.flat_map do |(event, designs)|
+          base = event == 'deletion' ? {} : base_change
+          designs.map do |design|
+            base.merge(action: mapping[event], file_path: design.full_path)
+          end
+        end
+
+        sha = repository.multi_action(
+          evaluator.author,
+          branch_name: 'master',
+          message: "edited #{version_actions.size} files",
+          actions: version_actions
+        )
+
+        version.update(sha: sha)
+      end
+    end
   end
 end

ee/spec/models/design_management/version_spec.rb

@@ -204,4 +204,53 @@ describe DesignManagement::Version do
       end.to change { current_version_id(design_a) }
     end
   end
+
+  describe '#author' do
+    let(:author) { create(:user) }
+    subject(:version) { create(:design_version, :committed, author: author) }
+
+    it { is_expected.to have_attributes(author: author) }
+  end
+
+  describe '#designs_by_event' do
+    context 'there is a single design' do
+      set(:design) { create(:design) }
+
+      shared_examples :a_correctly_categorised_design do |kind, category|
+        let(:version) { create(:design_version, kind => [design]) }
+
+        it 'returns a hash with a single key and the single design in that bucket' do
+          expect(version.designs_by_event).to eq(category => [design])
+        end
+      end
+
+      it_behaves_like :a_correctly_categorised_design, :created_designs, 'creation'
+      it_behaves_like :a_correctly_categorised_design, :modified_designs, 'modification'
+      it_behaves_like :a_correctly_categorised_design, :deleted_designs, 'deletion'
+    end
+
+    context 'there are a bunch of different designs in a variety of states' do
+      let(:version) do
+        create(:design_version,
+               created_designs: create_list(:design, 3),
+               modified_designs: create_list(:design, 4),
+               deleted_designs: create_list(:design, 5))
+      end
+
+      it 'puts them in the right buckets' do
+        expect(version.designs_by_event).to match(
+          a_hash_including(
+            'creation' =>  have_attributes(size: 3),
+            'modification' => have_attributes(size: 4),
+            'deletion' => have_attributes(size: 5)
+          )
+        )
+      end
+
+      it 'does not suffer from N+1 queries' do
+        version.designs.map(&:id) # we don't care about the set-up queries
+        expect { version.designs_by_event }.not_to exceed_query_limit(2)
+      end
+    end
+  end
 end

ee/spec/models/ee/system_note_metadata_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::SystemNoteMetadata do
+  %i[designs_added designs_modified designs_removed].each do |action|
+    context 'when action type is valid' do
+      subject do
+        build(:system_note_metadata, note: build(:note), action: action )
+      end
+
+      it { is_expected.to be_valid }
+    end
+  end
+end

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -18,6 +18,18 @@ describe DesignManagement::SaveDesignsService do
 
   before do
     project.add_developer(developer)
+
+    allow(::DesignManagement::NewVersionWorker).to receive(:perform_async)
+  end
+
+  RSpec::Matchers.define :enqueue_worker do
+    match do |action|
+      expect(::DesignManagement::NewVersionWorker)
+        .to receive(:perform_async).once.with(Integer)
+      action.call
+    end
+
+    supports_block_expectations
   end
 
   def run_service(files_to_upload = nil)
@@ -98,11 +110,10 @@ describe DesignManagement::SaveDesignsService do
         it 'creates a commit in the repository' do
           run_service
 
-          commit = design_repository.commit # Get the HEAD
-
-          expect(commit).not_to be_nil
-          expect(commit.author).to eq(user)
-          expect(commit.message).to include(rails_sample_name)
+          expect(design_repository.commit).to have_attributes(
+            author: user,
+            message: include(rails_sample_name)
+          )
         end
 
         it 'causes diff_refs not to be nil' do
@@ -215,6 +226,10 @@ describe DesignManagement::SaveDesignsService do
 
             expect { run_service }.to change { commit_count.call }.by(1)
           end
+
+          it 'enqueues just one new version worker' do
+            expect { run_service }.to enqueue_worker
+          end
         end
 
         context 'when uploading multiple files' do
@@ -235,6 +250,10 @@ describe DesignManagement::SaveDesignsService do
             expect { run_service }.to change { counter.read(:create) }.by 2
           end
 
+          it 'enqueues a new version worker' do
+            expect { run_service }.to enqueue_worker
+          end
+
           it 'creates a single commit' do
             commit_count = -> do
               design_repository.expire_all_method_caches

ee/spec/services/system_note_service_spec.rb

@@ -6,6 +6,7 @@ describe SystemNoteService do
   include ProjectForksHelper
   include Gitlab::Routing
   include RepoHelpers
+  include DesignManagementTestHelpers
 
   set(:group)    { create(:group) }
   set(:project)  { create(:project, :repository, group: group) }
@@ -70,6 +71,91 @@ describe SystemNoteService do
     end
   end
 
+  describe '.design_version_added' 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) }
+    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
+
+    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(:design_version_path, version) }
+        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 '.approve_mr' do
     let(:noteable) { create(:merge_request, source_project: project) }
     subject { described_class.approve_mr(noteable, author) }
@@ -191,7 +277,6 @@ describe SystemNoteService do
 
   describe '.epic_issue' do
     let(:noteable) { epic }
-    let(:project) { nil }
 
     context 'issue added to an epic' do
       subject { described_class.epic_issue(epic, issue, author, :added) }

ee/spec/workers/design_management/new_version_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DesignManagement::NewVersionWorker do
+  describe '#perform' do
+    let(:worker) { described_class.new }
+
+    context 'the id is wrong or out-of-date' do
+      let(:version_id) { -1 }
+
+      it 'does not create system notes' do
+        expect(SystemNoteService).not_to receive(:design_version_added)
+
+        worker.perform(version_id)
+      end
+
+      it 'logs the reason for this failure' do
+        expect(Sidekiq.logger).to receive(:warn)
+          .with(an_instance_of(ActiveRecord::RecordNotFound))
+
+        worker.perform(version_id)
+      end
+    end
+
+    context 'the version id is valid' do
+      set(:version) { create(:design_version, :committed, designs_count: 2) }
+
+      it 'creates a system note' do
+        expect { worker.perform(version.id) }.to change { Note.system.count }.by(1)
+      end
+
+      it 'does not log anything' do
+        expect(Sidekiq.logger).not_to receive(:warn)
+
+        worker.perform(version.id)
+      end
+    end
+
+    context 'the version includes multiple types of action' do
+      set(:version) do
+        create(:design_version, :committed,
+               created_designs: create_list(:design, 1),
+               modified_designs: create_list(:design, 1))
+      end
+
+      it 'creates two system notes' do
+        expect { worker.perform(version.id) }.to change { Note.system.count }.by(2)
+      end
+
+      it 'calls design_version_added' do
+        expect(SystemNoteService).to receive(:design_version_added).with(version)
+
+        worker.perform(version.id)
+      end
+    end
+  end
+end