Implement a second round of review comments from @DouweM.

- Don't use `TableReferences` - using `.arel_table` is shorter!
- Move some database-related code to `Gitlab::Database`
- Remove the `MergeRequest#issues_closed` and
  `Issue#closed_by_merge_requests`  associations. They were either
  shadowing or were too similar to existing methods. They are not being
  used anywhere, so it's better to remove them to reduce confusion.
- Use Rails 3-style validations
- Index for `MergeRequest::Metrics#first_deployed_to_production_at`
- Only include `CycleAnalyticsHelpers::TestGeneration` for specs that
  need it.
- Other minor refactorings.

9 files changed, 64 insertions, 94 deletions

diff --git a/app/models/cycle_analytics.rb b/app/models/cycle_analytics.rb
index 3ca160252ff..be295487fd2 100644
--- a/app/models/cycle_analytics.rb
+++ b/app/models/cycle_analytics.rb
@@ -5,55 +5,54 @@ class CycleAnalytics
   def initialize(project, from:)
     @project = project
     @from = from
-    @summary = Summary.new(project, from: from)
   end
 
   def summary
-    @summary
+    @summary ||= Summary.new(@project, from: @from)
   end
 
   def issue
     calculate_metric(:issue,
-                     TableReferences.issues[:created_at],
-                     [TableReferences.issue_metrics[:first_associated_with_milestone_at],
-                      TableReferences.issue_metrics[:first_added_to_board_at]])
+                     Issue.arel_table[:created_at],
+                     [Issue::Metrics.arel_table[:first_associated_with_milestone_at],
+                      Issue::Metrics.arel_table[:first_added_to_board_at]])
   end
 
   def plan
     calculate_metric(:plan,
-                     [TableReferences.issue_metrics[:first_associated_with_milestone_at],
-                      TableReferences.issue_metrics[:first_added_to_board_at]],
-                     TableReferences.issue_metrics[:first_mentioned_in_commit_at])
+                     [Issue::Metrics.arel_table[:first_associated_with_milestone_at],
+                      Issue::Metrics.arel_table[:first_added_to_board_at]],
+                     Issue::Metrics.arel_table[:first_mentioned_in_commit_at])
   end
 
   def code
     calculate_metric(:code,
-                     TableReferences.issue_metrics[:first_mentioned_in_commit_at],
-                     TableReferences.merge_requests[:created_at])
+                     Issue::Metrics.arel_table[:first_mentioned_in_commit_at],
+                     MergeRequest.arel_table[:created_at])
   end
 
   def test
     calculate_metric(:test,
-                     TableReferences.merge_request_metrics[:latest_build_started_at],
-                     TableReferences.merge_request_metrics[:latest_build_finished_at])
+                     MergeRequest::Metrics.arel_table[:latest_build_started_at],
+                     MergeRequest::Metrics.arel_table[:latest_build_finished_at])
   end
 
   def review
     calculate_metric(:review,
-                     TableReferences.merge_requests[:created_at],
-                     TableReferences.merge_request_metrics[:merged_at])
+                     MergeRequest.arel_table[:created_at],
+                     MergeRequest::Metrics.arel_table[:merged_at])
   end
 
   def staging
     calculate_metric(:staging,
-                     TableReferences.merge_request_metrics[:merged_at],
-                     TableReferences.merge_request_metrics[:first_deployed_to_production_at])
+                     MergeRequest::Metrics.arel_table[:merged_at],
+                     MergeRequest::Metrics.arel_table[:first_deployed_to_production_at])
   end
 
   def production
     calculate_metric(:production,
-                     TableReferences.issues[:created_at],
-                     TableReferences.merge_request_metrics[:first_deployed_to_production_at])
+                     Issue.arel_table[:created_at],
+                     MergeRequest::Metrics.arel_table[:first_deployed_to_production_at])
   end
 
   private
@@ -69,9 +68,7 @@ class CycleAnalytics
       cte_table,
       subtract_datetimes(base_query, end_time_attrs, start_time_attrs, name.to_s))
 
-    median_queries = Array.wrap(median_datetime(cte_table, interval_query, name))
-    results = median_queries.map { |query| run_query(query) }
-    extract_median(results).presence
+    median_datetime(cte_table, interval_query, name)
   end
 
   # Join table with a row for every <issue,merge_request> pair (where the merge request
@@ -79,27 +76,22 @@ class CycleAnalytics
   # are loaded with an inner join, so issues / merge requests without metrics are
   # automatically excluded.
   def base_query
-    arel_table = TableReferences.merge_requests_closing_issues
+    arel_table = MergeRequestsClosingIssues.arel_table
 
     # Load issues
-    query = arel_table.join(TableReferences.issues).on(TableReferences.issues[:id].eq(arel_table[:issue_id])).
-            join(TableReferences.issue_metrics).on(TableReferences.issues[:id].eq(TableReferences.issue_metrics[:issue_id])).
-            where(TableReferences.issues[:project_id].eq(@project.id)).
-            where(TableReferences.issues[:deleted_at].eq(nil)).
-            where(TableReferences.issues[:created_at].gteq(@from))
+    query = arel_table.join(Issue.arel_table).on(Issue.arel_table[:id].eq(arel_table[:issue_id])).
+            join(Issue::Metrics.arel_table).on(Issue.arel_table[:id].eq(Issue::Metrics.arel_table[:issue_id])).
+            where(Issue.arel_table[:project_id].eq(@project.id)).
+            where(Issue.arel_table[:deleted_at].eq(nil)).
+            where(Issue.arel_table[:created_at].gteq(@from))
 
     # Load merge_requests
-    query = query.join(TableReferences.merge_requests, Arel::Nodes::OuterJoin).
-            on(TableReferences.merge_requests[:id].eq(arel_table[:merge_request_id])).
-            join(TableReferences.merge_request_metrics).
-            on(TableReferences.merge_requests[:id].eq(TableReferences.merge_request_metrics[:merge_request_id]))
+    query = query.join(MergeRequest.arel_table, Arel::Nodes::OuterJoin).
+            on(MergeRequest.arel_table[:id].eq(arel_table[:merge_request_id])).
+            join(MergeRequest::Metrics.arel_table).
+            on(MergeRequest.arel_table[:id].eq(MergeRequest::Metrics.arel_table[:merge_request_id]))
 
     # Limit to merge requests that have been deployed to production after `@from`
-    query.where(TableReferences.merge_request_metrics[:first_deployed_to_production_at].gteq(@from))
-  end
-
-  def run_query(query)
-    query = query.to_sql unless query.is_a?(String)
-    ActiveRecord::Base.connection.execute(query)
+    query.where(MergeRequest::Metrics.arel_table[:first_deployed_to_production_at].gteq(@from))
   end
 end
diff --git a/app/models/cycle_analytics/summary.rb b/app/models/cycle_analytics/summary.rb
index 04a90a8da49..53b2cacb131 100644
--- a/app/models/cycle_analytics/summary.rb
+++ b/app/models/cycle_analytics/summary.rb
@@ -6,7 +6,7 @@ class CycleAnalytics
     end
 
     def new_issues
-      @project.issues.where("created_at > ?", @from).count
+      @project.issues.created_after(@from).count
     end
 
     def commits
diff --git a/app/models/cycle_analytics/table_references.rb b/app/models/cycle_analytics/table_references.rb
deleted file mode 100644
index f276723ee0e..00000000000
--- a/app/models/cycle_analytics/table_references.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-class CycleAnalytics
-  module TableReferences
-    class << self
-      def issues
-        Issue.arel_table
-      end
-
-      def issue_metrics
-        Issue::Metrics.arel_table
-      end
-
-      def merge_requests
-        MergeRequest.arel_table
-      end
-
-      def merge_request_metrics
-        MergeRequest::Metrics.arel_table
-      end
-
-      def merge_requests_closing_issues
-        MergeRequestsClosingIssues.arel_table
-      end
-    end
-  end
-end
diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index 15f349c89bb..07d7e19e70d 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -43,23 +43,30 @@ class Deployment < ActiveRecord::Base
     project.repository.is_ancestor?(commit.id, sha)
   end
 
-  def update_merge_request_metrics
-    if environment.update_merge_request_metrics?
-      merge_requests = project.merge_requests.
-                       joins(:metrics).
-                       where(target_branch: self.ref, merge_request_metrics: { first_deployed_to_production_at: nil }).
-                       where("merge_request_metrics.merged_at <= ?", self.created_at)
-
-      if previous_deployment
-        merge_requests = merge_requests.where("merge_request_metrics.merged_at >= ?", previous_deployment.created_at)
-      end
+  def update_merge_request_metrics!
+    return unless environment.update_merge_request_metrics?
+
+    merge_requests = project.merge_requests.
+                     joins(:metrics).
+                     where(target_branch: self.ref, merge_request_metrics: { first_deployed_to_production_at: nil }).
+                     where("merge_request_metrics.merged_at <= ?", self.created_at)
 
