Merge branch '225555-update-usage-ping-data-to-track-usage-of-diffnotes-2' into 'master'

Resolve "Update usage ping data to track usage of DiffNotes"

See merge request gitlab-org/gitlab!40391

app/services/event_create_service.rb

@@ -23,11 +23,15 @@ class EventCreateService
   end
 
   def open_mr(merge_request, current_user)
-    create_record_event(merge_request, current_user, :created)
+    create_record_event(merge_request, current_user, :created).tap do
+      track_event(event_action: :created, event_target: MergeRequest, author_id: current_user.id)
+    end
   end
 
   def close_mr(merge_request, current_user)
-    create_record_event(merge_request, current_user, :closed)
+    create_record_event(merge_request, current_user, :closed).tap do
+      track_event(event_action: :closed, event_target: MergeRequest, author_id: current_user.id)
+    end
   end
 
   def reopen_mr(merge_request, current_user)
@@ -35,7 +39,9 @@ class EventCreateService
   end
 
   def merge_mr(merge_request, current_user)
-    create_record_event(merge_request, current_user, :merged)
+    create_record_event(merge_request, current_user, :merged).tap do
+      track_event(event_action: :merged, event_target: MergeRequest, author_id: current_user.id)
+    end
   end
 
   def open_milestone(milestone, current_user)
@@ -55,7 +61,11 @@ class EventCreateService
   end
 
   def leave_note(note, current_user)
-    create_record_event(note, current_user, :commented)
+    create_record_event(note, current_user, :commented).tap do
+      if note.is_a?(DiffNote) && note.for_merge_request?
+        track_event(event_action: :commented, event_target: MergeRequest, author_id: current_user.id)
+      end
+    end
   end
 
   def join_project(project, current_user)
@@ -109,7 +119,7 @@ class EventCreateService
   def wiki_event(wiki_page_meta, author, action, fingerprint)
     raise IllegalActionError, action unless Event::WIKI_ACTIONS.include?(action)
 
-    Gitlab::UsageDataCounters::TrackUniqueEvents.track_event(event_action: action, event_target: wiki_page_meta.class, author_id: author.id)
+    track_event(event_action: action, event_target: wiki_page_meta.class, author_id: author.id)
 
     duplicate = Event.for_wiki_meta(wiki_page_meta).for_fingerprint(fingerprint).first
     return duplicate if duplicate.present?
@@ -154,7 +164,7 @@ class EventCreateService
     result = Event.insert_all(attribute_sets, returning: %w[id])
 
     tuples.each do |record, status, _|
-      Gitlab::UsageDataCounters::TrackUniqueEvents.track_event(event_action: status, event_target: record.class, author_id: current_user.id)
+      track_event(event_action: status, event_target: record.class, author_id: current_user.id)
     end
 
     result
@@ -172,7 +182,7 @@ class EventCreateService
       new_event
     end
 
-    Gitlab::UsageDataCounters::TrackUniqueEvents.track_event(event_action: :pushed, event_target: Project, author_id: current_user.id)
+    track_event(event_action: :pushed, event_target: Project, author_id: current_user.id)
 
     Users::LastPushEventService.new(current_user)
       .cache_last_push_event(event)
@@ -206,6 +216,10 @@ class EventCreateService
 
     { resource_parent_attr => resource_parent.id }
   end
+
+  def track_event(**params)
+    Gitlab::UsageDataCounters::TrackUniqueEvents.track_event(**params)
+  end
 end
 
 EventCreateService.prepend_if_ee('EE::EventCreateService')

changelogs/unreleased/225555-update-usage-ping-data-to-track-usage-of-diffnotes-2.yml

+---
+title: Add merge request usage to usage data
+merge_request: 40391
+author:
+type: other

lib/gitlab/usage_data.rb

@@ -426,16 +426,17 @@ module Gitlab
         {} # augmented in EE
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def merge_requests_users(time_period)
-        distinct_count(
-          Event.where(target_type: Event::TARGET_TYPES[:merge_request].to_s).where(time_period),
-          :author_id,
-          start: user_minimum_id,
-          finish: user_maximum_id
-        )
+        counter = Gitlab::UsageDataCounters::TrackUniqueEvents
+
+        redis_usage_data do
+          counter.count_unique_events(
+            event_action: Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION,
+            date_from: time_period[:created_at].first,
+            date_to: time_period[:created_at].last
+          )
+        end
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       def installation_type
         if Rails.env.production?

lib/gitlab/usage_data_counters/track_unique_events.rb

@@ -6,6 +6,7 @@ module Gitlab
       WIKI_ACTION = :wiki_action
       DESIGN_ACTION = :design_action
       PUSH_ACTION = :project_action
+      MERGE_REQUEST_ACTION = :merge_request_action
 
       ACTION_TRANSFORMATIONS = HashWithIndifferentAccess.new({
         wiki: {
@@ -20,6 +21,12 @@ module Gitlab
         },
         project: {
           pushed: PUSH_ACTION
+        },
+        merge_request: {
+          closed: MERGE_REQUEST_ACTION,
+          merged: MERGE_REQUEST_ACTION,
+          created: MERGE_REQUEST_ACTION,
+          commented: MERGE_REQUEST_ACTION
         }
       }).freeze
 

