Use finished_at for deployment frequency in VSA

This change changes the query for deployment frequency for VSA.
Additionally there is a refactor to merge DeploymenysFinder and
Analytics::Deploymentsfinder to avoid duplicated functionality.

app/finders/deployments_finder.rb

 # frozen_string_literal: true
 
+# Arguments:
+#   params:
+#     project: Project model - Find deployments for this project
+#     updated_after: DateTime
+#     updated_before: DateTime
+#     finished_after: DateTime
+#     finished_before: DateTime
+#     environment: String
+#     status: String (see Deployment.statuses)
+#     order_by: String (see ALLOWED_SORT_VALUES constant)
+#     sort: String (asc | desc)
 class DeploymentsFinder
-  attr_reader :project, :params
+  attr_reader :params
 
-  ALLOWED_SORT_VALUES = %w[id iid created_at updated_at ref].freeze
+  ALLOWED_SORT_VALUES = %w[id iid created_at updated_at ref finished_at].freeze
   DEFAULT_SORT_VALUE = 'id'
 
   ALLOWED_SORT_DIRECTIONS = %w[asc desc].freeze
   DEFAULT_SORT_DIRECTION = 'asc'
 
-  def initialize(project, params = {})
-    @project = project
+  def initialize(params = {})
     @params = params
   end
 
@@ -19,33 +29,20 @@ class DeploymentsFinder
     items = by_updated_at(items)
     items = by_environment(items)
     items = by_status(items)
+    items = preload_associations(items)
+    items = by_finished_between(items)
     sort(items)
   end
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def init_collection
-    project
-      .deployments
-      .includes(
-        :user,
-        environment: [],
-        deployable: {
-          job_artifacts: [],
-          pipeline: {
-            project: {
-              route: [],
-              namespace: :route
-            }
-          },
-          project: {
-            namespace: :route
-          }
-        }
-      )
+    if params[:project]
+      params[:project].deployments
+    else
+      Deployment.none
+    end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   # rubocop: disable CodeReuse/ActiveRecord
   def sort(items)
@@ -68,6 +65,12 @@ class DeploymentsFinder
     end
   end
 
+  def by_finished_between(items)
+    items = items.finished_between(params[:finished_after], params[:finished_before].presence) if params[:finished_after].present?
+
+    items
+  end
+
   def by_status(items)
     return items unless params[:status].present?
 
@@ -87,4 +90,27 @@ class DeploymentsFinder
       sort_values['id'] = sort_values.delete('created_at') if sort_values['created_at'] # Sorting by `id` produces the same result as sorting by `created_at`
     end
   end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def preload_associations(scope)
+    scope.includes(
+      :user,
+      environment: [],
+      deployable: {
+        job_artifacts: [],
+        pipeline: {
+          project: {
+            route: [],
+            namespace: :route
+          }
+        },
+        project: {
+          namespace: :route
+        }
+      }
+    )
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
 end
+
+DeploymentsFinder.prepend_if_ee('EE::DeploymentsFinder')

config/feature_flags/development/query_deploymenys_via_finished_at_in_vsa.yml

+---
+name: query_deploymenys_via_finished_at_in_vsa
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53050
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/300649
+milestone: '13.9'
+type: development
+group: group::optimize
+default_enabled: false

ee/app/finders/analytics/deployments_finder.rb

-# frozen_string_literal: true
-
-module Analytics
-  class DeploymentsFinder
-    def initialize(project:, environment_name:, from:, to: nil)
-      @project = project
-      @environment_name = environment_name
-      @from = from
-      @to = to
-    end
-
-    attr_reader :project, :environment_name, :from, :to
-
-    def execute
-      filter_deployments(project.deployments)
-    end
-
-    private
-
-    def filter_deployments(all_deployments)
-      deployments = filter_by_time(all_deployments)
-      deployments = filter_by_success(deployments)
-      deployments = filter_by_environment_name(deployments)
-      # rubocop: disable CodeReuse/ActiveRecord
-      deployments = deployments.order('finished_at')
-      # rubocop: enable CodeReuse/ActiveRecord
-      deployments
-    end
-
-    def filter_by_time(deployments)
-      deployments.finished_between(from, to)
-    end
-
-    def filter_by_success(deployments)
-      deployments.success
-    end
-
-    def filter_by_environment_name(deployments)
-      deployments.for_environment_name(environment_name)
-    end
-  end
-end

ee/app/finders/ee/deployments_finder.rb

