From 21aa5833d09dc8adf7fd456a8d34324767b18e43 Mon Sep 17 00:00:00 2001
From: Mario de la Ossa <mariodelaossa@gmail.com>
Date: Mon, 19 Oct 2020 19:29:03 -0600
Subject: [PATCH] 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 | 6 +
app/services/notes/destroy_service.rb | 7 +
app/services/notes/update_service.rb | 6 +
.../unreleased/229918-issuedata-comments.yml | 5 +
.../issue_activity_unique_counter.rb | 15 ++
.../usage_data_counters/known_events.yml | 12 +
.../issue_activity_unique_counter_spec.rb | 30 +++
spec/services/notes/create_service_spec.rb | 249 ++++++++++--------
spec/services/notes/destroy_service_spec.rb | 56 ++--
spec/services/notes/update_service_spec.rb | 28 ++
10 files changed, 279 insertions(+), 135 deletions(-)
create mode 100644 changelogs/unreleased/229918-issuedata-comments.yml
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 48f44affb23be..b2826b5c9053e 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -73,6 +73,8 @@ def when_saved(note)
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 @@ def track_event(note, user)
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
diff --git a/app/services/notes/destroy_service.rb b/app/services/notes/destroy_service.rb
index ee8a680fcb491..2b6ec47eaef8d 100644
--- a/app/services/notes/destroy_service.rb
+++ b/app/services/notes/destroy_service.rb
@@ -8,6 +8,13 @@ def execute(note)
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
diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb
index 193d308007885..37872f7fbdb89 100644
--- a/app/services/notes/update_service.rb
+++ b/app/services/notes/update_service.rb
@@ -14,6 +14,8 @@ def execute(note)
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 @@ def update_confidentiality(note)
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
diff --git a/changelogs/unreleased/229918-issuedata-comments.yml b/changelogs/unreleased/229918-issuedata-comments.yml
new file mode 100644
index 0000000000000..90250d977cafe
--- /dev/null
+++ b/changelogs/unreleased/229918-issuedata-comments.yml
@@ -0,0 +1,5 @@
+---
+title: UsageData for issues added/removed/edited
+merge_request: 45609
+author:
+type: added
diff --git a/lib/gitlab/usage_data_counters/issue_activity_unique_counter.rb b/lib/gitlab/usage_data_counters/issue_activity_unique_counter.rb
index e8839875109e6..b6d570a3d08b6 100644
--- a/lib/gitlab/usage_data_counters/issue_activity_unique_counter.rb
+++ b/lib/gitlab/usage_data_counters/issue_activity_unique_counter.rb
@@ -33,6 +33,9 @@ module IssueActivityUniqueCounter
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 @@ def track_issue_time_spent_changed_action(author:, time: Time.zone.now)
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)
diff --git a/lib/gitlab/usage_data_counters/known_events.yml b/lib/gitlab/usage_data_counters/known_events.yml
index bc56c5d6d9b27..023427946aa8d 100644
--- a/lib/gitlab/usage_data_counters/known_events.yml
+++ b/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
diff --git a/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb
index e08dc41d0cc75..a53ab6ff97b72 100644
--- a/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb
+++ b/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb
@@ -292,6 +292,36 @@ def track_action(params)
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)
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index 1e5536a2d0bee..ace31196c6a44 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -67,147 +67,164 @@
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
diff --git a/spec/services/notes/destroy_service_spec.rb b/spec/services/notes/destroy_service_spec.rb
index d1076f77cec4d..f0e5b29ac9b0b 100644
--- a/spec/services/notes/destroy_service_spec.rb
+++ b/spec/services/notes/destroy_service_spec.rb
@@ -24,36 +24,54 @@
.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
diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb
index 66efdf8abe7b1..e2f51c9af678a 100644
--- a/spec/services/notes/update_service_spec.rb
+++ b/spec/services/notes/update_service_spec.rb
@@ -47,6 +47,22 @@ def update_note(opts)
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 @@ def update_note(opts)
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 @@ def update_note(opts)
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
--
GitLab