From 1c9be5d7f53bdd7f28988eab154a504659aabcfa Mon Sep 17 00:00:00 2001
From: Pavel Shutsin <pshutsin@gitlab.com>
Date: Tue, 1 Oct 2019 13:44:21 +0300
Subject: [PATCH] Limit Productivity Analytics date filtering
Users shouldn't be able to select dates for filtering earlier
than the feature was introduced.
---
.../application_setting_implementation.rb | 3 +-
.../32052-add-pa-start-date-limit.yml | 5 +++
...8_add_productivity_analytics_start_date.rb | 11 ++++++
..._fill_productivity_analytics_start_date.rb | 31 +++++++++++++++
db/schema.rb | 1 +
.../finders/productivity_analytics_finder.rb | 16 +++++++-
ee/app/models/productivity_analytics.rb | 4 ++
.../productivity_analytics/show.html.haml | 2 +-
.../productivity_analytics_finder_spec.rb | 20 ++++++++++
ee/spec/models/productivity_analytics_spec.rb | 15 ++++++-
..._productivity_analytics_start_date_spec.rb | 39 +++++++++++++++++++
11 files changed, 142 insertions(+), 5 deletions(-)
create mode 100644 changelogs/unreleased/32052-add-pa-start-date-limit.yml
create mode 100644 db/migrate/20191004080818_add_productivity_analytics_start_date.rb
create mode 100644 db/migrate/20191004081520_fill_productivity_analytics_start_date.rb
create mode 100644 spec/migrations/fill_productivity_analytics_start_date_spec.rb
diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb
index 9119f8766ebe0..339563b7e4be3 100644
--- a/app/models/application_setting_implementation.rb
+++ b/app/models/application_setting_implementation.rb
@@ -136,7 +136,8 @@ def defaults
snowplow_iglu_registry_url: nil,
custom_http_clone_url_root: nil,
pendo_enabled: false,
- pendo_url: nil
+ pendo_url: nil,
+ productivity_analytics_start_date: Time.now
}
end
diff --git a/changelogs/unreleased/32052-add-pa-start-date-limit.yml b/changelogs/unreleased/32052-add-pa-start-date-limit.yml
new file mode 100644
index 0000000000000..fa2cf796edbb4
--- /dev/null
+++ b/changelogs/unreleased/32052-add-pa-start-date-limit.yml
@@ -0,0 +1,5 @@
+---
+title: Add productivity analytics merge date filtering limit
+merge_request: 32052
+author:
+type: fixed
diff --git a/db/migrate/20191004080818_add_productivity_analytics_start_date.rb b/db/migrate/20191004080818_add_productivity_analytics_start_date.rb
new file mode 100644
index 0000000000000..287b0755bc190
--- /dev/null
+++ b/db/migrate/20191004080818_add_productivity_analytics_start_date.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddProductivityAnalyticsStartDate < ActiveRecord::Migration[5.2]
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ def change
+ add_column :application_settings, :productivity_analytics_start_date, :datetime_with_timezone
+ end
+end
diff --git a/db/migrate/20191004081520_fill_productivity_analytics_start_date.rb b/db/migrate/20191004081520_fill_productivity_analytics_start_date.rb
new file mode 100644
index 0000000000000..9432cd6870883
--- /dev/null
+++ b/db/migrate/20191004081520_fill_productivity_analytics_start_date.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+# Expected migration duration: 1 minute
+class FillProductivityAnalyticsStartDate < ActiveRecord::Migration[5.2]
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ add_concurrent_index :merge_request_metrics, :merged_at,
+ where: "merged_at > '2019-09-01' AND commits_count IS NOT NULL",
+ name: 'fill_productivity_analytics_start_date_tmp_index'
+
+ execute(
+ <<SQL
+ UPDATE application_settings
+ SET productivity_analytics_start_date = COALESCE((SELECT MIN(merged_at) FROM merge_request_metrics
+ WHERE merged_at > '2019-09-01' AND commits_count IS NOT NULL), NOW())
+SQL
+ )
+
+ remove_concurrent_index :merge_request_metrics, :merged_at,
+ name: 'fill_productivity_analytics_start_date_tmp_index'
+ end
+
+ def down
+ execute('UPDATE application_settings SET productivity_analytics_start_date = NULL')
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 36f82f9913b7a..1c3c024743fc2 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -350,6 +350,7 @@
t.string "eks_access_key_id", limit: 128
t.string "encrypted_eks_secret_access_key_iv", limit: 255
t.text "encrypted_eks_secret_access_key"
+ t.datetime_with_timezone "productivity_analytics_start_date"
t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id"
diff --git a/ee/app/finders/productivity_analytics_finder.rb b/ee/app/finders/productivity_analytics_finder.rb
index 88d4dec285a86..ff18d656ff564 100644
--- a/ee/app/finders/productivity_analytics_finder.rb
+++ b/ee/app/finders/productivity_analytics_finder.rb
@@ -37,10 +37,22 @@ def by_merged_at(items)
return items unless params[:merged_at_after] || params[:merged_at_before]
items = items.joins(:metrics)
- items = items.where(metrics_table[:merged_at].gteq(params[:merged_at_after])) if params[:merged_at_after]
- items = items.where(metrics_table[:merged_at].lteq(params[:merged_at_before])) if params[:merged_at_before]
+ items = items.where(metrics_table[:merged_at].gteq(merged_at_between[:from])) if merged_at_between[:from]
+ items = items.where(metrics_table[:merged_at].lteq(merged_at_between[:to])) if merged_at_between[:to]
items
end
# rubocop: enable CodeReuse/ActiveRecord
+
+ def merged_at_between
+ @merged_at_between ||= begin
+ boundaries = { from: params[:merged_at_after], to: params[:merged_at_before] }
+
+ if ProductivityAnalytics.start_date && ProductivityAnalytics.start_date > boundaries[:from]
+ boundaries[:from] = ProductivityAnalytics.start_date
+ end
+
+ boundaries
+ end
+ end
end
diff --git a/ee/app/models/productivity_analytics.rb b/ee/app/models/productivity_analytics.rb
index 760e810358b76..63615d6b13b1f 100644
--- a/ee/app/models/productivity_analytics.rb
+++ b/ee/app/models/productivity_analytics.rb
@@ -16,6 +16,10 @@ class ProductivityAnalytics
METRIC_TYPES = METRIC_COLUMNS.keys.freeze
DEFAULT_TYPE = 'days_to_merge'.freeze
+ def self.start_date
+ ApplicationSetting.current&.productivity_analytics_start_date
+ end
+
def initialize(merge_requests:, sort: nil)
@merge_requests = merge_requests.joins(:metrics)
@sort = sort
diff --git a/ee/app/views/analytics/productivity_analytics/show.html.haml b/ee/app/views/analytics/productivity_analytics/show.html.haml
index 8ecacd091c80b..be1d78e6cab20 100644
--- a/ee/app/views/analytics/productivity_analytics/show.html.haml
+++ b/ee/app/views/analytics/productivity_analytics/show.html.haml
@@ -5,5 +5,5 @@
.js-group-project-select-container
.js-search-bar.filter-container.hide
= render 'shared/issuable/search_bar', type: :productivity_analytics
- .js-timeframe-container
+ .js-timeframe-container{ data: { start_date: ProductivityAnalytics.start_date } }
.js-productivity-analytics-app-container{ data: { endpoint: analytics_productivity_analytics_path, empty_state_svg_path: image_path('illustrations/productivity-analytics-empty-state.svg'), no_access_svg_path: image_path('illustrations/analytics/no-access.svg') } }
diff --git a/ee/spec/finders/productivity_analytics_finder_spec.rb b/ee/spec/finders/productivity_analytics_finder_spec.rb
index a455f2f95d86c..2c0e54c2e980f 100644
--- a/ee/spec/finders/productivity_analytics_finder_spec.rb
+++ b/ee/spec/finders/productivity_analytics_finder_spec.rb
@@ -44,6 +44,12 @@
end
context 'allows to filter by merged_at' do
+ let(:pa_start_date) { 2.years.ago }
+
+ before do
+ allow(ProductivityAnalytics).to receive(:start_date).and_return(pa_start_date)
+ end
+
around do |example|
Timecop.freeze { example.run }
end
@@ -76,6 +82,20 @@
expect(subject.execute).to match_array([short_mr])
end
end
+
+ context 'with merged_at_after earlier than PA start date' do
+ let(:search_params) do
+ { merged_at_after: 3.years.ago.to_s }
+ end
+
+ it 'uses start_date as filter value' do
+ metrics_data = { merged_at: (2.years + 1.day).ago }
+ create(:merge_request, :merged, :with_productivity_metrics, created_at: 800.days.ago, metrics_data: metrics_data)
+ long_mr
+
+ expect(subject.execute).to match_array([long_mr])
+ end
+ end
end
end
end
diff --git a/ee/spec/models/productivity_analytics_spec.rb b/ee/spec/models/productivity_analytics_spec.rb
index 7524481503507..1c9886b9dfe05 100644
--- a/ee/spec/models/productivity_analytics_spec.rb
+++ b/ee/spec/models/productivity_analytics_spec.rb
@@ -4,7 +4,7 @@
describe ProductivityAnalytics do
describe 'metrics data' do
- let(:analytics) { described_class.new(merge_requests: finder_mrs, sort: custom_sort) }
+ subject(:analytics) { described_class.new(merge_requests: finder_mrs, sort: custom_sort) }
let(:finder_mrs) { ProductivityAnalyticsFinder.new(create(:admin), finder_options).execute }
let(:finder_options) { { state: 'merged' } }
@@ -221,4 +221,17 @@
end
end
end
+
+ describe '.start_date' do
+ subject(:start_date) { described_class.start_date }
+
+ let(:application_setting) do
+ instance_double('ApplicationSetting', productivity_analytics_start_date: 'mocked-start-date')
+ end
+
+ it 'delegates to ApplicationSetting' do
+ allow(ApplicationSetting).to receive('current').and_return(application_setting)
+ expect(start_date).to eq 'mocked-start-date'
+ end
+ end
end
diff --git a/spec/migrations/fill_productivity_analytics_start_date_spec.rb b/spec/migrations/fill_productivity_analytics_start_date_spec.rb
new file mode 100644
index 0000000000000..7cbba9ef20e79
--- /dev/null
+++ b/spec/migrations/fill_productivity_analytics_start_date_spec.rb
@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20191004081520_fill_productivity_analytics_start_date.rb')
+
+describe FillProductivityAnalyticsStartDate, :migration do
+ let(:settings_table) { table('application_settings') }
+ let(:metrics_table) { table('merge_request_metrics') }
+
+ before do
+ settings_table.create!
+ end
+
+ context 'with NO productivity analytics data available' do
+ it 'sets start_date to NOW' do
+ expect { migrate! }.to change {
+ settings_table.first&.productivity_analytics_start_date
+ }.to(be_like_time(Time.now))
+ end
+ end
+
+ context 'with productivity analytics data available' do
+ before do
+ ActiveRecord::Base.transaction do
+ ActiveRecord::Base.connection.execute('ALTER TABLE merge_request_metrics DISABLE TRIGGER ALL')
+ metrics_table.create!(merged_at: Time.parse('2019-09-09'), commits_count: nil, merge_request_id: 3)
+ metrics_table.create!(merged_at: Time.parse('2019-10-10'), commits_count: 5, merge_request_id: 1)
+ metrics_table.create!(merged_at: Time.parse('2019-11-11'), commits_count: 10, merge_request_id: 2)
+ ActiveRecord::Base.connection.execute('ALTER TABLE merge_request_metrics ENABLE TRIGGER ALL')
+ end
+ end
+
+ it 'set start_date to earliest merged_at value with PA data available' do
+ expect { migrate! }.to change {
+ settings_table.first&.productivity_analytics_start_date
+ }.to(be_like_time(Time.parse('2019-10-10')))
+ end
+ end
+end
--
GitLab