Skip to content

G
gitlab-ce
Commit 35015840 authored by Mark Chao's avatar Mark Chao
Browse files
Options

Merge ApprovableForRule to Approvable 

Merge VisibleApprovableForRule to VisibleApprovable
parent 7dc7bfa9
Hide whitespace changes
Inline Side-by-side
Showing with 146 additions and 684 deletions
ee/app/models/concerns/approvable.rb
View file @ 35015840
...@@ -7,6 +7,20 @@ module Approvable ...@@ -7,6 +7,20 @@ module Approvable
# such as approver_groups and target_project in presenters # such as approver_groups and target_project in presenters
include ::VisibleApprovable include ::VisibleApprovable
FORWARDABLE_METHODS = %i{
approval_needed?
approved?
approvals_left
can_approve?
has_approved?
any_approver_allowed?
authors_can_approve?
committers_can_approve?
approvers_overwritten?
}.freeze
delegate(*FORWARDABLE_METHODS, to: :approval_state)
def approval_feature_available? def approval_feature_available?
strong_memoize(:approval_feature_available) do strong_memoize(:approval_feature_available) do
if project if project
...@@ -21,30 +35,10 @@ module Approvable ...@@ -21,30 +35,10 @@ module Approvable
@approval_state ||= ApprovalState.new(self) @approval_state ||= ApprovalState.new(self)
end end
def approval_needed?
approvals_required&.nonzero?
end
def approved?
approvals_left < 1
end
def approvals_given def approvals_given
approvals.size approvals.size
end end
# Number of approvals remaining (excluding existing approvals) before the MR is
# considered approved. If there are fewer potential approvers than approvals left,
# users should either reduce the number of approvers on projects and/or merge
# requests settings and/or allow MR authors to approve their own merge
# requests (in case only one approval is needed).
#
def approvals_left
approvals_left_count = approvals_required - approvals.size
[approvals_left_count, 0].max
end
def approvals_required def approvals_required
[approvals_before_merge.to_i, target_project.approvals_before_merge.to_i].max [approvals_before_merge.to_i, target_project.approvals_before_merge.to_i].max
end end
...@@ -55,58 +49,6 @@ module Approvable ...@@ -55,58 +49,6 @@ module Approvable
super super
end end
# Even though this method is used in VisibleApprovable
# we do not want to encounter a scenario where there are
# no user approvers but there is one merge request group
# approver that is not visible to the current_user,
# which would make this method return false
# when it should still report as overwritten.
# To prevent this from happening, approvers_overwritten?
# makes use of the unfiltered version of approver_groups so that
# it always takes into account every approver
# available
#
def approvers_overwritten?
approvers.to_a.any? || approver_groups.to_a.any?
end
def can_approve?(user)
return false unless user
# The check below considers authors and committers being able to approve the MR.
# That is, they're included/excluded from that list accordingly.
return true if approvers_left.include?(user)
# We can safely unauthorize authors and committers if it reaches this guard clause.
return false if author == user
return false if committers.include?(user)
return false unless user.can?(:update_merge_request, self)
any_approver_allowed? && approvals.where(user: user).empty?
end
def has_approved?(user)
return false unless user
approved_by_users.include?(user)
end
# Once there are fewer approvers left in the list than approvals required or
# there are no more approvals required
# allow other project members to approve the MR.
#
def any_approver_allowed?
remaining_approvals = approvals_left
remaining_approvals.zero? || remaining_approvals > approvers_left.count
end
def authors_can_approve?
target_project.merge_requests_author_approval?
end
def committers_can_approve?
!target_project.merge_requests_disable_committers_approval?
end
def approver_ids=(value) def approver_ids=(value)
::Gitlab::Utils.ensure_array_from_string(value).each do |user_id| ::Gitlab::Utils.ensure_array_from_string(value).each do |user_id|
next if author && user_id == author.id next if author && user_id == author.id
......
ee/app/models/concerns/approvable_for_rule.rb deleted 100644 → 0
View file @ 7dc7bfa9
# frozen_string_literal: true
module ApprovableForRule
include VisibleApprovableForRule
FORWARDABLE_METHODS = %w{
approval_needed?
approved?
approvals_left
can_approve?
has_approved?
any_approver_allowed?
authors_can_approve?
committers_can_approve?
approvers_overwritten?
}.freeze
FORWARDABLE_METHODS.each do |method|
define_method(method) do |*args|
return super(*args) if approval_rules_disabled?
approval_state.public_send(method, *args) # rubocop:disable GitlabSecurity/PublicSend
end
end
end
ee/app/models/concerns/visible_approvable.rb
View file @ 35015840
...@@ -8,9 +8,7 @@ module VisibleApprovable ...@@ -8,9 +8,7 @@ module VisibleApprovable
# Users in the list of approvers who have not already approved this MR. # Users in the list of approvers who have not already approved this MR.
# #
def approvers_left def approvers_left
strong_memoize(:approvers_left) do approval_state.unactioned_approvers
User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approved_by_users.select(:id))
end
end end
# The list of approvers from either this MR (if they've been set on the MR) or the # The list of approvers from either this MR (if they've been set on the MR) or the
...@@ -22,58 +20,21 @@ module VisibleApprovable ...@@ -22,58 +20,21 @@ module VisibleApprovable
# #
# @return [Array<User>] # @return [Array<User>]
def overall_approvers(exclude_code_owners: false) def overall_approvers(exclude_code_owners: false)
code_owners = [] if exclude_code_owners options = { target: :users }
options[:code_owner] = false if exclude_code_owners
if approvers_overwritten? approvers = approval_state.filtered_approvers(options)
code_owners ||= [] # already persisted into database, no need to recompute approvers.uniq!
approvers_relation = approver_users approvers
else
code_owners ||= self.code_owners.dup
approvers_relation = target_project.approver_users
end
approvers_relation = project.members_among(approvers_relation)
if author && !authors_can_approve?
approvers_relation = approvers_relation.where.not(id: author.id)
end
if committers.any? && !committers_can_approve?
approvers_relation = approvers_relation.where.not(id: committers.select(:id))
end
results = code_owners.concat(approvers_relation)
results.uniq!
results
end
def overall_approver_groups
approvers_overwritten? ? approver_groups : target_project.approver_groups
end end
def all_approvers_including_groups def all_approvers_including_groups
strong_memoize(:all_approvers_including_groups) do approval_state.approvers
approvers = []
# Approvers not sourced from group level
approvers << overall_approvers
approvers << approvers_from_groups
approvers.flatten
end
end end
def approvers_from_groups def approvers_from_groups
group_approvers = overall_approver_groups.flat_map(&:users) groups = approval_state.wrapped_approval_rules.flat_map(&:groups)
group_approvers.delete(author) unless authors_can_approve? User.joins(:group_members).where(members: { source_id: groups })
unless committers_can_approve?
committer_approver_ids = committers.where(id: group_approvers.map(&:id)).pluck(:id)
group_approvers.reject! { |user| committer_approver_ids.include?(user.id) }
end
group_approvers
end end
def reset_approval_cache! def reset_approval_cache!
......
ee/app/models/concerns/visible_approvable_for_rule.rb deleted 100644 → 0
View file @ 7dc7bfa9
# frozen_string_literal: true
# This module may only be used by presenters or modules
# that include the Approvable concern
module VisibleApprovableForRule
def approvers_left
return super if approval_rules_disabled?
approval_state.unactioned_approvers
end
def overall_approvers(exclude_code_owners: false)
return super if approval_rules_disabled?
options = { target: :users }
options[:code_owner] = false if exclude_code_owners
approvers = approval_state.filtered_approvers(options)
approvers.uniq!
approvers
end
def all_approvers_including_groups
return super if approval_rules_disabled?
approval_state.approvers
end
def approvers_from_groups
return super if approval_rules_disabled?
groups = approval_state.wrapped_approval_rules.flat_map(&:groups)
User.joins(:group_members).where(members: { source_id: groups })
end
def approval_rules_disabled?
::Feature.disabled?(:approval_rules, project, default_enabled: true)
end
end
ee/app/models/ee/merge_request.rb
View file @ 35015840
...@@ -8,7 +8,6 @@ module EE ...@@ -8,7 +8,6 @@ module EE
include ::Approvable include ::Approvable
include ::Gitlab::Utils::StrongMemoize include ::Gitlab::Utils::StrongMemoize
include FromUnion include FromUnion
prepend ApprovableForRule
prepended do prepended do
include Elastic::MergeRequestsSearch include Elastic::MergeRequestsSearch
......
ee/app/presenters/ee/merge_request_presenter.rb
View file @ 35015840
...@@ -3,7 +3,6 @@ ...@@ -3,7 +3,6 @@
module EE module EE
module MergeRequestPresenter module MergeRequestPresenter
include ::VisibleApprovable include ::VisibleApprovable
prepend VisibleApprovableForRule
def approvals_path def approvals_path
if expose_mr_approval_path? if expose_mr_approval_path?
......
ee/spec/controllers/projects/merge_requests_controller_spec.rb
View file @ 35015840
...@@ -9,7 +9,6 @@ shared_examples 'approvals' do ...@@ -9,7 +9,6 @@ shared_examples 'approvals' do
let!(:approval_rule) { create(:approval_project_rule, project: project, users: [approver, user], approvals_required: 2) } let!(:approval_rule) { create(:approval_project_rule, project: project, users: [approver, user], approvals_required: 2) }
before do before do
# merge_request.update_attribute :approvals_before_merge, 2
project.add_developer(approver) project.add_developer(approver)
end end
......
ee/spec/models/approvable_for_rule_spec.rb deleted 100644 → 0
View file @ 7dc7bfa9
# frozen_string_literal: true
require 'spec_helper'
# Based on approvable_spec.rb
describe ApprovableForRule do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:author) { merge_request.author }
describe '#approvers_overwritten?' do
subject { merge_request.approvers_overwritten? }
it 'returns false when merge request has no approvers' do
is_expected.to be false
end
it 'returns true when merge request has user approver' do
create(:approver, target: merge_request)
is_expected.to be true
end
it 'returns true when merge request has group approver' do
group = create(:group_with_members)
create(:approver_group, target: merge_request, group: group)
is_expected.to be true
end
end
describe '#can_approve?' do
subject { merge_request.can_approve?(user) }
it 'returns false if user is nil' do
expect(merge_request.can_approve?(nil)).to be false
end
it 'returns true when user is included in the approvers list' do
user = create(:approver, target: merge_request).user
expect(merge_request.can_approve?(user)).to be true
end
context 'when the user is the author' do
context 'and author is an approver' do
before do
create(:approver, target: merge_request, user: author)
end
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(merge_request.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(merge_request.can_approve?(author)).to be false
end
end
context 'and author is not an approver' do
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(merge_request.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(merge_request.can_approve?(author)).to be false
end
end
end
context 'when user is a committer' do
let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
before do
project.add_developer(user)
end
context 'and committer is an approver' do
before do
create(:approver, target: merge_request, user: user)
end
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(merge_request.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(merge_request.can_approve?(user)).to be false
end
end
context 'and committer is not an approver' do
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(merge_request.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(merge_request.can_approve?(user)).to be false
end
end
end
it 'returns false when user is unable to update the merge request' do
user = create(:user)
project.add_guest(user)
expect(merge_request.can_approve?(user)).to be false
end
context 'when approvals are required' do
before do
project.update!(approvals_before_merge: 1)
end
it 'returns true when approvals are still accepted and user still has not approved' do
user = create(:user)
project.add_developer(user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when there is still one approver missing' do
user = create(:user)
project.add_developer(user)
create(:approver, target: merge_request)
expect(merge_request.can_approve?(user)).to be false
end
end
end
end
ee/spec/models/approvable_spec.rb
View file @ 35015840
require 'spec_helper' require 'spec_helper'
describe Approvable do describe Approvable do
let(:merge_request) { create(:merge_request) } subject(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project } let(:project) { merge_request.project }
let(:author) { merge_request.author } let(:author) { merge_request.author }
before do
stub_feature_flags(approval_rules: false)
end
describe '#approval_feature_available?' do describe '#approval_feature_available?' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
...@@ -27,140 +23,7 @@ describe Approvable do ...@@ -27,140 +23,7 @@ describe Approvable do
end end
end end
describe '#approvers_overwritten?' do described_class::FORWARDABLE_METHODS.each do |method|
subject { merge_request.approvers_overwritten? } it { is_expected.to delegate_method(method).to(:approval_state) }
it 'returns false when merge request has no approvers' do
is_expected.to be false
end
it 'returns true when merge request has user approver' do
create(:approver, target: merge_request)
is_expected.to be true
end
it 'returns true when merge request has group approver' do
group = create(:group_with_members)
create(:approver_group, target: merge_request, group: group)
is_expected.to be true
end
end
describe '#can_approve?' do
subject { merge_request.can_approve?(user) }
it 'returns false if user is nil' do
expect(merge_request.can_approve?(nil)).to be false
end
it 'returns true when user is included in the approvers list' do
user = create(:approver, target: merge_request).user
expect(merge_request.can_approve?(user)).to be true
end
context 'when authors can approve' do
before do
project.update(merge_requests_author_approval: true)
end
context 'when the user is the author' do
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: author)
expect(merge_request.can_approve?(author)).to be true
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(author)).to be false
end
end
context 'when user is committer' do
let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
before do
project.add_developer(user)
end
context 'and committers can not approve' do
before do
project.update(merge_requests_disable_committers_approval: true)
end
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be false
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(user)).to be false
end
end
context 'and committers can approve' do
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(user)).to be false
end
end
end
end
context 'when authors cannot approve' do
before do
project.update(merge_requests_author_approval: false)
end
it 'returns false when user is the author' do
create(:approver, target: merge_request, user: author)
expect(merge_request.can_approve?(author)).to be false
end
it 'returns true when user is a committer' do
user = create(:user, email: merge_request.commits.without_merge_commits.first.committer_email)
project.add_developer(user)
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be true
end
end
it 'returns false when user is unable to update the merge request' do
user = create(:user)
project.add_guest(user)
expect(merge_request.can_approve?(user)).to be false
end
context 'when approvals are required' do
before do
project.update(approvals_before_merge: 1)
end
it 'returns true when approvals are still accepted and user still has not approved' do
user = create(:user)
project.add_developer(user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when there is still one approver missing' do
user = create(:user)
project.add_developer(user)
create(:approver, target: merge_request)
expect(merge_request.can_approve?(user)).to be false
end
end
end end
end end
ee/spec/models/approval_state_spec.rb
View file @ 35015840
...@@ -1174,8 +1174,83 @@ describe ApprovalState do ...@@ -1174,8 +1174,83 @@ describe ApprovalState do
let(:developer) { create_project_member(:developer) } let(:developer) { create_project_member(:developer) }
let(:other_developer) { create_project_member(:developer) } let(:other_developer) { create_project_member(:developer) }
let(:reporter) { create_project_member(:reporter) } let(:reporter) { create_project_member(:reporter) }
let(:guest) { create_project_member(:guest) }
let(:stranger) { create(:user) } let(:stranger) { create(:user) }
context 'when the user is the author' do
context 'and author is an approver' do
before do
create(:approval_project_rule, project: project, users: [author])
end
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(subject.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(subject.can_approve?(author)).to be false
end
end
context 'and author is not an approver' do
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(subject.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(subject.can_approve?(author)).to be false
end
end
end
context 'when user is a committer' do
let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
before do
project.add_developer(user)
end
context 'and committer is an approver' do
before do
create(:approval_project_rule, project: project, users: [user])
end
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(subject.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(subject.can_approve?(user)).to be false
end
end
context 'and committer is not an approver' do
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(subject.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(subject.can_approve?(user)).to be false
end
end
end
context 'when there is one approver required' do context 'when there is one approver required' do
let!(:rule) { create_rule(approvals_required: 1) } let!(:rule) { create_rule(approvals_required: 1) }
...@@ -1318,6 +1393,7 @@ describe ApprovalState do ...@@ -1318,6 +1393,7 @@ describe ApprovalState do
expect(subject.can_approve?(author)).to be_falsey expect(subject.can_approve?(author)).to be_falsey
expect(subject.can_approve?(reporter)).to be_falsey expect(subject.can_approve?(reporter)).to be_falsey
expect(subject.can_approve?(guest)).to be_falsey
expect(subject.can_approve?(stranger)).to be_falsey expect(subject.can_approve?(stranger)).to be_falsey
expect(subject.can_approve?(nil)).to be_falsey expect(subject.can_approve?(nil)).to be_falsey
end end
...@@ -1347,6 +1423,7 @@ describe ApprovalState do ...@@ -1347,6 +1423,7 @@ describe ApprovalState do
expect(subject.can_approve?(author)).to be_falsey expect(subject.can_approve?(author)).to be_falsey
expect(subject.can_approve?(reporter)).to be_falsey expect(subject.can_approve?(reporter)).to be_falsey
expect(subject.can_approve?(other_developer)).to be_truthy expect(subject.can_approve?(other_developer)).to be_truthy
expect(subject.can_approve?(guest)).to be_falsey
expect(subject.can_approve?(stranger)).to be_falsey expect(subject.can_approve?(stranger)).to be_falsey
expect(subject.can_approve?(nil)).to be_falsey expect(subject.can_approve?(nil)).to be_falsey
end end
......
ee/spec/models/visible_approvable_for_rule_spec.rb deleted 100644 → 0
View file @ 7dc7bfa9
# frozen_string_literal: true
require 'spec_helper'
# Based on visible_approvable_spec.rb
describe VisibleApprovableForRule do
let(:resource) { create(:merge_request, source_project: project, target_project: project) }
let!(:project) { create(:project, :repository) }
let!(:user) { project.creator }
describe '#approvers_left' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
let!(:approver) { create(:approver, target: resource) }
subject { resource.approvers_left }
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new { subject }
expect { subject }.not_to exceed_query_limit(control)
end
it 'returns all approvers left' do
resource.approvals.create!(user: approver.user)
is_expected.to match_array(public_approver_group.users + private_approver_group.users)
end
end
describe '#overall_approvers' do
let(:approver) { create(:user) }
let(:code_owner) { build(:user) }
let!(:project_regular_rule) { create(:approval_project_rule, project: project, users: [approver]) }
let!(:code_owner_rule) { create(:code_owner_rule, merge_request: resource, users: [code_owner]) }
before do
project.add_developer(approver)
project.add_developer(code_owner)
end
subject { resource.overall_approvers }
it 'returns a list of all the project approvers' do
is_expected.to contain_exactly(approver, code_owner)
end
context 'when exclude_code_owners is true' do
subject { resource.overall_approvers(exclude_code_owners: true) }
it 'excludes code owners' do
is_expected.to contain_exactly(approver)
end
end
context 'when approvers are overwritten' do
let!(:merge_request_regular_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [create(:user)]) }
it 'returns the list of all the merge request level approvers' do
is_expected.to contain_exactly(*merge_request_regular_rule.users, code_owner)
end
end
context 'when author is an approver' do
let!(:approver) { resource.author }
it 'excludes author if author cannot approve' do
project.update(merge_requests_author_approval: false)
is_expected.not_to include(approver)
end
it 'includes author if author is able to approve' do
project.update(merge_requests_author_approval: true)
is_expected.to include(approver)
end
end
context 'when a committer is an approver' do
let!(:approver) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
it 'excludes the committer if committers cannot approve' do
project.update(merge_requests_disable_committers_approval: true)
is_expected.not_to include(approver)
end
it 'includes the committer if committers are able to approve' do
project.update(merge_requests_disable_committers_approval: false)
is_expected.to include(approver)
end
end
end
describe '#all_approvers_including_groups' do
let!(:group) { create(:group_with_members) }
let!(:approver_group) { create(:approver_group, target: resource, group: group) }
let!(:approver) { create(:approver, target: resource) }
subject { resource.all_approvers_including_groups }
it 'returns all approvers (groups and users)' do
is_expected.to match_array(approver_group.users + [approver.user])
end
end
describe '#authors_can_approve?' do
subject { resource.authors_can_approve? }
it 'returns false when merge_requests_author_approval flag is off' do
is_expected.to be false
end
it 'returns true when merge_requests_author_approval flag is turned on' do
project.update(merge_requests_author_approval: true)
is_expected.to be true
end
end
end
ee/spec/models/visible_approvable_spec.rb
View file @ 35015840
...@@ -5,16 +5,15 @@ describe VisibleApprovable do ...@@ -5,16 +5,15 @@ describe VisibleApprovable do
let!(:project) { create(:project, :repository) } let!(:project) { create(:project, :repository) }
let!(:user) { project.creator } let!(:user) { project.creator }
before do
stub_feature_flags(approval_rules: false)
end
describe '#approvers_left' do describe '#approvers_left' do
let!(:private_group) { create(:group_with_members, :private) } let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) } let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) } let!(:approver) { create(:user) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) } let!(:rule) { create(:approval_project_rule, project: project, groups: [public_group, private_group], users: [approver])}
let!(:approver) { create(:approver, target: resource) }
before do
project.add_developer(approver)
end
subject { resource.approvers_left } subject { resource.approvers_left }
...@@ -25,130 +24,88 @@ describe VisibleApprovable do ...@@ -25,130 +24,88 @@ describe VisibleApprovable do
end end
it 'returns all approvers left' do it 'returns all approvers left' do
resource.approvals.create!(user: approver.user) resource.approvals.create!(user: approver)
is_expected.to match_array(public_approver_group.users + private_approver_group.users) is_expected.to match_array(public_group.users + private_group.users)
end end
end end
describe '#overall_approvers' do describe '#overall_approvers' do
let!(:project_approver) { create(:approver, target: project) } let(:approver) { create(:user) }
let(:code_owner) { build(:user) } let(:code_owner) { build(:user) }
let!(:project_regular_rule) { create(:approval_project_rule, project: project, users: [approver]) }
let!(:code_owner_rule) { create(:code_owner_rule, merge_request: resource, users: [code_owner]) }
before do before do
allow(resource).to receive(:code_owners).and_return([code_owner]) project.add_developer(approver)
project.add_developer(project_approver.user) project.add_developer(code_owner)
end end
subject { resource.overall_approvers } subject { resource.overall_approvers }
it 'returns a list of all the project approvers' do it 'returns a list of all the project approvers' do
is_expected.to contain_exactly(project_approver.user, code_owner) is_expected.to contain_exactly(approver, code_owner)
end end
context 'when exclude_code_owners is true' do context 'when exclude_code_owners is true' do
subject { resource.overall_approvers(exclude_code_owners: true) } subject { resource.overall_approvers(exclude_code_owners: true) }
it 'excludes code owners' do it 'excludes code owners' do
is_expected.to contain_exactly(project_approver.user) is_expected.to contain_exactly(approver)
end end
end end
context 'when author is approver' do context 'when approvers are overwritten' do
let!(:author_approver) { create(:approver, target: project, user: resource.author) } let!(:merge_request_regular_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [create(:user)]) }
it 'excludes author if authors cannot approve' do
is_expected.not_to include(author_approver.user)
end
it 'includes author if authors are able to approve' do
project.update(merge_requests_author_approval: true)
is_expected.to include(author_approver.user)
end
end
context 'when committer is approver' do
let(:user) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
let!(:committer_approver) { create(:approver, target: project, user: user) }
before do
project.add_developer(user)
end
it 'excludes committer if committers cannot approve' do
project.update(merge_requests_disable_committers_approval: true)
is_expected.not_to include(committer_approver.user)
end
it 'includes committer if committers are able to approve' do it 'returns the list of all the merge request level approvers' do
is_expected.to include(committer_approver.user) is_expected.to contain_exactly(*merge_request_regular_rule.users, code_owner)
end end
end end
context 'when approvers are overwritten' do context 'when author is an approver' do
let!(:approver) { create(:approver, target: resource) } let!(:approver) { resource.author }
before do it 'excludes author if author cannot approve' do
project.add_developer(approver.user) project.update(merge_requests_author_approval: false)
end
it 'returns the list of all the merge request user approvers' do is_expected.not_to include(approver)
is_expected.to contain_exactly(approver.user)
end end
end
context 'when approver is no longer part of project' do it 'includes author if author is able to approve' do
it 'excludes non-project members' do project.update(merge_requests_author_approval: true)
project.team.find_member(project_approver.user).destroy!
is_expected.not_to include(project_approver.user) is_expected.to include(approver)
end end
end end
end
describe '#overall_approver_groups' do context 'when a committer is an approver' do
before do let!(:approver) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
group = create(:group_with_members)
create(:approver_group, target: project, group: group)
end
subject { resource.overall_approver_groups } it 'excludes the committer if committers cannot approve' do
project.update(merge_requests_disable_committers_approval: true)
it 'returns all the project approver groups' do is_expected.not_to include(approver)
is_expected.to match_array(project.approver_groups) end
end
context 'when group approvers are overwritten' do it 'includes the committer if committers are able to approve' do
it 'returns all the merge request approver groups' do project.update(merge_requests_disable_committers_approval: false)
group = create(:group_with_members)
create(:approver_group, target: resource, group: group)
is_expected.to match_array(resource.approver_groups) is_expected.to include(approver)
end end
end end
end end
describe '#all_approvers_including_groups' do describe '#all_approvers_including_groups' do
let!(:group) { create(:group_with_members) } let!(:group) { create(:group_with_members) }
let!(:approver_group) { create(:approver_group, target: resource, group: group) } let!(:approver) { create(:user) }
let!(:approver) { create(:approver, target: resource) } let!(:rule) { create(:approval_project_rule, project: project, groups: [group], users: [approver]) }
before do
project.add_developer(approver.user)
end
subject { resource.all_approvers_including_groups } subject { resource.all_approvers_including_groups }
it 'only queries once' do
expect(resource).to receive(:overall_approvers).and_call_original.once
3.times { subject }
end
it 'returns all approvers (groups and users)' do it 'returns all approvers (groups and users)' do
is_expected.to match_array(approver_group.users + [approver.user]) is_expected.to match_array(group.users + [approver])
end end
end end
...@@ -168,8 +125,9 @@ describe VisibleApprovable do ...@@ -168,8 +125,9 @@ describe VisibleApprovable do
describe '#reset_approval_cache!' do describe '#reset_approval_cache!' do
before do before do
approver = create(:approver, target: resource) approver = create(:user)
project.add_developer(approver.user) project.add_developer(approver)
create(:approval_project_rule, project: project, users: [approver])
end end
subject { resource.reset_approval_cache! } subject { resource.reset_approval_cache! }
......
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7