Merge remote-tracking branch 'origin/master' into rc/ce-to-ee-monday

app/assets/javascripts/weight_select.js

@@ -30,9 +30,9 @@
             $loading.fadeOut();
             $selectbox.hide();
             if (data.weight != null) {
-              $value.html(data.weight);
+              $value.html(`<strong>${data.weight}</strong>`);
             } else {
-              $value.html('None');
+              $value.html('<span class="no-value">None</span>');
             }
             return $sidebarCollapsedValue.html(data.weight);
           });

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},

app/views/shared/issuable/_sidebar.html.haml

@@ -162,11 +162,11 @@
             = icon('spinner spin', class: 'block-loading')
             - if can?(current_user, :"admin_#{issuable.to_ability_name}", @project)
               = link_to 'Edit', '#', class: 'edit-link pull-right'
-          .value.bold.hide-collapsed
+          .value.hide-collapsed
             - if issuable.weight
-              = issuable.weight
+              %strong= issuable.weight
             - else
-              .light None
+              %span.no-value None
           .selectbox.hide-collapsed
             = weight_dropdown_tag(issuable, title: 'Change weight', data: { field_name: 'weight', issue_update: "#{issuable_json_path(issuable)}", ability_name: "#{issuable.to_ability_name}" }) do
               %ul

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

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

doc/gitlab-geo/troubleshooting.md

@@ -55,6 +55,7 @@ your browser. We perform the following health checks on each secondary node
 to help identify if something is wrong:
 
 - Is the node running?
+- Is the node's secondary database configured for streaming replication?
 - Is the node's secondary tracking database configured?
 - Is the node's secondary tracking database connected?
 - Is the node's secondary tracking database up-to-date?

lib/gitlab/geo/health_check.rb

@@ -4,6 +4,7 @@ module Gitlab
       def self.perform_checks
         return '' unless Gitlab::Geo.secondary?
         return 'The Geo database configuration file is missing.' unless Gitlab::Geo.configured?
+        return 'The Geo node has a database that is not configured for streaming replication with the primary node.' unless self.database_secondary?
 
         database_version  = self.get_database_version.to_i
         migration_version = self.get_migration_version.to_i
@@ -48,6 +49,14 @@ module Gitlab
 
         latest_migration
       end
+
+      def self.database_secondary?
+        raise NotImplemented unless Gitlab::Database.postgresql?
+
+        ActiveRecord::Base.connection.execute('SELECT pg_is_in_recovery()')
+          .first
+          .fetch('pg_is_in_recovery') == 't'
+      end
     end
   end
 end

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

spec/lib/gitlab/geo/health_check_spec.rb

@@ -12,6 +12,14 @@ describe Gitlab::Geo::HealthCheck do
       expect(subject.perform_checks).to be_blank
     end
 
+    it 'returns an error when database is not configured for streaming replication' do
+      allow(Gitlab::Geo).to receive(:secondary?) { true }
+      allow(Gitlab::Database).to receive(:postgresql?) { true }
+      allow(ActiveRecord::Base).to receive_message_chain(:connection, :execute, :first, :fetch) { 'f' }
+
+      expect(subject.perform_checks).not_to be_blank
+    end
+
     it 'returns an error when configuration file is missing for tracking DB' do
       allow(Rails.configuration).to receive(:respond_to?).with(:geo_database) { false }