Merge branch 'ajk-design-activity-b' into 'master'

Add support for event creation on designs

See merge request gitlab-org/gitlab!33449

app/models/design_management/design.rb

@@ -20,6 +20,8 @@ module DesignManagement
     has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
     has_many :user_mentions, class_name: 'DesignUserMention', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
+    has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+
     validates :project, :filename, presence: true
     validates :issue, presence: true, unless: :importing?
     validates :filename, uniqueness: { scope: :issue_id }, length: { maximum: 255 }
@@ -189,6 +191,11 @@ module DesignManagement
     alias_method :after_note_created,   :after_note_changed
     alias_method :after_note_destroyed, :after_note_changed
 
+    # Part of the interface of objects we can create events about
+    def resource_parent
+      project
+    end
+
     private
 
     def head_version

app/services/design_management/delete_designs_service.rb

@@ -15,6 +15,7 @@ module DesignManagement
       return error('Forbidden!') unless can_delete_designs?
 
       version = delete_designs!
+      EventCreateService.new.destroy_designs(designs, current_user)
 
       success(version: version)
     end
@@ -48,7 +49,9 @@ module DesignManagement
     end
 
     def design_action(design)
-      on_success { counter.count(:delete) }
+      on_success do
+        counter.count(:delete)
+      end
 
       DesignManagement::DesignAction.new(design, :delete)
     end

app/services/design_management/save_designs_service.rb

@@ -20,6 +20,7 @@ module DesignManagement
       uploaded_designs, version = upload_designs!
       skipped_designs = designs - uploaded_designs
 
+      create_events
       success({ designs: uploaded_designs, version: version, skipped_designs: skipped_designs })
     rescue ::ActiveRecord::RecordInvalid => e
       error(e.message)
@@ -47,7 +48,7 @@ module DesignManagement
     end
 
     def build_actions
-      files.zip(designs).flat_map do |(file, design)|
+      @actions ||= files.zip(designs).flat_map do |(file, design)|
         Array.wrap(build_design_action(file, design))
       end
     end
@@ -57,7 +58,9 @@ module DesignManagement
       return if design_unchanged?(design, content)
 
       action = new_file?(design) ? :create : :update
-      on_success { ::Gitlab::UsageDataCounters::DesignsCounter.count(action) }
+      on_success do
+        ::Gitlab::UsageDataCounters::DesignsCounter.count(action)
+      end
 
       DesignManagement::DesignAction.new(design, action, content)
     end
@@ -67,6 +70,16 @@ module DesignManagement
       content == existing_blobs[design]&.data
     end
 
+    def create_events
+      by_action = @actions.group_by(&:action).transform_values { |grp| grp.map(&:design) }
+
+      event_create_service.save_designs(current_user, **by_action)
+    end
+
+    def event_create_service
+      @event_create_service ||= EventCreateService.new
+    end
+
     def commit_message
       <<~MSG
       Updated #{files.size} #{'designs'.pluralize(files.size)}

app/services/event_create_service.rb

@@ -96,6 +96,29 @@ class EventCreateService
     create_push_event(BulkPushEventPayloadService, project, current_user, push_data)
   end
 
+  def save_designs(current_user, create: [], update: [])
+    created = create.group_by(&:project).flat_map do |project, designs|
+      Feature.enabled?(:design_activity_events, project) ? designs : []
+    end.to_set
+    updated = update.group_by(&:project).flat_map do |project, designs|
+      Feature.enabled?(:design_activity_events, project) ? designs : []
+    end.to_set
+    return [] if created.empty? && updated.empty?
+
+    records = created.zip([:created].cycle) + updated.zip([:updated].cycle)
+
+    create_record_events(records, current_user)
+  end
+
+  def destroy_designs(designs, current_user)
+    designs = designs.select do |design|
+      Feature.enabled?(:design_activity_events, design.project)
+    end
+    return [] unless designs.present?
+
+    create_record_events(designs.zip([:destroyed].cycle), current_user)
+  end
+
   # Create a new wiki page event
   #
   # @param [WikiPage::Meta] wiki_page_meta The event target
@@ -134,7 +157,32 @@ class EventCreateService
   end
 
   def create_record_event(record, current_user, status)
-    create_event(record.resource_parent, current_user, status, target_id: record.id, target_type: record.class.name)
+    create_event(record.resource_parent, current_user, status,
+                 target_id: record.id, target_type: record.class.name)
+  end
+
+  # If creating several events, this method will insert them all in a single
+  # statement
+  #
+  # @param [[Eventable, Symbol]] a list of pairs of records and a valid status
+  # @param [User] the author of the event
+  def create_record_events(pairs, current_user)
+    base_attrs = {
+      created_at: Time.now.utc,
+      updated_at: Time.now.utc,
+      author_id: current_user.id
+    }
+
+    attribute_sets = pairs.map do |record, status|
+      action = Event.actions[status]
+      raise IllegalActionError, "#{status} is not a valid status" if action.nil?
+
+      parent_attrs(record.resource_parent)
+        .merge(base_attrs)
+        .merge(action: action, target_id: record.id, target_type: record.class.name)
+    end
+
+    Event.insert_all(attribute_sets, returning: %w[id])
   end
 
   def create_push_event(service_class, project, current_user, push_data)
@@ -160,16 +208,22 @@ class EventCreateService
       action: status,
       author_id: current_user.id
     )
+    attributes.merge!(parent_attrs(resource_parent))
 
+    Event.create!(attributes)
+  end
+
+  def parent_attrs(resource_parent)
     resource_parent_attr = case resource_parent
                            when Project
-                             :project
+                             :project_id
                            when Group
-                             :group
+                             :group_id
                            end
-    attributes[resource_parent_attr] = resource_parent if resource_parent_attr
 
-    Event.create!(attributes)
+    return {} unless resource_parent_attr
+
+    { resource_parent_attr => resource_parent.id }
   end
 
   def create_resource_event(issuable, current_user, status)

spec/lib/gitlab/import_export/all_models.yml

@@ -599,6 +599,7 @@ design: &design
 - versions
 - notes
 - user_mentions
+- events
 designs: *design
 actions:
 - design

spec/services/design_management/delete_designs_service_spec.rb

@@ -56,6 +56,10 @@ describe DesignManagement::DeleteDesignsService do
       let(:enabled) { false }
 
       it_behaves_like "a service error"
+
+      it 'does not create any events in the activity stream' do
+        expect { run_service rescue nil }.not_to change { Event.count }
+      end
     end
 
     context "when the feature is available" do
@@ -72,7 +76,9 @@ describe DesignManagement::DeleteDesignsService do
 
         it 'does not log any events' do
           counter = ::Gitlab::UsageDataCounters::DesignsCounter
-          expect { run_service rescue nil }.not_to change { counter.totals }
+
+          expect { run_service rescue nil }
+            .not_to change { [counter.totals, Event.count] }
         end
       end
 
@@ -92,6 +98,12 @@ describe DesignManagement::DeleteDesignsService do
           expect { run_service }.to change { counter.read(:delete) }.by(1)
         end
 
+        it 'creates an event in the activity stream' do
+          expect { run_service }
+            .to change { Event.count }.by(1)
+            .and change { Event.destroyed_action.for_design.count }.by(1)
+        end
+
         it 'informs the new-version-worker' do
           expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer)
 
@@ -129,14 +141,14 @@ describe DesignManagement::DeleteDesignsService do
 
         let!(:designs) { create_designs(2) }
 
-        it 'removes those designs' do
+        it 'makes the correct changes' do
+          counter = ::Gitlab::UsageDataCounters::DesignsCounter
+
           expect { run_service }
             .to change { issue.designs.current.count }.from(3).to(1)
-        end
-
-        it 'logs the correct number of deletion events' do
-          counter = ::Gitlab::UsageDataCounters::DesignsCounter
-          expect { run_service }.to change { counter.read(:delete) }.by(2)
+            .and change { counter.read(:delete) }.by(2)
+            .and change { Event.count }.by(2)
+            .and change { Event.destroyed_action.for_design.count }.by(2)
         end
 
         it_behaves_like "a success"

spec/services/design_management/save_designs_service_spec.rb

@@ -65,6 +65,10 @@ describe DesignManagement::SaveDesignsService do
       end
 
       it_behaves_like 'a service error'
+
+      it 'does not create an event in the activity stream' do
+        expect { run_service }.not_to change { Event.count }
+      end
     end
 
     context 'when the feature is available' do
@@ -89,6 +93,12 @@ describe DesignManagement::SaveDesignsService do
         expect { run_service }.to change { counter.read(:create) }.by(1)
       end
 
+      it 'creates an event in the activity stream' do
+        expect { run_service }
+          .to change { Event.count }.by(1)
+          .and change { Event.for_design.created_action.count }.by(1)
+      end
+
       it 'creates a commit in the repository' do
         run_service
 
@@ -166,9 +176,12 @@ describe DesignManagement::SaveDesignsService do
           expect(updated_designs.first.versions.size).to eq(2)
         end
 
-        it 'increments the update counter' do
+        it 'records the correct events' do
           counter = Gitlab::UsageDataCounters::DesignsCounter
