Commit dd7dec17 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Merge branch 'id-allow-empty-rule' into 'master'

Allow empty approval rule

See merge request gitlab-org/gitlab!15378
parents 1c806738 30b7f6d2
<script> <script>
import { mapState, mapActions, mapGetters } from 'vuex'; import { mapState, mapActions } from 'vuex';
import { GlLoadingIcon, GlButton } from '@gitlab/ui'; import { GlLoadingIcon, GlButton } from '@gitlab/ui';
import ModalRuleCreate from './modal_rule_create.vue'; import ModalRuleCreate from './modal_rule_create.vue';
import ModalRuleRemove from './modal_rule_remove.vue'; import ModalRuleRemove from './modal_rule_remove.vue';
import FallbackRules from './fallback_rules.vue';
export default { export default {
components: { components: {
...@@ -11,7 +10,6 @@ export default { ...@@ -11,7 +10,6 @@ export default {
ModalRuleRemove, ModalRuleRemove,
GlButton, GlButton,
GlLoadingIcon, GlLoadingIcon,
FallbackRules,
}, },
computed: { computed: {
...mapState({ ...mapState({
...@@ -19,7 +17,6 @@ export default { ...@@ -19,7 +17,6 @@ export default {
isLoading: state => state.approvals.isLoading, isLoading: state => state.approvals.isLoading,
hasLoaded: state => state.approvals.hasLoaded, hasLoaded: state => state.approvals.hasLoaded,
}), }),
...mapGetters(['isEmpty']),
createModalId() { createModalId() {
return `${this.settings.prefix}-approvals-create-modal`; return `${this.settings.prefix}-approvals-create-modal`;
}, },
...@@ -42,12 +39,9 @@ export default { ...@@ -42,12 +39,9 @@ export default {
<gl-loading-icon v-if="!hasLoaded" :size="2" /> <gl-loading-icon v-if="!hasLoaded" :size="2" />
<template v-else> <template v-else>
<div class="border-bottom"> <div class="border-bottom">
<slot v-if="isEmpty" name="fallback"> <slot name="rules"></slot>
<fallback-rules />
</slot>
<slot v-else name="rules"></slot>
</div> </div>
<div v-if="settings.canEdit" class="border-bottom py-3 px-2"> <div v-if="settings.canEdit && settings.allowMultiRule" class="border-bottom py-3 px-2">
<gl-loading-icon v-if="isLoading" /> <gl-loading-icon v-if="isLoading" />
<div v-if="settings.allowMultiRule" class="d-flex"> <div v-if="settings.allowMultiRule" class="d-flex">
<gl-button <gl-button
......
<script>
import Icon from '~/vue_shared/components/icon.vue';
import { GlPopover, GlLink } from '@gitlab/ui';
export default {
components: {
Icon,
GlPopover,
GlLink,
},
props: {
eligibleApproversDocsPath: {
type: String,
required: false,
default: '',
},
},
};
</script>
<template>
<div class="d-flex align-items-center">
<span class="mt-n1">{{ __('Any eligible user') }}</span>
<span id="popovercontainer" class="ml-2 align-self-end">
<icon
id="pop-approver"
name="question-o"
:aria-label="__('help')"
:size="14"
class="author-link suggestion-help-hover"
/>
<gl-popover
target="pop-approver"
container="popovercontainer"
placement="top"
triggers="focus"
>
<template #title>{{ __('Who can be an approver?') }}</template>
<ul class="pl-3">
<li>
{{ __('Any member with Developer or higher permissions to the project.') }}
</li>
<li>{{ __('Code Owners to the merge request changes.') }}</li>
<li>
{{
__("Users or groups set as approvers in the project's or merge request's settings.")
}}
</li>
</ul>
<gl-link :href="eligibleApproversDocsPath">{{ __('More information') }}</gl-link>
</gl-popover>
</span>
</div>
</template>
<script>
import { mapState } from 'vuex';
import Icon from '~/vue_shared/components/icon.vue';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import Rules from './rules.vue';
import RuleControls from './rule_controls.vue';
export default {
components: {
Icon,
UserAvatarList,
Rules,
RuleControls,
},
props: {
hasControls: {
type: Boolean,
required: false,
default: true,
},
},
computed: {
...mapState({
approvalsRequired: state => state.approvals.fallbackApprovalsRequired,
minApprovalsRequired: state => state.approvals.minFallbackApprovalsRequired || 0,
}),
rules() {
return [
{
isFallback: true,
approvalsRequired: this.approvalsRequired,
minApprovalsRequired: this.minApprovalsRequired,
},
];
},
},
};
</script>
<template>
<rules :rules="rules">
<template slot="thead" slot-scope="{ members, approvalsRequired }">
<tr class="d-none d-sm-table-row">
<th class="w-75 pl-0">{{ members }}</th>
<th>{{ approvalsRequired }}</th>
<th v-if="hasControls"></th>
</tr>
</template>
<template slot="tr" slot-scope="{ rule }">
<td class="pl-0">
{{ s__('ApprovalRule|All members with Developer role or higher and code owners (if any)') }}
</td>
<td class="text-nowrap">
<slot
name="approvals-required"
:approvals-required="rule.approvalsRequired"
:min-approvals-required="rule.minApprovalsRequired"
>
<icon name="approval" class="align-top text-tertiary" />
<span>{{ rule.approvalsRequired }}</span>
</slot>
</td>
<td v-if="hasControls" class="text-nowrap px-2 w-0">
<rule-controls :rule="rule" />
</td>
</template>
</rules>
</template>
...@@ -2,21 +2,18 @@ ...@@ -2,21 +2,18 @@
import App from '../app.vue'; import App from '../app.vue';
import MrRules from './mr_rules.vue'; import MrRules from './mr_rules.vue';
import MrRulesHiddenInputs from './mr_rules_hidden_inputs.vue'; import MrRulesHiddenInputs from './mr_rules_hidden_inputs.vue';
import MrFallbackRules from './mr_fallback_rules.vue';
export default { export default {
components: { components: {
App, App,
MrRules, MrRules,
MrRulesHiddenInputs, MrRulesHiddenInputs,
MrFallbackRules,
}, },
}; };
</script> </script>
<template> <template>
<app> <app>
<mr-fallback-rules slot="fallback" />
<mr-rules slot="rules" /> <mr-rules slot="rules" />
<mr-rules-hidden-inputs slot="footer" /> <mr-rules-hidden-inputs slot="footer" />
</app> </app>
......
<script>
import { mapActions } from 'vuex';
import { GlButton } from '@gitlab/ui';
import RuleInput from './rule_input.vue';
import EmptyRuleName from '../empty_rule_name.vue';
import RuleControls from './../rule_controls.vue';
export default {
components: {
RuleInput,
EmptyRuleName,
RuleControls,
GlButton,
},
props: {
rule: {
type: Object,
required: true,
},
allowMultiRule: {
type: Boolean,
required: true,
},
eligibleApproversDocsPath: {
type: String,
required: false,
default: '',
},
isMrEdit: {
type: Boolean,
default: true,
},
canEdit: {
type: Boolean,
required: true,
},
},
methods: {
...mapActions({ openCreateModal: 'createModal/open' }),
},
};
</script>
<template>
<tr>
<td colspan="2">
<empty-rule-name :eligible-approvers-docs-path="eligibleApproversDocsPath" />
</td>
<td class="js-approvals-required">
<rule-input :rule="rule" :is-mr-edit="isMrEdit" />
</td>
<td>
<gl-button
v-if="!allowMultiRule && canEdit"
class="ml-auto btn-info btn-inverted"
data-qa-selector="add_approvers_button"
@click="openCreateModal(null)"
>
{{ __('Add approval rule') }}
</gl-button>
</td>
</tr>
</template>
<script>
import { mapState, mapActions } from 'vuex';
import FallbackRules from '../fallback_rules.vue';
export default {
components: {
FallbackRules,
},
computed: {
...mapState(['settings']),
},
methods: {
...mapActions(['putFallbackRule']),
},
};
</script>
<template>
<fallback-rules :has-controls="settings.canEdit">
<input
slot="approvals-required"
slot-scope="{ approvalsRequired, minApprovalsRequired }"
:value="approvalsRequired"
:disabled="!settings.canEdit"
class="form-control mw-6em"
type="number"
:min="minApprovalsRequired"
@input="putFallbackRule({ approvalsRequired: Number($event.target.value) })"
/>
</fallback-rules>
</template>
<script> <script>
import { mapState, mapActions } from 'vuex'; import { mapState, mapActions } from 'vuex';
import { RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR, RULE_NAME_ANY_APPROVER } from '../../constants';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import Rules from '../rules.vue'; import Rules from '../rules.vue';
import RuleControls from '../rule_controls.vue'; import RuleControls from '../rule_controls.vue';
import EmptyRule from './empty_rule.vue';
import RuleInput from './rule_input.vue';
export default { export default {
components: { components: {
UserAvatarList, UserAvatarList,
Rules, Rules,
RuleControls, RuleControls,
EmptyRule,
RuleInput,
}, },
computed: { computed: {
...mapState(['settings']), ...mapState(['settings']),
...mapState({ ...mapState({
rules: state => state.approvals.rules, rules: state => state.approvals.rules,
}), }),
}, hasNamedRule() {
methods: { if (this.settings.allowMultiRule) {
...mapActions(['putRule']), return this.rules.some(rule => rule.ruleType !== RULE_TYPE_ANY_APPROVER);
canEdit(rule) { }
const { canEdit, allowMultiRule } = this.settings;
return canEdit && (!allowMultiRule || !rule.hasSource); const [rule] = this.rules;
return rule.ruleType
? rule.ruleType === RULE_TYPE_REGULAR
: rule.name !== RULE_NAME_ANY_APPROVER;
},
canEdit() {
return this.settings.canEdit;
},
},
watch: {
rules: {
handler(newValue) {
if (!this.settings.allowMultiRule && newValue.length === 0) {
this.setEmptyRule();
}
if (
this.settings.allowMultiRule &&
!newValue.some(rule => rule.ruleType === RULE_TYPE_ANY_APPROVER)
) {
this.addEmptyRule();
}
},
immediate: true,
}, },
}, },
methods: {
...mapActions(['setEmptyRule', 'addEmptyRule']),
},
}; };
</script> </script>
...@@ -31,32 +60,37 @@ export default { ...@@ -31,32 +60,37 @@ export default {
<rules :rules="rules"> <rules :rules="rules">
<template slot="thead" slot-scope="{ name, members, approvalsRequired }"> <template slot="thead" slot-scope="{ name, members, approvalsRequired }">
<tr> <tr>
<th v-if="settings.allowMultiRule">{{ name }}</th> <th :class="hasNamedRule ? 'w-25' : 'w-75'">{{ hasNamedRule ? name : members }}</th>
<th :class="settings.allowMultiRule ? 'w-50 d-none d-sm-table-cell' : 'w-75'"> <th :class="hasNamedRule ? 'w-75' : null">
{{ members }} <span v-if="hasNamedRule">{{ members }}</span>
</th> </th>
<th>{{ approvalsRequired }}</th> <th>{{ approvalsRequired }}</th>
<th></th> <th></th>
</tr> </tr>
</template> </template>
<template slot="tr" slot-scope="{ rule }"> <template slot="tbody" slot-scope="{ rules }">
<td v-if="settings.allowMultiRule" class="js-name">{{ rule.name }}</td> <template v-for="(rule, index) in rules">
<td class="js-members" :class="settings.allowMultiRule ? 'd-none d-sm-table-cell' : ''"> <empty-rule
v-if="rule.ruleType === 'any_approver'"
:key="index"
:rule="rule"
:allow-multi-rule="settings.allowMultiRule"
:eligible-approvers-docs-path="settings.eligibleApproversDocsPath"
:can-edit="canEdit"
/>
<tr v-else :key="index">
<td class="js-name">{{ rule.name }}</td>
<td class="js-members" :class="settings.allowMultiRule ? 'd-none d-sm-table-cell' : null">
<user-avatar-list :items="rule.approvers" :img-size="24" /> <user-avatar-list :items="rule.approvers" :img-size="24" />
</td> </td>
<td class="js-approvals-required"> <td class="js-approvals-required">
<input <rule-input :rule="rule" />
:value="rule.approvalsRequired"
:disabled="!settings.canEdit"
class="form-control mw-6em"
type="number"
:min="rule.minApprovalsRequired || 0"
@input="putRule({ id: rule.id, approvalsRequired: Number($event.target.value) })"
/>
</td> </td>
<td class="text-nowrap px-2 w-0 js-controls"> <td class="text-nowrap px-2 w-0 js-controls">
<rule-controls v-if="canEdit(rule)" :rule="rule" /> <rule-controls v-if="canEdit" :rule="rule" />
</td> </td>
</tr>
</template>
</template> </template>
</rules> </rules>
</template> </template>
...@@ -47,6 +47,8 @@ export default { ...@@ -47,6 +47,8 @@ export default {
/> />
<div v-for="rule in rules" :key="rule.id"> <div v-for="rule in rules" :key="rule.id">
<input v-if="!rule.isNew" :value="rule.id" :name="$options.INPUT_ID" type="hidden" /> <input v-if="!rule.isNew" :value="rule.id" :name="$options.INPUT_ID" type="hidden" />
<input v-else :name="$options.INPUT_ID" type="hidden" />
<input <input
v-if="rule.isNew && rule.hasSource" v-if="rule.isNew && rule.hasSource"
:value="rule.sourceId" :value="rule.sourceId"
......
<script>
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),
});
}
},
},
};
</script>
<template>
<input
:value="rule.approvalsRequired"
:disabled="!settings.canEdit"
class="form-control mw-6em"
type="number"
:min="rule.minApprovalsRequired || 0"
@input="onInputChange"
/>
</template>
<script> <script>
import { mapState } from 'vuex'; import { mapState, mapActions } from 'vuex';
import { n__, sprintf } from '~/locale'; import { n__, sprintf } from '~/locale';
import { RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR } from '../../constants';
import Icon from '~/vue_shared/components/icon.vue'; import Icon from '~/vue_shared/components/icon.vue';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import ApprovalCheckRulePopover from '../approval_check_rule_popover.vue'; import ApprovalCheckRulePopover from '../approval_check_rule_popover.vue';
import Rules from '../rules.vue'; import Rules from '../rules.vue';
import RuleControls from '../rule_controls.vue'; import RuleControls from '../rule_controls.vue';
import EmptyRule from '../mr_edit/empty_rule.vue';
import RuleInput from '../mr_edit/rule_input.vue';
export default { export default {
components: { components: {
...@@ -14,14 +17,39 @@ export default { ...@@ -14,14 +17,39 @@ export default {
Rules, Rules,
UserAvatarList, UserAvatarList,
ApprovalCheckRulePopover, ApprovalCheckRulePopover,
EmptyRule,
RuleInput,
}, },
computed: { computed: {
...mapState(['settings']), ...mapState(['settings']),
...mapState({ ...mapState({
rules: state => state.approvals.rules, rules: state => state.approvals.rules,
}), }),
hasNamedRule() {
return this.rules.some(rule => rule.ruleType === RULE_TYPE_REGULAR);
},
hasAnyRule() {
return (
this.settings.allowMultiRule &&
!this.rules.some(rule => rule.ruleType === RULE_TYPE_ANY_APPROVER)
);
},
},
watch: {
rules: {
handler(newValue) {
if (
this.settings.allowMultiRule &&
!newValue.some(rule => rule.ruleType === RULE_TYPE_ANY_APPROVER)
) {
this.addEmptyRule();
}
},
immediate: true,
},
}, },
methods: { methods: {
...mapActions(['addEmptyRule']),
summaryText(rule) { summaryText(rule) {
return this.settings.allowMultiRule return this.settings.allowMultiRule
? this.summaryMultipleRulesText(rule) ? this.summaryMultipleRulesText(rule)
...@@ -56,6 +84,11 @@ export default { ...@@ -56,6 +84,11 @@ export default {
{ name: rule.name, count: rule.approvalsRequired }, { name: rule.name, count: rule.approvalsRequired },
); );
}, },
canEdit(rule) {
const { canEdit, allowMultiRule } = this.settings;
return canEdit && (!allowMultiRule || !rule.hasSource);
},
}, },
}; };
</script> </script>
...@@ -64,29 +97,40 @@ export default { ...@@ -64,29 +97,40 @@ export default {
<rules :rules="rules"> <rules :rules="rules">
<template slot="thead" slot-scope="{ name, members, approvalsRequired }"> <template slot="thead" slot-scope="{ name, members, approvalsRequired }">
<tr class="d-none d-sm-table-row"> <tr class="d-none d-sm-table-row">
<th v-if="settings.allowMultiRule" class="w-25">{{ name }}</th> <th class="w-25">{{ hasNamedRule ? name : members }}</th>
<th class="w-50">{{ members }}</th> <th :class="settings.allowMultiRule ? 'w-50 d-none d-sm-table-cell' : 'w-75'">
<span v-if="hasNamedRule">{{ members }}</span>
</th>
<th>{{ approvalsRequired }}</th> <th>{{ approvalsRequired }}</th>
<th></th> <th></th>
</tr> </tr>
</template> </template>
<template slot="tr" slot-scope="{ rule }"> <template slot="tbody" slot-scope="{ rules }">
<td class="d-table-cell d-sm-none js-summary">{{ summaryText(rule) }}</td> <template v-for="(rule, index) in rules">
<td v-if="settings.allowMultiRule" class="d-none d-sm-table-cell js-name"> <empty-rule
{{ rule.name }} v-if="rule.ruleType === 'any_approver'"
<approval-check-rule-popover :key="index"
:rule="rule" :rule="rule"
:security-approvals-help-page-path="settings.securityApprovalsHelpPagePath" :allow-multi-rule="settings.allowMultiRule"
:is-mr-edit="false"
:eligible-approvers-docs-path="settings.eligibleApproversDocsPath"
:can-edit="canEdit(rule)"
/> />
<tr v-else :key="index">
<td class="js-name">
{{ rule.name }}
</td> </td>
<td class="d-none d-sm-table-cell js-members"> <td class="js-members" :class="settings.allowMultiRule ? 'd-none d-sm-table-cell' : null">
<user-avatar-list :items="rule.approvers" :img-size="24" /> <user-avatar-list :items="rule.approvers" :img-size="24" empty-text="" />
</td> </td>
<td class="d-none d-sm-table-cell js-approvals-required"> <td class="js-approvals-required">
<icon name="approval" class="align-top text-tertiary" /> <rule-input :rule="rule" />
<span>{{ rule.approvalsRequired }}</span>
</td> </td>
<td class="text-nowrap px-2 w-0 js-controls"><rule-controls :rule="rule" /></td> <td class="text-nowrap px-2 w-0 js-controls">
<rule-controls v-if="canEdit(rule)" :rule="rule" />
</td>
</tr>
</template>
</template> </template>
</rules> </rules>
</template> </template>
...@@ -16,9 +16,6 @@ export default { ...@@ -16,9 +16,6 @@ export default {
}, },
computed: { computed: {
...mapState(['settings']), ...mapState(['settings']),
isRemoveable() {
return !this.rule.isFallback && this.settings.allowMultiRule;
},
}, },
methods: { methods: {
...mapActions(['requestEditRule', 'requestDeleteRule']), ...mapActions(['requestEditRule', 'requestDeleteRule']),
...@@ -29,9 +26,9 @@ export default { ...@@ -29,9 +26,9 @@ export default {
<template> <template>
<div> <div>
<gl-button variant="none" @click="requestEditRule(rule)"> <gl-button variant="none" @click="requestEditRule(rule)">
<span>{{ __('Edit') }}</span> </gl-button <span>{{ __('Edit') }}</span>
><gl-button </gl-button>
v-if="isRemoveable" <gl-button
class="prepend-left-8 btn btn-inverted" class="prepend-left-8 btn btn-inverted"
variant="danger" variant="danger"
@click="requestDeleteRule(rule)" @click="requestDeleteRule(rule)"
......
...@@ -133,7 +133,15 @@ export default { ...@@ -133,7 +133,15 @@ export default {
}, },
}, },
methods: { methods: {
...mapActions(['putFallbackRule', 'postRule', 'putRule', 'deleteRule']), ...mapActions(['putFallbackRule', 'postRule', 'putRule', 'deleteRule', 'postRegularRule']),
addSelection() {
if (!this.approversToAdd.length) {
return;
}
this.approvers = this.approversToAdd.concat(this.approvers);
this.approversToAdd = [];
},
/** /**
* Validate and submit the form based on what type it is. * Validate and submit the form based on what type it is.
* - Fallback rule? * - Fallback rule?
...@@ -157,6 +165,10 @@ export default { ...@@ -157,6 +165,10 @@ export default {
submitRule() { submitRule() {
const data = this.submissionData; const data = this.submissionData;
if (!this.settings.allowMultiRule && this.settings.prefix === 'mr-edit') {
return data.id ? this.putRule(data) : this.postRegularRule(data);
}
return data.id ? this.putRule(data) : this.postRule(data); return data.id ? this.putRule(data) : this.postRule(data);
}, },
/** /**
...@@ -225,7 +237,7 @@ export default { ...@@ -225,7 +237,7 @@ export default {
<template> <template>
<form novalidate @submit.prevent.stop="submit"> <form novalidate @submit.prevent.stop="submit">
<div class="row"> <div class="row">
<div v-if="settings.allowMultiRule" class="form-group col-sm-6"> <div class="form-group col-sm-6">
<label class="label-wrapper"> <label class="label-wrapper">
<span class="mb-2 bold inline">{{ s__('ApprovalRule|Rule name') }}</span> <span class="mb-2 bold inline">{{ s__('ApprovalRule|Rule name') }}</span>
<input <input
......
...@@ -24,9 +24,7 @@ export default { ...@@ -24,9 +24,7 @@ export default {
<slot name="thead" v-bind="$options.HEADERS"></slot> <slot name="thead" v-bind="$options.HEADERS"></slot>
</thead> </thead>
<tbody> <tbody>
<tr v-for="rule in rules" :key="rule.id"> <slot name="tbody" :rules="rules"></slot>
<slot :rule="rule" name="tr"></slot>
</tr>
</tbody> </tbody>
</table> </table>
</template> </template>
...@@ -8,6 +8,8 @@ export const RULE_TYPE_FALLBACK = 'fallback'; ...@@ -8,6 +8,8 @@ export const RULE_TYPE_FALLBACK = 'fallback';
export const RULE_TYPE_REGULAR = 'regular'; export const RULE_TYPE_REGULAR = 'regular';
export const RULE_TYPE_REPORT_APPROVER = 'report_approver'; export const RULE_TYPE_REPORT_APPROVER = 'report_approver';
export const RULE_TYPE_CODE_OWNER = 'code_owner'; 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 VULNERABILITY_CHECK_NAME = 'Vulnerability-Check';
export const LICENSE_CHECK_NAME = 'License-Check'; export const LICENSE_CHECK_NAME = 'License-Check';
......
import _ from 'underscore'; import { RULE_TYPE_REGULAR, RULE_TYPE_ANY_APPROVER } from './constants';
import { RULE_TYPE_REGULAR, RULE_TYPE_FALLBACK } 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 => ({ export const mapApprovalRuleRequest = req => ({
name: req.name, name: req.name,
...@@ -23,10 +44,11 @@ export const mapApprovalRuleResponse = res => ({ ...@@ -23,10 +44,11 @@ export const mapApprovalRuleResponse = res => ({
containsHiddenGroups: res.contains_hidden_groups, containsHiddenGroups: res.contains_hidden_groups,
users: res.users, users: res.users,
groups: res.groups, groups: res.groups,
ruleType: res.rule_type,
}); });
export const mapApprovalSettingsResponse = res => ({ export const mapApprovalSettingsResponse = res => ({
rules: res.rules.map(mapApprovalRuleResponse), rules: withDefaultEmptyRule(res.rules.map(mapApprovalRuleResponse)),
fallbackApprovalsRequired: res.fallback_approvals_required, fallbackApprovalsRequired: res.fallback_approvals_required,
}); });
...@@ -52,19 +74,16 @@ export const mapMRSourceRule = ({ id, ...rule }) => ({ ...@@ -52,19 +74,16 @@ export const mapMRSourceRule = ({ id, ...rule }) => ({
* from the fallback rule. * from the fallback rule.
*/ */
export const mapMRApprovalSettingsResponse = res => { export const mapMRApprovalSettingsResponse = res => {
const rulesByType = _.groupBy(res.rules, x => x.rule_type); const rules = res.rules.filter(({ rule_type }) => visibleTypes.has(rule_type));
const regularRules = rulesByType[RULE_TYPE_REGULAR] || [];
const [fallback] = rulesByType[RULE_TYPE_FALLBACK] || []; const fallbackApprovalsRequired = res.fallback_approvals_required || 0;
const fallbackApprovalsRequired = fallback
? fallback.approvals_required
: res.fallback_approvals_required || 0;
return { return {
rules: regularRules rules: withDefaultEmptyRule(
rules
.map(mapApprovalRuleResponse) .map(mapApprovalRuleResponse)
.map(res.approval_rules_overwritten ? x => x : mapMRSourceRule), .map(res.approval_rules_overwritten ? x => x : mapMRSourceRule),
),
fallbackApprovalsRequired, fallbackApprovalsRequired,
minFallbackApprovalsRequired: 0, minFallbackApprovalsRequired: 0,
}; };
......
export const SET_LOADING = 'SET_LOADING'; export const SET_LOADING = 'SET_LOADING';
export const SET_APPROVAL_SETTINGS = 'SET_APPROVAL_SETTINGS'; export const SET_APPROVAL_SETTINGS = 'SET_APPROVAL_SETTINGS';
export const ADD_EMPTY_RULE = 'ADD_EMPTY_RULE';
import * as types from './mutation_types'; import * as types from './mutation_types';
import { RULE_TYPE_ANY_APPROVER } from '../../../constants';
export default { export default {
[types.SET_LOADING](state, isLoading) { [types.SET_LOADING](state, isLoading) {
...@@ -7,7 +8,22 @@ export default { ...@@ -7,7 +8,22 @@ export default {
[types.SET_APPROVAL_SETTINGS](state, settings) { [types.SET_APPROVAL_SETTINGS](state, settings) {
state.hasLoaded = true; state.hasLoaded = true;
state.rules = settings.rules; state.rules = settings.rules;
state.initialRules = settings.rules;
state.fallbackApprovalsRequired = settings.fallbackApprovalsRequired; state.fallbackApprovalsRequired = settings.fallbackApprovalsRequired;
state.minFallbackApprovalsRequired = settings.minFallbackApprovalsRequired; 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,
});
},
}; };
...@@ -4,4 +4,5 @@ export default () => ({ ...@@ -4,4 +4,5 @@ export default () => ({
rules: [], rules: [],
fallbackApprovalsRequired: 0, fallbackApprovalsRequired: 0,
minFallbackApprovalsRequired: 0, minFallbackApprovalsRequired: 0,
initialRules: [],
}); });
...@@ -4,6 +4,7 @@ import { __ } from '~/locale'; ...@@ -4,6 +4,7 @@ import { __ } from '~/locale';
import Api from '~/api'; import Api from '~/api';
import axios from '~/lib/utils/axios_utils'; import axios from '~/lib/utils/axios_utils';
import * as types from './mutation_types'; import * as types from './mutation_types';
import { RULE_TYPE_ANY_APPROVER } from '../../../constants';
import { mapMRApprovalSettingsResponse } from '../../../mappers'; import { mapMRApprovalSettingsResponse } from '../../../mappers';
const fetchGroupMembers = _.memoize(id => Api.groupMembers(id).then(response => response.data)); const fetchGroupMembers = _.memoize(id => Api.groupMembers(id).then(response => response.data));
...@@ -36,11 +37,16 @@ const seedLocalRule = rule => ...@@ -36,11 +37,16 @@ const seedLocalRule = rule =>
.then(seedUsers) .then(seedUsers)
.then(seedGroups); .then(seedGroups);
const seedNewRule = rule => ({ const seedNewRule = rule => {
const name = rule.ruleType === RULE_TYPE_ANY_APPROVER ? '' : rule.name;
return {
...rule, ...rule,
isNew: true, isNew: true,
name,
id: _.uniqueId('new'), id: _.uniqueId('new'),
}); };
};
export const requestRules = ({ commit }) => { export const requestRules = ({ commit }) => {
commit(types.SET_LOADING, true); commit(types.SET_LOADING, true);
...@@ -113,4 +119,25 @@ export const requestDeleteRule = ({ dispatch }, rule) => { ...@@ -113,4 +119,25 @@ export const requestDeleteRule = ({ dispatch }, rule) => {
dispatch('deleteRule', rule.id); 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 () => {}; export default () => {};
...@@ -4,3 +4,6 @@ export const DELETE_RULE = 'DELETE_RULE'; ...@@ -4,3 +4,6 @@ export const DELETE_RULE = 'DELETE_RULE';
export const PUT_RULE = 'PUT_RULE'; export const PUT_RULE = 'PUT_RULE';
export const POST_RULE = 'POST_RULE'; export const POST_RULE = 'POST_RULE';
export const SET_FALLBACK_RULE = 'SET_FALLBACK_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';
import _ from 'underscore'; import _ from 'underscore';
import base from '../base/mutations'; import base from '../base/mutations';
import * as types from './mutation_types'; import * as types from './mutation_types';
import { RULE_TYPE_ANY_APPROVER } from '../../../constants';
export default { export default {
...base, ...base,
...@@ -20,6 +21,19 @@ export default { ...@@ -20,6 +21,19 @@ export default {
state.rules.splice(idx, 1); 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 }) { [types.PUT_RULE](state, { id, ...newRule }) {
const idx = _.findIndex(state.rules, x => x.id === id); const idx = _.findIndex(state.rules, x => x.id === id);
...@@ -31,9 +45,45 @@ export default { ...@@ -31,9 +45,45 @@ export default {
state.rules.splice(idx, 1, rule); state.rules.splice(idx, 1, rule);
}, },
[types.POST_RULE](state, rule) { [types.POST_RULE](state, 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); state.rules.push(rule);
}
},
[types.POST_REGULAR_RULE](state, rule) {
state.rules.unshift(rule);
}, },
[types.SET_FALLBACK_RULE](state, fallback) { [types.SET_FALLBACK_RULE](state, fallback) {
state.fallbackApprovalsRequired = fallback.approvalsRequired; 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,
},
];
}
},
}; };
...@@ -103,4 +103,8 @@ export const requestDeleteRule = ({ dispatch }, rule) => { ...@@ -103,4 +103,8 @@ export const requestDeleteRule = ({ dispatch }, rule) => {
dispatch('deleteModal/open', rule); dispatch('deleteModal/open', rule);
}; };
export const addEmptyRule = ({ commit }) => {
commit(types.ADD_EMPTY_RULE);
};
export default () => {}; export default () => {};
...@@ -50,7 +50,7 @@ export default { ...@@ -50,7 +50,7 @@ export default {
return this.mr.approvals || {}; return this.mr.approvals || {};
}, },
hasFooter() { hasFooter() {
return Boolean(this.approvals.has_approval_rules); return Boolean(this.mr.approvals);
}, },
approvedBy() { approvedBy() {
return this.approvals.approved_by ? this.approvals.approved_by.map(x => x.user) : []; return this.approvals.approved_by ? this.approvals.approved_by.map(x => x.user) : [];
......
...@@ -3,7 +3,7 @@ import _ from 'underscore'; ...@@ -3,7 +3,7 @@ import _ from 'underscore';
import { sprintf, __ } from '~/locale'; import { sprintf, __ } from '~/locale';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import ApprovalCheckRulePopover from 'ee/approvals/components/approval_check_rule_popover.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'; import ApprovedIcon from './approved_icon.vue';
export default { export default {
...@@ -70,6 +70,9 @@ export default { ...@@ -70,6 +70,9 @@ export default {
name: rule.name, name: rule.name,
}); });
}, },
ruleName(rule) {
return rule.rule_type === RULE_TYPE_ANY_APPROVER ? __('Any eligible user') : rule.name;
},
}, },
}; };
</script> </script>
...@@ -96,7 +99,7 @@ export default { ...@@ -96,7 +99,7 @@ export default {
<td class="w-0"><approved-icon :is-approved="rule.approved" /></td> <td class="w-0"><approved-icon :is-approved="rule.approved" /></td>
<td :colspan="rule.fallback ? 2 : 1"> <td :colspan="rule.fallback ? 2 : 1">
<div class="d-none d-sm-block js-name" :class="rule.nameClass"> <div class="d-none d-sm-block js-name" :class="rule.nameClass">
{{ rule.name }} {{ ruleName(rule) }}
<approval-check-rule-popover <approval-check-rule-popover
:rule="rule" :rule="rule"
:security-approvals-help-page-path="securityApprovalsHelpPagePath" :security-approvals-help-page-path="securityApprovalsHelpPagePath"
......
# frozen_string_literal: true
class ApprovalMergeRequestFallback
attr_reader :merge_request
delegate :approved_by_users, :project, to: :merge_request
def initialize(merge_request)
@merge_request = merge_request
end
# Implements all `WrappedApprovalRule` required methods
def id
'fallback-rule'
end
def name
''
end
def users
User.none
end
def groups
Group.none
end
def approvals_required
@approvals_required ||= [merge_request.approvals_before_merge.to_i,
project.min_fallback_approvals].max
end
def approvals_left
@approvals_left ||= [approvals_required - approved_by_users.size, 0].max
end
def approvers
[]
end
def approved_approvers
approved_by_users
end
def approved?
approved_approvers.size >= approvals_required
end
def code_owner
false
end
def source_rule
nil
end
def regular
false
end
def rule_type
:fallback
end
def project
merge_request.target_project
end
end
...@@ -116,7 +116,9 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -116,7 +116,9 @@ class ApprovalMergeRequestRule < ApplicationRecord
# Before being merged, approved_approvers are dynamically calculated in ApprovalWrappedRule instead of being persisted. # Before being merged, approved_approvers are dynamically calculated in ApprovalWrappedRule instead of being persisted.
return unless merge_request.merged? 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 end
def refresh_required_approvals!(project_approval_rule) def refresh_required_approvals!(project_approval_rule)
......
...@@ -44,30 +44,13 @@ class ApprovalState ...@@ -44,30 +44,13 @@ class ApprovalState
strong_memoize(:wrapped_approval_rules) do strong_memoize(:wrapped_approval_rules) do
next [] unless approval_feature_available? next [] unless approval_feature_available?
result = use_fallback? ? [fallback_rule] : regular_rules user_defined_rules + code_owner_rules + report_approver_rules
result += code_owner_rules
result += report_approver_rules
result
end end
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) # Determines which set of rules to use (MR or project)
def approval_rules_overwritten? def approval_rules_overwritten?
regular_merge_request_rules.any? { |rule| rule.approvers.present? } || project.can_override_approvers? && user_defined_merge_request_rules.any?
(project.can_override_approvers? && merge_request.approvals_before_merge.present?)
end end
alias_method :approvers_overwritten?, :approval_rules_overwritten? alias_method :approvers_overwritten?, :approval_rules_overwritten?
...@@ -83,10 +66,6 @@ class ApprovalState ...@@ -83,10 +66,6 @@ class ApprovalState
end end
end end
def any_approver_allowed?
!has_regular_rule_with_approvers? || approved?
end
def approvals_required def approvals_required
strong_memoize(:approvals_required) do strong_memoize(:approvals_required) do
wrapped_approval_rules.sum(&:approvals_required) wrapped_approval_rules.sum(&:approvals_required)
...@@ -109,16 +88,12 @@ class ApprovalState ...@@ -109,16 +88,12 @@ class ApprovalState
strong_memoize(:approvers) { filtered_approvers(target: :approvers) } strong_memoize(:approvers) { filtered_approvers(target: :approvers) }
end end
# @param regular [Boolean]
# @param code_owner [Boolean] # @param code_owner [Boolean]
# @param report_approver [Boolean]
# @param target [:approvers, :users] # @param target [:approvers, :users]
# @param unactioned [Boolean] # @param unactioned [Boolean]
def filtered_approvers(regular: true, code_owner: true, report_approver: true, target: :approvers, unactioned: false) def filtered_approvers(code_owner: true, target: :approvers, unactioned: false)
rules = [] rules = user_defined_rules + report_approver_rules
rules.concat(regular_rules) if regular
rules.concat(code_owner_rules) if code_owner rules.concat(code_owner_rules) if code_owner
rules.concat(report_approver_rules) if report_approver
filter_approvers(rules.flat_map(&target), unactioned: unactioned) filter_approvers(rules.flat_map(&target), unactioned: unactioned)
end end
...@@ -142,7 +117,6 @@ class ApprovalState ...@@ -142,7 +117,6 @@ class ApprovalState
return false unless user.can?(:approve_merge_request, merge_request) return false unless user.can?(:approve_merge_request, merge_request)
return true if unactioned_approvers.include?(user) return true if unactioned_approvers.include?(user)
return false unless any_approver_allowed?
# Users can only approve once. # Users can only approve once.
return false if approvals.where(user: user).any? return false if approvals.where(user: user).any?
# At this point, follow self-approval rules. Otherwise authors must # At this point, follow self-approval rules. Otherwise authors must
...@@ -171,9 +145,22 @@ class ApprovalState ...@@ -171,9 +145,22 @@ class ApprovalState
# This is a temporary method for backward compatibility # This is a temporary method for backward compatibility
# before introduction of approval rules. # before introduction of approval rules.
# This avoids re-queries. # This avoids re-queries.
# https://gitlab.com/gitlab-org/gitlab/issues/33329
def first_regular_rule def first_regular_rule
strong_memoize(:first_regular_rule) do 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
end end
...@@ -187,43 +174,39 @@ class ApprovalState ...@@ -187,43 +174,39 @@ class ApprovalState
self.class.filter_committers(approvers, merge_request) self.class.filter_committers(approvers, merge_request)
end end
def has_regular_rule_with_approvers? def user_defined_merge_request_rules
regular_rules.any? { |rule| rule.approvers.present? } strong_memoize(:user_defined_merge_request_rules) do
end # Filter out the rules without approvers since such rules aren't useful
regular_rules =
def regular_rules wrapped_rules
strong_memoize(:regular_rules) do .select { |rule| rule.regular? && rule.approvers.present? }
rules = approval_rules_overwritten? ? regular_merge_request_rules : regular_project_rules .sort_by(&:id)
unless project.multiple_approval_rules_available? any_approver_rules =
rules = rules[0, 1] wrapped_rules.select(&:any_approver?)
end
wrap_rules(rules) rules = any_approver_rules + regular_rules
project.multiple_approval_rules_available? ? rules : rules.take(1)
end end
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 def code_owner_rules
strong_memoize(:code_owner_rules) do strong_memoize(:code_owner_rules) do
wrap_rules(merge_request.approval_rules.select(&:code_owner?)) wrapped_rules.select(&:code_owner?)
end end
end end
def report_approver_rules def report_approver_rules
strong_memoize(:report_approver_rules) do strong_memoize(:report_approver_rules) do
wrap_rules(merge_request.approval_rules.select(&:report_approver?)) wrapped_rules.select(&:report_approver?)
end end
end end
def wrap_rules(rules) def wrapped_rules
rules.map { |rule| ApprovalWrappedRule.new(merge_request, rule) } strong_memoize(:wrapped_rules) do
merge_request.approval_rules.map do |rule|
ApprovalWrappedRule.wrap(merge_request, rule)
end
end
end end
end end
# 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
# 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
...@@ -5,14 +5,23 @@ class ApprovalWrappedRule ...@@ -5,14 +5,23 @@ class ApprovalWrappedRule
extend Forwardable extend Forwardable
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
REQUIRED_APPROVALS_PER_CODE_OWNER_RULE = 1
attr_reader :merge_request attr_reader :merge_request
attr_reader :approval_rule attr_reader :approval_rule
def_delegators(:@approval_rule, def_delegators(:@approval_rule,
:id, :name, :users, :groups, :code_owner, :code_owner?, :source_rule, :regular?, :any_approver?, :code_owner?, :report_approver?,
:rule_type) :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) def initialize(merge_request, approval_rule)
@merge_request = merge_request @merge_request = merge_request
...@@ -24,10 +33,7 @@ class ApprovalWrappedRule ...@@ -24,10 +33,7 @@ class ApprovalWrappedRule
end end
def approvers def approvers
filtered_approvers = filter_approvers(@approval_rule.approvers)
ApprovalState.filter_author(@approval_rule.approvers, merge_request)
ApprovalState.filter_committers(filtered_approvers, merge_request)
end end
# @return [Array<User>] all approvers related to this rule # @return [Array<User>] all approvers related to this rule
...@@ -47,7 +53,7 @@ class ApprovalWrappedRule ...@@ -47,7 +53,7 @@ class ApprovalWrappedRule
end end
strong_memoize(:approved_approvers) do 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| approvers.select do |approver|
overall_approver_ids.include?(approver.id) overall_approver_ids.include?(approver.id)
...@@ -80,27 +86,12 @@ class ApprovalWrappedRule ...@@ -80,27 +86,12 @@ class ApprovalWrappedRule
approvers - approved_approvers approvers - approved_approvers
end end
def approvals_required
if code_owner?
code_owner_approvals_required
else
approval_rule.approvals_required
end
end
private private
def code_owner_approvals_required def filter_approvers(approvers)
strong_memoize(:code_owner_approvals_required) do filtered_approvers =
next 0 unless branch_requires_code_owner_approval? ApprovalState.filter_author(approvers, merge_request)
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?
ProtectedBranch.branch_requires_code_owner_approval?(project, merge_request.target_branch) ApprovalState.filter_committers(filtered_approvers, merge_request)
end end
end end
...@@ -13,7 +13,6 @@ module Approvable ...@@ -13,7 +13,6 @@ module Approvable
approvals_left approvals_left
can_approve? can_approve?
has_approved? has_approved?
any_approver_allowed?
authors_can_approve? authors_can_approve?
committers_can_approve? committers_can_approve?
approvers_overwritten? approvers_overwritten?
......
...@@ -22,6 +22,7 @@ module ApprovalRuleLike ...@@ -22,6 +22,7 @@ module ApprovalRuleLike
validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } 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 :with_users, -> { preload(:users, :group_users) }
scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) }
end end
# Users who are eligible to approve, including specified group members. # Users who are eligible to approve, including specified group members.
...@@ -45,4 +46,8 @@ module ApprovalRuleLike ...@@ -45,4 +46,8 @@ module ApprovalRuleLike
def report_approver? def report_approver?
raise NotImplementedError raise NotImplementedError
end end
def any_approver?
raise NotImplementedError
end
end end
...@@ -390,6 +390,7 @@ module EE ...@@ -390,6 +390,7 @@ module EE
default_issues_tracker? || jira_tracker_active? default_issues_tracker? || jira_tracker_active?
end end
# TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329
def approvals_before_merge def approvals_before_merge
return 0 unless feature_available?(:merge_request_approvers) return 0 unless feature_available?(:merge_request_approvers)
...@@ -398,24 +399,24 @@ module EE ...@@ -398,24 +399,24 @@ module EE
def visible_approval_rules def visible_approval_rules
strong_memoize(:visible_approval_rules) do strong_memoize(:visible_approval_rules) do
visible_regular_approval_rules + approval_rules.report_approver visible_user_defined_rules + approval_rules.report_approver
end end
end end
def visible_regular_approval_rules def visible_user_defined_rules
strong_memoize(:visible_regular_approval_rules) do strong_memoize(:visible_user_defined_rules) do
regular_rules = approval_rules.regular.order(:id) 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
end end
# TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329
def min_fallback_approvals def min_fallback_approvals
strong_memoize(:min_fallback_approvals) do strong_memoize(:min_fallback_approvals) do
visible_regular_approval_rules.map(&:approvals_required).max || visible_user_defined_rules.map(&:approvals_required).max.to_i
approvals_before_merge.to_i
end end
end end
......
...@@ -14,6 +14,7 @@ module ApprovalRules ...@@ -14,6 +14,7 @@ module ApprovalRules
params.reverse_merge!(rule_type: :report_approver) params.reverse_merge!(rule_type: :report_approver)
end end
handle_any_approver_rule_creation(target, params) if target.is_a?(Project)
copy_approval_project_rule_properties(params) if target.is_a?(MergeRequest) copy_approval_project_rule_properties(params) if target.is_a?(MergeRequest)
super(@rule.project, user, params) super(@rule.project, user, params)
...@@ -36,5 +37,17 @@ module ApprovalRules ...@@ -36,5 +37,17 @@ module ApprovalRules
params[:users] = approval_project_rule.users params[:users] = approval_project_rule.users
params[:groups] = approval_project_rule.groups params[:groups] = approval_project_rule.groups
end 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
end end
...@@ -46,6 +46,11 @@ module ApprovalRules ...@@ -46,6 +46,11 @@ module ApprovalRules
provided_user_ids = rule_attributes[:user_ids].map(&:to_i) provided_user_ids = rule_attributes[:user_ids].map(&:to_i)
rule_attributes[:user_ids] = provided_user_ids & visible_user_ids rule_attributes[:user_ids] = provided_user_ids & visible_user_ids
end 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 end
# Append hidden groups to params to prevent them from being removed, # Append hidden groups to params to prevent them from being removed,
......
...@@ -8,6 +8,7 @@ ...@@ -8,6 +8,7 @@
'settings_path': expose_path(api_v4_projects_approval_settings_path(id: @project.id)), '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)), 'rules_path': expose_path(api_v4_projects_approval_settings_rules_path(id: @project.id)),
'allow_multi_rule': @project.multiple_approval_rules_available?.to_s, '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')} } '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 .text-center.prepend-top-default
= sprite_icon('spinner', size: 24, css_class: 'gl-spinner') = sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
......
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s, 'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s,
'mr_id': issuable.iid, 'mr_id': issuable.iid,
'mr_settings_path': presenter.api_approval_settings_path, '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 } } 'project_settings_path': presenter.api_project_approval_settings_path } }
= sprite_icon('spinner', size: 24, css_class: 'gl-spinner') = sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
- if can_update_approvers - if can_update_approvers
......
---
title: Add new approval rule type which allows anyone to approve
merge_request: 15378
author:
type: added
...@@ -530,7 +530,7 @@ module EE ...@@ -530,7 +530,7 @@ module EE
expose :approval_rules_left, using: ApprovalRuleShort expose :approval_rules_left, using: ApprovalRuleShort
expose :has_approval_rules do |approval_state| expose :has_approval_rules do |approval_state|
approval_state.has_non_fallback_rules? approval_state.user_defined_rules.present?
end end
expose :merge_request_approvers_available do |approval_state| expose :merge_request_approvers_available do |approval_state|
......
...@@ -29,6 +29,11 @@ FactoryBot.define do ...@@ -29,6 +29,11 @@ FactoryBot.define do
end end
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 factory :approval_project_rule do
project project
sequence(:name) { |n| "#{ApprovalRuleLike::DEFAULT_NAME}-#{n}" } sequence(:name) { |n| "#{ApprovalRuleLike::DEFAULT_NAME}-#{n}" }
......
...@@ -7,10 +7,7 @@ describe "User creates a merge request", :js do ...@@ -7,10 +7,7 @@ describe "User creates a merge request", :js do
let(:approver) { create(:user) } let(:approver) { create(:user) }
let(:project) do let(:project) do
create(:project, create(:project, :repository, merge_requests_template: template_text)
:repository,
approvals_before_merge: 1,
merge_requests_template: template_text)
end end
let(:template_text) { "This merge request should contain the following." } let(:template_text) { "This merge request should contain the following." }
let(:title) { "Some feature" } let(:title) { "Some feature" }
......
...@@ -17,29 +17,11 @@ describe 'Merge request > User selects branches for new MR', :js do ...@@ -17,29 +17,11 @@ describe 'Merge request > User selects branches for new MR', :js do
sign_in(user) sign_in(user)
end end
context 'when approvals are zero for the target project' do context 'create a merge request for the selected branches' do
before 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' }) visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature_conflict' })
end 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 context 'saving the MR' do
it 'shows the saved MR' do it 'shows the saved MR' do
fill_in 'merge_request_title', with: 'Test' fill_in 'merge_request_title', with: 'Test'
......
...@@ -7,7 +7,7 @@ describe 'Merge request > User sets approvers', :js do ...@@ -7,7 +7,7 @@ describe 'Merge request > User sets approvers', :js do
include FeatureApprovalHelper include FeatureApprovalHelper
let(:user) { create(:user) } 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!(:config_selector) { '.js-approval-rules' }
let!(:modal_selector) { '#mr-edit-approvals-create-modal' } let!(:modal_selector) { '#mr-edit-approvals-create-modal' }
...@@ -24,7 +24,7 @@ describe 'Merge request > User sets approvers', :js do ...@@ -24,7 +24,7 @@ describe 'Merge request > User sets approvers', :js do
end end
it 'does not allow setting the author as an approver but allows setting the current user as an approver' do 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 open_approver_select
expect(find('.select2-results')).not_to have_content(author.name) expect(find('.select2-results')).not_to have_content(author.name)
...@@ -46,7 +46,7 @@ describe 'Merge request > User sets approvers', :js do ...@@ -46,7 +46,7 @@ describe 'Merge request > User sets approvers', :js do
end 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 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 open_approver_select
expect(find('.select2-results')).to have_content(other_user.name) expect(find('.select2-results')).to have_content(other_user.name)
...@@ -71,7 +71,7 @@ describe 'Merge request > User sets approvers', :js do ...@@ -71,7 +71,7 @@ describe 'Merge request > User sets approvers', :js do
visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' }) 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 open_approver_select
expect(find('.select2-results')).not_to have_content(group.name) expect(find('.select2-results')).not_to have_content(group.name)
...@@ -84,7 +84,10 @@ describe 'Merge request > User sets approvers', :js do ...@@ -84,7 +84,10 @@ describe 'Merge request > User sets approvers', :js do
find('.select2-results .user-result', text: group.name).click find('.select2-results .user-result', text: group.name).click
close_approver_select close_approver_select
click_button 'Update approval rule'
within('.modal-content') do
click_button 'Add approval rule'
end
click_on("Submit merge request") click_on("Submit merge request")
wait_for_all_requests wait_for_all_requests
...@@ -137,7 +140,7 @@ describe 'Merge request > User sets approvers', :js do ...@@ -137,7 +140,7 @@ describe 'Merge request > User sets approvers', :js do
visit edit_project_merge_request_path(project, merge_request) visit edit_project_merge_request_path(project, merge_request)
open_modal open_modal(text: 'Add approval rule')
open_approver_select open_approver_select
expect(find('.select2-results')).not_to have_content(group.name) expect(find('.select2-results')).not_to have_content(group.name)
...@@ -150,7 +153,9 @@ describe 'Merge request > User sets approvers', :js do ...@@ -150,7 +153,9 @@ describe 'Merge request > User sets approvers', :js do
find('.select2-results .user-result', text: group.name).click find('.select2-results .user-result', text: group.name).click
close_approver_select close_approver_select
click_button 'Update approval rule' within('.modal-content') do
click_button 'Add approval rule'
end
click_on("Save changes") click_on("Save changes")
wait_for_all_requests wait_for_all_requests
......
...@@ -6,7 +6,7 @@ describe 'Project settings > [EE] Merge Requests', :js do ...@@ -6,7 +6,7 @@ describe 'Project settings > [EE] Merge Requests', :js do
include FeatureApprovalHelper include FeatureApprovalHelper
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, approvals_before_merge: 1) } let(:project) { create(:project) }
let(:group) { create(:group) } let(:group) { create(:group) }
let(:group_member) { create(:user) } let(:group_member) { create(:user) }
let(:non_member) { create(:user) } let(:non_member) { create(:user) }
...@@ -23,7 +23,7 @@ describe 'Project settings > [EE] Merge Requests', :js do ...@@ -23,7 +23,7 @@ describe 'Project settings > [EE] Merge Requests', :js do
it 'adds approver' do it 'adds approver' do
visit edit_project_path(project) visit edit_project_path(project)
open_modal open_modal(text: 'Add approval rule')
open_approver_select open_approver_select
expect(find('.select2-results')).to have_content(user.name) expect(find('.select2-results')).to have_content(user.name)
...@@ -39,7 +39,9 @@ describe 'Project settings > [EE] Merge Requests', :js do ...@@ -39,7 +39,9 @@ describe 'Project settings > [EE] Merge Requests', :js do
expect(find('.select2-results')).not_to have_content(user.name) expect(find('.select2-results')).not_to have_content(user.name)
close_approver_select close_approver_select
click_button 'Update approval rule' within('.modal-content') do
click_button 'Add approval rule'
end
wait_for_requests wait_for_requests
expect_avatar(find('.js-members'), user) expect_avatar(find('.js-members'), user)
...@@ -48,7 +50,7 @@ describe 'Project settings > [EE] Merge Requests', :js do ...@@ -48,7 +50,7 @@ describe 'Project settings > [EE] Merge Requests', :js do
it 'adds approver group' do it 'adds approver group' do
visit edit_project_path(project) visit edit_project_path(project)
open_modal open_modal(text: 'Add approval rule')
open_approver_select open_approver_select
expect(find('.select2-results')).to have_content(group.name) expect(find('.select2-results')).to have_content(group.name)
...@@ -58,7 +60,9 @@ describe 'Project settings > [EE] Merge Requests', :js do ...@@ -58,7 +60,9 @@ describe 'Project settings > [EE] Merge Requests', :js do
expect(find('.content-list')).to have_content(group.name) 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 wait_for_requests
expect_avatar(find('.js-members'), group.users) expect_avatar(find('.js-members'), group.users)
......
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');
});
});
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);
});
});
});
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,
);
});
});
});
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,
);
});
});
...@@ -96,10 +96,10 @@ describe('EE Approvals App', () => { ...@@ -96,10 +96,10 @@ describe('EE Approvals App', () => {
}; };
}); });
it('does not show Rules', () => { it('does show Rules', () => {
factory(); factory();
expect(findRules().exists()).toBe(false); expect(findRules().exists()).toBe(true);
}); });
it('shows loading icon if loading', () => { it('shows loading icon if loading', () => {
......
...@@ -3,14 +3,13 @@ import Vuex from 'vuex'; ...@@ -3,14 +3,13 @@ import Vuex from 'vuex';
import { createStoreOptions } from 'ee/approvals/stores'; import { createStoreOptions } from 'ee/approvals/stores';
import MREditModule from 'ee/approvals/stores/modules/mr_edit'; import MREditModule from 'ee/approvals/stores/modules/mr_edit';
import MREditApp from 'ee/approvals/components/mr_edit/app.vue'; 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 MRRules from 'ee/approvals/components/mr_edit/mr_rules.vue';
import MRRulesHiddenInputs from 'ee/approvals/components/mr_edit/mr_rules_hidden_inputs.vue'; import MRRulesHiddenInputs from 'ee/approvals/components/mr_edit/mr_rules_hidden_inputs.vue';
const localVue = createLocalVue(); const localVue = createLocalVue();
localVue.use(Vuex); localVue.use(Vuex);
describe('EE Approvlas MREditApp', () => { describe('EE Approvals MREditApp', () => {
let wrapper; let wrapper;
let store; let store;
...@@ -37,12 +36,8 @@ describe('EE Approvlas MREditApp', () => { ...@@ -37,12 +36,8 @@ describe('EE Approvlas MREditApp', () => {
factory(); factory();
}); });
it('renders MR fallback rules', () => {
expect(wrapper.find(MRFallbackRules).exists()).toBe(true);
});
it('does not render MR rules', () => { it('does not render MR rules', () => {
expect(wrapper.find(MRRules).exists()).toBe(false); expect(wrapper.find(MRRules).exists()).toBe(true);
}); });
it('renders hidden inputs', () => { it('renders hidden inputs', () => {
...@@ -56,10 +51,6 @@ describe('EE Approvlas MREditApp', () => { ...@@ -56,10 +51,6 @@ describe('EE Approvlas MREditApp', () => {
factory(); factory();
}); });
it('does not render MR fallback rules', () => {
expect(wrapper.find(MRFallbackRules).exists()).toBe(false);
});
it('renders MR rules', () => { it('renders MR rules', () => {
expect(wrapper.find(MRRules).exists()).toBe(true); expect(wrapper.find(MRRules).exists()).toBe(true);
}); });
......
...@@ -163,10 +163,10 @@ describe('EE Approvlas MRRulesHiddenInputs', () => { ...@@ -163,10 +163,10 @@ describe('EE Approvlas MRRulesHiddenInputs', () => {
rule.isNew = true; rule.isNew = true;
}); });
it('does not render id input', () => { it('does render id input', () => {
factory(); factory();
expect(findHiddenInputs().map(x => x.name)).not.toContain(INPUT_ID); expect(findHiddenInputs().map(x => x.name)).toContain(INPUT_ID);
}); });
describe('with source', () => { describe('with source', () => {
......
...@@ -6,7 +6,7 @@ import MRRules from 'ee/approvals/components/mr_edit/mr_rules.vue'; ...@@ -6,7 +6,7 @@ import MRRules from 'ee/approvals/components/mr_edit/mr_rules.vue';
import Rules from 'ee/approvals/components/rules.vue'; import Rules from 'ee/approvals/components/rules.vue';
import RuleControls from 'ee/approvals/components/rule_controls.vue'; import RuleControls from 'ee/approvals/components/rule_controls.vue';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.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; const { HEADERS } = Rules;
...@@ -37,7 +37,6 @@ describe('EE Approvals MRRules', () => { ...@@ -37,7 +37,6 @@ describe('EE Approvals MRRules', () => {
.find('td.js-members') .find('td.js-members')
.find(UserAvatarList) .find(UserAvatarList)
.props('items'); .props('items');
const findRuleApprovalsRequired = () => wrapper.find('td.js-approvals-required input');
const findRuleControls = () => wrapper.find('td.js-controls').find(RuleControls); const findRuleControls = () => wrapper.find('td.js-controls').find(RuleControls);
beforeEach(() => { beforeEach(() => {
...@@ -59,57 +58,70 @@ describe('EE Approvals MRRules', () => { ...@@ -59,57 +58,70 @@ describe('EE Approvals MRRules', () => {
describe('when allow multiple rules', () => { describe('when allow multiple rules', () => {
beforeEach(() => { beforeEach(() => {
store.state.settings.allowMultiRule = true; 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(); factory();
expect(findHeaders()).toEqual([HEADERS.name, HEADERS.members, HEADERS.approvalsRequired, '']); expect(store.modules.approvals.state.rules.length).toBe(2);
}); });
describe('with sourced MR rule', () => { it('should always display any_approver first', () => {
const expected = createMRRuleWithSource(); store.modules.approvals.state.rules = [createMRRule()];
factory();
beforeEach(() => { expect(store.modules.approvals.state.rules[0].ruleType).toBe('any_approver');
approvalRules = [createMRRuleWithSource()]; });
it('should only have 1 any_approver', () => {
store.modules.approvals.state.rules = [createEmptyRule(), createMRRule()];
factory(); factory();
});
it('shows name', () => { const anyApproverCount = store.modules.approvals.state.rules.filter(
expect(findRuleName().text()).toEqual(expected.name); rule => rule.ruleType === 'any_approver',
);
expect(anyApproverCount.length).toBe(1);
}); });
it('shows members', () => { it('renders headers when there are multiple rules', () => {
expect(findRuleMembers()).toEqual(expected.approvers); store.modules.approvals.state.rules = [createEmptyRule(), createMRRule()];
factory();
expect(findHeaders()).toEqual([HEADERS.name, HEADERS.members, HEADERS.approvalsRequired, '']);
}); });
it('shows approvals required input', () => { it('renders headers when there is a single any rule', () => {
const approvalsRequired = findRuleApprovalsRequired(); store.modules.approvals.state.rules = [createEmptyRule()];
factory();
expect(Number(approvalsRequired.element.value)).toEqual(expected.approvalsRequired); expect(findHeaders()).toEqual([HEADERS.members, '', HEADERS.approvalsRequired, '']);
expect(Number(approvalsRequired.attributes('min'))).toEqual(expected.minApprovalsRequired);
expect(approvalsRequired.attributes('disabled')).toBeUndefined();
}); });
it('does not show controls', () => { it('renders headers when there is a single named rule', () => {
const controls = findRuleControls(); store.modules.approvals.state.rules = [createMRRule()];
factory();
expect(controls.exists()).toBe(false); expect(findHeaders()).toEqual([HEADERS.name, HEADERS.members, HEADERS.approvalsRequired, '']);
}); });
it('dispatches putRule on change of approvals required', () => { describe('with sourced MR rule', () => {
const action = store.modules.approvals.actions.putRule; const expected = createMRRuleWithSource();
const approvalsRequired = findRuleApprovalsRequired();
const newValue = expected.approvalsRequired + 1;
approvalsRequired.setValue(newValue); beforeEach(() => {
approvalRules = [createMRRuleWithSource()];
expect(action).toHaveBeenCalledWith( factory();
jasmine.anything(), });
{ id: expected.id, approvalsRequired: newValue },
undefined, it('shows name', () => {
); expect(findRuleName().text()).toEqual(expected.name);
});
it('shows members', () => {
expect(findRuleMembers()).toEqual(expected.approvers);
}); });
}); });
...@@ -129,20 +141,6 @@ describe('EE Approvals MRRules', () => { ...@@ -129,20 +141,6 @@ describe('EE Approvals MRRules', () => {
expect(controls.props('rule')).toEqual(expected); 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', () => { describe('with settings cannot edit', () => {
beforeEach(() => { beforeEach(() => {
store.state.settings.canEdit = false; store.state.settings.canEdit = false;
...@@ -154,12 +152,6 @@ describe('EE Approvals MRRules', () => { ...@@ -154,12 +152,6 @@ describe('EE Approvals MRRules', () => {
expect(controls.exists()).toBe(false); 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', () => { ...@@ -167,23 +159,50 @@ describe('EE Approvals MRRules', () => {
describe('when allow single rule', () => { describe('when allow single rule', () => {
beforeEach(() => { beforeEach(() => {
store.state.settings.allowMultiRule = false; store.state.settings.allowMultiRule = false;
store.state.settings.eligibleApproversDocsPath = 'some/path';
});
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', () => { it('does not show name header for any rule', () => {
store.modules.approvals.state.rules = [createEmptyRule()];
factory(); factory();
expect(findHeaders()).not.toContain(HEADERS.name); 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', () => { describe('with source rule', () => {
const expected = createMRRuleWithSource();
beforeEach(() => { beforeEach(() => {
approvalRules = [createMRRuleWithSource()]; approvalRules = [createMRRuleWithSource()];
factory(); factory();
}); });
it('does not show name', () => { it('shows name', () => {
expect(findRuleName().exists()).toBe(false); expect(findRuleName().text()).toEqual(expected.name);
}); });
it('shows controls', () => { it('shows controls', () => {
......
...@@ -4,7 +4,7 @@ import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list ...@@ -4,7 +4,7 @@ import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list
import { createStoreOptions } from 'ee/approvals/stores'; import { createStoreOptions } from 'ee/approvals/stores';
import projectSettingsModule from 'ee/approvals/stores/modules/project_settings'; import projectSettingsModule from 'ee/approvals/stores/modules/project_settings';
import ProjectRules from 'ee/approvals/components/project_settings/project_rules.vue'; 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'; import { createProjectRules } from '../../mocks';
const TEST_RULES = createProjectRules(); const TEST_RULES = createProjectRules();
...@@ -15,18 +15,13 @@ localVue.use(Vuex); ...@@ -15,18 +15,13 @@ localVue.use(Vuex);
const findCell = (tr, name) => tr.find(`td.js-${name}`); const findCell = (tr, name) => tr.find(`td.js-${name}`);
const getRowData = tr => { const getRowData = tr => {
const summary = findCell(tr, 'summary');
const name = findCell(tr, 'name'); const name = findCell(tr, 'name');
const members = findCell(tr, 'members'); const members = findCell(tr, 'members');
const controls = findCell(tr, 'controls');
const approvalsRequired = findCell(tr, 'approvals-required'); const approvalsRequired = findCell(tr, 'approvals-required');
return { return {
name: name.text(), name: name.text(),
summary: summary.text(),
approvers: members.find(UserAvatarList).props('items'), approvers: members.find(UserAvatarList).props('items'),
approvalsRequired: Number(approvalsRequired.text()), approvalsRequired: approvalsRequired.find(RuleInput).props('rule').approvalsRequired,
ruleControl: controls.find(RuleControls).props('rule'),
}; };
}; };
...@@ -60,19 +55,26 @@ describe('Approvals ProjectRules', () => { ...@@ -60,19 +55,26 @@ describe('Approvals ProjectRules', () => {
it('renders row for each rule', () => { it('renders row for each rule', () => {
factory(); factory();
const rows = wrapper.findAll('tbody tr'); const rows = wrapper.findAll('tbody tr').filter((tr, index) => index !== 0);
const data = rows.wrappers.map(getRowData); const data = rows.wrappers.map(getRowData);
expect(data).toEqual( expect(data).toEqual(
TEST_RULES.map(rule => ({ TEST_RULES.filter((rule, index) => index !== 0).map(rule => ({
name: rule.name, name: rule.name,
summary: jasmine.stringMatching(`${rule.approvalsRequired} approval.*from ${rule.name}`),
approvalsRequired: rule.approvalsRequired,
approvers: rule.approvers, 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', () => { describe('when only allow single rule', () => {
...@@ -92,10 +94,10 @@ describe('Approvals ProjectRules', () => { ...@@ -92,10 +94,10 @@ describe('Approvals ProjectRules', () => {
expect(findCell(row, 'name').exists()).toBe(false); expect(findCell(row, 'name').exists()).toBe(false);
}); });
it('renders single summary', () => { it('should only display 1 rule', () => {
expect(findCell(row, 'summary').text()).toEqual( factory();
`${rule.approvalsRequired} approvals required from ${rule.approvers.length} members`,
); expect(store.modules.approvals.state.rules.length).toBe(1);
}); });
}); });
...@@ -114,13 +116,6 @@ describe('Approvals ProjectRules', () => { ...@@ -114,13 +116,6 @@ describe('Approvals ProjectRules', () => {
rows = wrapper.findAll('tbody tr'); 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', () => { it('should not render the popover for a standard approval group', () => {
const secondRow = rows.at(1); const secondRow = rows.at(1);
const nameCell = findCell(secondRow, 'name'); const nameCell = findCell(secondRow, 'name');
......
...@@ -112,8 +112,8 @@ describe('EE Approvals RuleControls', () => { ...@@ -112,8 +112,8 @@ describe('EE Approvals RuleControls', () => {
expect(findEditButton().exists()).toBe(true); expect(findEditButton().exists()).toBe(true);
}); });
it('does not render remove button', () => { it('does remove button', () => {
expect(findRemoveButton()).toBe(undefined); expect(findRemoveButton().exists()).toBe(true);
}); });
}); });
}); });
...@@ -387,7 +387,7 @@ describe('EE Approvals RuleForm', () => { ...@@ -387,7 +387,7 @@ describe('EE Approvals RuleForm', () => {
it('hides name', () => { it('hides name', () => {
createComponent(); createComponent();
expect(findNameInput().exists()).toBe(false); expect(findNameInput().exists()).toBe(true);
}); });
describe('with no init rule', () => { describe('with no init rule', () => {
......
export const createProjectRules = () => [ export const createProjectRules = () => [
{ id: 1, name: 'Lorem', approvalsRequired: 2, approvers: [{ id: 7 }, { id: 8 }] }, {
{ id: 2, name: 'Ipsum', approvalsRequired: 0, approvers: [{ id: 9 }] }, id: 1,
{ id: 3, name: 'Dolarsit', approvalsRequired: 3, approvers: [] }, 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 = () => ({ export const createMRRule = () => ({
...@@ -10,9 +16,20 @@ export const createMRRule = () => ({ ...@@ -10,9 +16,20 @@ export const createMRRule = () => ({
approvers: [{ id: 1 }, { id: 2 }], approvers: [{ id: 1 }, { id: 2 }],
approvalsRequired: 2, approvalsRequired: 2,
minApprovalsRequired: 0, minApprovalsRequired: 0,
ruleType: 'regular',
});
export const createEmptyRule = () => ({
id: 5,
name: 'All Members',
approvers: [],
approvalsRequired: 3,
minApprovalsRequired: 0,
ruleType: 'any_approver',
}); });
export const createMRRuleWithSource = () => ({ export const createMRRuleWithSource = () => ({
...createEmptyRule(),
...createMRRule(), ...createMRRule(),
minApprovalsRequired: 1, minApprovalsRequired: 1,
hasSource: true, hasSource: true,
......
...@@ -19,7 +19,6 @@ const TEST_HELP_PATH = 'help/path'; ...@@ -19,7 +19,6 @@ const TEST_HELP_PATH = 'help/path';
const TEST_PASSWORD = 'password'; const TEST_PASSWORD = 'password';
const testApprovedBy = () => [1, 7, 10].map(id => ({ id })); const testApprovedBy = () => [1, 7, 10].map(id => ({ id }));
const testApprovals = () => ({ const testApprovals = () => ({
has_approval_rules: true,
approved: false, approved: false,
approved_by: testApprovedBy().map(user => ({ user })), approved_by: testApprovedBy().map(user => ({ user })),
approval_rules_left: [], approval_rules_left: [],
......
# 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
...@@ -104,6 +104,18 @@ describe ApprovalMergeRequestRule do ...@@ -104,6 +104,18 @@ describe ApprovalMergeRequestRule do
end end
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 context 'scopes' do
set(:rb_rule) { create(:code_owner_rule, name: '*.rb') } set(:rb_rule) { create(:code_owner_rule, name: '*.rb') }
set(:js_rule) { create(:code_owner_rule, name: '*.js') } set(:js_rule) { create(:code_owner_rule, name: '*.js') }
...@@ -262,6 +274,8 @@ describe ApprovalMergeRequestRule do ...@@ -262,6 +274,8 @@ describe ApprovalMergeRequestRule do
let!(:approval2) { create(:approval, merge_request: merge_request, user: member2) } let!(:approval2) { create(:approval, merge_request: merge_request, user: member2) }
let!(:approval3) { create(:approval, merge_request: merge_request, user: member3) } let!(:approval3) { create(:approval, merge_request: merge_request, user: member3) }
let(:any_approver_rule) { create(:any_approver_rule, merge_request: merge_request) }
before do before do
subject.users = [member1, member2] subject.users = [member1, member2]
end end
...@@ -269,8 +283,10 @@ describe ApprovalMergeRequestRule do ...@@ -269,8 +283,10 @@ describe ApprovalMergeRequestRule do
context 'when not merged' do context 'when not merged' do
it 'does nothing' do it 'does nothing' do
subject.sync_approved_approvers subject.sync_approved_approvers
any_approver_rule.sync_approved_approvers
expect(subject.approved_approvers.reload).to be_empty expect(subject.approved_approvers.reload).to be_empty
expect(any_approver_rule.approved_approvers).to be_empty
end end
end end
...@@ -282,6 +298,12 @@ describe ApprovalMergeRequestRule do ...@@ -282,6 +298,12 @@ describe ApprovalMergeRequestRule do
expect(subject.approved_approvers.reload).to contain_exactly(member1, member2) expect(subject.approved_approvers.reload).to contain_exactly(member1, member2)
end 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
end end
......
...@@ -18,7 +18,19 @@ describe ApprovalProjectRule do ...@@ -18,7 +18,19 @@ describe ApprovalProjectRule do
end end
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 it 'returns nothing' do
create_list(:approval_project_rule, 2) create_list(:approval_project_rule, 2)
......
This diff is collapsed.
# 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
# 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
...@@ -165,58 +165,10 @@ describe ApprovalWrappedRule do ...@@ -165,58 +165,10 @@ describe ApprovalWrappedRule do
end end
describe '#approvals_required' do 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 it 'returns the attribute saved on the model' do
expect(subject.approvals_required).to eq(19) expect(subject.approvals_required).to eq(19)
end end
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
end
end
end end
...@@ -996,16 +996,17 @@ describe Project do ...@@ -996,16 +996,17 @@ describe Project do
end end
end end
describe '#visible_regular_approval_rules' do describe '#visible_user_defined_rules' do
let(:project) { create(:project) } let(:project) { create(:project) }
let!(:approval_rules) { create_list(:approval_project_rule, 2, project: 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 before do
stub_licensed_features(multiple_approval_rules: true) stub_licensed_features(multiple_approval_rules: true)
end end
it 'returns all approval rules' do 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 end
context 'when multiple approval rules is not available' do context 'when multiple approval rules is not available' do
...@@ -1014,20 +1015,16 @@ describe Project do ...@@ -1014,20 +1015,16 @@ describe Project do
end end
it 'returns the first approval rule' do 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 end
end end
describe '#min_fallback_approvals' do describe '#min_fallback_approvals' do
let(:project) { create(:project, approvals_before_merge: 1) } let(:project) { create(:project) }
it 'returns approvals before merge if there are no rules' do
expect(project.min_fallback_approvals).to eq(1)
end
context 'when approval rules are present' do
before do 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: 2)
create(:approval_project_rule, project: project, approvals_required: 3) create(:approval_project_rule, project: project, approvals_required: 3)
...@@ -1044,7 +1041,6 @@ describe Project do ...@@ -1044,7 +1041,6 @@ describe Project do
expect(project.min_fallback_approvals).to eq(2) expect(project.min_fallback_approvals).to eq(2)
end end
end end
end
describe '#merge_requests_require_code_owner_approval?' do describe '#merge_requests_require_code_owner_approval?' do
let(:project) { build(:project) } let(:project) { build(:project) }
......
...@@ -62,8 +62,8 @@ describe ApprovalRulePresenter do ...@@ -62,8 +62,8 @@ describe ApprovalRulePresenter do
it_behaves_like 'filtering private group' it_behaves_like 'filtering private group'
end end
context 'fallback rule' do context 'any_approver rule' do
let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) } let(:rule) { create(:any_approver_rule) }
it 'contains no groups without raising an error' do it 'contains no groups without raising an error' do
expect(subject.groups).to be_empty expect(subject.groups).to be_empty
...@@ -103,8 +103,8 @@ describe ApprovalRulePresenter do ...@@ -103,8 +103,8 @@ describe ApprovalRulePresenter do
it_behaves_like 'detecting hidden group' it_behaves_like 'detecting hidden group'
end end
context 'fallback rule' do context 'any_approver rule' do
let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) } let(:rule) { create(:any_approver_rule) }
it 'contains no groups without raising an error' do it 'contains no groups without raising an error' do
expect(subject.contains_hidden_groups?).to eq(false) expect(subject.contains_hidden_groups?).to eq(false)
......
...@@ -226,16 +226,6 @@ describe API::MergeRequestApprovals do ...@@ -226,16 +226,6 @@ describe API::MergeRequestApprovals do
expect(response).to have_gitlab_http_status(422) expect(response).to have_gitlab_http_status(422)
end end
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 end
context 'as a project admin' do context 'as a project admin' do
......
...@@ -94,6 +94,37 @@ describe ApprovalRules::CreateService do ...@@ -94,6 +94,37 @@ describe ApprovalRules::CreateService do
specify { expect(result[:rule].rule_type).to eq('report_approver') } specify { expect(result[:rule].rule_type).to eq('report_approver') }
end end
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 end
context 'when target is merge request' do context 'when target is merge request' do
......
...@@ -12,7 +12,6 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -12,7 +12,6 @@ describe ApprovalRules::ParamsFilteringService do
let(:user) { create(:user) } let(:user) { create(:user) }
describe '#execute' do describe '#execute' do
shared_examples_for(:assigning_users_and_groups) do
before do before do
project.add_maintainer(user) project.add_maintainer(user)
project.add_reporter(project_member) project.add_reporter(project_member)
...@@ -27,6 +26,7 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -27,6 +26,7 @@ describe ApprovalRules::ParamsFilteringService do
.and_return(can_update_approvers?) .and_return(can_update_approvers?)
end end
shared_examples_for(:assigning_users_and_groups) do
context 'user can update approvers' do context 'user can update approvers' do
let(:can_update_approvers?) { true } let(:can_update_approvers?) { true }
...@@ -55,7 +55,6 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -55,7 +55,6 @@ describe ApprovalRules::ParamsFilteringService do
end end
context 'create' do context 'create' do
it_behaves_like :assigning_users_and_groups do
let(:merge_request) { build(:merge_request, target_project: project, source_project: project) } let(:merge_request) { build(:merge_request, target_project: project, source_project: project) }
let(:params) do let(:params) do
{ {
...@@ -64,14 +63,36 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -64,14 +63,36 @@ describe ApprovalRules::ParamsFilteringService do
source_branch: 'feature', source_branch: 'feature',
target_branch: 'master', target_branch: 'master',
force_remove_source_branch: '1', force_remove_source_branch: '1',
approval_rules_attributes: [ approval_rules_attributes: approval_rules_attributes
}
end
it_behaves_like :assigning_users_and_groups do
let(:approval_rules_attributes) do
[
{ name: 'foo', user_ids: [project_member.id, outsider.id] }, { name: 'foo', user_ids: [project_member.id, outsider.id] },
{ name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] } { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] }
] ]
}
end end
let(:expected_groups) { [accessible_group] } let(:expected_groups) { [accessible_group] }
end 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 end
context 'update' do context 'update' do
......
...@@ -637,7 +637,6 @@ describe EE::NotificationService, :mailer do ...@@ -637,7 +637,6 @@ describe EE::NotificationService, :mailer do
let!(:rule) { create(:approval_project_rule, project: project, users: project_approvers, approvals_required: 1 )} let!(:rule) { create(:approval_project_rule, project: project, users: project_approvers, approvals_required: 1 )}
before do before do
merge_request.target_project.update(approvals_before_merge: 1)
reset_delivered_emails! reset_delivered_emails!
end end
...@@ -648,7 +647,7 @@ describe EE::NotificationService, :mailer do ...@@ -648,7 +647,7 @@ describe EE::NotificationService, :mailer do
end end
it 'does not email the approvers when approval is not necessary' do 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) notification.new_merge_request(merge_request, @u_disabled)
project_approvers.each { |approver| should_not_email(approver) } project_approvers.each { |approver| should_not_email(approver) }
......
# frozen_string_literal: true # frozen_string_literal: true
module FeatureApprovalHelper module FeatureApprovalHelper
def open_modal def open_modal(text: 'Edit')
page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()" page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()"
within(config_selector) do within(config_selector) do
click_on('Edit') click_on(text)
end end
end end
......
...@@ -1826,9 +1826,15 @@ msgstr "" ...@@ -1826,9 +1826,15 @@ msgstr ""
msgid "Any Milestone" msgid "Any Milestone"
msgstr "" msgstr ""
msgid "Any eligible user"
msgstr ""
msgid "Any encrypted tokens" msgid "Any encrypted tokens"
msgstr "" msgstr ""
msgid "Any member with Developer or higher permissions to the project."
msgstr ""
msgid "Any namespace" msgid "Any namespace"
msgstr "" msgstr ""
...@@ -1936,9 +1942,6 @@ msgid_plural "ApprovalRuleSummary|%{count} approvals required from %{membersCoun ...@@ -1936,9 +1942,6 @@ msgid_plural "ApprovalRuleSummary|%{count} approvals required from %{membersCoun
msgstr[0] "" msgstr[0] ""
msgstr[1] "" msgstr[1] ""
msgid "ApprovalRule|All members with Developer role or higher and code owners (if any)"
msgstr ""
msgid "ApprovalRule|Approvers" msgid "ApprovalRule|Approvers"
msgstr "" msgstr ""
...@@ -4307,6 +4310,9 @@ msgstr "" ...@@ -4307,6 +4310,9 @@ msgstr ""
msgid "Code Owners" msgid "Code Owners"
msgstr "" msgstr ""
msgid "Code Owners to the merge request changes."
msgstr ""
msgid "Code owner approval is required" msgid "Code owner approval is required"
msgstr "" msgstr ""
...@@ -19351,6 +19357,9 @@ msgstr "" ...@@ -19351,6 +19357,9 @@ msgstr ""
msgid "Users" msgid "Users"
msgstr "" msgstr ""
msgid "Users or groups set as approvers in the project's or merge request's settings."
msgstr ""
msgid "Users outside of license" msgid "Users outside of license"
msgstr "" msgstr ""
...@@ -19748,6 +19757,9 @@ msgstr "" ...@@ -19748,6 +19757,9 @@ msgstr ""
msgid "Whitelist to allow requests to the local network from hooks and services" msgid "Whitelist to allow requests to the local network from hooks and services"
msgstr "" msgstr ""
msgid "Who can be an approver?"
msgstr ""
msgid "Who can see this group?" msgid "Who can see this group?"
msgstr "" msgstr ""
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment