Merge branch '1979-1-2-fix-hook' into 'master'

Fix nil approvals_before_merge migrated as 0 approvals_required

See merge request gitlab-org/gitlab-ee!9143

ee/app/services/ee/merge_requests/update_service.rb

@@ -50,7 +50,8 @@ module EE
         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)
+        approvals_required = merge_request.approvals_before_merge || merge_request.target_project.approvals_before_merge
+        merge_request.approval_rules.regular.update_all(approvals_required: approvals_required)
       end
     end
   end

ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb

@@ -57,6 +57,10 @@ module Gitlab
         belongs_to :target_project, class_name: "Project"
         has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
 
+        def approvals_required
+          approvals_before_merge || target_project.approvals_before_merge
+        end
+
         def approver_ids
           @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).joins(:user).pluck('distinct user_id')
         end
@@ -86,6 +90,10 @@ module Gitlab
         def approver_group_ids
           @approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).joins(:group).pluck('distinct group_id')
         end
+
+        def approvals_required
+          approvals_before_merge
+        end
       end
 
       class User < ActiveRecord::Base
@@ -154,7 +162,7 @@ module Gitlab
 
         unless rule.persisted?
           rule.name ||= ApprovalRuleLike::DEFAULT_NAME
-          rule.approvals_required = target.approvals_before_merge || 0
+          rule.approvals_required = target.approvals_required
           rule.save!
         end
 

ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb

@@ -44,14 +44,27 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
           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(target.approval_rules.regular.first).to eq(approval_rule)
 
           expect(approval_rule.users).to contain_exactly(user1)
           expect(approval_rule.groups).to contain_exactly(group1)
         end
+
+        context 'when rule is not created yet' do
+          let(:approval_rule) { nil }
+
+          it 'creates rule in new schema' do
+            described_class.new.perform(target_type, target.id)
+
+            approval_rule = target.approval_rules.regular.first
+
+            expect(approval_rule.approvals_required).to eq(2)
+            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
       end
 
       context 'when member not in old schema but in new schema' do
@@ -98,7 +111,7 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
     end
 
     context 'merge request' do
-      let(:target) { create(:merge_request) }
+      let(:target) { create(:merge_request, approvals_before_merge: 2) }
       let(:target_type) { 'MergeRequest' }
       let(:approval_rule) { create(:approval_merge_request_rule, merge_request: target) }
 
@@ -128,6 +141,18 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
         end
       end
 
+      context 'when approvals_before_merge is nil' do
+        it "updates with project's approvals_required" do
+          target.target_project.update(approvals_before_merge: 3)
+          target.update(approvals_before_merge: nil)
+          create_member_in(create(:user), :old_schema)
+
+          described_class.new.perform(target_type, target.id)
+
+          expect(target.approval_rules.regular.first.approvals_required).to eq(3)
+        end
+      end
+
       context 'when approver is no longer overwritten' do
         before do
           create_member_in(create(:user), :new_schema)
@@ -196,7 +221,7 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
     end
 
     context 'project' do
-      let(:target) { create(:project) }
+      let(:target) { create(:project, approvals_before_merge: 2) }
       let(:target_type) { 'Project' }
       let(:approval_rule) { create(:approval_project_rule, project: target) }
 

ee/spec/services/merge_requests/update_service_spec.rb

@@ -58,14 +58,25 @@ describe MergeRequests::UpdateService, :mailer do
     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)
+      using RSpec::Parameterized::TableSyntax
 
-        update_merge_request(approvals_before_merge: 42)
+      where(:project_value, :mr_before_value, :mr_after_value, :result) do
+        3 | 4   | 5   | 5
+        3 | 4   | nil | 3
+        3 | nil | 5   | 5
+      end
+
+      with_them do
+        let(:project) { create(:project, :repository, approvals_before_merge: project_value) }
+
+        it "updates approval_rules' approvals_required" do
+          merge_request.update(approvals_before_merge: mr_before_value)
+          rule = create(:approval_merge_request_rule, merge_request: merge_request)
+
+          update_merge_request(approvals_before_merge: mr_after_value)
 
-        expect(rule.reload.approvals_required).to eq(42)
-        expect(code_owner_rule.reload.approvals_required).to eq(0)
+          expect(rule.reload.approvals_required).to eq(result)
+        end
       end
     end
   end