From 918e589c2b29c18d9fe3a8e6c93a3f490c86beb1 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 21 Sep 2016 00:47:37 +0530 Subject: 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. --- app/models/cycle_analytics.rb | 66 +++++++++++--------------- app/models/cycle_analytics/summary.rb | 2 +- app/models/cycle_analytics/table_references.rb | 25 ---------- app/models/deployment.rb | 37 +++++++++------ app/models/issue.rb | 3 +- app/models/merge_request.rb | 8 +--- app/models/merge_requests_closing_issues.rb | 6 +-- app/services/create_deployment_service.rb | 2 +- app/views/projects/pipelines/_head.html.haml | 9 ++-- 9 files changed, 64 insertions(+), 94 deletions(-) delete mode 100644 app/models/cycle_analytics/table_references.rb (limited to 'app') diff --git a/app/models/cycle_analytics.rb b/app/models/cycle_analytics.rb index 3ca160252ff..be295487fd2 100644 --- a/app/models/cycle_analytics.rb +++ b/app/models/cycle_analytics.rb @@ -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 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 diff --git a/app/models/cycle_analytics/summary.rb b/app/models/cycle_analytics/summary.rb index 04a90a8da49..53b2cacb131 100644 --- a/app/models/cycle_analytics/summary.rb +++ b/app/models/cycle_analytics/summary.rb @@ -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 diff --git a/app/models/cycle_analytics/table_references.rb b/app/models/cycle_analytics/table_references.rb deleted file mode 100644 index f276723ee0e..00000000000 --- a/app/models/cycle_analytics/table_references.rb +++ /dev/null @@ -1,25 +0,0 @@ -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 diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 15f349c89bb..07d7e19e70d 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -43,23 +43,30 @@ class Deployment < ActiveRecord::Base project.repository.is_ancestor?(commit.id, sha) end - def update_merge_request_metrics - if 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 }). - where("merge_request_metrics.merged_at <= ?", self.created_at) - - if previous_deployment - merge_requests = merge_requests.where("merge_request_metrics.merged_at >= ?", previous_deployment.created_at) - end + 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 }). + where("merge_request_metrics.merged_at <= ?", self.created_at) - # Need to use `map` instead of `select` because MySQL doesn't allow `SELECT`ing from the same table - # that we're updating. - MergeRequest::Metrics. - where(merge_request_id: merge_requests.map(&:id), first_deployed_to_production_at: nil). - update_all(first_deployed_to_production_at: self.created_at) + if previous_deployment + merge_requests = merge_requests.where("merge_request_metrics.merged_at >= ?", previous_deployment.created_at) end + + # 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_request_ids, first_deployed_to_production_at: nil). + update_all(first_deployed_to_production_at: self.created_at) end def previous_deployment diff --git a/app/models/issue.rb b/app/models/issue.rb index 6a264ace165..25ba38a1cff 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0ac291ce1a0..c05718f4d5a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -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")) diff --git a/app/models/merge_requests_closing_issues.rb b/app/models/merge_requests_closing_issues.rb index cd49076002d..ab597c37947 100644 --- a/app/models/merge_requests_closing_issues.rb +++ b/app/models/merge_requests_closing_issues.rb @@ -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 diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index e958e91d612..799ad3e1bd0 100644 --- a/app/services/create_deployment_service.rb +++ b/app/services/create_deployment_service.rb @@ -13,7 +13,7 @@ class CreateDeploymentService < BaseService deployable: deployable ) - deployment.update_merge_request_metrics + deployment.update_merge_request_metrics! deployment end diff --git a/app/views/projects/pipelines/_head.html.haml b/app/views/projects/pipelines/_head.html.haml index fa1470f5fbc..a97674e84ba 100644 --- a/app/views/projects/pipelines/_head.html.haml +++ b/app/views/projects/pipelines/_head.html.haml @@ -20,7 +20,8 @@ %span Environments - = nav_link(controller: %w(cycle_analytics)) do - = link_to project_cycle_analytics_path(@project), title: 'Cycle Analytics' do - %span - Cycle Analytics + - if current_user + = nav_link(controller: %w(cycle_analytics)) do + = link_to project_cycle_analytics_path(@project), title: 'Cycle Analytics' do + %span + Cycle Analytics -- cgit v1.2.1