Include subgroups in Contribution Analytics

- Subgroups are also included in the calculation
- Remove ActiveRecord calls from the view
- Extract calculations to a separate class
- Combine count queries into one

ee/app/controllers/groups/analytics_controller.rb

@@ -3,57 +3,27 @@
 class Groups::AnalyticsController < Groups::ApplicationController
   before_action :group
   before_action :check_contribution_analytics_available!
-  before_action :load_events
 
   layout 'group'
 
   def show
-    respond_to do |format|
-      format.html do
-        @stats = {}
-        @stats[:push] = count_by_user(event_totals[:push])
-        @stats[:merge_requests_created] = count_by_user(event_totals[:merge_requests_created])
-        @stats[:issues_closed] = count_by_user(event_totals[:issues_closed])
-      end
+    @start_date = data_collector.from
 
+    respond_to do |format|
+      format.html
       format.json do
         render json: GroupAnalyticsSerializer
-                 .new(events: event_totals)
-                 .represent(users), status: :ok
+          .new(data_collector: data_collector)
+          .represent(data_collector.users), status: :ok
       end
     end
   end
 
   private
 
-  def count_by_user(data)
-    users.map { |user| data.fetch(user.id, 0) }
-  end
-
-  # rubocop: disable CodeReuse/ActiveRecord
-  def users
-    @users ||= @group.users.select(:id, :name, :username).reorder(:id)
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
-  # rubocop: disable CodeReuse/ActiveRecord
-  def load_events
-    @start_date = params[:start_date] || Date.today - 1.week
-    @events = Event.contributions
-                .where("created_at > ?", @start_date)
-                .where(project_id: @group.projects)
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
-  def event_totals
-    @event_totals ||= {
-      push: @events.code_push.totals_by_author,
-      issues_created: @events.issues.created.totals_by_author,
-      issues_closed: @events.issues.closed.totals_by_author,
-      merge_requests_created: @events.merge_requests.created.totals_by_author,
-      merge_requests_merged: @events.merge_requests.merged.totals_by_author,
-      total_events: @events.totals_by_author
-    }
+  def data_collector
+    @data_collector ||= Gitlab::ContributionAnalytics::DataCollector
+      .new(group: @group, from: params[:start_date] || 1.week.ago.to_date)
   end
 
   def check_contribution_analytics_available!

ee/app/models/ee/event.rb

@@ -11,6 +11,7 @@ module EE
       scope :closed, -> { where(action: ::Event::CLOSED) }
       scope :merged, -> { where(action: ::Event::MERGED) }
       scope :totals_by_author, -> { group(:author_id).count }
+      scope :totals_by_author_target_type_action, -> { group(:author_id, :target_type, :action).count }
     end
   end
 end

ee/app/serializers/user_analytics_entity.rb

@@ -3,9 +3,6 @@
 class UserAnalyticsEntity < Grape::Entity
   include RequestAwareEntity
 
-  EVENT_TYPES = [:push, :issues_created, :issues_closed, :merge_requests_created,
-                 :merge_requests_merged, :total_events].freeze
-
   expose :username
 
   expose :name, as: :fullname
@@ -14,9 +11,9 @@ class UserAnalyticsEntity < Grape::Entity
     user_path(user)
   end
 
-  EVENT_TYPES.each do |event_type|
+  Gitlab::ContributionAnalytics::DataCollector::EVENT_TYPES.each do |event_type|
     expose event_type do |user|
-      request.events[event_type].fetch(user.id, 0)
+      request.data_collector.totals[event_type].fetch(user.id, 0)
     end
   end
 end

ee/app/views/groups/analytics/show.html.haml

@@ -21,9 +21,9 @@
       Contribution analytics for issues, merge requests and push events since #{@start_date}
 
   %h3 Push
-  - code_push_count = @events.code_push.count
-  - commits_count = PushEventPayload.commit_count_for(@events.code_push)
-  - person_count = @events.code_push.pluck(:author_id).uniq.count # rubocop: disable CodeReuse/ActiveRecord
+  - code_push_count = @data_collector.total_push_count
+  - commits_count = @data_collector.total_commit_count
+  - person_count = @data_collector.total_push_author_count
   - person_count_string = pluralize person_count, 'person'
   - pushes_string = _('<strong>%{pushes}</strong> pushes, more than <strong>%{commits}</strong> commits by <strong>%{people}</strong> contributors.').html_safe % { pushes: code_push_count, commits: commits_count , people: person_count_string }
   - if code_push_count > 0 || commits_count > 0 || person_count > 0
