Address CR remarks

- Eliminate obscure instance_exec call
- Add parent_id attribute to Stage concern
- Use keyword arguments for DataCollector
- Strong memoize DataCollector methods

10 files changed, 79 insertions, 27 deletions

diff --git a/app/models/analytics/cycle_analytics/project_stage.rb b/app/models/analytics/cycle_analytics/project_stage.rb
index a312bd24e78..23f0db0829b 100644
--- a/app/models/analytics/cycle_analytics/project_stage.rb
+++ b/app/models/analytics/cycle_analytics/project_stage.rb
@@ -9,6 +9,7 @@ module Analytics
       belongs_to :project
 
       alias_attribute :parent, :project
+      alias_attribute :parent_id, :project_id
     end
   end
 end
diff --git a/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb b/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb
index a6b87a74cd0..0afe2dbd021 100644
--- a/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb
+++ b/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb
@@ -10,12 +10,6 @@ module Gitlab
         delegate :subject_model, to: :stage
 
         # rubocop: disable CodeReuse/ActiveRecord
-        PROJECT_QUERY_RULES = {
-          Project => {
-            Issue => ->(query) { query.where(project_id: stage.parent.id) },
-            MergeRequest => ->(query) { query.where(target_project_id: stage.parent.id) }
-          }
-        }.freeze
 
         def initialize(stage:, params: {})
           @stage = stage
@@ -40,12 +34,24 @@ module Gitlab
         end
 
         def filter_by_parent_model(query)
-          instance_exec(query, &query_rules.fetch(stage.parent.class).fetch(subject_model))
+          parent_class = stage.parent.class
+
+          if parent_class.eql?(Project)
+            if subject_model.eql?(Issue)
+              query.where(project_id: stage.parent_id)
+            elsif subject_model.eql?(MergeRequest)
+              query.where(target_project_id: stage.parent_id)
+            else
+              raise ArgumentError, "unknown subject_model: #{subject_model}"
+            end
+          else
+            raise ArgumentError, "unknown parent_class: #{parent_class}"
+          end
         end
 
         def filter_by_time_range(query)
-          from = params[:from] || 30.days.ago
-          to = params[:to] || nil
+          from = params.fetch(:from, 30.days.ago)
+          to = params[:to]
 
           query = query.where(subject_table[:created_at].gteq(from))
           query = query.where(subject_table[:created_at].lteq(to)) if to
@@ -55,11 +61,6 @@ module Gitlab
         def subject_table
           subject_model.arel_table
         end
-
-        # EE will override this to include Group rules
-        def query_rules
-          PROJECT_QUERY_RULES
-        end
       end
     end
   end
diff --git a/lib/gitlab/analytics/cycle_analytics/data_collector.rb b/lib/gitlab/analytics/cycle_analytics/data_collector.rb
index fe089e05f91..cd61df407a3 100644
--- a/lib/gitlab/analytics/cycle_analytics/data_collector.rb
+++ b/lib/gitlab/analytics/cycle_analytics/data_collector.rb
@@ -10,17 +10,23 @@ module Gitlab
       #     from: DateTime
       #     to: DateTime
       class DataCollector
-        def initialize(stage, params = {})
+        include Gitlab::Utils::StrongMemoize
+
+        def initialize(stage:, params: {})
           @stage = stage
           @params = params
         end
 
         def records_fetcher
-          RecordsFetcher.new(stage: stage, query: query, params: params)
+          strong_memoize(:records_fetcher) do
+            RecordsFetcher.new(stage: stage, query: query, params: params)
+          end
         end
 
         def median
-          Median.new(stage: stage, query: query)
+          strong_memoize(:median) do
+            Median.new(stage: stage, query: query)
+          end
         end
 
         private
diff --git a/lib/gitlab/analytics/cycle_analytics/median.rb b/lib/gitlab/analytics/cycle_analytics/median.rb
index d93a61a233b..86984b5f06b 100644
--- a/lib/gitlab/analytics/cycle_analytics/median.rb
+++ b/lib/gitlab/analytics/cycle_analytics/median.rb
@@ -21,10 +21,10 @@ module Gitlab
         attr_reader :stage, :query
 
         def percentile_cont
-          percentile_disc_ordering = Arel::Nodes::UnaryOperation.new(Arel::Nodes::SqlLiteral.new('ORDER BY'), duration)
+          percentile_cont_ordering = Arel::Nodes::UnaryOperation.new(Arel::Nodes::SqlLiteral.new('ORDER BY'), duration)
           Arel::Nodes::NamedFunction.new(
             'percentile_cont(0.5) WITHIN GROUP',
-            [percentile_disc_ordering]
+            [percentile_cont_ordering]
           )
         end
 
diff --git a/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb b/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb
index 60d079cf850..24ccaa0c922 100644
--- a/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb
+++ b/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb
@@ -40,9 +40,10 @@ module Gitlab
               AnalyticsBuildSerializer.new.represent(ci_build_records.map { |e| e['build'] })
             else
               records.map do |record|
+                project = record.project
                 attributes = record.attributes.merge({
-                  project_path: record.project.path,
-                  namespace_path: record.project.namespace.path
+                  project_path: project.path,
+                  namespace_path: project.namespace.path
                 })
                 serializer.represent(attributes)
               end
@@ -71,7 +72,7 @@ module Gitlab
         # EE will override this to include Group rules
         def finder_params
           {
-            Project => { project_id: stage.parent.id }
+            Project => { project_id: stage.parent_id }
           }
         end
 
diff --git a/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb
index 84107ae6d7a..27b31902691 100644
--- a/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb
+++ b/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb
@@ -11,10 +11,13 @@ describe Gitlab::Analytics::CycleAnalytics::RecordsFetcher do
   let(:user) { create(:user) }
 
   subject do
-    Gitlab::Analytics::CycleAnalytics::DataCollector.new(stage, {
-      from: 1.year.ago,
-      current_user: user
-    }).records_fetcher.serialized_records
+    Gitlab::Analytics::CycleAnalytics::DataCollector.new(
+      stage: stage,
+      params: {
+        from: 1.year.ago,
+        current_user: user
+      }
+    ).records_fetcher.serialized_records
   end
 
   describe '#serialized_records' do
diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb
index 345164177bf..29c8d548754 100644
--- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb
+++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb
@@ -3,5 +3,20 @@
 require 'spec_helper'
 
 describe Gitlab::Analytics::CycleAnalytics::StageEvents::CodeStageStart do
+  let(:subject) { described_class.new({}) }
+  let(:project) { create(:project) }
+
   it_behaves_like 'cycle analytics event'
+
+  it 'needs connection with an issue via merge_requests_closing_issues table' do
+    issue = create(:issue, project: project)
+    merge_request = create(:merge_request, source_project: project)
+    create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request)
+
+    other_merge_request = create(:merge_request, source_project: project, source_branch: 'a', target_branch: 'master')
+
+    records = subject.apply_query_customization(MergeRequest.all)
+    expect(records).to eq([merge_request])
+    expect(records).not_to include(other_merge_request)
+  end
 end
diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb
index e8ce4197232..cb63139f0a8 100644
--- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb
+++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb
@@ -3,5 +3,22 @@
 require 'spec_helper'
 
 describe Gitlab::Analytics::CycleAnalytics::StageEvents::PlanStageStart do
+  let(:subject) { described_class.new({}) }
+  let(:project) { create(:project) }
+
   it_behaves_like 'cycle analytics event'
+
+  it 'filters issues where first_associated_with_milestone_at or first_added_to_board_at is filled' do
+    issue1 = create(:issue, project: project)
+    issue1.metrics.update!(first_added_to_board_at: 1.month.ago, first_mentioned_in_commit_at: 2.months.ago)
+
+    issue2 = create(:issue, project: project)
+    issue2.metrics.update!(first_associated_with_milestone_at: 1.month.ago, first_mentioned_in_commit_at: 2.months.ago)
+
+    issue_without_metrics = create(:issue, project: project)
+
+    records = subject.apply_query_customization(Issue.all)
+    expect(records).to match_array([issue1, issue2])
+    expect(records).not_to include(issue_without_metrics)
+  end
 end
diff --git a/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb
index 552c5091050..451ee4542be 100644
--- a/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb
+++ b/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb
@@ -36,7 +36,7 @@ end
 shared_examples 'using Gitlab::Analytics::CycleAnalytics::DataCollector as backend' do
   let(:stage_params) { Gitlab::Analytics::CycleAnalytics::DefaultStages.send("params_for_#{stage_name}_stage").merge(project: project) }
   let(:stage) { Analytics::CycleAnalytics::ProjectStage.new(stage_params) }
-  let(:data_collector) { Gitlab::Analytics::CycleAnalytics::DataCollector.new(stage, from: from, current_user: project.creator) }
+  let(:data_collector) { Gitlab::Analytics::CycleAnalytics::DataCollector.new(stage: stage, params: { from: from, current_user: project.creator }) }
   let(:attribute_to_verify) { :title }
 
   context 'provides the same results as the old implementation' do
diff --git a/spec/support/shared_examples/cycle_analytics_stage_shared_examples.rb b/spec/support/shared_examples/cycle_analytics_stage_shared_examples.rb
index 94742751147..b16498e8908 100644
--- a/spec/support/shared_examples/cycle_analytics_stage_shared_examples.rb
+++ b/spec/support/shared_examples/cycle_analytics_stage_shared_examples.rb
@@ -89,4 +89,12 @@ shared_examples_for 'cycle analytics stage' do
       expect(stage).not_to be_matches_with_stage_params(params)
     end
   end
+
+  describe '#parent_id' do
+    it "delegates to 'parent_name'_id attribute" do
+      stage = described_class.new(parent: parent)
+
+      expect(stage.parent_id).to eq(parent.id)
+    end
+  end
 end