Skip to content
Snippets Groups Projects
Commit 63c5279a authored by Bob Van Landuyt's avatar Bob Van Landuyt
Browse files

Model changes for code owner approval rules

- Adds the validations to match the uniqueness index.
- Adds scopes for finding code owner rules based on a pattern
- Adds a safe way to create code owner rules avoiding race conditions
parent 093178b0
No related branches found
No related tags found
No related merge requests found
......@@ -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] }, if: :code_owner
belongs_to :merge_request
......@@ -15,6 +19,13 @@ class ApprovalMergeRequestRule < ApplicationRecord
has_one :approval_merge_request_rule_source
has_one :approval_project_rule, through: :approval_merge_request_rule_source
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
......
......@@ -6,6 +6,12 @@
name ApprovalRuleLike::DEFAULT_NAME
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
project
name ApprovalRuleLike::DEFAULT_NAME
......
......@@ -210,7 +210,7 @@ def create_member_in(member, *populate_in)
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
......
......@@ -7,6 +7,83 @@
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
it 'allows two records with the same name' do
create(:approval_merge_request_rule, name: 'Default rule')
new = build(:approval_merge_request_rule, name: 'Default rule')
expect(new).to be_valid
expect { new.save! }.not_to raise_error
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')
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
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
......
......@@ -4,10 +4,10 @@
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) }
......
......@@ -266,7 +266,7 @@
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) }
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment