Merge branch '229918-fix-some-issue-events' into 'master'

Fix usage data tracking for some issue events

See merge request gitlab-org/gitlab!49571

app/models/resource_label_event.rb

@@ -15,7 +15,6 @@ class ResourceLabelEvent < ResourceEvent
   validate :exactly_one_issuable
 
   after_save :expire_etag_cache
-  after_save :usage_metrics
   after_destroy :expire_etag_cache
 
   enum action: {
@@ -114,16 +113,6 @@ class ResourceLabelEvent < ResourceEvent
   def discussion_id_key
     [self.class.name, created_at, user_id]
   end
-
-  def for_issue?
-    issue_id.present?
-  end
-
-  def usage_metrics
-    return unless for_issue?
-
-    Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_label_changed_action(author: user)
-  end
 end
 
 ResourceLabelEvent.prepend_if_ee('EE::ResourceLabelEvent')

app/models/resource_state_event.rb

@@ -11,7 +11,7 @@ class ResourceStateEvent < ResourceEvent
   # state is used for issue and merge request states.
   enum state: Issue.available_states.merge(MergeRequest.available_states).merge(reopened: 5)
 
-  after_save :usage_metrics
+  after_create :issue_usage_metrics
 
   def self.issuable_attrs
     %i(issue merge_request).freeze
@@ -27,7 +27,7 @@ class ResourceStateEvent < ResourceEvent
 
   private
 
-  def usage_metrics
+  def issue_usage_metrics
     return unless for_issue?
 
     case state

app/models/resource_timebox_event.rb

@@ -13,7 +13,7 @@ class ResourceTimeboxEvent < ResourceEvent
     remove: 2
   }
 
-  after_save :usage_metrics
+  after_create :issue_usage_metrics
 
   def self.issuable_attrs
     %i(issue merge_request).freeze
@@ -25,7 +25,13 @@ class ResourceTimeboxEvent < ResourceEvent
 
   private
 
-  def usage_metrics
+  def for_issue?
+    issue_id.present?
+  end
+
+  def issue_usage_metrics
+    return unless for_issue?
+
     case self
     when ResourceMilestoneEvent
       Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_milestone_changed_action(author: user)

app/services/resource_events/change_labels_service.rb

@@ -24,6 +24,8 @@ module ResourceEvents
 
       Gitlab::Database.bulk_insert(ResourceLabelEvent.table_name, labels) # rubocop:disable Gitlab/BulkInsert
       resource.expire_note_etag_cache
+
+      Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_label_changed_action(author: user) if resource.is_a?(Issue)
     end
 
     private

changelogs/unreleased/229918-fix-some-issue-events.yml

+---
+title: Fix usage data tracking of some issue events
+merge_request: 49571
+author:
+type: fixed

ee/app/models/ee/resource_timebox_event.rb

@@ -6,8 +6,10 @@ module EE
 
     private
 
-    override :usage_metrics
-    def usage_metrics
+    override :issue_usage_metrics
+    def issue_usage_metrics
+      return unless for_issue?
+
       case self
       when ResourceIterationEvent
         ::Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_iteration_changed_action(author: user)

ee/app/models/resource_weight_event.rb

@@ -4,12 +4,4 @@ class ResourceWeightEvent < ResourceEvent
   include IssueResourceEvent
 
   validates :issue, presence: true
-
-  after_save :usage_metrics
-
-  private
-
-  def usage_metrics
-    Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_weight_changed_action(author: user)
-  end
 end

ee/app/services/ee/issuable/common_system_notes_service.rb

@@ -42,7 +42,7 @@ module EE
       def handle_weight_change
         return unless issuable.previous_changes.include?('weight')
 
-        ::ResourceEvents::ChangeWeightService.new([issuable], current_user, Time.current).execute
+        ::ResourceEvents::ChangeWeightService.new(issuable, current_user, Time.current).execute
       end
 
       def handle_health_status_change

ee/app/services/ee/resource_events/change_weight_service.rb

