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.

7 files changed, 149 insertions, 87 deletions

diff --git a/spec/models/cycle_analytics/code_spec.rb b/spec/models/cycle_analytics/code_spec.rb
index e81c78df8f0..b9381e33914 100644
--- a/spec/models/cycle_analytics/code_spec.rb
+++ b/spec/models/cycle_analytics/code_spec.rb
@@ -1,19 +1,28 @@
 require 'spec_helper'
 
 describe 'CycleAnalytics#code', feature: true do
+  extend CycleAnalyticsHelpers::TestGeneration
+
   let(:project) { create(:project) }
   let(:from_date) { 10.days.ago }
   let(:user) { create(:user, :admin) }
   subject { CycleAnalytics.new(project, from: from_date) }
 
-  generate_cycle_analytics_spec(phase: :code,
-                                data_fn: -> (context) { { issue: context.create(:issue, project: context.project) } },
-                                start_time_conditions: [["issue mentioned in a commit", -> (context, data) { context.create_commit_referencing_issue(data[:issue]) }]],
-                                end_time_conditions:   [["merge request that closes issue is created", -> (context, data) { context.create_merge_request_closing_issue(data[:issue]) }]],
-                                post_fn: -> (context, data) do
-                                  context.merge_merge_requests_closing_issue(data[:issue])
-                                  context.deploy_master
-                                end)
+  generate_cycle_analytics_spec(
+    phase: :code,
+    data_fn: -> (context) { { issue: context.create(:issue, project: context.project) } },
+    start_time_conditions: [["issue mentioned in a commit",
+                             -> (context, data) do
+                               context.create_commit_referencing_issue(data[:issue])
+                             end]],
+    end_time_conditions:   [["merge request that closes issue is created",
+                             -> (context, data) do
+                               context.create_merge_request_closing_issue(data[:issue])
+                             end]],
+    post_fn: -> (context, data) do
+      context.merge_merge_requests_closing_issue(data[:issue])
+      context.deploy_master
+    end)
 
   context "when a regular merge request (that doesn't close the issue) is created" do
     it "returns nil" do
diff --git a/spec/models/cycle_analytics/issue_spec.rb b/spec/models/cycle_analytics/issue_spec.rb
index 8d7d03193f0..e9cc71254ab 100644
--- a/spec/models/cycle_analytics/issue_spec.rb
+++ b/spec/models/cycle_analytics/issue_spec.rb
@@ -1,23 +1,36 @@
 require 'spec_helper'
 
 describe 'CycleAnalytics#issue', models: true do
+  extend CycleAnalyticsHelpers::TestGeneration
+
   let(:project) { create(:project) }
   let(:from_date) { 10.days.ago }
   let(:user) { create(:user, :admin) }
   subject { CycleAnalytics.new(project, from: from_date) }
 
-  generate_cycle_analytics_spec(phase: :issue,
-                                data_fn: -> (context) { { issue: context.build(:issue, project: context.project) } },
-                                start_time_conditions: [["issue created", -> (context, data) { data[:issue].save }]],
-                                end_time_conditions:   [["issue associated with a milestone", -> (context, data) { data[:issue].update(milestone: context.create(:milestone, project: context.project)) if data[:issue].persisted? }],
-                                                        ["list label added to issue", -> (context, data) { data[:issue].update(label_ids: [context.create(:label, lists: [context.create(:list)]).id]) if data[:issue].persisted? }]],
-                                post_fn: -> (context, data) do
-                                  if data[:issue].persisted?
-                                    context.create_merge_request_closing_issue(data[:issue].reload)
-                                    context.merge_merge_requests_closing_issue(data[:issue])
-                                    context.deploy_master
-                                  end
-                                end)
+  generate_cycle_analytics_spec(
+    phase: :issue,
+    data_fn: -> (context) { { issue: context.build(:issue, project: context.project) } },
+    start_time_conditions: [["issue created", -> (context, data) { data[:issue].save }]],
+    end_time_conditions:   [["issue associated with a milestone",
+                             -> (context, data) do
+                               if data[:issue].persisted?
+                                 data[:issue].update(milestone: context.create(:milestone, project: context.project))
+                               end
+                             end],
+                            ["list label added to issue",
+                             -> (context, data) do
+                               if data[:issue].persisted?
+                                 data[:issue].update(label_ids: [context.create(:label, lists: [context.create(:list)]).id])
+                               end
+                             end]],
+    post_fn: -> (context, data) do
+      if data[:issue].persisted?
+        context.create_merge_request_closing_issue(data[:issue].reload)
+        context.merge_merge_requests_closing_issue(data[:issue])
+        context.deploy_master
+      end
+    end)
 
   context "when a regular label (instead of a list label) is added to the issue" do
     it "returns nil" do
diff --git a/spec/models/cycle_analytics/plan_spec.rb b/spec/models/cycle_analytics/plan_spec.rb
index 4a66615e53b..5b8c96dc992 100644
--- a/spec/models/cycle_analytics/plan_spec.rb
+++ b/spec/models/cycle_analytics/plan_spec.rb
@@ -1,26 +1,38 @@
 require 'spec_helper'
 
 describe 'CycleAnalytics#plan', feature: true do
+  extend CycleAnalyticsHelpers::TestGeneration
+
   let(:project) { create(:project) }
   let(:from_date) { 10.days.ago }
   let(:user) { create(:user, :admin) }
   subject { CycleAnalytics.new(project, from: from_date) }
 
-  generate_cycle_analytics_spec(phase: :plan,
-                                data_fn: -> (context) do
-                                  {
-                                    issue: context.create(:issue, project: context.project),
-                                    branch_name: context.random_git_name
-                                  }
-                                end,
-                                start_time_conditions: [["issue associated with a milestone", -> (context, data) { data[:issue].update(milestone: context.create(:milestone, project: context.project)) }],
-                                                        ["list label added to issue", -> (context, data) { data[:issue].update(label_ids: [context.create(:label, lists: [context.create(:list)]).id]) }]],
-                                end_time_conditions:   [["issue mentioned in a commit", -> (context, data) { context.create_commit_referencing_issue(data[:issue], branch_name: data[:branch_name]) }]],
-                                post_fn: -> (context, data) do
-                                  context.create_merge_request_closing_issue(data[:issue], source_branch: data[:branch_name])
-                                  context.merge_merge_requests_closing_issue(data[:issue])
-                                  context.deploy_master
-                                end)
+  generate_cycle_analytics_spec(
+    phase: :plan,
+    data_fn: -> (context) do
+      {
+        issue: context.create(:issue, project: context.project),
+        branch_name: context.random_git_name
+      }
+    end,
+    start_time_conditions: [["issue associated with a milestone",
+                             -> (context, data) do
+                               data[:issue].update(milestone: context.create(:milestone, project: context.project))
+                             end],
+                            ["list label added to issue",
+                             -> (context, data) do
+                               data[:issue].update(label_ids: [context.create(:label, lists: [context.create(:list)]).id])
+                             end]],
+    end_time_conditions:   [["issue mentioned in a commit",
+                             -> (context, data) do
+                               context.create_commit_referencing_issue(data[:issue], branch_name: data[:branch_name])
+                             end]],
+    post_fn: -> (context, data) do
+      context.create_merge_request_closing_issue(data[:issue], source_branch: data[:branch_name])
+      context.merge_merge_requests_closing_issue(data[:issue])
+      context.deploy_master
+    end)
 
   context "when a regular label (instead of a list label) is added to the issue" do
     it "returns nil" do
diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb
index 703f8d5f782..1f5e5cab92d 100644
--- a/spec/models/cycle_analytics/production_spec.rb
+++ b/spec/models/cycle_analytics/production_spec.rb
@@ -1,28 +1,31 @@
 require 'spec_helper'
 
 describe 'CycleAnalytics#production', feature: true do
+  extend CycleAnalyticsHelpers::TestGeneration
+
   let(:project) { create(:project) }
   let(:from_date) { 10.days.ago }
   let(:user) { create(:user, :admin) }
   subject { CycleAnalytics.new(project, from: from_date) }
 
-  generate_cycle_analytics_spec(phase: :production,
-                                data_fn: -> (context) { { issue: context.build(:issue, project: context.project) } },
-                                start_time_conditions: [["issue is created", -> (context, data) { data[:issue].save }]],
-                                before_end_fn: lambda do |context, data|
-                                  context.create_merge_request_closing_issue(data[:issue])
-                                  context.merge_merge_requests_closing_issue(data[:issue])
-                                end,
-                                end_time_conditions:
-                                  [["merge request that closes issue is deployed to production", -> (context, data) { context.deploy_master }],
-                                   ["production deploy happens after merge request is merged (along with other changes)",
-                                    lambda do |context, data|
-                                      # Make other changes on master
-                                      sha = context.project.repository.commit_file(context.user, context.random_git_name, "content", "commit message", 'master', false)
-                                      context.project.repository.commit(sha)
-
-                                      context.deploy_master
-                                    end]])
+  generate_cycle_analytics_spec(
+    phase: :production,
+    data_fn: -> (context) { { issue: context.build(:issue, project: context.project) } },
+    start_time_conditions: [["issue is created", -> (context, data) { data[:issue].save }]],
+    before_end_fn: lambda do |context, data|
+      context.create_merge_request_closing_issue(data[:issue])
+      context.merge_merge_requests_closing_issue(data[:issue])
+    end,
+    end_time_conditions:
+      [["merge request that closes issue is deployed to production", -> (context, data) { context.deploy_master }],
+       ["production deploy happens after merge request is merged (along with other changes)",
+        lambda do |context, data|
+          # Make other changes on master
+          sha = context.project.repository.commit_file(context.user, context.random_git_name, "content", "commit message", 'master', false)
+          context.project.repository.commit(sha)
+
+          context.deploy_master
+        end]])
 
   context "when a regular merge request (that doesn't close the issue) is merged and deployed" do
     it "returns nil" do
diff --git a/spec/models/cycle_analytics/review_spec.rb b/spec/models/cycle_analytics/review_spec.rb
index 100ce11299a..b6e26d8f261 100644
--- a/spec/models/cycle_analytics/review_spec.rb
+++ b/spec/models/cycle_analytics/review_spec.rb
@@ -1,16 +1,25 @@
 require 'spec_helper'
 
 describe 'CycleAnalytics#review', feature: true do
+  extend CycleAnalyticsHelpers::TestGeneration
+
   let(:project) { create(:project) }
   let(:from_date) { 10.days.ago }
   let(:user) { create(:user, :admin) }
   subject { CycleAnalytics.new(project, from: from_date) }
 
-  generate_cycle_analytics_spec(phase: :review,
-                                data_fn: -> (context) { { issue: context.create(:issue, project: context.project) } },
-                                start_time_conditions: [["merge request that closes issue is created", -> (context, data) { context.create_merge_request_closing_issue(data[:issue]) }]],
-                                end_time_conditions:   [["merge request that closes issue is merged", -> (context, data) { context.merge_merge_requests_closing_issue(data[:issue]) }]],
-                                post_fn: -> (context, data) { context.deploy_master })
+  generate_cycle_analytics_spec(
+    phase: :review,
+    data_fn: -> (context) { { issue: context.create(:issue, project: context.project) } },
+    start_time_conditions: [["merge request that closes issue is created",
+                             -> (context, data) do
+                               context.create_merge_request_closing_issue(data[:issue])
+                             end]],
+    end_time_conditions:   [["merge request that closes issue is merged",
+                             -> (context, data) do
+                               context.merge_merge_requests_closing_issue(data[:issue])
+                             end]],
+    post_fn: -> (context, data) { context.deploy_master })
 
   context "when a regular merge request (that doesn't close the issue) is created and merged" do
     it "returns nil" do
diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb
index 105f566f41c..af1c4477ddb 100644
--- a/spec/models/cycle_analytics/staging_spec.rb
+++ b/spec/models/cycle_analytics/staging_spec.rb
@@ -1,26 +1,41 @@
 require 'spec_helper'
 
 describe 'CycleAnalytics#staging', feature: true do
+  extend CycleAnalyticsHelpers::TestGeneration
+
   let(:project) { create(:project) }
   let(:from_date) { 10.days.ago }
   let(:user) { create(:user, :admin) }
   subject { CycleAnalytics.new(project, from: from_date) }
 
-  generate_cycle_analytics_spec(phase: :staging,
-                                data_fn: lambda do |context|
-                                  issue = context.create(:issue, project: context.project)
-                                  { issue: issue, merge_request: context.create_merge_request_closing_issue(issue) }
-                                end,
-                                start_time_conditions: [["merge request that closes issue is merged", -> (context, data) { context.merge_merge_requests_closing_issue(data[:issue])} ]],
-                                end_time_conditions:   [["merge request that closes issue is deployed to production", -> (context, data) { context.deploy_master }],
-                                                        ["production deploy happens after merge request is merged (along with other changes)",
-                                                         lambda do |context, data|
-                                                           # Make other changes on master
-                                                           sha = context.project.repository.commit_file(context.user, context.random_git_name, "content", "commit message", 'master', false)
-                                                           context.project.repository.commit(sha)
-
-                                                           context.deploy_master
-                                                         end]])
+  generate_cycle_analytics_spec(
+    phase: :staging,
+    data_fn: lambda do |context|
+      issue = context.create(:issue, project: context.project)
+      { issue: issue, merge_request: context.create_merge_request_closing_issue(issue) }
+    end,
+    start_time_conditions: [["merge request that closes issue is merged",
+                             -> (context, data) do
+                               context.merge_merge_requests_closing_issue(data[:issue])
+                             end ]],
+    end_time_conditions:   [["merge request that closes issue is deployed to production",
+                             -> (context, data) do
+                               context.deploy_master
+                             end],
+                            ["production deploy happens after merge request is merged (along with other changes)",
+                             lambda do |context, data|
+                               # Make other changes on master
+                               sha = context.project.repository.commit_file(
+                                 context.user,
+                                 context.random_git_name,
+                                 "content",
+                                 "commit message",
+                                 'master',
+                                 false)
+                               context.project.repository.commit(sha)
+
+                               context.deploy_master
+                             end]])
 
   context "when a regular merge request (that doesn't close the issue) is merged and deployed" do
     it "returns nil" do
diff --git a/spec/models/cycle_analytics/test_spec.rb b/spec/models/cycle_analytics/test_spec.rb
index 79edc29c173..89ace0b2742 100644
--- a/spec/models/cycle_analytics/test_spec.rb
+++ b/spec/models/cycle_analytics/test_spec.rb
@@ -1,26 +1,27 @@
 require 'spec_helper'
 
 describe 'CycleAnalytics#test', feature: true do
+  extend CycleAnalyticsHelpers::TestGeneration
+
   let(:project) { create(:project) }
   let(:from_date) { 10.days.ago }
   let(:user) { create(:user, :admin) }
   subject { CycleAnalytics.new(project, from: from_date) }
 
-  generate_cycle_analytics_spec(phase: :test,
-                                data_fn: lambda do |context|
-                                  issue = context.create(:issue, project: context.project)
-                                  merge_request = context.create_merge_request_closing_issue(issue)
-                                  {
-                                    pipeline: context.create(:ci_pipeline, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, project: context.project),
-                                    issue: issue
-                                  }
-                                end,
-                                start_time_conditions: [["pipeline is started", -> (context, data) { data[:pipeline].run! }]],
-                                end_time_conditions:   [["pipeline is finished", -> (context, data) { data[:pipeline].succeed! }]],
-                                post_fn: -> (context, data) do
-                                  context.merge_merge_requests_closing_issue(data[:issue])
-                                  context.deploy_master
-                                end)
+  generate_cycle_analytics_spec(
+    phase: :test,
+    data_fn: lambda do |context|
+      issue = context.create(:issue, project: context.project)
+      merge_request = context.create_merge_request_closing_issue(issue)
+      pipeline = context.create(:ci_pipeline, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, project: context.project)
+      { pipeline: pipeline, issue: issue }
+    end,
+    start_time_conditions: [["pipeline is started", -> (context, data) { data[:pipeline].run! }]],
+    end_time_conditions:   [["pipeline is finished", -> (context, data) { data[:pipeline].succeed! }]],
+    post_fn: -> (context, data) do
+      context.merge_merge_requests_closing_issue(data[:issue])
+      context.deploy_master
+    end)
 
   context "when the pipeline is for a regular merge request (that doesn't close an issue)" do
     it "returns nil" do