Add Issue usagedata for add/remvoe/edit comments

Track unique MAU for comments added, removed, or edited on Issues

app/services/notes/create_service.rb

@@ -73,6 +73,8 @@ module Notes
       if note.for_merge_request? && note.diff_note? && note.start_of_discussion?
         Discussions::CaptureDiffNotePositionService.new(note.noteable, note.diff_file&.paths).execute(note.discussion)
       end
+
+      track_note_creation_usage_for_issues(note) if note.for_issue?
     end
 
     def do_commands(note, update_params, message, only_commands)
@@ -113,5 +115,9 @@ module Notes
 
       track_usage_event(:incident_management_incident_comment, user.id)
     end
+
+    def track_note_creation_usage_for_issues(note)
+      Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_comment_added_action(author: note.author)
+    end
   end
 end

app/services/notes/destroy_service.rb

@@ -8,6 +8,13 @@ module Notes
       end
 
       clear_noteable_diffs_cache(note)
+      track_note_removal_usage_for_issues(note) if note.for_issue?
+    end
+
+    private
+
+    def track_note_removal_usage_for_issues(note)
+      Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_comment_removed_action(author: note.author)
     end
   end
 end

app/services/notes/update_service.rb

@@ -14,6 +14,8 @@ module Notes
         note.save
       end
 
+      track_note_edit_usage_for_issues(note) if note.for_issue?
+
       only_commands = false
 
       quick_actions_service = QuickActionsService.new(project, current_user)
@@ -89,6 +91,10 @@ module Notes
 
       Note.id_in(note.discussion.notes.map(&:id)).update_all(confidential: params[:confidential])
     end
+
+    def track_note_edit_usage_for_issues(note)
+      Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_comment_edited_action(author: note.author)
+    end
   end
 end
 

changelogs/unreleased/229918-issuedata-comments.yml

+---
+title: UsageData for issues added/removed/edited
+merge_request: 45609
+author:
+type: added

lib/gitlab/usage_data_counters/issue_activity_unique_counter.rb

@@ -33,6 +33,9 @@ module Gitlab
       ISSUE_DUE_DATE_CHANGED = 'g_project_management_issue_due_date_changed'
       ISSUE_TIME_ESTIMATE_CHANGED = 'g_project_management_issue_time_estimate_changed'
       ISSUE_TIME_SPENT_CHANGED = 'g_project_management_issue_time_spent_changed'
+      ISSUE_COMMENT_ADDED = 'g_project_management_issue_comment_added'
+      ISSUE_COMMENT_EDITED = 'g_project_management_issue_comment_edited'
+      ISSUE_COMMENT_REMOVED = 'g_project_management_issue_comment_removed'
 
       class << self
         def track_issue_created_action(author:, time: Time.zone.now)
@@ -147,6 +150,18 @@ module Gitlab
           track_unique_action(ISSUE_TIME_SPENT_CHANGED, author, time)
         end
 
+        def track_issue_comment_added_action(author:, time: Time.zone.now)
+          track_unique_action(ISSUE_COMMENT_ADDED, author, time)
+        end
+
+        def track_issue_comment_edited_action(author:, time: Time.zone.now)
+          track_unique_action(ISSUE_COMMENT_EDITED, author, time)
+        end
+
+        def track_issue_comment_removed_action(author:, time: Time.zone.now)
+          track_unique_action(ISSUE_COMMENT_REMOVED, author, time)
+        end
+
         private
 
         def track_unique_action(action, author, time)

lib/gitlab/usage_data_counters/known_events.yml

@@ -303,3 +303,15 @@
   category: issues_edit
   redis_slot: project_management
   aggregation: daily
+- name: g_project_management_issue_comment_added
+  category: issues_edit
+  redis_slot: project_management
+  aggregation: daily
+- name: g_project_management_issue_comment_edited
+  category: issues_edit
+  redis_slot: project_management
+  aggregation: daily
+- name: g_project_management_issue_comment_removed
+  category: issues_edit
+  redis_slot: project_management
+  aggregation: daily

spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb

@@ -292,6 +292,36 @@ RSpec.describe Gitlab::UsageDataCounters::IssueActivityUniqueCounter, :clean_git
     end
   end
 
+  context 'for Issue comment added actions' do
+    it_behaves_like 'tracks and counts action' do
+      let(:action) { described_class::ISSUE_COMMENT_ADDED }
+
+      def track_action(params)
+        described_class.track_issue_comment_added_action(**params)
+      end
+    end
+  end
+
+  context 'for Issue comment edited actions' do
+    it_behaves_like 'tracks and counts action' do
+      let(:action) { described_class::ISSUE_COMMENT_EDITED }
+
+      def track_action(params)
+        described_class.track_issue_comment_edited_action(**params)
+      end
+    end
+  end
+
+  context 'for Issue comment removed actions' do
+    it_behaves_like 'tracks and counts action' do
+      let(:action) { described_class::ISSUE_COMMENT_REMOVED }
+
+      def track_action(params)
+        described_class.track_issue_comment_removed_action(**params)
+      end
+    end
+  end
+
   it 'can return the count of actions per user deduplicated', :aggregate_failures do
     described_class.track_issue_title_changed_action(author: user1)
     described_class.track_issue_description_changed_action(author: user1)

spec/services/notes/create_service_spec.rb

@@ -67,147 +67,164 @@ RSpec.describe Notes::CreateService do
           let(:current_user) { user }
         end
       end
-    end
 
-    context 'noteable highlight cache clearing' do
-      let(:project_with_repo) { create(:project, :repository) }
-      let(:merge_request) do
-        create(:merge_request, source_project: project_with_repo,
-                               target_project: project_with_repo)
-      end
+      it 'tracks issue comment usage data', :clean_gitlab_redis_shared_state do
+        event = Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_COMMENT_ADDED
+        counter = Gitlab::UsageDataCounters::HLLRedisCounter
 
-      let(:position) do
-        Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
-                                   new_path: "files/ruby/popen.rb",
-                                   old_line: nil,
-                                   new_line: 14,
-                                   diff_refs: merge_request.diff_refs)
+        expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_comment_added_action).with(author: user).and_call_original
+        expect do
+          described_class.new(project, user, opts).execute
+        end.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1)
       end
 
-      let(:new_opts) do
-        opts.merge(in_reply_to_discussion_id: nil,
-                   type: 'DiffNote',
-                   noteable_type: 'MergeRequest',
-                   noteable_id: merge_request.id,
-                   position: position.to_h)
-      end
+      context 'in a merge request' do
+        let_it_be(:project_with_repo) { create(:project, :repository) }
+        let_it_be(:merge_request) do
+          create(:merge_request, source_project: project_with_repo,
+                 target_project: project_with_repo)
+        end
 
-      before do
-        allow_any_instance_of(Gitlab::Diff::Position)
-          .to receive(:unfolded_diff?) { true }
-      end
+        context 'issue comment usage data' do
+          let(:opts) do
+            { note: 'Awesome comment', noteable_type: 'MergeRequest', noteable_id: merge_request.id }
+          end
 
-      it 'clears noteable diff cache when it was unfolded for the note position' do
-        expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear)
+          it 'does not track' do
+            expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_added_action)
 
-        described_class.new(project_with_repo, user, new_opts).execute
-      end
+            described_class.new(project, user, opts).execute
+          end
+        end
 
-      it 'does not clear cache when note is not the first of the discussion' do
-        prev_note =
-          create(:diff_note_on_merge_request, noteable: merge_request,
-                                              project: project_with_repo)
-        reply_opts =
-          opts.merge(in_reply_to_discussion_id: prev_note.discussion_id,
-                     type: 'DiffNote',
-                     noteable_type: 'MergeRequest',
-                     noteable_id: merge_request.id,
-                     position: position.to_h)
+        context 'noteable highlight cache clearing' do
+          let(:position) do
+            Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
+                                       new_path: "files/ruby/popen.rb",
+                                       old_line: nil,
+                                       new_line: 14,
+                                       diff_refs: merge_request.diff_refs)
+          end
 
-        expect(merge_request).not_to receive(:diffs)
+          let(:new_opts) do
+            opts.merge(in_reply_to_discussion_id: nil,
+                       type: 'DiffNote',
+                       noteable_type: 'MergeRequest',
+                       noteable_id: merge_request.id,
+                       position: position.to_h)
+          end
 
-        described_class.new(project_with_repo, user, reply_opts).execute
-      end
-    end
+          before do
+            allow_any_instance_of(Gitlab::Diff::Position)
+              .to receive(:unfolded_diff?) { true }
+          end
 
-    context 'note diff file' do
-      let(:project_with_repo) { create(:project, :repository) }
-      let(:merge_request) do
-        create(:merge_request,
-               source_project: project_with_repo,
-               target_project: project_with_repo)
-      end
+          it 'clears noteable diff cache when it was unfolded for the note position' do
+            expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear)
 
-      let(:line_number) { 14 }
-      let(:position) do
-        Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
-                                   new_path: "files/ruby/popen.rb",
-                                   old_line: nil,
-                                   new_line: line_number,
-                                   diff_refs: merge_request.diff_refs)
-      end
+            described_class.new(project_with_repo, user, new_opts).execute
+          end
 
