diff --git a/ee/app/assets/javascripts/approvals/components/app.vue b/ee/app/assets/javascripts/approvals/components/app.vue index 763cb49df3539c83db122d7d6db506165c370d1d..c337660b79eead4497e083a2c730773ff7f44fc2 100644 --- a/ee/app/assets/javascripts/approvals/components/app.vue +++ b/ee/app/assets/javascripts/approvals/components/app.vue @@ -1,9 +1,8 @@ + + diff --git a/ee/app/assets/javascripts/approvals/components/fallback_rules.vue b/ee/app/assets/javascripts/approvals/components/fallback_rules.vue deleted file mode 100644 index 54bb2ec7b9262b9c491d5b03da57ecf67108104c..0000000000000000000000000000000000000000 --- a/ee/app/assets/javascripts/approvals/components/fallback_rules.vue +++ /dev/null @@ -1,68 +0,0 @@ - - - diff --git a/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue b/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue index 55ae63e02ce983315be4d4f6a75f87a9225918c6..68c9db7d3c4c57df76210b494fa73e4abd3fe301 100644 --- a/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue +++ b/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue @@ -2,21 +2,18 @@ import App from '../app.vue'; import MrRules from './mr_rules.vue'; import MrRulesHiddenInputs from './mr_rules_hidden_inputs.vue'; -import MrFallbackRules from './mr_fallback_rules.vue'; export default { components: { App, MrRules, MrRulesHiddenInputs, - MrFallbackRules, }, }; diff --git a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue b/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue index 5e69193dcbc40ac10bd7c240408f12839a644ae8..983bfe9ff0c850ab33d80851e713dbb4e2b55083 100644 --- a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue +++ b/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue @@ -47,6 +47,8 @@ export default { />
+ + +import { mapState, mapActions } from 'vuex'; +import { RULE_TYPE_ANY_APPROVER } from '../../constants'; +import Icon from '~/vue_shared/components/icon.vue'; + +const ANY_RULE_NAME = 'All Members'; + +export default { + components: { + Icon, + }, + props: { + rule: { + type: Object, + required: true, + }, + isMrEdit: { + type: Boolean, + required: false, + default: true, + }, + }, + computed: { + ...mapState(['settings']), + }, + methods: { + ...mapActions(['putRule', 'postRule']), + onInputChange(event) { + if (this.rule.id) { + this.putRule({ id: this.rule.id, approvalsRequired: Number(event.target.value) }); + } else { + this.postRule({ + name: ANY_RULE_NAME, + ruleType: RULE_TYPE_ANY_APPROVER, + approvalsRequired: Number(event.target.value), + }); + } + }, + }, +}; + + + 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 e63a8b62002aefced831233376f99bb68edd01ed..77d0f848297f20315a47ef938c6674756c375f0c 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 @@ -1,11 +1,14 @@ @@ -64,29 +97,40 @@ export default { - diff --git a/ee/app/assets/javascripts/approvals/components/rule_controls.vue b/ee/app/assets/javascripts/approvals/components/rule_controls.vue index 4a123f35474dbe377597454202dab5ee100c58b8..80dec5d97fa5f75a1d046ba9ab77a76543e29d03 100644 --- a/ee/app/assets/javascripts/approvals/components/rule_controls.vue +++ b/ee/app/assets/javascripts/approvals/components/rule_controls.vue @@ -16,9 +16,6 @@ export default { }, computed: { ...mapState(['settings']), - isRemoveable() { - return !this.rule.isFallback && this.settings.allowMultiRule; - }, }, methods: { ...mapActions(['requestEditRule', 'requestDeleteRule']), @@ -29,9 +26,9 @@ export default { diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js index 616057c32071ff9c9044245954f1b7d6c23d1eda..127a97e27aa160992758046da62f46fe412b42f7 100644 --- a/ee/app/assets/javascripts/approvals/constants.js +++ b/ee/app/assets/javascripts/approvals/constants.js @@ -8,6 +8,8 @@ export const RULE_TYPE_FALLBACK = 'fallback'; export const RULE_TYPE_REGULAR = 'regular'; export const RULE_TYPE_REPORT_APPROVER = 'report_approver'; export const RULE_TYPE_CODE_OWNER = 'code_owner'; +export const RULE_TYPE_ANY_APPROVER = 'any_approver'; +export const RULE_NAME_ANY_APPROVER = 'All Members'; export const VULNERABILITY_CHECK_NAME = 'Vulnerability-Check'; export const LICENSE_CHECK_NAME = 'License-Check'; diff --git a/ee/app/assets/javascripts/approvals/mappers.js b/ee/app/assets/javascripts/approvals/mappers.js index 51418a1e9e6600fcdbcd56ba9c57be6bda77c4f5..6255c2c7c3f1b34bf9e7c793a87bca0eccb37462 100644 --- a/ee/app/assets/javascripts/approvals/mappers.js +++ b/ee/app/assets/javascripts/approvals/mappers.js @@ -1,5 +1,26 @@ -import _ from 'underscore'; -import { RULE_TYPE_REGULAR, RULE_TYPE_FALLBACK } from './constants'; +import { RULE_TYPE_REGULAR, RULE_TYPE_ANY_APPROVER } from './constants'; + +const visibleTypes = new Set([RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR]); + +function withDefaultEmptyRule(rules = []) { + if (rules && rules.length > 0) { + return rules; + } + + return [ + { + id: null, + name: '', + approvalsRequired: 0, + minApprovalsRequired: 0, + approvers: [], + containsHiddenGroups: false, + users: [], + groups: [], + ruleType: RULE_TYPE_ANY_APPROVER, + }, + ]; +} export const mapApprovalRuleRequest = req => ({ name: req.name, @@ -23,10 +44,11 @@ export const mapApprovalRuleResponse = res => ({ containsHiddenGroups: res.contains_hidden_groups, users: res.users, groups: res.groups, + ruleType: res.rule_type, }); export const mapApprovalSettingsResponse = res => ({ - rules: res.rules.map(mapApprovalRuleResponse), + rules: withDefaultEmptyRule(res.rules.map(mapApprovalRuleResponse)), fallbackApprovalsRequired: res.fallback_approvals_required, }); @@ -52,19 +74,16 @@ export const mapMRSourceRule = ({ id, ...rule }) => ({ * from the fallback rule. */ export const mapMRApprovalSettingsResponse = res => { - const rulesByType = _.groupBy(res.rules, x => x.rule_type); - - const regularRules = rulesByType[RULE_TYPE_REGULAR] || []; + const rules = res.rules.filter(({ rule_type }) => visibleTypes.has(rule_type)); - const [fallback] = rulesByType[RULE_TYPE_FALLBACK] || []; - const fallbackApprovalsRequired = fallback - ? fallback.approvals_required - : res.fallback_approvals_required || 0; + const fallbackApprovalsRequired = res.fallback_approvals_required || 0; return { - rules: regularRules - .map(mapApprovalRuleResponse) - .map(res.approval_rules_overwritten ? x => x : mapMRSourceRule), + rules: withDefaultEmptyRule( + rules + .map(mapApprovalRuleResponse) + .map(res.approval_rules_overwritten ? x => x : mapMRSourceRule), + ), fallbackApprovalsRequired, minFallbackApprovalsRequired: 0, }; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js b/ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js index 3e6a8207c2eb19a0c84a2c34a6d5626102988432..b128a93cfe4f9d429e34bf37491ec883b6102950 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js @@ -1,2 +1,3 @@ export const SET_LOADING = 'SET_LOADING'; export const SET_APPROVAL_SETTINGS = 'SET_APPROVAL_SETTINGS'; +export const ADD_EMPTY_RULE = 'ADD_EMPTY_RULE'; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js b/ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js index 236e10797ba3f9a37a524b9ba87ecba1b70d4803..c12f3877d57642db0324de97c6a7c7e11b464499 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js @@ -1,4 +1,5 @@ import * as types from './mutation_types'; +import { RULE_TYPE_ANY_APPROVER } from '../../../constants'; export default { [types.SET_LOADING](state, isLoading) { @@ -7,7 +8,22 @@ export default { [types.SET_APPROVAL_SETTINGS](state, settings) { state.hasLoaded = true; state.rules = settings.rules; + state.initialRules = settings.rules; state.fallbackApprovalsRequired = settings.fallbackApprovalsRequired; state.minFallbackApprovalsRequired = settings.minFallbackApprovalsRequired; }, + [types.ADD_EMPTY_RULE](state) { + state.rules.unshift({ + id: null, + name: '', + approvalsRequired: 0, + minApprovalsRequired: 0, + approvers: [], + containsHiddenGroups: false, + users: [], + groups: [], + ruleType: RULE_TYPE_ANY_APPROVER, + isNew: true, + }); + }, }; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/base/state.js b/ee/app/assets/javascripts/approvals/stores/modules/base/state.js index 2b2b6ec45575890ec8589ace325b39a5fc52a22d..0dc86ef4375526fd31734f5943d918fc7c992d5b 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/base/state.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/state.js @@ -4,4 +4,5 @@ export default () => ({ rules: [], fallbackApprovalsRequired: 0, minFallbackApprovalsRequired: 0, + initialRules: [], }); diff --git a/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/actions.js index 9b20e67dd864c849c305017f019e2d9b8e6e0cc1..5acf876d767f57cf951a165a7648db51a51ce084 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/actions.js @@ -4,6 +4,7 @@ import { __ } from '~/locale'; import Api from '~/api'; import axios from '~/lib/utils/axios_utils'; import * as types from './mutation_types'; +import { RULE_TYPE_ANY_APPROVER } from '../../../constants'; import { mapMRApprovalSettingsResponse } from '../../../mappers'; const fetchGroupMembers = _.memoize(id => Api.groupMembers(id).then(response => response.data)); @@ -36,11 +37,16 @@ const seedLocalRule = rule => .then(seedUsers) .then(seedGroups); -const seedNewRule = rule => ({ - ...rule, - isNew: true, - id: _.uniqueId('new'), -}); +const seedNewRule = rule => { + const name = rule.ruleType === RULE_TYPE_ANY_APPROVER ? '' : rule.name; + + return { + ...rule, + isNew: true, + name, + id: _.uniqueId('new'), + }; +}; export const requestRules = ({ commit }) => { commit(types.SET_LOADING, true); @@ -113,4 +119,25 @@ export const requestDeleteRule = ({ dispatch }, rule) => { dispatch('deleteRule', rule.id); }; +export const postRegularRule = ({ commit, dispatch }, rule) => + seedLocalRule(rule) + .then(seedNewRule) + .then(newRule => { + commit(types.POST_REGULAR_RULE, newRule); + commit(types.DELETE_ANY_RULE); + dispatch('createModal/close'); + }) + .catch(e => { + createFlash(__('An error occurred fetching the approvers for the new rule.')); + throw e; + }); + +export const setEmptyRule = ({ commit }) => { + commit(types.SET_EMPTY_RULE); +}; + +export const addEmptyRule = ({ commit }) => { + commit(types.ADD_EMPTY_RULE); +}; + export default () => {}; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutation_types.js b/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutation_types.js index a278fbeccd646539dc6899a206b40229d8c7b1e8..87c340d0c7315869c3e72f9a1df3e265d4e0bf55 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutation_types.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutation_types.js @@ -4,3 +4,6 @@ export const DELETE_RULE = 'DELETE_RULE'; export const PUT_RULE = 'PUT_RULE'; export const POST_RULE = 'POST_RULE'; export const SET_FALLBACK_RULE = 'SET_FALLBACK_RULE'; +export const POST_REGULAR_RULE = 'POST_REGULAR_RULE'; +export const DELETE_ANY_RULE = 'DELETE_ANY_RULE'; +export const SET_EMPTY_RULE = 'SET_EMPTY_RULE'; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutations.js b/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutations.js index 55da0ca0e1c753ab5b6f81a8212bc67d2d13882f..6c7098a411409037a68ec2c96451c3655a64d9d4 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutations.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutations.js @@ -1,6 +1,7 @@ import _ from 'underscore'; import base from '../base/mutations'; import * as types from './mutation_types'; +import { RULE_TYPE_ANY_APPROVER } from '../../../constants'; export default { ...base, @@ -20,6 +21,19 @@ export default { state.rules.splice(idx, 1); }, + [types.DELETE_ANY_RULE](state) { + const [newRule, oldRule] = state.rules; + + if (!newRule && !oldRule) { + return; + } + + if (!oldRule.isNew) { + state.rulesToDelete.push(oldRule.id); + } + + state.rules = [newRule]; + }, [types.PUT_RULE](state, { id, ...newRule }) { const idx = _.findIndex(state.rules, x => x.id === id); @@ -31,9 +45,45 @@ export default { state.rules.splice(idx, 1, rule); }, [types.POST_RULE](state, rule) { - state.rules.push(rule); + const [firstRule] = state.rules; + + if ( + firstRule && + firstRule.ruleType === RULE_TYPE_ANY_APPROVER && + rule.ruleType === RULE_TYPE_ANY_APPROVER + ) { + state.rules = [rule]; + } else { + state.rules.push(rule); + } + }, + [types.POST_REGULAR_RULE](state, rule) { + state.rules.unshift(rule); }, [types.SET_FALLBACK_RULE](state, fallback) { state.fallbackApprovalsRequired = fallback.approvalsRequired; }, + [types.SET_EMPTY_RULE](state) { + const anyRule = state.initialRules.find(rule => rule.ruleType === RULE_TYPE_ANY_APPROVER); + + if (anyRule) { + state.rules = [anyRule]; + state.rulesToDelete = []; + } else { + state.rules = [ + { + id: null, + name: '', + approvalsRequired: 0, + minApprovalsRequired: 0, + approvers: [], + containsHiddenGroups: false, + users: [], + groups: [], + ruleType: RULE_TYPE_ANY_APPROVER, + isNew: true, + }, + ]; + } + }, }; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js index 79f9e7d93c2436c9ab15d977ec8dba233653aca6..21ac5c21eec58fb010fe2eac1b500c87e7b544be 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js @@ -103,4 +103,8 @@ export const requestDeleteRule = ({ dispatch }, rule) => { dispatch('deleteModal/open', rule); }; +export const addEmptyRule = ({ commit }) => { + commit(types.ADD_EMPTY_RULE); +}; + export default () => {}; diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue index b86ced27083c7ffc0f1c21dd6c538062c0c96426..a3d400a38f4793bb499a9e682d4620356e8de44a 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue +++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue @@ -50,7 +50,7 @@ export default { return this.mr.approvals || {}; }, hasFooter() { - return Boolean(this.approvals.has_approval_rules); + return Boolean(this.mr.approvals); }, approvedBy() { return this.approvals.approved_by ? this.approvals.approved_by.map(x => x.user) : []; diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue index 59d2603c15450223a29dd8e766a518fa068f6b39..066dd14fdd161e11dfbd1ddfc14b6c4bbce218b9 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue +++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue @@ -3,7 +3,7 @@ import _ from 'underscore'; import { sprintf, __ } from '~/locale'; import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; import ApprovalCheckRulePopover from 'ee/approvals/components/approval_check_rule_popover.vue'; -import { RULE_TYPE_CODE_OWNER } from 'ee/approvals/constants'; +import { RULE_TYPE_CODE_OWNER, RULE_TYPE_ANY_APPROVER } from 'ee/approvals/constants'; import ApprovedIcon from './approved_icon.vue'; export default { @@ -70,6 +70,9 @@ export default { name: rule.name, }); }, + ruleName(rule) { + return rule.rule_type === RULE_TYPE_ANY_APPROVER ? __('Any eligible user') : rule.name; + }, }, }; @@ -96,7 +99,7 @@ export default {
- {{ rule.name }} + {{ ruleName(rule) }} = 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_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index 08727c882fe58dcd48125b2c674db22b4c6a91a0..055a14f78eb9a776e3fe635bdbb88ac8cb2c92fe 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -116,7 +116,9 @@ def sync_approved_approvers # Before being merged, approved_approvers are dynamically calculated in ApprovalWrappedRule instead of being persisted. return unless merge_request.merged? - self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id) + approvers = ApprovalWrappedRule.wrap(merge_request, self).approved_approvers + + self.approved_approver_ids = approvers.map(&:id) end def refresh_required_approvals!(project_approval_rule) diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index fbd7041696d905eaa2b48d75f7b6c05e246fe863..a1cf7c507a60a873e4e89f626fb46c7dd82fe269 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -44,30 +44,13 @@ def wrapped_approval_rules strong_memoize(:wrapped_approval_rules) do next [] unless approval_feature_available? - result = use_fallback? ? [fallback_rule] : regular_rules - result += code_owner_rules - result += report_approver_rules - result + user_defined_rules + code_owner_rules + report_approver_rules end end - def has_non_fallback_rules? - has_regular_rule_with_approvers? || code_owner_rules.present? || report_approver_rules.present? - end - - # Use the fallback rule if regular rules are empty - def use_fallback? - !has_regular_rule_with_approvers? - end - - def fallback_rule - @fallback_rule ||= ApprovalMergeRequestFallback.new(merge_request) - end - # Determines which set of rules to use (MR or project) def approval_rules_overwritten? - regular_merge_request_rules.any? { |rule| rule.approvers.present? } || - (project.can_override_approvers? && merge_request.approvals_before_merge.present?) + project.can_override_approvers? && user_defined_merge_request_rules.any? end alias_method :approvers_overwritten?, :approval_rules_overwritten? @@ -83,10 +66,6 @@ def approved? end end - def any_approver_allowed? - !has_regular_rule_with_approvers? || approved? - end - def approvals_required strong_memoize(:approvals_required) do wrapped_approval_rules.sum(&:approvals_required) @@ -109,16 +88,12 @@ def approvers strong_memoize(:approvers) { filtered_approvers(target: :approvers) } end - # @param regular [Boolean] # @param code_owner [Boolean] - # @param report_approver [Boolean] # @param target [:approvers, :users] # @param unactioned [Boolean] - def filtered_approvers(regular: true, code_owner: true, report_approver: true, target: :approvers, unactioned: false) - rules = [] - rules.concat(regular_rules) if regular + def filtered_approvers(code_owner: true, target: :approvers, unactioned: false) + rules = user_defined_rules + report_approver_rules rules.concat(code_owner_rules) if code_owner - rules.concat(report_approver_rules) if report_approver filter_approvers(rules.flat_map(&target), unactioned: unactioned) end @@ -142,7 +117,6 @@ def can_approve?(user) return false unless user.can?(:approve_merge_request, merge_request) return true if unactioned_approvers.include?(user) - return false unless any_approver_allowed? # Users can only approve once. return false if approvals.where(user: user).any? # At this point, follow self-approval rules. Otherwise authors must @@ -171,9 +145,22 @@ def committers_can_approve? # This is a temporary method for backward compatibility # before introduction of approval rules. # This avoids re-queries. + # https://gitlab.com/gitlab-org/gitlab/issues/33329 def first_regular_rule strong_memoize(:first_regular_rule) do - regular_rules.first + user_defined_rules.first + end + end + + def user_defined_rules + strong_memoize(:user_defined_rules) do + if approval_rules_overwritten? + user_defined_merge_request_rules + else + project.visible_user_defined_rules.map do |rule| + ApprovalWrappedRule.wrap(merge_request, rule) + end + end end end @@ -187,43 +174,39 @@ def filter_approvers(approvers, unactioned:) self.class.filter_committers(approvers, merge_request) end - def has_regular_rule_with_approvers? - regular_rules.any? { |rule| rule.approvers.present? } - end + def user_defined_merge_request_rules + strong_memoize(:user_defined_merge_request_rules) do + # Filter out the rules without approvers since such rules aren't useful + regular_rules = + wrapped_rules + .select { |rule| rule.regular? && rule.approvers.present? } + .sort_by(&:id) - def regular_rules - strong_memoize(:regular_rules) do - rules = approval_rules_overwritten? ? regular_merge_request_rules : regular_project_rules + any_approver_rules = + wrapped_rules.select(&:any_approver?) - unless project.multiple_approval_rules_available? - rules = rules[0, 1] - end - - wrap_rules(rules) + rules = any_approver_rules + regular_rules + project.multiple_approval_rules_available? ? rules : rules.take(1) 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?)) + wrapped_rules.select(&:code_owner?) end end def report_approver_rules strong_memoize(:report_approver_rules) do - wrap_rules(merge_request.approval_rules.select(&:report_approver?)) + wrapped_rules.select(&:report_approver?) end end - def wrap_rules(rules) - rules.map { |rule| ApprovalWrappedRule.new(merge_request, rule) } + def wrapped_rules + strong_memoize(:wrapped_rules) do + merge_request.approval_rules.map do |rule| + ApprovalWrappedRule.wrap(merge_request, rule) + end + end end end diff --git a/ee/app/models/approval_wrapped_any_approver_rule.rb b/ee/app/models/approval_wrapped_any_approver_rule.rb new file mode 100644 index 0000000000000000000000000000000000000000..27818452451c93a5eb871755ad2b5137d99b6e37 --- /dev/null +++ b/ee/app/models/approval_wrapped_any_approver_rule.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +# A common state computation interface to wrap around any_approver rule +class ApprovalWrappedAnyApproverRule < ApprovalWrappedRule + def name + 'All Members' + end + + def approved_approvers + filter_approvers(merge_request.approved_by_users) + end + + def approved? + strong_memoize(:approved) do + approvals_left <= 0 + end + end +end diff --git a/ee/app/models/approval_wrapped_code_owner_rule.rb b/ee/app/models/approval_wrapped_code_owner_rule.rb new file mode 100644 index 0000000000000000000000000000000000000000..951c4c9c0f8a4e4f5098f27948e945234c26b466 --- /dev/null +++ b/ee/app/models/approval_wrapped_code_owner_rule.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# A common state computation interface to wrap around code owner rule +class ApprovalWrappedCodeOwnerRule < ApprovalWrappedRule + REQUIRED_APPROVALS_PER_CODE_OWNER_RULE = 1 + + def approvals_required + strong_memoize(:code_owner_approvals_required) do + next 0 unless branch_requires_code_owner_approval? + + approvers.any? ? REQUIRED_APPROVALS_PER_CODE_OWNER_RULE : 0 + end + end + + private + + def branch_requires_code_owner_approval? + return false unless project.code_owner_approval_required_available? + + ProtectedBranch.branch_requires_code_owner_approval?(project, merge_request.target_branch) + end +end diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index 4fa106aac60414e01312b0de10c3f3dc04ec4d45..694fb2b78233fdb3697257644b85edd4103c18f9 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -5,14 +5,23 @@ class ApprovalWrappedRule extend Forwardable include Gitlab::Utils::StrongMemoize - REQUIRED_APPROVALS_PER_CODE_OWNER_RULE = 1 - attr_reader :merge_request attr_reader :approval_rule def_delegators(:@approval_rule, - :id, :name, :users, :groups, :code_owner, :code_owner?, :source_rule, - :rule_type) + :regular?, :any_approver?, :code_owner?, :report_approver?, + :id, :name, :users, :groups, :code_owner, :source_rule, + :rule_type, :approvals_required) + + def self.wrap(merge_request, rule) + if rule.any_approver? + ApprovalWrappedAnyApproverRule.new(merge_request, rule) + elsif rule.code_owner? + ApprovalWrappedCodeOwnerRule.new(merge_request, rule) + else + ApprovalWrappedRule.new(merge_request, rule) + end + end def initialize(merge_request, approval_rule) @merge_request = merge_request @@ -24,10 +33,7 @@ def project end def approvers - filtered_approvers = - ApprovalState.filter_author(@approval_rule.approvers, merge_request) - - ApprovalState.filter_committers(filtered_approvers, merge_request) + filter_approvers(@approval_rule.approvers) end # @return [Array] all approvers related to this rule @@ -47,7 +53,7 @@ def approved_approvers end strong_memoize(:approved_approvers) do - overall_approver_ids = merge_request.approvals.map(&:user_id) + overall_approver_ids = merge_request.approvals.map(&:user_id).to_set approvers.select do |approver| overall_approver_ids.include?(approver.id) @@ -80,27 +86,12 @@ def unactioned_approvers approvers - approved_approvers end - def approvals_required - if code_owner? - code_owner_approvals_required - else - approval_rule.approvals_required - end - end - private - def code_owner_approvals_required - strong_memoize(:code_owner_approvals_required) do - next 0 unless branch_requires_code_owner_approval? - - approvers.any? ? REQUIRED_APPROVALS_PER_CODE_OWNER_RULE : 0 - end - end - - def branch_requires_code_owner_approval? - return false unless project.code_owner_approval_required_available? + def filter_approvers(approvers) + filtered_approvers = + ApprovalState.filter_author(approvers, merge_request) - ProtectedBranch.branch_requires_code_owner_approval?(project, merge_request.target_branch) + ApprovalState.filter_committers(filtered_approvers, merge_request) end end diff --git a/ee/app/models/concerns/approvable.rb b/ee/app/models/concerns/approvable.rb index 8c54dc7dca329f323e95a1d6d9e56b3c1a93f0cf..917f8e2a380e1daaf3fa76a635d9a454829d5e04 100644 --- a/ee/app/models/concerns/approvable.rb +++ b/ee/app/models/concerns/approvable.rb @@ -13,7 +13,6 @@ module Approvable approvals_left can_approve? has_approved? - any_approver_allowed? authors_can_approve? committers_can_approve? approvers_overwritten? diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 839c4820b2fa46b435f0ed69b0fd427e4fbc4ddf..2cda0f182675fd83fbc2ad24d6d51cf9ccdb17ad 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -22,6 +22,7 @@ module ApprovalRuleLike validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } scope :with_users, -> { preload(:users, :group_users) } + scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) } end # Users who are eligible to approve, including specified group members. @@ -45,4 +46,8 @@ def regular? def report_approver? raise NotImplementedError end + + def any_approver? + raise NotImplementedError + end end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index a0143bdbf9cd1ab5fa792193d323fc414acdc741..2e95c807b8602b0ceabbaa4cb05765553ebcebd6 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -390,6 +390,7 @@ def reference_issue_tracker? default_issues_tracker? || jira_tracker_active? end + # TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329 def approvals_before_merge return 0 unless feature_available?(:merge_request_approvers) @@ -398,24 +399,24 @@ def approvals_before_merge def visible_approval_rules strong_memoize(:visible_approval_rules) do - visible_regular_approval_rules + approval_rules.report_approver + visible_user_defined_rules + approval_rules.report_approver end end - def visible_regular_approval_rules - strong_memoize(:visible_regular_approval_rules) do - regular_rules = approval_rules.regular.order(:id) + def visible_user_defined_rules + strong_memoize(:visible_user_defined_rules) do + rules = approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc) - next regular_rules.take(1) unless multiple_approval_rules_available? + next rules.take(1) unless multiple_approval_rules_available? - regular_rules + rules end end + # TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329 def min_fallback_approvals strong_memoize(:min_fallback_approvals) do - visible_regular_approval_rules.map(&:approvals_required).max || - approvals_before_merge.to_i + visible_user_defined_rules.map(&:approvals_required).max.to_i end end diff --git a/ee/app/services/approval_rules/create_service.rb b/ee/app/services/approval_rules/create_service.rb index 65879211dba150a6fd9e0c0d2cee830a9703a884..3c0d4250937f2678137db5f60469866c93919fc1 100644 --- a/ee/app/services/approval_rules/create_service.rb +++ b/ee/app/services/approval_rules/create_service.rb @@ -14,6 +14,7 @@ def initialize(target, user, params) params.reverse_merge!(rule_type: :report_approver) end + handle_any_approver_rule_creation(target, params) if target.is_a?(Project) copy_approval_project_rule_properties(params) if target.is_a?(MergeRequest) super(@rule.project, user, params) @@ -36,5 +37,17 @@ def copy_approval_project_rule_properties(params) params[:users] = approval_project_rule.users params[:groups] = approval_project_rule.groups end + + def handle_any_approver_rule_creation(target, params) + if params[:user_ids].blank? && params[:group_ids].blank? + params.reverse_merge!(rule_type: :any_approver, name: ApprovalRuleLike::ALL_MEMBERS) + + return + end + + return if target.multiple_approval_rules_available? + + target.approval_rules.any_approver.delete_all + end end end diff --git a/ee/app/services/approval_rules/params_filtering_service.rb b/ee/app/services/approval_rules/params_filtering_service.rb index dbb05c116e9b79d7c7310495d93c07827304b032..e53e4ce62b8e45d47b5f02a624722a51d1c05c6b 100644 --- a/ee/app/services/approval_rules/params_filtering_service.rb +++ b/ee/app/services/approval_rules/params_filtering_service.rb @@ -46,6 +46,11 @@ def handle_rule(rule_attributes) provided_user_ids = rule_attributes[:user_ids].map(&:to_i) rule_attributes[:user_ids] = provided_user_ids & visible_user_ids end + + if rule_attributes[:group_ids].blank? && rule_attributes[:user_ids].blank? + rule_attributes[:rule_type] = :any_approver + rule_attributes[:name] = ApprovalRuleLike::ALL_MEMBERS + end end # Append hidden groups to params to prevent them from being removed, diff --git a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml index a187c7e541be362e8258fe3fe26cd569f7ab9afc..1034e3e28c3010796e0ee4ceb34aaba96f5ac6a8 100644 --- a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml +++ b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml @@ -8,6 +8,7 @@ 'settings_path': expose_path(api_v4_projects_approval_settings_path(id: @project.id)), 'rules_path': expose_path(api_v4_projects_approval_settings_rules_path(id: @project.id)), 'allow_multi_rule': @project.multiple_approval_rules_available?.to_s, + 'eligible_approvers_docs_path': help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'eligible-approvers'), 'security_approvals_help_page_path': help_page_path('user/application_security/index.html', anchor: 'security-approvals-in-merge-requests-ultimate')} } .text-center.prepend-top-default = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') diff --git a/ee/app/views/shared/issuable/_approvals.html.haml b/ee/app/views/shared/issuable/_approvals.html.haml index 0c7cd8343dd6aa3dd3eff4eecf2469dda7ef4319..7a60829dd2eec2e023d79ea5d4510079c1007852 100644 --- a/ee/app/views/shared/issuable/_approvals.html.haml +++ b/ee/app/views/shared/issuable/_approvals.html.haml @@ -16,6 +16,7 @@ 'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s, 'mr_id': issuable.iid, 'mr_settings_path': presenter.api_approval_settings_path, + 'eligible_approvers_docs_path': help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'eligible-approvers'), 'project_settings_path': presenter.api_project_approval_settings_path } } = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') - if can_update_approvers diff --git a/ee/changelogs/unreleased/id-allow-empty-rule.yml b/ee/changelogs/unreleased/id-allow-empty-rule.yml new file mode 100644 index 0000000000000000000000000000000000000000..9b875967740905a5821f173cb79b38cb78b46358 --- /dev/null +++ b/ee/changelogs/unreleased/id-allow-empty-rule.yml @@ -0,0 +1,5 @@ +--- +title: Add new approval rule type which allows anyone to approve +merge_request: 15378 +author: +type: added diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index 2355e7cadcdf8ed5b1891db9a319f0b869a50196..8f46e0aaa33087ebe29db456fce659b25fa4328c 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -530,7 +530,7 @@ class ApprovalState < Grape::Entity expose :approval_rules_left, using: ApprovalRuleShort expose :has_approval_rules do |approval_state| - approval_state.has_non_fallback_rules? + approval_state.user_defined_rules.present? end expose :merge_request_approvers_available do |approval_state| diff --git a/ee/spec/factories/approval_rules.rb b/ee/spec/factories/approval_rules.rb index 58328ba4c1e1c0c571d6abbbec0932a99ece3eff..cc65056ef35adde6a80659a91940366dd22390a6 100644 --- a/ee/spec/factories/approval_rules.rb +++ b/ee/spec/factories/approval_rules.rb @@ -29,6 +29,11 @@ end end + factory :any_approver_rule, parent: :approval_merge_request_rule do + rule_type { :any_approver } + name { "All Members" } + end + factory :approval_project_rule do project sequence(:name) { |n| "#{ApprovalRuleLike::DEFAULT_NAME}-#{n}" } diff --git a/ee/spec/features/merge_request/user_creates_merge_request_spec.rb b/ee/spec/features/merge_request/user_creates_merge_request_spec.rb index 148feb43b8486b238bb4608380272d273b5b2e28..b26864601c31bee912af227368c5c891b2e8f677 100644 --- a/ee/spec/features/merge_request/user_creates_merge_request_spec.rb +++ b/ee/spec/features/merge_request/user_creates_merge_request_spec.rb @@ -7,10 +7,7 @@ let(:approver) { create(:user) } let(:project) do - create(:project, - :repository, - approvals_before_merge: 1, - merge_requests_template: template_text) + create(:project, :repository, merge_requests_template: template_text) end let(:template_text) { "This merge request should contain the following." } let(:title) { "Some feature" } diff --git a/ee/spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb b/ee/spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb index a7a5528f671dbfbccd3e359c37a7f6767a4a92c5..589a714ae7ab3b1d0832a5ac1efcf70b820c8868 100644 --- a/ee/spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb +++ b/ee/spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb @@ -17,29 +17,11 @@ def select_source_branch(branch_name) sign_in(user) end - context 'when approvals are zero for the target project' do + context 'create a merge request for the selected branches' do before do - project.update(approvals_before_merge: 0) - visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature_conflict' }) end - it 'shows approval settings' do - expect(page).to have_content('Approvers') - end - end - - context 'when approvals are enabled for the target project' do - before do - project.update(approvals_before_merge: 1) - - visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature_conflict' }) - end - - it 'shows approval settings' do - expect(page).to have_content('Approvers') - end - context 'saving the MR' do it 'shows the saved MR' do fill_in 'merge_request_title', with: 'Test' diff --git a/ee/spec/features/merge_request/user_sets_approvers_spec.rb b/ee/spec/features/merge_request/user_sets_approvers_spec.rb index 9358255e18582ac2394500372655a55c88373bc4..b9ab00a93803258570a35db3fb3dd66608bdf990 100644 --- a/ee/spec/features/merge_request/user_sets_approvers_spec.rb +++ b/ee/spec/features/merge_request/user_sets_approvers_spec.rb @@ -7,7 +7,7 @@ include FeatureApprovalHelper let(:user) { create(:user) } - let(:project) { create(:project, :public, :repository, approvals_before_merge: 1) } + let(:project) { create(:project, :public, :repository) } let!(:config_selector) { '.js-approval-rules' } let!(:modal_selector) { '#mr-edit-approvals-create-modal' } @@ -24,7 +24,7 @@ end it 'does not allow setting the author as an approver but allows setting the current user as an approver' do - open_modal + open_modal(text: 'Add approval rule') open_approver_select expect(find('.select2-results')).not_to have_content(author.name) @@ -46,7 +46,7 @@ end it 'allows setting other users as approvers but does not allow setting the current user as an approver, and filters non members from approvers list', :sidekiq_might_not_need_inline do - open_modal + open_modal(text: 'Add approval rule') open_approver_select expect(find('.select2-results')).to have_content(other_user.name) @@ -71,7 +71,7 @@ visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' }) - open_modal + open_modal(text: 'Add approval rule') open_approver_select expect(find('.select2-results')).not_to have_content(group.name) @@ -84,7 +84,10 @@ find('.select2-results .user-result', text: group.name).click close_approver_select - click_button 'Update approval rule' + + within('.modal-content') do + click_button 'Add approval rule' + end click_on("Submit merge request") wait_for_all_requests @@ -137,7 +140,7 @@ visit edit_project_merge_request_path(project, merge_request) - open_modal + open_modal(text: 'Add approval rule') open_approver_select expect(find('.select2-results')).not_to have_content(group.name) @@ -150,7 +153,9 @@ find('.select2-results .user-result', text: group.name).click close_approver_select - click_button 'Update approval rule' + within('.modal-content') do + click_button 'Add approval rule' + end click_on("Save changes") wait_for_all_requests diff --git a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb index da65f3c8b7b56dcdb72a3b97ab70bbd146647f15..85d59389b2e03c2cc0ccbe716d288ff5e6c7d5a1 100644 --- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -6,7 +6,7 @@ include FeatureApprovalHelper let(:user) { create(:user) } - let(:project) { create(:project, approvals_before_merge: 1) } + let(:project) { create(:project) } let(:group) { create(:group) } let(:group_member) { create(:user) } let(:non_member) { create(:user) } @@ -23,7 +23,7 @@ it 'adds approver' do visit edit_project_path(project) - open_modal + open_modal(text: 'Add approval rule') open_approver_select expect(find('.select2-results')).to have_content(user.name) @@ -39,7 +39,9 @@ expect(find('.select2-results')).not_to have_content(user.name) close_approver_select - click_button 'Update approval rule' + within('.modal-content') do + click_button 'Add approval rule' + end wait_for_requests expect_avatar(find('.js-members'), user) @@ -48,7 +50,7 @@ it 'adds approver group' do visit edit_project_path(project) - open_modal + open_modal(text: 'Add approval rule') open_approver_select expect(find('.select2-results')).to have_content(group.name) @@ -58,7 +60,9 @@ expect(find('.content-list')).to have_content(group.name) - click_button 'Update approval rule' + within('.modal-content') do + click_button 'Add approval rule' + end wait_for_requests expect_avatar(find('.js-members'), group.users) diff --git a/ee/spec/frontend/approvals/components/empty_rule_name_spec.js b/ee/spec/frontend/approvals/components/empty_rule_name_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..2125d16902d929af68972ef6dd2f6a9483248e8f --- /dev/null +++ b/ee/spec/frontend/approvals/components/empty_rule_name_spec.js @@ -0,0 +1,39 @@ +import { shallowMount } from '@vue/test-utils'; +import EmptyRuleName from 'ee/approvals/components/empty_rule_name.vue'; +import { GlLink } from '@gitlab/ui'; + +describe('Empty Rule Name', () => { + let wrapper; + + const createComponent = (props = {}) => { + wrapper = shallowMount(EmptyRuleName, { + propsData: { + rule: {}, + eligibleApproversDocsPath: 'some/path', + ...props, + }, + sync: false, + }); + }; + + afterEach(() => { + wrapper.destroy(); + wrapper = null; + }); + + it('has a rule name "Any eligible user"', () => { + createComponent(); + + expect(wrapper.text()).toContain('Any eligible user'); + }); + + it('renders a "more information" link ', () => { + createComponent(); + + expect(wrapper.find(GlLink).attributes('href')).toBe( + wrapper.props('eligibleApproversDocsPath'), + ); + expect(wrapper.find(GlLink).exists()).toBe(true); + expect(wrapper.find(GlLink).text()).toBe('More information'); + }); +}); diff --git a/ee/spec/frontend/approvals/components/mr_edit/empty_rule_spec.js b/ee/spec/frontend/approvals/components/mr_edit/empty_rule_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..a2a97e9015a0e2ab0ae3d619f20ac1d024ed5b36 --- /dev/null +++ b/ee/spec/frontend/approvals/components/mr_edit/empty_rule_spec.js @@ -0,0 +1,51 @@ +import { shallowMount } from '@vue/test-utils'; +import EmptyRule from 'ee/approvals/components/mr_edit/empty_rule.vue'; +import { GlButton } from '@gitlab/ui'; + +describe('Empty Rule', () => { + let wrapper; + + const createComponent = (props = {}) => { + wrapper = shallowMount(EmptyRule, { + propsData: { + rule: {}, + ...props, + }, + sync: false, + }); + }; + + afterEach(() => { + wrapper.destroy(); + wrapper = null; + }); + + describe('multiple rules', () => { + it('does not display "Add approval rule" button', () => { + createComponent({ + allowMultiRule: true, + canEdit: true, + }); + expect(wrapper.find(GlButton).exists()).toBe(false); + }); + }); + + describe('single rule', () => { + it('displays "Add approval rule" button if allowed to edit', () => { + createComponent({ + allowMultiRule: false, + canEdit: true, + }); + + expect(wrapper.find(GlButton).exists()).toBe(true); + }); + + it('does not display "Add approval rule" button if not allowed to edit', () => { + createComponent({ + allowMultiRule: true, + canEdit: false, + }); + expect(wrapper.find(GlButton).exists()).toBe(false); + }); + }); +}); diff --git a/ee/spec/frontend/approvals/components/mr_edit/mr_fallback_rules_spec.js b/ee/spec/frontend/approvals/components/mr_edit/mr_fallback_rules_spec.js deleted file mode 100644 index 89be00305ed038ad392dba3c4c030d85265a7b31..0000000000000000000000000000000000000000 --- a/ee/spec/frontend/approvals/components/mr_edit/mr_fallback_rules_spec.js +++ /dev/null @@ -1,92 +0,0 @@ -import { mount, createLocalVue } from '@vue/test-utils'; -import Vuex from 'vuex'; -import { createStoreOptions } from 'ee/approvals/stores'; -import MREditModule from 'ee/approvals/stores/modules/mr_edit'; -import MRFallbackRules from 'ee/approvals/components/mr_edit/mr_fallback_rules.vue'; - -const localVue = createLocalVue(); -localVue.use(Vuex); - -const TEST_APPROVALS_REQUIRED = 3; -const TEST_MIN_APPROVALS_REQUIRED = 2; - -describe('EE Approvals MRFallbackRules', () => { - let wrapper; - let store; - - const factory = () => { - wrapper = mount(localVue.extend(MRFallbackRules), { - localVue, - store: new Vuex.Store(store), - sync: false, - }); - }; - - beforeEach(() => { - store = createStoreOptions(MREditModule()); - store.modules.approvals.state = { - hasLoaded: true, - rules: [], - minFallbackApprovalsRequired: TEST_MIN_APPROVALS_REQUIRED, - fallbackApprovalsRequired: TEST_APPROVALS_REQUIRED, - }; - store.modules.approvals.actions.putFallbackRule = jasmine.createSpy('putFallbackRule'); - }); - - afterEach(() => { - wrapper.destroy(); - }); - - const findInput = () => wrapper.find('input'); - - describe('if can not edit', () => { - beforeEach(() => { - store.state.settings.canEdit = false; - factory(); - }); - - it('input is disabled', () => { - expect(findInput().attributes('disabled')).toBe('disabled'); - }); - - it('input has value', () => { - expect(Number(findInput().element.value)).toBe(TEST_APPROVALS_REQUIRED); - }); - }); - - describe('if can edit', () => { - beforeEach(() => { - store.state.settings.canEdit = true; - factory(); - }); - - it('input is not disabled', () => { - expect(findInput().attributes('disabled')).toBe(undefined); - }); - - it('input has value', () => { - expect(Number(findInput().element.value)).toBe(TEST_APPROVALS_REQUIRED); - }); - - it('input has min value', () => { - expect(Number(findInput().attributes('min'))).toBe(TEST_MIN_APPROVALS_REQUIRED); - }); - - it('input dispatches putFallbackRule on change', () => { - const action = store.modules.approvals.actions.putFallbackRule; - const nextValue = TEST_APPROVALS_REQUIRED + 1; - - expect(action).not.toHaveBeenCalled(); - - findInput().setValue(nextValue); - - expect(action).toHaveBeenCalledWith( - jasmine.anything(), - { - approvalsRequired: nextValue, - }, - undefined, - ); - }); - }); -}); diff --git a/ee/spec/frontend/approvals/components/mr_edit/rule_input_spec.js b/ee/spec/frontend/approvals/components/mr_edit/rule_input_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..c1a90650e466bab6c238fc8edbe41a488cd46eab --- /dev/null +++ b/ee/spec/frontend/approvals/components/mr_edit/rule_input_spec.js @@ -0,0 +1,90 @@ +import Vuex from 'vuex'; +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import RuleInput from 'ee/approvals/components/mr_edit/rule_input.vue'; +import MREditModule from 'ee/approvals/stores/modules/mr_edit'; +import { createStoreOptions } from 'ee/approvals/stores'; + +const localVue = createLocalVue(); +localVue.use(Vuex); + +describe('Rule Input', () => { + let wrapper; + let store; + + const createComponent = (props = {}) => { + wrapper = shallowMount(RuleInput, { + propsData: { + rule: { + approvalsRequired: 9, + id: 5, + }, + ...props, + }, + localVue, + store: new Vuex.Store(store), + sync: false, + }); + }; + + beforeEach(() => { + store = createStoreOptions(MREditModule()); + store.state.settings.canEdit = true; + + store.modules.approvals.actions = { + putRule: jest.fn(), + }; + }); + + afterEach(() => { + wrapper.destroy(); + wrapper = null; + store = null; + }); + + it('has value equal to the approvalsRequired', () => { + createComponent(); + expect(Number(wrapper.element.value)).toBe(wrapper.props().rule.approvalsRequired); + }); + + it('is disabled when settings cannot edit ', () => { + store.state.settings.canEdit = false; + createComponent(); + + expect(wrapper.attributes().disabled).toBe('disabled'); + }); + + it('is disabled when settings can edit ', () => { + createComponent(); + + expect(wrapper.attributes().disabled).not.toBe('disabled'); + }); + + it('has min equal to the minApprovalsRequired', () => { + createComponent({ + rule: { + minApprovalsRequired: 4, + }, + }); + + expect(Number(wrapper.attributes().min)).toBe(wrapper.props().rule.minApprovalsRequired); + }); + + it('defaults min approvals required input to 0', () => { + createComponent(); + delete wrapper.props().rule.approvalsRequired; + expect(Number(wrapper.attributes('min'))).toEqual(0); + }); + + it('dispatches putRule on change', () => { + const action = store.modules.approvals.actions.putRule; + createComponent(); + wrapper.element.value = wrapper.props().rule.approvalsRequired + 1; + wrapper.trigger('input'); + + expect(action).toHaveBeenCalledWith( + expect.anything(), + { approvalsRequired: 10, id: 5 }, + undefined, + ); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/app_spec.js b/ee/spec/javascripts/approvals/components/app_spec.js index e10c0e33387e39618a6e8a281e6730bee0a2736f..57255bbf326646f0bbb4cf718a5a87f9e88f9b2b 100644 --- a/ee/spec/javascripts/approvals/components/app_spec.js +++ b/ee/spec/javascripts/approvals/components/app_spec.js @@ -96,10 +96,10 @@ describe('EE Approvals App', () => { }; }); - it('does not show Rules', () => { + it('does show Rules', () => { factory(); - expect(findRules().exists()).toBe(false); + expect(findRules().exists()).toBe(true); }); it('shows loading icon if loading', () => { diff --git a/ee/spec/javascripts/approvals/components/mr_edit/app_spec.js b/ee/spec/javascripts/approvals/components/mr_edit/app_spec.js index 06f07a73b5f86ef96d69f5edba1a17b66e0877d8..3ed10d16b85fc2df9b214237c19b0dfb31461adb 100644 --- a/ee/spec/javascripts/approvals/components/mr_edit/app_spec.js +++ b/ee/spec/javascripts/approvals/components/mr_edit/app_spec.js @@ -3,14 +3,13 @@ import Vuex from 'vuex'; import { createStoreOptions } from 'ee/approvals/stores'; import MREditModule from 'ee/approvals/stores/modules/mr_edit'; import MREditApp from 'ee/approvals/components/mr_edit/app.vue'; -import MRFallbackRules from 'ee/approvals/components/mr_edit/mr_fallback_rules.vue'; import MRRules from 'ee/approvals/components/mr_edit/mr_rules.vue'; import MRRulesHiddenInputs from 'ee/approvals/components/mr_edit/mr_rules_hidden_inputs.vue'; const localVue = createLocalVue(); localVue.use(Vuex); -describe('EE Approvlas MREditApp', () => { +describe('EE Approvals MREditApp', () => { let wrapper; let store; @@ -37,12 +36,8 @@ describe('EE Approvlas MREditApp', () => { factory(); }); - it('renders MR fallback rules', () => { - expect(wrapper.find(MRFallbackRules).exists()).toBe(true); - }); - it('does not render MR rules', () => { - expect(wrapper.find(MRRules).exists()).toBe(false); + expect(wrapper.find(MRRules).exists()).toBe(true); }); it('renders hidden inputs', () => { @@ -56,10 +51,6 @@ describe('EE Approvlas MREditApp', () => { factory(); }); - it('does not render MR fallback rules', () => { - expect(wrapper.find(MRFallbackRules).exists()).toBe(false); - }); - it('renders MR rules', () => { expect(wrapper.find(MRRules).exists()).toBe(true); }); diff --git a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js b/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js index 2b5e1d1c1127f34612c7bad95883d2e7da1c5dc8..ed5cf910c6ad18a26a84cfd431e9c2c7cfcc04f0 100644 --- a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js +++ b/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js @@ -163,10 +163,10 @@ describe('EE Approvlas MRRulesHiddenInputs', () => { rule.isNew = true; }); - it('does not render id input', () => { + it('does render id input', () => { factory(); - expect(findHiddenInputs().map(x => x.name)).not.toContain(INPUT_ID); + expect(findHiddenInputs().map(x => x.name)).toContain(INPUT_ID); }); describe('with source', () => { diff --git a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_spec.js b/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_spec.js index 31539b8faca34beef6bb5cae25d07d70dd23e1ac..5a5bf4f9ba1a75b383936bdc7f4209e1a0e76015 100644 --- a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_spec.js +++ b/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_spec.js @@ -6,7 +6,7 @@ import MRRules from 'ee/approvals/components/mr_edit/mr_rules.vue'; import Rules from 'ee/approvals/components/rules.vue'; import RuleControls from 'ee/approvals/components/rule_controls.vue'; import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; -import { createMRRule, createMRRuleWithSource } from '../../mocks'; +import { createEmptyRule, createMRRule, createMRRuleWithSource } from '../../mocks'; const { HEADERS } = Rules; @@ -37,7 +37,6 @@ describe('EE Approvals MRRules', () => { .find('td.js-members') .find(UserAvatarList) .props('items'); - const findRuleApprovalsRequired = () => wrapper.find('td.js-approvals-required input'); const findRuleControls = () => wrapper.find('td.js-controls').find(RuleControls); beforeEach(() => { @@ -59,9 +58,50 @@ describe('EE Approvals MRRules', () => { describe('when allow multiple rules', () => { beforeEach(() => { store.state.settings.allowMultiRule = true; + store.state.settings.eligibleApproversDocsPath = 'some/path'; }); - it('renders headers', () => { + it('should always have any_approver rule', () => { + store.modules.approvals.state.rules = [createMRRule()]; + factory(); + + expect(store.modules.approvals.state.rules.length).toBe(2); + }); + + it('should always display any_approver first', () => { + store.modules.approvals.state.rules = [createMRRule()]; + factory(); + + expect(store.modules.approvals.state.rules[0].ruleType).toBe('any_approver'); + }); + + it('should only have 1 any_approver', () => { + store.modules.approvals.state.rules = [createEmptyRule(), createMRRule()]; + factory(); + + const anyApproverCount = store.modules.approvals.state.rules.filter( + rule => rule.ruleType === 'any_approver', + ); + + expect(anyApproverCount.length).toBe(1); + }); + + it('renders headers when there are multiple rules', () => { + store.modules.approvals.state.rules = [createEmptyRule(), createMRRule()]; + factory(); + + expect(findHeaders()).toEqual([HEADERS.name, HEADERS.members, HEADERS.approvalsRequired, '']); + }); + + it('renders headers when there is a single any rule', () => { + store.modules.approvals.state.rules = [createEmptyRule()]; + factory(); + + expect(findHeaders()).toEqual([HEADERS.members, '', HEADERS.approvalsRequired, '']); + }); + + it('renders headers when there is a single named rule', () => { + store.modules.approvals.state.rules = [createMRRule()]; factory(); expect(findHeaders()).toEqual([HEADERS.name, HEADERS.members, HEADERS.approvalsRequired, '']); @@ -83,34 +123,6 @@ describe('EE Approvals MRRules', () => { it('shows members', () => { expect(findRuleMembers()).toEqual(expected.approvers); }); - - it('shows approvals required input', () => { - const approvalsRequired = findRuleApprovalsRequired(); - - expect(Number(approvalsRequired.element.value)).toEqual(expected.approvalsRequired); - expect(Number(approvalsRequired.attributes('min'))).toEqual(expected.minApprovalsRequired); - expect(approvalsRequired.attributes('disabled')).toBeUndefined(); - }); - - it('does not show controls', () => { - const controls = findRuleControls(); - - expect(controls.exists()).toBe(false); - }); - - it('dispatches putRule on change of approvals required', () => { - const action = store.modules.approvals.actions.putRule; - const approvalsRequired = findRuleApprovalsRequired(); - const newValue = expected.approvalsRequired + 1; - - approvalsRequired.setValue(newValue); - - expect(action).toHaveBeenCalledWith( - jasmine.anything(), - { id: expected.id, approvalsRequired: newValue }, - undefined, - ); - }); }); describe('with custom MR rule', () => { @@ -129,20 +141,6 @@ describe('EE Approvals MRRules', () => { expect(controls.props('rule')).toEqual(expected); }); - describe('without min approvals required', () => { - beforeEach(() => { - delete approvalRules[0].minApprovalsRequired; - }); - - it('defaults min approvals required input to 0', () => { - factory(); - - const input = findRuleApprovalsRequired(); - - expect(Number(input.attributes('min'))).toEqual(0); - }); - }); - describe('with settings cannot edit', () => { beforeEach(() => { store.state.settings.canEdit = false; @@ -154,12 +152,6 @@ describe('EE Approvals MRRules', () => { expect(controls.exists()).toBe(false); }); - - it('disables input', () => { - const approvalsRequired = findRuleApprovalsRequired(); - - expect(approvalsRequired.attributes('disabled')).toBe('disabled'); - }); }); }); }); @@ -167,23 +159,50 @@ describe('EE Approvals MRRules', () => { describe('when allow single rule', () => { beforeEach(() => { store.state.settings.allowMultiRule = false; + store.state.settings.eligibleApproversDocsPath = 'some/path'; }); - it('does not show name header', () => { + it('should only show single regular rule', () => { + store.modules.approvals.state.rules = [createMRRule()]; + factory(); + + expect(store.modules.approvals.state.rules[0].ruleType).toBe('regular'); + expect(store.modules.approvals.state.rules.length).toBe(1); + }); + + it('should only show single any_approver rule', () => { + store.modules.approvals.state.rules = [createEmptyRule()]; + factory(); + + expect(store.modules.approvals.state.rules[0].ruleType).toBe('any_approver'); + expect(store.modules.approvals.state.rules.length).toBe(1); + }); + + it('does not show name header for any rule', () => { + store.modules.approvals.state.rules = [createEmptyRule()]; factory(); expect(findHeaders()).not.toContain(HEADERS.name); }); + it('does not show approvers header for regular rule', () => { + store.modules.approvals.state.rules = [createMRRule()]; + factory(); + + expect(findHeaders()).toEqual([HEADERS.name, HEADERS.members, HEADERS.approvalsRequired, '']); + }); + describe('with source rule', () => { + const expected = createMRRuleWithSource(); + beforeEach(() => { approvalRules = [createMRRuleWithSource()]; factory(); }); - it('does not show name', () => { - expect(findRuleName().exists()).toBe(false); + it('shows name', () => { + expect(findRuleName().text()).toEqual(expected.name); }); it('shows controls', () => { diff --git a/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js b/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js index 04a0deb130854595f038dd56fbaa84bb81ce3384..ef9b4aca18f20e031c1d75528adef4e15004c485 100644 --- a/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js +++ b/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js @@ -4,7 +4,7 @@ import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list import { createStoreOptions } from 'ee/approvals/stores'; import projectSettingsModule from 'ee/approvals/stores/modules/project_settings'; import ProjectRules from 'ee/approvals/components/project_settings/project_rules.vue'; -import RuleControls from 'ee/approvals/components/rule_controls.vue'; +import RuleInput from 'ee/approvals/components/mr_edit/rule_input.vue'; import { createProjectRules } from '../../mocks'; const TEST_RULES = createProjectRules(); @@ -15,18 +15,13 @@ localVue.use(Vuex); const findCell = (tr, name) => tr.find(`td.js-${name}`); const getRowData = tr => { - const summary = findCell(tr, 'summary'); const name = findCell(tr, 'name'); const members = findCell(tr, 'members'); - const controls = findCell(tr, 'controls'); const approvalsRequired = findCell(tr, 'approvals-required'); - return { name: name.text(), - summary: summary.text(), approvers: members.find(UserAvatarList).props('items'), - approvalsRequired: Number(approvalsRequired.text()), - ruleControl: controls.find(RuleControls).props('rule'), + approvalsRequired: approvalsRequired.find(RuleInput).props('rule').approvalsRequired, }; }; @@ -60,19 +55,26 @@ describe('Approvals ProjectRules', () => { it('renders row for each rule', () => { factory(); - const rows = wrapper.findAll('tbody tr'); + const rows = wrapper.findAll('tbody tr').filter((tr, index) => index !== 0); const data = rows.wrappers.map(getRowData); expect(data).toEqual( - TEST_RULES.map(rule => ({ + TEST_RULES.filter((rule, index) => index !== 0).map(rule => ({ name: rule.name, - summary: jasmine.stringMatching(`${rule.approvalsRequired} approval.*from ${rule.name}`), - approvalsRequired: rule.approvalsRequired, approvers: rule.approvers, - ruleControl: rule, + approvalsRequired: rule.approvalsRequired, })), ); }); + + it('should always have any_approver rule', () => { + factory(); + const hasAnyApproverRule = store.modules.approvals.state.rules.some( + rule => rule.ruleType === 'any_approver', + ); + + expect(hasAnyApproverRule).toBe(true); + }); }); describe('when only allow single rule', () => { @@ -92,10 +94,10 @@ describe('Approvals ProjectRules', () => { expect(findCell(row, 'name').exists()).toBe(false); }); - it('renders single summary', () => { - expect(findCell(row, 'summary').text()).toEqual( - `${rule.approvalsRequired} approvals required from ${rule.approvers.length} members`, - ); + it('should only display 1 rule', () => { + factory(); + + expect(store.modules.approvals.state.rules.length).toBe(1); }); }); @@ -114,13 +116,6 @@ describe('Approvals ProjectRules', () => { rows = wrapper.findAll('tbody tr'); }); - it('should render the popover for the Vulnerability-Check group', () => { - const firstRow = rows.at(0); - const nameCell = findCell(firstRow, 'name'); - - expect(nameCell.find('.js-help').exists()).toBeTruthy(); - }); - it('should not render the popover for a standard approval group', () => { const secondRow = rows.at(1); const nameCell = findCell(secondRow, 'name'); diff --git a/ee/spec/javascripts/approvals/components/rule_controls_spec.js b/ee/spec/javascripts/approvals/components/rule_controls_spec.js index aebc037baf8b85c55cbabb8d42fb8ce6a37d0080..39e89a3055f8c31b7e73a65b5d9483834d686973 100644 --- a/ee/spec/javascripts/approvals/components/rule_controls_spec.js +++ b/ee/spec/javascripts/approvals/components/rule_controls_spec.js @@ -112,8 +112,8 @@ describe('EE Approvals RuleControls', () => { expect(findEditButton().exists()).toBe(true); }); - it('does not render remove button', () => { - expect(findRemoveButton()).toBe(undefined); + it('does remove button', () => { + expect(findRemoveButton().exists()).toBe(true); }); }); }); diff --git a/ee/spec/javascripts/approvals/components/rule_form_spec.js b/ee/spec/javascripts/approvals/components/rule_form_spec.js index 1bf3f9d72b6b68cecd637a75d2474ddff48c850c..3132a0fbfbbb4571c2b45a4b9d6c55aed6965005 100644 --- a/ee/spec/javascripts/approvals/components/rule_form_spec.js +++ b/ee/spec/javascripts/approvals/components/rule_form_spec.js @@ -387,7 +387,7 @@ describe('EE Approvals RuleForm', () => { it('hides name', () => { createComponent(); - expect(findNameInput().exists()).toBe(false); + expect(findNameInput().exists()).toBe(true); }); describe('with no init rule', () => { diff --git a/ee/spec/javascripts/approvals/mocks.js b/ee/spec/javascripts/approvals/mocks.js index 67aa74170e544251fa9de3025f343b24a23fe553..073ba48f11c7645c07981faf4dd4614ea467f0d3 100644 --- a/ee/spec/javascripts/approvals/mocks.js +++ b/ee/spec/javascripts/approvals/mocks.js @@ -1,7 +1,13 @@ export const createProjectRules = () => [ - { id: 1, name: 'Lorem', approvalsRequired: 2, approvers: [{ id: 7 }, { id: 8 }] }, - { id: 2, name: 'Ipsum', approvalsRequired: 0, approvers: [{ id: 9 }] }, - { id: 3, name: 'Dolarsit', approvalsRequired: 3, approvers: [] }, + { + id: 1, + name: 'Lorem', + approvalsRequired: 2, + approvers: [{ id: 7 }, { id: 8 }], + ruleType: 'regular', + }, + { id: 2, name: 'Ipsum', approvalsRequired: 0, approvers: [{ id: 9 }], ruleType: 'regular' }, + { id: 3, name: 'Dolarsit', approvalsRequired: 3, approvers: [], ruleType: 'regular' }, ]; export const createMRRule = () => ({ @@ -10,9 +16,20 @@ export const createMRRule = () => ({ approvers: [{ id: 1 }, { id: 2 }], approvalsRequired: 2, minApprovalsRequired: 0, + ruleType: 'regular', +}); + +export const createEmptyRule = () => ({ + id: 5, + name: 'All Members', + approvers: [], + approvalsRequired: 3, + minApprovalsRequired: 0, + ruleType: 'any_approver', }); export const createMRRuleWithSource = () => ({ + ...createEmptyRule(), ...createMRRule(), minApprovalsRequired: 1, hasSource: true, diff --git a/ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_spec.js b/ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_spec.js index c0b32fbbad6da384aead674c2869cd32950a9a25..64e7769f20a500e5521f27466bfd0f9ab7e519cb 100644 --- a/ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_spec.js +++ b/ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_spec.js @@ -19,7 +19,6 @@ const TEST_HELP_PATH = 'help/path'; const TEST_PASSWORD = 'password'; const testApprovedBy = () => [1, 7, 10].map(id => ({ id })); const testApprovals = () => ({ - has_approval_rules: true, approved: false, approved_by: testApprovedBy().map(user => ({ user })), approval_rules_left: [], diff --git a/ee/spec/models/approval_merge_request_fallback_spec.rb b/ee/spec/models/approval_merge_request_fallback_spec.rb deleted file mode 100644 index f64f2c1554ea160c94fd06fe4c796d81f1a9dff6..0000000000000000000000000000000000000000 --- a/ee/spec/models/approval_merge_request_fallback_spec.rb +++ /dev/null @@ -1,56 +0,0 @@ -# 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_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index ba92bc95717cf25a777ca0a1ec6befa47db82c49..b2f657331fe2c6bc71dac0d327d5fccd58e1f7af 100644 --- a/ee/spec/models/approval_merge_request_rule_spec.rb +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -104,6 +104,18 @@ end end + describe '.regular_or_any_approver scope' do + it 'returns regular or any-approver rules' do + any_approver_rule = create(:any_approver_rule) + regular_rule = create(:approval_merge_request_rule) + create(:report_approver_rule) + + expect(described_class.regular_or_any_approver).to( + contain_exactly(any_approver_rule, regular_rule) + ) + end + end + context 'scopes' do set(:rb_rule) { create(:code_owner_rule, name: '*.rb') } set(:js_rule) { create(:code_owner_rule, name: '*.js') } @@ -262,6 +274,8 @@ let!(:approval2) { create(:approval, merge_request: merge_request, user: member2) } let!(:approval3) { create(:approval, merge_request: merge_request, user: member3) } + let(:any_approver_rule) { create(:any_approver_rule, merge_request: merge_request) } + before do subject.users = [member1, member2] end @@ -269,8 +283,10 @@ context 'when not merged' do it 'does nothing' do subject.sync_approved_approvers + any_approver_rule.sync_approved_approvers expect(subject.approved_approvers.reload).to be_empty + expect(any_approver_rule.approved_approvers).to be_empty end end @@ -282,6 +298,12 @@ expect(subject.approved_approvers.reload).to contain_exactly(member1, member2) end + + it 'stores all the approvals for any-approver rule' do + any_approver_rule.sync_approved_approvers + + expect(any_approver_rule.approved_approvers.reload).to contain_exactly(member1, member2, member3) + end end end diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb index 667585558fc2cd436ca3a1fb90cec39e19019c1f..b738a2c43100f7a8c151758ffbc72ac9837313d5 100644 --- a/ee/spec/models/approval_project_rule_spec.rb +++ b/ee/spec/models/approval_project_rule_spec.rb @@ -18,7 +18,19 @@ end end - describe '.code_ownerscope' do + describe '.regular_or_any_approver scope' do + it 'returns regular or any-approver rules' do + any_approver_rule = create(:approval_project_rule, rule_type: :any_approver) + regular_rule = create(:approval_project_rule) + create(:approval_project_rule, :security_report) + + expect(described_class.regular_or_any_approver).to( + contain_exactly(any_approver_rule, regular_rule) + ) + end + end + + describe '.code_owner scope' do it 'returns nothing' do create_list(:approval_project_rule, 2) diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb index 8028b31f5746c10636f09c1e141af1ce056ac212..1d32b441e850035c1e87ad0b395b58fbfafbbe01 100644 --- a/ee/spec/models/approval_state_spec.rb +++ b/ee/spec/models/approval_state_spec.rb @@ -10,6 +10,7 @@ def create_rule(additional_params = {}) case params.delete(:rule_type) when :code_owner then :code_owner_rule when :report_approver then :report_approver_rule + when :any_approver then :any_approver_rule else :approval_merge_request_rule end @@ -127,10 +128,8 @@ def disable_feature end end - context 'when `approvals_before_merge` is set on a merge request' do - before do - merge_request.update!(approvals_before_merge: 7) - end + context 'when merge request has any approver rule' do + let!(:any_approver_rule) { create(:any_approver_rule, merge_request: merge_request) } it 'returns true' do expect(subject.approval_rules_overwritten?).to eq(true) @@ -140,6 +139,7 @@ def disable_feature before do project.update!(disable_overriding_approvers_per_merge_request: true) end + it 'returns true' do expect(subject.approval_rules_overwritten?).to eq(false) end @@ -181,8 +181,8 @@ def disable_feature end context 'when overall approvals required is not zero' do - before do - project.update!(approvals_before_merge: 1) + let!(:any_approver_rule) do + create(:approval_project_rule, project: project, rule_type: :any_approver, approvals_required: 1) end it 'returns true' do @@ -244,9 +244,9 @@ def disable_feature end end - shared_examples_for 'checking fallback_approvals_required' do - before do - project.update!(approvals_before_merge: 1) + shared_examples_for 'checking any_approver rule' do + let!(:any_approver_rule) do + create(:approval_project_rule, project: project, rule_type: :any_approver, approvals_required: 1) end context 'when it is not met' do @@ -265,7 +265,7 @@ def disable_feature end context 'when no rules' do - it_behaves_like 'checking fallback_approvals_required' + it_behaves_like 'checking any_approver rule' end context 'when only code owner rules present' do @@ -274,7 +274,7 @@ def disable_feature end it_behaves_like 'when rules are present' - it_behaves_like 'checking fallback_approvals_required' + it_behaves_like 'checking any_approver rule' end context 'when only report approver rules present' do @@ -283,12 +283,11 @@ def disable_feature end it_behaves_like 'when rules are present' - it_behaves_like 'checking fallback_approvals_required' + it_behaves_like 'checking any_approver rule' end context 'when regular rules present' do before do - project.update!(approvals_before_merge: 999) 2.times { create_rule(users: [create(:user)]) } end @@ -305,40 +304,6 @@ def disable_feature end end - describe '#any_approver_allowed?' do - context 'when no rules' do - it 'returns true' do - expect(subject.any_approver_allowed?).to eq(true) - end - end - - context 'when with rules' do - before do - create_rule(approvals_required: 1, users: [approver1]) - end - - context 'when approved' do - before do - allow(subject).to receive(:approved?).and_return(true) - end - - it 'returns true' do - expect(subject.any_approver_allowed?).to eq(true) - end - end - - context 'when not approved' do - before do - allow(subject).to receive(:approved?).and_return(false) - end - - it 'returns false' do - expect(subject.approved?).to eq(false) - end - end - end - end - describe '#approvals_left' do before do create_rule(approvals_required: 5) @@ -394,7 +359,7 @@ def create_unapproved_rule end describe '#filtered_approvers' do - describe 'only direct users, without code owners or report_approvers' do + describe 'only direct users, without code owners' do it 'includes only rule user members' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) @@ -402,28 +367,8 @@ def create_unapproved_rule create_rule(users: [approver3], rule_type: :report_approver) expect( - subject.filtered_approvers(code_owner: false, report_approver: false, target: :users) - ).to contain_exactly(approver1) - end - end - - describe 'only code owners' do - it 'includes only code owners' do - create_rule(users: [approver1]) - create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], rule_type: :code_owner) - - expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) - end - end - - describe 'only report approvers' do - it 'includes only report approvers' do - create_rule(users: [approver1]) - create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], rule_type: :report_approver) - - expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) + subject.filtered_approvers(code_owner: false, target: :users) + ).to contain_exactly(approver1, approver3) end end @@ -470,12 +415,6 @@ def create_unapproved_rule it 'allows the author to approve the MR if within the approvers list' do expect(subject.can_approve?(author)).to be_truthy end - - it 'does not allow the author to approve the MR if not within the approvers list' do - allow(subject).to receive(:approvers).and_return([]) - - expect(subject.can_approve?(author)).to be_falsey - end end context 'when authors are not authorized to approve their own MRs' do @@ -502,17 +441,13 @@ def create_project_member(role, user_attrs = {}) let(:reporter) { create_project_member(:reporter) } let(:stranger) { create(:user) } - context 'when there are no approval rules' do - before do - project.update!(approvals_before_merge: 1) + context 'when there are no regular approval rules' do + let!(:any_approver_rule) do + create(:approval_project_rule, project: project, rule_type: :any_approver, approvals_required: 1) end it_behaves_like 'a MR that all members with write access can approve' - it 'has fallback rules apply' do - expect(subject.use_fallback?).to be_truthy - end - it 'requires one approval' do expect(subject.approvals_left).to eq(1) end @@ -698,37 +633,6 @@ def create_project_member(role, user_attrs = {}) expect(subject.approvers).to eq([approver]) end end - - context 'without approvers' do - let(:approver) { author } - - it 'requires one approval' do - rule = subject.wrapped_approval_rules.last - - expect(rule).to be_a(ApprovalMergeRequestFallback) - expect(subject.approvers).to eq([]) - end - end - end - end - - context 'when that approver is not the MR author' do - before do - rule.users << approver - end - - it 'requires one approval' do - expect(subject.approvals_left).to eq(1) - end - - it 'only allows the approver to approve the MR' do - expect(subject.can_approve?(approver)).to be_truthy - - expect(subject.can_approve?(author)).to be_falsey - expect(subject.can_approve?(developer)).to be_falsey - expect(subject.can_approve?(reporter)).to be_falsey - expect(subject.can_approve?(stranger)).to be_falsey - expect(subject.can_approve?(nil)).to be_falsey end end end @@ -826,7 +730,7 @@ def create_project_member(role, user_attrs = {}) expect(subject.approvals_left).to eq(3) end - it 'only allows the approvers to approve the MR' do + it 'allows anyone with write access except for author to approve the MR' do expect(subject.can_approve?(developer)).to be_truthy expect(subject.can_approve?(approver)).to be_truthy expect(subject.can_approve?(approver2)).to be_truthy @@ -836,34 +740,60 @@ def create_project_member(role, user_attrs = {}) expect(subject.can_approve?(stranger)).to be_falsey expect(subject.can_approve?(nil)).to be_falsey end + end + end - context 'when only 1 approval approved' do - it 'only allows the approvers to approve the MR' do - create(:approval, user: approver, merge_request: merge_request) + describe '#any_approver_rules' do + let(:approval_rule) { subject.wrapped_approval_rules.last.approval_rule } - expect(subject.can_approve?(developer)).to be_truthy - expect(subject.can_approve?(approver)).to be_falsey - expect(subject.can_approve?(approver2)).to be_truthy + context 'a project with any_approver rule' do + let!(:project_rule) do + create(:approval_project_rule, rule_type: :any_approver, project: project) + end - expect(subject.can_approve?(author)).to be_falsey - expect(subject.can_approve?(reporter)).to be_falsey - expect(subject.can_approve?(other_developer)).to be_falsey - expect(subject.can_approve?(stranger)).to be_falsey - expect(subject.can_approve?(nil)).to be_falsey + it 'returns project rules' do + expect(subject.wrapped_approval_rules.size).to eq(1) + expect(approval_rule).to eq(project_rule) + end + + context 'a merge request with regular rule' do + let!(:rule) { create_rule(rule_type: :any_approver, approvals_required: 2) } + + it 'returns merge request rules' do + expect(subject.wrapped_approval_rules.size).to eq(1) + expect(approval_rule).to eq(rule) end end + end + end - context 'when all approvals received' do - it 'allows anyone with write access except for author to approve the MR' do - create(:approval, user: approver, merge_request: merge_request) - create(:approval, user: approver2, merge_request: merge_request) - create(:approval, user: developer, merge_request: merge_request) + context 'when any_approver rule with 2 approvals required exist' do + let!(:rule) { create_rule(rule_type: :any_approver, approvals_required: 2) } - expect(subject.can_approve?(author)).to be_falsey - expect(subject.can_approve?(reporter)).to be_falsey - expect(subject.can_approve?(other_developer)).to be_truthy - expect(subject.can_approve?(stranger)).to be_falsey - expect(subject.can_approve?(nil)).to be_falsey + it_behaves_like 'a MR that all members with write access can approve' + + it 'requires the 2 approvals' do + expect(subject.approvals_left).to eq(2) + end + + context 'a user approves the MR' do + before do + create(:approval, merge_request: merge_request, user: approver) + end + + it 'requires 1 approval' do + expect(subject.has_approved?(approver)).to eq(true) + expect(subject.approvals_left).to eq(1) + end + + context 'another user approves the MR' do + before do + create(:approval, merge_request: merge_request, user: approver1) + end + + it 'becomes approved' do + expect(subject.has_approved?(approver1)).to eq(true) + expect(subject.approved?).to eq(true) end end end @@ -976,24 +906,6 @@ def create_rules end end - describe '#has_non_fallback_rules?' do - it 'returns true when there are rules' do - create_rules - - expect(subject.has_non_fallback_rules?).to be(true) - end - - it 'returns false if there are no rules' do - expect(subject.has_non_fallback_rules?).to be(false) - end - - it 'returns false if there are only fallback rules' do - project.update!(approvals_before_merge: 1) - - expect(subject.has_non_fallback_rules?).to be(false) - end - end - describe '#approval_needed?' do context 'when feature not available' do it 'returns false' do @@ -1059,7 +971,7 @@ def create_rules end end - shared_examples_for 'checking fallback_approvals_required' do + shared_examples_for 'checking any_approver rule' do before do project.update!(approvals_before_merge: 1) end @@ -1080,7 +992,7 @@ def create_rules end context 'when no rules' do - it_behaves_like 'checking fallback_approvals_required' + it_behaves_like 'checking any_approver rule' end context 'when only code owner rules present' do @@ -1090,7 +1002,7 @@ def create_rules end it_behaves_like 'when rules are present' - it_behaves_like 'checking fallback_approvals_required' + it_behaves_like 'checking any_approver rule' end context 'when only report approver rules present' do @@ -1099,7 +1011,7 @@ def create_rules end it_behaves_like 'when rules are present' - it_behaves_like 'checking fallback_approvals_required' + it_behaves_like 'checking any_approver rule' end context 'when regular rules present' do @@ -1132,7 +1044,7 @@ def create_rules merge_request.update!(approvals_before_merge: 1) end - it_behaves_like 'checking fallback_approvals_required' + it_behaves_like 'checking any_approver rule' end end @@ -1142,29 +1054,7 @@ def create_rules merge_request.update!(approvals_before_merge: 1) end - it_behaves_like 'checking fallback_approvals_required' - end - end - - describe '#any_approver_allowed?' do - context 'when approved' do - before do - allow(subject).to receive(:approved?).and_return(true) - end - - it 'returns true' do - expect(subject.any_approver_allowed?).to eq(true) - end - end - - context 'when not approved' do - before do - allow(subject).to receive(:approved?).and_return(false) - end - - it 'returns false' do - expect(subject.approved?).to eq(false) - end + it_behaves_like 'checking any_approver rule' end end @@ -1204,34 +1094,16 @@ def create_rules end describe '#filtered_approvers' do - describe 'only direct users, without code owners or report approvers' do - it 'includes only rule user members' do + describe 'only direct users, without code owners' do + it 'excludes code owners' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) create_rule(users: [approver2], rule_type: :code_owner) create_rule(users: [approver3], rule_type: :report_approver) expect( - subject.filtered_approvers(code_owner: false, report_approver: false, target: :users) - ).to contain_exactly(approver1) - end - end - - describe 'excludes regular rule' do - it 'includes only code owners' do - create_rule(users: [approver1]) - create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], rule_type: :code_owner) - - expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) - end - - it 'includes only report approvers' do - create_rule(users: [approver1]) - create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], rule_type: :report_approver) - - expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) + subject.filtered_approvers(code_owner: false, target: :users) + ).to contain_exactly(approver1, approver3) end end @@ -1278,12 +1150,6 @@ def create_rules it 'allows the author to approve the MR if within the approvers list' do expect(subject.can_approve?(author)).to be_truthy end - - it 'does not allow the author to approve the MR if not within the approvers list' do - allow(subject).to receive(:approvers).and_return([]) - - expect(subject.can_approve?(author)).to be_falsey - end end context 'when authors are not authorized to approve their own MRs' do @@ -1409,26 +1275,6 @@ def create_project_member(role) expect(subject.approved?).to eq(false) end end - - context 'when that approver is not the MR author' do - before do - rule.users << approver - end - - it 'requires one approval' do - expect(subject.approvals_left).to eq(1) - end - - it 'only allows the approver to approve the MR' do - expect(subject.can_approve?(approver)).to be_truthy - - expect(subject.can_approve?(author)).to be_falsey - expect(subject.can_approve?(developer)).to be_falsey - expect(subject.can_approve?(reporter)).to be_falsey - expect(subject.can_approve?(stranger)).to be_falsey - expect(subject.can_approve?(nil)).to be_falsey - end - end end context 'when there are multiple approvers required' do @@ -1524,7 +1370,7 @@ def create_project_member(role) expect(subject.approvals_left).to eq(3) end - it 'only allows the approvers to approve the MR' do + it 'allows anyone with write access except for author to approve the MR' do expect(subject.can_approve?(developer)).to be_truthy expect(subject.can_approve?(approver)).to be_truthy expect(subject.can_approve?(approver2)).to be_truthy @@ -1536,22 +1382,6 @@ def create_project_member(role) expect(subject.can_approve?(nil)).to be_falsey end - context 'when only 1 approval approved' do - it 'only allows the approvers to approve the MR' do - create(:approval, user: approver, merge_request: merge_request) - - expect(subject.can_approve?(developer)).to be_truthy - expect(subject.can_approve?(approver)).to be_falsey - expect(subject.can_approve?(approver2)).to be_truthy - - expect(subject.can_approve?(author)).to be_falsey - expect(subject.can_approve?(reporter)).to be_falsey - expect(subject.can_approve?(other_developer)).to be_falsey - expect(subject.can_approve?(stranger)).to be_falsey - expect(subject.can_approve?(nil)).to be_falsey - end - end - context 'when an approver does not have access to the merge request' do before do project.members.find_by(user_id: developer.id).destroy @@ -1561,21 +1391,6 @@ def create_project_member(role) expect(subject.can_approve?(developer)).to be_falsey end end - - context 'when all approvals received' do - it 'allows anyone with write access except for author to approve the MR' do - create(:approval, user: approver, merge_request: merge_request) - create(:approval, user: approver2, merge_request: merge_request) - create(:approval, user: developer, merge_request: merge_request) - - expect(subject.can_approve?(author)).to be_falsey - expect(subject.can_approve?(reporter)).to be_falsey - 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?(nil)).to be_falsey - end - end end end end diff --git a/ee/spec/models/approval_wrapped_any_approver_rule_spec.rb b/ee/spec/models/approval_wrapped_any_approver_rule_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cd4460b99ebcfd1918cb6c1bb3c91b02e99d403e --- /dev/null +++ b/ee/spec/models/approval_wrapped_any_approver_rule_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ApprovalWrappedAnyApproverRule do + let(:merge_request) { create(:merge_request) } + + subject { described_class.new(merge_request, rule) } + + let(:rule) do + create(:any_approver_rule, merge_request: merge_request, approvals_required: 2) + end + + let(:approver1) { create(:user) } + let(:approver2) { create(:user) } + + before do + create(:approval, merge_request: merge_request, user: approver1) + create(:approval, merge_request: merge_request, user: approver2) + end + + context '#approvals_approvers' do + it 'contains every approved user' do + expect(subject.approved_approvers).to contain_exactly(approver1, approver2) + end + + context 'when an author and a committer approved' do + before do + merge_request.project.update!( + merge_requests_author_approval: false, + merge_requests_disable_committers_approval: true + ) + + create(:approval, merge_request: merge_request, user: merge_request.author) + + committer = create(:user, username: 'commiter') + create(:approval, merge_request: merge_request, user: committer) + allow(merge_request).to receive(:committers).and_return(User.where(id: committer.id)) + end + + it 'does not contain an author' do + expect(subject.approved_approvers).to contain_exactly(approver1, approver2) + end + end + end + + it '#approved?' do + expect(subject.approved?).to eq(true) + end +end diff --git a/ee/spec/models/approval_wrapped_code_owner_rule_spec.rb b/ee/spec/models/approval_wrapped_code_owner_rule_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c72e3e09dcf60566dd3f724ac461f6207d767cad --- /dev/null +++ b/ee/spec/models/approval_wrapped_code_owner_rule_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ApprovalWrappedCodeOwnerRule do + using RSpec::Parameterized::TableSyntax + + let(:merge_request) { create(:merge_request) } + + subject { described_class.new(merge_request, rule) } + + describe '#approvals_required' do + where(:feature_enabled, :approver_count, :expected_required_approvals) do + true | 0 | 0 + true | 2 | 1 + false | 2 | 0 + false | 0 | 0 + end + + with_them do + let(:rule) do + create(:code_owner_rule, + merge_request: merge_request, + users: create_list(:user, approver_count)) + end + + let(:branch) { subject.project.repository.branches.find { |b| b.name == merge_request.target_branch } } + + context "when project.code_owner_approval_required_available? is true" do + before do + allow(subject.project) + .to receive(:code_owner_approval_required_available?).and_return(true) + end + + context "when the project doesn't require code owner approval on all MRs" do + it 'returns the expected number of approvals for protected_branches that do require approval' do + allow(subject.project) + .to receive(:merge_requests_require_code_owner_approval?).and_return(false) + allow(ProtectedBranch) + .to receive(:branch_requires_code_owner_approval?).with(subject.project, branch.name).and_return(feature_enabled) + + expect(subject.approvals_required).to eq(expected_required_approvals) + end + end + end + + context "when project.code_owner_approval_required_available? is falsy" do + it "returns nil" do + allow(subject.project) + .to receive(:code_owner_approval_required_available?).and_return(false) + + expect(subject.approvals_required).to eq(0) + end + end + end + end +end diff --git a/ee/spec/models/approval_wrapped_rule_spec.rb b/ee/spec/models/approval_wrapped_rule_spec.rb index a65bc507375b15b3c9af61137895603355e13704..80e2470af0e2999589628bf8255c6834a7ed0e38 100644 --- a/ee/spec/models/approval_wrapped_rule_spec.rb +++ b/ee/spec/models/approval_wrapped_rule_spec.rb @@ -165,58 +165,10 @@ end describe '#approvals_required' do - context 'for regular rules' do - let(:rule) { create(:approval_merge_request_rule, approvals_required: 19) } + let(:rule) { create(:approval_merge_request_rule, approvals_required: 19) } - it 'returns the attribute saved on the model' do - expect(subject.approvals_required).to eq(19) - end - end - - context 'for code owner rules' do - where(:feature_enabled, :approver_count, :expected_required_approvals) do - true | 0 | 0 - true | 2 | 1 - false | 2 | 0 - false | 0 | 0 - end - - with_them do - let(:rule) do - create(:code_owner_rule, - merge_request: merge_request, - users: create_list(:user, approver_count)) - end - - let(:branch) { subject.project.repository.branches.find { |b| b.name == merge_request.target_branch } } - - context "when project.code_owner_approval_required_available? is true" do - before do - allow(subject.project) - .to receive(:code_owner_approval_required_available?).and_return(true) - end - - context "when the project doesn't require code owner approval on all MRs" do - it 'returns the expected number of approvals for protected_branches that do require approval' do - allow(subject.project) - .to receive(:merge_requests_require_code_owner_approval?).and_return(false) - allow(ProtectedBranch) - .to receive(:branch_requires_code_owner_approval?).with(subject.project, branch.name).and_return(feature_enabled) - - expect(subject.approvals_required).to eq(expected_required_approvals) - end - end - end - - context "when project.code_owner_approval_required_available? is falsy" do - it "returns nil" do - allow(subject.project) - .to receive(:code_owner_approval_required_available?).and_return(false) - - expect(subject.approvals_required).to eq(0) - end - end - end + it 'returns the attribute saved on the model' do + expect(subject.approvals_required).to eq(19) end end end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index e9027ef8fe3f0ae311076577aa94af28384bcfa1..f7671f28a1c93e0603d9befca85387fd0a62457d 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -996,16 +996,17 @@ end end - describe '#visible_regular_approval_rules' do + describe '#visible_user_defined_rules' do let(:project) { create(:project) } let!(:approval_rules) { create_list(:approval_project_rule, 2, project: project) } + let!(:any_approver_rule) { create(:approval_project_rule, rule_type: :any_approver, 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) + expect(project.visible_user_defined_rules).to eq([any_approver_rule, *approval_rules]) end context 'when multiple approval rules is not available' do @@ -1014,35 +1015,30 @@ end it 'returns the first approval rule' do - expect(project.visible_regular_approval_rules).to contain_exactly(approval_rules.first) + expect(project.visible_user_defined_rules).to eq([any_approver_rule]) end end end describe '#min_fallback_approvals' do - let(:project) { create(:project, approvals_before_merge: 1) } - - it 'returns approvals before merge if there are no rules' do - expect(project.min_fallback_approvals).to eq(1) - end + let(:project) { create(:project) } - 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) + before do + create(:approval_project_rule, project: project, rule_type: :any_approver, approvals_required: 2) + 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 + 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 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) + 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 + expect(project.min_fallback_approvals).to eq(2) end end diff --git a/ee/spec/presenters/approval_rule_presenter_spec.rb b/ee/spec/presenters/approval_rule_presenter_spec.rb index 796ad437959e32544660ec08e085af7c337f3200..5b42c4e532c9c78bbf1cd5722be28d805a4aacea 100644 --- a/ee/spec/presenters/approval_rule_presenter_spec.rb +++ b/ee/spec/presenters/approval_rule_presenter_spec.rb @@ -62,8 +62,8 @@ it_behaves_like 'filtering private group' end - context 'fallback rule' do - let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) } + context 'any_approver rule' do + let(:rule) { create(:any_approver_rule) } it 'contains no groups without raising an error' do expect(subject.groups).to be_empty @@ -103,8 +103,8 @@ it_behaves_like 'detecting hidden group' end - context 'fallback rule' do - let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) } + context 'any_approver rule' do + let(:rule) { create(:any_approver_rule) } it 'contains no groups without raising an error' do expect(subject.contains_hidden_groups?).to eq(false) diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb index 286804fb50f377e18bee6ef95868710b0cd521bc..e135bea02fa2807fd19a0200448e12f0016613bd 100644 --- a/ee/spec/requests/api/merge_request_approvals_spec.rb +++ b/ee/spec/requests/api/merge_request_approvals_spec.rb @@ -226,16 +226,6 @@ expect(response).to have_gitlab_http_status(422) end end - - it 'only shows approver groups that are visible to current user' do - private_group = create(:group, :private) - merge_request.approver_groups.create(group: private_group) - - post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 } - - expect(response).to have_gitlab_http_status(201) - expect(json_response['approver_groups'].size).to eq(expected_approver_group_size) - end end context 'as a project admin' do diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb index b55465f06a3cf51b3e3fdc0bcd76afc9cb2efdd9..42674cb87ad6d8a3ab969e91de3f376e4250e2ef 100644 --- a/ee/spec/services/approval_rules/create_service_spec.rb +++ b/ee/spec/services/approval_rules/create_service_spec.rb @@ -94,6 +94,37 @@ specify { expect(result[:rule].rule_type).to eq('report_approver') } end end + + context 'when approval rule is being created' do + subject { described_class.new(target, user, { user_ids: [], group_ids: [] }) } + + it 'sets default attributes for any-approver rule' do + rule = subject.execute[:rule] + + expect(rule[:rule_type]).to eq('any_approver') + expect(rule[:name]).to eq('All Members') + end + end + + context 'when any-approver rule exists' do + let!(:any_approver_rule) do + create(:approval_project_rule, project: target, rule_type: :any_approver) + end + + context 'multiple approval rules are not enabled' do + subject { described_class.new(target, user, { user_ids: [1], group_ids: [] }) } + + before do + stub_licensed_features(multiple_approval_rules: false) + end + + it 'removes the rule if a regular one is created' do + expect { subject.execute }.to change( + target.approval_rules.any_approver, :count + ).from(1).to(0) + end + end + end end context 'when target is merge request' do diff --git a/ee/spec/services/approval_rules/params_filtering_service_spec.rb b/ee/spec/services/approval_rules/params_filtering_service_spec.rb index 4d3653395da4e3024f09f14b7d612afc9eea0972..4834fe82547163da74d842be74236017f52bd305 100644 --- a/ee/spec/services/approval_rules/params_filtering_service_spec.rb +++ b/ee/spec/services/approval_rules/params_filtering_service_spec.rb @@ -12,21 +12,21 @@ let(:user) { create(:user) } describe '#execute' do - shared_examples_for(:assigning_users_and_groups) do - before do - project.add_maintainer(user) - project.add_reporter(project_member) + before do + project.add_maintainer(user) + project.add_reporter(project_member) - accessible_group.add_developer(user) + accessible_group.add_developer(user) - allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).and_call_original - allow(Ability) - .to receive(:allowed?) - .with(user, :update_approvers, merge_request) - .and_return(can_update_approvers?) - end + allow(Ability) + .to receive(:allowed?) + .with(user, :update_approvers, merge_request) + .and_return(can_update_approvers?) + end + shared_examples_for(:assigning_users_and_groups) do context 'user can update approvers' do let(:can_update_approvers?) { true } @@ -55,23 +55,44 @@ end context 'create' do + let(:merge_request) { build(:merge_request, target_project: project, source_project: project) } + let(:params) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master', + force_remove_source_branch: '1', + approval_rules_attributes: approval_rules_attributes + } + end + it_behaves_like :assigning_users_and_groups do - let(:merge_request) { build(:merge_request, target_project: project, source_project: project) } - let(:params) do - { - title: 'Awesome merge_request', - description: 'please fix', - source_branch: 'feature', - target_branch: 'master', - force_remove_source_branch: '1', - approval_rules_attributes: [ - { name: 'foo', user_ids: [project_member.id, outsider.id] }, - { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] } - ] - } + let(:approval_rules_attributes) do + [ + { name: 'foo', user_ids: [project_member.id, outsider.id] }, + { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] } + ] end let(:expected_groups) { [accessible_group] } end + + context 'any approver rule' do + let(:can_update_approvers?) { true } + let(:approval_rules_attributes) do + [ + { user_ids: [], group_ids: [] } + ] + end + + it 'sets rule type for the rules attributes' do + params = service.execute + rule = params[:approval_rules_attributes].first + + expect(rule[:rule_type]).to eq(:any_approver) + expect(rule[:name]).to eq('All Members') + end + end end context 'update' do diff --git a/ee/spec/services/ee/notification_service_spec.rb b/ee/spec/services/ee/notification_service_spec.rb index cd445fe44d00f46b082956619d838c4331b7b290..1041b5f69383a565bcddc6893328ea9e6bb042e8 100644 --- a/ee/spec/services/ee/notification_service_spec.rb +++ b/ee/spec/services/ee/notification_service_spec.rb @@ -637,7 +637,6 @@ def add_users_with_subscription(group, issuable) let!(:rule) { create(:approval_project_rule, project: project, users: project_approvers, approvals_required: 1 )} before do - merge_request.target_project.update(approvals_before_merge: 1) reset_delivered_emails! end @@ -648,7 +647,7 @@ def add_users_with_subscription(group, issuable) end it 'does not email the approvers when approval is not necessary' do - rule.update(approvals_required: 0) + project.approval_rules.update_all(approvals_required: 0) notification.new_merge_request(merge_request, @u_disabled) project_approvers.each { |approver| should_not_email(approver) } diff --git a/ee/spec/support/helpers/feature_approval_helper.rb b/ee/spec/support/helpers/feature_approval_helper.rb index 8215bacd45bdabba23616d9d6e90360fda992e3b..1cc1830cd90dedfa676ca465ad84256b3d437e12 100644 --- a/ee/spec/support/helpers/feature_approval_helper.rb +++ b/ee/spec/support/helpers/feature_approval_helper.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true module FeatureApprovalHelper - def open_modal + def open_modal(text: 'Edit') page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()" within(config_selector) do - click_on('Edit') + click_on(text) end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 108097542c2374e45fe398a3d57dc414bbb8f148..6bf08eebda2a602a4812a5bbd16e5d97fcd691a2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1816,9 +1816,15 @@ msgstr "" msgid "Any Milestone" msgstr "" +msgid "Any eligible user" +msgstr "" + msgid "Any encrypted tokens" msgstr "" +msgid "Any member with Developer or higher permissions to the project." +msgstr "" + msgid "Any namespace" msgstr "" @@ -1923,9 +1929,6 @@ msgid_plural "ApprovalRuleSummary|%{count} approvals required from %{membersCoun msgstr[0] "" msgstr[1] "" -msgid "ApprovalRule|All members with Developer role or higher and code owners (if any)" -msgstr "" - msgid "ApprovalRule|Approvers" msgstr "" @@ -4294,6 +4297,9 @@ msgstr "" msgid "Code Owners" msgstr "" +msgid "Code Owners to the merge request changes." +msgstr "" + msgid "Code owner approval is required" msgstr "" @@ -19350,6 +19356,9 @@ msgstr "" msgid "Users" msgstr "" +msgid "Users or groups set as approvers in the project's or merge request's settings." +msgstr "" + msgid "Users outside of license" msgstr "" @@ -19750,6 +19759,9 @@ msgstr "" msgid "Whitelist to allow requests to the local network from hooks and services" msgstr "" +msgid "Who can be an approver?" +msgstr "" + msgid "Who can see this group?" msgstr ""