Merge branch '10395-COAR-phase-2' into 'master'

Move "Require code owner approval" setting from Project to ProtectedBranches

See merge request gitlab-org/gitlab!16187

app/models/protected_branch.rb

@@ -40,6 +40,11 @@ class ProtectedBranch < ApplicationRecord
   def self.protected_refs(project)
     project.protected_branches.select(:name)
   end
+
+  def self.branch_requires_code_owner_approval?(project, branch_name)
+    # NOOP
+    #
+  end
 end
 
 ProtectedBranch.prepend_if_ee('EE::ProtectedBranch')

db/post_migrate/20190827102026_migrate_code_owner_approval_status_to_protected_branches_in_batches.rb

+# frozen_string_literal: true
+
+class MigrateCodeOwnerApprovalStatusToProtectedBranchesInBatches < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+  BATCH_SIZE = 200
+
+  class Project < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'projects'
+    self.inheritance_column = :_type_disabled
+
+    has_many :protected_branches
+  end
+
+  class ProtectedBranch < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'protected_branches'
+    self.inheritance_column = :_type_disabled
+
+    belongs_to :project
+  end
+
+  def up
+    add_concurrent_index :projects, :id, name: "temp_active_projects_with_prot_branches", where: 'archived = false and pending_delete = false and merge_requests_require_code_owner_approval = true'
+
+    ProtectedBranch
+      .joins(:project)
+      .where(projects: { archived: false, pending_delete: false, merge_requests_require_code_owner_approval: true })
+      .each_batch(of: BATCH_SIZE) do |batch|
+        batch.update_all(code_owner_approval_required: true)
+      end
+
+    remove_concurrent_index_by_name(:projects, "temp_active_projects_with_prot_branches")
+  end
+
+  def down
+    # noop
+    #
+  end
+end

ee/app/models/approval_wrapped_rule.rb

@@ -92,16 +92,15 @@ class ApprovalWrappedRule
 
   def code_owner_approvals_required
     strong_memoize(:code_owner_approvals_required) do
-      next 0 unless merge_requests_require_code_owner_approval?
+      next 0 unless branch_requires_code_owner_approval?
 
       approvers.any? ? REQUIRED_APPROVALS_PER_CODE_OWNER_RULE : 0
     end
   end
 
-  def merge_requests_require_code_owner_approval?
+  def branch_requires_code_owner_approval?
     return false unless project.code_owner_approval_required_available?
 
-    project.merge_requests_require_code_owner_approval? ||
-      project.branch_requires_code_owner_approval?(merge_request.target_branch)
+    ProtectedBranch.branch_requires_code_owner_approval?(project, merge_request.target_branch)
   end
 end

ee/app/models/concerns/ee/protected_branch.rb

@@ -8,6 +8,14 @@ module EE
       protected_ref_access_levels :unprotect
     end
 
+    class_methods do
+      def branch_requires_code_owner_approval?(project, branch_name)
+        return false unless project.code_owner_approval_required_available?
+
+        project.protected_branches.requiring_code_owner_approval.matching(branch_name).any?
+      end
+    end
+
     def code_owner_approval_required
       super && project.code_owner_approval_required_available?
     end

ee/app/models/ee/project.rb

@@ -102,7 +102,7 @@ module EE
       scope :verification_failed_wikis, -> { joins(:repository_state).merge(ProjectRepositoryState.verification_failed_wikis) }
       scope :for_plan_name, -> (name) { joins(namespace: :plan).where(plans: { name: name }) }
       scope :requiring_code_owner_approval,
-            -> { where(merge_requests_require_code_owner_approval: true) }
+            -> { joins(:protected_branches).where(protected_branches: { code_owner_approval_required: true }) }
 
       scope :with_security_reports_stored, -> { where('EXISTS (?)', ::Vulnerabilities::Occurrence.scoped_project.select(1)) }
       scope :with_security_reports, -> { where('EXISTS (?)', ::Ci::JobArtifact.security_reports.scoped_project.select(1)) }
@@ -377,13 +377,14 @@ module EE
     end
 
     def merge_requests_require_code_owner_approval?
-      super && code_owner_approval_required_available?
+      code_owner_approval_required_available? &&
+        protected_branches.requiring_code_owner_approval.any?
     end
 
     def branch_requires_code_owner_approval?(branch_name)
       return false unless code_owner_approval_required_available?
 
-      protected_branches.requiring_code_owner_approval.matching(branch_name).any?
+      ::ProtectedBranch.branch_requires_code_owner_approval?(self, branch_name)
     end
 
     def require_password_to_approve

ee/changelogs/unreleased/10395-COAR-phase-2.yml

+---
+title: Move Require code owner approval setting from Project to ProtectedBranches
+merge_request: 16187
+author:
+type: changed

ee/spec/factories/projects.rb

@@ -55,9 +55,5 @@ FactoryBot.modify do
     trait :random_last_repository_updated_at do
       last_repository_updated_at { rand(1.year).seconds.ago }
     end
-
-    trait :requiring_code_owner_approval do
-      merge_requests_require_code_owner_approval true
-    end
   end
 end

ee/spec/features/merge_request/user_sees_approval_widget_spec.rb

@@ -107,7 +107,11 @@ describe 'Merge request > User sees approval widget', :js do
         context 'when code owner approval is required' do
           before do
             stub_licensed_features(code_owner_approval_required: true, multiple_approval_rules: true)
+
             project.update!(merge_requests_require_code_owner_approval: true)
+
+            allow(ProtectedBranch)
+              .to receive(:branch_requires_code_owner_approval?).and_return(true)
           end
 
           it 'shows the code owner rule as required' do

ee/spec/features/projects/merge_requests/user_edits_merge_request_spec.rb

@@ -16,7 +16,6 @@ describe 'Projects > Merge Requests > User edits a merge request' do
 
     let(:project) do
       create(:project, :custom_repo,
-             merge_requests_require_code_owner_approval: true,
              files: { 'docs/CODEOWNERS' => "*.rb @ruby-owner\n*.js @js-owner" })
     end
 
@@ -36,6 +35,10 @@ describe 'Projects > Merge Requests > User edits a merge request' do
                                      message: 'Add a ruby file',
                                      branch_name: 'feature')
 
+      create(:protected_branch,
+        code_owner_approval_required: true,
+        project: project)
+
       # To make sure the rules are created for the merge request, the services
       # that do that aren't triggered from factories
       MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute

ee/spec/features/projects/settings/user_manages_approval_settings_spec.rb

@@ -35,6 +35,12 @@ describe 'EE > Projects > Settings > User manages approval rule settings' do
   context 'when `code_owner_approval_required` is available' do
     let(:licensed_features) { { code_owner_approval_required: true } }
 
+    before do
+      create(:protected_branch,
+        code_owner_approval_required: true,
+        project: project)
+    end
+
     it_behaves_like 'dirty submit form', [{ form: '#js-merge-request-approval-settings', input: '#project_merge_requests_author_approval' }]
 
     it 'allows the user to enforce code owner approval' do

ee/spec/lib/gitlab/usage_data_spec.rb

@@ -195,9 +195,15 @@ describe Gitlab::UsageData do
 
   describe 'code owner approval required' do
     before do
-      create(:project, :archived, :requiring_code_owner_approval)
-      create(:project, :requiring_code_owner_approval, pending_delete: true)
-      create(:project, :requiring_code_owner_approval)
+      create(:protected_branch, code_owner_approval_required: true)
+
+      create(:protected_branch,
+        code_owner_approval_required: true,
+        project: create(:project, :archived))
+
+      create(:protected_branch,
+        code_owner_approval_required: true,
+        project: create(:project, pending_delete: true))
     end
 
     it 'counts the projects actively requiring code owner approval' do

ee/spec/models/approval_wrapped_rule_spec.rb

@@ -196,23 +196,12 @@ describe ApprovalWrappedRule do
               .to receive(:code_owner_approval_required_available?).and_return(true)
           end
 
-          context "when no protected_branches require code owner approval" do
-            it 'returns the correct number of approvals' do
-              allow(subject.project)
-                .to receive(:merge_requests_require_code_owner_approval?).and_return(feature_enabled)
-              allow(subject.project)
-                .to receive(:branch_requires_code_owner_approval?).with(branch.name).and_return(false)
-
-              expect(subject.approvals_required).to eq(expected_required_approvals)
-            end
-          end
-
           context "when the project doesn't require code owner approval on all MRs" do
             it 'returns the expected number of approvals for protected_branches that do require approval' do
               allow(subject.project)
                 .to receive(:merge_requests_require_code_owner_approval?).and_return(false)
-              allow(subject.project)
-                .to receive(:branch_requires_code_owner_approval?).with(branch.name).and_return(feature_enabled)
+              allow(ProtectedBranch)
+                .to receive(:branch_requires_code_owner_approval?).with(subject.project, branch.name).and_return(feature_enabled)
 
               expect(subject.approvals_required).to eq(expected_required_approvals)
             end

ee/spec/models/project_spec.rb

@@ -45,11 +45,15 @@ describe Project do
 
   context 'scopes' do
     describe '.requiring_code_owner_approval' do
+      let!(:project) { create(:project) }
+      let!(:expected_project) { protected_branch_needing_approval.project }
+      let!(:protected_branch_needing_approval) { create(:protected_branch, code_owner_approval_required: true) }
+
       it 'only includes the right projects' do
-        create(:project)
-        expected_project = create(:project, merge_requests_require_code_owner_approval: true)
+        scoped_query_result = described_class.requiring_code_owner_approval
 
-        expect(described_class.requiring_code_owner_approval).to contain_exactly(expected_project)
+        expect(described_class.count).to eq(2)
+        expect(scoped_query_result).to contain_exactly(expected_project)
       end
     end
 
@@ -906,13 +910,17 @@ describe Project do
       true  | true  | true
       false | true  | false
       true  | false | false
-      true  | nil   | false
     end
 
     with_them do
       before do
         stub_licensed_features(code_owner_approval_required: feature_available)
-        project.merge_requests_require_code_owner_approval = feature_enabled
+
+        if feature_enabled
+          create(:protected_branch,
+            project: project,
+            code_owner_approval_required: true)
+        end
       end
 
       it 'requires code owner approval when needed' do

spec/migrations/migrate_code_owner_approval_status_to_protected_branches_in_batches_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190827102026_migrate_code_owner_approval_status_to_protected_branches_in_batches.rb')
+
+describe MigrateCodeOwnerApprovalStatusToProtectedBranchesInBatches, :migration do
+  let(:namespaces)         { table(:namespaces) }
+  let(:projects)           { table(:projects) }
+  let(:protected_branches) { table(:protected_branches) }
+
+  let(:namespace) do
+    namespaces.create!(
+      path: 'gitlab-instance-administrators',
+      name: 'GitLab Instance Administrators'
+    )
+  end
+
+  let(:project) do
+    projects.create!(
+      namespace_id: namespace.id,
+      name: 'GitLab Instance Administration'
+    )
+  end
+
+  let!(:protected_branch_1) do
+    protected_branches.create!(
+      name: "branch name",
+      project_id: project.id
+    )
+  end
+
+  describe '#up' do
+    context "when there's no projects needing approval" do
+      it "doesn't change any protected branch records" do
+        expect { migrate! }
+          .not_to change { ProtectedBranch.where(code_owner_approval_required: true).count }
+      end
+    end
+
+    context "when there's a project needing approval" do
+      let!(:project_needing_approval) do
+        projects.create!(
+          namespace_id: namespace.id,
+          name: 'GitLab Instance Administration',
+          merge_requests_require_code_owner_approval: true
+        )
+      end
+
+      let!(:protected_branch_2) do
+        protected_branches.create!(
+          name: "branch name",
+          project_id: project_needing_approval.id
+        )
+      end
+
+      it "changes N protected branch records" do
+        expect { migrate! }
+          .to change { ProtectedBranch.where(code_owner_approval_required: true).count }
+          .by(1)
+      end
+    end
+  end
+end