Merge branch 'bvl-remove-multiple-codeowner-rules-flag' into 'master'

Remove `multiple_code_owner_rules` feature flag

Closes #9623

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

ee/app/models/approval_merge_request_rule.rb

@@ -4,8 +4,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
   include Gitlab::Utils::StrongMemoize
   include ApprovalRuleLike
 
-  DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner'
-
   scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
   scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
 

ee/app/models/ee/merge_request.rb

@@ -107,12 +107,6 @@ module EE
       end
     end
 
-    def code_owners
-      strong_memoize(:code_owners) do
-        ::Gitlab::CodeOwners.for_merge_request(self).freeze
-      end
-    end
-
     def has_license_management_reports?
       actual_head_pipeline&.has_reports?(::Ci::JobArtifact.license_management_reports)
     end
@@ -136,25 +130,5 @@ module EE
 
       compare_reports(::Ci::CompareMetricsReportsService)
     end
-
-    def sync_code_owners_with_approvers
-      return if merged?
-
-      owners = code_owners
-
-      if owners.present?
-        ApplicationRecord.transaction do
-          rule = approval_rules.code_owner.first
-          rule ||= ApprovalMergeRequestRule.find_or_create_code_owner_rule(
-            self,
-            ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER
-          )
-
-          rule.users = owners.uniq
-        end
-      else
-        approval_rules.code_owner.delete_all
-      end
-    end
   end
 end

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -48,25 +48,6 @@ module EE
 
         results
       end
-
-      def create_approvers(merge_request, users)
-        return if users.empty?
-        return unless merge_request.approvers_overwritten?
-
-        now = Time.now
-
-        rows = users.map do |user|
-          {
-            target_id: merge_request.id,
-            target_type: merge_request.class.name,
-            user_id: user.id,
-            created_at: now,
-            updated_at: now
-          }
-        end
-
-        ::Gitlab::Database.bulk_insert(Approver.table_name, rows)
-      end
     end
   end
 end

ee/app/services/merge_requests/sync_code_owner_approval_rules.rb

@@ -8,18 +8,6 @@ module MergeRequests
     end
 
     def execute
-      if ::Feature.enabled?(:multiple_code_owner_rules, default_enabled: true)
-        sync_rules
-      else
-        merge_request.sync_code_owners_with_approvers
-      end
-    end
-
-    private
-
-    attr_reader :merge_request, :previous_diff
-
-    def sync_rules
       return if merge_request.merged?
 
       delete_outdated_code_owner_rules
@@ -35,6 +23,10 @@ module MergeRequests
       end
     end
 
+    private
+
+    attr_reader :merge_request, :previous_diff
+
     def create_rule(entry)
       ApprovalMergeRequestRule.find_or_create_code_owner_rule(merge_request, entry.pattern)
     end

ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb

@@ -99,7 +99,9 @@ module Gitlab
           return if state == 'merged' || state == 'closed'
 
           Gitlab::GitalyClient.allow_n_plus_1_calls do
-            owners = ::MergeRequest.find(id).code_owners
+            gl_merge_request = ::MergeRequest.find(id)
+            owners = Gitlab::CodeOwners.entries_for_merge_request(gl_merge_request)
+                       .flat_map(&:users).uniq
 
             if owners.present?
               ApplicationRecord.transaction do

ee/lib/gitlab/code_owners.rb

@@ -18,14 +18,9 @@ module Gitlab
     #   Find code owners entries at a particular MergeRequestDiff.
     #   Assumed to be the most recent one if not provided.
     def self.entries_for_merge_request(merge_request, merge_request_diff: nil)
-      loader_for_merge_request(merge_request, merge_request_diff)&.entries || []
-    end
+      return [] unless merge_request.project.feature_available?(:code_owners)
 
-    def self.for_merge_request(merge_request, merge_request_diff: nil)
-      loader = loader_for_merge_request(merge_request, merge_request_diff)
-      return [] unless loader
-
-      loader.members.reject { |owner| owner == merge_request.author }
+      loader_for_merge_request(merge_request, merge_request_diff)&.entries || []
     end
 
     def self.loader_for_merge_request(merge_request, merge_request_diff)

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

@@ -186,7 +186,8 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
         let(:owners) { create_list(:user, 2) }
 
         before do
-          allow(::Gitlab::CodeOwners).to receive(:for_merge_request).and_return(owners)
+          entry = double('code owner entry', users: owners)
+          allow(::Gitlab::CodeOwners).to receive(:entries_for_merge_request).and_return([entry])
         end
 
         context 'when merge request is merged' do

ee/spec/lib/gitlab/code_owners_spec.rb

@@ -7,7 +7,7 @@ describe Gitlab::CodeOwners do
 
   let!(:code_owner) { create(:user, username: 'owner-1') }
   let(:project) { create(:project, :repository) }
-  let(:codeowner_content) { "docs/CODEOWNERS @owner-1" }
+  let(:codeowner_content) { 'docs/CODEOWNERS @owner-1' }
   let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
   let(:codeowner_blob_ref) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
 
@@ -82,59 +82,16 @@ describe Gitlab::CodeOwners do
         end
       end
     end
-  end
-
-  describe '.for_merge_request' do
-    let(:codeowner_lookup_ref) { merge_request.target_branch }
-    let(:merge_request) do
-      build(
-        :merge_request,
-        source_project: project,
-        source_branch: 'feature',
-        target_project: project,
-        target_branch: 'with-codeowners'
-      )
-    end
-
-    context 'when the feature is available' do
-      before do
-        stub_licensed_features(code_owners: true)
-      end
-
-      it 'returns owners for merge request' do
-        expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: nil).and_return(['docs/CODEOWNERS'])
-
-        expect(described_class.for_merge_request(merge_request)).to eq([code_owner])
-      end
-
-      context 'when owner is merge request author' do
-        let(:merge_request) { build(:merge_request, target_project: project, author: code_owner) }
-
-        it 'excludes author' do
-          expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: nil).and_return(['docs/CODEOWNERS'])
-
-          expect(described_class.for_merge_request(merge_request)).to eq([])
-        end
-      end
-
-      context 'when merge_request_diff is specified' do
-        let(:merge_request_diff) { double(:merge_request_diff) }
-
-        it 'returns owners at the specified ref' do
-          expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: merge_request_diff).and_return(['docs/CODEOWNERS'])
-
-          expect(described_class.for_merge_request(merge_request, merge_request_diff: merge_request_diff)).to eq([code_owner])
-        end
-      end
-    end
 
     context 'when the feature is not available' do
       before do
         stub_licensed_features(code_owners: false)
       end
 
-      it 'returns no users' do
-        expect(described_class.for_merge_request(merge_request)).to eq([])
+      it 'skips reading codeowners and returns an empty array' do
+        expect(merge_request).not_to receive(:modified_paths)
+
+        expect(described_class.entries_for_merge_request(merge_request)).to eq([])
       end
     end
   end

ee/spec/models/merge_request_spec.rb

+# frozen_string_literal: true
+
 require 'spec_helper'
 
 # Store feature-specific specs in `ee/spec/models/merge_request instead of
@@ -186,18 +188,6 @@ describe MergeRequest do
     end
   end
 
-  describe '#code_owners' do
-    subject(:merge_request) { build(:merge_request) }
-    let(:owners) { [double(:owner)] }
-
-    it 'returns code owners, frozen' do
-      allow(::Gitlab::CodeOwners).to receive(:for_merge_request).with(subject).and_return(owners)
-
-      expect(subject.code_owners).to eq(owners)
-      expect(subject.code_owners).to be_frozen
-    end
-  end
-
   describe '#approvals_before_merge' do
     where(:license_value, :db_value, :expected) do
       true  | 5   | 5
@@ -219,54 +209,6 @@ describe MergeRequest do
     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) { create(:code_owner_rule, merge_request: subject, 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 '#has_license_management_reports?' do
     subject { merge_request.has_license_management_reports? }
     let(:project) { create(:project, :repository) }

ee/spec/services/ee/merge_requests/create_service_spec.rb

@@ -24,33 +24,14 @@ describe MergeRequests::CreateService do
   end
 
   describe '#execute' do
-    context 'code owners' do
-      it 'refreshes code owners for the merge request' do
-        fake_refresh_service = instance_double(::MergeRequests::SyncCodeOwnerApprovalRules)
+    it 'refreshes code owners for the merge request' do
+      fake_refresh_service = instance_double(::MergeRequests::SyncCodeOwnerApprovalRules)
 
-        expect(::MergeRequests::SyncCodeOwnerApprovalRules)
-          .to receive(:new).with(kind_of(MergeRequest)).and_return(fake_refresh_service)
-        expect(fake_refresh_service).to receive(:execute)
+      expect(::MergeRequests::SyncCodeOwnerApprovalRules)
+        .to receive(:new).with(kind_of(MergeRequest)).and_return(fake_refresh_service)
+      expect(fake_refresh_service).to receive(:execute)
 
-        service.execute
-      end
-
-      context 'when multiple code owner rules is disabled' do
-        let!(:owners) { create_list(:user, 2) }
-
-        before do
-          stub_feature_flags(multiple_code_owner_rules: false)
-          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
+      service.execute
     end
 
     it_behaves_like 'new issuable with scoped labels' do

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -87,34 +87,6 @@ describe MergeRequests::RefreshService do
 
           subject
         end
-
-        context 'when multiple code owner rules are disabled' do
-          let(:new_owners) { [owner] }
-
-          before do
-            stub_feature_flags(multiple_code_owner_rules: false)
-            relevant_merge_requests.each do |merge_request|
-              expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
-            end
-
-            [forked_merge_request].each do |merge_request|
-              expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request)
-            end
-          end
-
-          it 'triggers syncing of code owners' do
-            relevant_merge_requests.each do |merge_request|
-              expect(merge_request.approval_rules.code_owner.exists?).to eq(false)
-            end
-
-            subject
-
-            relevant_merge_requests.each do |merge_request|
-              code_owner_rule = merge_request.approval_rules.code_owner.first
-              expect(code_owner_rule.users).to eq(new_owners)
-            end
-          end
-        end
       end
     end
 
@@ -146,7 +118,7 @@ describe MergeRequests::RefreshService do
         expect(merge_request.all_pipelines.last).to be_merge_request_pipeline
       end
 
-      context "when MergeRequestUpdateWorker is retried by an exception" do
+      context 'when MergeRequestUpdateWorker is retried by an exception' do
         it 'does not re-create a duplicate merge request pipeline' do
           expect do
             service.execute(oldrev, newrev, "refs/heads/#{source_branch}")

ee/spec/services/merge_requests/sync_code_owner_approval_rules_spec.rb

@@ -54,17 +54,5 @@ describe MergeRequests::SyncCodeOwnerApprovalRules do
 
       expect(other_rule.reload.users).to eq(rb_owners)
     end
-
-    context 'when multiple code owner rules are disabled' do
-      before do
-        stub_feature_flags(multiple_code_owner_rules: false)
-      end
-
-      it 'calls the old sync method' do
-        expect(merge_request).to receive(:sync_code_owners_with_approvers)
-
-        service.execute
-      end
-    end
   end
 end