Merge branch 'ac-revert-codeowners-fix' into 'master'

Revert CODEOWNERS fixes

See merge request gitlab-org/gitlab!31087

changelogs/unreleased/sh-revert-codeowners-check.yml

+---
+title: Revert CODEOWNERS validation of Web requests in diff check
+merge_request: 31087
+author:
+type: fixed

ee/lib/ee/gitlab/checks/diff_check.rb

@@ -12,7 +12,7 @@ module EE
         def path_validations
           validations = [super].flatten
 
-          if project.branch_requires_code_owner_approval?(branch_name)
+          if !updated_from_web? && project.branch_requires_code_owner_approval?(branch_name)
             validations << validate_code_owners
           end
 
@@ -34,13 +34,11 @@ module EE
           matched_rules = loader.entries.collect { |e| "- #{e.pattern}" }
           code_owner_path = project.repository.code_owners_blob(ref: branch_name).path || "CODEOWNERS"
 
-          msg = "Pushes to protected branches that contain changes to files that\n" \
+          "Pushes to protected branches that contain changes to files that\n" \
             "match patterns defined in `#{code_owner_path}` are disabled for\n" \
             "this project. Please submit these changes via a merge request.\n\n" \
             "The following pattern(s) from `#{code_owner_path}` were matched:\n" \
             "#{matched_rules.join('\n')}\n"
-
-          updated_from_web? ? msg.tr("\n", " ") : msg
         end
 
         def validate_path_locks?

ee/lib/gitlab/code_owners/groups_loader.rb

@@ -23,15 +23,7 @@ module Gitlab
         return Group.none if extractor.names.empty?
 
         groups = project.invited_groups.where_full_path_in(extractor.names)
-        group_list = groups.with_route.with_users.to_a
-
-        if extractor.names.include?(project.group&.full_path)
-          project.group.users.load
-
-          group_list << project.group
-        end
-
-        group_list
+        groups.with_route.with_users
       end
     end
   end

ee/spec/lib/gitlab/checks/diff_check_spec.rb

@@ -39,8 +39,6 @@ describe Gitlab::Checks::DiffCheck do
         allow(project.repository).to receive(:code_owners_blob)
           .with(ref: codeowner_lookup_ref)
           .and_return(codeowner_blob)
-
-        stub_feature_flags(sectional_codeowners: false)
       end
 
       context 'the MR contains a renamed file matching a file path' do
@@ -58,6 +56,10 @@ describe Gitlab::Checks::DiffCheck do
         end
 
         context 'and the user is not listed as a codeowner' do
+          before do
+            stub_feature_flags(sectional_codeowners: false)
+          end
+
           it "returns an error message" do
             expect { diff_check.validate! }.to raise_error do |error|
               expect(error).to be_a(Gitlab::GitAccess::ForbiddenError)
@@ -82,42 +84,16 @@ describe Gitlab::Checks::DiffCheck do
           subject.send(:validate_code_owners).call(["docs/CODEOWNERS", "README"])
         end
 
-        shared_examples_for "returns an error message" do
-          it "returns the expected error message" do
-            expect(validation_result).to include("Pushes to protected branches")
-          end
-        end
-
         context "and the user is not listed as a code owner" do
           before do
             stub_feature_flags(sectional_codeowners: false)
           end
 
-          context "for a non-web-based request" do
-            it_behaves_like "returns an error message"
-
-            it "returns an error message with newline chars" do
-              expect(validation_result).to include("\n")
-            end
-          end
-
-          context "for a web-based request" do
-            before do
-              expect(subject).to receive(:updated_from_web?).and_return(true)
-            end
-
-            it_behaves_like "returns an error message"
-
-            it "returns an error message with newline chars removed" do
-              expect(validation_result).not_to include("\n")
-            end
+          it "returns an error message" do
+            expect(validation_result).to include("Pushes to protected branches")
           end
         end
 