-      # Need to use `map` instead of `select` because MySQL doesn't allow `SELECT`ing from the same table
-      # that we're updating.
-      MergeRequest::Metrics.
-        where(merge_request_id: merge_requests.map(&:id), first_deployed_to_production_at: nil).
-        update_all(first_deployed_to_production_at: self.created_at)
+    if previous_deployment
+      merge_requests = merge_requests.where("merge_request_metrics.merged_at >= ?", previous_deployment.created_at)
     end
+
+    # Need to use `map` instead of `select` because MySQL doesn't allow `SELECT`ing from the same table
+    # that we're updating.
+    merge_request_ids =
+      if Gitlab::Database.postgresql?
+        merge_requests.select(:id)
+      elsif Gitlab::Database.mysql?
+        merge_requests.map(&:id)
+      end
+
+    MergeRequest::Metrics.
+      where(merge_request_id: merge_request_ids, first_deployed_to_production_at: nil).
+      update_all(first_deployed_to_production_at: self.created_at)
   end
 
   def previous_deployment
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 6a264ace165..25ba38a1cff 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -24,7 +24,6 @@ class Issue < ActiveRecord::Base
   has_many :events, as: :target, dependent: :destroy
 
   has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues'
-  has_many :closed_by_merge_requests, through: :merge_requests_closing_issues, source: :merge_request
 
   validates :project, presence: true
 
@@ -39,6 +38,8 @@ class Issue < ActiveRecord::Base
   scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') }
   scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') }
 
+  scope :created_after, -> (datetime) { where("created_at >= ?", datetime) }
+
   attr_spammable :title, spam_title: true
   attr_spammable :description, spam_description: true
 
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 0ac291ce1a0..c05718f4d5a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -17,7 +17,6 @@ class MergeRequest < ActiveRecord::Base
   has_many :events, as: :target, dependent: :destroy
 
   has_many :merge_requests_closing_issues, class_name:  'MergeRequestsClosingIssues'
-  has_many :issues_closed, through: :merge_requests_closing_issues, source: :issue
 
   serialize :merge_params, Hash
 
@@ -524,11 +523,8 @@ class MergeRequest < ActiveRecord::Base
   # Return the set of issues that will be closed if this merge request is accepted.
   def closes_issues(current_user = self.author)
     if target_branch == project.default_branch
-      messages = if merge_request_diff
-                   commits.map(&:safe_message) << description
-                 else
-                   [description]
-                 end
+      messages = [description]
+      messages.concat(commits.map(&:safe_message)) if merge_request_diff
 
       Gitlab::ClosingIssueExtractor.new(project, current_user).
         closed_by_message(messages.join("\n"))
diff --git a/app/models/merge_requests_closing_issues.rb b/app/models/merge_requests_closing_issues.rb
index cd49076002d..ab597c37947 100644
--- a/app/models/merge_requests_closing_issues.rb
+++ b/app/models/merge_requests_closing_issues.rb
@@ -2,8 +2,6 @@ class MergeRequestsClosingIssues < ActiveRecord::Base
   belongs_to :merge_request
   belongs_to :issue
 
-  validates_uniqueness_of :merge_request_id, scope: :issue_id
-
-  validates_presence_of :merge_request
-  validates_presence_of :issue
+  validates :merge_request_id, uniqueness: { scope: :issue_id }, presence: true
+  validates :issue_id, presence: true
 end
diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index e958e91d612..799ad3e1bd0 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -13,7 +13,7 @@ class CreateDeploymentService < BaseService
       deployable: deployable
     )
 
-    deployment.update_merge_request_metrics
+    deployment.update_merge_request_metrics!
 
     deployment
   end
diff --git a/app/views/projects/pipelines/_head.html.haml b/app/views/projects/pipelines/_head.html.haml
index fa1470f5fbc..a97674e84ba 100644
--- a/app/views/projects/pipelines/_head.html.haml
+++ b/app/views/projects/pipelines/_head.html.haml
@@ -20,7 +20,8 @@
             %span
               Environments
 
-      = nav_link(controller: %w(cycle_analytics)) do
-        = link_to project_cycle_analytics_path(@project), title: 'Cycle Analytics' do
-          %span
-            Cycle Analytics
+      - if current_user
+        = nav_link(controller: %w(cycle_analytics)) do
+          = link_to project_cycle_analytics_path(@project), title: 'Cycle Analytics' do
+            %span
+              Cycle Analytics