Skip to content

G
gitlab-ce
Commit b03dbecc authored by Nick Thomas's avatar Nick Thomas
Browse files
Options

Merge branch '10685-remove-feature-flag' into 'master' 

Remove `approval_rules` feature flag and remove obsolete approvals implementation

Closes #10685

See merge request gitlab-org/gitlab-ee!12436
parents d7d420f1 be74f73c
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 481 additions and 3418 deletions
doc/user/project/merge_requests/merge_request_approvals.md
View file @ b03dbecc
......@@ -3,7 +3,7 @@
> Introduced in [GitLab Enterprise Edition 7.12](https://about.gitlab.com/2015/06/22/gitlab-7-12-released/#merge-request-approvers-ee-only).
NOTE: **Note:**
If you are running a self-managed instance, the new interface shown on
Prior to 12.0, if you are running a self-managed instance, the new interface shown on
this page will not be available unless the feature flag
`approval_rules` is enabled, which can be done from the Rails console by
instance administrators.
......
ee/app/assets/javascripts/approvals/setup_single_rule_approvals.js deleted 100644 → 0
View file @ d7d420f1
import $ from 'jquery';
import _ from 'underscore';
export default () => {
$('.approver-list').on(
'confirm:complete',
'.unsaved-approvers.approver .btn-remove',
function approverListClickCallback(ev, answer) {
if (answer) {
const removeElement = $(this).closest('li');
const approverId = parseInt(removeElement.attr('id').replace('user_', ''), 10);
const approverIds = $('input#merge_request_approver_ids');
const skipUsers = approverIds.data('skipUsers') || [];
const approverIndex = skipUsers.indexOf(approverId);
removeElement.remove();
if (approverIndex > -1) {
approverIds.data('skipUsers', skipUsers.splice(approverIndex, 1));
}
}
return false;
},
);
$('.approver-list').on(
'confirm:complete',
'.unsaved-approvers.approver-group .btn-remove',
function approverListRemoveClickCallback(ev, answer) {
if (answer) {
const removeElement = $(this).closest('li');
const approverGroupId = parseInt(removeElement.attr('id').replace('group_', ''), 10);
const approverGroupIds = $('input#merge_request_approver_group_ids');
const skipGroups = approverGroupIds.data('skipGroups') || [];
const approverGroupIndex = skipGroups.indexOf(approverGroupId);
removeElement.remove();
if (approverGroupIndex > -1) {
approverGroupIds.data('skipGroups', skipGroups.splice(approverGroupIndex, 1));
}
}
return false;
},
);
$('form.merge-request-form').on('submit', function mergeRequestFormSubmitCallback() {
if ($('input#merge_request_approver_ids').length) {
let approverIds = $.map($('li.unsaved-approvers.approver').not('.approver-template'), li =>
li.id.replace('user_', ''),
);
const approversInput = $(this).find('input#merge_request_approver_ids');
approverIds = approverIds.concat(approversInput.val().split(','));
approversInput.val(_.compact(approverIds).join(','));
}
if ($('input#merge_request_approver_group_ids').length) {
let approverGroupIds = $.map($('li.unsaved-approvers.approver-group'), li =>
li.id.replace('group_', ''),
);
const approverGroupsInput = $(this).find('input#merge_request_approver_group_ids');
approverGroupIds = approverGroupIds.concat(approverGroupsInput.val().split(','));
approverGroupsInput.val(_.compact(approverGroupIds).join(','));
}
});
$('.suggested-approvers a').on('click', function sugestedApproversClickCallback() {
const userId = this.id.replace('user_', '');
const username = this.text;
if ($(`.approver-list #user_${userId}`).length) {
return false;
}
const approverItemHTML = $('.unsaved-approvers.approver-template')
.clone()
.removeClass('hide approver-template')[0]
.outerHTML.replace(/\{approver_name\}/g, username)
.replace(/\{user_id\}/g, userId);
$('.no-approvers').remove();
$('.approver-list').append(approverItemHTML);
return false;
});
};
ee/app/assets/javascripts/approvers_select.js deleted 100644 → 0
View file @ d7d420f1
import $ from 'jquery';
import _ from 'underscore';
import Api from 'ee/api';
import { __ } from '~/locale';
import Flash from '~/flash';
import axios from '~/lib/utils/axios_utils';
export default class ApproversSelect {
constructor() {
this.$approverSelect = $('.js-select-user-and-group');
const name = this.$approverSelect.data('name');
this.fieldNames = [`${name}[approver_ids]`, `${name}[approver_group_ids]`];
this.$loadWrapper = $('.load-wrapper');
this.bindEvents();
this.addEvents();
this.initSelect2();
}
bindEvents() {
this.handleSelectChange = this.handleSelectChange.bind(this);
this.fetchGroups = this.fetchGroups.bind(this);
this.fetchUsers = this.fetchUsers.bind(this);
}
addEvents() {
$(document).on('click', '.js-add-approvers', () => this.addApprover());
$(document).on('click', '.js-approver-remove', e => ApproversSelect.removeApprover(e));
}
static getApprovers(fieldName, approverList) {
const input = $(`[name="${fieldName}"]`);
const existingApprovers = $(approverList).map((i, el) => parseInt($(el).data('id'), 10));
const selectedApprovers = input
.val()
.split(',')
.filter(val => val !== '');
return [...existingApprovers, ...selectedApprovers];
}
fetchGroups(term) {
const options = {
skip_groups: ApproversSelect.getApprovers(this.fieldNames[1], '.js-approver-group'),
};
return Api.groups(term, options);
}
fetchUsers(term) {
const options = {
skip_users: ApproversSelect.getApprovers(this.fieldNames[0], '.js-approver'),
project_id: $('#project_id').val(),
};
return Api.approverUsers(term, options);
}
handleSelectChange(e) {
const { added, removed } = e;
const userInput = $(`[name="${this.fieldNames[0]}"]`);
const groupInput = $(`[name="${this.fieldNames[1]}"]`);
if (added) {
if (added.full_name) {
groupInput.val(`${groupInput.val()},${added.id}`.replace(/^,/, ''));
} else {
userInput.val(`${userInput.val()},${added.id}`.replace(/^,/, ''));
}
}
if (removed) {
if (removed.full_name) {
groupInput.val(groupInput.val().replace(new RegExp(`,?${removed.id}`), ''));
} else {
userInput.val(userInput.val().replace(new RegExp(`,?${removed.id}`), ''));
}
}
}
initSelect2() {
import(/* webpackChunkName: 'select2' */ 'select2/select2')
.then(() => {
this.$approverSelect
.select2({
placeholder: __('Search for users or groups'),
multiple: true,
minimumInputLength: 0,
query: query => {
const fetchGroups = this.fetchGroups(query.term);
const fetchUsers = this.fetchUsers(query.term);
return Promise.all([fetchGroups, fetchUsers]).then(([groups, users]) => {
const data = {
results: groups.concat(users),
};
return query.callback(data);
});
},
formatResult: ApproversSelect.formatResult,
formatSelection: ApproversSelect.formatSelection,
dropdownCss() {
const $input = $('.js-select-user-and-group .select2-input');
const offset = $input.offset();
let { left } = offset;
const inputRightPosition = left + $input.outerWidth();
const $dropdown = $('.select2-drop-active');
if ($dropdown.outerWidth() > $input.outerWidth()) {
left = `${inputRightPosition - $dropdown.width()}px`;
}
return {
left,
right: 'auto',
width: 'auto',
};
},
})
.on('change', this.handleSelectChange);
})
.catch(() => {});
}
static formatSelection(group) {
return _.escape(group.full_name || group.name);
}
static formatResult({
name,
username,
avatar_url: avatarUrl,
full_name: fullName,
full_path: fullPath,
}) {
if (username) {
const avatar = avatarUrl || gon.default_avatar_url;
return `
<div class="user-result">
<div class="user-image">
<img class="avatar s40" src="${avatar}">
</div>
<div class="user-info">
<div class="user-name">${_.escape(name)}</div>
<div class="user-username">@${_.escape(username)}</div>
</div>
</div>
`;
}
return `
<div class="group-result">
<div class="group-name">${_.escape(fullName)}</div>
<div class="group-path">${_.escape(fullPath)}</div>
</div>
`;
}
addApprover() {
this.fieldNames.forEach(ApproversSelect.saveApprovers);
}
static saveApprovers(fieldName) {
const $input = $(`[name="${fieldName}"]`);
const newValue = $input.val();
const $loadWrapper = $('.load-wrapper');
const $approverSelect = $('.js-select-user-and-group');
if (!newValue) {
return;
}
const $form = $('.js-add-approvers').closest('form');
$loadWrapper.removeClass('hidden');
axios
.post($form.attr('action'), `_method=PATCH&${[encodeURIComponent(fieldName)]}=${newValue}`, {
headers: {
'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8',
},
})
.then(({ data }) => {
ApproversSelect.updateApproverList(data);
ApproversSelect.saveApproversComplete($input, $approverSelect, $loadWrapper);
})
.catch(() => {
Flash(__('An error occurred while adding approver'));
ApproversSelect.saveApproversComplete($input, $approverSelect, $loadWrapper);
});
}
static saveApproversComplete($input, $approverSelect, $loadWrapper) {
$input.val('');
$approverSelect.select2('val', '').trigger('change');
$loadWrapper.addClass('hidden');
}
static removeApprover(e) {
e.preventDefault();
const target = e.currentTarget;
const $loadWrapper = $('.load-wrapper');
$loadWrapper.removeClass('hidden');
axios
.post(target.getAttribute('href'), '_method=DELETE', {
headers: {
'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8',
},
})
.then(({ data }) => {
ApproversSelect.updateApproverList(data);
$loadWrapper.addClass('hidden');
})
.catch(() => {
Flash(__('An error occurred while removing approver'));
$loadWrapper.addClass('hidden');
});
}
static updateApproverList(html) {
$('.js-current-approvers').html(
$(html)
.find('.js-current-approvers')
.html(),
);
}
}
ee/app/assets/javascripts/pages/projects/edit/index.js
View file @ b03dbecc
......@@ -4,7 +4,6 @@ import '~/pages/projects/edit';
import UsersSelect from '~/users_select';
import UserCallout from '~/user_callout';
import groupsSelect from '~/groups_select';
import ApproversSelect from 'ee/approvers_select';
import mountApprovals from 'ee/approvals/mount_project_settings';
import initServiceDesk from 'ee/projects/settings_service_desk';
......@@ -14,7 +13,6 @@ document.addEventListener('DOMContentLoaded', () => {
new UserCallout({ className: 'js-service-desk-callout' });
new UserCallout({ className: 'js-mr-approval-callout' });
new ApproversSelect();
initServiceDesk();
mountApprovals(document.getElementById('js-mr-approvals-settings'));
});
ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js
View file @ b03dbecc
import initApprovals from 'ee/approvals/setup_single_rule_approvals';
import mountApprovals from 'ee/approvals/mount_mr_edit';
export default () => {
initApprovals();
mountApprovals(document.getElementById('js-mr-approvals-input'));
};
ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue → ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
View file @ b03dbecc
......@@ -10,7 +10,7 @@ import ApprovalsSummary from './approvals_summary.vue';
import ApprovalsSummaryOptional from './approvals_summary_optional.vue';
import ApprovalsFooter from './approvals_footer.vue';
import ApprovalsAuth from './approvals_auth.vue';
import { FETCH_LOADING, FETCH_ERROR, APPROVE_ERROR, UNAPPROVE_ERROR } from '../messages';
import { FETCH_LOADING, FETCH_ERROR, APPROVE_ERROR, UNAPPROVE_ERROR } from './messages';
export default {
name: 'MRWidgetMultipleRuleApprovals',
......
ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary.vue → ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_summary.vue
View file @ b03dbecc
......@@ -2,7 +2,7 @@
import { n__, sprintf } from '~/locale';
import { toNounSeriesText } from '~/lib/utils/grammar';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import { APPROVED_MESSAGE } from '../messages';
import { APPROVED_MESSAGE } from './messages';
export default {
components: {
......
ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue → ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_summary_optional.vue
View file @ b03dbecc
<script>
import { GlTooltipDirective, GlLink } from '@gitlab/ui';
import Icon from '~/vue_shared/components/icon.vue';
import { OPTIONAL, OPTIONAL_CAN_APPROVE } from '../messages';
import { OPTIONAL, OPTIONAL_CAN_APPROVE } from './messages';
export default {
components: {
......
ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/index.js deleted 100644 → 0
View file @ d7d420f1
import MultipleRuleApprovals from './multiple_rule/approvals.vue';
import SingleRuleApprovals from './single_rule/approvals.vue';
export default {
functional: true,
render(h, context) {
const component = gon.features.approvalRules ? MultipleRuleApprovals : SingleRuleApprovals;
return h(component, context.data, context.children);
},
};
ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals.vue deleted 100644 → 0
View file @ d7d420f1
<script>
/* === WARNING ===
* This file will be removed pending the removal of the `approval_rules` feature flag.
*
* If a new feature needs to be added, please please make changes in the `./multiple_rule`
* directory (see the feature issue https://gitlab.com/gitlab-org/gitlab-ee/issues/1979).
*
* Follow along via this issue: https://gitlab.com/gitlab-org/gitlab-ee/issues/10685.
*/
import createFlash from '~/flash';
import MrWidgetContainer from '~/vue_merge_request_widget/components/mr_widget_container.vue';
import MrWidgetIcon from '~/vue_merge_request_widget/components/mr_widget_icon.vue';
import ApprovalsBody from './approvals_body.vue';
import ApprovalsFooter from './approvals_footer.vue';
import { FETCH_LOADING, FETCH_ERROR } from '../messages';
export default {
name: 'MRWidgetSingleRuleApprovals',
components: {
ApprovalsBody,
ApprovalsFooter,
MrWidgetContainer,
MrWidgetIcon,
},
props: {
mr: {
type: Object,
required: true,
},
service: {
type: Object,
required: true,
},
},
data() {
return {
fetchingApprovals: true,
};
},
computed: {
status() {
if (this.mr.approvals.approvals_left > 0) {
return 'warning';
}
return 'success';
},
approvalsOptional() {
return (
!this.fetchingApprovals &&
this.mr.approvals.approvals_required === 0 &&
this.mr.approvals.approved_by.length === 0
);
},
},
created() {
this.service
.fetchApprovals()
.then(data => {
this.mr.setApprovals(data);
this.fetchingApprovals = false;
})
.catch(() => createFlash(FETCH_ERROR));
},
FETCH_LOADING,
};
</script>
<template>
<mr-widget-container>
<div
v-if="mr.hasApprovalsAvailable"
class="media media-section js-mr-approvals align-items-center"
>
<mr-widget-icon name="approval" />
<div v-show="fetchingApprovals" class="mr-approvals-loading-state media-body">
<span class="approvals-loading-text"> {{ $options.FETCH_LOADING }} </span>
</div>
<div v-if="!fetchingApprovals" class="approvals-components media-body">
<approvals-body
:mr="mr"
:service="service"
:user-can-approve="mr.approvals.user_can_approve"
:user-has-approved="mr.approvals.user_has_approved"
:approved-by="mr.approvals.approved_by"
:approvals-left="mr.approvals.approvals_left"
:approvals-optional="approvalsOptional"
:suggested-approvers="mr.approvals.suggested_approvers"
/>
<approvals-footer
:mr="mr"
:service="service"
:user-can-approve="mr.approvals.user_can_approve"
:user-has-approved="mr.approvals.user_has_approved"
:approved-by="mr.approvals.approved_by"
:approvals-left="mr.approvals.approvals_left"
/>
</div>
</div>
</mr-widget-container>
</template>
ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals_body.vue deleted 100644 → 0
View file @ d7d420f1
<script>
/* === WARNING ===
* This file will be removed pending the removal of the `approval_rules` feature flag.
*
* If a new feature needs to be added, please please make changes in the `./multiple_rule`
* directory (see the feature issue https://gitlab.com/gitlab-org/gitlab-ee/issues/1979).
*
* Follow along via this issue: https://gitlab.com/gitlab-org/gitlab-ee/issues/10685.
*/
import { n__, s__, sprintf } from '~/locale';
import Flash from '~/flash';
import Icon from '~/vue_shared/components/icon.vue';
import MrWidgetAuthor from '~/vue_merge_request_widget/components/mr_widget_author.vue';
import tooltip from '~/vue_shared/directives/tooltip';
import eventHub from '~/vue_merge_request_widget/event_hub';
import { APPROVE_ERROR, OPTIONAL_CAN_APPROVE, OPTIONAL } from '../messages';
export default {
name: 'ApprovalsBody',
components: {
MrWidgetAuthor,
Icon,
},
directives: {
tooltip,
},
props: {
mr: {
type: Object,
required: true,
},
service: {
type: Object,
required: true,
},
approvedBy: {
type: Array,
required: false,
default: () => [],
},
approvalsOptional: {
type: Boolean,
required: false,
default: false,
},
approvalsLeft: {
type: Number,
required: false,
default: 0,
},
userCanApprove: {
type: Boolean,
required: false,
default: false,
},
userHasApproved: {
type: Boolean,
required: false,
default: false,
},
suggestedApprovers: {
type: Array,
required: false,
default: () => [],
},
},
data() {
return {
approving: false,
};
},
computed: {
approvalsRequiredStringified() {
if (this.approvalsOptional) {
if (this.userCanApprove) {
return OPTIONAL_CAN_APPROVE;
}
return OPTIONAL;
}
if (this.approvalsLeft === 0) {
return this.userCanApprove
? s__('mrWidget|Merge request approved; you can approve additionally')
: s__('mrWidget|Merge request approved');
}
if (this.suggestedApprovers.length >= 1) {
return sprintf(
n__(
'mrWidget|Requires 1 more approval by',
'mrWidget|Requires %d more approvals by',
this.approvalsLeft,
),
);
}
return sprintf(
n__(
'mrWidget|Requires 1 more approval',
'mrWidget|Requires %d more approvals',
this.approvalsLeft,
),
);
},
approveButtonText() {
let approveButtonText = s__('mrWidget|Approve');
if (this.approvalsLeft <= 0) {
approveButtonText = s__('mrWidget|Add approval');
}
return approveButtonText;
},
approveButtonClass() {
return {
'btn-inverted': this.showApproveButton && this.approvalsLeft <= 0,
};
},
showApprovalDocLink() {
return this.approvalsOptional && this.showApproveButton;
},
showApproveButton() {
return this.userCanApprove && !this.userHasApproved && this.mr.isOpen;
},
showSuggestedApprovers() {
return this.approvalsLeft > 0 && this.suggestedApprovers && this.suggestedApprovers.length;
},
},
methods: {
approveMergeRequest() {
this.approving = true;
this.service
.approveMergeRequest()
.then(data => {
this.mr.setApprovals(data);
eventHub.$emit('MRWidgetUpdateRequested');
this.approving = false;
})
.catch(() => {
this.approving = false;
Flash(APPROVE_ERROR);
});
},
},
};
</script>
<template>
<div class="approvals-body space-children">
<span v-if="showApproveButton" class="approvals-approve-button-wrap">
<button
:disabled="approving"
:class="approveButtonClass"
class="btn btn-primary btn-sm approve-btn"
@click="approveMergeRequest"
>
<i v-if="approving" class="fa fa-spinner fa-spin" aria-hidden="true"></i>
{{ approveButtonText }}
</button>
</span>
<span :class="approvalsOptional ? 'text-muted' : 'bold'" class="approvals-required-text">
{{ approvalsRequiredStringified }}
<a
v-if="showApprovalDocLink"
v-tooltip
:href="mr.approvalsHelpPath"
:title="__('About this feature')"
data-placement="bottom"
target="_blank"
rel="noopener noreferrer nofollow"
data-container="body"
>
<icon name="question-o" />
</a>
<span v-if="showSuggestedApprovers">
<mr-widget-author
v-for="approver in suggestedApprovers"
:key="approver.username"
:author="approver"
:show-author-name="false"
:show-author-tooltip="true"
/>
</span>
</span>
</div>
</template>
ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals_footer.vue deleted 100644 → 0
View file @ d7d420f1
<script>
/* === WARNING ===
* This file will be removed pending the removal of the `approval_rules` feature flag.
*
* If a new feature needs to be added, please please make changes in the `./multiple_rule`
* directory (see the feature issue https://gitlab.com/gitlab-org/gitlab-ee/issues/1979).
*
* Follow along via this issue: https://gitlab.com/gitlab-org/gitlab-ee/issues/10685.
*/
import Flash from '~/flash';
import Icon from '~/vue_shared/components/icon.vue';
import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link.vue';
import { s__ } from '~/locale';
import eventHub from '~/vue_merge_request_widget/event_hub';
import { UNAPPROVE_ERROR } from '../messages';
export default {
name: 'ApprovalsFooter',
components: {
Icon,
UserAvatarLink,
},
props: {
mr: {
type: Object,
required: true,
},
service: {
type: Object,
required: true,
},
approvedBy: {
type: Array,
required: false,
default: () => [],
},
approvalsLeft: {
type: Number,
required: false,
default: 0,
},
userCanApprove: {
type: Boolean,
required: false,
default: false,
},
userHasApproved: {
type: Boolean,
required: false,
default: false,
},
suggestedApprovers: {
type: Array,
required: false,
default: () => [],
},
},
data() {
return {
unapproving: false,
};
},
computed: {
showUnapproveButton() {
const isMerged = this.mr.state === 'merged';
return this.userHasApproved && !this.userCanApprove && !isMerged;
},
approvedByText() {
return s__('mrWidget|Approved by');
},
removeApprovalText() {
return s__('mrWidget|Remove your approval');
},
},
methods: {
unapproveMergeRequest() {
this.unapproving = true;
this.service
.unapproveMergeRequest()
.then(data => {
this.mr.setApprovals(data);
eventHub.$emit('MRWidgetUpdateRequested');
this.unapproving = false;
})
.catch(() => {
this.unapproving = false;
Flash(UNAPPROVE_ERROR);
});
},
},
};
</script>
<template>
<div v-if="approvedBy.length" class="approved-by-users approvals-footer clearfix mr-info-list">
<div class="approvers-prefix">
<p>{{ approvedByText }}</p>
<div class="approvers-list">
<user-avatar-link
v-for="approver in approvedBy"
:key="approver.user.username"
class="js-approver-list-member"
:img-size="20"
:img-src="approver.user.avatar_url"
:img-alt="approver.user.name"
:link-href="approver.user.web_url"
:tooltip-text="approver.user.name"
tooltip-placement="bottom"
/>
<icon
v-for="n in approvalsLeft"
:key="n"
name="dotted-circle"
class="avatar avatar-placeholder s20"
/>
</div>
<button
v-if="showUnapproveButton"
:disabled="unapproving"
type="button"
class="btn btn-sm unapprove-btn-wrap"
@click="unapproveMergeRequest"
>
<i v-if="unapproving" class="fa fa-spinner fa-spin" aria-hidden="true"> </i>
{{ removeApprovalText }}
</button>
</div>
</div>
</template>
ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue
View file @ b03dbecc
......@@ -8,7 +8,7 @@ import MrWidgetLicenses from 'ee/vue_shared/license_management/mr_widget_license
import { n__, s__, __, sprintf } from '~/locale';
import CEWidgetOptions from '~/vue_merge_request_widget/mr_widget_options.vue';
import MrWidgetApprovals from './components/approvals';
import MrWidgetApprovals from './components/approvals/approvals.vue';
import MrWidgetGeoSecondaryNode from './components/states/mr_widget_secondary_geo_node.vue';
export default {
......@@ -145,7 +145,6 @@ export default {
return {
...base,
approvalsPath: store.approvalsPath,
apiApprovalsPath: store.apiApprovalsPath,
apiApprovalSettingsPath: store.apiApprovalSettingsPath,
apiApprovePath: store.apiApprovePath,
......
ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js
View file @ b03dbecc
......@@ -5,38 +5,31 @@ export default class MRWidgetService extends CEWidgetService {
constructor(mr) {
super(mr);
this.approvalsPath = mr.approvalsPath;
// This feature flag will be the default behavior when
// https://gitlab.com/gitlab-org/gitlab-ee/issues/1979 is closed
if (gon.features.approvalRules) {
this.apiApprovalsPath = mr.apiApprovalsPath;
this.apiApprovalSettingsPath = mr.apiApprovalSettingsPath;
this.apiApprovePath = mr.apiApprovePath;
this.apiUnapprovePath = mr.apiUnapprovePath;
this.fetchApprovals = () => axios.get(this.apiApprovalsPath).then(res => res.data);
this.fetchApprovalSettings = () =>
axios.get(this.apiApprovalSettingsPath).then(res => res.data);
this.approveMergeRequest = () => axios.post(this.apiApprovePath).then(res => res.data);
this.approveMergeRequestWithAuth = approvalPassword =>
axios
.post(this.apiApprovePath, { approval_password: approvalPassword })
.then(res => res.data);
this.unapproveMergeRequest = () => axios.post(this.apiUnapprovePath).then(res => res.data);
}
this.apiApprovalsPath = mr.apiApprovalsPath;
this.apiApprovalSettingsPath = mr.apiApprovalSettingsPath;
this.apiApprovePath = mr.apiApprovePath;
this.apiUnapprovePath = mr.apiUnapprovePath;
}
fetchApprovals() {
return axios.get(this.approvalsPath).then(res => res.data);
return axios.get(this.apiApprovalsPath).then(res => res.data);
}
fetchApprovalSettings() {
return axios.get(this.apiApprovalSettingsPath).then(res => res.data);
}
approveMergeRequest() {
return axios.post(this.approvalsPath).then(res => res.data);
return axios.post(this.apiApprovePath).then(res => res.data);
}
approveMergeRequestWithAuth(approvalPassword) {
return axios
.post(this.apiApprovePath, { approval_password: approvalPassword })
.then(res => res.data);
}
unapproveMergeRequest() {
return axios.delete(this.approvalsPath).then(res => res.data);
return axios.post(this.apiUnapprovePath).then(res => res.data);
}
// eslint-disable-next-line class-methods-use-this
......
ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
View file @ b03dbecc
......@@ -59,7 +59,6 @@ export default class MergeRequestStore extends CEMergeRequestStore {
this.hasApprovalsAvailable = Boolean(
data.has_approvals_available || this.hasApprovalsAvailable,
);
this.approvalsPath = data.approvals_path || this.approvalsPath;
this.apiApprovalsPath = data.api_approvals_path || this.apiApprovalsPath;
this.apiApprovalSettingsPath = data.api_approval_settings_path || this.apiApprovalSettingsPath;
this.apiApprovePath = data.api_approve_path || this.apiApprovePath;
......@@ -69,13 +68,8 @@ export default class MergeRequestStore extends CEMergeRequestStore {
setApprovals(data) {
this.approvals = mapApprovalsResponse(data);
this.approvalsLeft = !!data.approvals_left;
if (gon.features.approvalRules) {
this.isApproved = data.approved || false;
this.preventMerge = !this.isApproved;
} else {
this.isApproved = !this.approvalsLeft || false;
this.preventMerge = this.hasApprovalsAvailable && this.approvalsLeft;
}
this.isApproved = data.approved || false;
this.preventMerge = !this.isApproved;
}
setApprovalRules(data) {
......
ee/app/controllers/ee/projects/merge_requests_controller.rb
View file @ b03dbecc
......@@ -9,7 +9,6 @@ module EE
prepended do
before_action only: [:show] do
push_frontend_feature_flag(:approval_rules, merge_request.project, default_enabled: true)
push_frontend_feature_flag(:visual_review_app, merge_request.project, default_enabled: false)
end
......@@ -70,13 +69,10 @@ module EE
def render_approvals_json
respond_to do |format|
format.json do
entity = if ::Feature.enabled?(:approval_rules, merge_request.project, default_enabled: true)
EE::API::Entities::ApprovalState.new(merge_request.approval_state, current_user: current_user)
else
EE::API::Entities::MergeRequestApprovals.new(merge_request, current_user: current_user)
end
render json: entity
render json: EE::API::Entities::ApprovalState.new(
merge_request.approval_state,
current_user: current_user
)
end
end
end
......
ee/app/models/concerns/approvable.rb
View file @ b03dbecc
......@@ -7,6 +7,20 @@ module Approvable
# such as approver_groups and target_project in presenters
include ::VisibleApprovable
FORWARDABLE_METHODS = %i{
approval_needed?
approved?
approvals_left
can_approve?
has_approved?
any_approver_allowed?
authors_can_approve?
committers_can_approve?
approvers_overwritten?
}.freeze
delegate(*FORWARDABLE_METHODS, to: :approval_state)
def approval_feature_available?
strong_memoize(:approval_feature_available) do
if project
......@@ -21,30 +35,10 @@ module Approvable
@approval_state ||= ApprovalState.new(self)
end
def approval_needed?
approvals_required&.nonzero?
end
def approved?
approvals_left < 1
end
def approvals_given
approvals.size
end
# Number of approvals remaining (excluding existing approvals) before the MR is
# considered approved. If there are fewer potential approvers than approvals left,
# users should either reduce the number of approvers on projects and/or merge
# requests settings and/or allow MR authors to approve their own merge
# requests (in case only one approval is needed).
#
def approvals_left
approvals_left_count = approvals_required - approvals.size
[approvals_left_count, 0].max
end
def approvals_required
[approvals_before_merge.to_i, target_project.approvals_before_merge.to_i].max
end
......@@ -55,58 +49,6 @@ module Approvable
super
end
# Even though this method is used in VisibleApprovable
# we do not want to encounter a scenario where there are
# no user approvers but there is one merge request group
# approver that is not visible to the current_user,
# which would make this method return false
# when it should still report as overwritten.
# To prevent this from happening, approvers_overwritten?
# makes use of the unfiltered version of approver_groups so that
# it always takes into account every approver
# available
#
def approvers_overwritten?
approvers.to_a.any? || approver_groups.to_a.any?
end
def can_approve?(user)
return false unless user
# The check below considers authors and committers being able to approve the MR.
# That is, they're included/excluded from that list accordingly.
return true if approvers_left.include?(user)
# We can safely unauthorize authors and committers if it reaches this guard clause.
return false if author == user
return false if committers.include?(user)
return false unless user.can?(:update_merge_request, self)
any_approver_allowed? && approvals.where(user: user).empty?
end
def has_approved?(user)
return false unless user
approved_by_users.include?(user)
end
# Once there are fewer approvers left in the list than approvals required or
# there are no more approvals required
# allow other project members to approve the MR.
#
def any_approver_allowed?
remaining_approvals = approvals_left
remaining_approvals.zero? || remaining_approvals > approvers_left.count
end
def authors_can_approve?
target_project.merge_requests_author_approval?
end
def committers_can_approve?
!target_project.merge_requests_disable_committers_approval?
end
def approver_ids=(value)
::Gitlab::Utils.ensure_array_from_string(value).each do |user_id|
next if author && user_id == author.id
......
ee/app/models/concerns/approvable_for_rule.rb deleted 100644 → 0
View file @ d7d420f1
# frozen_string_literal: true
module ApprovableForRule
include VisibleApprovableForRule
FORWARDABLE_METHODS = %w{
approval_needed?
approved?
approvals_left
can_approve?
has_approved?
any_approver_allowed?
authors_can_approve?
committers_can_approve?
approvers_overwritten?
}.freeze
FORWARDABLE_METHODS.each do |method|
define_method(method) do |*args|
return super(*args) if approval_rules_disabled?
approval_state.public_send(method, *args) # rubocop:disable GitlabSecurity/PublicSend
end
end
end
ee/app/models/concerns/visible_approvable.rb
View file @ b03dbecc
......@@ -8,9 +8,7 @@ module VisibleApprovable
# Users in the list of approvers who have not already approved this MR.
#
def approvers_left
strong_memoize(:approvers_left) do
User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approved_by_users.select(:id))
end
approval_state.unactioned_approvers
end
# The list of approvers from either this MR (if they've been set on the MR) or the
......@@ -22,58 +20,21 @@ module VisibleApprovable
#
# @return [Array<User>]
def overall_approvers(exclude_code_owners: false)
code_owners = [] if exclude_code_owners
options = { target: :users }
options[:code_owner] = false if exclude_code_owners
if approvers_overwritten?
code_owners ||= [] # already persisted into database, no need to recompute
approvers_relation = approver_users
else
code_owners ||= self.code_owners.dup
approvers_relation = target_project.approver_users
end
approvers_relation = project.members_among(approvers_relation)
if author && !authors_can_approve?
approvers_relation = approvers_relation.where.not(id: author.id)
end
if committers.any? && !committers_can_approve?
approvers_relation = approvers_relation.where.not(id: committers.select(:id))
end
results = code_owners.concat(approvers_relation)
results.uniq!
results
end
def overall_approver_groups
approvers_overwritten? ? approver_groups : target_project.approver_groups
approvers = approval_state.filtered_approvers(options)
approvers.uniq!
approvers
end
def all_approvers_including_groups
strong_memoize(:all_approvers_including_groups) do
approvers = []
# Approvers not sourced from group level
approvers << overall_approvers
approvers << approvers_from_groups
approvers.flatten
end
approval_state.approvers
end
def approvers_from_groups
group_approvers = overall_approver_groups.flat_map(&:users)
group_approvers.delete(author) unless authors_can_approve?
unless committers_can_approve?
committer_approver_ids = committers.where(id: group_approvers.map(&:id)).pluck(:id)
group_approvers.reject! { |user| committer_approver_ids.include?(user.id) }
end
group_approvers
groups = approval_state.wrapped_approval_rules.flat_map(&:groups)
User.joins(:group_members).where(members: { source_id: groups })
end
def reset_approval_cache!
......
ee/app/models/concerns/visible_approvable_for_rule.rb deleted 100644 → 0
View file @ d7d420f1
# frozen_string_literal: true
# This module may only be used by presenters or modules
# that include the Approvable concern
module VisibleApprovableForRule
def approvers_left
return super if approval_rules_disabled?
approval_state.unactioned_approvers
end
def overall_approvers(exclude_code_owners: false)
return super if approval_rules_disabled?
options = { target: :users }
options[:code_owner] = false if exclude_code_owners
approvers = approval_state.filtered_approvers(options)
approvers.uniq!
approvers
end
def all_approvers_including_groups
return super if approval_rules_disabled?
approval_state.approvers
end
def approvers_from_groups
return super if approval_rules_disabled?
groups = approval_state.wrapped_approval_rules.flat_map(&:groups)
User.joins(:group_members).where(members: { source_id: groups })
end
def approval_rules_disabled?
::Feature.disabled?(:approval_rules, project, default_enabled: true)
end
end
ee/app/models/ee/merge_request.rb
View file @ b03dbecc
......@@ -8,7 +8,6 @@ module EE
include ::Approvable
include ::Gitlab::Utils::StrongMemoize
include FromUnion
prepend ApprovableForRule
prepended do
include Elastic::MergeRequestsSearch
......@@ -107,7 +106,6 @@ module EE
end
def validate_approval_rule_source
return if ::Feature.disabled?(:approval_rules, project, default_enabled: true)
return unless approval_rules.any?
local_project_rule_ids = approval_rules.map { |rule| rule.approval_merge_request_rule_source&.approval_project_rule_id }
......@@ -126,16 +124,7 @@ module EE
strong_memoize(:participant_approvers) do
next [] unless approval_needed?
if ::Feature.enabled?(:approval_rules, project, default_enabled: true)
approval_state.filtered_approvers(code_owner: false, unactioned: true)
else
approvers = [
*overall_approvers(exclude_code_owners: true),
*approvers_from_groups
]
::User.where(id: approvers.map(&:id)).where.not(id: approved_by_users.select(:id))
end
approval_state.filtered_approvers(code_owner: false, unactioned: true)
end
end
......
ee/app/models/ee/project.rb
View file @ b03dbecc
......@@ -324,8 +324,6 @@ module EE
end
def visible_regular_approval_rules
return approval_rules.none unless ::Feature.enabled?(:approval_rules, self, default_enabled: true)
strong_memoize(:visible_regular_approval_rules) do
regular_rules = approval_rules.regular.order(:id)
......
ee/app/presenters/ee/merge_request_presenter.rb
View file @ b03dbecc
......@@ -3,7 +3,6 @@
module EE
module MergeRequestPresenter
include ::VisibleApprovable
prepend VisibleApprovableForRule
def approvals_path
if expose_mr_approval_path?
......
ee/app/services/ee/merge_requests/refresh_service.rb
View file @ b03dbecc
......@@ -40,29 +40,10 @@ module EE
def update_approvers
return yield unless project.feature_available?(:code_owners)
if ::Feature.enabled?(:approval_rules, project, default_enabled: true)
results = yield
results = yield
merge_requests_for_source_branch.each do |merge_request|
::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
end
else
previous_diffs = fetch_latest_merge_request_diffs
results = yield
merge_requests = merge_requests_for_source_branch
ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord)
merge_requests.each do |merge_request|
previous_diff = previous_diffs.find { |diff| diff.merge_request == merge_request }
previous_code_owners = ::Gitlab::CodeOwners.for_merge_request(merge_request, merge_request_diff: previous_diff)
new_code_owners = merge_request.code_owners - previous_code_owners
create_approvers(merge_request, new_code_owners)
merge_request.sync_code_owners_with_approvers
end
merge_requests_for_source_branch.each do |merge_request|
::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
end
results
......
ee/app/services/ee/merge_requests/update_service.rb
View file @ b03dbecc
......@@ -15,7 +15,6 @@ module EE
end
merge_request = super(merge_request)
sync_approval_rules(merge_request)
if should_remove_old_approvers && merge_request.valid?
cleanup_approvers(merge_request, reload: true)
......@@ -49,16 +48,6 @@ module EE
merge_request.approvals.delete_all if target_project.reset_approvals_on_push
end
# TODO remove after #1979 is closed
def sync_approval_rules(merge_request)
return if ::Feature.enabled?(:approval_rules, merge_request.target_project, default_enabled: true)
return if merge_request.merged?
return unless merge_request.previous_changes.include?(:approvals_before_merge)
approvals_required = merge_request.approvals_before_merge || merge_request.target_project.approvals_before_merge
merge_request.approval_rules.regular.update_all(approvals_required: approvals_required)
end
end
end
end
ee/app/services/ee/projects/update_service.rb
View file @ b03dbecc
......@@ -35,8 +35,6 @@ module EE
sync_wiki_on_enable if !wiki_was_enabled && project.wiki_enabled?
project.import_state.force_import_job! if params[:mirror].present? && project.mirror?
project.remove_import_data if project.previous_changes.include?('mirror') && !project.mirror?
sync_approval_rules
end
result
......@@ -67,14 +65,6 @@ module EE
def sync_wiki_on_enable
::Geo::RepositoryUpdatedService.new(project.wiki.repository).execute
end
# TODO remove after #1979 is closed
def sync_approval_rules
return if ::Feature.enabled?(:approval_rules, project, default_enabled: true)
return unless project.previous_changes.include?(:approvals_before_merge)
project.approval_rules.update_all(approvals_required: project.approvals_before_merge)
end
end
end
end
ee/app/views/projects/_merge_request_approvals_settings_form.html.haml
View file @ b03dbecc
- can_override_approvers = project.can_override_approvers?
- if Feature.enabled?(:approval_rules, project, default_enabled: true)
= render 'shared/merge_request_approvals_settings/multiple_rules_form', form: form, project: project
- else
= render 'shared/merge_request_approvals_settings/single_rule_form', form: form, project: project
.form-group
= form.label :approver_ids, class: 'label-bold' do
= _("Add approvers")
#js-mr-approvals-settings{ data: { 'project_id': @project.id,
'project_path': expose_path(api_v4_projects_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)),
'allow_multi_rule': @project.multiple_approval_rules_available?.to_s } }
.text-center.prepend-top-default
= sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
- if project.code_owner_approval_required_available?
.form-group.require-code-owner-approval
......@@ -42,7 +48,7 @@
= link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals',
anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'
- if Feature.enabled?(:approval_rules, project) && password_authentication_enabled_for_web?
- if password_authentication_enabled_for_web?
.form-group.self-approval
.form-check
= form.check_box :require_password_to_approve, class: 'form-check-input'
......
ee/app/views/shared/issuable/_approvals.html.haml
View file @ b03dbecc
......@@ -11,10 +11,13 @@
= form.label :approver_ids, class: 'col-form-label col-sm-2' do
Approvers
.col-sm-10
- if Feature.enabled?(:approval_rules, @target_project, default_enabled: true)
= render 'shared/issuable/approvals_multiple_rule', issuable: issuable, presenter: presenter
- else
= render 'shared/issuable/approvals_single_rule', issuable: issuable, presenter: presenter, form: form
#js-mr-approvals-input{ data: { 'project_id': @target_project.id,
'can_edit': can?(current_user, :update_approvers, issuable).to_s,
'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s,
'mr_id': issuable.iid,
'mr_settings_path': presenter.api_approval_settings_path,
'project_settings_path': presenter.api_project_approval_settings_path } }
= sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
- if can_update_approvers
- approver_presenter = MergeRequestApproverPresenter.new(issuable, skip_user: current_user)
.form-text.text-muted.suggested-approvers
......
ee/app/views/shared/issuable/_approvals_multiple_rule.html.haml deleted 100644 → 0
View file @ d7d420f1
#js-mr-approvals-input{ data: { 'project_id': @target_project.id,
'can_edit': can?(current_user, :update_approvers, issuable).to_s,
'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s,
'mr_id': issuable.iid,
'mr_settings_path': presenter.api_approval_settings_path,
'project_settings_path': presenter.api_project_approval_settings_path } }
= sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
ee/app/views/shared/issuable/_approvals_single_rule.html.haml deleted 100644 → 0
View file @ d7d420f1
- issuable = local_assigns.fetch(:issuable)
- presenter = local_assigns.fetch(:presenter)
- form = local_assigns.fetch(:form)
- can_update_approvers = can?(current_user, :update_approvers, issuable)
- if can_update_approvers
- skip_users = [*presenter.all_approvers_including_groups, (issuable.author unless presenter.authors_can_approve?), *(issuable.committers unless presenter.committers_can_approve?)].compact
= users_select_tag("merge_request[approver_ids]",
multiple: true,
class: 'input-large',
email_user: true,
skip_users: skip_users,
project: issuable.target_project)
.form-text.text-muted
= _('This merge request must be approved by these users. You can override the project settings by setting your own list of approvers.')
- skip_groups = presenter.overall_approver_groups.pluck(:group_id) # rubocop: disable CodeReuse/ActiveRecord
= groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true, project: issuable.target_project }, class: 'input-large')
.form-text.text-muted
= _('This merge request must be approved by members of these groups. You can override the project settings by setting your own list of approvers.')
.card.prepend-top-10
.card-header
= _('Approvers')
%ul.content-list.approver-list.qa-approver-list
- if presenter.all_approvers_including_groups.empty?
%li.no-approvers= _('There are no approvers')
- else
- unsaved_approvers = !presenter.approvers_overwritten?
- item_classes = unsaved_approvers ? ['unsaved-approvers'] : []
- presenter.overall_approvers.each do |approver|
%li{ id: dom_id(approver), class: item_classes + ['approver'] }
= link_to approver.name, approver, class: 'qa-approver'
- if can_update_approvers
.float-right
- if unsaved_approvers
%button{ class: 'btn-sm btn btn-remove', title: _('Remove approver'), data: { confirm: _("Are you sure you want to remove approver %{name}") % { name: approver.name } } }
= icon("sign-out")
= _('Remove')
- else
= link_to project_merge_request_approver_via_user_id_path(@project, issuable, user_id: approver.id), data: { confirm: _("Are you sure you want to remove approver %{name}") % { name: approver.name } }, method: :delete, class: "btn-sm btn btn-remove", title: _('Remove approver') do
= icon("sign-out")
= _('Remove')
- presenter.overall_approver_groups.each do |approver_group|
%li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] }
= _('Group:')
= link_to approver_group.group.name, approver_group.group
- if can_update_approvers
.float-right
- if unsaved_approvers
%button{ class: "btn-sm btn btn-remove", title: _('Remove group'), data: { confirm: _("Are you sure you want to remove group %{name}") % { name: approver_group.group.name } } }
= icon("sign-out")
= _('Remove')
- else
= link_to project_merge_request_approver_group_path(@project, issuable, approver_group), data: { confirm: _("Are you sure you want to remove group %{name}") % { name: approver_group.group.name } }, method: :delete, class: "btn-sm btn btn-remove", title: _('Remove group') do
= icon("sign-out")
= _('Remove')
.col-sm-12
.form-group.row
= form.label :approvals_before_merge, class: 'label-bold' do
= _('Approvals required')
= form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_update_approvers
ee/app/views/shared/merge_request_approvals_settings/_multiple_rules_form.html.haml deleted 100644 → 0
View file @ d7d420f1
.form-group
= form.label :approver_ids, class: 'label-bold' do
= _("Add approvers")
#js-mr-approvals-settings{ data: { 'project_id': @project.id,
'project_path': expose_path(api_v4_projects_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)),
'allow_multi_rule': @project.multiple_approval_rules_available?.to_s } }
.text-center.prepend-top-default
= sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
ee/app/views/shared/merge_request_approvals_settings/_single_rule_form.html.haml deleted 100644 → 0
View file @ d7d420f1
.form-group
= form.label :approver_ids, class: 'label-bold' do
= _("Add approvers")
= hidden_field_tag "project[approver_ids]"
= hidden_field_tag "project[approver_group_ids]"
.row
.col-md-9
.input-group.input-btn-group
= hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project', :style => "max-width: unset;" }
%button.btn.btn-success.js-add-approvers.js-dirty-submit{ type: 'button', title: _('Add approver(s)') }
= _("Add")
.form-text.text-muted
= _("Add users or groups who are allowed to approve every merge request")
.card.prepend-top-10.js-current-approvers
.load-wrapper.hidden
= icon('spinner spin', class: 'approver-list-loader')
.card-header
= _("Approvers")
%span.badge.badge-pill
- ids = []
- project.approvers.each do |user|
- ids << user.user_id
- project.approver_groups.each do |group|
- group.users.each do |user|
- unless ids.include?(user.id)
- ids << user.id
= ids.count
%ul.content-list.approver-list
- project.approvers.each do |approver|
%li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } }
= link_to approver.user.name, approver.user
.float-right
- confirm_message = _("Are you sure you want to remove approver %{name}?") % { name: approver.user.name }
%button{ href: project_approver_path(project, approver), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove approver") }
= icon("trash")
- project.approver_groups.each do |approver_group|
%li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } }
%span
%span.light
= _("Group:")
= link_to approver_group.group.name, approver_group.group
%span.badge.badge-pill
= approver_group.group.members.count
.float-right
- confirm_message = _("Are you sure you want to remove group %{name}?") % { name: approver_group.group.name }
%button{ href: project_approver_group_path(project, approver_group), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove group") }
= icon("trash")
- if project.approvers.empty? && project.approver_groups.empty?
%li= _("There are no approvers")
.form-group
= form.label :approvals_before_merge, class: 'label-bold' do
= _("Approvals required")
= form.number_field :approvals_before_merge, class: "form-control w-auto", min: 0
.form-text.text-muted
= _("Set number of approvers required before open merge requests can be merged")
ee/changelogs/unreleased/10685-remove-feature-flag.yml 0 → 100644
View file @ b03dbecc
---
title: Remove old approver system in favor of new approval rule system
merge_request: 12436
author:
type: removed
ee/lib/api/helpers/approval_helpers.rb
View file @ b03dbecc
......@@ -4,11 +4,7 @@ module API
module Helpers
module ApprovalHelpers
def present_approval(merge_request)
if Feature.enabled?(:approval_rules, merge_request.project, default_enabled: true)
present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
else
present merge_request.present(current_user: current_user), with: ::EE::API::Entities::MergeRequestApprovals, current_user: current_user
end
present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
end
end
end
......
ee/lib/api/merge_request_approvals.rb
View file @ b03dbecc
......@@ -54,8 +54,6 @@ module API
hidden: true
}
get 'approval_settings' do
not_found! unless ::Feature.enabled?(:approval_rules, user_project, default_enabled: true)
merge_request = find_merge_request_with_access(params[:merge_request_iid])
present merge_request.approval_state, with: ::EE::API::Entities::MergeRequestApprovalRules, current_user: current_user
......
ee/lib/api/project_approval_rules.rb
View file @ b03dbecc
......@@ -3,7 +3,6 @@
module API
class ProjectApprovalRules < ::Grape::API
before { authenticate! }
before { not_found! unless ::Feature.enabled?(:approval_rules, user_project, default_enabled: true) }
ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
......
ee/spec/controllers/projects/merge_requests_controller_spec.rb
View file @ b03dbecc
......@@ -5,12 +5,11 @@ shared_examples 'approvals' do
JSON.parse(response.body)
end
let!(:approver) { create(:approver, target: project) }
let!(:user_approver) { create(:approver, target: project, user: user) }
let!(:approver) { create(:user) }
let!(:approval_rule) { create(:approval_project_rule, project: project, users: [approver, user], approvals_required: 2) }
before do
merge_request.update_attribute :approvals_before_merge, 2
project.add_developer(approver.user)
project.add_developer(approver)
end
describe 'approve' do
......@@ -34,12 +33,12 @@ shared_examples 'approvals' do
expect(approvals['user_has_approved']).to be true
expect(approvals['user_can_approve']).to be false
expect(approvals['suggested_approvers'].size).to eq 1
expect(approvals['suggested_approvers'][0]['username']).to eq approver.user.username
expect(approvals['suggested_approvers'][0]['username']).to eq approver.username
end
end
describe 'approvals' do
let!(:approval) { create(:approval, merge_request: merge_request, user: approver.user) }
let!(:approval) { create(:approval, merge_request: merge_request, user: approver) }
def get_approvals
get :approvals,
......@@ -59,29 +58,12 @@ shared_examples 'approvals' do
expect(response).to be_success
expect(approvals['approvals_left']).to eq 1
expect(approvals['approved_by'].size).to eq 1
expect(approvals['approved_by'][0]['user']['username']).to eq approver.user.username
expect(approvals['approved_by'][0]['user']['username']).to eq approver.username
expect(approvals['user_has_approved']).to be false
expect(approvals['user_can_approve']).to be true
expect(approvals['suggested_approvers'].size).to eq 1
expect(approvals['suggested_approvers'][0]['username']).to eq user.username
end
context 'with unauthorized group' do
let(:private_group) { create(:group_with_members, :private) }
before do
create(:approver_group, target: merge_request, group: private_group)
end
it 'does not expose approvers from a private group the current user has no access to' do
get_approvals
approvals = json_response
expect(response).to be_success
expect(approvals['suggested_approvers'].size).to eq(0)
end
end
end
describe 'unapprove' do
......@@ -119,7 +101,6 @@ describe Projects::MergeRequestsController do
let(:viewer) { user }
before do
stub_feature_flags(approval_rules: false)
sign_in(viewer)
end
......
ee/spec/features/merge_request/user_approves_spec.rb
View file @ b03dbecc
require 'rails_helper'
describe 'Merge request > User approves', :js do
# TODO: https://gitlab.com/gitlab-org/gitlab-ee/issues/9430
xdescribe 'Merge request > User approves', :js do
let(:user) { create(:user) }
let(:project) { create(:project, :public, :repository, approvals_before_merge: 1) }
let(:merge_request) { create(:merge_request, source_project: project) }
before do
stub_feature_flags(approval_rules: false)
end
context 'Approving by approvers from groups' do
let(:other_user) { create(:user) }
let(:group) { create :group }
......@@ -65,7 +62,6 @@ describe 'Merge request > User approves', :js do
context 'when CI is running but no approval given' do
before do
stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
create :approver_group, group: group, target: merge_request
pipeline = create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch)
merge_request.update(head_pipeline: pipeline)
......
ee/spec/features/merge_request/user_creates_merge_request_spec.rb
View file @ b03dbecc
......@@ -18,14 +18,12 @@ describe "User creates a merge request", :js do
let(:user2) { create(:user) }
before do
stub_feature_flags(approval_rules: false)
project.add_maintainer(user)
project.add_maintainer(user2)
project.add_maintainer(approver)
sign_in(user)
project.approvers.create(user_id: approver.id)
create(:approval_project_rule, project: project, users: [approver])
visit(project_new_merge_request_path(project))
end
......@@ -43,19 +41,20 @@ describe "User creates a merge request", :js do
expect(find_field("merge_request_description").value).to eq(template_text)
page.within("ul .unsaved-approvers") do
expect(page).to have_content(approver.name)
end
page.within(".suggested-approvers") do
expect(page).to have_content(user2.name)
page.within('.js-approval-rules') do
expect(page).to have_css("img[alt=\"#{approver.name}\"]")
end
click_link(user2.name)
page.within("ul.approver-list") do
expect(page).to have_content(user2.name)
end
# TODO: Fix https://gitlab.com/gitlab-org/gitlab-ee/issues/11527
# page.within(".suggested-approvers") do
# expect(page).to have_content(user2.name)
# end
#
# click_link(user2.name)
#
# page.within("ul.approver-list") do
# expect(page).to have_content(user2.name)
# end
fill_in("Title", with: title)
click_button("Submit merge request")
......@@ -64,8 +63,8 @@ describe "User creates a merge request", :js do
click_link("Edit", match: :first)
end
page.within("ul.approver-list") do
expect(page).to have_content(user2.name)
end
# page.within("ul.approver-list") do
# expect(page).to have_content(user2.name)
# end
end
end
ee/spec/features/merge_request/user_sees_approval_widget_spec.rb
View file @ b03dbecc
......@@ -6,8 +6,6 @@ describe 'Merge request > User sees approval widget', :js do
let(:merge_request) { create(:merge_request, source_project: project) }
before do
stub_feature_flags(approval_rules: false)
sign_in(user)
end
......@@ -22,17 +20,14 @@ describe 'Merge request > User sees approval widget', :js do
visit project_merge_request_path(project, merge_request)
end
it 'does not show checking ability text' do
# TODO: https://gitlab.com/gitlab-org/gitlab-ee/issues/9430
xit 'does not show checking ability text' do
expect(find('.js-mr-approvals')).not_to have_text('Checking ability to merge automatically')
expect(find('.js-mr-approvals')).to have_selector('.approvals-body')
end
end
context 'when rules are enabled' do
before do
stub_feature_flags(approval_rules: true)
end
context 'merge request approvers enabled' do
let(:project) { create(:project, :public, :repository, approvals_before_merge: 3) }
......
ee/spec/features/merge_request/user_sets_approvers_spec.rb
View file @ b03dbecc
......@@ -2,13 +2,12 @@ require 'rails_helper'
describe 'Merge request > User sets approvers', :js do
include ProjectForksHelper
include FeatureApprovalHelper
let(:user) { create(:user) }
let(:project) { create(:project, :public, :repository, approvals_before_merge: 1) }
before do
stub_feature_flags(approval_rules: false)
end
let!(:config_selector) { '.js-approval-rules' }
let!(:modal_selector) { '#mr-edit-approvals-create-modal' }
context 'when editing an MR with a different author' do
let(:author) { create(:user) }
......@@ -20,11 +19,12 @@ describe 'Merge request > User sets approvers', :js do
sign_in(user)
visit edit_project_merge_request_path(project, merge_request)
find('#s2id_merge_request_approver_ids .select2-input').click
end
it 'does not allow setting the author as an approver but allows setting the current user as an approver' do
open_modal
open_approver_select
expect(find('.select2-results')).not_to have_content(author.name)
expect(find('.select2-results')).to have_content(user.name)
end
......@@ -41,13 +41,13 @@ describe 'Merge request > User sets approvers', :js do
sign_in(user)
visit project_new_merge_request_path(forked_project, merge_request: { target_branch: 'master', source_branch: 'feature' })
find('#s2id_merge_request_approver_ids .select2-input').click
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' do
open_modal
open_approver_select
expect(find('.select2-results')).to have_content(other_user.name)
expect(find('.select2-results')).not_to have_content(user.name)
expect(find('.select2-results')).not_to have_content(non_member.name)
end
end
......@@ -57,7 +57,6 @@ describe 'Merge request > User sets approvers', :js do
let(:other_user) { create(:user) }
before do
stub_feature_flags(approval_rules: false) # TODO https://gitlab.com/gitlab-org/gitlab-ee/issues/9430
project.add_developer(user)
project.add_developer(other_user)
......@@ -69,42 +68,55 @@ describe 'Merge request > User sets approvers', :js do
group.add_developer(other_user)
visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' })
find('#s2id_merge_request_approver_group_ids .select2-input').click
wait_for_requests
open_modal
open_approver_select
expect(find('.select2-results')).not_to have_content(group.name)
close_approver_select
group.add_developer(user) # only display groups that user has access to
open_approver_select
expect(find('.select2-results')).to have_content(group.name)
find('.select2-results').click
find('.select2-results .user-result', text: group.name).click
close_approver_select
click_button 'Add'
click_button 'Update approvers'
click_on("Submit merge request")
wait_for_all_requests
find('.approvals-components')
expect(page).to have_content("Requires 1 more approval")
expect(page).to have_selector(".approvals-required-text a[title='#{other_user.name}']")
expect(page).to have_content("Requires approval.")
expect(page).to have_selector("img[alt='#{other_user.name}']")
end
it 'allows delete approvers group when it is set in project' do
approver = create :user
project.add_developer(approver)
group = create :group
group.add_developer(other_user)
create :approver_group, group: group, target: project
create :approver, user: approver, target: project
group.add_developer(approver)
create :approval_project_rule, project: project, users: [approver], groups: [group], approvals_required: 1
visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' })
within('.approver-list li.approver-group') do
accept_confirm { click_on "Remove" }
end
open_modal
remove_approver(group.name)
expect(page).to have_css('.approver-list li', count: 1)
within(modal_selector) do
expect(page).to have_css('.content-list li', count: 1)
end
click_button 'Update approvers'
click_on("Submit merge request")
wait_for_all_requests
click_on("View eligible approvers") if page.has_button?("View eligible approvers")
wait_for_requests
expect(page).not_to have_selector(".approvals-required-text a[title='#{other_user.name}']")
expect(page).to have_selector(".approvals-required-text a[title='#{approver.name}']")
expect(page).to have_content("Requires 1 more approval")
expect(page).not_to have_selector(".js-approvers img[alt='#{other_user.name}']")
expect(page).to have_selector(".js-approvers img[alt='#{approver.name}']")
end
end
......@@ -113,7 +125,6 @@ describe 'Merge request > User sets approvers', :js do
let(:merge_request) { create(:merge_request, source_project: project) }
before do
stub_feature_flags(approval_rules: false) # TODO https://gitlab.com/gitlab-org/gitlab-ee/issues/9430
project.add_developer(user)
sign_in(user)
......@@ -122,71 +133,94 @@ describe 'Merge request > User sets approvers', :js do
it 'allows setting groups as approvers' do
group = create :group
group.add_developer(other_user)
group.add_developer(user)
visit edit_project_merge_request_path(project, merge_request)
find('#s2id_merge_request_approver_group_ids .select2-input').click
wait_for_requests
open_modal
open_approver_select
expect(find('.select2-results')).not_to have_content(group.name)
close_approver_select
group.add_developer(user) # only display groups that user has access to
open_approver_select
expect(find('.select2-results')).to have_content(group.name)
find('.select2-results').click
find('.select2-results .user-result', text: group.name).click
close_approver_select
click_button 'Add'
click_button 'Update approvers'
click_on("Save changes")
wait_for_all_requests
wait_for_requests
find('.approvals-components')
expect(page).to have_content("Requires 1 more approval")
expect(page).to have_content("Requires approval.")
expect(page).to have_selector("img[alt='#{other_user.name}']")
end
it 'allows delete approvers group when it`s set in project' do
approver = create :user
project.add_developer(approver)
group = create :group
group.add_developer(other_user)
create :approver_group, group: group, target: project
create :approver, user: approver, target: project
group.add_developer(approver)
create :approval_project_rule, project: project, users: [approver], groups: [group], approvals_required: 1
visit edit_project_merge_request_path(project, merge_request)
within('.approver-list li.approver-group') do
accept_confirm { click_on "Remove" }
end
open_modal
remove_approver(group.name)
expect(page).to have_css('.approver-list li', count: 1)
wait_for_requests
within(modal_selector) do
expect(page).to have_css('.content-list li', count: 1)
end
click_button 'Update approvers'
click_on("Save changes")
wait_for_all_requests
find('.approvals-components')
expect(page).to have_content("Requires 1 more approval")
expect(page).to have_selector(".approvals-required-text a[title='#{approver.name}']")
click_on("View eligible approvers")
wait_for_requests
expect(page).not_to have_selector(".js-approvers img[alt='#{other_user.name}']")
expect(page).to have_selector(".js-approvers img[alt='#{approver.name}']")
expect(page).to have_content("Requires approval.")
end
it 'allows changing approvals number' do
create_list :approver, 3, target: project
approvers = create_list(:user, 3)
approvers.each { |approver| project.add_developer(approver) }
create :approval_project_rule, project: project, users: approvers, approvals_required: 2
visit project_merge_request_path(project, merge_request)
wait_for_requests
# project setting in the beginning on the show MR page
find('.approvals-components')
expect(page).to have_content("Requires 1 more approval")
expect(page).to have_content("Requires 2 more approvals")
find('.merge-request').click_on 'Edit'
open_modal
# project setting in the beginning on the edit MR page
expect(find('#merge_request_approvals_before_merge').value).to eq('1')
expect(page).to have_field 'No. approvals required', exact: 2
fill_in 'merge_request_approvals_before_merge', with: '3'
fill_in 'No. approvals required', with: '3'
click_button 'Update approvers'
click_on('Save changes')
wait_for_all_requests
# new MR setting on the show MR page
find('.approvals-components')
expect(page).to have_content("Requires 3 more approvals")
# new MR setting on the edit MR page
find('.merge-request').click_on 'Edit'
wait_for_requests
# new MR setting on the edit MR page
expect(find('#merge_request_approvals_before_merge').value).to eq('3')
open_modal
expect(page).to have_field 'No. approvals required', exact: 3
end
end
end
......
ee/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb
View file @ b03dbecc
......@@ -14,7 +14,6 @@ describe 'User approves a merge request', :js do
context 'when user can approve' do
before do
stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
visit(merge_request_path(merge_request))
end
......@@ -31,12 +30,12 @@ describe 'User approves a merge request', :js do
context 'when a merge request is approved additionally' do
before do
stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
project.add_developer(user2)
project.add_developer(user3)
end
it 'shows multiple approvers beyond the needed count' do
# TODO: https://gitlab.com/gitlab-org/gitlab-ee/issues/9430
xit 'shows multiple approvers beyond the needed count' do
visit(merge_request_path(merge_request))
click_button('Approve')
......@@ -47,7 +46,8 @@ describe 'User approves a merge request', :js do
sign_in_visit_merge_request(user2, true)
sign_in_visit_merge_request(user3, true)
expect(all('.js-approver-list-member').count).to eq(3)
widget = find('.js-mr-approvals').find(:xpath, '..')
expect(widget.all('.user-avatar-link').count).to eq(3)
end
it "doesn't show the add approval when a merge request is closed" do
......@@ -75,7 +75,6 @@ describe 'User approves a merge request', :js do
context 'when user cannot approve' do
before do
stub_feature_flags(approval_rules: false) # TODO check in !9001 when feature enabled
merge_request.approvers.create(user_id: user2.id)
visit(merge_request_path(merge_request))
......@@ -85,7 +84,7 @@ describe 'User approves a merge request', :js do
page.within('.mr-state-widget') do
expect(page).to have_button('Merge', disabled: true)
expect(page).not_to have_button('Approve')
expect(page).to have_content('Requires 1 more approval')
expect(page).to have_content('Requires approval')
end
end
end
......
ee/spec/features/projects/settings/merge_requests_settings_spec.rb
View file @ b03dbecc
......@@ -2,16 +2,17 @@ require 'spec_helper'
describe 'Project settings > [EE] Merge Requests', :js do
include GitlabRoutingHelper
include FeatureApprovalHelper
let(:user) { create(:user) }
let(:project) { create(:project, approvals_before_merge: 1) }
let(:group) { create(:group) }
let(:group_member) { create(:user) }
let(:non_member) { create(:user) }
let!(:config_selector) { '.js-approval-rules' }
let!(:modal_selector) { '#project-settings-approvals-create-modal' }
before do
stub_feature_flags(approval_rules: false)
sign_in(user)
project.add_maintainer(user)
group.add_developer(user)
......@@ -21,68 +22,68 @@ describe 'Project settings > [EE] Merge Requests', :js do
it 'adds approver' do
visit edit_project_path(project)
find('#s2id_approver_user_and_group_ids .select2-input').click
wait_for_requests
open_modal
open_approver_select
expect(find('.select2-results')).to have_content(user.name)
expect(find('.select2-results')).not_to have_content(non_member.name)
find('.user-result', text: user.name).click
close_approver_select
click_button 'Add'
expect(find('.js-current-approvers')).to have_content(user.name)
expect(find('.content-list')).to have_content(user.name)
find('.js-select-user-and-group').click
open_approver_select
expect(find('.select2-results')).not_to have_content(user.name)
end
it 'filter approvers' do
visit edit_project_path(project)
find('.js-select-user-and-group').click
close_approver_select
click_button 'Update approvers'
wait_for_requests
expect(find('.select2-results')).to have_content(user.name)
expect(find('.select2-results')).not_to have_content(non_member.name)
expect_avatar(find('.js-members'), user)
end
it 'adds approver group' do
visit edit_project_path(project)
find('#s2id_approver_user_and_group_ids .select2-input').click
wait_for_requests
within('.js-current-approvers') do
expect(find('.card-header .badge')).to have_content('0')
end
open_modal
open_approver_select
expect(find('.select2-results')).to have_content(group.name)
find('.select2-results .group-result').click
find('.user-result', text: group.name).click
close_approver_select
click_button 'Add'
expect(find('.approver-list-loader')).to be_visible
expect(page).to have_css('.js-current-approvers li.approver-group', count: 1)
expect(find('.content-list')).to have_content(group.name)
expect(page).to have_css('.js-current-approvers li.approver-group', count: 1)
within('.js-current-approvers') do
expect(find('.card-header .badge')).to have_content('2')
end
click_button 'Update approvers'
wait_for_requests
expect_avatar(find('.js-members'), group.users)
end
context 'with an approver group' do
let(:non_group_approver) { create(:user) }
let!(:rule) { create(:approval_project_rule, project: project, groups: [group], users: [non_group_approver]) }
before do
create(:approver_group, group: group, target: project)
project.add_developer(non_group_approver)
end
it 'removes approver group' do
visit edit_project_path(project)
expect(find('.js-current-approvers')).to have_content(group.name)
expect_avatar(find('.js-members'), rule.approvers)
within('.js-current-approvers') do
accept_confirm { click_on "Remove" }
end
open_modal
remove_approver(group.name)
click_button "Update approvers"
wait_for_requests
expect(find('.js-current-approvers')).not_to have_content(group.name)
expect_avatar(find('.js-members'), [non_group_approver])
end
end
......
ee/spec/frontend/vue_mr_widget/components/approvals/multiple_rule/approvals_footer_spec.js → ee/spec/frontend/vue_mr_widget/components/approvals/approvals_footer_spec.js
View file @ b03dbecc
......@@ -3,8 +3,8 @@ import { createLocalVue, shallowMount } from '@vue/test-utils';
import { GlButton, GlLoadingIcon } from '@gitlab/ui';
import Icon from '~/vue_shared/components/icon.vue';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import ApprovalsList from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_list.vue';
import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_footer.vue';
import ApprovalsList from 'ee/vue_merge_request_widget/components/approvals/approvals_list.vue';
import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/approvals_footer.vue';
const localVue = createLocalVue();
......
ee/spec/frontend/vue_mr_widget/components/approvals/multiple_rule/approvals_list_spec.js → ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js
View file @ b03dbecc
import { createLocalVue, shallowMount } from '@vue/test-utils';
import _ from 'underscore';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import ApprovedIcon from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approved_icon.vue';
import ApprovalsList from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_list.vue';
import ApprovedIcon from 'ee/vue_merge_request_widget/components/approvals/approved_icon.vue';
import ApprovalsList from 'ee/vue_merge_request_widget/components/approvals/approvals_list.vue';
const localVue = createLocalVue();
......
ee/spec/frontend/vue_mr_widget/components/approvals/multiple_rule/approvals_summary_optional_spec.js → ee/spec/frontend/vue_mr_widget/components/approvals/approvals_summary_optional_spec.js
View file @ b03dbecc
......@@ -4,7 +4,7 @@ import {
OPTIONAL,
OPTIONAL_CAN_APPROVE,
} from 'ee/vue_merge_request_widget/components/approvals/messages';
import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue';
import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/approvals_summary_optional.vue';
const localVue = createLocalVue();
......
ee/spec/frontend/vue_mr_widget/components/approvals/multiple_rule/approvals_summary_spec.js → ee/spec/frontend/vue_mr_widget/components/approvals/approvals_summary_spec.js
View file @ b03dbecc
......@@ -3,7 +3,7 @@ import _ from 'underscore';
import { toNounSeriesText } from '~/lib/utils/grammar';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import { APPROVED_MESSAGE } from 'ee/vue_merge_request_widget/components/approvals/messages';
import ApprovalsSummary from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary.vue';
import ApprovalsSummary from 'ee/vue_merge_request_widget/components/approvals/approvals_summary.vue';
const localVue = createLocalVue();
......
ee/spec/frontend/vue_mr_widget/components/approvals/multiple_rule/approved_icon_spec.js → ee/spec/frontend/vue_mr_widget/components/approvals/approved_icon_spec.js
View file @ b03dbecc
import { createLocalVue, shallowMount } from '@vue/test-utils';
import Icon from '~/vue_shared/components/icon.vue';
import ApprovedIcon from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approved_icon.vue';
import ApprovedIcon from 'ee/vue_merge_request_widget/components/approvals/approved_icon.vue';
const localVue = createLocalVue();
const EXPECTED_SIZE = 16;
......
ee/spec/javascripts/approvals/setup_single_rule_approvals_spec.js deleted 100644 → 0
View file @ d7d420f1
import $ from 'jquery';
import setup from 'ee/approvals/setup_single_rule_approvals';
describe('EE setup_single_rule_approvals', () => {
preloadFixtures('merge_requests_ee/merge_request_edit.html');
let $approversEl;
let $suggestionEl;
beforeEach(() => {
loadFixtures('merge_requests_ee/merge_request_edit.html');
$approversEl = $('ul.approver-list');
$suggestionEl = $('.suggested-approvers');
setup();
});
describe('add suggested approver', () => {
it('should add approver when suggested user is clicked', () => {
expect($approversEl.find('li.approver').length).toEqual(0);
$suggestionEl
.find('a')
.first()
.click();
$suggestionEl
.find('a')
.last()
.click();
expect($approversEl.find('li.approver').length).toEqual(2);
});
it('only adds approver once when the same suggested user is clicked multiple times', () => {
expect($approversEl.find('li.approver').length).toEqual(0);
$suggestionEl
.find('a')
.first()
.click();
$suggestionEl
.find('a')
.first()
.click();
expect($approversEl.find('li.approver').length).toEqual(1);
});
});
describe('remove unsaved approver', () => {
beforeEach(() => {
$suggestionEl.find('a').click(); // Adds two approvers
});
it('should remove approver if confirm window result is positive', () => {
spyOn(window, 'confirm').and.returnValue(true);
$approversEl
.find('.unsaved-approvers.approver .btn-remove')
.first()
.click();
expect($approversEl.find('li.approver').length).toEqual(1);
});
it('should not remove approver if confirm window result is negative', () => {
spyOn(window, 'confirm').and.returnValue(false);
$approversEl
.find('.unsaved-approvers.approver .btn-remove')
.first()
.click();
expect($approversEl.find('li.approver').length).toEqual(2);
});
});
describe('remove unsaved approver group', () => {
it('should remove approver group if confirm window result is positive', () => {
spyOn(window, 'confirm').and.returnValue(true);
expect($approversEl.find('li.approver-group').length).toEqual(1);
$approversEl
.find('.unsaved-approvers.approver-group .btn-remove')
.first()
.click();
expect($approversEl.find('li.approver-group').length).toEqual(0);
});
it('should not remove approver group if confirm window result is negative', () => {
spyOn(window, 'confirm').and.returnValue(false);
expect($approversEl.find('li.approver-group').length).toEqual(1);
$approversEl
.find('.unsaved-approvers.approver-group .btn-remove')
.first()
.click();
expect($approversEl.find('li.approver-group').length).toEqual(1);
});
});
});
ee/spec/javascripts/approvers_select_spec.js deleted 100644 → 0
View file @ d7d420f1
import ApproversSelect from 'ee/approvers_select';
describe('ApproversSelect', () => {
describe('saveApproversComplete', () => {
let $input;
let $approverSelect;
let $loadWrapper;
beforeEach(() => {
$input = {
val: jasmine.createSpy(),
};
$approverSelect = {
select2: jasmine.createSpy().and.callFake(function() {
return this;
}),
trigger: jasmine.createSpy(),
};
$loadWrapper = {
addClass: jasmine.createSpy(),
};
ApproversSelect.saveApproversComplete($input, $approverSelect, $loadWrapper);
});
it('should empty the $input value', () => {
expect($input.val).toHaveBeenCalledWith('');
});
it('should empty the select2 value and trigger a change event', () => {
expect($approverSelect.select2).toHaveBeenCalledWith('val', '');
expect($approverSelect.trigger).toHaveBeenCalledWith('change');
});
it('should hide loadWrapper', () => {
expect($loadWrapper.addClass).toHaveBeenCalledWith('hidden');
});
});
describe('formatResult', () => {
it('escapes name', () => {
const output = ApproversSelect.formatResult({
name: '<script>alert("testing")</script>',
username: 'testing',
avatar_url: gl.TEST_HOST,
full_name: '<script>alert("testing")</script>',
full_path: 'testing',
});
expect(output).not.toContain('<script>alert("testing")</script>');
});
it('escapes full name', () => {
const output = ApproversSelect.formatResult({
username: 'testing',
avatar_url: gl.TEST_HOST,
full_name: '<script>alert("testing")</script>',
full_path: 'testing',
});
expect(output).not.toContain('<script>alert("testing")</script>');
});
});
describe('formatSelection', () => {
it('escapes full name', () => {
expect(
ApproversSelect.formatSelection({
full_name: '<script>alert("testing")</script>',
}),
).not.toBe('<script>alert("testing")</script>');
});
it('escapes name', () => {
expect(
ApproversSelect.formatSelection({
name: '<script>alert("testing")</script>',
}),
).not.toBe('<script>alert("testing")</script>');
});
});
});
ee/spec/javascripts/fixtures/merge_requests.rb deleted 100644 → 0
View file @ d7d420f1
# frozen_string_literal: true
require 'spec_helper'
describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :controller do
include JavaScriptFixturesHelpers
let(:admin) { create(:admin) }
let(:namespace) { create(:namespace, name: 'frontend-fixtures' )}
let(:project) { create(:project, :repository, namespace: namespace, path: 'merge-requests-project') }
let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: admin, approvals_before_merge: 2) }
let(:suggested_approvers) do
create_list(:user, 3).tap do |users|
users.each { |user| project.add_developer(user) }
end
end
render_views
before(:all) do
clean_frontend_fixtures('merge_requests_ee/')
end
before do
stub_feature_flags(approval_rules: false)
# Ensure some approver suggestions are displayed
service = double(:service)
expect(::Gitlab::AuthorityAnalyzer).to receive(:new).and_return(service)
expect(service).to receive(:calculate).and_return(suggested_approvers)
# Ensure a project level group is present (but unsaved)
approver_group = create(:approver_group, target: project)
approver_group.group.add_owner(create(:owner))
stub_feature_flags(approval_rules: false)
sign_in(admin)
end
after do
remove_repository(project)
end
it 'merge_requests_ee/merge_request_edit.html' do
get :edit,
params: {
id: merge_request.id,
namespace_id: project.namespace.to_param,
project_id: project
},
format: :html
expect(merge_request.all_approvers_including_groups.size).to eq(1)
expect(response).to be_success
end
end
ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_auth_spec.js → ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_auth_spec.js
View file @ b03dbecc
import { createLocalVue, shallowMount } from '@vue/test-utils';
import { GlLoadingIcon } from '@gitlab/ui';
import ApprovalsAuth from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_auth.vue';
import ApprovalsAuth from 'ee/vue_merge_request_widget/components/approvals/approvals_auth.vue';
const TEST_PASSWORD = 'password';
......
ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_spec.js → ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_spec.js
View file @ b03dbecc
import { createLocalVue, shallowMount } from '@vue/test-utils';
import { GlButton, GlLoadingIcon } from '@gitlab/ui';
import eventHub from '~/vue_merge_request_widget/event_hub';
import Approvals from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue';
import ApprovalsSummary from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary.vue';
import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue';
import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_footer.vue';
import ApprovalsAuth from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_auth.vue';
import Approvals from 'ee/vue_merge_request_widget/components/approvals/approvals.vue';
import ApprovalsSummary from 'ee/vue_merge_request_widget/components/approvals/approvals_summary.vue';
import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/approvals_summary_optional.vue';
import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/approvals_footer.vue';
import ApprovalsAuth from 'ee/vue_merge_request_widget/components/approvals/approvals_auth.vue';
import {
FETCH_LOADING,
......
ee/spec/javascripts/vue_mr_widget/components/approvals/single_rule/approvals_body_spec.js deleted 100644 → 0
View file @ d7d420f1
import Vue from 'vue';
import ApprovalsBody from 'ee/vue_merge_request_widget/components/approvals/single_rule/approvals_body.vue';
import {
OPTIONAL,
OPTIONAL_CAN_APPROVE,
} from 'ee/vue_merge_request_widget/components/approvals/messages';
describe('Approvals Body Component', () => {
let vm;
const initialData = {
mr: {
isOpen: true,
},
service: {},
suggestedApprovers: [{ name: 'Approver 1' }],
userCanApprove: false,
userHasApproved: true,
approvedBy: [],
approvalsLeft: 1,
approvalsOptional: false,
pendingAvatarSvg: '<svg></svg>',
checkmarkSvg: '<svg></svg>',
};
beforeEach(() => {
setFixtures('<div id="mock-container"></div>');
const ApprovalsBodyComponent = Vue.extend(ApprovalsBody);
vm = new ApprovalsBodyComponent({
el: '#mock-container',
propsData: initialData,
});
});
afterEach(() => {
vm.$destroy();
});
it('should correctly set component props', () => {
Object.keys(vm).forEach(propKey => {
if (initialData[propKey]) {
expect(vm[propKey]).toBe(initialData[propKey]);
}
});
});
describe('Computed properties', () => {
describe('approvalsRequiredStringified', () => {
it('should display the correct string for 1 possible approver', () => {
const correctText = 'Requires 1 more approval by';
expect(vm.approvalsRequiredStringified).toBe(correctText);
});
it('should display the correct string for 2 possible approvers', done => {
const correctText = 'Requires 2 more approvals by';
vm.approvalsLeft = 2;
vm.suggestedApprovers.push({ name: 'Approver 2' });
Vue.nextTick(() => {
expect(vm.approvalsRequiredStringified).toBe(correctText);
done();
});
});
it('should display the correct string for 0 approvals required', done => {
const correctText = OPTIONAL;
vm.approvalsOptional = true;
Vue.nextTick(() => {
expect(vm.approvalsRequiredStringified).toBe(correctText);
done();
});
});
it('should display the correct string for 0 approvals required and if the user is able to approve', done => {
const correctText = OPTIONAL_CAN_APPROVE;
vm.approvalsOptional = true;
vm.userCanApprove = true;
Vue.nextTick(() => {
expect(vm.approvalsRequiredStringified).toBe(correctText);
done();
});
});
it('shows the "Merge request approved" copy when there are enough approvals in place', done => {
vm.approvalsLeft = 0;
Vue.nextTick(() => {
expect(vm.approvalsRequiredStringified).toBe('Merge request approved');
done();
});
});
it('shows the correct copy when there are enough approvals in place but user can approve', done => {
vm.approvalsLeft = 0;
vm.userCanApprove = true;
Vue.nextTick(() => {
expect(vm.approvalsRequiredStringified).toBe(
'Merge request approved; you can approve additionally',
);
done();
});
});
it('shows the "Requires 1 more approval" without by when no suggested approvals are available', done => {
const correctText = 'Requires 1 more approval';
vm.suggestedApprovers = [];
Vue.nextTick(() => {
expect(vm.approvalsRequiredStringified).toBe(correctText);
done();
});
});
it('shows the "Requires 2 more approvals" without by when no suggested approvals are available', done => {
const correctText = 'Requires 2 more approvals';
vm.approvalsLeft = 2;
vm.suggestedApprovers = [];
Vue.nextTick(() => {
expect(vm.approvalsRequiredStringified).toBe(correctText);
done();
});
});
});
describe('showApproveButton', () => {
it('should not be true when the user cannot approve', done => {
vm.userCanApprove = false;
vm.userHasApproved = true;
Vue.nextTick(() => {
expect(vm.showApproveButton).toBe(false);
done();
});
});
it('should be true when the user can approve', done => {
vm.userCanApprove = true;
vm.userHasApproved = false;
Vue.nextTick(() => {
expect(vm.showApproveButton).toBe(true);
done();
});
});
});
describe('approveButtonText', () => {
it('The approve button should have the "Approve" text', done => {
vm.approvalsLeft = 1;
vm.userHasApproved = false;
vm.userCanApprove = true;
Vue.nextTick(() => {
expect(vm.approveButtonText).toBe('Approve');
done();
});
});
it('The approve button should have the "Add approval" text', done => {
vm.approvalsLeft = 0;
vm.userHasApproved = false;
vm.userCanApprove = true;
Vue.nextTick(() => {
expect(vm.approveButtonText).toBe('Add approval');
done();
});
});
});
});
});
ee/spec/javascripts/vue_mr_widget/components/approvals/single_rule/approvals_footer_spec.js deleted 100644 → 0
View file @ d7d420f1
import Vue from 'vue';
import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/single_rule/approvals_footer.vue';
import { TEST_HOST } from 'spec/test_constants';
describe('Approvals Footer Component', () => {
let vm;
const initialData = {
mr: {
state: 'readyToMerge',
},
service: {},
userCanApprove: false,
userHasApproved: true,
approvedBy: [],
approvalsLeft: 3,
};
beforeEach(() => {
setFixtures('<div id="mock-container"></div>');
const ApprovalsFooterComponent = Vue.extend(ApprovalsFooter);
vm = new ApprovalsFooterComponent({
el: '#mock-container',
propsData: initialData,
});
});
afterEach(() => {
vm.$destroy();
});
it('should correctly set component props', () => {
Object.keys(vm).forEach(propKey => {
if (initialData[propKey]) {
expect(vm[propKey]).toBe(initialData[propKey]);
}
});
});
describe('Computed properties', () => {
it('should correctly set showUnapproveButton when the user can unapprove', () => {
expect(vm.showUnapproveButton).toBeTruthy();
vm.mr.state = 'merged';
expect(vm.showUnapproveButton).toBeFalsy();
});
it('should correctly set showUnapproveButton when the user can not unapprove', done => {
vm.userCanApprove = true;
Vue.nextTick(() => {
expect(vm.showUnapproveButton).toBe(false);
done();
});
});
});
describe('approvers list', () => {
const avatarUrl = `${TEST_HOST}/dummy.jpg`;
it('shows link to member avatar for for each approver', done => {
vm.approvedBy = [
{
user: {
username: 'Tanuki',
avatar_url: avatarUrl,
},
},
];
Vue.nextTick(() => {
const memberImage = document.querySelector('.approvers-list img');
expect(memberImage.src).toContain(avatarUrl);
done();
});
});
it('allows to add multiple approvers withoutd duplicate-key errors', done => {
vm.approvedBy = [
{
user: {
username: 'Tanuki',
avatar_url: avatarUrl,
},
},
{
user: {
username: 'Tanuki2',
avatar_url: avatarUrl,
},
},
];
Vue.nextTick(() => {
const approvers = document.querySelectorAll('.approvers-list img');
expect(approvers.length).toBe(2);
done();
});
});
});
});
ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js
View file @ b03dbecc
......@@ -6,6 +6,7 @@ import MRWidgetService from 'ee/vue_merge_request_widget/services/mr_widget_serv
import MRWidgetStore from 'ee/vue_merge_request_widget/stores/mr_widget_store';
import mountComponent from 'spec/helpers/vue_mount_component_helper';
import { TEST_HOST } from 'spec/test_constants';
import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
import state from 'ee/vue_shared/security_reports/store/state';
import mockData, {
......@@ -39,7 +40,6 @@ describe('ee merge request widget options', () => {
}
beforeEach(() => {
gon.features = { approvalRules: false };
delete mrWidgetOptions.extends.el; // Prevent component mounting
Component = Vue.extend(mrWidgetOptions);
......@@ -47,7 +47,6 @@ describe('ee merge request widget options', () => {
});
afterEach(() => {
gon.features = null;
vm.$destroy();
mock.restore();
......@@ -850,16 +849,22 @@ describe('ee merge request widget options', () => {
});
describe('data', () => {
it('passes approvals_path to service', () => {
const approvalsPath = `${TEST_HOST}/approvals/path`;
it('passes approval api paths to service', () => {
const paths = {
api_approvals_path: `${TEST_HOST}/api/approvals/path`,
api_approval_settings_path: `${TEST_HOST}/api/approval/settings/path`,
api_approve_path: `${TEST_HOST}/api/approve/path`,
api_unapprove_path: `${TEST_HOST}/api/unapprove/path`,
};
vm = mountComponent(Component, {
mrData: {
...mockData,
approvals_path: approvalsPath,
...paths,
},
});
expect(vm.service.approvalsPath).toEqual(approvalsPath);
expect(vm.service).toEqual(jasmine.objectContaining(convertObjectPropsToCamelCase(paths)));
});
});
});
ee/spec/models/approvable_for_rule_spec.rb deleted 100644 → 0
View file @ d7d420f1
# frozen_string_literal: true
require 'spec_helper'
# Based on approvable_spec.rb
describe ApprovableForRule do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:author) { merge_request.author }
describe '#approvers_overwritten?' do
subject { merge_request.approvers_overwritten? }
it 'returns false when merge request has no approvers' do
is_expected.to be false
end
it 'returns true when merge request has user approver' do
create(:approver, target: merge_request)
is_expected.to be true
end
it 'returns true when merge request has group approver' do
group = create(:group_with_members)
create(:approver_group, target: merge_request, group: group)
is_expected.to be true
end
end
describe '#can_approve?' do
subject { merge_request.can_approve?(user) }
it 'returns false if user is nil' do
expect(merge_request.can_approve?(nil)).to be false
end
it 'returns true when user is included in the approvers list' do
user = create(:approver, target: merge_request).user
expect(merge_request.can_approve?(user)).to be true
end
context 'when the user is the author' do
context 'and author is an approver' do
before do
create(:approver, target: merge_request, user: author)
end
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(merge_request.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(merge_request.can_approve?(author)).to be false
end
end
context 'and author is not an approver' do
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(merge_request.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(merge_request.can_approve?(author)).to be false
end
end
end
context 'when user is a committer' do
let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
before do
project.add_developer(user)
end
context 'and committer is an approver' do
before do
create(:approver, target: merge_request, user: user)
end
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(merge_request.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(merge_request.can_approve?(user)).to be false
end
end
context 'and committer is not an approver' do
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(merge_request.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(merge_request.can_approve?(user)).to be false
end
end
end
it 'returns false when user is unable to update the merge request' do
user = create(:user)
project.add_guest(user)
expect(merge_request.can_approve?(user)).to be false
end
context 'when approvals are required' do
before do
project.update!(approvals_before_merge: 1)
end
it 'returns true when approvals are still accepted and user still has not approved' do
user = create(:user)
project.add_developer(user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when there is still one approver missing' do
user = create(:user)
project.add_developer(user)
create(:approver, target: merge_request)
expect(merge_request.can_approve?(user)).to be false
end
end
end
end
ee/spec/models/approvable_spec.rb
View file @ b03dbecc
require 'spec_helper'
describe Approvable do
let(:merge_request) { create(:merge_request) }
subject(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:author) { merge_request.author }
before do
stub_feature_flags(approval_rules: false)
end
describe '#approval_feature_available?' do
let(:project) { create(:project) }
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
......@@ -27,140 +23,7 @@ describe Approvable do
end
end
describe '#approvers_overwritten?' do
subject { merge_request.approvers_overwritten? }
it 'returns false when merge request has no approvers' do
is_expected.to be false
end
it 'returns true when merge request has user approver' do
create(:approver, target: merge_request)
is_expected.to be true
end
it 'returns true when merge request has group approver' do
group = create(:group_with_members)
create(:approver_group, target: merge_request, group: group)
is_expected.to be true
end
end
describe '#can_approve?' do
subject { merge_request.can_approve?(user) }
it 'returns false if user is nil' do
expect(merge_request.can_approve?(nil)).to be false
end
it 'returns true when user is included in the approvers list' do
user = create(:approver, target: merge_request).user
expect(merge_request.can_approve?(user)).to be true
end
context 'when authors can approve' do
before do
project.update(merge_requests_author_approval: true)
end
context 'when the user is the author' do
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: author)
expect(merge_request.can_approve?(author)).to be true
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(author)).to be false
end
end
context 'when user is committer' do
let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
before do
project.add_developer(user)
end
context 'and committers can not approve' do
before do
project.update(merge_requests_disable_committers_approval: true)
end
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be false
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(user)).to be false
end
end
context 'and committers can approve' do
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(user)).to be false
end
end
end
end
context 'when authors cannot approve' do
before do
project.update(merge_requests_author_approval: false)
end
it 'returns false when user is the author' do
create(:approver, target: merge_request, user: author)
expect(merge_request.can_approve?(author)).to be false
end
it 'returns true when user is a committer' do
user = create(:user, email: merge_request.commits.without_merge_commits.first.committer_email)
project.add_developer(user)
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be true
end
end
it 'returns false when user is unable to update the merge request' do
user = create(:user)
project.add_guest(user)
expect(merge_request.can_approve?(user)).to be false
end
context 'when approvals are required' do
before do
project.update(approvals_before_merge: 1)
end
it 'returns true when approvals are still accepted and user still has not approved' do
user = create(:user)
project.add_developer(user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when there is still one approver missing' do
user = create(:user)
project.add_developer(user)
create(:approver, target: merge_request)
expect(merge_request.can_approve?(user)).to be false
end
end
described_class::FORWARDABLE_METHODS.each do |method|
it { is_expected.to delegate_method(method).to(:approval_state) }
end
end
ee/spec/models/approval_state_spec.rb
View file @ b03dbecc
......@@ -1174,8 +1174,83 @@ describe ApprovalState do
let(:developer) { create_project_member(:developer) }
let(:other_developer) { create_project_member(:developer) }
let(:reporter) { create_project_member(:reporter) }
let(:guest) { create_project_member(:guest) }
let(:stranger) { create(:user) }
context 'when the user is the author' do
context 'and author is an approver' do
before do
create(:approval_project_rule, project: project, users: [author])
end
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(subject.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(subject.can_approve?(author)).to be false
end
end
context 'and author is not an approver' do
it 'returns true when authors can approve' do
project.update!(merge_requests_author_approval: true)
expect(subject.can_approve?(author)).to be true
end
it 'returns false when authors cannot approve' do
project.update!(merge_requests_author_approval: false)
expect(subject.can_approve?(author)).to be false
end
end
end
context 'when user is a committer' do
let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
before do
project.add_developer(user)
end
context 'and committer is an approver' do
before do
create(:approval_project_rule, project: project, users: [user])
end
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(subject.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(subject.can_approve?(user)).to be false
end
end
context 'and committer is not an approver' do
it 'return true when committers can approve' do
project.update!(merge_requests_disable_committers_approval: false)
expect(subject.can_approve?(user)).to be true
end
it 'return false when committers cannot approve' do
project.update!(merge_requests_disable_committers_approval: true)
expect(subject.can_approve?(user)).to be false
end
end
end
context 'when there is one approver required' do
let!(:rule) { create_rule(approvals_required: 1) }
......@@ -1318,6 +1393,7 @@ describe ApprovalState do
expect(subject.can_approve?(author)).to be_falsey
expect(subject.can_approve?(reporter)).to be_falsey
expect(subject.can_approve?(guest)).to be_falsey
expect(subject.can_approve?(stranger)).to be_falsey
expect(subject.can_approve?(nil)).to be_falsey
end
......@@ -1347,6 +1423,7 @@ describe ApprovalState do
expect(subject.can_approve?(author)).to be_falsey
expect(subject.can_approve?(reporter)).to be_falsey
expect(subject.can_approve?(other_developer)).to be_truthy
expect(subject.can_approve?(guest)).to be_falsey
expect(subject.can_approve?(stranger)).to be_falsey
expect(subject.can_approve?(nil)).to be_falsey
end
......
ee/spec/models/merge_request_spec.rb
View file @ b03dbecc
This diff is collapsed.
ee/spec/models/project_spec.rb
View file @ b03dbecc
......@@ -1024,16 +1024,6 @@ describe Project do
expect(project.visible_regular_approval_rules).to contain_exactly(approval_rules.first)
end
end
context 'when approval rules are disabled' do
before do
stub_feature_flags(approval_rules: false)
end
it 'does not return any approval rules' do
expect(project.visible_regular_approval_rules).to be_empty
end
end
end
describe '#min_fallback_approvals' do
......@@ -1060,12 +1050,6 @@ describe Project do
expect(project.min_fallback_approvals).to eq(2)
end
it 'returns approvals before merge when code owner rules is disabled' do
stub_feature_flags(approval_rules: false)
expect(project.min_fallback_approvals).to eq(1)
end
end
end
......
ee/spec/models/visible_approvable_for_rule_spec.rb deleted 100644 → 0
View file @ d7d420f1
# frozen_string_literal: true
require 'spec_helper'
# Based on visible_approvable_spec.rb
describe VisibleApprovableForRule do
let(:resource) { create(:merge_request, source_project: project, target_project: project) }
let!(:project) { create(:project, :repository) }
let!(:user) { project.creator }
describe '#approvers_left' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
let!(:approver) { create(:approver, target: resource) }
subject { resource.approvers_left }
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new { subject }
expect { subject }.not_to exceed_query_limit(control)
end
it 'returns all approvers left' do
resource.approvals.create!(user: approver.user)
is_expected.to match_array(public_approver_group.users + private_approver_group.users)
end
end
describe '#overall_approvers' do
let(:approver) { create(:user) }
let(:code_owner) { build(:user) }
let!(:project_regular_rule) { create(:approval_project_rule, project: project, users: [approver]) }
let!(:code_owner_rule) { create(:code_owner_rule, merge_request: resource, users: [code_owner]) }
before do
project.add_developer(approver)
project.add_developer(code_owner)
end
subject { resource.overall_approvers }
it 'returns a list of all the project approvers' do
is_expected.to contain_exactly(approver, code_owner)
end
context 'when exclude_code_owners is true' do
subject { resource.overall_approvers(exclude_code_owners: true) }
it 'excludes code owners' do
is_expected.to contain_exactly(approver)
end
end
context 'when approvers are overwritten' do
let!(:merge_request_regular_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [create(:user)]) }
it 'returns the list of all the merge request level approvers' do
is_expected.to contain_exactly(*merge_request_regular_rule.users, code_owner)
end
end
context 'when author is an approver' do
let!(:approver) { resource.author }
it 'excludes author if author cannot approve' do
project.update(merge_requests_author_approval: false)
is_expected.not_to include(approver)
end
it 'includes author if author is able to approve' do
project.update(merge_requests_author_approval: true)
is_expected.to include(approver)
end
end
context 'when a committer is an approver' do
let!(:approver) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
it 'excludes the committer if committers cannot approve' do
project.update(merge_requests_disable_committers_approval: true)
is_expected.not_to include(approver)
end
it 'includes the committer if committers are able to approve' do
project.update(merge_requests_disable_committers_approval: false)
is_expected.to include(approver)
end
end
end
describe '#all_approvers_including_groups' do
let!(:group) { create(:group_with_members) }
let!(:approver_group) { create(:approver_group, target: resource, group: group) }
let!(:approver) { create(:approver, target: resource) }
subject { resource.all_approvers_including_groups }
it 'returns all approvers (groups and users)' do
is_expected.to match_array(approver_group.users + [approver.user])
end
end
describe '#authors_can_approve?' do
subject { resource.authors_can_approve? }
it 'returns false when merge_requests_author_approval flag is off' do
is_expected.to be false
end
it 'returns true when merge_requests_author_approval flag is turned on' do
project.update(merge_requests_author_approval: true)
is_expected.to be true
end
end
end
ee/spec/models/visible_approvable_spec.rb
View file @ b03dbecc
......@@ -5,16 +5,15 @@ describe VisibleApprovable do
let!(:project) { create(:project, :repository) }
let!(:user) { project.creator }
before do
stub_feature_flags(approval_rules: false)
end
describe '#approvers_left' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
let!(:approver) { create(:approver, target: resource) }
let!(:approver) { create(:user) }
let!(:rule) { create(:approval_project_rule, project: project, groups: [public_group, private_group], users: [approver])}
before do
project.add_developer(approver)
end
subject { resource.approvers_left }
......@@ -25,130 +24,88 @@ describe VisibleApprovable do
end
it 'returns all approvers left' do
resource.approvals.create!(user: approver.user)
resource.approvals.create!(user: approver)
is_expected.to match_array(public_approver_group.users + private_approver_group.users)
is_expected.to match_array(public_group.users + private_group.users)
end
end
describe '#overall_approvers' do
let!(:project_approver) { create(:approver, target: project) }
let(:approver) { create(:user) }
let(:code_owner) { build(:user) }
let!(:project_regular_rule) { create(:approval_project_rule, project: project, users: [approver]) }
let!(:code_owner_rule) { create(:code_owner_rule, merge_request: resource, users: [code_owner]) }
before do
allow(resource).to receive(:code_owners).and_return([code_owner])
project.add_developer(project_approver.user)
project.add_developer(approver)
project.add_developer(code_owner)
end
subject { resource.overall_approvers }
it 'returns a list of all the project approvers' do
is_expected.to contain_exactly(project_approver.user, code_owner)
is_expected.to contain_exactly(approver, code_owner)
end
context 'when exclude_code_owners is true' do
subject { resource.overall_approvers(exclude_code_owners: true) }
it 'excludes code owners' do
is_expected.to contain_exactly(project_approver.user)
is_expected.to contain_exactly(approver)
end
end
context 'when author is approver' do
let!(:author_approver) { create(:approver, target: project, user: resource.author) }
it 'excludes author if authors cannot approve' do
is_expected.not_to include(author_approver.user)
end
it 'includes author if authors are able to approve' do
project.update(merge_requests_author_approval: true)
is_expected.to include(author_approver.user)
end
end
context 'when committer is approver' do
let(:user) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
let!(:committer_approver) { create(:approver, target: project, user: user) }
before do
project.add_developer(user)
end
it 'excludes committer if committers cannot approve' do
project.update(merge_requests_disable_committers_approval: true)
is_expected.not_to include(committer_approver.user)
end
context 'when approvers are overwritten' do
let!(:merge_request_regular_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [create(:user)]) }
it 'includes committer if committers are able to approve' do
is_expected.to include(committer_approver.user)
it 'returns the list of all the merge request level approvers' do
is_expected.to contain_exactly(*merge_request_regular_rule.users, code_owner)
end
end
context 'when approvers are overwritten' do
let!(:approver) { create(:approver, target: resource) }
context 'when author is an approver' do
let!(:approver) { resource.author }
before do
project.add_developer(approver.user)
end
it 'excludes author if author cannot approve' do
project.update(merge_requests_author_approval: false)
it 'returns the list of all the merge request user approvers' do
is_expected.to contain_exactly(approver.user)
is_expected.not_to include(approver)
end
end
context 'when approver is no longer part of project' do
it 'excludes non-project members' do
project.team.find_member(project_approver.user).destroy!
it 'includes author if author is able to approve' do
project.update(merge_requests_author_approval: true)
is_expected.not_to include(project_approver.user)
is_expected.to include(approver)
end
end
end
describe '#overall_approver_groups' do
before do
group = create(:group_with_members)
create(:approver_group, target: project, group: group)
end
context 'when a committer is an approver' do
let!(:approver) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
subject { resource.overall_approver_groups }
it 'excludes the committer if committers cannot approve' do
project.update(merge_requests_disable_committers_approval: true)
it 'returns all the project approver groups' do
is_expected.to match_array(project.approver_groups)
end
is_expected.not_to include(approver)
end
context 'when group approvers are overwritten' do
it 'returns all the merge request approver groups' do
group = create(:group_with_members)
create(:approver_group, target: resource, group: group)
it 'includes the committer if committers are able to approve' do
project.update(merge_requests_disable_committers_approval: false)
is_expected.to match_array(resource.approver_groups)
is_expected.to include(approver)
end
end
end
describe '#all_approvers_including_groups' do
let!(:group) { create(:group_with_members) }
let!(:approver_group) { create(:approver_group, target: resource, group: group) }
let!(:approver) { create(:approver, target: resource) }
before do
project.add_developer(approver.user)
end
let!(:approver) { create(:user) }
let!(:rule) { create(:approval_project_rule, project: project, groups: [group], users: [approver]) }
subject { resource.all_approvers_including_groups }
it 'only queries once' do
expect(resource).to receive(:overall_approvers).and_call_original.once
3.times { subject }
end
it 'returns all approvers (groups and users)' do
is_expected.to match_array(approver_group.users + [approver.user])
is_expected.to match_array(group.users + [approver])
end
end
......@@ -168,8 +125,9 @@ describe VisibleApprovable do
describe '#reset_approval_cache!' do
before do
approver = create(:approver, target: resource)
project.add_developer(approver.user)
approver = create(:user)
project.add_developer(approver)
create(:approval_project_rule, project: project, users: [approver])
end
subject { resource.reset_approval_cache! }
......
ee/spec/presenters/merge_request_presenter_spec.rb
View file @ b03dbecc
......@@ -84,110 +84,36 @@ describe MergeRequestPresenter do
describe '#approvers_left' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
let!(:approver) { create(:approver, target: merge_request) }
let!(:approver) { create(:user) }
let!(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: [approver], groups: [private_group, public_group]) }
before do
stub_feature_flags(approval_rules: false)
merge_request.approvals.create!(user: approver.user)
end
subject { described_class.new(merge_request, current_user: user).approvers_left }
it { is_expected.to match_array(public_approver_group.users) }
context 'when user has access to private group' do
before do
private_group.add_user(user, Gitlab::Access::DEVELOPER)
end
it do
approvers = public_approver_group.users + private_approver_group.users - [user]
is_expected.to match_array(approvers)
end
end
end
describe '#approvers_left with approval_rule enabled' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
let!(:approver) { create(:approver, target: merge_request) }
before do
merge_request.approvals.create!(user: approver.user)
merge_request.approvals.create!(user: approver)
end
subject { described_class.new(merge_request, current_user: user).approvers_left }
it 'contains all approvers' do
approvers = public_approver_group.users + private_approver_group.users - [user]
approvers = public_group.users + private_group.users - [user]
is_expected.to match_array(approvers)
end
end
describe '#overall_approver_groups' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
subject { described_class.new(merge_request, current_user: user).overall_approver_groups }
it { is_expected.to match_array([public_approver_group]) }
context 'when user has access to private group' do
before do
private_group.add_user(user, Gitlab::Access::DEVELOPER)
end
it { is_expected.to match_array([public_approver_group, private_approver_group]) }
end
end
describe '#all_approvers_including_groups' do
describe '#all_approvers_including_groups with approval_rule enabled' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
let!(:approver) { create(:approver, target: merge_request) }
subject { described_class.new(merge_request, current_user: user).all_approvers_including_groups }
let!(:approver) { create(:user) }
let!(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: [approver], groups: [private_group, public_group]) }
before do
stub_feature_flags(approval_rules: false)
end
it { is_expected.to match_array(public_approver_group.users + [approver.user]) }
context 'when user has access to private group' do
before do
private_group.add_user(user, Gitlab::Access::DEVELOPER)
end
it do
approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]
is_expected.to match_array(approvers)
end
project.add_developer(approver)
end
end
describe '#all_approvers_including_groups with approval_rule enabled' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
let!(:approver) { create(:approver, target: merge_request) }
subject { described_class.new(merge_request, current_user: user).all_approvers_including_groups }
it do
approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]
approvers = [public_group.users, private_group.users, approver].flatten - [user]
is_expected.to match_array(approvers)
end
......
ee/spec/requests/api/project_approvals_spec.rb
View file @ b03dbecc
......@@ -10,15 +10,10 @@ describe API::ProjectApprovals do
let(:url) { "/projects/#{project.id}/approvals" }
before do
stub_feature_flags(approval_rules: false)
end
describe 'GET /projects/:id/approvers' do
describe 'GET /projects/:id/approvals' do
context 'when the request is correct' do
before do
project.approvers.create(user: approver)
project.approver_groups.create(group: group)
create(:approval_project_rule, project: project, users: [approver], groups: [group])
get api(url, user)
end
......@@ -34,7 +29,7 @@ describe API::ProjectApprovals do
it 'only shows approver groups that are visible to the user' do
private_group = create(:group, :private)
project.approver_groups.create(group: private_group)
create(:approval_project_rule, project: project, users: [approver], groups: [private_group])
get api(url, user)
......@@ -43,7 +38,7 @@ describe API::ProjectApprovals do
end
end
describe 'POST /projects/:id/approvers' do
describe 'POST /projects/:id/approvals' do
shared_examples_for 'a user with access' do
context 'when missing parameters' do
it 'returns 400 status' do
......
ee/spec/services/ee/merge_requests/refresh_service_spec.rb
View file @ b03dbecc
......@@ -73,72 +73,7 @@ describe MergeRequests::RefreshService do
end
end
context 'when code owners enabled, with approval_rule disabled' do
let(:old_owners) { [] }
let(:new_owners) { [] }
let(:relevant_merge_requests) { [merge_request, another_merge_request] }
before do
stub_feature_flags(approval_rules: false)
relevant_merge_requests.each do |merge_request|
expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request, merge_request_diff: anything).and_wrap_original do |m, *args|
expect(args.last[:merge_request_diff]).to eq(merge_request.merge_request_diffs.order(id: :desc).offset(1).first)
old_owners
end
end
[forked_merge_request].each do |merge_request|
expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request, anything)
end
expect(service.todo_service).not_to receive(:add_merge_request_approvers)
expect(service.notification_service).not_to receive(:add_merge_request_approvers)
end
context 'merge request has overwritten approvers' do
context 'when new owners are being added' do
let(:new_owners) { [owner] }
it 'does not create Approver' do
expect { subject }.not_to change { Approver.count }
rule = merge_request.approval_rules.code_owner.first
expect(rule.users).to eq(new_owners)
end
end
end
context 'merge request has default approvers' do
let(:existing_approver) { create(:user) }
before do
create(:approver, target: merge_request, user: existing_approver)
end
context 'when new owners are being added' do
let(:new_owners) { [owner] }
it 'creates Approver' do
expect { subject }.to change { Approver.count }.by(1)
new_approver = merge_request.approvers.last
expect(merge_request.approvers.first.user).to eq(existing_approver)
expect(new_approver.user).to eq(owner)
expect(new_approver.created_at).to be_present
expect(new_approver.updated_at).to be_present
rule = merge_request.approval_rules.code_owner.first
expect(rule.users).to eq(new_owners)
end
end
end
end
context 'when code owners enabled, with approval_rule enabled' do
context 'when code owners enabled' do
let(:relevant_merge_requests) { [merge_request, another_merge_request] }
it 'refreshes the code owner rules for all relevant merge requests' do
......
ee/spec/services/ee/notification_service_spec.rb
View file @ b03dbecc
......@@ -491,8 +491,6 @@ describe EE::NotificationService, :mailer do
end
before do
stub_feature_flags(approval_rules: false)
project.add_maintainer(merge_request.author)
merge_request.assignees.each { |assignee| project.add_maintainer(assignee) }
build_team(merge_request.target_project)
......@@ -511,10 +509,10 @@ describe EE::NotificationService, :mailer do
context 'when the target project has approvers set' do
let(:project_approvers) { create_list(:user, 3) }
let!(:rule) { create(:approval_project_rule, project: project, users: project_approvers, approvals_required: 1 )}
before do
merge_request.target_project.update(approvals_before_merge: 1)
project_approvers.each { |approver| create(:approver, user: approver, target: merge_request.target_project) }
reset_delivered_emails!
end
......@@ -525,7 +523,7 @@ describe EE::NotificationService, :mailer do
end
it 'does not email the approvers when approval is not necessary' do
merge_request.target_project.update(approvals_before_merge: 0)
rule.update(approvals_required: 0)
notification.new_merge_request(merge_request, @u_disabled)
project_approvers.each { |approver| should_not_email(approver) }
......@@ -533,9 +531,9 @@ describe EE::NotificationService, :mailer do
context 'when the merge request has approvers set' do
let(:mr_approvers) { create_list(:user, 3) }
let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: mr_approvers, approvals_required: 1 )}
before do
mr_approvers.each { |approver| create(:approver, user: approver, target: merge_request) }
reset_delivered_emails!
end
......
ee/spec/services/merge_requests/remove_approval_service_spec.rb
View file @ b03dbecc
......@@ -8,10 +8,6 @@ describe MergeRequests::RemoveApprovalService do
subject(:service) { described_class.new(project, user) }
before do
stub_feature_flags(approval_rules: false)
end
def execute!
service.execute(merge_request)
end
......
ee/spec/services/merge_requests/update_service_spec.rb
View file @ b03dbecc
......@@ -60,58 +60,27 @@ describe MergeRequests::UpdateService, :mailer do
context 'when approvals_before_merge changes' do
using RSpec::Parameterized::TableSyntax
context 'when approval_rules is disabled' do
before do
stub_feature_flags(approval_rules: false)
end
where(:project_value, :mr_before_value, :mr_after_value, :result) do
3 | 4 | 5 | 5
3 | 4 | nil | 3
3 | nil | 5 | 5
end
with_them do
let(:project) { create(:project, :repository, approvals_before_merge: project_value) }
it "updates approval_rules' approvals_required" do
merge_request.update(approvals_before_merge: mr_before_value)
rule = create(:approval_merge_request_rule, merge_request: merge_request)
update_merge_request(approvals_before_merge: mr_after_value)
expect(rule.reload.approvals_required).to eq(result)
end
end
where(:project_value, :mr_before_value, :mr_after_value, :result) do
3 | 4 | 5 | 5
3 | 4 | nil | 3
3 | nil | 5 | 5
end
context 'when approval_rules is enabled' do
where(:project_value, :mr_before_value, :mr_after_value, :result) do
3 | 4 | 5 | 5
3 | 4 | nil | 3
3 | nil | 5 | 5
end
with_them do
let(:project) { create(:project, :repository, approvals_before_merge: project_value) }
with_them do
let(:project) { create(:project, :repository, approvals_before_merge: project_value) }
it "does not update" do
merge_request.update(approvals_before_merge: mr_before_value)
rule = create(:approval_merge_request_rule, merge_request: merge_request)
it "does not update" do
merge_request.update(approvals_before_merge: mr_before_value)
rule = create(:approval_merge_request_rule, merge_request: merge_request)
update_merge_request(approvals_before_merge: mr_after_value)
update_merge_request(approvals_before_merge: mr_after_value)
expect(rule.reload.approvals_required).to eq(0)
end
expect(rule.reload.approvals_required).to eq(0)
end
end
end
context 'merge' do
before do
stub_feature_flags(approval_rules: false)
end
let(:opts) { { merge: merge_request.diff_head_sha } }
context 'when not approved' do
......@@ -150,8 +119,6 @@ describe MergeRequests::UpdateService, :mailer do
let(:new_approver) { create(:user) }
before do
stub_feature_flags(approval_rules: false)
project.add_developer(existing_approver)
project.add_developer(removed_approver)
project.add_developer(new_approver)
......
ee/spec/services/projects/update_service_spec.rb
View file @ b03dbecc
......@@ -177,40 +177,6 @@ describe Projects::UpdateService, '#execute' do
end
end
context 'with approval_rules' do
context 'when approval_rules is disabled' do
it "updates approval_rules' approvals_required" do
stub_feature_flags(approval_rules: false)
rule = create(:approval_project_rule, project: project)
update_project(project, user, approvals_before_merge: 42)
expect(rule.reload.approvals_required).to eq(42)
end
end
context 'when approval_rules is enabled' do
it 'does not update' do
rule = create(:approval_project_rule, project: project)
update_project(project, user, approvals_before_merge: 42)
expect(rule.reload.approvals_required).to eq(0)
end
end
context 'when approval_rule feature is enabled' do
it "does not update approval_rules' approvals_required" do
rule = create(:approval_project_rule, project: project)
expect do
update_project(project, user, approvals_before_merge: 42)
end.not_to change { rule.reload.approvals_required }
end
end
end
describe 'repository_storage' do
let(:admin_user) { create(:user, admin: true) }
let(:user) { create(:user) }
......
ee/spec/support/helpers/feature_approval_helper.rb 0 → 100644
View file @ b03dbecc
# frozen_string_literal: true
module FeatureApprovalHelper
def open_modal
page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()"
within(config_selector) do
click_on('Edit')
end
end
def open_approver_select
within(modal_selector) do
find('.select2-input').click
end
wait_for_requests
end
def close_approver_select
within(modal_selector) do
find('.select2-input').send_keys :escape
end
end
def remove_approver(name)
el = page.find("#{modal_selector} .content-list li", text: /#{name}/i)
el.find('button').click
end
def expect_avatar(container, users)
users = Array(users)
members = container.all('.js-members img.avatar').map do |member|
member['alt']
end
users.each do |user|
expect(members).to include(user.name)
end
expect(members.size).to eq(users.size)
end
end
ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb
View file @ b03dbecc
......@@ -9,23 +9,16 @@ describe 'shared/issuable/_approvals.html.haml' do
let(:form) { double('form') }
before do
stub_feature_flags(approval_rules: false)
allow(view).to receive(:can?).and_return(true)
allow(view).to receive(:current_user).and_return(user)
allow(form).to receive(:label)
allow(form).to receive(:number_field)
allow(MergeRequestApproverPresenter).to receive(:new).and_return(approver_presenter)
assign(:project, project)
assign(:target_project, project)
end
context 'has no approvers' do
it 'shows empty approvers list' do
render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
expect(rendered).to have_text('There are no approvers')
end
context 'can override approvers' do
before do
render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
......@@ -34,14 +27,6 @@ describe 'shared/issuable/_approvals.html.haml' do
it 'shows suggested approvers' do
expect(rendered).to have_css('.suggested-approvers')
end
it 'shows select approvers field' do
expect(rendered).to have_css('#merge_request_approver_ids', visible: false)
end
it 'shows select approver groups field' do
expect(rendered).to have_css('#merge_request_approver_group_ids', visible: false)
end
end
context 'can not override approvers' do
......@@ -82,14 +67,6 @@ describe 'shared/issuable/_approvals.html.haml' do
expect(rendered).to have_text(approver_group[:name])
end
context 'can override approvers' do
it 'shows remove button for approver' do
render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
expect(rendered).to have_css('.btn-remove')
end
end
context 'can not override approvers' do
it 'hides remove button' do
allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
......
locale/gitlab.pot
View file @ b03dbecc
......@@ -693,9 +693,6 @@ msgstr ""
msgid "Add an SSH key"
msgstr ""
msgid "Add approver(s)"
msgstr ""
msgid "Add approvers"
msgstr ""
......@@ -750,9 +747,6 @@ msgstr ""
msgid "Add user(s) to the group:"
msgstr ""
msgid "Add users or groups who are allowed to approve every merge request"
msgstr ""
msgid "Add users to group"
msgstr ""
......@@ -1082,9 +1076,6 @@ msgstr ""
msgid "An error occurred when updating the issue weight"
msgstr ""
msgid "An error occurred while adding approver"
msgstr ""
msgid "An error occurred while deleting the approvers group"
msgstr ""
......@@ -1193,9 +1184,6 @@ msgstr ""
msgid "An error occurred while parsing recent searches"
msgstr ""
msgid "An error occurred while removing approver"
msgstr ""
msgid "An error occurred while removing epics."
msgstr ""
......@@ -1414,12 +1402,6 @@ msgstr ""
msgid "Approvals"
msgstr ""
msgid "Approvals required"
msgstr ""
msgid "Approvers"
msgstr ""
msgid "Apr"
msgstr ""
......@@ -1483,18 +1465,6 @@ msgstr ""
msgid "Are you sure you want to remove %{group_name}?"
msgstr ""
msgid "Are you sure you want to remove approver %{name}"
msgstr ""
msgid "Are you sure you want to remove approver %{name}?"
msgstr ""
msgid "Are you sure you want to remove group %{name}"
msgstr ""
msgid "Are you sure you want to remove group %{name}?"
msgstr ""
msgid "Are you sure you want to remove the attachment?"
msgstr ""
......@@ -10778,9 +10748,6 @@ msgstr ""
msgid "Remove all or specific label(s)"
msgstr ""
msgid "Remove approver"
msgstr ""
msgid "Remove approvers"
msgstr ""
......@@ -11357,9 +11324,6 @@ msgstr ""
msgid "Search for projects, issues, etc."
msgstr ""
msgid "Search for users or groups"
msgstr ""
msgid "Search forks"
msgstr ""
......@@ -11717,9 +11681,6 @@ msgstr ""
msgid "Set notification email for abuse reports."
msgstr ""
msgid "Set number of approvers required before open merge requests can be merged"
msgstr ""
msgid "Set requirements for a user to sign-in. Enable mandatory two-factor authentication."
msgstr ""
......@@ -13101,9 +13062,6 @@ msgstr ""
msgid "There are no SSH keys with access to your account."
msgstr ""
msgid "There are no approvers"
msgstr ""
msgid "There are no archived projects yet"
msgstr ""
......@@ -13395,12 +13353,6 @@ msgstr ""
msgid "This merge request is locked."
msgstr ""
msgid "This merge request must be approved by members of these groups. You can override the project settings by setting your own list of approvers."
msgstr ""
msgid "This merge request must be approved by these users. You can override the project settings by setting your own list of approvers."
msgstr ""
msgid "This namespace has already been taken! Please choose another one."
msgstr ""
......@@ -15946,9 +15898,6 @@ msgstr ""
msgid "mrWidget|%{metricsLinkStart} Memory %{metricsLinkEnd} usage is %{emphasisStart} unchanged %{emphasisEnd} at %{memoryFrom}MB"
msgstr ""
msgid "mrWidget|Add approval"
msgstr ""
msgid "mrWidget|Allows commits from members who can merge to the target branch"
msgstr ""
......@@ -16042,15 +15991,9 @@ msgstr ""
msgid "mrWidget|Merge locally"
msgstr ""
msgid "mrWidget|Merge request approved"
msgstr ""
msgid "mrWidget|Merge request approved."
msgstr ""
msgid "mrWidget|Merge request approved; you can approve additionally"
msgstr ""
msgid "mrWidget|Merged by"
msgstr ""
......@@ -16081,22 +16024,9 @@ msgstr ""
msgid "mrWidget|Refreshing now"
msgstr ""
msgid "mrWidget|Remove your approval"
msgstr ""
msgid "mrWidget|Request to merge"
msgstr ""
msgid "mrWidget|Requires 1 more approval"
msgid_plural "mrWidget|Requires %d more approvals"
msgstr[0] ""
msgstr[1] ""
msgid "mrWidget|Requires 1 more approval by"
msgid_plural "mrWidget|Requires %d more approvals by"
msgstr[0] ""
msgstr[1] ""
msgid "mrWidget|Resolve conflicts"
msgstr ""
......
qa/qa/ee/page/merge_request/show.rb
View file @ b03dbecc
......@@ -32,10 +32,6 @@ module QA
element :review_preview_toggle
end
view 'ee/app/views/shared/issuable/_approvals_single_rule.html.haml' do
element :approver_list
end
def start_review
click_element :start_review
end
......
spec/features/merge_request/user_creates_merge_request_spec.rb
View file @ b03dbecc
......@@ -8,8 +8,6 @@ describe "User creates a merge request", :js do
let(:user) { create(:user) }
before do
stub_feature_flags(approval_rules: false)
project.add_maintainer(user)
sign_in(user)
end
......
spec/features/projects/settings/forked_project_settings_spec.rb
View file @ b03dbecc
......@@ -7,7 +7,6 @@ describe 'Projects > Settings > For a forked project', :js do
let(:forked_project) { fork_project(original_project, user) }
before do
stub_feature_flags(approval_rules: false)
original_project.add_maintainer(user)
forked_project.add_maintainer(user)
sign_in(user)
......
spec/features/projects_spec.rb
View file @ b03dbecc
......@@ -6,7 +6,6 @@ describe 'Project' do
before do
stub_feature_flags(vue_file_list: false)
stub_feature_flags(approval_rules: false)
end
describe 'creating from template' do
......
spec/javascripts/vue_mr_widget/mr_widget_options_spec.js
View file @ b03dbecc
......@@ -21,7 +21,6 @@ describe('mrWidgetOptions', () => {
const COLLABORATION_MESSAGE = 'Allows commits from members who can merge to the target branch';
beforeEach(() => {
gon.features = { approvalRules: false };
// Prevent component mounting
delete mrWidgetOptions.el;
......@@ -32,7 +31,6 @@ describe('mrWidgetOptions', () => {
});
afterEach(() => {
gon.features = null;
vm.$destroy();
});
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7