-# frozen_string_literal: true
-
-module EE
-  module ResourceEvents
-    class ChangeWeightService
-      attr_reader :resources, :user, :event_created_at
-
-      def initialize(resources, user, created_at)
-        @resources = resources
-        @user = user
-        @event_created_at = created_at
-      end
-
-      def execute
-        unless resource_weight_changes.empty?
-          ::Gitlab::Database.bulk_insert(ResourceWeightEvent.table_name, resource_weight_changes) # rubocop:disable Gitlab/BulkInsert
-          resources.each(&:expire_note_etag_cache)
-        end
-      end
-
-      private
-
-      def resource_weight_changes
-        @weight_changes ||= resources.map do |resource|
-          changes = []
-          base_data = { user_id: user.id, issue_id: resource.id }
-
-          changes << base_data.merge({ weight: resource.previous_weight, created_at: resource.previous_updated_at }) if resource.first_weight_event?
-          changes << base_data.merge({ weight: resource.weight, created_at: event_created_at })
-        end.flatten
-      end
-    end
-  end
-end

ee/app/services/resource_events/change_weight_service.rb

@@ -2,31 +2,31 @@
 
 module ResourceEvents
   class ChangeWeightService
-    attr_reader :resources, :user, :event_created_at
+    attr_reader :resource, :user, :event_created_at
 
-    def initialize(resources, user, created_at)
-      @resources = resources
+    def initialize(resource, user, created_at)
+      @resource = resource
       @user = user
       @event_created_at = created_at
     end
 
     def execute
-      unless resource_weight_changes.empty?
-        ::Gitlab::Database.bulk_insert(ResourceWeightEvent.table_name, resource_weight_changes) # rubocop:disable Gitlab/BulkInsert
-        resources.each(&:expire_note_etag_cache)
-      end
+      ::Gitlab::Database.bulk_insert(ResourceWeightEvent.table_name, resource_weight_changes) # rubocop:disable Gitlab/BulkInsert
+      resource.expire_note_etag_cache
+
+      Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_weight_changed_action(author: user)
     end
 
     private
 
     def resource_weight_changes
-      @weight_changes ||= resources.map do |resource|
-        changes = []
-        base_data = { user_id: user.id, issue_id: resource.id }
+      changes = []
+      base_data = { user_id: user.id, issue_id: resource.id }
+
+      changes << base_data.merge({ weight: resource.previous_weight, created_at: resource.previous_updated_at }) if resource.first_weight_event?
+      changes << base_data.merge({ weight: resource.weight, created_at: event_created_at })
 
-        changes << base_data.merge({ weight: resource.previous_weight, created_at: resource.previous_updated_at }) if resource.first_weight_event?
-        changes << base_data.merge({ weight: resource.weight, created_at: event_created_at })
-      end.flatten
+      changes
     end
   end
 end

ee/spec/models/resource_weight_event_spec.rb

@@ -73,14 +73,4 @@ RSpec.describe ResourceWeightEvent, type: :model do
       expect(event.discussion_id).to eq('73d167c478')
     end
   end
-
-  context 'callbacks' do
-    describe '#usage_metrics' do
-      it 'tracks changed weights' do
-        expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_weight_changed_action).with(author: user1)
-
-        create(:resource_weight_event, issue: issue1, user: user1)
-      end
-    end
-  end
 end

ee/spec/services/resource_events/change_weight_service_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe ResourceEvents::ChangeWeightService do
   let(:issue) { create(:issue, weight: 3) }
   let(:created_at_time) { Time.utc(2019, 1, 1, 12, 30, 48, '123.123'.to_r) }
 
-  subject { described_class.new([issue], user, created_at_time).execute }
+  subject { described_class.new(issue, user, created_at_time).execute }
 
   before do
     ResourceWeightEvent.new(issue: issue, user: user).save!
@@ -51,31 +51,16 @@ RSpec.describe ResourceEvents::ChangeWeightService do
     end
   end
 
+  it 'tracks issue usage data counters' do
+    expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_weight_changed_action).with(author: user)
+
+    subject
+  end
+
   def expect_event_record(record, weight:, created_at:)
     expect(record.issue).to eq(issue)
     expect(record.user).to eq(user)
     expect(record.weight).to eq(weight)
     expect(record.created_at).to be_like_time(created_at)
   end
