Merge branch...

Merge branch '220541-instrument-usage-of-overridden-approval-rules-for-self-managed-instances-2' into 'master'

Add usage ping for MRs with overridden project rules

Closes #220541

See merge request gitlab-org/gitlab!36230

db/migrate/20200817195628_add_modified_to_approval_merge_request_rule.rb

+# frozen_string_literal: true
+
+class AddModifiedToApprovalMergeRequestRule < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :approval_merge_request_rules, :modified_from_project_rule, :boolean, default: false, null: false
+  end
+end

db/post_migrate/20200901170135_backfill_modified_column_for_approval_merge_request_rules.rb

+# frozen_string_literal: true
+
+class BackfillModifiedColumnForApprovalMergeRequestRules < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::Migrations::BackgroundMigrationHelpers
+
+  disable_ddl_transaction!
+
+  class ApprovalMergeRequestRule < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'approval_merge_request_rules'
+  end
+
+  def change
+    queue_background_migration_jobs_by_range_at_intervals(ApprovalMergeRequestRule, 'AddModifiedToApprovalMergeRequestRule', 2.minutes, batch_size: 10_000)
+  end
+end

db/schema_migrations/20200817195628

+4da25ee40eabd81765b562929c819da1fc2b0f8afe1f4eefe6b769fcd8f0d4cd
\ No newline at end of file

db/schema_migrations/20200901170135

+fca99780272ca4ceb42fd38d16f67cd4a0a675ecdaf91e51c4c0728205212ed0
\ No newline at end of file

db/structure.sql

@@ -9338,6 +9338,7 @@ CREATE TABLE approval_merge_request_rules (
     rule_type smallint DEFAULT 1 NOT NULL,
     report_type smallint,
     section text,
+    modified_from_project_rule boolean DEFAULT false NOT NULL,
     CONSTRAINT check_6fca5928b2 CHECK ((char_length(section) <= 255))
 );
 

ee/app/models/approval_merge_request_rule.rb

@@ -37,8 +37,11 @@ class ApprovalMergeRequestRule < ApplicationRecord
   has_and_belongs_to_many :approved_approvers, class_name: 'User', join_table: :approval_merge_request_rules_approved_approvers
   has_one :approval_merge_request_rule_source
   has_one :approval_project_rule, through: :approval_merge_request_rule_source
+  has_one :approval_project_rule_project, through: :approval_project_rule, source: :project
   alias_method :source_rule, :approval_project_rule
 
+  before_update :compare_with_project_rule
+
   validate :validate_approval_project_rule
 
   enum rule_type: {
@@ -61,6 +64,11 @@ class ApprovalMergeRequestRule < ApplicationRecord
   scope :with_head_pipeline, -> { includes(merge_request: [:head_pipeline]) }
   scope :open_merge_requests, -> { merge(MergeRequest.opened) }
   scope :for_checks_that_can_be_refreshed, -> { license_compliance.open_merge_requests.with_head_pipeline }
+  scope :with_projects_that_can_override_rules, -> do
+    joins(:approval_project_rule_project)
+      .where(projects: { disable_overriding_approvers_per_merge_request: [false, nil] })
+  end
+  scope :modified_from_project_rule, -> { with_projects_that_can_override_rules.where(modified_from_project_rule: true) }
 
   def self.find_or_create_code_owner_rule(merge_request, entry)
     merge_request.approval_rules.code_owner.where(name: entry.pattern).where(section: entry.section).first_or_create do |rule|
@@ -125,6 +133,10 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   private
 
+  def compare_with_project_rule
+    self.modified_from_project_rule = overridden? ? true : false
+  end
+
   def validate_approval_project_rule
     return if approval_project_rule.blank?
     return if merge_request.project == approval_project_rule.project

ee/app/models/approval_project_rule.rb

@@ -6,6 +6,8 @@ class ApprovalProjectRule < ApplicationRecord
 
   belongs_to :project
   has_and_belongs_to_many :protected_branches
+  has_many :approval_merge_request_rule_sources
+  has_many :approval_merge_request_rules, through: :approval_merge_request_rule_sources
 
   enum rule_type: {
     regular: 0,

ee/app/models/ee/merge_request.rb

@@ -34,6 +34,8 @@ module EE
           end
         end
       end
+      has_many :approval_merge_request_rule_sources, through: :approval_rules
+      has_many :approval_project_rules, through: :approval_merge_request_rule_sources
       has_one :merge_train, inverse_of: :merge_request, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
       has_many :blocks_as_blocker,

ee/changelogs/unreleased/220541-instrument-usage-of-overridden-approval-rules-for-self-managed-.yml

+---
+title: Add usage ping for MRs with overridden project rules
+merge_request: 36230
+author:
+type: added

ee/lib/ee/gitlab/usage_data.rb

@@ -190,6 +190,7 @@ module EE
                                                                 start: approval_merge_request_rule_minimum_id,
                                                                 finish: approval_merge_request_rule_maximum_id),
                 merge_requests_with_optional_codeowners: distinct_count(::ApprovalMergeRequestRule.code_owner_approval_optional, :merge_request_id),
+                merge_requests_with_overridden_project_rules: merge_requests_with_overridden_project_rules,
                 merge_requests_with_required_codeowners: distinct_count(::ApprovalMergeRequestRule.code_owner_approval_required, :merge_request_id),
                 projects_mirrored_with_pipelines_enabled: count(::Project.mirrored_with_enabled_pipelines),
                 projects_reporting_ci_cd_back_to_github: count(::GithubService.active),
@@ -241,6 +242,7 @@ module EE
                                                             start: approval_merge_request_rule_minimum_id,
                                                             finish: approval_merge_request_rule_maximum_id),
             merge_requests_with_optional_codeowners: distinct_count(::ApprovalMergeRequestRule.code_owner_approval_optional.where(time_period), :merge_request_id),
+            merge_requests_with_overridden_project_rules: merge_requests_with_overridden_project_rules(time_period),
             merge_requests_with_required_codeowners: distinct_count(::ApprovalMergeRequestRule.code_owner_approval_required.where(time_period), :merge_request_id),
             projects_imported_from_github: distinct_count(::Project.github_imported.where(time_period), :creator_id),
             projects_with_repositories_enabled: distinct_count(::Project.with_repositories_enabled.where(time_period),
@@ -386,13 +388,13 @@ module EE
 
         def approval_merge_request_rule_minimum_id
           strong_memoize(:approval_merge_request_rule_minimum_id) do
-            ::ApprovalMergeRequestRule.minimum(:id)
+            ::ApprovalMergeRequestRule.minimum(:merge_request_id)
           end
         end
 
         def approval_merge_request_rule_maximum_id
           strong_memoize(:approval_merge_request_rule_maximum_id) do
-            ::ApprovalMergeRequestRule.maximum(:id)
+            ::ApprovalMergeRequestRule.maximum(:merge_request_id)
           end
         end
 
@@ -404,7 +406,42 @@ module EE
           ::Gitlab::Auth::Ldap::Config.available_servers
         end
 
-        # rubocop:disable CodeReuse/ActiveRecord
+        def merge_requests_with_overridden_project_rules(time_period = nil)
+          sql =
+            <<~SQL
+          (EXISTS (
+            SELECT
+              1
+            FROM
+              approval_merge_request_rule_sources
+            WHERE
+              approval_merge_request_rule_sources.approval_merge_request_rule_id = approval_merge_request_rules.id
+              AND NOT EXISTS (
+                SELECT
+                  1
+                FROM
+                  approval_project_rules
+                WHERE
+                  approval_project_rules.id = approval_merge_request_rule_sources.approval_project_rule_id
+                  AND EXISTS (
+                    SELECT
+                      1
+                    FROM
+                      projects
+                    WHERE
+                      projects.id = approval_project_rules.project_id
+                      AND projects.disable_overriding_approvers_per_merge_request = FALSE))))
+              OR("approval_merge_request_rules"."modified_from_project_rule" = TRUE)
+            SQL
+
+          distinct_count(
+            ::ApprovalMergeRequestRule.where(time_period).where(sql),
+            :merge_request_id,
+            start: approval_merge_request_rule_minimum_id,
+            finish: approval_merge_request_rule_maximum_id
+          )
+        end
+
         def projects_jira_issuelist_active
           # rubocop: disable UsageData/LargeTable:
           min_id = JiraTrackerData.where(issues_enabled: true).minimum(:service_id)

ee/spec/lib/ee/gitlab/usage_data_spec.rb

@@ -281,9 +281,13 @@ RSpec.describe Gitlab::UsageData do
         project = create(:project, :repository_private, :github_imported,
                           :test_repo, creator: user)
         merge_request = create(:merge_request, source_project: project)
+        overridden_merge_request = create(:merge_request, source_project: project, target_branch: "override")
         project_rule = create(:approval_project_rule, project: project)
+        overridden_project_rule = create(:approval_project_rule, project: project, approvals_required: 1)
         merge_rule = create(:approval_merge_request_rule, merge_request: merge_request)
+        overridden_mr_rule = create(:approval_merge_request_rule, merge_request: overridden_merge_request, approvals_required: 5)
         create(:approval_merge_request_rule_source, approval_merge_request_rule: merge_rule, approval_project_rule: project_rule)
+        create(:approval_merge_request_rule_source, approval_merge_request_rule: overridden_mr_rule, approval_project_rule: overridden_project_rule)
         create(:project, creator: user)
         create(:project, creator: user, disable_overriding_approvers_per_merge_request: true)
         create(:project, creator: user, disable_overriding_approvers_per_merge_request: false)
@@ -315,7 +319,7 @@ RSpec.describe Gitlab::UsageData do
       end
 
       expect(described_class.usage_activity_by_stage_create({})).to include(
-        approval_project_rules: 8,
+        approval_project_rules: 10,
         approval_project_rules_with_target_branch: 2,
         approval_project_rules_with_more_approvers_than_required: 2,
         approval_project_rules_with_less_approvers_than_required: 2,
@@ -325,6 +329,7 @@ RSpec.describe Gitlab::UsageData do
         merge_requests_with_added_rules: 12,
         merge_requests_with_optional_codeowners: 4,
         merge_requests_with_required_codeowners: 8,
+        merge_requests_with_overridden_project_rules: 4,
         projects_imported_from_github: 2,
         projects_with_repositories_enabled: 26,
         protected_branches: 2,
@@ -335,7 +340,7 @@ RSpec.describe Gitlab::UsageData do
         total_number_of_locked_files: 14
       )
       expect(described_class.usage_activity_by_stage_create(described_class.last_28_days_time_period)).to include(
-        approval_project_rules: 8,
+        approval_project_rules: 10,
         approval_project_rules_with_target_branch: 2,
         approval_project_rules_with_more_approvers_than_required: 2,
         approval_project_rules_with_less_approvers_than_required: 2,

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -7,6 +7,12 @@ RSpec.describe ApprovalMergeRequestRule do
 
   subject { create(:approval_merge_request_rule, merge_request: merge_request) }
 
+  describe 'associations' do
+    subject { build_stubbed(:approval_merge_request_rule) }
+
+    it { is_expected.to have_one(:approval_project_rule_project).through(:approval_project_rule) }
+  end
+
   describe 'validations' do
     it 'is valid' do
       expect(build(:approval_merge_request_rule)).to be_valid

ee/spec/models/approval_project_rule_spec.rb

@@ -11,6 +11,13 @@ RSpec.describe ApprovalProjectRule do
     end
   end
 
+  describe 'associations' do
+    subject { build_stubbed(:approval_project_rule) }
+
+    it { is_expected.to have_many(:approval_merge_request_rule_sources) }
+    it { is_expected.to have_many(:approval_merge_request_rules).through(:approval_merge_request_rule_sources) }
+  end
+
   describe '.regular' do
     it 'returns non-report_approver records' do
       rules = create_list(:approval_project_rule, 2)

ee/spec/models/merge_request_spec.rb

@@ -25,6 +25,8 @@ RSpec.describe MergeRequest do
     it { is_expected.to have_many(:approved_by_users) }
     it { is_expected.to have_one(:merge_train) }
     it { is_expected.to have_many(:approval_rules) }
+    it { is_expected.to have_many(:approval_merge_request_rule_sources).through(:approval_rules) }
+    it { is_expected.to have_many(:approval_project_rules).through(:approval_merge_request_rule_sources) }
 
     describe 'approval_rules association' do
       describe '#applicable_to_branch' do

lib/gitlab/background_migration/add_modified_to_approval_merge_request_rule.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # Compare all current rules to project rules
+    class AddModifiedToApprovalMergeRequestRule
+      # Stubbed class to access the Group table
+      class Group < ActiveRecord::Base
+        self.table_name = 'namespaces'
+        self.inheritance_column = :_type_disabled
+      end
+
+      # Stubbed class to access the ApprovalMergeRequestRule table
+      class ApprovalMergeRequestRule < ActiveRecord::Base
+        self.table_name = 'approval_merge_request_rules'
+
+        has_one :approval_merge_request_rule_source, class_name: 'AddModifiedToApprovalMergeRequestRule::ApprovalMergeRequestRuleSource'
+        has_one :approval_project_rule, through: :approval_merge_request_rule_source
+        has_and_belongs_to_many :groups,
+          class_name: 'AddModifiedToApprovalMergeRequestRule::Group', join_table: "#{self.table_name}_groups"
+      end
+
+      # Stubbed class to access the ApprovalProjectRule table
+      class ApprovalProjectRule < ActiveRecord::Base
+        self.table_name = 'approval_project_rules'
+
+        has_many :approval_merge_request_rule_sources, class_name: 'AddModifiedToApprovalMergeRequestRule::ApprovalMergeRequestRuleSource'
+        has_and_belongs_to_many :groups,
+          class_name: 'AddModifiedToApprovalMergeRequestRule::Group', join_table: "#{self.table_name}_groups"
+      end
+
+      # Stubbed class to access the ApprovalMergeRequestRuleSource table
+      class ApprovalMergeRequestRuleSource < ActiveRecord::Base
+        self.table_name = 'approval_merge_request_rule_sources'
+
+        belongs_to :approval_merge_request_rule, class_name: 'AddModifiedToApprovalMergeRequestRule::ApprovalMergeRequestRule'
+        belongs_to :approval_project_rule, class_name: 'AddModifiedToApprovalMergeRequestRule::ApprovalProjectRule'
+      end
+
+      def perform(start_id, stop_id)
+        approval_merge_requests_rules = ApprovalMergeRequestRule
+          .joins(:groups, :approval_merge_request_rule_source)
+          .where(id: start_id..stop_id)
+          .pluck(
+            'approval_merge_request_rule_sources.id as ars_id',
+            'approval_merge_request_rules_groups.id as amrg_id'
+          )
+
+        approval_project_rules = ApprovalProjectRule
+          .joins(:groups, approval_merge_request_rule_sources: :approval_merge_request_rule)
+          .where(approval_merge_request_rules: { id: start_id..stop_id })
+          .pluck(
+            'approval_merge_request_rule_sources.id as ars_id',
+            'approval_project_rules_groups.id as apg_id'
+          )
+
+        different_names_or_approval_sources = ApprovalMergeRequestRule.joins(:approval_project_rule, :approval_merge_request_rule_source)
+          .where(id: start_id..stop_id)
+          .where('approval_merge_request_rules.name != approval_project_rules.name OR ' \
+                'approval_merge_request_rules.approvals_required != approval_project_rules.approvals_required')
+          .pluck('approval_merge_request_rule_sources.id as ars_id')
+
+        intersected_set = approval_merge_requests_rules.to_set ^ approval_project_rules.to_set
+        source_ids = intersected_set.collect { |rule| rule[0] }.uniq
+
+        rule_sources = ApprovalMergeRequestRuleSource.where(id: source_ids + different_names_or_approval_sources)
+        changed_merge_request_rules = ApprovalMergeRequestRule.where(id: rule_sources.select(:approval_merge_request_rule_id))
+
+        changed_merge_request_rules.update_all(modified_from_project_rule: true)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/add_modified_to_approval_merge_request_rule_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::AddModifiedToApprovalMergeRequestRule, schema: 20200817195628 do
+  let(:determine_if_rules_are_modified) { described_class.new }
+
+  let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab') }
+  let(:projects) { table(:projects) }
+  let(:normal_project) { projects.create!(namespace_id: namespace.id) }
+  let(:overridden_project) { projects.create!(namespace_id: namespace.id) }
+  let(:rules) { table(:approval_merge_request_rules) }
+  let(:project_rules) { table(:approval_project_rules) }
+  let(:sources) { table(:approval_merge_request_rule_sources) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:groups) { table(:namespaces) }
+  let(:mr_groups) { table(:approval_merge_request_rules_groups) }
+  let(:project_groups) { table(:approval_project_rules_groups) }
+
+  before do
+    project_rule = project_rules.create!(project_id: normal_project.id, approvals_required: 3, name: 'test rule')
+    overridden_project_rule = project_rules.create!(project_id: overridden_project.id, approvals_required: 5, name: 'other test rule')
+    overridden_project_rule_two = project_rules.create!(project_id: overridden_project.id, approvals_required: 7, name: 'super cool rule')
+
+    merge_request = merge_requests.create!(target_branch: 'feature', source_branch: 'default', source_project_id: normal_project.id, target_project_id: normal_project.id)
+    overridden_merge_request = merge_requests.create!(target_branch: 'feature-2', source_branch: 'default', source_project_id: overridden_project.id, target_project_id: overridden_project.id)
+
+    merge_rule = rules.create!(merge_request_id: merge_request.id, approvals_required: 3, name: 'test rule')
+    overridden_merge_rule = rules.create!(merge_request_id: overridden_merge_request.id, approvals_required: 6, name: 'other test rule')
+    overridden_merge_rule_two = rules.create!(merge_request_id: overridden_merge_request.id, approvals_required: 7, name: 'super cool rule')
+
+    sources.create!(approval_project_rule_id: project_rule.id, approval_merge_request_rule_id: merge_rule.id)
+    sources.create!(approval_project_rule_id: overridden_project_rule.id, approval_merge_request_rule_id: overridden_merge_rule.id)
+    sources.create!(approval_project_rule_id: overridden_project_rule_two.id, approval_merge_request_rule_id: overridden_merge_rule_two.id)
+
+    group1 = groups.create!(name: "group1", path: "test_group1", type: 'Group')
+    group2 = groups.create!(name: "group2", path: "test_group2", type: 'Group')
+    group3 = groups.create!(name: "group3", path: "test_group3", type: 'Group')
+
+    project_groups.create!(approval_project_rule_id: overridden_project_rule_two.id, group_id: group1.id)
+    project_groups.create!(approval_project_rule_id: overridden_project_rule_two.id, group_id: group2.id)
+    project_groups.create!(approval_project_rule_id: overridden_project_rule_two.id, group_id: group3.id)
+
+    mr_groups.create!(approval_merge_request_rule_id: overridden_merge_rule.id, group_id: group1.id)
+    mr_groups.create!(approval_merge_request_rule_id: overridden_merge_rule_two.id, group_id: group2.id)
+  end
+
+  describe '#perform' do
+    it 'changes the correct rules' do
+      original_count = rules.all.count
+
+      determine_if_rules_are_modified.perform(rules.minimum(:id), rules.maximum(:id))
+
+      results = rules.where(modified_from_project_rule: true)
+
+      expect(results.count).to eq 2
+      expect(results.collect(&:name)).to eq(['other test rule', 'super cool rule'])
+      expect(rules.count).to eq original_count
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -159,6 +159,8 @@ merge_requests:
 - assignees
 - reviews
 - approval_rules
+- approval_merge_request_rule_sources
+- approval_project_rules
 - approvals
 - approvers
 - approver_users
@@ -467,6 +469,8 @@ project:
 - feature_usage
 - approval_rules
 - approval_merge_request_rules
+- approval_merge_request_rule_sources
+- approval_project_rules
 - approvers
 - approver_users
 - audit_events