From 26adb8bbde87a35bf1e6ff7ca42060ac8cccca1f Mon Sep 17 00:00:00 2001
From: Zakir Dzhamaliddinov <zakir.dzhamaliddinov@gmail.com>
Date: Thu, 16 Nov 2023 11:43:20 +0000
Subject: [PATCH] Include merge request approved events in Profile heat map
https://gitlab.com/gitlab-org/gitlab/-/issues/378613
Changelog: fixed
---
app/models/event.rb | 25 ++-
app/models/user.rb | 2 +-
.../contributions_calendar_refactoring.yml | 8 +
lib/gitlab/contributions_calendar.rb | 101 ++++++++---
.../lib/gitlab/contributions_calendar_spec.rb | 163 ++++++++++--------
spec/models/event_spec.rb | 53 +++++-
6 files changed, 244 insertions(+), 108 deletions(-)
create mode 100644 config/feature_flags/development/contributions_calendar_refactoring.yml
diff --git a/app/models/event.rb b/app/models/event.rb
index 7de7ad8ccd63b..3ff5a46f2d976 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -89,10 +89,26 @@ class Event < ApplicationRecord
scope :for_wiki_page, -> { where(target_type: 'WikiPage::Meta') }
scope :for_design, -> { where(target_type: 'DesignManagement::Design') }
scope :for_issue, -> { where(target_type: ISSUE_TYPES) }
+ scope :for_merge_request, -> { where(target_type: 'MergeRequest') }
scope :for_fingerprint, ->(fingerprint) do
fingerprint.present? ? where(fingerprint: fingerprint) : none
end
scope :for_action, ->(action) { where(action: action) }
+ scope :created_between, ->(start_time, end_time) { where(created_at: start_time..end_time) }
+ scope :count_by_dates, ->(date_interval) { group("DATE(created_at + #{date_interval})").count }
+
+ scope :contributions, -> do
+ contribution_actions = [actions[:pushed], actions[:commented]]
+
+ contributable_target_types = %w[MergeRequest Issue WorkItem]
+ target_contribution_actions = [actions[:created], actions[:closed], actions[:merged], actions[:approved]]
+
+ where(
+ 'action IN (?) OR (target_type IN (?) AND action IN (?))',
+ contribution_actions,
+ contributable_target_types, target_contribution_actions
+ )
+ end
scope :with_associations, -> do
# We're using preload for "push_event_payload" as otherwise the association
@@ -133,15 +149,6 @@ def find_sti_class(action)
end
end
- # Update Gitlab::ContributionsCalendar#activity_dates if this changes
- def contributions
- where(
- 'action IN (?) OR (target_type IN (?) AND action IN (?))',
- [actions[:pushed], actions[:commented]],
- %w[MergeRequest Issue WorkItem], [actions[:created], actions[:closed], actions[:merged]]
- )
- end
-
def limit_recent(limit = 20, offset = nil)
recent.limit(limit).offset(offset)
end
diff --git a/app/models/user.rb b/app/models/user.rb
index 25f2256313665..9c099a5dfcee9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1776,7 +1776,7 @@ def oauth_authorized_tokens
def contributed_projects
events = Event.select(:project_id)
.contributions.where(author_id: self)
- .where("created_at > ?", Time.current - 1.year)
+ .created_after(Time.current - 1.year)
.distinct
.reorder(nil)
diff --git a/config/feature_flags/development/contributions_calendar_refactoring.yml b/config/feature_flags/development/contributions_calendar_refactoring.yml
new file mode 100644
index 0000000000000..e779939cc28e9
--- /dev/null
+++ b/config/feature_flags/development/contributions_calendar_refactoring.yml
@@ -0,0 +1,8 @@
+---
+name: contributions_calendar_refactoring
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134991
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/429648
+milestone: '16.6'
+type: development
+group: group::tenant scale
+default_enabled: false
diff --git a/lib/gitlab/contributions_calendar.rb b/lib/gitlab/contributions_calendar.rb
index 2068a9ae7d518..c91fdd59c578b 100644
--- a/lib/gitlab/contributions_calendar.rb
+++ b/lib/gitlab/contributions_calendar.rb
@@ -3,6 +3,7 @@
module Gitlab
class ContributionsCalendar
include TimeZoneHelper
+ include ::Gitlab::Utils::StrongMemoize
attr_reader :contributor
attr_reader :current_user
@@ -16,18 +17,23 @@ def initialize(contributor, current_user = nil)
.execute(current_user, ignore_visibility: @contributor.include_private_contributions?)
end
- # rubocop: disable CodeReuse/ActiveRecord
def activity_dates
- return {} if @projects.empty?
- return @activity_dates if @activity_dates.present?
+ return {} if projects.empty?
start_time = @contributor_time_instance.years_ago(1).beginning_of_day
end_time = @contributor_time_instance.end_of_day
date_interval = "INTERVAL '#{@contributor_time_instance.utc_offset} seconds'"
+ if Feature.enabled?(:contributions_calendar_refactoring)
+ return contributions_between(start_time, end_time).count_by_dates(date_interval)
+ end
+
+ # TODO: Remove after feature flag `contributions_calendar_refactoring` is rolled out
+ # See https://gitlab.com/gitlab-org/gitlab/-/issues/429648
+ # rubocop: disable CodeReuse/ActiveRecord -- will be removed
# Can't use Event.contributions here because we need to check 3 different
- # project_features for the (currently) 3 different contribution types
+ # project_features for the (currently) 4 different contribution types
repo_events = events_created_between(start_time, end_time, :repository)
.where(action: :pushed)
issue_events = events_created_between(start_time, end_time, :issues)
@@ -42,55 +48,110 @@ def activity_dates
.from_union([repo_events, issue_events, mr_events, note_events], remove_duplicates: false)
.group(:date)
.map(&:attributes)
+ # rubocop: enable CodeReuse/ActiveRecord
- @activity_dates = events.each_with_object(Hash.new { |h, k| h[k] = 0 }) do |event, activities|
+ events.each_with_object(Hash.new { |h, k| h[k] = 0 }) do |event, activities|
activities[event["date"]] += event["num_events"]
end
end
- # rubocop: enable CodeReuse/ActiveRecord
- # rubocop: disable CodeReuse/ActiveRecord
def events_by_date(date)
return Event.none unless can_read_cross_project?
date_in_time_zone = date.in_time_zone(@contributor_time_instance.time_zone)
+ if Feature.enabled?(:contributions_calendar_refactoring)
+ return contributions_between(date_in_time_zone.beginning_of_day, date_in_time_zone.end_of_day).with_associations
+ end
+
+ # TODO: Remove after feature flag `contributions_calendar_refactoring` is rolled out
+ # See https://gitlab.com/gitlab-org/gitlab/-/issues/429648
+ # rubocop: disable CodeReuse/ActiveRecord -- will be removed
Event.contributions.where(author_id: contributor.id)
.where(created_at: date_in_time_zone.beginning_of_day..date_in_time_zone.end_of_day)
.where(project_id: projects)
.with_associations
+ # rubocop: enable CodeReuse/ActiveRecord
end
- # rubocop: enable CodeReuse/ActiveRecord
- def starting_year
- @contributor_time_instance.years_ago(1).year
- end
+ private
- def starting_month
- @contributor_time_instance.month
+ def contributions_between(start_time, end_time)
+ # Can't use Event.contributions here because we need to check 3 different
+ # project_features for the (currently) 4 different contribution types
+ repo_events =
+ project_events_created_between(start_time, end_time, features: :repository)
+ .for_action(:pushed)
+
+ issue_events =
+ project_events_created_between(start_time, end_time, features: :issues)
+ .for_issue
+ .for_action(%i[created closed])
+
+ mr_events =
+ project_events_created_between(start_time, end_time, features: :merge_requests)
+ .for_merge_request
+ .for_action(%i[merged created closed approved])
+
+ note_events =
+ project_events_created_between(start_time, end_time, features: %i[issues merge_requests])
+ .for_action(:commented)
+
+ Event.from_union([repo_events, issue_events, mr_events, note_events], remove_duplicates: false)
end
- private
-
def can_read_cross_project?
Ability.allowed?(current_user, :read_cross_project)
end
- # rubocop: disable CodeReuse/ActiveRecord
- def events_created_between(start_time, end_time, feature)
+ # rubocop: disable CodeReuse/ActiveRecord -- no need to move this to ActiveRecord model
+ def project_events_created_between(start_time, end_time, features:)
+ Array(features).reduce(Event.none) do |events, feature|
+ events.or(contribution_events(start_time, end_time).where(project_id: authed_projects(feature)))
+ end
+ end
+ # rubocop: enable CodeReuse/ActiveRecord
+
+ def authed_projects(feature)
+ strong_memoize("#{feature}_projects") do
+ # no need to check features access of current user, if the contributor opted-in
+ # to show all private events anyway - otherwise they would get filtered out again
+ next contributed_project_ids if contributor.include_private_contributions?
+
+ # rubocop: disable CodeReuse/ActiveRecord -- no need to move this to ActiveRecord model
+ ProjectFeature
+ .with_feature_available_for_user(feature, current_user)
+ .where(project_id: contributed_project_ids)
+ .pluck(:project_id)
+ # rubocop: enable CodeReuse/ActiveRecord
+ end
+ end
+
+ # rubocop: disable CodeReuse/ActiveRecord -- no need to move this to ActiveRecord model
+ def contributed_project_ids
# re-running the contributed projects query in each union is expensive, so
# use IN(project_ids...) instead. It's the intersection of two users so
# the list will be (relatively) short
@contributed_project_ids ||= projects.distinct.pluck(:id)
+ end
+ # rubocop: enable CodeReuse/ActiveRecord
+ def contribution_events(start_time, end_time)
+ contributor.events.created_between(start_time, end_time)
+ end
+
+ # TODO: Remove after feature flag `contributions_calendar_refactoring` is rolled out
+ # See https://gitlab.com/gitlab-org/gitlab/-/issues/429648
+ # rubocop: disable CodeReuse/ActiveRecord -- will be removed
+ def events_created_between(start_time, end_time, feature)
# no need to check feature access of current user, if the contributor opted-in
# to show all private events anyway - otherwise they would get filtered out again
- authed_projects = if @contributor.include_private_contributions?
- @contributed_project_ids
+ authed_projects = if contributor.include_private_contributions?
+ contributed_project_ids
else
ProjectFeature
.with_feature_available_for_user(feature, current_user)
- .where(project_id: @contributed_project_ids)
+ .where(project_id: contributed_project_ids)
.reorder(nil)
.select(:project_id)
end
diff --git a/spec/lib/gitlab/contributions_calendar_spec.rb b/spec/lib/gitlab/contributions_calendar_spec.rb
index 326e27fa716d4..732b7cb3e5937 100644
--- a/spec/lib/gitlab/contributions_calendar_spec.rb
+++ b/spec/lib/gitlab/contributions_calendar_spec.rb
@@ -19,9 +19,9 @@
end
end
- let_it_be(:feature_project) do
+ let_it_be(:public_project_with_private_issues) do
create(:project, :public, :issues_private) do |project|
- create(:project_member, user: contributor, project: project).project
+ create(:project_member, user: contributor, project: project)
end
end
@@ -45,7 +45,12 @@ def calendar(current_user = nil)
end
def create_event(project, day, hour = 0, action = :created, target_symbol = :issue)
- targets[project] ||= create(target_symbol, project: project, author: contributor)
+ targets[project] ||=
+ if target_symbol == :merge_request
+ create(:merge_request, source_project: project, author: contributor)
+ else
+ create(target_symbol, project: project, author: contributor)
+ end
Event.create!(
project: project,
@@ -58,49 +63,65 @@ def create_event(project, day, hour = 0, action = :created, target_symbol = :iss
end
describe '#activity_dates', :aggregate_failures do
- it "returns a hash of date => count" do
- create_event(public_project, last_week)
- create_event(public_project, last_week)
- create_event(public_project, today)
- work_item_event = create_event(private_project, today, 0, :created, :work_item)
+ shared_examples_for 'returns a hash of date => count' do
+ specify do
+ create_event(public_project, last_week)
+ create_event(public_project, last_week)
+ create_event(public_project, today)
+ work_item_event = create_event(private_project, today, 0, :created, :work_item)
- # make sure the target is a work item as we want to include those in the count
- expect(work_item_event.target_type).to eq('WorkItem')
- expect(calendar(contributor).activity_dates).to eq(last_week => 2, today => 2)
+ # make sure the target is a work item as we want to include those in the count
+ expect(work_item_event.target_type).to eq('WorkItem')
+ expect(calendar(contributor).activity_dates).to eq(last_week => 2, today => 2)
+ end
end
- context "when the user has opted-in for private contributions" do
- before do
- contributor.update_column(:include_private_contributions, true)
- end
+ shared_examples 'private contribution events' do
+ context "when the user has opted-in for private contributions" do
+ before do
+ contributor.update_column(:include_private_contributions, true)
+ end
- it "shows private and public events to all users" do
- create_event(private_project, today)
- create_event(public_project, today)
+ it "shows private and public events to all users" do
+ create_event(private_project, today)
+ create_event(public_project, today)
- expect(calendar.activity_dates[today]).to eq(2)
- expect(calendar(user).activity_dates[today]).to eq(2)
- expect(calendar(contributor).activity_dates[today]).to eq(2)
- end
+ expect(calendar.activity_dates[today]).to eq(2)
+ expect(calendar(user).activity_dates[today]).to eq(2)
+ expect(calendar(contributor).activity_dates[today]).to eq(2)
+ end
+
+ # tests for bug https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74826
+ it "still counts correct with feature access levels set to private" do
+ create_event(private_project, today)
- # tests for bug https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74826
- it "still counts correct with feature access levels set to private" do
- create_event(private_project, today)
+ private_project.project_feature.update_attribute(:issues_access_level, ProjectFeature::PRIVATE)
+ private_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::PRIVATE)
+ private_project.project_feature.update_attribute(:merge_requests_access_level, ProjectFeature::PRIVATE)
- private_project.project_feature.update_attribute(:issues_access_level, ProjectFeature::PRIVATE)
- private_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::PRIVATE)
- private_project.project_feature.update_attribute(:merge_requests_access_level, ProjectFeature::PRIVATE)
+ expect(calendar.activity_dates[today]).to eq(1)
+ expect(calendar(user).activity_dates[today]).to eq(1)
+ expect(calendar(contributor).activity_dates[today]).to eq(1)
+ end
- expect(calendar.activity_dates[today]).to eq(1)
- expect(calendar(user).activity_dates[today]).to eq(1)
- expect(calendar(contributor).activity_dates[today]).to eq(1)
+ it "does not fail if there are no contributed projects" do
+ expect(calendar.activity_dates[today]).to eq(nil)
+ end
end
+ end
- it "does not fail if there are no contributed projects" do
- expect(calendar.activity_dates[today]).to eq(nil)
+ context 'when contributions_calendar_refactoring feature flag is disabled' do
+ before do
+ stub_feature_flags(contributions_calendar_refactoring: false)
end
+
+ it_behaves_like 'returns a hash of date => count'
+ include_examples 'private contribution events'
end
+ it_behaves_like 'returns a hash of date => count'
+ include_examples 'private contribution events'
+
it "counts the diff notes on merge request" do
create_event(public_project, today, 0, :commented, :diff_note_on_merge_request)
@@ -114,6 +135,15 @@ def create_event(project, day, hour = 0, action = :created, target_symbol = :iss
expect(calendar(contributor).activity_dates[today]).to eq(2)
end
+ it "counts merge request events" do
+ create_event(public_project, today, 0, :created, :merge_request)
+ create_event(public_project, today, 1, :closed, :merge_request)
+ create_event(public_project, today, 2, :approved, :merge_request)
+ create_event(public_project, today, 3, :merged, :merge_request)
+
+ expect(calendar(contributor).activity_dates[today]).to eq(4)
+ end
+
context "when events fall under different dates depending on the system time zone" do
before do
create_event(public_project, today, 1)
@@ -189,65 +219,56 @@ def create_event(project, day, hour = 0, action = :created, target_symbol = :iss
it "only shows private events to authorized users" do
e1 = create_event(public_project, today)
e2 = create_event(private_project, today)
- e3 = create_event(feature_project, today)
+ e3 = create_event(public_project_with_private_issues, today, 0, :created, :issue)
create_event(public_project, last_week)
- expect(calendar.events_by_date(today)).to contain_exactly(e1, e3)
- expect(calendar(contributor).events_by_date(today)).to contain_exactly(e1, e2, e3)
- end
-
- it "includes diff notes on merge request" do
- e1 = create_event(public_project, today, 0, :commented, :diff_note_on_merge_request)
-
expect(calendar.events_by_date(today)).to contain_exactly(e1)
+ expect(calendar(contributor).events_by_date(today)).to contain_exactly(e1, e2, e3)
end
- context 'when the user cannot read cross project' do
+ context 'when contributions_calendar_refactoring feature flag is disabled' do
before do
- allow(Ability).to receive(:allowed?).and_call_original
- expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false }
+ stub_feature_flags(contributions_calendar_refactoring: false)
end
- it 'does not return any events' do
- create_event(public_project, today)
+ it "only shows private events to authorized users" do
+ e1 = create_event(public_project, today)
+ e2 = create_event(private_project, today)
+ e3 = create_event(public_project_with_private_issues, today, 0, :created, :issue)
+ create_event(public_project, last_week)
- expect(calendar(user).events_by_date(today)).to be_empty
+ expect(calendar.events_by_date(today)).to contain_exactly(e1, e3)
+ expect(calendar(contributor).events_by_date(today)).to contain_exactly(e1, e2, e3)
end
end
- end
- describe '#starting_year' do
- let(:travel_time) { Time.find_zone('UTC').local(2020, 12, 31, 19, 0, 0) }
+ it "includes diff notes on merge request" do
+ e1 = create_event(public_project, today, 0, :commented, :diff_note_on_merge_request)
- context "when the contributor's timezone is not set" do
- it "is the start of last year in the system timezone" do
- expect(calendar.starting_year).to eq(2019)
- end
+ expect(calendar.events_by_date(today)).to contain_exactly(e1)
end
- context "when the contributor's timezone is set to Sydney" do
- let(:contributor) { create(:user, { timezone: 'Sydney' }) }
+ it 'includes merge request events' do
+ mr_created_event = create_event(public_project, today, 0, :created, :merge_request)
+ mr_closed_event = create_event(public_project, today, 1, :closed, :merge_request)
+ mr_approved_event = create_event(public_project, today, 2, :approved, :merge_request)
+ mr_merged_event = create_event(public_project, today, 3, :merged, :merge_request)
- it "is the start of last year in Sydney" do
- expect(calendar.starting_year).to eq(2020)
- end
+ expect(calendar.events_by_date(today)).to contain_exactly(
+ mr_created_event, mr_closed_event, mr_approved_event, mr_merged_event
+ )
end
- end
- describe '#starting_month' do
- let(:travel_time) { Time.find_zone('UTC').local(2020, 12, 31, 19, 0, 0) }
-
- context "when the contributor's timezone is not set" do
- it "is the start of this month in the system timezone" do
- expect(calendar.starting_month).to eq(12)
+ context 'when the user cannot read cross project' do
+ before do
+ allow(Ability).to receive(:allowed?).and_call_original
+ expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false }
end
- end
- context "when the contributor's timezone is set to Sydney" do
- let(:contributor) { create(:user, { timezone: 'Sydney' }) }
+ it 'does not return any events' do
+ create_event(public_project, today)
- it "is the start of this month in Sydney" do
- expect(calendar.starting_month).to eq(1)
+ expect(calendar(user).events_by_date(today)).to be_empty
end
end
end
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index 3e4fe57c59b14..0cae348855038 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -115,6 +115,18 @@
end
end
+ describe '.for_merge_request' do
+ let(:mr_event) { create(:event, :for_merge_request, project: project) }
+
+ before do
+ create(:event, :for_issue, project: project)
+ end
+
+ it 'returns events for MergeRequest target_type' do
+ expect(described_class.for_merge_request).to contain_exactly(mr_event)
+ end
+ end
+
describe '.created_at' do
it 'can find the right event' do
time = 1.day.ago
@@ -128,6 +140,21 @@
end
end
+ describe '.created_between' do
+ it 'returns events created between given timestamps' do
+ start_time = 2.days.ago
+ end_time = Date.today
+
+ create(:event, created_at: 3.days.ago)
+ e1 = create(:event, created_at: 2.days.ago)
+ e2 = create(:event, created_at: 1.day.ago)
+
+ found = described_class.created_between(start_time, end_time)
+
+ expect(found).to contain_exactly(e1, e2)
+ end
+ end
+
describe '.for_fingerprint' do
let_it_be(:with_fingerprint) { create(:event, fingerprint: 'aaa', project: project) }
@@ -152,16 +179,28 @@
end
describe '.contributions' do
- let!(:merge_request_event) { create(:event, :created, :for_merge_request, project: project) }
- let!(:issue_event) { create(:event, :created, :for_issue, project: project) }
+ let!(:merge_request_events) do
+ %i[created closed merged approved].map do |action|
+ create(:event, :for_merge_request, action: action, project: project)
+ end
+ end
+
let!(:work_item_event) { create(:event, :created, :for_work_item, project: project) }
- let!(:design_event) { create(:design_event, project: project) }
+ let!(:issue_events) do
+ %i[created closed].map { |action| create(:event, :for_issue, action: action, project: project) }
+ end
+
+ let!(:push_event) { create_push_event(project, project.owner) }
+ let!(:comment_event) { create(:event, :commented, project: project) }
+
+ before do
+ create(:design_event, project: project) # should not be in scope
+ end
- it 'returns events for MergeRequest, Issue and WorkItem' do
+ it 'returns events for MergeRequest, Issue, WorkItem and push, comment events' do
expect(described_class.contributions).to contain_exactly(
- merge_request_event,
- issue_event,
- work_item_event
+ *merge_request_events, *issue_events, work_item_event,
+ push_event, comment_event
)
end
end
--
GitLab