-      let(:previous_note) do
-        create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo)
-      end
+          it 'does not clear cache when note is not the first of the discussion' do
+            prev_note =
+              create(:diff_note_on_merge_request, noteable: merge_request,
+                     project: project_with_repo)
+            reply_opts =
+              opts.merge(in_reply_to_discussion_id: prev_note.discussion_id,
+                         type: 'DiffNote',
+                         noteable_type: 'MergeRequest',
+                         noteable_id: merge_request.id,
+                         position: position.to_h)
 
-      before do
-        project_with_repo.add_maintainer(user)
-      end
+            expect(merge_request).not_to receive(:diffs)
 
-      context 'when eligible to have a note diff file' do
-        let(:new_opts) do
-          opts.merge(in_reply_to_discussion_id: nil,
-                     type: 'DiffNote',
-                     noteable_type: 'MergeRequest',
-                     noteable_id: merge_request.id,
-                     position: position.to_h)
+            described_class.new(project_with_repo, user, reply_opts).execute
+          end
         end
 
-        it 'note is associated with a note diff file' do
-          MergeRequests::MergeToRefService.new(merge_request.project, merge_request.author).execute(merge_request)
-
-          note = described_class.new(project_with_repo, user, new_opts).execute
-
-          expect(note).to be_persisted
-          expect(note.note_diff_file).to be_present
-          expect(note.diff_note_positions).to be_present
-        end
-      end
+        context 'note diff file' do
+          let(:line_number) { 14 }
+          let(:position) do
+            Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
+                                       new_path: "files/ruby/popen.rb",
+                                       old_line: nil,
+                                       new_line: line_number,
+                                       diff_refs: merge_request.diff_refs)
+          end
 
-      context 'when DiffNote is a reply' do
-        let(:new_opts) do
-          opts.merge(in_reply_to_discussion_id: previous_note.discussion_id,
-                     type: 'DiffNote',
-                     noteable_type: 'MergeRequest',
-                     noteable_id: merge_request.id,
-                     position: position.to_h)
-        end
+          let(:previous_note) do
+            create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo)
+          end
 
-        it 'note is not associated with a note diff file' do
-          expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new)
+          before do
+            project_with_repo.add_maintainer(user)
+          end
 
-          note = described_class.new(project_with_repo, user, new_opts).execute
+          context 'when eligible to have a note diff file' do
+            let(:new_opts) do
+              opts.merge(in_reply_to_discussion_id: nil,
+                         type: 'DiffNote',
+                         noteable_type: 'MergeRequest',
+                         noteable_id: merge_request.id,
+                         position: position.to_h)
+            end
 
-          expect(note).to be_persisted
-          expect(note.note_diff_file).to be_nil
-        end
+            it 'note is associated with a note diff file' do
+              MergeRequests::MergeToRefService.new(merge_request.project, merge_request.author).execute(merge_request)
 
-        context 'when DiffNote from an image' do
-          let(:image_position) do
-            Gitlab::Diff::Position.new(old_path: "files/images/6049019_460s.jpg",
-                                       new_path: "files/images/6049019_460s.jpg",
-                                       width: 100,
-                                       height: 100,
-                                       x: 1,
-                                       y: 100,
-                                       diff_refs: merge_request.diff_refs,
-                                       position_type: 'image')
-          end
+              note = described_class.new(project_with_repo, user, new_opts).execute
 
-          let(:new_opts) do
-            opts.merge(in_reply_to_discussion_id: nil,
-                       type: 'DiffNote',
-                       noteable_type: 'MergeRequest',
-                       noteable_id: merge_request.id,
-                       position: image_position.to_h)
+              expect(note).to be_persisted
+              expect(note.note_diff_file).to be_present
+              expect(note.diff_note_positions).to be_present
+            end
           end
 
-          it 'note is not associated with a note diff file' do
-            note = described_class.new(project_with_repo, user, new_opts).execute
-
-            expect(note).to be_persisted
-            expect(note.note_diff_file).to be_nil
+          context 'when DiffNote is a reply' do
+            let(:new_opts) do
+              opts.merge(in_reply_to_discussion_id: previous_note.discussion_id,
+                         type: 'DiffNote',
+                         noteable_type: 'MergeRequest',
+                         noteable_id: merge_request.id,
+                         position: position.to_h)
+            end
+
+            it 'note is not associated with a note diff file' do
+              expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new)
+
+              note = described_class.new(project_with_repo, user, new_opts).execute
+
+              expect(note).to be_persisted
+              expect(note.note_diff_file).to be_nil
+            end
+
+            context 'when DiffNote from an image' do
+              let(:image_position) do
+                Gitlab::Diff::Position.new(old_path: "files/images/6049019_460s.jpg",
+                                           new_path: "files/images/6049019_460s.jpg",
+                                           width: 100,
+                                           height: 100,
+                                           x: 1,
+                                           y: 100,
+                                           diff_refs: merge_request.diff_refs,
+                                           position_type: 'image')
+              end
+
+              let(:new_opts) do
+                opts.merge(in_reply_to_discussion_id: nil,
+                           type: 'DiffNote',
+                           noteable_type: 'MergeRequest',
+                           noteable_id: merge_request.id,
+                           position: image_position.to_h)
+              end
+
+              it 'note is not associated with a note diff file' do
+                note = described_class.new(project_with_repo, user, new_opts).execute
+
+                expect(note).to be_persisted
+                expect(note.note_diff_file).to be_nil
+              end
+            end
           end
         end
       end

