Merge branch 'bvl-codeowner-rules-model' into 'master'

Model changes for code owner approval rules

See merge request gitlab-org/gitlab-ee!9398

db/schema.rb

@@ -233,6 +233,7 @@ ActiveRecord::Schema.define(version: 20190131122559) do
     t.integer "approvals_required", limit: 2, default: 0, null: false
     t.boolean "code_owner", default: false, null: false
     t.string "name", 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
   end
 

ee/app/models/approval_merge_request_rule.rb

@@ -7,6 +7,10 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   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) }
+
+  validates :name, uniqueness: { scope: [:merge_request, :code_owner] }
 
   belongs_to :merge_request
 
@@ -18,6 +22,13 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   validate :validate_approvals_required
 
+  def self.find_or_create_code_owner_rule(merge_request, pattern)
+    merge_request.approval_rules.safe_find_or_create_by(
+      code_owner: true,
+      name: pattern
+    )
+  end
+
   def project
     merge_request.target_project
   end

ee/db/migrate/20190128172533_add_index_to_name_on_approval_merge_request_rules.rb

+# frozen_string_literal: true
+
+class AddIndexToNameOnApprovalMergeRequestRules < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'approval_rule_name_index_for_code_owners'
+
+  def up
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :code_owner, :name],
+      unique: true,
+      where: "code_owner = 't'",
+      name: INDEX_NAME
+    )
+  end
+
+  def down
+    remove_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :name],
+      name: INDEX_NAME
+    )
+  end
+end

ee/spec/factories/approval_rules.rb

@@ -3,7 +3,13 @@
 FactoryBot.define do
   factory :approval_merge_request_rule do
     merge_request
-    name ApprovalRuleLike::DEFAULT_NAME
+    sequence(:name) { |n| "#{ApprovalRuleLike::DEFAULT_NAME}-#{n}" }
+  end
+
+  factory :code_owner_rule, parent: :approval_merge_request_rule do
+    merge_request
+    code_owner true
+    sequence(:name) { |n| "*-#{n}.js" }
   end
 
   factory :approval_project_rule do

ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb

@@ -210,7 +210,7 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
         end
 
         context 'when code owner rule exists' do
-          let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: target, code_owner: true, users: [create(:user)]) }
+          let!(:code_owner_rule) { create(:code_owner_rule, merge_request: target, users: [create(:user)]) }
 
           it 'reuses and updates existing rule' do
             expect do

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -7,6 +7,83 @@ describe ApprovalMergeRequestRule do
 
   subject { create(:approval_merge_request_rule, merge_request: merge_request) }
 
+  describe 'validations' do
+    it 'is valid' do
+      expect(build(:approval_merge_request_rule)).to be_valid
+    end
+
+    it 'is invalid when the name is missing' do
+      expect(build(:approval_merge_request_rule, name: nil)).not_to be_valid
+    end
+
+    context 'code owner rules' do
+      it 'is valid' do
+        expect(build(:code_owner_rule)).to be_valid
+      end
+
+      it 'is invalid when reusing the same name within the same merge request' do
+        existing = create(:code_owner_rule, name: '*.rb', merge_request: merge_request)
+
+        new = build(:code_owner_rule, merge_request: existing.merge_request, name: '*.rb')
+
+        expect(new).not_to be_valid
+        expect { new.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
+      end
+
+      it 'allows a regular rule with the same name as the codeowner rule' do
+        create(:code_owner_rule, name: '*.rb', merge_request: merge_request)
+
+        new = build(:approval_merge_request_rule, name: '*.rb', merge_request: merge_request)
+
+        expect(new).to be_valid
+        expect { new.save! }.not_to raise_error
+      end
+    end
+  end
+
+  context 'scopes'  do
+    set(:rb_rule) { create(:code_owner_rule, name: '*.rb') }
+    set(:js_rule) { create(:code_owner_rule, name: '*.js') }
+    set(:css_rule) { create(:code_owner_rule, name: '*.css') }
+    set(:approval_rule) { create(:approval_merge_request_rule) }
+
+    describe '.not_matching_pattern' do
+      it 'returns the correct rules' do
+        expect(described_class.not_matching_pattern(['*.rb', '*.js']))
+          .to contain_exactly(css_rule)
+      end
+    end
+
+    describe '.matching_pattern' do
+      it 'returns the correct rules' do
+        expect(described_class.matching_pattern(['*.rb', '*.js']))
+          .to contain_exactly(rb_rule, js_rule)
+      end
+    end
+  end
+
+  describe '.find_or_create_code_owner_rule' do
+    set(:merge_request) { create(:merge_request) }
+    set(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) }
+
+    it 'finds an existing rule' do
+      expect(described_class.find_or_create_code_owner_rule(merge_request, '*.rb'))
+        .to eq(existing_code_owner_rule)
+    end
+
+    it 'creates a new rule if it does not exist' do
+      expect { described_class.find_or_create_code_owner_rule(merge_request, '*.js') }
+        .to change { merge_request.approval_rules.count }.by(1)
+    end
+
+    it 'retries when a record was created between the find and the create' do
+      expect(described_class).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique)
+      allow(described_class).to receive(:find_or_create_by).and_call_original
+
+      expect(described_class.find_or_create_code_owner_rule(merge_request, '*.js')).not_to be_nil
+    end
+  end
+
   describe '#project' do
     it 'returns project of MergeRequest' do
       expect(subject.project).to be_present

ee/spec/models/approval_state_spec.rb

@@ -4,10 +4,10 @@ require 'spec_helper'
 
 describe ApprovalState do
   def create_rule(additional_params = {})
-    create(
-      :approval_merge_request_rule,
-      additional_params.merge(merge_request: merge_request)
-    )
+    params = additional_params.merge(merge_request: merge_request)
+    factory = params.delete(:code_owner) ? :code_owner_rule : :approval_merge_request_rule
+
+    create(factory, params)
   end
 
   let(:merge_request) { create(:merge_request) }

ee/spec/models/merge_request_spec.rb

@@ -348,7 +348,7 @@ describe MergeRequest do
     let(:code_owners) { create_list(:user, 2) }
 
     let!(:regular_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: approvers) }
-    let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: code_owners, code_owner: true) }
+    let!(:code_owner_rule) { create(:code_owner_rule, merge_request: merge_request, users: code_owners) }
 
     let!(:approval) { create(:approval, merge_request: merge_request, user: approvers.last) }