@@ -36,8 +36,8 @@
       #js_pushes_chart_vue
 
   %h3 Merge Requests
-  - mr_created_count = @events.merge_requests.created.count
-  - mr_merged_count = @events.merge_requests.merged.count
+  - mr_created_count = @data_collector.total_merge_requests_created_count
+  - mr_merged_count = @data_collector.total_merge_requests_merged_count
   - if mr_created_count > 0 || mr_merged_count > 0
     = _('<strong>%{created_count}</strong> created, <strong>%{accepted_count}</strong> accepted.').html_safe % { created_count: mr_created_count, accepted_count: mr_merged_count }
   - else
@@ -48,8 +48,8 @@
       #js_merge_requests_chart_vue
 
   %h3 Issues
-  - issues_created_count = @events.issues.created.count
-  - issues_closed_count = @events.issues.closed.pluck(:target_id).uniq.count # rubocop: disable CodeReuse/ActiveRecord
+  - issues_created_count = @data_collector.total_issues_created_count
+  - issues_closed_count = @data_collector.total_issues_closed_count
   - if issues_created_count > 0 && issues_closed_count > 0
     = _('<strong>%{created_count}</strong> created, <strong>%{closed_count}</strong> closed.').html_safe % { created_count: issues_created_count, closed_count: issues_closed_count }
   - else
@@ -62,11 +62,6 @@
   #js-group-member-contributions{ data: { member_contributions_path: group_analytics_path(@group, { start_date: @start_date, format: :json }) } }
   -# haml-lint:disable InlineJavaScript
   %script#js-analytics-data{ type: "application/json" }
-    - data = {}
-    - data[:labels] = @users.map(&:name)
-    - [:push, :issues_closed, :merge_requests_created].each do |scope|
-      - data[scope] = {}
-      - data[scope][:data] = @stats[scope]
-    = data.to_json.html_safe
+    = @data_collector.group_member_contributions_table_data.to_json.html_safe
 - elsif show_promotions?
   = render 'shared/promotions/promote_contribution_analytics'

ee/changelogs/unreleased/61056-data-improvements-for-contribution-analytics.yml

+---
+title: Include Subgroups in Contribution Analytics calcualtions
+merge_request: 14536
+author:
+type: fixed

ee/lib/gitlab/contribution_analytics/data_collector.rb

+# frozen_string_literal: true
+
+# rubocop: disable CodeReuse/ActiveRecord
+
+module Gitlab
+  module ContributionAnalytics
+    class DataCollector
+      EVENT_TYPES = %i[push issues_created issues_closed merge_requests_created merge_requests_merged total_events].freeze
+
+      attr_reader :group, :from
+
+      def initialize(group:, from: 1.week.ago.to_date)
+        @group = group
+        @from = from
+      end
+
+      def push_by_author_count
+        all_counts.each_with_object({}) do |((author_id, target_type, action), count), hash|
+          hash[author_id] = count if target_type.nil? && action.eql?(Event::PUSHED)
+        end
+      end
+
+      def issues_created_by_author_count
+        all_counts.each_with_object({}) do |((author_id, target_type, action), count), hash|
+          hash[author_id] = count if target_type.eql?(Issue.name) && action.eql?(Event::CREATED)
+        end
+      end
+
+      def issues_closed_by_author_count
+        all_counts.each_with_object({}) do |((author_id, target_type, action), count), hash|
+          hash[author_id] = count if target_type.eql?(Issue.name) && action.eql?(Event::CLOSED)
+        end
+      end
+
+      def merge_requests_created_by_author_count
+        all_counts.each_with_object({}) do |((author_id, target_type, action), count), hash|
+          hash[author_id] = count if target_type.eql?(MergeRequest.name) && action.eql?(Event::CREATED)
+        end
+      end
+
+      def merge_requests_merged_by_author_count
+        all_counts.each_with_object({}) do |((author_id, target_type, action), count), hash|
+          hash[author_id] = count if target_type.eql?(MergeRequest.name) && action.eql?(Event::MERGED)
+        end
+      end
+
+      def total_events_by_author_count
+        all_counts.each_with_object({}) do |((author_id, target_type, action), count), hash|
+          hash[author_id] ||= 0
+          hash[author_id] += count
+        end
+      end
+
+      def total_push_author_count
+        all_counts.count { |(_, _, action), _| action.eql?(Event::PUSHED) }
+      end
+
+      def total_push_count
+        all_counts.sum { |(_, _, action), count| action.eql?(Event::PUSHED) ? count : 0 }
+      end
+
+      def total_commit_count
+        PushEventPayload.commit_count_for(base_query.code_push)
+      end
+
+      def total_merge_requests_created_count
+        all_counts.sum { |(_, target_type, action), count| target_type.eql?(MergeRequest.name) && action.eql?(Event::CREATED) ? count : 0 }
+      end
+
+      def total_merge_requests_merged_count
+        all_counts.sum { |(_, target_type, action), count| target_type.eql?(MergeRequest.name) && action.eql?(Event::MERGED) ? count : 0 }
+      end
+
+      def total_issues_created_count
+        all_counts.sum { |(_, target_type, action), count| target_type.eql?(Issue.name) && action.eql?(Event::CREATED) ? count : 0 }
+      end
+
+      def total_issues_closed_count
+        all_counts.sum { |(_, target_type, action), count| target_type.eql?(Issue.name) && action.eql?(Event::CLOSED) ? count : 0 }
+      end
+
+      def users
+        @users ||= User
+          .select(:id, :name, :username)
+          .where(id: total_events_by_author_count.keys)
+          .reorder(:id)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      def group_member_contributions_table_data
+        {
+          labels: users.map(&:name),
+          push: { data: count_by_user(push_by_author_count) },
+          issues_closed: { data: count_by_user(issues_closed_by_author_count) },
+          merge_requests_created: { data: count_by_user(merge_requests_created_by_author_count) }
+        }
+      end
+
+      def totals
+        @totals ||= {
+          push: push_by_author_count,
+          issues_created: issues_created_by_author_count,
+          issues_closed: issues_closed_by_author_count,
+          merge_requests_created: merge_requests_created_by_author_count,
+          merge_requests_merged: merge_requests_merged_by_author_count,
+          total_events: total_events_by_author_count
+        }
+      end
+
+      private
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def base_query
+        Event
+          .where(action: ::Event::PUSHED).or(
+            Event.where(target_type: [::MergeRequest.name, ::Issue.name], action: [::Event::CREATED, ::Event::CLOSED, ::Event::MERGED])
+          )
+          .where(Event.arel_table[:created_at].gteq(from))
+          .joins(:project)
+          .merge(::Project.inside_path(group.full_path))
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      # Format:
+      # {
+      #   [user1_id, target_type, action] => count,
+      #   [user2_id, target_type, action] => count
+      # }
+      def all_counts
+        @all_counts ||= Rails.cache.fetch(cache_key, expires_in: 1.minute) do
+          base_query.totals_by_author_target_type_action
+        end
+      end
+
+      def count_by_user(data)
+        users.map { |user| data.fetch(user.id, 0) }
+      end
+
+      def cache_key
+        [group, from]
+      end
+    end
+  end
+end

ee/spec/controllers/groups/analytics_controller_spec.rb

@@ -67,14 +67,14 @@ describe Groups::AnalyticsController do
 
     expect(response).to have_gitlab_http_status(200)
 
-    expect(assigns[:users]).to match_array([user, user2, user3])
-    expect(assigns[:events].length).to eq(6)
-    stats = assigns[:stats]
+    expect(assigns[:data_collector].users).to match_array([user, user2, user3])
+    expect(assigns[:data_collector].total_events_by_author_count.values.sum).to eq(6)
+    stats = assigns[:data_collector].group_member_contributions_table_data
 
     # NOTE: The array ordering matters! The view references them all by index
-    expect(stats[:merge_requests_created]).to eq([0, 1, 1])
-    expect(stats[:issues_closed]).to eq([1, 1, 0])
-    expect(stats[:push]).to eq([1, 0, 1])
+    expect(stats[:merge_requests_created][:data]).to eq([0, 1, 1])
+    expect(stats[:issues_closed][:data]).to eq([1, 1, 0])
+    expect(stats[:push][:data]).to eq([1, 0, 1])
   end
 
   it "returns member contributions JSON when format is JSON" do
@@ -94,6 +94,32 @@ describe Groups::AnalyticsController do
     expect(first_user["total_events"]).to eq(2)
   end
 
+  it "includes projects in subgroups" do
+    subgroup = create(:group, parent: group)
+    subproject = create(:project, :repository, group: subgroup)
+
+    create_event(user, subproject, issue, Event::CLOSED)
+    create_push_event(user, subproject)
+
+    get :show, params: { group_id: group.path }, format: :json
+
+    first_user = json_response.first
+    expect(first_user["issues_closed"]).to eq(2)
+    expect(first_user["push"]).to eq(2)
+  end
+
+  it "excludes projects outside of the group" do
+    empty_group = create(:group)
+    other_project = create(:project, :repository)
+
+    create_event(user, other_project, issue, Event::CLOSED)
+    create_push_event(user, other_project)
+
+    get :show, params: { group_id: empty_group.path }, format: :json
+
+    expect(json_response).to be_empty
+  end
+
   it 'does not cause N+1 queries when the format is JSON' do
     control_count = ActiveRecord::QueryRecorder.new do
       get :show, params: { group_id: group.path }, format: :json

ee/spec/lib/gitlab/contribution_analytics/data_collector_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::ContributionAnalytics::DataCollector do
+  context '#totals' do
+    it 'collects event counts grouped by users by calling #base_query' do
+      group = create(:group)
+      user = create(:user)
+      project1 = create(:project, group: group)
+      project2 = create(:project, group: group)
+
+      issue = create(:closed_issue, project: project1)
+      mr = create(:merge_request, source_project: project2)
+
+      create(:event, :closed, project: project1, target: issue, author: user)
+      create(:event, :created, project: project2, target: mr, author: user)
+
+      data_collector = described_class.new(group: group)
+      expect(data_collector.totals).to eq({
+        issues_closed: { user.id => 1 },
+        issues_created: {},
+        merge_requests_created: { user.id => 1 },
+        merge_requests_merged: {},
+        push: {},
+        total_events: { user.id => 2 }
+      })
+    end
+  end
+
+  context 'deriving various counts from #all_counts' do
+    let(:all_counts) do
+      {
+        [1, nil, Event::PUSHED] => 2,
+        [2, nil, Event::PUSHED] => 2,
+        [1, MergeRequest.name, Event::MERGED] => 2,
+        [4, MergeRequest.name, Event::MERGED] => 2,
+        [5, MergeRequest.name, Event::CREATED] => 0,
+        [6, MergeRequest.name, Event::CREATED] => 1,
+        [10, Issue.name, Event::CLOSED] => 10,
+        [11, Issue.name, Event::CLOSED] => 11
+      }
+    end
+    let(:data_collector) { described_class.new(group: Group.new) }
+
+    before do
+      allow(data_collector).to receive(:all_counts).and_return(all_counts)
+    end
+
+    describe 'extracts correct counts from all_counts' do
+      it 'for #push_by_author_count' do
+        expect(data_collector.push_by_author_count).to eq({ 1 => 2, 2 => 2 })
+      end
+
+      it 'for #total_push_author_count' do
+        expect(data_collector.total_push_author_count).to eq(2)
+      end
+
+      it 'for #total_push_count' do
+        expect(data_collector.total_push_count).to eq(4)
+      end
+
+      it 'for #total_merge_requests_created_count' do
+        expect(data_collector.total_merge_requests_created_count).to eq(1)
+      end
+
+      it 'for #total_merge_requests_merged_count' do
+        expect(data_collector.total_merge_requests_merged_count).to eq(4)
+      end
+
+      it 'for #total_issues_closed_count' do
+        expect(data_collector.total_issues_closed_count).to eq(21)
+      end
+
+      it 'handles empty result' do
+        allow(data_collector).to receive(:all_counts).and_return({})
+
+        expect(data_collector.push_by_author_count).to eq({})
+        expect(data_collector.total_push_author_count).to eq(0)
+        expect(data_collector.total_push_count).to eq(0)
+        expect(data_collector.total_merge_requests_created_count).to eq(0)
+        expect(data_collector.total_merge_requests_merged_count).to eq(0)
+        expect(data_collector.total_issues_closed_count).to eq(0)
+      end
+    end
+  end
+end

ee/spec/serializers/user_analytics_entity_spec.rb

@@ -12,14 +12,10 @@ describe UserAnalyticsEntity do
       total_events: {}
     }
   end
-  let(:request) { double('request') }
+  let(:request) { double('request', data_collector: instance_double(Gitlab::ContributionAnalytics::DataCollector, totals: events)) }
 
   subject(:json) { described_class.new(user, request: request).as_json }
 
-  before do
-    allow(request).to receive(:events).and_return(events)
-  end
-
   it 'has all the user attributes' do
     is_expected.to include(:username, :fullname, :user_web_url)
   end