-          expect { run_service }.to change { counter.read(:update) }.by 1
+          expect { run_service }
+            .to change { counter.read(:update) }.by(1)
+            .and change { Event.count }.by(1)
+            .and change { Event.for_design.updated_action.count }.by(1)
         end
 
         context 'when uploading a new design' do
@@ -217,6 +230,14 @@ describe DesignManagement::SaveDesignsService do
             .and change { counter.read(:update) }.by(1)
         end
 
+        it 'creates the correct activity stream events' do
+          expect { run_service }
+            .to change { Event.count }.by(2)
+            .and change { Event.for_design.count }.by(2)
+            .and change { Event.created_action.count }.by(1)
+            .and change { Event.updated_action.count }.by(1)
+        end
+
         it 'creates a single commit' do
           commit_count = -> do
             design_repository.expire_all_method_caches

spec/services/event_create_service_spec.rb

@@ -5,6 +5,9 @@ require 'spec_helper'
 describe EventCreateService do
   let(:service) { described_class.new }
 
+  let_it_be(:user, reload: true) { create :user }
+  let_it_be(:project) { create(:project) }
+
   describe 'Issues' do
     describe '#open_issue' do
       let(:issue) { create(:issue) }
@@ -87,8 +90,6 @@ describe EventCreateService do
   end
 
   describe 'Milestone' do
-    let(:user) { create :user }
-
     describe '#open_milestone' do
       let(:milestone) { create(:milestone) }
 
@@ -210,9 +211,6 @@ describe EventCreateService do
   end
 
   describe '#push', :clean_gitlab_redis_shared_state do
-    let(:project) { create(:project) }
-    let(:user) { create(:user) }
-
     let(:push_data) do
       {
         commits: [
@@ -234,9 +232,6 @@ describe EventCreateService do
   end
 
   describe '#bulk_push', :clean_gitlab_redis_shared_state do
-    let(:project) { create(:project) }
-    let(:user) { create(:user) }
-
     let(:push_data) do
       {
         action: :created,
@@ -251,9 +246,6 @@ describe EventCreateService do
   end
 
   describe 'Project' do
-    let(:user) { create :user }
-    let(:project) { create(:project) }
-
     describe '#join_project' do
       subject { service.join_project(project, user) }
 
@@ -268,4 +260,81 @@ describe EventCreateService do
       it { expect { subject }.to change { Event.count }.from(0).to(1) }
     end
   end
+
+  describe 'design events' do
+    let_it_be(:design) { create(:design, project: project) }
+    let_it_be(:author) { user }
+
+    shared_examples 'feature flag gated multiple event creation' do
+      context 'the feature flag is off' do
+        before do
+          stub_feature_flags(design_activity_events: false)
+        end
+
+        specify { expect(result).to be_empty }
+        specify { expect { result }.not_to change { Event.count } }
+        specify { expect { result }.not_to exceed_query_limit(0) }
+      end
+
+      context 'the feature flag is enabled for a single project' do
+        before do
+          stub_feature_flags(design_activity_events: project)
+        end
+
+        specify { expect(result).not_to be_empty }
+        specify { expect { result }.to change { Event.count }.by(1) }
+      end
+    end
+
+    describe '#save_designs' do
+      let_it_be(:updated) { create_list(:design, 5) }
+      let_it_be(:created) { create_list(:design, 3) }
+
+      let(:result) { service.save_designs(author, create: created, update: updated) }
+
+      specify { expect { result }.to change { Event.count }.by(8) }
+
+      specify { expect { result }.not_to exceed_query_limit(1) }
+
+      it 'creates 3 created design events' do
+        ids = result.pluck('id')
+        events = Event.created_action.where(id: ids)
+
+        expect(events.map(&:design)).to match_array(created)
+      end
+
+      it 'creates 5 created design events' do
+        ids = result.pluck('id')
+        events = Event.updated_action.where(id: ids)
+
+        expect(events.map(&:design)).to match_array(updated)
+      end
+
+      it_behaves_like 'feature flag gated multiple event creation' do
+        let(:project) { created.first.project }
+      end
+    end
+
+    describe '#destroy_designs' do
+      let_it_be(:designs) { create_list(:design, 5) }
+      let_it_be(:author) { create(:user) }
+
+      let(:result) { service.destroy_designs(designs, author) }
+
+      specify { expect { result }.to change { Event.count }.by(5) }
+
+      specify { expect { result }.not_to exceed_query_limit(1) }
+
+      it 'creates 5 destroyed design events' do
+        ids = result.pluck('id')
+        events = Event.destroyed_action.where(id: ids)
+
+        expect(events.map(&:design)).to match_array(designs)
+      end
+
+      it_behaves_like 'feature flag gated multiple event creation' do
+        let(:project) { designs.first.project }
+      end
+    end
+  end
 end