diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 02c2388c05cf642e4ea8fd61848d9f488d207724..410351520301cecd4fdefcc1b6f29f7224adb66e 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -66,3 +66,5 @@ def set_projects! end end end + +MergeRequests::CreateService.include(EE::MergeRequests::CreateService) diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index f26e3bee06f68cb35875064108c32769dda98715..f247349ecbd83cdeca9c175a31dc3efdfcee2b68 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -52,3 +52,5 @@ def create_event(merge_request) end end end + +MergeRequests::PostMergeService.prepend(EE::MergeRequests::PostMergeService) diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index ec9e69060dc3d64716eb31fb76e0243ebf279d34..6dbc1bb45ce4e7c5e9ce9f14ef98838a1cb9ef9e 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -3,6 +3,8 @@ class ApprovalMergeRequestRule < ApplicationRecord include ApprovalRuleLike + DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner' + scope :regular, -> { where(code_owner: false) } scope :code_owner, -> { where(code_owner: true) } # special code owner rules, updated internally when code changes diff --git a/ee/app/models/approver.rb b/ee/app/models/approver.rb index 9bf17aca25fab131015ba682bd4b70de887123da..26c3502fde44d565d66df3b3aef2b274259cf1b3 100644 --- a/ee/app/models/approver.rb +++ b/ee/app/models/approver.rb @@ -4,6 +4,8 @@ class Approver < ActiveRecord::Base belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :user + include ApproverMigrateHook + validates :user, presence: true def find_by_user_id(user_id) diff --git a/ee/app/models/approver_group.rb b/ee/app/models/approver_group.rb index d1177657361d48e50c87379c43e582f938130369..3c1cd1cb0d6947a94ac274b493c56b75dc94442d 100644 --- a/ee/app/models/approver_group.rb +++ b/ee/app/models/approver_group.rb @@ -4,6 +4,8 @@ class ApproverGroup < ActiveRecord::Base belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :group + include ApproverMigrateHook + validates :group, presence: true delegate :users, to: :group diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index bb4826d622725ed9c9bf0796d9f2c9003b25f846..03d0dc29faa7868f25c5b60ee421145fffba0ad3 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -8,7 +8,7 @@ module ApprovalRuleLike 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 + has_many :group_users, -> { distinct }, through: :groups, source: :users validates :name, presence: true end diff --git a/ee/app/models/concerns/approver_migrate_hook.rb b/ee/app/models/concerns/approver_migrate_hook.rb new file mode 100644 index 0000000000000000000000000000000000000000..738dc48bdda49207430dba254bbaf9d4854fe912 --- /dev/null +++ b/ee/app/models/concerns/approver_migrate_hook.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true +# +# Sync up old models (Approver and ApproverGroup) +# to new models (ApprovalMergeRequestRule and ApprovalProjectRule) +# +# TODO: remove once #1979 is closed + +module ApproverMigrateHook + extend ActiveSupport::Concern + + included do + after_commit :schedule_approval_migration, on: [:create, :destroy] + end + + def schedule_approval_migration + # After merge, approval information is frozen + return if target.is_a?(MergeRequest) && target.merged? + + Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform(target.class.name, target.id, sync_code_owner_rule: false) + end +end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 18ff8260629d4e81ec5ed322b40f57216e190690..bbd9f900b659165e140c0db67dfd1e26bc20f3ea 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -81,5 +81,22 @@ def compare_license_management_reports compare_reports(::Ci::CompareLicenseManagementReportsService) end + + def sync_code_owners_with_approvers + return if merged? + + owners = code_owners + + if owners.present? + ActiveRecord::Base.transaction do + rule = approval_rules.code_owner.first + rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER) + + rule.users = code_owners.uniq + end + else + approval_rules.code_owner.delete_all + end + end end end diff --git a/ee/app/services/ee/merge_requests/create_service.rb b/ee/app/services/ee/merge_requests/create_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..bc0ca4b3bd84854859f8be339b2d0b4cc15cec53 --- /dev/null +++ b/ee/app/services/ee/merge_requests/create_service.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module MergeRequests + module CreateService + extend ::Gitlab::Utils::Override + + override :after_create + def after_create(issuable) + super + + issuable.sync_code_owners_with_approvers + end + end + end +end diff --git a/ee/app/services/ee/merge_requests/post_merge_service.rb b/ee/app/services/ee/merge_requests/post_merge_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..3427932858bbd5df5680b5074d5d88dd9cdf6937 --- /dev/null +++ b/ee/app/services/ee/merge_requests/post_merge_service.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module EE + module MergeRequests + module PostMergeService + extend ::Gitlab::Utils::Override + + override :execute + def execute(merge_request) + super + ApprovalRules::FinalizeService.new(merge_request).execute + end + end + end +end diff --git a/ee/app/services/ee/merge_requests/refresh_service.rb b/ee/app/services/ee/merge_requests/refresh_service.rb index b647c27330fdc2ded5b4f0ded44c7f894768247b..25c6af35585cfd572a6173d9617c10a6055cd30d 100644 --- a/ee/app/services/ee/merge_requests/refresh_service.rb +++ b/ee/app/services/ee/merge_requests/refresh_service.rb @@ -53,6 +53,8 @@ def update_approvers new_code_owners = merge_request.code_owners - previous_code_owners create_approvers(merge_request, new_code_owners) + + merge_request.sync_code_owners_with_approvers end results diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb index a5e70e52072c00204cd6f7b4cc42bc90c2aa1bd6..b32f01f957f6254a9d5ac9b50924a0ab1db39516 100644 --- a/ee/app/services/ee/merge_requests/update_service.rb +++ b/ee/app/services/ee/merge_requests/update_service.rb @@ -25,6 +25,8 @@ def execute(merge_request) cleanup_approvers(merge_request, reload: true) end + sync_approval_rules(merge_request) + merge_request end @@ -42,6 +44,14 @@ def reset_approvals(merge_request) merge_request.approvals.delete_all if target_project.reset_approvals_on_push end + + # TODO remove after #1979 is closed + def sync_approval_rules(merge_request) + return if merge_request.merged? + return unless merge_request.previous_changes.include?(:approvals_before_merge) + + merge_request.approval_rules.regular.update_all(approvals_required: merge_request.approvals_before_merge) + end end end end diff --git a/ee/app/services/ee/projects/update_service.rb b/ee/app/services/ee/projects/update_service.rb index d4666acec9f9754a8620ef9963b5978239b17285..80617e43ef7ec866b83aa9155b2c1ccee9db857e 100644 --- a/ee/app/services/ee/projects/update_service.rb +++ b/ee/app/services/ee/projects/update_service.rb @@ -37,6 +37,8 @@ def execute sync_wiki_on_enable if !wiki_was_enabled && project.wiki_enabled? project.import_state.force_import_job! if params[:mirror].present? && project.mirror? + + sync_approval_rules end result @@ -67,6 +69,13 @@ def log_audit_events def sync_wiki_on_enable ::Geo::RepositoryUpdatedService.new(project.wiki.repository).execute end + + # TODO remove after #1979 is closed + def sync_approval_rules + return unless project.previous_changes.include?(:approvals_before_merge) + + project.approval_rules.update_all(approvals_required: project.approvals_before_merge) + end end end end diff --git a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb new file mode 100644 index 0000000000000000000000000000000000000000..913e036eafb1fa6b51e3f1de55f135216ef73577 --- /dev/null +++ b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +class MigrateProjectApprovers < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + BATCH_SIZE = 3000 + + class MergeRequest < ActiveRecord::Base + include ::EachBatch + self.table_name = 'merge_requests' + end + + class Approver < ActiveRecord::Base + self.table_name = 'approvers' + end + + class ApproverGroup < ActiveRecord::Base + self.table_name = 'approver_groups' + end + + def up + get_project_ids.each do |project_id| + Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform('Project', project_id) + end + + bulk_queue_background_migration_jobs_by_range(MergeRequest, 'MigrateApproverToApprovalRulesInBatch', batch_size: BATCH_SIZE) + + check_time = Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress::RESCHEDULE_DELAY + BackgroundMigrationWorker.bulk_perform_in(check_time, [['MigrateApproverToApprovalRulesCheckProgress']]) + end + + def down + end + + private + + def get_project_ids + results = ActiveRecord::Base.connection.exec_query <<~SQL + SELECT DISTINCT target_id FROM ( + SELECT target_id FROM approvers WHERE target_type='Project' + UNION + SELECT target_id FROM approver_groups WHERE target_type='Project' + ) t + SQL + + results.rows.flatten + end +end diff --git a/ee/lib/api/merge_request_approvals.rb b/ee/lib/api/merge_request_approvals.rb index 1a189f1edbb8c9c701468a67f0f224cb08fa44af..48b5100daf0018c5d93f77bdaa86ce886346f186 100644 --- a/ee/lib/api/merge_request_approvals.rb +++ b/ee/lib/api/merge_request_approvals.rb @@ -77,6 +77,8 @@ def handle_merge_request_errors!(errors) requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.' end put 'approvers' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/8883') + merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers) merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request) diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..ee5006991d9156041949eac56a782c87f83effca --- /dev/null +++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # A Project/MergeRequest level migration, aiming to convert existing data + # (from approvers, approver_groups tables) + # to new rule based schema. + class MigrateApproverToApprovalRules + include Gitlab::Utils::StrongMemoize + + class Approver < ActiveRecord::Base + self.table_name = 'approvers' + end + + class ApproverGroup < ActiveRecord::Base + self.table_name = 'approver_groups' + end + + class ApprovalMergeRequestRule < ActiveRecord::Base + self.table_name = 'approval_merge_request_rules' + + belongs_to :merge_request + scope :code_owner, -> { where(code_owner: true) } + scope :regular, -> { where(code_owner: false) } # Non code owner rule + + has_and_belongs_to_many :users + has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_merge_request_rules_groups + 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 + end + + class ApprovalMergeRequestRuleSource < ActiveRecord::Base + self.table_name = 'approval_merge_request_rule_sources' + belongs_to :approval_merge_request_rule + belongs_to :approval_project_rule + end + + class ApprovalProjectRule < ActiveRecord::Base + self.table_name = 'approval_project_rules' + + belongs_to :project + has_and_belongs_to_many :users + has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_project_rules_groups + + scope :regular, -> { all } + end + + class MergeRequest < ActiveRecord::Base + self.table_name = 'merge_requests' + + belongs_to :target_project, class_name: "Project" + has_many :approval_rules, class_name: 'ApprovalMergeRequestRule' + + def approver_ids + @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).pluck('distinct user_id') + end + + def approver_group_ids + @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).pluck('distinct group_id') + end + + def sync_code_owners_with_approvers + return if state == 'merged' || state == 'closed' + + Gitlab::GitalyClient.allow_n_plus_1_calls do + ::MergeRequest.find(id).sync_code_owners_with_approvers + end + end + end + + class Project < ActiveRecord::Base + self.table_name = 'projects' + + has_many :approval_rules, class_name: 'ApprovalProjectRule' + + def approver_ids + @approver_ids ||= Approver.where(target_type: 'Project', target_id: id).pluck('distinct user_id') + end + + def approver_group_ids + @approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).pluck('distinct group_id') + end + end + + class User < ActiveRecord::Base + self.table_name = 'users' + end + + ALLOWED_TARGET_TYPES = %w{MergeRequest Project}.freeze + + # @param target_type [String] class of target, either 'MergeRequest' or 'Project' + # @param target_id [Integer] id of target + def perform(target_type, target_id, sync_code_owner_rule: true) + @target_type = target_type + @target_id = target_id + @sync_code_owner_rule = sync_code_owner_rule + + raise "Incorrect target_type #{target_type}" unless ALLOWED_TARGET_TYPES.include?(@target_type) + + ActiveRecord::Base.transaction do + case target + when MergeRequest + handle_merge_request + when Project + handle_project + end + end + end + + private + + def handle_merge_request + if rule = sync_rule + rule.approval_project_rule = target.target_project.approval_rules.regular.first + end + + target.sync_code_owners_with_approvers if @sync_code_owner_rule + end + + def handle_project + sync_rule + end + + def sync_rule + unless approvers_exists? + target.approval_rules.regular.delete_all + return + end + + rule = first_or_initialize + rule.update(user_ids: target.approver_ids, group_ids: target.approver_group_ids) + rule + end + + def target + strong_memoize(:target) do + case @target_type + when 'MergeRequest' + MergeRequest.find_by(id: @target_id) + when 'Project' + Project.find_by(id: @target_id) + end + end + end + + def first_or_initialize + rule = target.approval_rules.regular.first_or_initialize + + unless rule.persisted? + rule.name ||= ApprovalRuleLike::DEFAULT_NAME + rule.approvals_required = target.approvals_before_merge || 0 + rule.save! + end + + rule + end + + def approvers_exists? + target.approver_ids.any? || target.approver_group_ids.any? + end + end + end +end diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb new file mode 100644 index 0000000000000000000000000000000000000000..a5b45c467b3e41c52a2700eb771838b0fdb3e91d --- /dev/null +++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + class MigrateApproverToApprovalRulesCheckProgress + RESCHEDULE_DELAY = 1.day + + def perform + if remaining? + BackgroundMigrationWorker.perform_in(RESCHEDULE_DELAY, self.class.name) + else + Feature.enable(:approval_rule) + end + end + + private + + def remaining? + Gitlab::BackgroundMigration.exists?('MigrateApproverToApprovalRulesInBatch') + end + end + end +end diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb new file mode 100644 index 0000000000000000000000000000000000000000..5808b5d434feae1650b2f05ded8244a1c253eb9a --- /dev/null +++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + class MigrateApproverToApprovalRulesInBatch + class MergeRequest < ActiveRecord::Base + self.table_name = 'merge_requests' + end + + def perform(start_id, end_id) + merge_request_ids = MergeRequest.where('id >= ? AND id <= ?', start_id, end_id).pluck(:id) + merge_request_ids.each do |merge_request_id| + MigrateApproverToApprovalRules.new.perform('MergeRequest', merge_request_id) + end + end + end + end +end diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..f2f7c811da0ed26aba45876468c47386a51f26e1 --- /dev/null +++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress do + context 'when there is MigrateApproverToApprovalRulesInBatch jobs' do + it 'reschedules check' do + allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(true) + + expect(BackgroundMigrationWorker).to receive(:perform_in).with(described_class::RESCHEDULE_DELAY, described_class.name) + + described_class.new.perform + end + end + + context 'when there is no more MigrateApproverToApprovalRulesInBatch jobs' do + it 'enables feature' do + allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false) + + expect(Feature).to receive(:enable).with(:approval_rule) + + described_class.new.perform + end + end +end diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b34c38f74abc06775dabae0c62cd676be280f1e4 --- /dev/null +++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesInBatch do + context 'when there is no more MigrateApproverToApprovalRules jobs' do + let(:job) { double(:job) } + let(:project) { create(:project) } + + it 'migrates individual target' do + allow(Gitlab::BackgroundMigration::MigrateApproverToApprovalRules).to receive(:new).and_return(job) + + merge_requests = create_list(:merge_request, 3) + + expect(job).to receive(:perform).exactly(3).times + + described_class.new.perform(merge_requests.first.id, merge_requests.last.id) + end + end +end diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..e9c89df6b0d47f75be3fbb6f9eaa0bdabde94418 --- /dev/null +++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb @@ -0,0 +1,198 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do + def create_skip_sync(*args) + build(*args) do |record| + allow(record).to receive(:schedule_approval_migration) + record.save! + end + end + + def create_member_in(member, *populate_in) + if populate_in.include?(:old_schema) + case member + when User + create_skip_sync(:approver, target_type: target_type, target_id: target.id, user_id: member.id) + when Group + create_skip_sync(:approver_group, target_type: target_type, target_id: target.id, group_id: member.id) + end + end + + if populate_in.include?(:new_schema) + approval_rule.add_member(member) + end + end + + context 'sync approval rule and its members' do + shared_examples 'sync approval member' do + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:group1) { create(:group) } + let(:group2) { create(:group) } + + context 'when member in old schema but not in new schema' do + before do + create_member_in(user1, :old_schema) + create_member_in(group1, :old_schema) + end + + it 'creates in new schema' do + expect do + described_class.new.perform(target_type, target.id) + end.to change { approval_rule.users.count }.by(1) + .and change { approval_rule.groups.count }.by(1) + + approval_rule = target.approval_rules.first + + expect(approval_rule.approvals_required).to eq(0) + expect(approval_rule.name).to eq('Default') + + expect(approval_rule.users).to contain_exactly(user1) + expect(approval_rule.groups).to contain_exactly(group1) + end + end + + context 'when member not in old schema but in new schema' do + before do + create_member_in(user1, :new_schema) + create_member_in(user2, :old_schema, :new_schema) + create_member_in(group1, :new_schema) + create_member_in(group2, :old_schema, :new_schema) + end + + it 'removes in new schema' do + expect do + described_class.new.perform(target_type, target.id) + end.to change { approval_rule.users.count }.by(-1) + .and change { approval_rule.groups.count }.by(-1) + + approval_rule = target.approval_rules.first + + expect(approval_rule.users).to contain_exactly(user2) + expect(approval_rule.groups).to contain_exactly(group2) + end + end + end + + context 'merge request' do + let(:target) { create(:merge_request) } + let(:target_type) { 'MergeRequest' } + let(:approval_rule) { create(:approval_merge_request_rule, merge_request: target) } + + it_behaves_like 'sync approval member' + + context 'when project rule is present' do + let!(:project_rule) { create(:approval_project_rule, project: target.target_project) } + + it "sets MR rule's source to project rule without duplication" do + user = create(:user) + create_member_in(user, :old_schema) + create_member_in(user, :old_schema) + + described_class.new.perform(target_type, target.id) + + expect(target.approval_rules.regular.first.approval_project_rule).to eq(project_rule) + end + end + + context 'when project rule is absent' do + it "has MR rule's source as nil" do + create_member_in(create(:user), :old_schema) + + described_class.new.perform(target_type, target.id) + + expect(target.approval_rules.regular.first.approval_project_rule).to eq(nil) + end + end + + context 'when approver is no longer overwritten' do + before do + create_member_in(create(:user), :new_schema) + create_member_in(create(:group), :new_schema) + end + + it 'removes rule' do + expect do + described_class.new.perform(target_type, target.id) + end.to change { approval_rule.users.count }.by(-1) + .and change { approval_rule.groups.count }.by(-1) + + expect(target.approval_rules.exists?(approval_rule.id)).to eq(false) + end + end + + context '#sync_code_owners_with_approvers' do + let(:owners) { create_list(:user, 2) } + + before do + allow(::Gitlab::CodeOwners).to receive(:for_merge_request).and_return(owners) + end + + context 'when merge request is merged' do + let(:target) { create(:merged_merge_request) } + + it 'does nothing' do + expect do + described_class.new.perform(target_type, target.id) + end.not_to change { target.approval_rules.count } + end + end + + context 'when code owner rule does not exist' do + it 'creates rule' do + expect do + described_class.new.perform(target_type, target.id) + end.to change { target.approval_rules.code_owner.count }.by(1) + + expect(target.approval_rules.code_owner.first.users).to contain_exactly(*owners) + end + 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)]) } + + it 'reuses and updates existing rule' do + expect do + described_class.new.perform(target_type, target.id) + end.not_to change { target.approval_rules.count } + + expect(code_owner_rule.reload.users).to contain_exactly(*owners) + end + + context 'when there is no code owner' do + let(:owners) { [] } + + it 'removes rule' do + described_class.new.perform(target_type, target.id) + + expect(target.approval_rules.exists?(code_owner_rule.id)).to eq(false) + end + end + end + end + end + + context 'project' do + let(:target) { create(:project) } + let(:target_type) { 'Project' } + let(:approval_rule) { create(:approval_project_rule, project: target) } + + it_behaves_like 'sync approval member' + end + end + + context 'when target is deleted' do + let(:target) { create(:project) } + let(:target_type) { 'Project' } + + it "does not err" do + target.destroy + + expect do + described_class.new.perform(target_type, target.id) + end.not_to raise_error + end + end +end diff --git a/ee/spec/migrations/migrate_project_approvers_spec.rb b/ee/spec/migrations/migrate_project_approvers_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d624019dab87e4bcc92b75521d99936faf58a417 --- /dev/null +++ b/ee/spec/migrations/migrate_project_approvers_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('ee', 'db', 'post_migrate', '20181204040404_migrate_project_approvers.rb') + +describe MigrateProjectApprovers, :migration do + let(:migration) { described_class.new } + + describe '#up' do + let(:projects) { table(:projects) } + let(:namespaces) { table(:namespaces) } + let(:approvers) { table(:approvers) } + let(:approval_project_rules) { table(:approval_project_rules) } + let(:approval_project_rules_users) { table(:approval_project_rules_users) } + let(:users) { table(:users) } + let(:features) { table(:features) } + + before do + namespaces.create(id: 1, name: 'gitlab-org', path: 'gitlab-org') + projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1', namespace_id: 1) + projects.create!(id: 124, name: 'gitlab2', path: 'gitlab2', namespace_id: 1) + + users.create(id: 1, name: 'user1', email: 'user1@example.com', projects_limit: 0) + users.create(id: 2, name: 'user2', email: 'user2@example.com', projects_limit: 0) + + approvers.create!(target_id: 123, target_type: 'Project', user_id: 1) + approvers.create!(target_id: 124, target_type: 'Project', user_id: 2) + end + + it 'creates approval rules and its associations' do + migrate! + + expect(approval_project_rules.pluck(:project_id)).to eq([123, 124]) + + rule_ids = approval_project_rules.pluck(:id) + + expect(approval_project_rules_users.where(approval_project_rule_id: rule_ids.first).pluck(:user_id)).to contain_exactly(1) + expect(approval_project_rules_users.where(approval_project_rule_id: rule_ids.last).pluck(:user_id)).to contain_exactly(2) + expect(features.where(key: 'approval_rule').exists?).to eq(true) + 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 index ee7e02bbb2d2846a780ad39b0b5b4bb6eb8bf82a..7df935b6e402edb5c94d565f7364a2bc730ece0a 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -87,4 +87,16 @@ it_behaves_like 'approval rule like' end + + context '.group_users' do + subject { create(:approval_project_rule) } + + it 'returns distinct users' do + group1.add_guest(user1) + group2.add_guest(user1) + subject.groups = [group1, group2] + + expect(subject.group_users).to eq([user1]) + end + end end diff --git a/ee/spec/models/concerns/approver_migrate_hook_spec.rb b/ee/spec/models/concerns/approver_migrate_hook_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b254b1d88999c290e4ff407dc6f8c8611d63b32b --- /dev/null +++ b/ee/spec/models/concerns/approver_migrate_hook_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ApproverMigrateHook do + def members(rule) + rule.users.reload + rule.groups.reload + end + + context 'create rule and member mapping' do + shared_examples 'migrating approver' do + context 'when approver is created' do + it 'creates rule and member mapping' do + expect do + approver + end.to change { target.approval_rules.count }.by(1) + + rule = target.approval_rules.regular.last + + expect(rule.approvals_required).to eq(0) + expect(rule.name).to eq('Default') + expect(members(rule)).to contain_exactly(approver.member) + end + + context 'when rule already exists' do + let!(:approval_rule) { target.approval_rules.create(name: 'foo') } + + it 'reuses rule' do + expect do + approver + end.not_to change { target.approval_rules.regular.count } + + rule = target.approval_rules.regular.last + + expect(rule).to eq(approval_rule) + expect(members(rule)).to contain_exactly(approver.member) + end + + context 'when member mapping already exists' do + before do + approval_rule.add_member approver.member + end + + it 'does nothing' do + approver + + expect(members(approval_rule)).to contain_exactly(approver.member) + end + end + end + end + + context 'when approver is destroyed' do + it 'destroys rule member' do + approver + + rule = target.approval_rules.regular.first + + expect do + approver.destroy! + end.to change { members(rule).count }.by(-1) + end + end + end + + context 'User' do + let(:approver) { create(:approver, target: target) } + + context 'merge request' do + let(:target) { create(:merge_request) } + + it_behaves_like 'migrating approver' + end + + context 'project' do + let(:target) { create(:project) } + + it_behaves_like 'migrating approver' + end + end + + context 'Group' do + let(:approver) { create(:approver_group, target: target) } + + context 'merge request' do + let(:target) { create(:merge_request) } + + it_behaves_like 'migrating approver' + end + + context 'project' do + let(:target) { create(:project) } + + it_behaves_like 'migrating approver' + end + end + end +end diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index 309ac585fd0d12acb37a53ea07649bf281829358..a8488bb9e5c22acf9907ecf0f17ddcf863ce3366 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -70,6 +70,54 @@ end end + describe '#sync_code_owners_with_approvers' do + let(:owners) { create_list(:user, 2) } + + before do + allow(subject).to receive(:code_owners).and_return(owners) + end + + it 'does nothing when merge request is merged' do + allow(subject).to receive(:merged?).and_return(true) + + expect do + subject.sync_code_owners_with_approvers + end.not_to change { subject.approval_rules.count } + end + + context 'when code owner rule does not exist' do + it 'creates rule' do + expect do + subject.sync_code_owners_with_approvers + end.to change { subject.approval_rules.code_owner.count }.by(1) + + expect(subject.approval_rules.code_owner.first.users).to contain_exactly(*owners) + end + end + + context 'when code owner rule exists' do + let!(:code_owner_rule) { subject.approval_rules.code_owner.create!(name: 'Code Owner', users: [create(:user)]) } + + it 'reuses and updates existing rule' do + expect do + subject.sync_code_owners_with_approvers + end.not_to change { subject.approval_rules.count } + + expect(code_owner_rule.reload.users).to contain_exactly(*owners) + end + + context 'when there is no code owner' do + let(:owners) { [] } + + it 'removes rule' do + subject.sync_code_owners_with_approvers + + expect(subject.approval_rules.exists?(code_owner_rule.id)).to eq(false) + end + end + end + end + describe '#base_pipeline' do let!(:pipeline) { create(:ci_empty_pipeline, project: subject.project, sha: subject.diff_base_sha) } diff --git a/ee/spec/services/ee/merge_requests/create_service_spec.rb b/ee/spec/services/ee/merge_requests/create_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..132c8beef0f5c55a31412d119b8b7b1fb8b24de0 --- /dev/null +++ b/ee/spec/services/ee/merge_requests/create_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::CreateService do + include ProjectForksHelper + + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + let(:service) { described_class.new(project, user, opts) } + let(:opts) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master', + force_remove_source_branch: '1' + } + end + + before do + project.add_maintainer(user) + allow(service).to receive(:execute_hooks) + end + + describe '#execute' do + context 'code owners' do + let!(:owners) { create_list(:user, 2) } + + before do + allow(::Gitlab::CodeOwners).to receive(:for_merge_request).and_return(owners) + end + + it 'syncs code owner to approval rule' do + merge_request = service.execute + + rule = merge_request.approval_rules.code_owner.first + + expect(rule.users).to contain_exactly(*owners) + end + end + end +end diff --git a/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb b/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1b47226d8e93ce006aa889ced4db79e45da17ac9 --- /dev/null +++ b/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::PostMergeService do + let(:project) { merge_request.target_project } + let(:merge_request) { create(:merge_request) } + let(:current_user) { merge_request.author } + let(:service) { described_class.new(project, current_user) } + + describe '#execute' do + context 'finalize approvals' do + let(:finalize_service) { double(:finalize_service) } + + it 'executes ApprovalRules::FinalizeService' do + expect(ApprovalRules::FinalizeService).to receive(:new).and_return(finalize_service) + expect(finalize_service).to receive(:execute) + + service.execute(merge_request) + end + end + end +end diff --git a/ee/spec/services/ee/merge_requests/refresh_service_spec.rb b/ee/spec/services/ee/merge_requests/refresh_service_spec.rb index 64da1f747127a6f49bf8735e238ef94e4d0c8b4d..1b167a94b01835b3a27d0cd5327ec0850141874f 100644 --- a/ee/spec/services/ee/merge_requests/refresh_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/refresh_service_spec.rb @@ -102,6 +102,9 @@ it 'does not create Approver' do expect { subject }.not_to change { Approver.count } + + rule = merge_request.approval_rules.code_owner.first + expect(rule.users).to eq(new_owners) end end end @@ -125,6 +128,9 @@ expect(new_approver.user).to eq(owner) expect(new_approver.created_at).to be_present expect(new_approver.updated_at).to be_present + + rule = merge_request.approval_rules.code_owner.first + expect(rule.users).to eq(new_owners) end end end diff --git a/ee/spec/services/merge_requests/update_service_spec.rb b/ee/spec/services/merge_requests/update_service_spec.rb index 5a1ac265b013aa8fc6ef31ba137e0f032105ab2e..7902e1621a8b538e9d1c0a91b22b04f1848d8b93 100644 --- a/ee/spec/services/merge_requests/update_service_spec.rb +++ b/ee/spec/services/merge_requests/update_service_spec.rb @@ -56,5 +56,17 @@ def update_merge_request(opts) end.not_to change { ActionMailer::Base.deliveries.count } end end + + context 'when approvals_before_merge changes' do + it "updates approval_rules' approvals_required" do + rule = create(:approval_merge_request_rule, merge_request: merge_request) + code_owner_rule = create(:approval_merge_request_rule, merge_request: merge_request, code_owner: true) + + update_merge_request(approvals_before_merge: 42) + + expect(rule.reload.approvals_required).to eq(42) + expect(code_owner_rule.reload.approvals_required).to eq(0) + end + end end end diff --git a/ee/spec/services/projects/update_service_spec.rb b/ee/spec/services/projects/update_service_spec.rb index 50754cb8da9448fd1823603e4fe43f3c5f596491..5c644a54b97cab96cbbf702b1c1efd80471e30bf 100644 --- a/ee/spec/services/projects/update_service_spec.rb +++ b/ee/spec/services/projects/update_service_spec.rb @@ -215,6 +215,16 @@ end end + context 'with approval_rules' do + it "updates approval_rules' approvals_required" do + rule = create(:approval_project_rule, project: project) + + update_project(project, user, approvals_before_merge: 42) + + expect(rule.reload.approvals_required).to eq(42) + end + end + def update_project(project, user, opts) Projects::UpdateService.new(project, user, opts).execute end diff --git a/lib/gitlab/background_migration.rb b/lib/gitlab/background_migration.rb index d72befce5718805c0a08a6dc6b6fae2fcc6f732c..6cf40e2d4ca4a3138878d9031a6fc5f016ad9ec5 100644 --- a/lib/gitlab/background_migration.rb +++ b/lib/gitlab/background_migration.rb @@ -51,6 +51,19 @@ def self.perform(class_name, arguments) migration_class_for(class_name).new.perform(*arguments) end + def self.exists?(migration_class) + enqueued = Sidekiq::Queue.new(self.queue) + scheduled = Sidekiq::ScheduledSet.new + + [enqueued, scheduled].each do |queue| + queue.each do |job| + return true if job.queue == self.queue && job.args.first == migration_class + end + end + + false + end + def self.migration_class_for(class_name) const_get(class_name) end diff --git a/spec/lib/gitlab/background_migration_spec.rb b/spec/lib/gitlab/background_migration_spec.rb index 4ad69aeba434fbab23309a34c0d86271627c65f8..8a83b76fd945a6446d27485d6b28e332e359f37b 100644 --- a/spec/lib/gitlab/background_migration_spec.rb +++ b/spec/lib/gitlab/background_migration_spec.rb @@ -119,4 +119,48 @@ described_class.perform('Foo', [10, 20]) end end + + describe '.exists?' do + context 'when there are enqueued jobs present' do + let(:queue) do + [double(args: ['Foo', [10, 20]], queue: described_class.queue)] + end + + before do + allow(Sidekiq::Queue).to receive(:new) + .with(described_class.queue) + .and_return(queue) + end + + it 'returns true if specific job exists' do + expect(described_class.exists?('Foo')).to eq(true) + end + + it 'returns false if specific job does not exist' do + expect(described_class.exists?('Bar')).to eq(false) + end + end + + context 'when there are scheduled jobs present', :sidekiq, :redis do + before do + Sidekiq::Testing.disable! do + BackgroundMigrationWorker.perform_in(10.minutes, 'Foo') + + expect(Sidekiq::ScheduledSet.new).to be_one + end + end + + after do + Sidekiq::ScheduledSet.new.clear + end + + it 'returns true if specific job exists' do + expect(described_class.exists?('Foo')).to eq(true) + end + + it 'returns false if specific job does not exist' do + expect(described_class.exists?('Bar')).to eq(false) + end + end + end end