+# frozen_string_literal: true
+
+# Arguments:
+#   params:
+#     group: Group model - Find deployments within a group (including subgroups)
+#
+# Note: If project and group is given at the same time, the project will have precedence.
+# If project or group is missing, the finder will return empty resultset.
+module EE
+  module DeploymentsFinder
+    private
+
+    def init_collection
+      if params[:project].present?
+        super
+      elsif params[:group].present?
+        ::Deployment.for_project(::Project.in_namespace(params[:group].self_and_descendants))
+      else
+        ::Deployment.none
+      end
+    end
+  end
+end

ee/lib/api/analytics/project_deployment_frequency.rb

@@ -46,11 +46,11 @@ module API
 
         def deployments
           strong_memoize(:deployments) do
-            ::Analytics::DeploymentsFinder.new(
+            ::DeploymentsFinder.new(
               project: user_project,
-              environment_name: environment_name,
-              from: start_date,
-              to: end_date
+              environment: environment_name,
+              finished_after: start_date,
+              finished_before: end_date
             ).execute
           end
         end

ee/lib/gitlab/analytics/cycle_analytics/summary/group/deploy.rb

@@ -20,7 +20,14 @@ module Gitlab
 
             # rubocop: disable CodeReuse/ActiveRecord
             def deployments_count
-              @deployments_count ||= begin
+              @deployments_count ||= if Feature.enabled?(:query_deploymenys_via_finished_at_in_vsa)
+                                       deployments = DeploymentsFinder
+                                         .new(group: group, finished_after: options[:from], finished_before: options[:to], status: :success)
+                                         .execute
+
+                                       deployments = deployments.where(project_id: options[:projects]) if options[:projects].present?
+                                       deployments.count
+                                     else
                                        deployments = Deployment.joins(:project).merge(Project.inside_path(group.full_path))
                                        deployments = deployments.where(projects: { id: options[:projects] }) if options[:projects].present?
                                        deployments = deployments.where("deployments.created_at > ?", options[:from])

ee/spec/finders/analytics/deployments_finder_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Analytics::DeploymentsFinder do
-  let_it_be(:project) { create(:project, :repository) }
-  let_it_be(:prod) { create(:environment, project: project, name: "prod") }
-  let_it_be(:dev) { create(:environment, project: project, name: "dev") }
-  let_it_be(:other_project) { create(:project, :repository) }
-  let_it_be(:start_time) { DateTime.new(2017) }
-  let_it_be(:end_time) { DateTime.new(2019) }
-
-  def make_deployment(finished_at, env)
-    create(:deployment,
-           status: :success,
-           project: project,
-           environment: env,
-           finished_at: finished_at)
-  end
-
-  let_it_be(:deployment_2016) { make_deployment(DateTime.new(2016), prod) }
-  let_it_be(:deployment_2017) { make_deployment(DateTime.new(2017), prod) }
-  let_it_be(:deployment_2018) { make_deployment(DateTime.new(2018), prod) }
-  let_it_be(:dev_deployment_2018) { make_deployment(DateTime.new(2018), dev) }
-  let_it_be(:deployment_2019) { make_deployment(DateTime.new(2019), prod) }
-  let_it_be(:deployment_2020) { make_deployment(DateTime.new(2020), prod) }
-
-  describe '#execute' do
-    it 'returns successful deployments for the given project and datetime range' do
-      travel_to(start_time) do
-        create(:deployment, status: :running, project: project, environment: prod)
-        create(:deployment, status: :failed, project: project, environment: prod)
-        create(:deployment, status: :canceled, project: project, environment: prod)
-        create(:deployment, status: :skipped, project: project, environment: prod)
-        create(:deployment, status: :success, project: other_project, environment: prod)
-        create(:deployment, status: :success, project: other_project, environment: prod)
-      end
-
-      expect(described_class.new(
-        project: project,
-        environment_name: prod.name,
-        from: start_time,
-        to: end_time
-      ).execute).to contain_exactly(deployment_2017, deployment_2018)
-
-      expect(described_class.new(
-        project: project,
-        environment_name: prod.name,
-        from: start_time
-      ).execute).to contain_exactly(
-        deployment_2017,
-        deployment_2018,
-        deployment_2019,
-        deployment_2020
-      )
-
-      expect(described_class.new(
-        project: project,
-        environment_name: dev.name,
-        from: start_time
-      ).execute).to contain_exactly(dev_deployment_2018)
-    end
-  end
-end

ee/spec/finders/deployments_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DeploymentsFinder do
+  context 'when filtering by group' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:subgroup) { create(:group, parent: group) }
+
+    let_it_be(:project_in_group) { create(:project, :repository, group: group) }
+    let_it_be(:project_in_subgroup) { create(:project, :repository, group: subgroup) }
+
+    let_it_be(:deployment_in_group) { create(:deployment, status: :success, project: project_in_group) }
+    let_it_be(:deployment_in_subgroup) { create(:deployment, status: :success, project: project_in_subgroup) }
+
+    subject { described_class.new(group: group).execute }
+
+    it { is_expected.to match_array([deployment_in_group, deployment_in_subgroup]) }
+  end
+end

ee/spec/lib/gitlab/analytics/cycle_analytics/summary/group/stage_summary_spec.rb

@@ -127,13 +127,14 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
     end
   end
 
+  shared_examples 'shared examples for #deploys' do
     describe "#deploys" do
       context 'with from date' do
         before do
-        travel_to(5.days.ago) { create(:deployment, :success, project: project) }
-        travel_to(5.days.from_now) { create(:deployment, :success, project: project) }
-        travel_to(5.days.ago) { create(:deployment, :success, project: project_2) }
-        travel_to(5.days.from_now) { create(:deployment, :success, project: project_2) }
+          travel_to(5.days.ago) { create(:deployment, :success, project: project, finished_at: Time.zone.now) }
+          travel_to(5.days.from_now) { create(:deployment, :success, project: project, finished_at: Time.zone.now) }
+          travel_to(5.days.ago) { create(:deployment, :success, project: project_2, finished_at: Time.zone.now) }
+          travel_to(5.days.from_now) { create(:deployment, :success, project: project_2, finished_at: Time.zone.now) }
         end
 
         it "finds the number of deploys made created after it" do
@@ -149,7 +150,7 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
         context 'with subgroups' do
           before do
             travel_to(5.days.from_now) do
-            create(:deployment, :success, project: create(:project, :repository, namespace: create(:group, parent: group)))
+              create(:deployment, :success, finished_at: Time.zone.now, project: create(:project, :repository, namespace: create(:group, parent: group)))
             end
           end
 
@@ -161,7 +162,7 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
         context 'with projects specified in options' do
           before do
             travel_to(5.days.from_now) do
-            create(:deployment, :success, project: create(:project, :repository, namespace: group, name: 'not_applicable'))
+              create(:deployment, :success, finished_at: Time.zone.now, project: create(:project, :repository, namespace: group, name: 'not_applicable'))
             end
           end
 
@@ -184,7 +185,7 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
       context 'with other projects' do
         before do
           travel_to(5.days.from_now) do
-          create(:deployment, :success, project: create(:project, :repository, namespace: create(:group)))
+            create(:deployment, :success, finished_at: Time.zone.now, project: create(:project, :repository, namespace: create(:group)))
           end
         end
 
@@ -212,7 +213,7 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
 
       before do
         travel_to(5.days.ago) do
-        create(:deployment, :success, project: project)
+          create(:deployment, :success, finished_at: Time.zone.now, project: project)
         end
       end
 
@@ -229,7 +230,7 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
 
         before do
           travel_to(5.days.from_now) do
-          create(:deployment, :success, project: project)
+            create(:deployment, :success, finished_at: Time.zone.now, project: project)
           end
         end
 
@@ -239,4 +240,21 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
         end
       end
     end
+  end
+
+  context 'when query_deploymenys_via_finished_at_in_vsa feature flag is enabled' do
+    before do
+      stub_feature_flags(query_deploymenys_via_finished_at_in_vsa: true)
+    end
+
+    it_behaves_like 'shared examples for #deploys'
+  end
+
+  context 'when query_deploymenys_via_finished_at_in_vsa feature flag is disabled' do
+    before do
+      stub_feature_flags(query_deploymenys_via_finished_at_in_vsa: false)
+    end
+
+    it_behaves_like 'shared examples for #deploys'
+  end
 end

lib/api/deployments.rb

@@ -36,7 +36,7 @@ module API
       get ':id/deployments' do
         authorize! :read_deployment, user_project
 
-        deployments = DeploymentsFinder.new(user_project, params).execute
+        deployments = DeploymentsFinder.new(params.merge(project: user_project)).execute
 
         present paginate(deployments), with: Entities::Deployment
       end

lib/gitlab/cycle_analytics/summary/deploy.rb

