From 94f2a233c84e579131260c85962e981378e0901b Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 7 Feb 2019 17:54:44 +0100 Subject: [PATCH 1/2] Merge BE bvl-wrap-fallback-approval-rule --- .../merge_requests/application_controller.rb | 9 +-- .../models/approval_merge_request_fallback.rb | 68 +++++++++++++++++++ ee/app/models/approval_state.rb | 55 +++++++++------ ee/app/models/approval_wrapped_rule.rb | 2 +- ee/app/models/concerns/approval_rule_like.rb | 4 ++ ee/app/models/ee/project.rb | 21 ++++++ ee/lib/ee/api/entities.rb | 23 ++++--- .../creations_controller_spec.rb | 10 +-- .../merge_requests_controller_spec.rb | 22 +++--- .../public_api/v4/project_approval_rule.json | 1 + .../javascripts/fixtures/merge_requests.rb | 2 + .../approval_merge_request_fallback_spec.rb | 56 +++++++++++++++ ee/spec/models/approval_state_spec.rb | 60 ++++++++++++---- .../concerns/approval_rule_like_spec.rb | 8 +++ ee/spec/models/project_spec.rb | 52 ++++++++++++++ .../approval_rule_presenter_spec.rb | 11 ++- .../api/merge_request_approvals_spec.rb | 5 +- 17 files changed, 341 insertions(+), 68 deletions(-) create mode 100644 ee/app/models/approval_merge_request_fallback.rb create mode 100644 ee/spec/models/approval_merge_request_fallback_spec.rb diff --git a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb index 4a3159e99f93c2..347478ba526552 100644 --- a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb @@ -24,8 +24,9 @@ def merge_request_params_attributes end # If the number of approvals is not greater than the project default, set to - # nil, so that we fall back to the project default. If it's not set, we can - # let the normal update logic handle this. + # the project default, so that we fall back to the project default. And + # still allow overriding rules defined at the project level but not allow + # a number of approvals lower than what the project defined. def clamp_approvals_before_merge(mr_params) return mr_params unless mr_params[:approvals_before_merge] @@ -39,8 +40,8 @@ def clamp_approvals_before_merge(mr_params) project end - if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge - mr_params[:approvals_before_merge] = nil + if mr_params[:approvals_before_merge].to_i < target_project.min_fallback_approvals + mr_params[:approvals_before_merge] = target_project.min_fallback_approvals end mr_params diff --git a/ee/app/models/approval_merge_request_fallback.rb b/ee/app/models/approval_merge_request_fallback.rb new file mode 100644 index 00000000000000..971ec95b567fea --- /dev/null +++ b/ee/app/models/approval_merge_request_fallback.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +class ApprovalMergeRequestFallback + attr_reader :merge_request + delegate :approved_by_users, :project, to: :merge_request + + def initialize(merge_request) + @merge_request = merge_request + end + + # Implements all methods `WrappedApprovalRule` required methods + def id + 'fallback-rule' + end + + def name + '' + end + + def users + User.none + end + + def groups + Group.none + end + + def approvals_required + @approvals_required ||= [merge_request.approvals_before_merge.to_i, + project.min_fallback_approvals].max + end + + def approvals_left + @approvals_left ||= [approvals_required - approved_by_users.size, 0].max + end + + def approvers + [] + end + + def approved_approvers + approved_by_users + end + + def approved? + approved_approvers.size >= approvals_required + end + + def code_owner + false + end + + def source_rule + nil + end + + def regular + false + end + + def rule_type + :fallback + end + + def project + merge_request.target_project + end +end diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index 2c3523e6b76b48..7c43300034dd0b 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -31,7 +31,9 @@ def self.filter_author(users, merge_request) def wrapped_approval_rules strong_memoize(:wrapped_approval_rules) do - regular_rules + code_owner_rules + result = use_fallback? ? [fallback_rule] : regular_rules + result += code_owner_rules + result end end @@ -39,32 +41,40 @@ def has_approval_rules? !wrapped_approval_rules.empty? end + # Use the fallback rule if regular rules are empty def use_fallback? regular_rules.empty? end + def fallback_rule + ApprovalMergeRequestFallback.new(merge_request) + end + + # If a user only has access to a single approval rule. But the project allows + # overriding that rule, we need to allow setting `approvals_before_merge` on + # the merge request and validate that instead. So in that case we allow falling + # back to a `ApprovalMergeRequestFallback` + def single_overridden_project_rule? + !project.multiple_approval_rules_available? && + project.can_override_approvers? && + merge_request.approvals_before_merge.present? + end + + # Determines which set of rules to use (MR or project) def approval_rules_overwritten? - merge_request.approval_rules.any?(&:regular?) + regular_merge_request_rules.any? || single_overridden_project_rule? end alias_method :approvers_overwritten?, :approval_rules_overwritten? def approval_needed? return false unless project.feature_available?(:merge_request_approvers) - result = wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 } - result ||= fallback_approvals_required > 0 if use_fallback? - result - end - - def fallback_approvals_required - @fallback_approvals_required ||= [project.approvals_before_merge, merge_request.approvals_before_merge || 0].max + wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 } end def approved? strong_memoize(:approved) do - result = wrapped_approval_rules.all?(&:approved?) - result &&= approvals.size >= fallback_approvals_required if use_fallback? - result + wrapped_approval_rules.all?(&:approved?) end end @@ -74,9 +84,7 @@ def any_approver_allowed? def approvals_required strong_memoize(:approvals_required) do - result = wrapped_approval_rules.sum(&:approvals_required) - result = [result, fallback_approvals_required].max if use_fallback? - result + wrapped_approval_rules.sum(&:approvals_required) end end @@ -84,9 +92,7 @@ def approvals_required # considered approved. def approvals_left strong_memoize(:approvals_left) do - result = wrapped_approval_rules.sum(&:approvals_left) - result = [result, fallback_approvals_required - approved_approvers.size].max if use_fallback? - result + wrapped_approval_rules.sum(&:approvals_left) end end @@ -156,10 +162,9 @@ def first_regular_rule def regular_rules strong_memoize(:regular_rules) do - rule_source = approval_rules_overwritten? ? merge_request : project - rules = rule_source.approval_rules.select(&:regular?).sort_by(&:id) + rules = approval_rules_overwritten? ? regular_merge_request_rules : regular_project_rules - unless project.feature_available?(:multiple_approval_rules) + unless project.multiple_approval_rules_available? rules = rules[0, 1] end @@ -167,6 +172,14 @@ def regular_rules end end + def regular_merge_request_rules + @regular_merge_request_rules ||= merge_request.approval_rules.select(&:regular?).sort_by(&:id) + end + + def regular_project_rules + @regular_project_rules ||= project.visible_regular_approval_rules.to_a + end + def code_owner_rules strong_memoize(:code_owner_rules) do wrap_rules(merge_request.approval_rules.select(&:code_owner?)) diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index a27a29cf29c917..f6fa391b780914 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -8,7 +8,7 @@ class ApprovalWrappedRule attr_reader :merge_request attr_reader :approval_rule - def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner, :source_rule + def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner, :source_rule, :rule_type def initialize(merge_request, approval_rule) @merge_request = merge_request diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 0b1dca87a29bee..f5abafd0e76b8d 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -38,4 +38,8 @@ def remove_member(member) groups.delete(member) end end + + def rule_type + @rule_type ||= code_owner ? :code_owner : :regular + end end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index dd1333427a1ad3..48ac7ef5907240 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -213,6 +213,10 @@ def multiple_issue_boards_available? feature_available?(:multiple_project_issue_boards) end + def multiple_approval_rules_available? + feature_available?(:multiple_approval_rules) + end + def service_desk_enabled ::EE::Gitlab::ServiceDesk.enabled?(project: self) && super end @@ -286,6 +290,23 @@ def approvals_before_merge super end + def visible_regular_approval_rules + strong_memoize(:visible_regular_approval_rules) do + regular_rules = approval_rules.regular.order(:id) + + next regular_rules.take(1) unless multiple_approval_rules_available? + + regular_rules + end + end + + def min_fallback_approvals + strong_memoize(:min_fallback_approvals) do + visible_regular_approval_rules.map(&:approvals_required).max || + approvals_before_merge.to_i + end + end + def reset_approvals_on_push super && feature_available?(:merge_request_approvers) end diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index 4bee7addeeb966..8c8ee3a1897756 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -243,13 +243,16 @@ class SpecialBoardFilter < Grape::Entity expose :title end - class ApprovalRule < Grape::Entity + class ApprovalRuleShort < Grape::Entity + expose :id, :name, :rule_type + end + + class ApprovalRule < ApprovalRuleShort def initialize(object, options = {}) presenter = ::ApprovalRulePresenter.new(object, current_user: options[:current_user]) super(presenter, options) end - expose :id, :name expose :approvers, using: ::API::Entities::UserBasic expose :approvals_required expose :users, using: ::API::Entities::UserBasic @@ -273,16 +276,12 @@ class MergeRequestApprovalRules < Grape::Entity end expose :wrapped_approval_rules, as: :rules, using: MergeRequestApprovalRule - expose :fallback_approvals_required - expose :use_fallback do |approval_state| - approval_state.use_fallback? - end end # Decorates Project class ProjectApprovalRules < Grape::Entity - expose :approval_rules, as: :rules, using: ApprovalRule - expose :approvals_before_merge, as: :fallback_approvals_required + expose :visible_regular_approval_rules, as: :rules, using: ApprovalRule + expose :min_fallback_approvals, as: :fallback_approvals_required end # @deprecated @@ -383,13 +382,15 @@ class ApprovalState < Grape::Entity approval_state.can_approve?(options[:current_user]) end - expose :approval_rules_left do |approval_state, options| - approval_state.approval_rules_left.map(&:name) - end + expose :approval_rules_left, using: ApprovalRuleShort expose :has_approval_rules do |approval_state| approval_state.has_approval_rules? end + + expose :multiple_approval_rules_available do |approval_state| + approval_state.project.multiple_approval_rules_available? + end end class LdapGroup < Grape::Entity diff --git a/ee/spec/controllers/projects/merge_requests/creations_controller_spec.rb b/ee/spec/controllers/projects/merge_requests/creations_controller_spec.rb index f0342033837486..543922a50c31ae 100644 --- a/ee/spec/controllers/projects/merge_requests/creations_controller_spec.rb +++ b/ee/spec/controllers/projects/merge_requests/creations_controller_spec.rb @@ -38,8 +38,8 @@ def create_merge_request(overrides = {}) create_merge_request(approvals_before_merge: 1) end - it 'sets the param to nil' do - expect(created_merge_request.approvals_before_merge).to eq(nil) + it 'sets the param to the project value' do + expect(created_merge_request.reload.approvals_before_merge).to eq(2) end it 'creates the merge request' do @@ -53,8 +53,8 @@ def create_merge_request(overrides = {}) create_merge_request(approvals_before_merge: 2) end - it 'sets the param to nil' do - expect(created_merge_request.approvals_before_merge).to eq(nil) + it 'sets the param to the correct value' do + expect(created_merge_request.reload.approvals_before_merge).to eq(2) end it 'creates the merge request' do @@ -88,7 +88,7 @@ def create_merge_request(overrides = {}) end it 'uses the default from the target project' do - expect(created_merge_request.approvals_before_merge).to eq(nil) + expect(created_merge_request.reload.approvals_before_merge).to eq(4) end it 'creates the merge request' do diff --git a/ee/spec/controllers/projects/merge_requests_controller_spec.rb b/ee/spec/controllers/projects/merge_requests_controller_spec.rb index c7b7a7f1e1e028..0eb46a67ef8830 100644 --- a/ee/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/ee/spec/controllers/projects/merge_requests_controller_spec.rb @@ -164,6 +164,12 @@ def update_merge_request(params = {}) expect(merge_request.reload.approvals_before_merge).to eq(2) end + it 'does not allow approvels before merge lower than the project setting' do + update_merge_request(approvals_before_merge: 0) + + expect(merge_request.reload.approvals_before_merge).to eq(1) + end + it 'creates rules' do users = create_list(:user, 3) users.each { |user| project.add_developer(user) } @@ -221,8 +227,8 @@ def update_merge_request(params = {}) update_merge_request(approvals_before_merge: 1) end - it 'sets the param to nil' do - expect(merge_request.approvals_before_merge).to eq(nil) + it 'sets the param to the sames as the project' do + expect(merge_request.reload.approvals_before_merge).to eq(2) end it 'updates the merge request' do @@ -236,8 +242,8 @@ def update_merge_request(params = {}) update_merge_request(approvals_before_merge: 2) end - it 'sets the param to nil' do - expect(merge_request.reload.approvals_before_merge).to eq(nil) + it 'sets the param to the same as the project' do + expect(merge_request.reload.approvals_before_merge).to eq(2) end it 'updates the merge request' do @@ -287,8 +293,8 @@ def update_merge_request(params = {}) update_merge_request(approvals_before_merge: 1) end - it 'sets the param to nil' do - expect(merge_request.reload.approvals_before_merge).to eq(nil) + it 'sets the param to the same as the target project' do + expect(merge_request.reload.approvals_before_merge).to eq(2) end it 'updates the merge request' do @@ -302,8 +308,8 @@ def update_merge_request(params = {}) update_merge_request(approvals_before_merge: 2) end - it 'sets the param to nil' do - expect(merge_request.reload.approvals_before_merge).to eq(nil) + it 'sets the param to the same as the target project' do + expect(merge_request.reload.approvals_before_merge).to eq(2) end it 'updates the merge request' do diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json index e3b5b4b020b377..c8510433db16fb 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json @@ -4,6 +4,7 @@ "id": { "type": "integer" }, "name": { "type": "string" }, "approvals_required": { "type": "integer" }, + "rule_type": { "type": "tring" }, "approvers": { "type": "array", "items": { diff --git a/ee/spec/javascripts/fixtures/merge_requests.rb b/ee/spec/javascripts/fixtures/merge_requests.rb index dbc2b325abe99a..79a3f497416fcc 100644 --- a/ee/spec/javascripts/fixtures/merge_requests.rb +++ b/ee/spec/javascripts/fixtures/merge_requests.rb @@ -33,6 +33,8 @@ approver_group = create(:approver_group, target: project) approver_group.group.add_owner(create(:owner)) + stub_feature_flags(approval_rules: false) + sign_in(admin) end diff --git a/ee/spec/models/approval_merge_request_fallback_spec.rb b/ee/spec/models/approval_merge_request_fallback_spec.rb new file mode 100644 index 00000000000000..f64f2c1554ea16 --- /dev/null +++ b/ee/spec/models/approval_merge_request_fallback_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe ApprovalMergeRequestFallback do + using RSpec::Parameterized::TableSyntax + + let(:merge_request) { create(:merge_request, approvals_before_merge: 2) } + let(:project) { merge_request.project } + subject(:rule) { described_class.new(merge_request) } + + describe '#approvals_required' do + where(:merge_request_requirement, :project_requirement, :project_rule_requirement, :expected) do + nil | nil | nil | 0 + 10 | 5 | nil | 10 + 2 | 9 | nil | 9 + 2 | 9 | 7 | 7 + 10 | 9 | 7 | 10 + end + + with_them do + before do + merge_request.approvals_before_merge = merge_request_requirement + project.approvals_before_merge = project_requirement + if project_rule_requirement + create(:approval_project_rule, + project: project, + approvals_required: project_rule_requirement) + end + end + + it 'returns the expected value' do + expect(rule.approvals_required).to eq(expected) + end + end + end + + describe '#approvals_left' do + it 'returns the correct number of approvals left' do + create(:approval, merge_request: merge_request) + + expect(rule.approvals_left).to eq(1) + end + end + + describe '#approved?' do + it 'is falsy' do + expect(rule.approved?).to be(false) + end + + it 'is true if there where enough approvals' do + create_list(:approval, 2, merge_request: merge_request) + + expect(rule.approved?).to be(true) + end + end +end diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb index 91424969e156f1..ff61555ff4ec9a 100644 --- a/ee/spec/models/approval_state_spec.rb +++ b/ee/spec/models/approval_state_spec.rb @@ -10,6 +10,13 @@ def create_rule(additional_params = {}) create(factory, params) end + def approve_rules(rules) + rules_to_approve = rules.select { |rule| rule.approvals_required > 0 } + rules_to_approve.each do |rule| + create(:approval, merge_request: merge_request, user: rule.users.first) + end + end + let(:merge_request) { create(:merge_request) } let(:project) { merge_request.target_project } let(:approver1) { create(:user) } @@ -130,9 +137,7 @@ def create_rule(additional_params = {}) shared_examples_for 'when rules are present' do context 'when all rules are approved' do before do - subject.wrapped_approval_rules.each do |rule| - create(:approval, merge_request: merge_request, user: rule.users.first) - end + approve_rules(subject.wrapped_approval_rules) end it 'returns true' do @@ -154,10 +159,6 @@ def create_rule(additional_params = {}) shared_examples_for 'checking fallback_approvals_required' do before do project.update(approvals_before_merge: 1) - - subject.wrapped_approval_rules.each do |rule| - allow(rule).to receive(:approved?).and_return(true) - end end context 'when it is not met' do @@ -686,9 +687,7 @@ def create_rules shared_examples_for 'when rules are present' do context 'when all rules are approved' do before do - subject.wrapped_approval_rules.each do |rule| - create(:approval, merge_request: merge_request, user: rule.users.first) - end + approve_rules(subject.wrapped_approval_rules) end it 'returns true' do @@ -710,10 +709,6 @@ def create_rules shared_examples_for 'checking fallback_approvals_required' do before do project.update(approvals_before_merge: 1) - - subject.wrapped_approval_rules.each do |rule| - allow(rule).to receive(:approved?).and_return(true) - end end context 'when it is not met' do @@ -737,7 +732,8 @@ def create_rules context 'when only code owner rules present' do before do - 2.times { create_rule(users: [create(:user)], code_owner: true) } + # setting approvals required to 0 since we don't want to block on them now + 2.times { create_rule(users: [create(:user)], code_owner: true, approvals_required: 0) } end it_behaves_like 'when rules are present' @@ -752,6 +748,40 @@ def create_rules it_behaves_like 'when rules are present' end + + context 'when a single project rule is present' do + before do + create(:approval_project_rule, users: [create(:user)], project: project) + end + + it_behaves_like 'when rules are present' + + context 'when the project rule is overridden by a fallback but the project does not allow overriding' do + before do + merge_request.update!(approvals_before_merge: 1) + project.update!(disable_overriding_approvers_per_merge_request: true) + end + + it_behaves_like 'when rules are present' + end + + context 'when the project rule is overridden by a fallback' do + before do + merge_request.update!(approvals_before_merge: 1) + end + + it_behaves_like 'checking fallback_approvals_required' + end + end + + context 'when a single project rule is present that is overridden in the merge request' do + before do + create(:approval_project_rule, users: [create(:user)], project: project) + merge_request.update!(approvals_before_merge: 1) + end + + it_behaves_like 'checking fallback_approvals_required' + end end describe '#any_approver_allowed?' do diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 44a112f70af798..50ab066336915c 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -118,4 +118,12 @@ expect(subject.group_users).to eq([user1]) end end + + context '#rule_type' do + it 'returns the correct type' do + expect(build(:code_owner_rule).rule_type).to eq(:code_owner) + expect(build(:approval_merge_request_rule).rule_type).to eq(:regular) + expect(build(:approval_project_rule).rule_type).to eq(:regular) + end + end end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 4ac039c8e4fc47..f7ade1a579aa09 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -944,6 +944,58 @@ end end + describe '#visible_regular_approval_rules' do + let(:project) { create(:project) } + let!(:approval_rules) { create_list(:approval_project_rule, 2, project: project) } + + before do + stub_licensed_features(multiple_approval_rules: true) + end + + it 'returns all approval rules' do + expect(project.visible_regular_approval_rules).to contain_exactly(*approval_rules) + end + + context 'when multiple approval rules is not available' do + before do + stub_licensed_features(multiple_approval_rules: false) + end + + it 'returns the first approval rule' do + expect(project.visible_regular_approval_rules).to contain_exactly(approval_rules.first) + end + end + end + + describe '#min_fallback_approvals' do + let(:project) { create(:project) } + + it 'returns approvals before merge if there are no rules' do + project.approvals_before_merge = 3 + + expect(project.min_fallback_approvals).to eq(3) + end + + context 'when approval rules are present' do + before do + create(:approval_project_rule, project: project, approvals_required: 2) + create(:approval_project_rule, project: project, approvals_required: 3) + + stub_licensed_features(multiple_approval_rules: true) + end + + it 'returns the maximum requirement' do + expect(project.min_fallback_approvals).to eq(3) + end + + it 'returns the first rule requirement if there is a rule' do + stub_licensed_features(multiple_approval_rules: false) + + expect(project.min_fallback_approvals).to eq(2) + end + end + end + shared_examples 'project with disabled services' do it 'has some disabled services' do stub_const('License::ANY_PLAN_FEATURES', []) diff --git a/ee/spec/presenters/approval_rule_presenter_spec.rb b/ee/spec/presenters/approval_rule_presenter_spec.rb index 173a708306a8ef..87fdb3220e94ea 100644 --- a/ee/spec/presenters/approval_rule_presenter_spec.rb +++ b/ee/spec/presenters/approval_rule_presenter_spec.rb @@ -8,10 +8,9 @@ set(:public_group) { create(:group) } set(:private_group) { create(:group, :private) } let(:groups) { [public_group, private_group] } + subject { described_class.new(rule, current_user: user) } shared_examples 'filtering private group' do - subject { described_class.new(rule, current_user: user) } - context 'when user has no access to private group' do it 'excludes private group' do expect(subject.groups).to contain_exactly(public_group) @@ -41,5 +40,13 @@ it_behaves_like 'filtering private group' end + + context 'fallback rule' do + let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) } + + it 'contains no groups without raising an error' do + expect(subject.groups).to be_empty + end + end end end diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb index aae11c23dbd4f0..12be401815155b 100644 --- a/ee/spec/requests/api/merge_request_approvals_spec.rb +++ b/ee/spec/requests/api/merge_request_approvals_spec.rb @@ -434,7 +434,10 @@ def approve(extra_params = {}) expect(response).to have_gitlab_http_status(200) expect(json_response['approvals_required']).to eq 2 expect(json_response['approvals_left']).to eq 2 - expect(json_response['approval_rules_left']).to eq(['foo']) + + short_approval = { "id" => rule.id, "name" => rule.name, "rule_type" => rule.rule_type.to_s } + expect(json_response['approval_rules_left']).to eq([short_approval]) + expect(json_response['approved_by']).to be_empty expect(json_response['user_can_approve']).to be true expect(json_response['user_has_approved']).to be false -- GitLab From ea91770aad622682dbd060a77a40c6e4fc6cbe41 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Thu, 7 Feb 2019 07:34:19 -0600 Subject: [PATCH 2/2] Update FE to support single rule mode - Also cleans up some of the approval specs **What is single rule mode?** Multi-rule is only available on GitLab Ultimate. This should behave very similarly to the pre-existing behavior (11.7). When in single mode, we don't "Add rules", we just edit the one rule. In the BE, technically this is sometimes the fallback rule (no approvers) or just a regular rule. The fallback properties have been moved to their own rule, so there's some updates on the mappers to support that. --- .../javascripts/approvals/components/app.vue | 2 +- .../approvals/components/fallback_rules.vue | 2 +- .../components/mr_edit/mr_fallback_rules.vue | 2 +- .../approvals/components/mr_edit/mr_rules.vue | 19 +- .../project_settings/project_rules.vue | 33 +- .../approvals/components/rule_controls.vue | 5 +- .../approvals/components/rule_form.vue | 84 ++- .../assets/javascripts/approvals/constants.js | 4 + .../assets/javascripts/approvals/mappers.js | 43 +- .../javascripts/approvals/mount_mr_edit.js | 3 +- .../approvals/mount_project_settings.js | 4 +- .../javascripts/approvals/stores/state.js | 1 + .../approvals/multiple_rule/approvals.vue | 2 +- .../vue_merge_request_widget/mappers.js | 42 ++ .../stores/mr_widget_store.js | 21 +- .../_approvals_multiple_rule.html.haml | 1 + .../_multiple_rules_form.html.haml | 3 +- .../approvals/components/app_spec.js | 180 ++++--- .../mr_edit/mr_fallback_rules_spec.js | 2 +- .../components/mr_edit/mr_rules_spec.js | 165 +++--- .../project_settings/project_rules_spec.js | 84 ++- .../components/rule_controls_spec.js | 108 ++-- .../approvals/components/rule_form_spec.js | 481 +++++++++++++----- locale/gitlab.pot | 10 + 24 files changed, 882 insertions(+), 419 deletions(-) create mode 100644 ee/app/assets/javascripts/vue_merge_request_widget/mappers.js diff --git a/ee/app/assets/javascripts/approvals/components/app.vue b/ee/app/assets/javascripts/approvals/components/app.vue index ada536a7cc7aa5..d76487e3cd41e6 100644 --- a/ee/app/assets/javascripts/approvals/components/app.vue +++ b/ee/app/assets/javascripts/approvals/components/app.vue @@ -47,7 +47,7 @@ export default {
-
+
{{ __('Add approvers') }} diff --git a/ee/app/assets/javascripts/approvals/components/fallback_rules.vue b/ee/app/assets/javascripts/approvals/components/fallback_rules.vue index eb9243307dafcb..54bb2ec7b9262b 100644 --- a/ee/app/assets/javascripts/approvals/components/fallback_rules.vue +++ b/ee/app/assets/javascripts/approvals/components/fallback_rules.vue @@ -50,7 +50,7 @@ export default { {{ s__('ApprovalRule|All members with Developer role or higher and code owners (if any)') }} - + diff --git a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules.vue b/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules.vue index f8dcb048d2daf2..abf164bafccaec 100644 --- a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules.vue +++ b/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules.vue @@ -18,6 +18,11 @@ export default { }, methods: { ...mapActions(['putRule']), + canEdit(rule) { + const { canEdit, allowMultiRule } = this.settings; + + return canEdit && (!allowMultiRule || !rule.hasSource); + }, }, }; @@ -26,15 +31,17 @@ export default { diff --git a/ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue b/ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue index 85b2df5b69e5f5..b48452b0819259 100644 --- a/ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue +++ b/ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue @@ -14,12 +14,37 @@ export default { RuleControls, }, computed: { + ...mapState(['settings']), ...mapState({ rules: state => state.approvals.rules, }), }, methods: { summaryText(rule) { + return this.settings.allowMultiRule + ? this.summaryMultipleRulesText(rule) + : this.summarySingleRuleText(rule); + }, + membersCountText(rule) { + return n__( + 'ApprovalRuleSummary|%d member', + 'ApprovalRuleSummary|%d members', + rule.approvers.length, + ); + }, + summarySingleRuleText(rule) { + const membersCount = this.membersCountText(rule); + + return sprintf( + n__( + 'ApprovalRuleSummary|%{count} approval required from %{membersCount}', + 'ApprovalRuleSummary|%{count} approvals required from %{membersCount}', + rule.approvalsRequired, + ), + { membersCount, count: rule.approvalsRequired }, + ); + }, + summaryMultipleRulesText(rule) { return sprintf( n__( '%{count} approval required from %{name}', @@ -37,16 +62,16 @@ export default {