diff --git a/doc/user/project/merge_requests/approvals/index.md b/doc/user/project/merge_requests/approvals/index.md
index 9988cdcddd0613a8e334b4efddd3451a5fc8883d..3a39ff4f5f79ebcba83ddf331d14da820e83327a 100644
--- a/doc/user/project/merge_requests/approvals/index.md
+++ b/doc/user/project/merge_requests/approvals/index.md
@@ -119,12 +119,13 @@ Whenever an approval rule cannot be satisfied, the rule is displayed as `(!) Aut
- The only eligible approver is the author of the merge request.
- No eligible approvers (either groups or users) have been assigned to the approval rule.
+- The number of required approvals is more than the number of eligible approvers.
-These rules will be automatically approved (fail-open state) to unblock their respective merge requests,
+These rules are automatically approved to unblock their respective merge requests,
unless they were created through a security policy.
Invalid approval rules created through a security policy are presented with `(!) Action Required`
-and are not automatically approved (fail-closed state), blocking their respective merge requests.
+and are not automatically approved, blocking their respective merge requests.
## Related topics
diff --git a/ee/app/graphql/types/merge_requests/approval_state_type.rb b/ee/app/graphql/types/merge_requests/approval_state_type.rb
index c76dda8073b13597bb7bde43b34030a2b3d5cb68..c5dc8d9cfe99fe237079c58fbf21a33351b13de5 100644
--- a/ee/app/graphql/types/merge_requests/approval_state_type.rb
+++ b/ee/app/graphql/types/merge_requests/approval_state_type.rb
@@ -18,6 +18,7 @@ class ApprovalStateType < BaseObject
field :invalid_approvers_rules,
[::Types::ApprovalRuleType],
null: true,
+ calls_gitaly: true,
description: 'List of approval rules that are associated with the merge request, but invalid.'
field :suggested_approvers, Types::UserType.connection_type,
diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb
index 9136b9c2f2d0567ea78e380d779d23581c7a1256..172afff51d82d9c02589da12c2f6a1e7216df67c 100644
--- a/ee/app/models/approval_state.rb
+++ b/ee/app/models/approval_state.rb
@@ -198,12 +198,12 @@ def invalid_approvers_rules
strong_memoize(:invalid_approvers_rules) do
wrapped_approval_rules.select do |rule|
next if rule.any_approver?
- next unless rule.approvers.empty?
+ next if rule.approvers.any? && rule.approvers.size >= rule.approvals_required
if rule.code_owner?
rule.branch_requires_code_owner_approval?
else
- rule.approvals_required > 0
+ rule.approvals_required > rule.approvers.size
end
end
end
diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb
index b553f8440ae689fc5396249019b5ad7ce9a305ff..01ca90195af428c596fd9d2f314db051ad68dda1 100644
--- a/ee/app/models/approval_wrapped_rule.rb
+++ b/ee/app/models/approval_wrapped_rule.rb
@@ -91,7 +91,7 @@ def approved?
end
def invalid_rule?
- !any_approver? && approvals_required > 0 && unactioned_approvers.size <= 0
+ !any_approver? && approvals_required > approvers.size
end
def allow_merge_when_invalid?
diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb
index 2e9a60c62f255697ab1f3c6820a024e8098191bf..d24ff78d4c50eded034be626d7e2b8bbabb2f34a 100644
--- a/ee/spec/models/approval_state_spec.rb
+++ b/ee/spec/models/approval_state_spec.rb
@@ -1600,8 +1600,18 @@ def create_rules
end
end
- context 'when invalid' do
- let!(:rule) { invalid_rule }
+ context 'when invalid due to no approvers' do
+ let!(:rule) { invalid_no_approvers_rule }
+
+ it 'returns the rule' do
+ approval_rules = subject.map(&:approval_rule)
+
+ expect(approval_rules).to include(rule)
+ end
+ end
+
+ context 'when invalid due to not enough approvers' do
+ let!(:rule) { invalid_not_enough_approvers_rule }
it 'returns the rule' do
approval_rules = subject.map(&:approval_rule)
@@ -1613,7 +1623,10 @@ def create_rules
context 'when the rule is code owner' do
let(:valid_rule) { create(:code_owner_rule, merge_request: merge_request, users: create_list(:user, 1)) }
- let(:invalid_rule) { create(:code_owner_rule, merge_request: merge_request) }
+ let(:invalid_no_approvers_rule) { create(:code_owner_rule, merge_request: merge_request) }
+ let(:invalid_not_enough_approvers_rule) do
+ create(:code_owner_rule, merge_request: merge_request, users: create_list(:user, 1), approvals_required: 2)
+ end
before do
stub_licensed_features(code_owner_approval_required: true)
@@ -1659,7 +1672,10 @@ def create_rules
context 'when the rule is approval_merge_request_rule' do
let(:valid_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: create_list(:user, 1)) }
- let(:invalid_rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1) }
+ let(:invalid_no_approvers_rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1) }
+ let(:invalid_not_enough_approvers_rule) do
+ create(:approval_merge_request_rule, merge_request: merge_request, users: create_list(:user, 1), approvals_required: 2)
+ end
context 'when approvals required is 0' do
let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
@@ -1676,7 +1692,10 @@ def create_rules
context 'when the rule is report_approver' do
let(:valid_rule) { create(:report_approver_rule, merge_request: merge_request, users: create_list(:user, 1)) }
- let(:invalid_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 1) }
+ let(:invalid_no_approvers_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 1) }
+ let(:invalid_not_enough_approvers_rule) do
+ create(:report_approver_rule, merge_request: merge_request, users: create_list(:user, 1), approvals_required: 2)
+ end
context 'when approvals required is 0' do
let!(:rule) { create(:report_approver_rule, merge_request: merge_request) }
diff --git a/ee/spec/models/approval_wrapped_rule_spec.rb b/ee/spec/models/approval_wrapped_rule_spec.rb
index 3e469ea5ccb3b4ee69c4b18d687f5b916b71f8a4..2dc5d363a22db5a6f99a3e53793484ba15c74e99 100644
--- a/ee/spec/models/approval_wrapped_rule_spec.rb
+++ b/ee/spec/models/approval_wrapped_rule_spec.rb
@@ -64,7 +64,7 @@
end
context 'when approvals left is not zero, but there is still unactioned approvers' do
- let(:approvals_required) { 99 }
+ let(:approvals_required) { 2 }
before do
rule.users << approver2
@@ -82,6 +82,18 @@
expect(subject.approved?).to eq(true)
end
end
+
+ context 'when approvals left is not zero, but there is not enough unactioned approvers' do
+ let(:approvals_required) { 99 }
+
+ before do
+ rule.users << approver2
+ end
+
+ it 'returns true' do
+ expect(subject.approved?).to eq(true)
+ end
+ end
end
describe '#invalid_rule?' do
@@ -101,6 +113,18 @@
end
end
+ context 'when more approvals are required than the number of approvers' do
+ let(:approvals_required) { 2 }
+
+ before do
+ rule.users << approver1
+ end
+
+ it 'returns true' do
+ expect(subject.invalid_rule?).to eq(true)
+ end
+ end
+
context 'when there are unactioned approvers and approvals are required' do
let(:approvals_required) { 1 }
@@ -113,6 +137,32 @@
end
end
+ context 'when there are no unactioned approvers because all required approvals are given' do
+ let(:approvals_required) { 1 }
+
+ before do
+ create(:approval, merge_request: merge_request, user: approver1)
+ rule.users << approver1
+ end
+
+ it 'returns false' do
+ expect(subject.invalid_rule?).to eq(false)
+ end
+ end
+
+ context 'when there are more approvers than required approvals' do
+ let(:approvals_required) { 1 }
+
+ before do
+ rule.users << approver1
+ rule.users << approver2
+ end
+
+ it 'returns false' do
+ expect(subject.invalid_rule?).to eq(false)
+ end
+ end
+
context 'when no approvals are required' do
let(:approvals_required) { 0 }
diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb
index 25b98f0700aefdd130905b1bc1a4aae12a2d3b32..dbd19c57d8daccc9f9dc3878085852aa3b044fd8 100644
--- a/ee/spec/requests/api/merge_request_approvals_spec.rb
+++ b/ee/spec/requests/api/merge_request_approvals_spec.rb
@@ -84,6 +84,7 @@
project.add_developer(approver)
project.add_developer(create(:user))
rule.users << approver
+ rule.users << create(:user)
rule.groups << group
get api(path, approver)