@@ -15,6 +15,12 @@ module Gitlab
         private
 
         def deployments_count
+          if Feature.enabled?(:query_deploymenys_via_finished_at_in_vsa)
+            DeploymentsFinder
+              .new(project: @project, finished_after: @from, finished_before: @to, status: :success)
+              .execute
+              .count
+          else
             query = @project.deployments.success.where("created_at >= ?", @from)
             query = query.where("created_at <= ?", @to) if @to
             query.count
@@ -22,4 +28,5 @@ module Gitlab
         end
       end
     end
+  end
 end

spec/finders/deployments_finder_spec.rb

@@ -3,10 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe DeploymentsFinder do
-  subject { described_class.new(project, params).execute }
+  subject { described_class.new(params).execute }
 
-  let(:project) { create(:project, :public, :test_repo) }
-  let(:params) { {} }
+  let_it_be(:project) { create(:project, :public, :test_repo) }
+  let(:params) { { project: project } }
 
   describe "#execute" do
     it 'returns all deployments by default' do
@@ -14,9 +14,17 @@ RSpec.describe DeploymentsFinder do
       is_expected.to match_array(deployments)
     end
 
+    context 'when project is missing' do
+      let(:params) { {} }
+
+      it 'returns nothing' do
+        is_expected.to eq([])
+      end
+    end
+
     describe 'filtering' do
       context 'when updated_at filters are specified' do
-        let(:params) { { updated_before: 1.day.ago, updated_after: 3.days.ago } }
+        let(:params) { { project: project, updated_before: 1.day.ago, updated_after: 3.days.ago } }
         let!(:deployment_1) { create(:deployment, :success, project: project, updated_at: 2.days.ago) }
         let!(:deployment_2) { create(:deployment, :success, project: project, updated_at: 4.days.ago) }
         let!(:deployment_3) { create(:deployment, :success, project: project, updated_at: 1.hour.ago) }
@@ -37,7 +45,7 @@ RSpec.describe DeploymentsFinder do
           create(:deployment, project: project, environment: environment2)
         end
 
-        let(:params) { { environment: environment1.name } }
+        let(:params) { { project: project, environment: environment1.name } }
 
         it 'returns deployments for the given environment' do
           is_expected.to match_array([deployment1])
@@ -47,7 +55,7 @@ RSpec.describe DeploymentsFinder do
       context 'when the deployment status is specified' do
         let!(:deployment1) { create(:deployment, :success, project: project) }
         let!(:deployment2) { create(:deployment, :failed, project: project) }
-        let(:params) { { status: 'success' } }
+        let(:params) { { project: project, status: 'success' } }
 
         it 'returns deployments for the given environment' do
           is_expected.to match_array([deployment1])
@@ -55,22 +63,48 @@ RSpec.describe DeploymentsFinder do
       end
 
       context 'when using an invalid deployment status' do
-        let(:params) { { status: 'kittens' } }
+        let(:params) { { project: project, status: 'kittens' } }
 
         it 'raises ArgumentError' do
           expect { subject }.to raise_error(ArgumentError)
         end
       end
+
+      context 'when filtering by finished time' do
+        let!(:deployment_1) { create(:deployment, :success, project: project, finished_at: 2.days.ago) }
+        let!(:deployment_2) { create(:deployment, :success, project: project, finished_at: 4.days.ago) }
+        let!(:deployment_3) { create(:deployment, :success, project: project, finished_at: 5.hours.ago) }
+
+        context 'when filtering by finished_after and finished_before' do
+          let(:params) { { project: project, finished_after: 3.days.ago, finished_before: 1.day.ago } }
+
+          it { is_expected.to match_array([deployment_1]) }
+        end
+
+        context 'when the finished_before parameter is missing' do
+          let(:params) { { project: project, finished_after: 3.days.ago } }
+
+          it { is_expected.to match_array([deployment_1, deployment_3]) }
+        end
+
+        context 'when finished_after is missing' do
+          let(:params) { { project: project, finished_before: 1.day.ago } }
+
+          it 'does not apply any filters on finished time' do
+            is_expected.to match_array([deployment_1, deployment_2, deployment_3])
+          end
+        end
+      end
     end
 
     describe 'ordering' do
       using RSpec::Parameterized::TableSyntax
 
-      let(:params) { { order_by: order_by, sort: sort } }
+      let(:params) { { project: project, order_by: order_by, sort: sort } }
 
-      let!(:deployment_1) { create(:deployment, :success, project: project, iid: 11, ref: 'master', created_at: 2.days.ago, updated_at: Time.now) }
-      let!(:deployment_2) { create(:deployment, :success, project: project, iid: 12, ref: 'feature', created_at: 1.day.ago, updated_at: 2.hours.ago) }
-      let!(:deployment_3) { create(:deployment, :success, project: project, iid: 8, ref: 'video', created_at: Time.now, updated_at: 1.hour.ago) }
+      let!(:deployment_1) { create(:deployment, :success, project: project, iid: 11, ref: 'master', created_at: 2.days.ago, updated_at: Time.now, finished_at: 3.hours.ago) }
+      let!(:deployment_2) { create(:deployment, :success, project: project, iid: 12, ref: 'feature', created_at: 1.day.ago, updated_at: 2.hours.ago, finished_at: 1.hour.ago) }
+      let!(:deployment_3) { create(:deployment, :success, project: project, iid: 8, ref: 'video', created_at: Time.now, updated_at: 1.hour.ago, finished_at: 2.hours.ago) }
 
       where(:order_by, :sort, :ordered_deployments) do
         'created_at'  | 'asc'  | [:deployment_1, :deployment_2, :deployment_3]
@@ -83,6 +117,8 @@ RSpec.describe DeploymentsFinder do
         'ref'         | 'desc' | [:deployment_3, :deployment_1, :deployment_2]
         'updated_at'  | 'asc'  | [:deployment_2, :deployment_3, :deployment_1]
         'updated_at'  | 'desc' | [:deployment_1, :deployment_3, :deployment_2]
+        'finished_at' | 'asc'  | [:deployment_1, :deployment_3, :deployment_2]
+        'finished_at' | 'desc' | [:deployment_2, :deployment_3, :deployment_1]
         'invalid'     | 'asc'  | [:deployment_1, :deployment_2, :deployment_3]
         'iid'         | 'err'  | [:deployment_3, :deployment_1, :deployment_2]
       end
@@ -95,7 +131,7 @@ RSpec.describe DeploymentsFinder do
     end
 
     describe 'transform `created_at` sorting to `id` sorting' do
-      let(:params) { { order_by: 'created_at', sort: 'asc' } }
+      let(:params) { { project: project, order_by: 'created_at', sort: 'asc' } }
 
       it 'sorts by only one column' do
         expect(subject.order_values.size).to eq(1)
@@ -107,7 +143,7 @@ RSpec.describe DeploymentsFinder do
     end
 
     describe 'tie-breaker for `updated_at` sorting' do
-      let(:params) { { order_by: 'updated_at', sort: 'asc' } }
+      let(:params) { { project: project, order_by: 'updated_at', sort: 'asc' } }
 
       it 'sorts by two columns' do
         expect(subject.order_values.size).to eq(2)

spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb

@@ -218,7 +218,7 @@ RSpec.describe Gitlab::CycleAnalytics::StageSummary do
 
     context 'when `to` is given' do
       before do
-        Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project) }
+        Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project, finished_at: Time.zone.now) }
       end
 
       it 'finds records created between `from` and `to` range' do
@@ -230,10 +230,31 @@ RSpec.describe Gitlab::CycleAnalytics::StageSummary do
       end
 
       context 'when `from` and `to` are within a day' do
+        context 'when query_deploymenys_via_finished_at_in_vsa feature flag is off' do
+          before do
+            stub_feature_flags(query_deploymenys_via_finished_at_in_vsa: false)
+          end
+
           it 'returns the number of deployments made on that day' do
             freeze_time do
               create(:deployment, :success, project: project)
-            options[:from] = options[:to] = Time.now
+              options[:from] = options[:to] = Time.zone.now
+
+              expect(subject).to eq('1')
+            end
+          end
+        end
+
+        context 'when query_deploymenys_via_finished_at_in_vsa feature flag is off' do
+          before do
+            stub_feature_flags(query_deploymenys_via_finished_at_in_vsa: true)
+          end
+
+          it 'returns the number of deployments made on that day' do
+            freeze_time do
+              create(:deployment, :success, project: project, finished_at: Time.zone.now)
+              options[:from] = Time.zone.now.at_beginning_of_day
+              options[:to] = Time.zone.now.at_end_of_day
 
               expect(subject).to eq('1')
             end
@@ -241,4 +262,5 @@ RSpec.describe Gitlab::CycleAnalytics::StageSummary do
         end
       end
     end
+  end
 end