spec/services/notes/destroy_service_spec.rb

@@ -24,36 +24,54 @@ RSpec.describe Notes::DestroyService do
         .to change { user.todos_pending_count }.from(1).to(0)
     end
 
-    context 'noteable highlight cache clearing' do
-      let(:repo_project) { create(:project, :repository) }
-      let(:merge_request) do
+    it 'tracks issue comment removal usage data', :clean_gitlab_redis_shared_state do
+      note = create(:note, project: project, noteable: issue)
+      event = Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_COMMENT_REMOVED
+      counter = Gitlab::UsageDataCounters::HLLRedisCounter
+
+      expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_comment_removed_action).with(author: user).and_call_original
+      expect do
+        described_class.new(project, user).execute(note)
+      end.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1)
+    end
+
+    context 'in a merge request' do
+      let_it_be(:repo_project) { create(:project, :repository) }
+      let_it_be(:merge_request) do
         create(:merge_request, source_project: repo_project,
-                               target_project: repo_project)
+               target_project: repo_project)
       end
-
-      let(:note) do
+      let_it_be(:note) do
         create(:diff_note_on_merge_request, project: repo_project,
-                                            noteable: merge_request)
+               noteable: merge_request)
       end
 
-      before do
-        allow(note.position).to receive(:unfolded_diff?) { true }
-      end
-
-      it 'clears noteable diff cache when it was unfolded for the note position' do
-        expect(merge_request).to receive_message_chain(:diffs, :clear_cache)
+      it 'does not track issue comment removal usage data' do
+        expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_removed_action)
 
         described_class.new(repo_project, user).execute(note)
       end
 
-      it 'does not clear cache when note is not the first of the discussion' do
-        reply_note = create(:diff_note_on_merge_request, in_reply_to: note,
-                                                         project: repo_project,
-                                                         noteable: merge_request)
+      context 'noteable highlight cache clearing' do
+        before do
+          allow(note.position).to receive(:unfolded_diff?) { true }
+        end
+
+        it 'clears noteable diff cache when it was unfolded for the note position' do
+          expect(merge_request).to receive_message_chain(:diffs, :clear_cache)
+
+          described_class.new(repo_project, user).execute(note)
+        end
+
+        it 'does not clear cache when note is not the first of the discussion' do
+          reply_note = create(:diff_note_on_merge_request, in_reply_to: note,
+                              project: repo_project,
+                              noteable: merge_request)
 
-        expect(merge_request).not_to receive(:diffs)
+          expect(merge_request).not_to receive(:diffs)
 
-        described_class.new(repo_project, user).execute(reply_note)
+          described_class.new(repo_project, user).execute(reply_note)
+        end
       end
     end
   end

spec/services/notes/update_service_spec.rb

@@ -47,6 +47,22 @@ RSpec.describe Notes::UpdateService do
       end
     end
 
+    it 'does not track usage data when params is blank' do
+      expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_edited_action)
+
+      update_note({})
+    end
+
+    it 'tracks usage data', :clean_gitlab_redis_shared_state do
+      event = Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_COMMENT_EDITED
+      counter = Gitlab::UsageDataCounters::HLLRedisCounter
+
+      expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_comment_edited_action).with(author: user).and_call_original
+      expect do
+        update_note(note: 'new text')
+      end.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1)
+    end
+
     context 'with system note' do
       before do
         note.update_column(:system, true)
@@ -55,6 +71,12 @@ RSpec.describe Notes::UpdateService do
       it 'does not update the note' do
         expect { update_note(note: 'new text') }.not_to change { note.reload.note }
       end
+
+      it 'does not track usage data' do
+        expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_edited_action)
+
+        update_note(note: 'new text')
+      end
     end
 
     context 'suggestions' do
@@ -220,6 +242,12 @@ RSpec.describe Notes::UpdateService do
         expect(note).not_to receive(:create_new_cross_references!)
         update_note({ note: "Updated with new reference: #{issue.to_reference}" })
       end
+
+      it 'does not track usage data' do
+        expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_edited_action)
+
+        update_note(note: 'new text')
+      end
     end
   end
 end