-
-  describe 'bulk issue weight updates' do
-    let(:issues) { create_list(:issue, 3, weight: 1) }
-
-    before do
-      issues.each { |issue| issue.update!(weight: 3) }
-    end
-
-    it 'bulk insert weight changes' do
-      expect do
-        described_class.new(issues, user, created_at_time).execute
-      end.to change { ResourceWeightEvent.count }.by(6)
-    end
-
-    it 'calls first_weight_event? once per resource' do
-      service = described_class.new(issues, user, created_at_time)
-      allow(service).to receive(:first_weight_event?).exactly(3).times
-
-      service.execute
-    end
-  end
 end

spec/models/resource_label_event_spec.rb

@@ -51,14 +51,6 @@ RSpec.describe ResourceLabelEvent, type: :model do
   end
 
   context 'callbacks' do
-    describe '#usage_metrics' do
-      it 'tracks changed labels' do
-        expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_label_changed_action)
-
-        subject.save!
-      end
-    end
-
     describe '#expire_etag_cache' do
       def expect_expiration(issue)
         expect_next_instance_of(Gitlab::EtagCaching::Store) do |instance|

spec/models/resource_state_event_spec.rb

@@ -41,7 +41,7 @@ RSpec.describe ResourceStateEvent, type: :model do
   end
 
   context 'callbacks' do
-    describe '#usage_metrics' do
+    describe '#issue_usage_metrics' do
       it 'tracks closed issues' do
         expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_closed_action)
 
@@ -53,6 +53,12 @@ RSpec.describe ResourceStateEvent, type: :model do
 
         create(described_class.name.underscore.to_sym, issue: issue, state: described_class.states[:reopened])
       end
+
+      it 'does not track merge requests' do
+        expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_closed_action)
+
+        create(described_class.name.underscore.to_sym, merge_request: merge_request, state: described_class.states[:closed])
+      end
     end
   end
 end

spec/services/resource_events/change_labels_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe ResourceEvents::ChangeLabelsService do
   describe '.change_labels' do
     subject { described_class.new(resource, author).execute(added_labels: added, removed_labels: removed) }
 
-    let(:labels) { create_list(:label, 2, project: project) }
+    let_it_be(:labels) { create_list(:label, 2, project: project) }
 
     def expect_label_event(event, label, action)
       expect(event.user).to eq(author)
@@ -57,5 +57,28 @@ RSpec.describe ResourceEvents::ChangeLabelsService do
         expect { subject }.to change { resource.resource_label_events.count }.from(0).to(2)
       end
     end
+
+    describe 'usage data' do
+      let(:added)   { [labels[0]] }
+      let(:removed) { [labels[1]] }
+
+      context 'when resource is an issue' do
+        it 'tracks changed labels' do
+          expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_label_changed_action)
+
+          subject
+        end
+      end
+
+      context 'when resource is a merge request' do
+        let(:resource) { create(:merge_request, source_project: project) }
+
+        it 'does not track changed labels' do
+          expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_label_changed_action)
+
+          subject
+        end
+      end
+    end
   end
 end

spec/support/shared_examples/models/resource_timebox_event_shared_examples.rb

@@ -75,11 +75,17 @@ RSpec.shared_examples 'timebox resource event actions' do
 end
 
 RSpec.shared_examples 'timebox resource tracks issue metrics' do |type|
-  describe '#usage_metrics' do
-    it 'tracks usage' do
+  describe '#issue_usage_metrics' do
+    it 'tracks usage for issues' do
       expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:"track_issue_#{type}_changed_action")
 
       create(described_class.name.underscore.to_sym, issue: create(:issue))
     end
+
+    it 'does not track usage for merge requests' do
+      expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:"track_issue_#{type}_changed_action")
+
+      create(described_class.name.underscore.to_sym, merge_request: create(:merge_request))
+    end
   end
 end