-        context "and the user is not listed as a code owner" do
-          it_behaves_like "returns an error message"
-        end
-
         context "and the user is listed as a code owner" do
           # `user` is set as the owner of the incoming change by the shared
           #   context found in 'push rules checks context'
@@ -127,32 +103,6 @@ describe Gitlab::Checks::DiffCheck do
             expect(validation_result).to be_nil
           end
         end
-
-        context "when the codeowner entity is a group" do
-          let(:group_a) { create(:group) }
-          let(:project) { create(:project, :repository, namespace: group_a) }
-          let(:codeowner_content) do
-            <<~CODEOWNERS
-            *.rb @#{code_owner.username}
-            docs/CODEOWNERS @#{group_a.name}
-            *.js.coffee @#{group_a.name}
-            CODEOWNERS
-          end
-
-          context "and the user is part of the codeowning-group" do
-            before do
-              group_a.add_developer(user)
-            end
-
-            it "returns nil" do
-              expect(validation_result).to be_nil
-            end
-          end
-
-          context "and the user is not part of the codeowning-group" do
-            it_behaves_like "returns an error message"
-          end
-        end
       end
 
       context "the MR doesn't contain a matching file path" do
@@ -178,16 +128,30 @@ describe Gitlab::Checks::DiffCheck do
       end
 
       context "when the feature is enabled on the project" do
-        before do
-          expect(project).to receive(:branch_requires_code_owner_approval?)
-            .once.and_return(true)
+        context "updated_from_web? == false" do
+          before do
+            expect(subject).to receive(:updated_from_web?).and_return(false)
+            expect(project).to receive(:branch_requires_code_owner_approval?)
+              .once.and_return(true)
+          end
+
+          it "returns an array of Proc(s)" do
+            validations = subject.send(:path_validations)
+
+            expect(validations.any?).to be_truthy
+            expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
+          end
         end
 
-        it "returns an array of Proc(s)" do
-          validations = subject.send(:path_validations)
+        context "updated_from_web? == true" do
+          before do
+            expect(subject).to receive(:updated_from_web?).and_return(true)
+            expect(project).not_to receive(:branch_requires_code_owner_approval?)
+          end
 
-          expect(validations.any?).to be_truthy
-          expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
+          it "returns an empty array" do
+            expect(subject.send(:path_validations)).to eq([])
+          end
         end
       end
     end

ee/spec/lib/gitlab/code_owners/groups_loader_spec.rb

@@ -51,18 +51,7 @@ describe Gitlab::CodeOwners::GroupsLoader do
         group = create(:group, path: "GROUP-1")
         create(:group, path: "GROUP-2")
         project.invited_groups << group
-        load_groups
-
-        expect(entry).to have_received(:add_matching_groups_from).with([group])
-      end
-    end
-
-    context "input matches project.group" do
-      let(:group) { create(:group) }
-      let(:project) { create(:project, :repository, namespace: group) }
-      let(:text) { "@#{project.group.name}" }
 
-      it "returns the project's group" do
         load_groups
 
         expect(entry).to have_received(:add_matching_groups_from).with([group])

ee/spec/lib/gitlab/code_owners/loader_spec.rb

@@ -71,7 +71,7 @@ describe Gitlab::CodeOwners::Loader do
         expect(loader.entries).to contain_exactly(expected_entry, other_entry)
       end
 
-      it 'only performs 4 queries for users and groups' do
+      it 'only performs 3 query for users and groups' do
         test_group = create(:group, path: 'test-group')
         test_group.add_developer(create(:user))
 
@@ -80,12 +80,9 @@ describe Gitlab::CodeOwners::Loader do
 
         project.invited_groups << [test_group, another_group]
 
-        # - 1 query for users
-        # - 1 for the emails to later divide them across the entries
-        # - 1 for groups with joined routes and users
-        # - 1 for loading the users for the parent group
-        #
-        expect { loader.entries }.not_to exceed_query_limit(4)
+        # One query for users, one for the emails to later divide them across the entries
+        # one for groups with joined routes and users
+        expect { loader.entries }.not_to exceed_query_limit(3)
       end
     end