From e90fe1f99b54557ecc5588bedf13c6ecab5ec1af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20=C4=8Cavoj?= <mcavoj@gitlab.com>
Date: Sat, 22 Apr 2023 08:45:05 +0000
Subject: [PATCH] Set approval rule as invalid if there are not enough
approvers
A rule is already marked as invalid if there are no approvers.
This change also marks it as invalid if there are some approvers,
but less than the number of required approvals.
Changelog: changed
EE: true
---
.../project/merge_requests/approvals/index.md | 5 +-
.../merge_requests/approval_state_type.rb | 1 +
ee/app/models/approval_state.rb | 4 +-
ee/app/models/approval_wrapped_rule.rb | 2 +-
ee/spec/models/approval_state_spec.rb | 29 +++++++++--
ee/spec/models/approval_wrapped_rule_spec.rb | 52 ++++++++++++++++++-
.../api/merge_request_approvals_spec.rb | 1 +
7 files changed, 83 insertions(+), 11 deletions(-)
diff --git a/doc/user/project/merge_requests/approvals/index.md b/doc/user/project/merge_requests/approvals/index.md
index 9988cdcddd061..3a39ff4f5f79e 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 c76dda8073b13..c5dc8d9cfe99f 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 9136b9c2f2d05..172afff51d82d 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 b553f8440ae68..01ca90195af42 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 2e9a60c62f255..d24ff78d4c50e 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 3e469ea5ccb3b..2dc5d363a22db 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 25b98f0700aef..dbd19c57d8dac 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)
--
GitLab