diff --git a/db/schema.rb b/db/schema.rb index 1809ac6d95740dce5980ec2d84e86e6350134c76..65ab91c98726b2e6252fb043884d3c8052b72b64 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -219,6 +219,67 @@ t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree 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| t.integer "merge_request_id", null: false t.integer "user_id", null: false @@ -3163,6 +3224,20 @@ 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", "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 "approver_groups", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "badges", "namespaces", column: "group_id", on_delete: :cascade diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb new file mode 100644 index 0000000000000000000000000000000000000000..ec9e69060dc3d64716eb31fb76e0243ebf279d34 --- /dev/null +++ b/ee/app/models/approval_merge_request_rule.rb @@ -0,0 +1,40 @@ +# 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] + 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 diff --git a/ee/app/models/approval_merge_request_rule_source.rb b/ee/app/models/approval_merge_request_rule_source.rb new file mode 100644 index 0000000000000000000000000000000000000000..bd4b05b655a75944260d6dfc6bab7057595eaa46 --- /dev/null +++ b/ee/app/models/approval_merge_request_rule_source.rb @@ -0,0 +1,7 @@ +# 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 diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb new file mode 100644 index 0000000000000000000000000000000000000000..0083d9a574799843d6e56cead3a871634495095e --- /dev/null +++ b/ee/app/models/approval_project_rule.rb @@ -0,0 +1,16 @@ +# 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 diff --git a/ee/app/models/approver.rb b/ee/app/models/approver.rb index 07d9dc5ed9883ea1e4c10dc6c06d89276b622ac1..9bf17aca25fab131015ba682bd4b70de887123da 100644 --- a/ee/app/models/approver.rb +++ b/ee/app/models/approver.rb @@ -9,4 +9,8 @@ class Approver < ActiveRecord::Base def find_by_user_id(user_id) find_by(user_id: user_id) end + + def member + user + end end diff --git a/ee/app/models/approver_group.rb b/ee/app/models/approver_group.rb index 965a67391286a5e43e22c9c24b367f8e87922b83..d1177657361d48e50c87379c43e582f938130369 100644 --- a/ee/app/models/approver_group.rb +++ b/ee/app/models/approver_group.rb @@ -13,4 +13,8 @@ def self.filtered_approver_groups(approver_groups, user) approver_groups.joins(:group).merge(public_or_visible_groups) end + + def member + group + end end diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb new file mode 100644 index 0000000000000000000000000000000000000000..bb4826d622725ed9c9bf0796d9f2c9003b25f846 --- /dev/null +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -0,0 +1,39 @@ +# 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] + 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 diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index c16417f1c35c254cb69fa1273305eccdceb3570d..18ff8260629d4e81ec5ed322b40f57216e190690 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -16,6 +16,7 @@ module MergeRequest has_many :approved_by_users, through: :approvals, source: :user 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 :approval_rules, class_name: 'ApprovalMergeRequestRule' has_many :draft_notes validate :validate_approvals_before_merge, unless: :importing? diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 2f1a836f56e2a25a8ef459f9b94a9be2d4caa52c..1f253b7b803b9fdb0c5ff8ddcb8eaf0b32696892 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -44,6 +44,7 @@ module Project has_many :reviews, inverse_of: :project 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 :approval_rules, class_name: 'ApprovalProjectRule' has_many :audit_events, as: :entity has_many :path_locks has_many :vulnerability_feedback, class_name: 'Vulnerabilities::Feedback' diff --git a/ee/app/services/approval_rules/finalize_service.rb b/ee/app/services/approval_rules/finalize_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..9d482e87d459fbcdc9dd5e0430dd790572d54f42 --- /dev/null +++ b/ee/app/services/approval_rules/finalize_service.rb @@ -0,0 +1,44 @@ +# 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 diff --git a/ee/db/migrate/20181204031328_create_approval_rules.rb b/ee/db/migrate/20181204031328_create_approval_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..27df6a077360cba5d5676ed4decba48a26dad4e2 --- /dev/null +++ b/ee/db/migrate/20181204031328_create_approval_rules.rb @@ -0,0 +1,26 @@ +# 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 diff --git a/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb b/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb new file mode 100644 index 0000000000000000000000000000000000000000..ff18e58f278ed02c26eb86f2956e02b5376c77a3 --- /dev/null +++ b/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb @@ -0,0 +1,28 @@ +# 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 diff --git a/ee/db/migrate/20181204031330_create_approval_rule_members.rb b/ee/db/migrate/20181204031330_create_approval_rule_members.rb new file mode 100644 index 0000000000000000000000000000000000000000..5ef159c7a630ae3d139b84325cd9cff6ab7842af --- /dev/null +++ b/ee/db/migrate/20181204031330_create_approval_rule_members.rb @@ -0,0 +1,35 @@ +# 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 diff --git a/ee/db/migrate/20181204031331_create_approval_merge_request_rules_approval_project_rules.rb b/ee/db/migrate/20181204031331_create_approval_merge_request_rules_approval_project_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..2190fd65e13b4b02b8170655f3b0d772579a702a --- /dev/null +++ b/ee/db/migrate/20181204031331_create_approval_merge_request_rules_approval_project_rules.rb @@ -0,0 +1,26 @@ +# 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 diff --git a/ee/spec/factories/approval_rules.rb b/ee/spec/factories/approval_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..7c6a1d80dd566f85c70eccbb7028aea75854f92a --- /dev/null +++ b/ee/spec/factories/approval_rules.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/ee/spec/lib/gitlab/import_export/all_models.yml b/ee/spec/lib/gitlab/import_export/all_models.yml index b7f300bca8f2c5b6c285e6d3f9749a1644309f7c..b683afb892eed11ed3f49661fd684dff23ce7be3 100644 --- a/ee/spec/lib/gitlab/import_export/all_models.yml +++ b/ee/spec/lib/gitlab/import_export/all_models.yml @@ -6,6 +6,7 @@ milestone: - boards merge_requests: - reviews +- approval_rules - approvals - approvers - approver_groups @@ -50,6 +51,7 @@ project: - jenkins_service - jenkins_deprecated_service - index_status +- approval_rules - approvers - pages_domains - audit_events diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..16bc819617a59908112695a7134057f071687dd2 --- /dev/null +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -0,0 +1,72 @@ +# 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 diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4410b18e00a8517ba0fa779ad5c949b769908d4b --- /dev/null +++ b/ee/spec/models/approval_project_rule_spec.rb @@ -0,0 +1,30 @@ +# 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 diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ee7e02bbb2d2846a780ad39b0b5b4bb6eb8bf82a --- /dev/null +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -0,0 +1,90 @@ +# 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 diff --git a/ee/spec/services/approval_rules/finalize_service_spec.rb b/ee/spec/services/approval_rules/finalize_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2ed4dec67d539025aab86002b6b446f33ca209c9 --- /dev/null +++ b/ee/spec/services/approval_rules/finalize_service_spec.rb @@ -0,0 +1,100 @@ +# 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