spec/lib/gitlab/usage_data_spec.rb

@@ -959,24 +959,25 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
     end
   end
 
-  describe '.merge_requests_users' do
-    let(:time_period) { { created_at: 2.days.ago..Time.current } }
-    let(:merge_request) { create(:merge_request) }
-    let(:other_user) { create(:user) }
-    let(:another_user) { create(:user) }
+  describe '.merge_requests_users', :clean_gitlab_redis_shared_state do
+    let(:time_period) { { created_at: 2.days.ago..time } }
+    let(:time) { Time.current }
 
     before do
-      create(:event, target: merge_request, author: merge_request.author, created_at: 1.day.ago)
-      create(:event, target: merge_request, author: merge_request.author, created_at: 1.hour.ago)
-      create(:event, target: merge_request, author: merge_request.author, created_at: 3.days.ago)
-      create(:event, target: merge_request, author: other_user, created_at: 1.day.ago)
-      create(:event, target: merge_request, author: other_user, created_at: 1.hour.ago)
-      create(:event, target: merge_request, author: other_user, created_at: 3.days.ago)
-      create(:event, target: merge_request, author: another_user, created_at: 4.days.ago)
+      counter = Gitlab::UsageDataCounters::TrackUniqueEvents
+      merge_request = Event::TARGET_TYPES[:merge_request]
+      project = Event::TARGET_TYPES[:project]
+
+      counter.track_event(event_action: :commented, event_target: merge_request, author_id: 1, time: time)
+      counter.track_event(event_action: :opened, event_target: merge_request, author_id: 1, time: time)
+      counter.track_event(event_action: :merged, event_target: merge_request, author_id: 2, time: time)
+      counter.track_event(event_action: :closed, event_target: merge_request, author_id: 3, time: time)
+      counter.track_event(event_action: :opened, event_target: merge_request, author_id: 4, time: time - 3.days)
+      counter.track_event(event_action: :created, event_target: project, author_id: 5, time: time)
     end
 
     it 'returns the distinct count of users using merge requests (via events table) within the specified time period' do
-      expect(described_class.merge_requests_users(time_period)).to eq(2)
+      expect(described_class.merge_requests_users(time_period)).to eq(3)
     end
   end
 

spec/services/event_create_service_spec.rb

@@ -8,6 +8,16 @@ RSpec.describe EventCreateService do
   let_it_be(:user, reload: true) { create :user }
   let_it_be(:project) { create(:project) }
 
+  shared_examples 'it records the event in the event counter' do
+    specify do
+      tracking_params = { event_action: event_action, date_from: Date.yesterday, date_to: Date.today }
+
+      expect { subject }
+        .to change { Gitlab::UsageDataCounters::TrackUniqueEvents.count_unique_events(tracking_params) }
+        .by(1)
+    end
+  end
+
   describe 'Issues' do
     describe '#open_issue' do
       let(:issue) { create(:issue) }
@@ -40,34 +50,52 @@ RSpec.describe EventCreateService do
     end
   end
 
-  describe 'Merge Requests' do
+  describe 'Merge Requests', :clean_gitlab_redis_shared_state do
     describe '#open_mr' do
+      subject(:open_mr) { service.open_mr(merge_request, merge_request.author) }
+
       let(:merge_request) { create(:merge_request) }
 
-      it { expect(service.open_mr(merge_request, merge_request.author)).to be_truthy }
+      it { expect(open_mr).to be_truthy }
 
       it "creates new event" do
-        expect { service.open_mr(merge_request, merge_request.author) }.to change { Event.count }
+        expect { open_mr }.to change { Event.count }
+      end
+
+      it_behaves_like "it records the event in the event counter" do
+        let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION }
       end
     end
 
     describe '#close_mr' do
+      subject(:close_mr) { service.close_mr(merge_request, merge_request.author) }
+
       let(:merge_request) { create(:merge_request) }
 
-      it { expect(service.close_mr(merge_request, merge_request.author)).to be_truthy }
+      it { expect(close_mr).to be_truthy }
 
       it "creates new event" do
-        expect { service.close_mr(merge_request, merge_request.author) }.to change { Event.count }
+        expect { close_mr }.to change { Event.count }
+      end
+
+      it_behaves_like "it records the event in the event counter" do
+        let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION }
       end
     end
 
     describe '#merge_mr' do
+      subject(:merge_mr) { service.merge_mr(merge_request, merge_request.author) }
+
       let(:merge_request) { create(:merge_request) }
 
-      it { expect(service.merge_mr(merge_request, merge_request.author)).to be_truthy }
+      it { expect(merge_mr).to be_truthy }
 
       it "creates new event" do
-        expect { service.merge_mr(merge_request, merge_request.author) }.to change { Event.count }
+        expect { merge_mr }.to change { Event.count }
+      end
+
+      it_behaves_like "it records the event in the event counter" do
+        let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION }
       end
     end
 
@@ -180,6 +208,8 @@ RSpec.describe EventCreateService do
     where(:action) { Event::WIKI_ACTIONS.map { |action| [action] } }
 
     with_them do
+      subject { create_event }
+
       it 'creates the event' do
         expect(create_event).to have_attributes(
           wiki_page?: true,
@@ -201,13 +231,8 @@ RSpec.describe EventCreateService do
         expect(duplicate).to eq(event)
       end
 
-      it 'records the event in the event counter' do
-        counter_class = Gitlab::UsageDataCounters::TrackUniqueEvents
-        tracking_params = { event_action: counter_class::WIKI_ACTION, date_from: Date.yesterday, date_to: Date.today }
-
-        expect { create_event }
-          .to change { counter_class.count_unique_events(tracking_params) }
-          .by(1)
+      it_behaves_like "it records the event in the event counter" do
+        let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::WIKI_ACTION }
       end
     end
 
@@ -242,13 +267,8 @@ RSpec.describe EventCreateService do
 
     it_behaves_like 'service for creating a push event', PushEventPayloadService
 
-    it 'records the event in the event counter' do
-      counter_class = Gitlab::UsageDataCounters::TrackUniqueEvents
-      tracking_params = { event_action: counter_class::PUSH_ACTION, date_from: Date.yesterday, date_to: Date.today }
-
-      expect { subject }
-        .to change { counter_class.count_unique_events(tracking_params) }
-        .from(0).to(1)
+    it_behaves_like "it records the event in the event counter" do
+      let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::PUSH_ACTION }
     end
   end
 
@@ -265,13 +285,8 @@ RSpec.describe EventCreateService do
 
     it_behaves_like 'service for creating a push event', BulkPushEventPayloadService
 
-    it 'records the event in the event counter' do
-      counter_class = Gitlab::UsageDataCounters::TrackUniqueEvents
-      tracking_params = { event_action: counter_class::PUSH_ACTION, date_from: Date.yesterday, date_to: Date.today }
-
-      expect { subject }
-        .to change { counter_class.count_unique_events(tracking_params) }
-        .from(0).to(1)
+    it_behaves_like "it records the event in the event counter" do
+      let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::PUSH_ACTION }
     end
   end
 
@@ -299,7 +314,7 @@ RSpec.describe EventCreateService 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) }
+      subject(:result) { service.save_designs(author, create: created, update: updated) }
 
       specify { expect { result }.to change { Event.count }.by(8) }
 
@@ -319,13 +334,8 @@ RSpec.describe EventCreateService do
         expect(events.map(&:design)).to match_array(updated)
       end
 
-      it 'records the event in the event counter' do
-        counter_class = Gitlab::UsageDataCounters::TrackUniqueEvents
-        tracking_params = { event_action: counter_class::DESIGN_ACTION, date_from: Date.yesterday, date_to: Date.today }
-
-        expect { result }
-          .to change { counter_class.count_unique_events(tracking_params) }
-          .from(0).to(1)
+      it_behaves_like "it records the event in the event counter" do
+        let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION }
       end
     end
 
@@ -333,7 +343,7 @@ RSpec.describe EventCreateService do
       let_it_be(:designs) { create_list(:design, 5) }
       let_it_be(:author) { create(:user) }
 
-      let(:result) { service.destroy_designs(designs, author) }
+      subject(:result) { service.destroy_designs(designs, author) }
 
       specify { expect { result }.to change { Event.count }.by(5) }
 
@@ -346,13 +356,37 @@ RSpec.describe EventCreateService do
         expect(events.map(&:design)).to match_array(designs)
       end
 
-      it 'records the event in the event counter' do
+      it_behaves_like "it records the event in the event counter" do
+        let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION }
+      end
+    end
+  end
+
+  describe '#leave_note' do
+    subject(:leave_note) { service.leave_note(note, author) }
+
+    let(:note) { create(:note) }
+    let(:author) { create(:user) }
+    let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION }
+
+    it { expect(leave_note).to be_truthy }
+
+    it "creates new event" do
+      expect { leave_note }.to change { Event.count }.by(1)
+    end
+
+    context 'when it is a diff note' do
+      it_behaves_like "it records the event in the event counter" do
+        let(:note) { create(:diff_note_on_merge_request) }
+      end
+    end
+
+    context 'when it is not a diff note' do
+      it 'does not change the unique action counter' do
         counter_class = Gitlab::UsageDataCounters::TrackUniqueEvents
-        tracking_params = { event_action: counter_class::DESIGN_ACTION, date_from: Date.yesterday, date_to: Date.today }
+        tracking_params = { event_action: event_action, date_from: Date.yesterday, date_to: Date.today }
 
-        expect { result }
-          .to change { counter_class.count_unique_events(tracking_params) }
-          .from(0).to(1)
+        expect { subject }.not_to change { counter_class.count_unique_events(tracking_params) }
       end
     end
   end