Commit 0d47be08 authored by Kerri Miller's avatar Kerri Miller Committed by Rémy Coutable

Move codeowner checks into Commits::CreateService

Iteratively, we've pushed these checks into a number of different
locations, however having a unified, single-location for these checks is
for more sustainable into the future. By moving the existing "web
request" checks into Commits::CreateService, we can remove the existing
checks previously added to the file editor, enable checks on the Commits
REST API (create/update/delete, cherry-pick, revert), and can remove the
:skip_web_ui_code_owner_validations ermergency workaround
parent fc4d4d5f
......@@ -12,6 +12,7 @@ module EE
private
def validate_codeowner_rules
return unless ::Feature.enabled?(:use_legacy_codeowner_validations)
return if params[:file_path].blank?
codeowners_error = codeowners_check_error(project, branch_name, params[:file_path])
......
......@@ -11,6 +11,7 @@ module EE
def validate!
super
validate_against_codeowner_rules!
validate_repository_size!
end
......@@ -21,6 +22,56 @@ module EE
raise_error(size_checker.error_message.commit_error)
end
end
def validate_against_codeowner_rules!
return if ::Feature.enabled?(:use_legacy_codeowner_validations)
codeowners_error = check_against_codeowners(project, params[:branch_name], extracted_paths)
if codeowners_error.present?
raise_error(codeowners_error.tr("\n", " "))
end
end
def check_against_codeowners(project, branch, paths)
return if paths.empty?
::Gitlab::CodeOwners::Validator.new(project, branch, paths).execute
end
def extracted_paths
paths = []
paths << params[:file_path].presence
paths << paths_from_actions
paths << paths_from_start_sha
paths << paths_from_commit(params[:commit])
paths.flatten.compact.uniq
end
def paths_from_actions
return unless params[:actions].present?
params[:actions].flat_map do |entry|
[entry[:file_path], entry[:previous_path]]
end
end
def paths_from_start_sha
return unless params[:start_sha].present?
commit = project.commit(params[:start_sha])
return unless commit
paths_from_commit(commit)
end
def paths_from_commit(commit)
return unless commit.present?
commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }
end
end
end
end
......@@ -31,6 +31,8 @@ module EE
# Issue to remove this feature flag:
# https://gitlab.com/gitlab-org/gitlab/-/issues/217427
def skip_web_ui_code_owner_validations?
return true unless ::Feature.enabled?(:use_legacy_codeowner_validations)
::Feature.enabled?(:skip_web_ui_code_owner_validations, project)
end
......
......@@ -8,9 +8,30 @@ RSpec.describe Projects::BlobController do
let(:project) { create(:project, :public, :repository) }
shared_examples "file matches a codeowners rule" do
# Expected behavior for both these contexts should be equivalent, however
# the feature flag is used in code to control which code path is followed.
#
context ":use_legacy_codeowner_validations is true" do
before do
stub_feature_flags(use_legacy_codeowner_validations: true)
end
it_behaves_like "renders to the expected_view with an error msg"
end
context ":use_legacy_codeowner_validations is false" do
before do
stub_feature_flags(use_legacy_codeowner_validations: false)
end
it_behaves_like "renders to the expected_view with an error msg"
end
end
shared_examples "renders to the expected_view with an error msg" do
let(:error_msg) { "Example error msg" }
it "renders to the edit page with an error msg" do
it "renders to the expected_view with an error msg" do
default_params[:file_path] = "CHANGELOG"
expect_next_instance_of(Gitlab::CodeOwners::Validator) do |validator|
......
......@@ -125,24 +125,42 @@ RSpec.describe Gitlab::Checks::DiffCheck do
expect(subject).to receive(:updated_from_web?).and_return(true)
end
context "when skip_web_ui_code_owner_validations is disabled" do
context "when use_legacy_codeowner_validations is enabled" do
before do
stub_feature_flags(skip_web_ui_code_owner_validations: false)
allow(project).to receive(:branch_requires_code_owner_approval?)
.once.and_return(true)
stub_feature_flags(use_legacy_codeowner_validations: 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
context "when skip_web_ui_code_owner_validations is disabled" do
before do
stub_feature_flags(skip_web_ui_code_owner_validations: false)
allow(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
context "when skip_web_ui_code_owner_validations is enabled" do
before do
stub_feature_flags(skip_web_ui_code_owner_validations: true)
expect(project).not_to receive(:branch_requires_code_owner_approval?)
end
it "returns an empty array" do
expect(subject.send(:path_validations)).to eq([])
end
end
end
end
context "when skip_web_ui_code_owner_validations is enabled" do
context "when use_legacy_codeowner_validations is disabled" do
before do
stub_feature_flags(skip_web_ui_code_owner_validations: true)
stub_feature_flags(use_legacy_codeowner_validations: false)
expect(project).not_to receive(:branch_requires_code_owner_approval?)
end
......
This diff is collapsed.
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment