Commit 963a6999 authored by Lin Jen-Shin's avatar Lin Jen-Shin

Merge branch '1979-1-multi-block-approval-data-migration' into 'master'

Add ApprovalRule model

See merge request gitlab-org/gitlab-ee!8497
parents d6ae61dc 201a6511
...@@ -219,6 +219,67 @@ ActiveRecord::Schema.define(version: 20190103140724) do ...@@ -219,6 +219,67 @@ ActiveRecord::Schema.define(version: 20190103140724) do
t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree
end end
create_table "approval_merge_request_rule_sources", id: :bigserial, force: :cascade do |t|
t.bigint "approval_merge_request_rule_id", null: false
t.bigint "approval_project_rule_id", null: false
t.index ["approval_merge_request_rule_id"], name: "index_approval_merge_request_rule_sources_1", unique: true, using: :btree
t.index ["approval_project_rule_id"], name: "index_approval_merge_request_rule_sources_2", using: :btree
end
create_table "approval_merge_request_rules", id: :bigserial, force: :cascade do |t|
t.datetime_with_timezone "created_at", null: false
t.datetime_with_timezone "updated_at", null: false
t.integer "merge_request_id", null: false
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: "index_approval_merge_request_rules_1", using: :btree
end
create_table "approval_merge_request_rules_approved_approvers", id: :bigserial, force: :cascade do |t|
t.bigint "approval_merge_request_rule_id", null: false
t.integer "user_id", null: false
t.index ["approval_merge_request_rule_id", "user_id"], name: "index_approval_merge_request_rules_approved_approvers_1", unique: true, using: :btree
t.index ["user_id"], name: "index_approval_merge_request_rules_approved_approvers_2", using: :btree
end
create_table "approval_merge_request_rules_groups", id: :bigserial, force: :cascade do |t|
t.bigint "approval_merge_request_rule_id", null: false
t.integer "group_id", null: false
t.index ["approval_merge_request_rule_id", "group_id"], name: "index_approval_merge_request_rules_groups_1", unique: true, using: :btree
t.index ["group_id"], name: "index_approval_merge_request_rules_groups_2", using: :btree
end
create_table "approval_merge_request_rules_users", id: :bigserial, force: :cascade do |t|
t.bigint "approval_merge_request_rule_id", null: false
t.integer "user_id", null: false
t.index ["approval_merge_request_rule_id", "user_id"], name: "index_approval_merge_request_rules_users_1", unique: true, using: :btree
t.index ["user_id"], name: "index_approval_merge_request_rules_users_2", using: :btree
end
create_table "approval_project_rules", id: :bigserial, force: :cascade do |t|
t.datetime_with_timezone "created_at", null: false
t.datetime_with_timezone "updated_at", null: false
t.integer "project_id", null: false
t.integer "approvals_required", limit: 2, default: 0, null: false
t.string "name", null: false
t.index ["project_id"], name: "index_approval_project_rules_on_project_id", using: :btree
end
create_table "approval_project_rules_groups", id: :bigserial, force: :cascade do |t|
t.bigint "approval_project_rule_id", null: false
t.integer "group_id", null: false
t.index ["approval_project_rule_id", "group_id"], name: "index_approval_project_rules_groups_1", unique: true, using: :btree
t.index ["group_id"], name: "index_approval_project_rules_groups_2", using: :btree
end
create_table "approval_project_rules_users", id: :bigserial, force: :cascade do |t|
t.bigint "approval_project_rule_id", null: false
t.integer "user_id", null: false
t.index ["approval_project_rule_id", "user_id"], name: "index_approval_project_rules_users_1", unique: true, using: :btree
t.index ["user_id"], name: "index_approval_project_rules_users_2", using: :btree
end
create_table "approvals", force: :cascade do |t| create_table "approvals", force: :cascade do |t|
t.integer "merge_request_id", null: false t.integer "merge_request_id", null: false
t.integer "user_id", null: false t.integer "user_id", null: false
...@@ -3163,6 +3224,20 @@ ActiveRecord::Schema.define(version: 20190103140724) do ...@@ -3163,6 +3224,20 @@ ActiveRecord::Schema.define(version: 20190103140724) do
add_foreign_key "application_settings", "namespaces", column: "custom_project_templates_group_id", on_delete: :nullify add_foreign_key "application_settings", "namespaces", column: "custom_project_templates_group_id", on_delete: :nullify
add_foreign_key "application_settings", "projects", column: "file_template_project_id", name: "fk_ec757bd087", on_delete: :nullify add_foreign_key "application_settings", "projects", column: "file_template_project_id", name: "fk_ec757bd087", on_delete: :nullify
add_foreign_key "application_settings", "users", column: "usage_stats_set_by_user_id", name: "fk_964370041d", on_delete: :nullify add_foreign_key "application_settings", "users", column: "usage_stats_set_by_user_id", name: "fk_964370041d", on_delete: :nullify
add_foreign_key "approval_merge_request_rule_sources", "approval_merge_request_rules", on_delete: :cascade
add_foreign_key "approval_merge_request_rule_sources", "approval_project_rules", on_delete: :cascade
add_foreign_key "approval_merge_request_rules", "merge_requests", on_delete: :cascade
add_foreign_key "approval_merge_request_rules_approved_approvers", "approval_merge_request_rules", on_delete: :cascade
add_foreign_key "approval_merge_request_rules_approved_approvers", "users", on_delete: :cascade
add_foreign_key "approval_merge_request_rules_groups", "approval_merge_request_rules", on_delete: :cascade
add_foreign_key "approval_merge_request_rules_groups", "namespaces", column: "group_id", on_delete: :cascade
add_foreign_key "approval_merge_request_rules_users", "approval_merge_request_rules", on_delete: :cascade
add_foreign_key "approval_merge_request_rules_users", "users", on_delete: :cascade
add_foreign_key "approval_project_rules", "projects", on_delete: :cascade
add_foreign_key "approval_project_rules_groups", "approval_project_rules", on_delete: :cascade
add_foreign_key "approval_project_rules_groups", "namespaces", column: "group_id", on_delete: :cascade
add_foreign_key "approval_project_rules_users", "approval_project_rules", on_delete: :cascade
add_foreign_key "approval_project_rules_users", "users", on_delete: :cascade
add_foreign_key "approvals", "merge_requests", name: "fk_310d714958", on_delete: :cascade add_foreign_key "approvals", "merge_requests", name: "fk_310d714958", on_delete: :cascade
add_foreign_key "approver_groups", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "approver_groups", "namespaces", column: "group_id", on_delete: :cascade
add_foreign_key "badges", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "badges", "namespaces", column: "group_id", on_delete: :cascade
......
# frozen_string_literal: true
class ApprovalMergeRequestRule < ApplicationRecord
include ApprovalRuleLike
scope :regular, -> { where(code_owner: false) }
scope :code_owner, -> { where(code_owner: true) } # special code owner rules, updated internally when code changes
belongs_to :merge_request
# approved_approvers is only populated after MR is merged
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_project_rule, through: :approval_merge_request_rule_source
def project
merge_request.target_project
end
# Users who are eligible to approve, including specified group members.
# Excludes the author if 'self-approval' isn't explicitly
# enabled on project settings.
# @return [Array<User>]
def approvers
scope = super
if merge_request.author && !project.merge_requests_author_approval?
scope = scope.where.not(id: merge_request.author)
end
scope
end
def sync_approved_approvers
# Before being merged, approved_approvers are dynamically calculated in ApprovalWrappedRule instead of being persisted.
return unless merge_request.merged?
self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
end
end
# frozen_string_literal: true
# Allow MR rule to lookup its project rule source
class ApprovalMergeRequestRuleSource < ApplicationRecord
belongs_to :approval_merge_request_rule
belongs_to :approval_project_rule
end
# frozen_string_literal: true
class ApprovalProjectRule < ApplicationRecord
include ApprovalRuleLike
belongs_to :project
# To allow easier duck typing
scope :regular, -> { all }
scope :code_owner, -> { none }
def code_owner
false
end
alias_method :code_owner?, :code_owner
end
...@@ -9,4 +9,8 @@ class Approver < ActiveRecord::Base ...@@ -9,4 +9,8 @@ class Approver < ActiveRecord::Base
def find_by_user_id(user_id) def find_by_user_id(user_id)
find_by(user_id: user_id) find_by(user_id: user_id)
end end
def member
user
end
end end
...@@ -13,4 +13,8 @@ class ApproverGroup < ActiveRecord::Base ...@@ -13,4 +13,8 @@ class ApproverGroup < ActiveRecord::Base
approver_groups.joins(:group).merge(public_or_visible_groups) approver_groups.joins(:group).merge(public_or_visible_groups)
end end
def member
group
end
end end
# frozen_string_literal: true
module ApprovalRuleLike
extend ActiveSupport::Concern
DEFAULT_NAME = 'Default'
included do
has_and_belongs_to_many :users
has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups"
has_many :group_users, through: :groups, source: :users
validates :name, presence: true
end
# Users who are eligible to approve, including specified group members.
# @return [Array<User>]
def approvers
@approvers ||= User.from_union([users, group_users])
end
def add_member(member)
case member
when User
users << member unless users.exists?(member.id)
when Group
groups << member unless groups.exists?(member.id)
end
end
def remove_member(member)
case member
when User
users.delete(member)
when Group
groups.delete(member)
end
end
end
...@@ -16,6 +16,7 @@ module EE ...@@ -16,6 +16,7 @@ module EE
has_many :approved_by_users, through: :approvals, source: :user has_many :approved_by_users, through: :approvals, source: :user
has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
has_many :draft_notes has_many :draft_notes
validate :validate_approvals_before_merge, unless: :importing? validate :validate_approvals_before_merge, unless: :importing?
......
...@@ -44,6 +44,7 @@ module EE ...@@ -44,6 +44,7 @@ module EE
has_many :reviews, inverse_of: :project has_many :reviews, inverse_of: :project
has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :approval_rules, class_name: 'ApprovalProjectRule'
has_many :audit_events, as: :entity has_many :audit_events, as: :entity
has_many :path_locks has_many :path_locks
has_many :vulnerability_feedback, class_name: 'Vulnerabilities::Feedback' has_many :vulnerability_feedback, class_name: 'Vulnerabilities::Feedback'
......
# frozen_string_literal: true
module ApprovalRules
class FinalizeService
attr_reader :merge_request
def initialize(merge_request)
@merge_request = merge_request
end
def execute
return unless merge_request.merged?
ActiveRecord::Base.transaction do
if merge_request.approval_rules.regular.exists?
merge_group_members_into_users
else
copy_project_approval_rules
end
merge_request.approval_rules.each(&:sync_approved_approvers)
end
end
private
def merge_group_members_into_users
merge_request.approval_rules.each do |rule|
rule.users += rule.group_users
end
end
def copy_project_approval_rules
merge_request.target_project.approval_rules.each do |project_rule|
users = project_rule.approvers
groups = project_rule.groups.public_or_visible_to_user(merge_request.author)
merge_request.approval_rules.create!(
project_rule.attributes.slice('approvals_required', 'name').merge(users: users, groups: groups)
)
end
end
end
end
# frozen_string_literal: true
class CreateApprovalRules < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
create_table :approval_project_rules, id: :bigserial do |t|
t.timestamps_with_timezone
t.references :project, index: true, foreign_key: { on_delete: :cascade }, null: false
t.integer :approvals_required, limit: 2, default: 0, null: false
t.string :name, null: false
end
create_table :approval_merge_request_rules, id: :bigserial do |t|
t.timestamps_with_timezone
t.references :merge_request, index: false, foreign_key: { on_delete: :cascade }, null: false
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: 'index_approval_merge_request_rules_1'
end
end
end
# frozen_string_literal: true
class CreateApprovalRulesApprovals < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
create_table(:approval_merge_request_rules_approved_approvers, id: :bigserial) do |t|
t.references(
:approval_merge_request_rule,
type: :bigint,
null: false,
foreign_key: { on_delete: :cascade },
index: false
)
t.references(
:user,
type: :integer,
null: false,
foreign_key: { on_delete: :cascade },
index: { name: 'index_approval_merge_request_rules_approved_approvers_2' }
)
t.index [:approval_merge_request_rule_id, :user_id], unique: true, name: 'index_approval_merge_request_rules_approved_approvers_1'
end
end
end
# frozen_string_literal: true
class CreateApprovalRuleMembers < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
TABLES = [
{ table: 'approval_merge_request_rules_users', rule: 'approval_merge_request_rule', member: 'user', member_table: 'users' },
{ table: 'approval_merge_request_rules_groups', rule: 'approval_merge_request_rule', member: 'group', member_table: 'namespaces' },
{ table: 'approval_project_rules_users', rule: 'approval_project_rule', member: 'user', member_table: 'users' },
{ table: 'approval_project_rules_groups', rule: 'approval_project_rule', member: 'group', member_table: 'namespaces' }
].freeze
def up
TABLES.each do |params|
member_id = "#{params[:member]}_id"
rule_id = "#{params[:rule]}_id"
create_table(params[:table], id: :bigserial) do |t|
t.references params[:rule], null: false, type: :bigint, index: false, foreign_key: { on_delete: :cascade }
t.references params[:member], null: false, type: :integer, index: { name: "index_#{params[:table]}_2" }
# To accommodate Group being in the `namespaces` table
t.foreign_key params[:member_table], column: member_id, on_delete: :cascade
t.index [rule_id, member_id], unique: true, name: "index_#{params[:table]}_1"
end
end
end
def down
TABLES.each { |params| drop_table(params[:table]) }
end
end
# frozen_string_literal: true
class CreateApprovalMergeRequestRulesApprovalProjectRules < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
create_table(:approval_merge_request_rule_sources, id: :bigserial) do |t|
t.references(
:approval_merge_request_rule,
type: :bigint,
null: false,
foreign_key: { on_delete: :cascade },
index: { name: 'index_approval_merge_request_rule_sources_1', unique: true }
)
t.references(
:approval_project_rule,
type: :bigint,
null: false,
foreign_key: { on_delete: :cascade },
index: { name: 'index_approval_merge_request_rule_sources_2' }
)
end
end
end
# frozen_string_literal: true
FactoryBot.define do
factory :approval_merge_request_rule do
merge_request
name ApprovalRuleLike::DEFAULT_NAME
end
factory :approval_project_rule do
project
name ApprovalRuleLike::DEFAULT_NAME
end
end
...@@ -6,6 +6,7 @@ milestone: ...@@ -6,6 +6,7 @@ milestone:
- boards - boards
merge_requests: merge_requests:
- reviews - reviews
- approval_rules
- approvals - approvals
- approvers - approvers
- approver_groups - approver_groups
...@@ -50,6 +51,7 @@ project: ...@@ -50,6 +51,7 @@ project:
- jenkins_service - jenkins_service
- jenkins_deprecated_service - jenkins_deprecated_service
- index_status - index_status
- approval_rules
- approvers - approvers
- pages_domains - pages_domains
- audit_events - audit_events
......
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalMergeRequestRule do
let(:merge_request) { create(:merge_request) }
subject { create(:approval_merge_request_rule, merge_request: merge_request) }
describe '#project' do
it 'returns project of MergeRequest' do
expect(subject.project).to be_present
expect(subject.project).to eq(merge_request.project)
end
end
describe '#approvers' do
before do
create(:group) do |group|
group.add_guest(merge_request.author)
subject.groups << group
end
end
context 'when project merge_requests_author_approval is true' do
it 'contains author' do
merge_request.project.update(merge_requests_author_approval: true)
expect(subject.approvers).to contain_exactly(merge_request.author)
end
end
context 'when project merge_requests_author_approval is false' do
it 'contains author' do
merge_request.project.update(merge_requests_author_approval: false)
expect(subject.approvers).to be_empty
end
end
end
describe '#sync_approved_approvers' do
let(:member1) { create(:user) }
let(:member2) { create(:user) }
let(:member3) { create(:user) }
let!(:approval1) { create(:approval, merge_request: merge_request, user: member1) }
let!(:approval2) { create(:approval, merge_request: merge_request, user: member2) }
let!(:approval3) { create(:approval, merge_request: merge_request, user: member3) }
before do
subject.users = [member1, member2]
end
context 'when not merged' do
it 'does nothing' do
subject.sync_approved_approvers
expect(subject.approved_approvers.reload).to be_empty
end
end
context 'when merged' do
let(:merge_request) { create(:merged_merge_request) }
it 'records approved approvers as approved_approvers association' do
subject.sync_approved_approvers
expect(subject.approved_approvers.reload).to contain_exactly(member1, member2)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalProjectRule do
subject { create(:approval_project_rule) }
describe '.regular' do
it 'returns all records' do
rules = create_list(:approval_project_rule, 2)
expect(described_class.regular).to contain_exactly(*rules)
end
end
describe '.code_ownerscope' do
it 'returns nothing' do
create_list(:approval_project_rule, 2)
expect(described_class.code_owner).to be_empty
end
end
describe '#code_owner' do
it 'returns false' do
expect(subject.code_owner).to eq(false)
expect(subject.code_owner?).to eq(false)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalRuleLike do
let(:user1) { create(:user) }
let(:user2) { create(:user) }
let(:user3) { create(:user) }
let(:group1) { create(:group) }
let(:group2) { create(:group) }
let(:merge_request) { create(:merge_request) }
shared_examples 'approval rule like' do
describe '#add_member' do
it 'adds as a member of the rule' do
expect do
subject.add_member(user1)
subject.add_member(group1)
end.to change { subject.users.count }.by(1).and change { subject.groups.count }.by(1)
end
it 'does nothing if already a member' do
subject.add_member(user1)
expect do
subject.add_member(user1)
end.not_to change { subject.users.count + subject.groups.count }
end
end
describe '#remove_member' do
it 'removes a member from the rule' do
subject.add_member(group1)
expect do
subject.remove_member(group1)
end.to change { subject.groups.count }.by(-1)
end
it 'does nothing if not a member' do
expect do
subject.remove_member(group1)
end.not_to change { subject.groups.count }
end
end
describe '#approvers' do
let(:group1_user) { create(:user) }
let(:group2_user) { create(:user) }
before do
subject.users << user1
subject.users << user2
subject.groups << group1
subject.groups << group2
group1.add_guest(group1_user)
group2.add_guest(group2_user)
end
it 'contains users as direct members and group members' do
expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user)
end
context 'when user is both a direct member and a group member' do
before do
group1.add_guest(user1)
group2.add_guest(user2)
end
it 'contains only unique users' do
expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user)
end
end
end
end
context 'MergeRequest' do
subject { create(:approval_merge_request_rule, merge_request: merge_request) }
it_behaves_like 'approval rule like'
end
context 'Project' do
subject { create(:approval_project_rule) }
it_behaves_like 'approval rule like'
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalRules::FinalizeService do
let(:project) { create(:project, :repository) }
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
describe '#execute' do
let!(:user1) { create(:user) }
let!(:user2) { create(:user) }
let!(:user3) { create(:user) }
let!(:group1) { create(:group) }
let!(:group2) { create(:group) }
let!(:group1_user) { create(:user) }
let!(:group2_user) { create(:user) }
let!(:approval1) { create(:approval, merge_request: merge_request, user: user1) }
let!(:approval2) { create(:approval, merge_request: merge_request, user: user3) }
let!(:approval3) { create(:approval, merge_request: merge_request, user: group1_user) }
let!(:approval4) { create(:approval, merge_request: merge_request, user: group2_user) }
let!(:project_rule) { create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12) }
subject { described_class.new(merge_request) }
before do
group1.add_guest(group1_user)
group2.add_guest(group2_user)
project_rule.users = [user1, user2]
project_rule.groups << group1
end
shared_examples 'skipping when unmerged' do
it 'does nothing if unmerged' do
expect do
subject.execute
end.not_to change { ApprovalMergeRequestRule.count }
end
end
context 'when there is no merge request rules' do
it_behaves_like 'skipping when unmerged'
context 'when merged' do
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')
end
it 'copies project rules to MR, keep snapshot of group member by including it as part of users association' do
expect do
subject.execute
end.to change { ApprovalMergeRequestRule.count }.by(1)
rule = merge_request.approval_rules.regular.first
expect(rule.name).to eq('foo')
expect(rule.approvals_required).to eq(12)
expect(rule.users).to contain_exactly(user1, user2, group1_user)
expect(rule.groups).to contain_exactly(group1)
expect(rule.approved_approvers).to contain_exactly(user1, group1_user)
end
end
end
context 'when there is a regular merge request rule' do
before do
rule = create(:approval_merge_request_rule, merge_request: merge_request, name: 'bar', approvals_required: 32)
rule.users = [user2, user3]
rule.groups << group2
end
it_behaves_like 'skipping when unmerged'
context 'when merged' do
let(: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
allow(subject).to receive(:copy_project_approval_rules)
expect do
subject.execute
end.not_to change { ApprovalMergeRequestRule.count }
rule = merge_request.approval_rules.regular.first
expect(rule.name).to eq('bar')
expect(rule.approvals_required).to eq(32)
expect(rule.users).to contain_exactly(user2, user3, group2_user)
expect(rule.groups).to contain_exactly(group2)
expect(rule.approved_approvers).to contain_exactly(user3, group2_user)
expect(subject).not_to have_received(:copy_project_approval_rules)
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