Commit b855cb6b authored by Mark Chao's avatar Mark Chao

Remove finalize MR approval rule in data migration

Past MR's approval requirement may change if project level setting
ever changed, so finalizing a past MR won't be accurate.

If there is a need to create estimated data,
another optional migration can be implemented.
parent 3487761d
......@@ -56,13 +56,6 @@ module Gitlab
scope
end
end
def sync_approvals
# Before being merged, approvals are dynamically calculated instead of being persisted in the db.
return unless merge_request.merged?
self.approvals = merge_request.approvals.where(user_id: approvers.map(&:id))
end
end
class ApprovalMergeRequestRuleSource < ApplicationRecord
......@@ -105,24 +98,6 @@ module Gitlab
def sync_code_owners_with_approvers
::MergeRequest.find(id).sync_code_owners_with_approvers
end
def finalize_approvals
return unless merged?
copy_project_approval_rules unless approval_rules.regular.exists?
approval_rules.reload.each(&:sync_approvals)
end
private
def copy_project_approval_rules
target_project.approval_rules.each do |project_rule|
rule = approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
rule.users = project_rule.users
rule.groups = project_rule.groups
end
end
end
class Project < ActiveRecord::Base
......@@ -175,8 +150,6 @@ module Gitlab
end
target.sync_code_owners_with_approvers
target.finalize_approvals if target.merged?
end
end
......
......@@ -190,117 +190,4 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
end
end
end
# Copied and modified from merge_request_spec.rb
describe '#finalize_approvals' do
let(:project) { create(:project, :repository) }
subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
let(:target) { merge_request }
let!(:member1) { create(:user) }
let!(:member2) { create(:user) }
let!(:member3) { create(:user) }
let!(:group1) { create(:group) }
let!(:group2) { create(:group) }
let!(:group1_member) { create(:user) }
let!(:group2_member) { create(:user) }
let!(:approval1) { create(:approval, merge_request: subject, user: member1) }
let!(:approval2) { create(:approval, merge_request: subject, user: member3) }
let!(:approval3) { create(:approval, merge_request: subject, user: group1_member) }
let!(:approval4) { create(:approval, merge_request: subject, user: group2_member) }
before do
group1.add_guest(group1_member)
group2.add_guest(group2_member)
rule = create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12)
rule.users = [member1, member2]
rule.groups << group1
end
context 'when without MR rules (project rule not overwritten)' do
it 'does nothing if unmerged' do
expect do
described_class.new.perform(target.class.name, target.id)
end.not_to change { ApprovalMergeRequestRule.count }
expect(approval1.approval_rules).to be_empty
expect(approval2.approval_rules).to be_empty
expect(approval3.approval_rules).to be_empty
expect(approval4.approval_rules).to be_empty
end
context 'when merged' do
subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
it 'copies project rules to MR' do
expect do
described_class.new.perform(target.class.name, target.id)
end.to change { ApprovalMergeRequestRule.count }.by(1)
rule = subject.approval_rules.first
expect(rule.name).to eq('foo')
expect(rule.approvals_required).to eq(12)
expect(rule.users).to contain_exactly(member1, member2)
expect(rule.groups).to contain_exactly(group1)
rule = subject.approval_rules.first
expect(approval1.approval_rules).to contain_exactly(rule)
expect(approval2.approval_rules).to be_empty
expect(approval3.approval_rules).to contain_exactly(rule)
expect(approval4.approval_rules).to be_empty
end
end
end
context 'when with MR approver exists (project rule overwritten)' do
before do
create_skip_sync(:approver, target: subject, user: member2)
create_skip_sync(:approver, target: subject, user: member3)
create_skip_sync(:approver_group, target: subject, group: group2)
merge_request.update(approvals_before_merge: 32)
end
it 'does not call finalize_approvals if unmerged' do
expect do
described_class.new.perform(target.class.name, target.id)
end.to change { ApprovalMergeRequestRule.count }.by(1)
expect(approval1.approval_rules).to be_empty
expect(approval2.approval_rules).to be_empty
expect(approval3.approval_rules).to be_empty
expect(approval4.approval_rules).to be_empty
end
context 'when merged' do
subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
it 'does not copy project rules, and updates approval mapping with MR rules' do
expect(subject).not_to receive(:copy_project_approval_rules)
expect do
described_class.new.perform(target.class.name, target.id)
end.to change { ApprovalMergeRequestRule.count }.by(1)
rule = subject.approval_rules.first
expect(rule.name).to eq('Default')
expect(rule.approvals_required).to eq(32)
expect(rule.users).to contain_exactly(member2, member3)
expect(rule.groups).to contain_exactly(group2)
expect(approval1.approval_rules).to be_empty
expect(approval2.approval_rules).to contain_exactly(rule)
expect(approval3.approval_rules).to be_empty
expect(approval4.approval_rules).to contain_exactly(rule)
end
end
end
end
end
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