Merge branch 'sh-optimize-analytics-controller' into 'master'

Remove N+1 queries for Groups::AnalyticsController

Closes gitlab-ce#22940 and #1886

See merge request !1514

app/controllers/groups/analytics_controller.rb

@@ -4,7 +4,7 @@ class Groups::AnalyticsController < Groups::ApplicationController
   layout 'group'
 
   def show
-    @users = @group.users
+    @users = @group.users.select(:id, :name, :username)
     @start_date = params[:start_date] || Date.today - 1.week
     @events = Event.contributions
       .where("created_at > ?", @start_date)
@@ -12,16 +12,21 @@ class Groups::AnalyticsController < Groups::ApplicationController
 
     @stats = {}
 
-    @stats[:merge_requests] = @users.map do |user|
-      @events.merge_requests.created.where(author_id: user).count
-    end
+    @stats[:total_events] = count_by_user(@events.totals_by_author)
+    @stats[:push] = count_by_user(@events.code_push.totals_by_author)
+    @stats[:merge_requests_created] = count_by_user(@events.merge_requests.created.totals_by_author)
+    @stats[:merge_requests_merged] = count_by_user(@events.merge_requests.merged.totals_by_author)
+    @stats[:issues_created] = count_by_user(@events.issues.created.totals_by_author)
+    @stats[:issues_closed] = count_by_user(@events.issues.closed.totals_by_author)
+  end
+
+  private
 
-    @stats[:issues] = @users.map do |user|
-      @events.issues.closed.where(author_id: user).count
-    end
+  def count_by_user(data)
+    user_ids.map { |id| data.fetch(id, 0) }
+  end
 
-    @stats[:push] = @users.map do |user|
-      @events.code_push.where(author_id: user).count
-    end
+  def user_ids
+    @user_ids ||= @users.map(&:id)
   end
 end

app/models/event.rb

@@ -46,6 +46,7 @@ class Event < ActiveRecord::Base
   scope :created, -> { where(action: CREATED) }
   scope :closed, -> { where(action: CLOSED) }
   scope :merged, -> { where(action: MERGED) }
+  scope :totals_by_author, -> { group(:author_id).count }
 
   class << self
     # Update Gitlab::ContributionsCalendar#activity_dates if this changes

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

@@ -60,7 +60,7 @@
   .col-md-8
     %div
       %p.light Merge requests created per group member
-      %canvas#merge_requests{ height: 250 }
+      %canvas#merge_requests_created{ height: 250 }
 
 %h3 Issues
 
@@ -77,7 +77,7 @@
   .col-md-8
     %div
       %p.light Issues closed per group member
-      %canvas#issues{ height: 250 }
+      %canvas#issues_closed{ height: 250 }
 
 .gray-content-block
   .oneline
@@ -109,21 +109,21 @@
           Total Contributions
           = icon('sort')
     %tbody
-      - @users.each do |user|
+      - @users.each_with_index do |user, index|
         %tr
           %td
             %strong
               = link_to user.name, user
-          %td= @events.code_push.where(author_id: user).count
-          %td= @events.issues.created.where(author_id: user).count
-          %td= @events.issues.closed.where(author_id: user).count
-          %td= @events.merge_requests.created.where(author_id: user).count
-          %td= @events.merge_requests.merged.where(author_id: user).count
-          %td= @events.where(author_id: user).count
+          %td= @stats[:push][index]
+          %td= @stats[:issues_created][index]
+          %td= @stats[:issues_closed][index]
+          %td= @stats[:merge_requests_created][index]
+          %td= @stats[:merge_requests_merged][index]
+          %td= @stats[:total_events][index]
 
 
 
-- [:push, :issues, :merge_requests].each do |scope|
+- [:push, :issues_closed, :merge_requests_created].each do |scope|
   :javascript
     var data = {
       labels : #{@users.map(&:name).to_json},

changelogs/unreleased-ee/sh-optimize-analytics-controller.yml

+---
+title: Remove N+1 queries for Groups::AnalyticsController
+merge_request:
+author:

spec/controllers/groups/analytics_controller_spec.rb

+require 'spec_helper'
+
+describe Groups::AnalyticsController do
+  let(:user) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:user3) { create(:user) }
+  let(:group) { create(:group) }
+  let(:project) { create(:project, group: group) }
+  let(:issue) { create(:issue, project: project) }
+  let(:merge_request) { create(:merge_request, :simple, source_project: project) }
+  let(:push_data) { Gitlab::DataBuilder::Push.build_sample(project, user) }
+
+  def create_event(author, project, target, action)
+    Event.create!(
+      project: project,
+      action: action,
+      target: target,
+      author: author,
+      created_at: Time.now)
+  end
+
+  def create_push_event(author, project)
+    event = create_event(author, project, nil, Event::PUSHED)
+    event.data = push_data
+    event.save
+  end
+
+  before do
+    group.add_owner(user)
+    group.add_user(user2, GroupMember::DEVELOPER)
+    group.add_user(user3, GroupMember::MASTER)
+    sign_in(user)
+
+    create_event(user, project, issue, Event::CLOSED)
+    create_event(user2, project, issue, Event::CLOSED)
+    create_event(user2, project, merge_request, Event::CREATED)
+    create_event(user3, project, merge_request, Event::CREATED)
+    create_push_event(user, project)
+    create_push_event(user3, project)
+  end
+
+  it 'sets instance variables properly' do
+    get :show, group_id: group.path
+
+    expect(controller.instance_variable_get(:@users)).to match_array([user, user2, user3])
+    expect(controller.instance_variable_get(:@events).length).to eq(6)
+    stats = controller.instance_variable_get(:@stats)
+    expect(stats[:total_events]).to eq([2, 2, 2])
+    expect(stats[:merge_requests_merged]).to eq([0, 0, 0])
+    expect(stats[:merge_requests_created]).to eq([1, 1, 0])
+    expect(stats[:issues_closed]).to eq([0, 1, 1])
+    expect(stats[:push]).to eq([1, 0, 1])
+  end
+
+  describe 'with views' do
+    render_views
+
+    it 'avoids a N+1 query in #show' do
+      control_count = ActiveRecord::QueryRecorder.new { get :show, group_id: group.path }.count
+
+      # Clear out controller state to force a refresh of the group
+      controller.instance_variable_set(:@group, nil)
+      user4 = create(:user)
+      group.add_user(user4, GroupMember::DEVELOPER)
+
+      expect { get :show, group_id: group.path }.not_to exceed_query_limit(control_count)
+    end
+  end
+end