diff --git a/db/schema.rb b/db/schema.rb index 9ac866d9f43a396c0117f851ac5c5e99281a7e45..6543da7354759704e4e300835b10803d68898ffe 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190527194900) do +ActiveRecord::Schema.define(version: 20190528173628) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -244,8 +244,11 @@ t.integer "approvals_required", limit: 2, default: 0, null: false t.boolean "code_owner", default: false, null: false t.string "name", null: false + t.integer "rule_type", limit: 2, default: 1, null: false t.index ["merge_request_id", "code_owner", "name"], name: "approval_rule_name_index_for_code_owners", unique: true, where: "(code_owner = true)", using: :btree t.index ["merge_request_id", "code_owner"], name: "index_approval_merge_request_rules_1", using: :btree + t.index ["merge_request_id", "rule_type", "name"], name: "index_approval_rule_name_for_code_owners_rule_type", unique: true, where: "(rule_type = 2)", using: :btree + t.index ["merge_request_id", "rule_type"], name: "index_approval_rules_code_owners_rule_type", where: "(rule_type = 2)", using: :btree end create_table "approval_merge_request_rules_approved_approvers", force: :cascade do |t| diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index 199a3da9ff5eb8a4fc8802702d8304f217d68fb5..080b730eb1c48aae21f96e069c694343b7b26db5 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -6,12 +6,15 @@ class ApprovalMergeRequestRule < ApplicationRecord DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner' - scope :regular, -> { where(code_owner: false) } - scope :code_owner, -> { where(code_owner: true) } # special code owner rules, updated internally when code changes scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) } scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) } + # Deprecated scope until code_owner column has been migrated to rule_type + scope :code_owner, -> { where(code_owner: true).or(rule_type: :code_owner) } validates :name, uniqueness: { scope: [:merge_request, :code_owner] } + # Temporary validations until `code_owner` can be dropped in favor of `rule_type` + validates :code_owner, inclusion: { in: [true], if: :code_owner? } + validates :code_owner, inclusion: { in: [false], if: :regular? } belongs_to :merge_request, inverse_of: :approval_rules @@ -23,9 +26,15 @@ class ApprovalMergeRequestRule < ApplicationRecord validate :validate_approvals_required + enum rule_type: { + regular: 1, + code_owner: 2 + } + def self.find_or_create_code_owner_rule(merge_request, pattern) merge_request.approval_rules.safe_find_or_create_by( - code_owner: true, + rule_type: :code_owner, + code_owner: true, # deprecated, replaced with `rule_type: :code_owner` name: pattern ) end @@ -65,10 +74,8 @@ def sync_approved_approvers self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id) end - def regular - !code_owner? - end - alias_method :regular?, :regular + # ApprovalRuleLike interface + alias_method :regular, :regular? private diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index fb26e85e2634c62b4bacf854aa07c9f37b2e1262..aca946d6b6340ef4cbeb77048700b9a824c6156d 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -177,7 +177,10 @@ def sync_code_owners_with_approvers if owners.present? ApplicationRecord.transaction do rule = approval_rules.code_owner.first - rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER) + rule ||= ApprovalMergeRequestRule.find_or_create_code_owner_rule( + self, + ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER + ) rule.users = owners.uniq end diff --git a/ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml b/ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml new file mode 100644 index 0000000000000000000000000000000000000000..13b9abf9a0abc8078523b84906ea28208032ca2d --- /dev/null +++ b/ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml @@ -0,0 +1,5 @@ +--- +title: Migrate code_owners to rule_type enum on approval_merge_request_rules +merge_request: 13036 +author: +type: changed diff --git a/ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb b/ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..7339a4fccbad814a65dc1f11e21e2c76e0051461 --- /dev/null +++ b/ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddRuleTypeToApprovalMergeRequestApprovalRules < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:approval_merge_request_rules, :rule_type, :integer, limit: 2, default: 1) + end + + def down + remove_column(:approval_merge_request_rules, :rule_type) + end +end diff --git a/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..d43319ad1f06262db7929159b58b32deb81951b1 --- /dev/null +++ b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexForCodeOwnerRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME = 'index_approval_rule_name_for_code_owners_rule_type' + INDEX_CODE_OWNERS_RULES_QUERY_NAME = 'index_approval_rules_code_owners_rule_type' + + class ApprovalMergeRequestRule < ActiveRecord::Base + include EachBatch + + enum rule_types: { + regular: 1, + code_owner: 2 + } + end + + def up + # Ensure only 1 code_owner rule per merge_request + add_concurrent_index( + :approval_merge_request_rules, + [:merge_request_id, :name], + unique: true, + where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}", + name: INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME + ) + + # Support lookups for all code_owner rules per merge_request + add_concurrent_index( + :approval_merge_request_rules, + [:merge_request_id], + where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}", + name: INDEX_CODE_OWNERS_RULES_QUERY_NAME + ) + end + + def down + remove_concurrent_index_by_name( + :approval_merge_request_rules, + INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME + ) + + remove_concurrent_index_by_name( + :approval_merge_request_rules, + INDEX_CODE_OWNERS_RULES_QUERY_NAME + ) + end +end diff --git a/ee/db/post_migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb b/ee/db/post_migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..0f0df45613444ec033bf96813aa019ffe650643d --- /dev/null +++ b/ee/db/post_migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PopulateRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class ApprovalMergeRequestRule < ActiveRecord::Base + include EachBatch + + enum rule_types: { + regular: 1, + code_owner: 2 + } + end + + def up + # On Gitlab.com, this should update about 17k rows. Since our updates are + # small and we are populating prior to indexing, the overhead should be small + ApprovalMergeRequestRule.where(code_owner: true).each_batch do |batch| + batch.update_all(rule_type: ApprovalMergeRequestRule.rule_types[:code_owner]) + end + end + + def down + # code_owner is already kept in sync with `rule_type`, so no changes are needed + end +end diff --git a/ee/spec/factories/approval_rules.rb b/ee/spec/factories/approval_rules.rb index 6065871cdc0188d26b3b42cc29a786d154507acb..99baad91b4605aba9398dbd10ba23c6b92fb7ecb 100644 --- a/ee/spec/factories/approval_rules.rb +++ b/ee/spec/factories/approval_rules.rb @@ -8,7 +8,8 @@ factory :code_owner_rule, parent: :approval_merge_request_rule do merge_request - code_owner true + rule_type :code_owner + code_owner true # deprecated, replaced with `rule_type: :code_owner` sequence(:name) { |n| "*-#{n}.js" } end diff --git a/ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb b/ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..143a7c53a831b77914749109ae70154467138c1f --- /dev/null +++ b/ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('ee', 'db', 'post_migrate', '20190520201748_populate_rule_type_on_approval_merge_request_rules.rb') + +describe PopulateRuleTypeOnApprovalMergeRequestRules, :migration do + let(:migration) { described_class.new } + + describe '#up' do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:merge_requests) { table(:merge_requests) } + let(:approval_rules) { table(:approval_merge_request_rules) } + let(:regular_rule_type) { ApprovalMergeRequestRule.rule_types[:regular] } + let(:code_owner_rule_type) { ApprovalMergeRequestRule.rule_types[:code_owner] } + + before do + namespaces.create!(id: 11, name: 'gitlab', path: 'gitlab') + projects.create!(id: 101, namespace_id: 11, name: 'gitlab', path: 'gitlab') + merge_requests.create!(id: 1, target_project_id: 101, source_project_id: 101, target_branch: 'feature', source_branch: 'master') + + approval_rules.create!(id: 1, merge_request_id: 1, name: "Default", code_owner: false, rule_type: regular_rule_type) + approval_rules.create!(id: 2, merge_request_id: 1, name: "Code Owners", code_owner: true, rule_type: regular_rule_type) + end + + it 'backfills ApprovalMergeRequestRules code_owner rule_type' do + expect(approval_rules.where(rule_type: regular_rule_type).pluck(:id)).to contain_exactly(1, 2) + expect(approval_rules.where(rule_type: code_owner_rule_type).pluck(:id)).to be_empty + + migrate! + + expect(approval_rules.where(rule_type: regular_rule_type).pluck(:id)).to contain_exactly(1) + expect(approval_rules.where(rule_type: code_owner_rule_type).pluck(:id)).to contain_exactly(2) + end + end +end diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index e9bc5c55c55c019bce47d10f2e8a17500448b961..0928c3161f2c6255e9993e25921e0c727153a9b2 100644 --- a/ee/spec/models/approval_merge_request_rule_spec.rb +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -38,6 +38,22 @@ expect(new).to be_valid expect { new.save! }.not_to raise_error end + + it 'validates code_owner when rule_type code_owner' do + new = build(:code_owner_rule) + expect(new).to be_valid + + new.code_owner = false + expect(new).not_to be_valid + end + + it 'validates code_owner when rule_type regular' do + new = build(:approval_merge_request_rule) + expect(new).to be_valid + + new.code_owner = true + expect(new).not_to be_valid + end end end @@ -60,6 +76,13 @@ .to contain_exactly(rb_rule, js_rule) end end + + describe '.code_owners' do + it 'returns the correct rules' do + expect(described_class.code_owner) + .to contain_exactly(rb_rule, js_rule, css_rule) + end + end end describe '.find_or_create_code_owner_rule' do @@ -100,7 +123,7 @@ end it 'returns false for code owner records' do - subject = create(:approval_merge_request_rule, merge_request: merge_request, code_owner: true) + subject = create(:code_owner_rule, merge_request: merge_request) expect(subject.regular).to eq(false) expect(subject.regular?).to eq(false) diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index e10febcd4166817d3a936225856cf225b82177ee..d3874959d43d171ed631e857a047d7bc7bf3f3d8 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -490,7 +490,7 @@ end context 'when code owner rule exists' do - let!(:code_owner_rule) { subject.approval_rules.code_owner.create!(name: 'Code Owner', users: [create(:user)]) } + let!(:code_owner_rule) { create(:code_owner_rule, merge_request: subject, name: 'Code Owner', users: [create(:user)]) } it 'reuses and updates existing rule' do expect do diff --git a/ee/spec/models/visible_approvable_for_rule_spec.rb b/ee/spec/models/visible_approvable_for_rule_spec.rb index 33c9558c765faa66b1e066d5d9f85789ba8db175..fecc68bf377c522922f1fabc87691fe17ce1dd66 100644 --- a/ee/spec/models/visible_approvable_for_rule_spec.rb +++ b/ee/spec/models/visible_approvable_for_rule_spec.rb @@ -35,7 +35,7 @@ let(:code_owner) { build(:user) } let!(:project_regular_rule) { create(:approval_project_rule, project: project, users: [approver]) } - let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [code_owner], code_owner: true) } + let!(:code_owner_rule) { create(:code_owner_rule, merge_request: resource, users: [code_owner]) } before do project.add_developer(approver) diff --git a/ee/spec/services/approval_rules/finalize_service_spec.rb b/ee/spec/services/approval_rules/finalize_service_spec.rb index 2ed4dec67d539025aab86002b6b446f33ca209c9..b6861f2e2813c4183c405f6097848d3caf1c7590 100644 --- a/ee/spec/services/approval_rules/finalize_service_spec.rb +++ b/ee/spec/services/approval_rules/finalize_service_spec.rb @@ -45,7 +45,7 @@ let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) } before do - merge_request.approval_rules.code_owner.create(name: 'Code Owner') + merge_request.approval_rules.code_owner.create(name: 'Code Owner', rule_type: :code_owner, code_owner: true) end it 'copies project rules to MR, keep snapshot of group member by including it as part of users association' do