Commit cd53569c authored by Gary Holtz's avatar Gary Holtz

Provides us a usage ping entry for overridden approval rules for MRs:

This requires the Can override approvals in merge request
option checked in repository settings.

This update also includes:

Adding some relationship links:

* ApprovalMergeRequestRule has_one Project
* ApprovalProjectRule has_many ApprovalMergeRequestRules
* ApprovalProjectRule has_many ApprovalMergeRequestRuleSources
* EE::MergeRequest has_many ApprovalMergeRequestRuleSources
* EE::MergeRequest has_many ApprovalProjectRules
* Scopes for Approval Merge Requests
* Scopes for EE:MergeRequest
* Two memoized functions for maximum/minimum IDs in the EE usage ping
parent f207da68
# 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
# 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
4da25ee40eabd81765b562929c819da1fc2b0f8afe1f4eefe6b769fcd8f0d4cd
\ No newline at end of file
fca99780272ca4ceb42fd38d16f67cd4a0a675ecdaf91e51c4c0728205212ed0
\ No newline at end of file
...@@ -9312,6 +9312,7 @@ CREATE TABLE approval_merge_request_rules ( ...@@ -9312,6 +9312,7 @@ CREATE TABLE approval_merge_request_rules (
rule_type smallint DEFAULT 1 NOT NULL, rule_type smallint DEFAULT 1 NOT NULL,
report_type smallint, report_type smallint,
section text, section text,
modified_from_project_rule boolean DEFAULT false NOT NULL,
CONSTRAINT check_6fca5928b2 CHECK ((char_length(section) <= 255)) CONSTRAINT check_6fca5928b2 CHECK ((char_length(section) <= 255))
); );
......
...@@ -37,8 +37,11 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -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_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_merge_request_rule_source
has_one :approval_project_rule, through: :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 alias_method :source_rule, :approval_project_rule
before_update :compare_with_project_rule
validate :validate_approval_project_rule validate :validate_approval_project_rule
enum rule_type: { enum rule_type: {
...@@ -61,6 +64,11 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -61,6 +64,11 @@ class ApprovalMergeRequestRule < ApplicationRecord
scope :with_head_pipeline, -> { includes(merge_request: [:head_pipeline]) } scope :with_head_pipeline, -> { includes(merge_request: [:head_pipeline]) }
scope :open_merge_requests, -> { merge(MergeRequest.opened) } scope :open_merge_requests, -> { merge(MergeRequest.opened) }
scope :for_checks_that_can_be_refreshed, -> { license_compliance.open_merge_requests.with_head_pipeline } 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) 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| merge_request.approval_rules.code_owner.where(name: entry.pattern).where(section: entry.section).first_or_create do |rule|
...@@ -123,8 +131,31 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -123,8 +131,31 @@ class ApprovalMergeRequestRule < ApplicationRecord
refresh_license_scanning_approvals(project_approval_rule) if license_scanning? refresh_license_scanning_approvals(project_approval_rule) if license_scanning?
end end
def different_from_project_rule?
return unless approval_project_rule
different_groups? || different_users? || different_name_or_approvals_required? ? true : false
end
private private
def different_groups?
groups.sort != approval_project_rule.groups.sort
end
def different_users?
users.sort != approval_project_rule.users.sort
end
def different_name_or_approvals_required?
true if approvals_required != approval_project_rule.approvals_required || name != approval_project_rule.name
end
def compare_with_project_rule
self.modified_from_project_rule =
different_from_project_rule? ? true : false
end
def validate_approval_project_rule def validate_approval_project_rule
return if approval_project_rule.blank? return if approval_project_rule.blank?
return if merge_request.project == approval_project_rule.project return if merge_request.project == approval_project_rule.project
......
...@@ -6,6 +6,8 @@ class ApprovalProjectRule < ApplicationRecord ...@@ -6,6 +6,8 @@ class ApprovalProjectRule < ApplicationRecord
belongs_to :project belongs_to :project
has_and_belongs_to_many :protected_branches 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: { enum rule_type: {
regular: 0, regular: 0,
......
...@@ -34,6 +34,8 @@ module EE ...@@ -34,6 +34,8 @@ module EE
end end
end 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_one :merge_train, inverse_of: :merge_request, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :blocks_as_blocker, has_many :blocks_as_blocker,
......
---
title: Add usage ping for MRs with overridden project rules
merge_request: 36230
author:
type: added
...@@ -190,6 +190,7 @@ module EE ...@@ -190,6 +190,7 @@ module EE
start: approval_merge_request_rule_minimum_id, start: approval_merge_request_rule_minimum_id,
finish: approval_merge_request_rule_maximum_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_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), 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_mirrored_with_pipelines_enabled: count(::Project.mirrored_with_enabled_pipelines),
projects_reporting_ci_cd_back_to_github: count(::GithubService.active), projects_reporting_ci_cd_back_to_github: count(::GithubService.active),
...@@ -242,6 +243,7 @@ module EE ...@@ -242,6 +243,7 @@ module EE
start: approval_merge_request_rule_minimum_id, start: approval_merge_request_rule_minimum_id,
finish: approval_merge_request_rule_maximum_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_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), 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_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), projects_with_repositories_enabled: distinct_count(::Project.with_repositories_enabled.where(time_period),
...@@ -388,13 +390,13 @@ module EE ...@@ -388,13 +390,13 @@ module EE
def approval_merge_request_rule_minimum_id def approval_merge_request_rule_minimum_id
strong_memoize(:approval_merge_request_rule_minimum_id) do strong_memoize(:approval_merge_request_rule_minimum_id) do
::ApprovalMergeRequestRule.minimum(:id) ::ApprovalMergeRequestRule.minimum(:merge_request_id)
end end
end end
def approval_merge_request_rule_maximum_id def approval_merge_request_rule_maximum_id
strong_memoize(:approval_merge_request_rule_maximum_id) do strong_memoize(:approval_merge_request_rule_maximum_id) do
::ApprovalMergeRequestRule.maximum(:id) ::ApprovalMergeRequestRule.maximum(:merge_request_id)
end end
end end
...@@ -406,7 +408,42 @@ module EE ...@@ -406,7 +408,42 @@ module EE
::Gitlab::Auth::Ldap::Config.available_servers ::Gitlab::Auth::Ldap::Config.available_servers
end 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 def projects_jira_issuelist_active
# rubocop: disable UsageData/LargeTable: # rubocop: disable UsageData/LargeTable:
min_id = JiraTrackerData.where(issues_enabled: true).minimum(:service_id) min_id = JiraTrackerData.where(issues_enabled: true).minimum(:service_id)
......
...@@ -283,9 +283,13 @@ RSpec.describe Gitlab::UsageData do ...@@ -283,9 +283,13 @@ RSpec.describe Gitlab::UsageData do
project = create(:project, :repository_private, :github_imported, project = create(:project, :repository_private, :github_imported,
:test_repo, creator: user) :test_repo, creator: user)
merge_request = create(:merge_request, source_project: project) 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) 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) 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: 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)
create(:project, creator: user, disable_overriding_approvers_per_merge_request: true) create(:project, creator: user, disable_overriding_approvers_per_merge_request: true)
create(:project, creator: user, disable_overriding_approvers_per_merge_request: false) create(:project, creator: user, disable_overriding_approvers_per_merge_request: false)
...@@ -327,6 +331,7 @@ RSpec.describe Gitlab::UsageData do ...@@ -327,6 +331,7 @@ RSpec.describe Gitlab::UsageData do
merge_requests_with_added_rules: 12, merge_requests_with_added_rules: 12,
merge_requests_with_optional_codeowners: 4, merge_requests_with_optional_codeowners: 4,
merge_requests_with_required_codeowners: 8, merge_requests_with_required_codeowners: 8,
merge_requests_with_overridden_project_rules: 4,
projects_imported_from_github: 2, projects_imported_from_github: 2,
projects_with_repositories_enabled: 26, projects_with_repositories_enabled: 26,
protected_branches: 2, protected_branches: 2,
......
...@@ -7,6 +7,12 @@ RSpec.describe ApprovalMergeRequestRule do ...@@ -7,6 +7,12 @@ RSpec.describe ApprovalMergeRequestRule do
subject { create(:approval_merge_request_rule, merge_request: merge_request) } 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 describe 'validations' do
it 'is valid' do it 'is valid' do
expect(build(:approval_merge_request_rule)).to be_valid expect(build(:approval_merge_request_rule)).to be_valid
......
...@@ -11,6 +11,13 @@ RSpec.describe ApprovalProjectRule do ...@@ -11,6 +11,13 @@ RSpec.describe ApprovalProjectRule do
end end
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 describe '.regular' do
it 'returns non-report_approver records' do it 'returns non-report_approver records' do
rules = create_list(:approval_project_rule, 2) rules = create_list(:approval_project_rule, 2)
......
...@@ -25,6 +25,8 @@ RSpec.describe MergeRequest do ...@@ -25,6 +25,8 @@ RSpec.describe MergeRequest do
it { is_expected.to have_many(:approved_by_users) } it { is_expected.to have_many(:approved_by_users) }
it { is_expected.to have_one(:merge_train) } it { is_expected.to have_one(:merge_train) }
it { is_expected.to have_many(:approval_rules) } 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 'approval_rules association' do
describe '#applicable_to_branch' do describe '#applicable_to_branch' do
......
# frozen_string_literal: true
module Gitlab
module BackgroundMigration
# Compare all current rules to project rules
class AddModifiedToApprovalMergeRequestRule
# 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
has_one :approval_project_rule, through: :approval_merge_request_rule_source
has_and_belongs_to_many :users
has_and_belongs_to_many :groups,
class_name: '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
has_and_belongs_to_many :users
has_and_belongs_to_many :groups,
class_name: '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
belongs_to :approval_project_rule
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
# 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: "1", path: "test_group1", type: 'Group')
group2 = groups.create!(name: "2", path: "test_group2", type: 'Group')
group3 = groups.create!(name: "3", 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_project_rule_two.id, group_id: group1.id)
mr_groups.create!(approval_merge_request_rule_id: overridden_project_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
...@@ -158,6 +158,8 @@ merge_requests: ...@@ -158,6 +158,8 @@ merge_requests:
- assignees - assignees
- reviews - reviews
- approval_rules - approval_rules
- approval_merge_request_rule_sources
- approval_project_rules
- approvals - approvals
- approvers - approvers
- approver_users - approver_users
...@@ -466,6 +468,8 @@ project: ...@@ -466,6 +468,8 @@ project:
- feature_usage - feature_usage
- approval_rules - approval_rules
- approval_merge_request_rules - approval_merge_request_rules
- approval_merge_request_rule_sources
- approval_project_rules
- approvers - approvers
- approver_users - approver_users
- audit_events - audit_events
......
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