Commit 918e589c authored by Timothy Andrew's avatar Timothy Andrew

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.
parent a4a0ce95
......@@ -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
......@@ -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
......
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
......@@ -43,8 +43,9 @@ class Deployment < ActiveRecord::Base
project.repository.is_ancestor?(commit.id, sha)
end
def update_merge_request_metrics
if environment.update_merge_request_metrics?
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 }).
......@@ -56,11 +57,17 @@ class Deployment < ActiveRecord::Base
# 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_requests.map(&:id), first_deployed_to_production_at: nil).
where(merge_request_id: merge_request_ids, first_deployed_to_production_at: nil).
update_all(first_deployed_to_production_at: self.created_at)
end
end
def previous_deployment
@previous_deployment ||=
......
......@@ -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
......
......@@ -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"))
......
......@@ -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
......@@ -13,7 +13,7 @@ class CreateDeploymentService < BaseService
deployable: deployable
)
deployment.update_merge_request_metrics
deployment.update_merge_request_metrics!
deployment
end
......
......@@ -20,6 +20,7 @@
%span
Environments
- if current_user
= nav_link(controller: %w(cycle_analytics)) do
= link_to project_cycle_analytics_path(@project), title: 'Cycle Analytics' do
%span
......
......@@ -29,7 +29,7 @@ class AddTableMergeRequestMetrics < ActiveRecord::Migration
t.datetime 'latest_build_started_at'
t.datetime 'latest_build_finished_at'
t.datetime 'first_deployed_to_production_at'
t.datetime 'first_deployed_to_production_at', index: true
t.datetime 'merged_at'
t.timestamps null: false
......
......@@ -3,11 +3,15 @@ module Gitlab
module Database
module Median
def median_datetime(arel_table, query_so_far, column_sym)
median_queries =
if Gitlab::Database.postgresql?
pg_median_datetime(arel_table, query_so_far, column_sym)
pg_median_datetime_sql(arel_table, query_so_far, column_sym)
elsif Gitlab::Database.mysql?
mysql_median_datetime(arel_table, query_so_far, column_sym)
mysql_median_datetime_sql(arel_table, query_so_far, column_sym)
end
results = Array.wrap(median_queries).map { |query| Util.run_query(query) }
extract_median(results).presence
end
def extract_median(results)
......@@ -22,7 +26,7 @@ module Gitlab
end
end
def mysql_median_datetime(arel_table, query_so_far, column_sym)
def mysql_median_datetime_sql(arel_table, query_so_far, column_sym)
query = arel_table.
from(arel_table.project(Arel.sql('*')).order(arel_table[column_sym]).as(arel_table.table_name)).
project(average([arel_table[column_sym]], 'median')).
......@@ -44,7 +48,7 @@ module Gitlab
]
end
def pg_median_datetime(arel_table, query_so_far, column_sym)
def pg_median_datetime_sql(arel_table, query_so_far, column_sym)
# Create a CTE with the column we're operating on, row number (after sorting by the column
# we're operating on), and count of the table we're operating on (duplicated across) all rows
# of the CTE. For example, if we're looking to find the median of the `projects.star_count`
......@@ -59,7 +63,8 @@ module Gitlab
cte = Arel::Nodes::As.new(
cte_table,
arel_table.
project(arel_table[column_sym].as(column_sym.to_s),
project(
arel_table[column_sym].as(column_sym.to_s),
Arel::Nodes::Over.new(Arel::Nodes::NamedFunction.new("row_number", []),
Arel::Nodes::Window.new.order(arel_table[column_sym])).as('row_id'),
arel_table.project("COUNT(1)").as('ct')).
......@@ -70,8 +75,10 @@ module Gitlab
# by 'where cte.row_id between cte.ct / 2.0 AND cte.ct / 2.0 + 1'). Find the average of the
# selected rows, and this is the median value.
cte_table.project(average([extract_epoch(cte_table[column_sym])], "median")).
where(Arel::Nodes::Between.new(cte_table[:row_id],
Arel::Nodes::And.new([(cte_table[:ct] / Arel.sql('2.0')),
where(Arel::Nodes::Between.new(
cte_table[:row_id],
Arel::Nodes::And.new(
[(cte_table[:ct] / Arel.sql('2.0')),
(cte_table[:ct] / Arel.sql('2.0') + 1)]))).
with(query_so_far, cte)
end
......
module Gitlab
module Database
module Util
class << self
def run_query(query)
query = query.to_sql unless query.is_a?(String)
ActiveRecord::Base.connection.execute(query)
end
end
end
end
end
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,
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]) }]],
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
......
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,
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? }]],
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)
......
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,
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]) }]],
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])
......
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,
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|
......
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,
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]) }]],
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
......
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,
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 }],
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)
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
......
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,
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
}
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! }]],
......
......@@ -108,7 +108,7 @@ describe MergeRequests::CreateService, services: true do
allow(service).to receive(:execute_hooks)
merge_request = service.execute
expect(merge_request.issues_closed).to match_array([first_issue, second_issue])
expect(merge_request.reload.closes_issues).to match_array([first_issue, second_issue])
end
end
end
......
......@@ -200,7 +200,7 @@ describe MergeRequests::RefreshService, services: true do
allow(refresh_service).to receive(:execute_hooks)
refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature')
expect(merge_request.reload.issues_closed).to eq([issue])
expect(merge_request.reload.closes_issues).to eq([issue])
end
end
......
......@@ -274,7 +274,7 @@ describe MergeRequests::UpdateService, services: true do
allow(service).to receive(:execute_hooks)
service.execute(merge_request)
expect(merge_request.reload.issues_closed).to match_array([first_issue, second_issue])
expect(merge_request.reload.closes_issues).to match_array([first_issue, second_issue])
end
it 'removes `MergeRequestsClosingIssues` records when issues are not closed anymore' do
......@@ -288,13 +288,13 @@ describe MergeRequests::UpdateService, services: true do
merge_request = MergeRequests::CreateService.new(project, user, opts).execute
expect(merge_request.issues_closed).to match_array([first_issue, second_issue])
expect(merge_request.reload.closes_issues).to match_array([first_issue, second_issue])
service = described_class.new(project, user, description: "not closing any issues")
allow(service).to receive(:execute_hooks)
service.execute(merge_request.reload)
expect(merge_request.reload.issues_closed).to be_empty
expect(merge_request.reload.closes_issues).to be_empty
end
end
end
......
......@@ -159,7 +159,3 @@ module CycleAnalyticsHelpers
end
end
end
RSpec.configure do |config|
config.extend CycleAnalyticsHelpers::TestGeneration
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment