Commit 41fcdee9 authored by Stan Hu's avatar Stan Hu

Merge branch '292828-track-suggestion-metrics' into 'master'

Track suggestion add/apply metrics

See merge request gitlab-org/gitlab!52189
parents 4fa3eee3 971d12fc
...@@ -30,6 +30,9 @@ module Suggestions ...@@ -30,6 +30,9 @@ module Suggestions
Suggestion.id_in(suggestion_set.suggestions) Suggestion.id_in(suggestion_set.suggestions)
.update_all(commit_id: result[:result], applied: true) .update_all(commit_id: result[:result], applied: true)
Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter
.track_apply_suggestion_action(user: current_user)
end end
def multi_service def multi_service
......
...@@ -27,6 +27,8 @@ module Suggestions ...@@ -27,6 +27,8 @@ module Suggestions
rows.in_groups_of(100, false) do |rows| rows.in_groups_of(100, false) do |rows|
Gitlab::Database.bulk_insert('suggestions', rows) # rubocop:disable Gitlab/BulkInsert Gitlab::Database.bulk_insert('suggestions', rows) # rubocop:disable Gitlab/BulkInsert
end end
Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter.track_add_suggestion_action(user: @note.author)
end end
end end
end end
---
title: Track suggestion add/apply metrics
merge_request: 52189
author:
type: other
---
name: usage_data_i_code_review_user_add_suggestion
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52189
rollout_issue_url:
milestone: '13.9'
type: development
group: group::code review
default_enabled: true
---
name: usage_data_i_code_review_user_apply_suggestion
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52189
rollout_issue_url:
milestone: '13.9'
type: development
group: group::code review
default_enabled: true
...@@ -521,6 +521,16 @@ ...@@ -521,6 +521,16 @@
category: code_review category: code_review
aggregation: weekly aggregation: weekly
feature_flag: usage_data_i_code_review_user_remove_multiline_mr_comment feature_flag: usage_data_i_code_review_user_remove_multiline_mr_comment
- name: i_code_review_user_add_suggestion
redis_slot: code_review
category: code_review
aggregation: weekly
feature_flag: usage_data_i_code_review_user_add_suggestion
- name: i_code_review_user_apply_suggestion
redis_slot: code_review
category: code_review
aggregation: weekly
feature_flag: usage_data_i_code_review_user_apply_suggestion
# Terraform # Terraform
- name: p_terraform_state_api_unique_users - name: p_terraform_state_api_unique_users
category: terraform category: terraform
......
...@@ -18,6 +18,8 @@ module Gitlab ...@@ -18,6 +18,8 @@ module Gitlab
MR_CREATE_MULTILINE_COMMENT_ACTION = 'i_code_review_user_create_multiline_mr_comment' MR_CREATE_MULTILINE_COMMENT_ACTION = 'i_code_review_user_create_multiline_mr_comment'
MR_EDIT_MULTILINE_COMMENT_ACTION = 'i_code_review_user_edit_multiline_mr_comment' MR_EDIT_MULTILINE_COMMENT_ACTION = 'i_code_review_user_edit_multiline_mr_comment'
MR_REMOVE_MULTILINE_COMMENT_ACTION = 'i_code_review_user_remove_multiline_mr_comment' MR_REMOVE_MULTILINE_COMMENT_ACTION = 'i_code_review_user_remove_multiline_mr_comment'
MR_ADD_SUGGESTION_ACTION = 'i_code_review_user_add_suggestion'
MR_APPLY_SUGGESTION_ACTION = 'i_code_review_user_apply_suggestion'
class << self class << self
def track_mr_diffs_action(merge_request:) def track_mr_diffs_action(merge_request:)
...@@ -68,6 +70,14 @@ module Gitlab ...@@ -68,6 +70,14 @@ module Gitlab
track_unique_action_by_user(MR_PUBLISH_REVIEW_ACTION, user) track_unique_action_by_user(MR_PUBLISH_REVIEW_ACTION, user)
end end
def track_add_suggestion_action(user:)
track_unique_action_by_user(MR_ADD_SUGGESTION_ACTION, user)
end
def track_apply_suggestion_action(user:)
track_unique_action_by_user(MR_APPLY_SUGGESTION_ACTION, user)
end
private private
def track_unique_action_by_merge_request(action, merge_request) def track_unique_action_by_merge_request(action, merge_request)
......
...@@ -148,4 +148,20 @@ RSpec.describe Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter, :cl ...@@ -148,4 +148,20 @@ RSpec.describe Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter, :cl
let(:action) { described_class::MR_PUBLISH_REVIEW_ACTION } let(:action) { described_class::MR_PUBLISH_REVIEW_ACTION }
end end
end end
describe '.track_add_suggestion_action' do
subject { described_class.track_add_suggestion_action(user: user) }
it_behaves_like 'a tracked merge request unique event' do
let(:action) { described_class::MR_ADD_SUGGESTION_ACTION }
end
end
describe '.track_apply_suggestion_action' do
subject { described_class.track_apply_suggestion_action(user: user) }
it_behaves_like 'a tracked merge request unique event' do
let(:action) { described_class::MR_APPLY_SUGGESTION_ACTION }
end
end
end end
...@@ -74,6 +74,14 @@ RSpec.describe Suggestions::ApplyService do ...@@ -74,6 +74,14 @@ RSpec.describe Suggestions::ApplyService do
expect(commit.author_name).to eq(user.name) expect(commit.author_name).to eq(user.name)
end end
it 'tracks apply suggestion event' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to receive(:track_apply_suggestion_action)
.with(user: user)
apply(suggestions)
end
context 'when a custom suggestion commit message' do context 'when a custom suggestion commit message' do
before do before do
project.update!(suggestion_commit_message: message) project.update!(suggestion_commit_message: message)
...@@ -570,56 +578,84 @@ RSpec.describe Suggestions::ApplyService do ...@@ -570,56 +578,84 @@ RSpec.describe Suggestions::ApplyService do
project.add_maintainer(user) project.add_maintainer(user)
end end
shared_examples_for 'service not tracking apply suggestion event' do
it 'does not track apply suggestion event' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.not_to receive(:track_apply_suggestion_action)
result
end
end
context 'diff file was not found' do context 'diff file was not found' do
it 'returns error message' do let(:result) { apply_service.new(user, suggestion).execute }
expect(suggestion.note).to receive(:latest_diff_file) { nil }
result = apply_service.new(user, suggestion).execute before do
expect(suggestion.note).to receive(:latest_diff_file) { nil }
end
it 'returns error message' do
expect(result).to eq(message: 'A file was not found.', expect(result).to eq(message: 'A file was not found.',
status: :error) status: :error)
end end
it_behaves_like 'service not tracking apply suggestion event'
end end
context 'when not all suggestions belong to the same branch' do context 'when not all suggestions belong to the same branch' do
it 'renders error message' do let(:merge_request2) do
merge_request2 = create(:merge_request, create(
:merge_request,
:conflict, :conflict,
source_project: project, source_project: project,
target_project: project) target_project: project
)
end
position2 = Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", let(:position2) do
Gitlab::Diff::Position.new(
old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb", new_path: "files/ruby/popen.rb",
old_line: nil, old_line: nil,
new_line: 15, new_line: 15,
diff_refs: merge_request2 diff_refs: merge_request2.diff_refs
.diff_refs) )
end
diff_note2 = create(:diff_note_on_merge_request, let(:diff_note2) do
create(
:diff_note_on_merge_request,
noteable: merge_request2, noteable: merge_request2,
position: position2, position: position2,
project: project) project: project
)
other_branch_suggestion = create(:suggestion, note: diff_note2) end
result = apply_service.new(user, suggestion, other_branch_suggestion).execute let(:other_branch_suggestion) { create(:suggestion, note: diff_note2) }
let(:result) { apply_service.new(user, suggestion, other_branch_suggestion).execute }
it 'renders error message' do
expect(result).to eq(message: 'Suggestions must all be on the same branch.', expect(result).to eq(message: 'Suggestions must all be on the same branch.',
status: :error) status: :error)
end end
it_behaves_like 'service not tracking apply suggestion event'
end end
context 'suggestion is not appliable' do context 'suggestion is not appliable' do
let(:inapplicable_reason) { "Can't apply this suggestion." } let(:inapplicable_reason) { "Can't apply this suggestion." }
let(:result) { apply_service.new(user, suggestion).execute }
it 'returns error message' do before do
expect(suggestion).to receive(:appliable?).and_return(false) expect(suggestion).to receive(:appliable?).and_return(false)
expect(suggestion).to receive(:inapplicable_reason).and_return(inapplicable_reason) expect(suggestion).to receive(:inapplicable_reason).and_return(inapplicable_reason)
end
result = apply_service.new(user, suggestion).execute it 'returns error message' do
expect(result).to eq(message: inapplicable_reason, status: :error) expect(result).to eq(message: inapplicable_reason, status: :error)
end end
it_behaves_like 'service not tracking apply suggestion event'
end end
context 'lines of suggestions overlap' do context 'lines of suggestions overlap' do
...@@ -632,12 +668,14 @@ RSpec.describe Suggestions::ApplyService do ...@@ -632,12 +668,14 @@ RSpec.describe Suggestions::ApplyService do
create_suggestion(to_content: "I Overlap!") create_suggestion(to_content: "I Overlap!")
end end
it 'returns error message' do let(:result) { apply_service.new(user, suggestion, overlapping_suggestion).execute }
result = apply_service.new(user, suggestion, overlapping_suggestion).execute
it 'returns error message' do
expect(result).to eq(message: 'Suggestions are not applicable as their lines cannot overlap.', expect(result).to eq(message: 'Suggestions are not applicable as their lines cannot overlap.',
status: :error) status: :error)
end end
it_behaves_like 'service not tracking apply suggestion event'
end end
end end
end end
...@@ -53,6 +53,15 @@ RSpec.describe Suggestions::CreateService do ...@@ -53,6 +53,15 @@ RSpec.describe Suggestions::CreateService do
subject { described_class.new(note) } subject { described_class.new(note) }
shared_examples_for 'service not tracking add suggestion event' do
it 'does not track add suggestion event' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.not_to receive(:track_add_suggestion_action)
subject.execute
end
end
describe '#execute' do describe '#execute' do
context 'should not try to parse suggestions' do context 'should not try to parse suggestions' do
context 'when not a diff note for merge requests' do context 'when not a diff note for merge requests' do
...@@ -66,6 +75,8 @@ RSpec.describe Suggestions::CreateService do ...@@ -66,6 +75,8 @@ RSpec.describe Suggestions::CreateService do
subject.execute subject.execute
end end
it_behaves_like 'service not tracking add suggestion event'
end end
context 'when diff note is not for text' do context 'when diff note is not for text' do
...@@ -76,17 +87,21 @@ RSpec.describe Suggestions::CreateService do ...@@ -76,17 +87,21 @@ RSpec.describe Suggestions::CreateService do
note: markdown) note: markdown)
end end
it 'does not try to parse suggestions' do before do
allow(note).to receive(:on_text?) { false } allow(note).to receive(:on_text?) { false }
end
it 'does not try to parse suggestions' do
expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse) expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse)
subject.execute subject.execute
end end
it_behaves_like 'service not tracking add suggestion event'
end end
end end
context 'should not create suggestions' do context 'when diff file is not found' do
let(:note) do let(:note) do
create(:diff_note_on_merge_request, project: project_with_repo, create(:diff_note_on_merge_request, project: project_with_repo,
noteable: merge_request, noteable: merge_request,
...@@ -94,13 +109,17 @@ RSpec.describe Suggestions::CreateService do ...@@ -94,13 +109,17 @@ RSpec.describe Suggestions::CreateService do
note: markdown) note: markdown)
end end
it 'creates no suggestion when diff file is not found' do before do
expect_next_instance_of(DiffNote) do |diff_note| expect_next_instance_of(DiffNote) do |diff_note|
expect(diff_note).to receive(:latest_diff_file).once { nil } expect(diff_note).to receive(:latest_diff_file).once { nil }
end end
end
it 'creates no suggestion' do
expect { subject.execute }.not_to change(Suggestion, :count) expect { subject.execute }.not_to change(Suggestion, :count)
end end
it_behaves_like 'service not tracking add suggestion event'
end end
context 'should create suggestions' do context 'should create suggestions' do
...@@ -137,6 +156,14 @@ RSpec.describe Suggestions::CreateService do ...@@ -137,6 +156,14 @@ RSpec.describe Suggestions::CreateService do
end end
end end
it 'tracks add suggestion event' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to receive(:track_add_suggestion_action)
.with(user: note.author)
subject.execute
end
context 'outdated position note' do context 'outdated position note' do
let!(:outdated_diff) { merge_request.merge_request_diff } let!(:outdated_diff) { merge_request.merge_request_diff }
let!(:latest_diff) { merge_request.create_merge_request_diff } let!(:latest_diff) { merge_request.create_merge_request_diff }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment