Commit c3390359 authored by Robert Hunt's avatar Robert Hunt Committed by Natalia Tepluhina

Restructured approvals creation/updating form

- Removed unnecessary containers
- Updating descriptions to use the label classes and match GitLab UI
- Moved inputs around
- Updated buttons to new variants
- Rewrote the branches_select to use GlDropdown
- Fixed console warnings
parent 0d347294
......@@ -1109,3 +1109,11 @@ header.header-content .dropdown-menu.frequent-items-dropdown-menu {
width: $gl-dropdown-width-wide;
}
}
// This class won't be needed once we can add a prop for this in the GitLab UI component
// https://gitlab.com/gitlab-org/gitlab-ui/-/issues/966
.gl-new-dropdown.gl-dropdown-menu-full-width {
.dropdown-menu {
width: 100%;
}
}
......@@ -146,10 +146,10 @@ To enable this access:
based on the Reporter role.
1. Navigate to your project's **Settings > General**, and in the
**Merge request approvals** section, click **Expand**.
1. [Add the group](../../group/index.md#create-a-group) to the permission list
for the protected branch.
1. Select **Add approval rule** or **Update approval rule**.
1. [Add the group](../../group/index.md#create-a-group) to the permission list.
![Update approval rule](img/update_approval_rule_v13_4.png)
![Update approval rule](img/update_approval_rule_v13_10.png)
#### Adding / editing a default approval rule
......@@ -240,7 +240,7 @@ the **Target branch** dropdown.
Alternatively, you can select a very specific protected branch from the **Target branch** dropdown:
![Scoped to protected branch](img/scoped_to_protected_branch_v12_8.png)
![Scoped to protected branch](img/scoped_to_protected_branch_v13_10.png)
To enable this configuration, see [Code Owner’s approvals for protected branches](../protected_branches.md#protected-branches-approval-by-code-owners).
......
<script>
import $ from 'jquery';
import 'select2/select2';
import { GlDropdown, GlDropdownItem, GlSearchBoxByType } from '@gitlab/ui';
import { debounce } from 'lodash';
import Api from 'ee/api';
import { loadCSSFile } from '~/lib/utils/css_utils';
import { __ } from '~/locale';
const anyBranch = {
id: null,
name: __('Any branch'),
};
function formatSelection(object) {
return `<span>${object.name}</span>`;
}
function formatResult(result) {
const isAnyBranch = result.id ? `monospace` : '';
return `
<span class="result-name ${isAnyBranch}">${result.name}</span>
`;
}
import { BRANCH_FETCH_DELAY, ANY_BRANCH } from '../constants';
export default {
components: {
GlDropdown,
GlDropdownItem,
GlSearchBoxByType,
},
props: {
projectId: {
type: String,
......@@ -39,87 +27,85 @@ export default {
default: false,
},
},
watch: {
value(val) {
if (val.length > 0) {
this.clear();
}
data() {
return {
branches: [],
initialLoading: false,
searching: false,
searchTerm: '',
selected: this.initRule?.protectedBranches[0] || ANY_BRANCH,
};
},
computed: {
dropdownClass() {
return {
'gl-w-full': true,
'gl-dropdown-menu-full-width': true,
'is-invalid': this.isInvalid,
};
},
isInvalid(val) {
const $container = this.$input.select2('container');
$container.toggleClass('is-invalid', val);
dropdownText() {
return this.selected.name;
},
},
mounted() {
const $modal = $('#project-settings-approvals-create-modal .modal-content');
this.$input = $(this.$refs.input);
loadCSSFile(gon.select2_css_path)
this.initialLoading = true;
this.fetchBranches()
.then(() => {
this.$input
.select2({
minimumInputLength: 0,
multiple: false,
closeOnSelect: false,
formatResult,
formatSelection,
initSelection: (element, callback) => this.initialOption(element, callback),
query: debounce(({ term, callback }) => {
// eslint-disable-next-line promise/no-nesting
this.fetchBranches(term)
.then(callback)
.catch(() => {});
}, 250),
id: ({ type, id }) => `${type}${id}`,
})
.on('change', (e) => this.onChange(e))
.on('select2-open', () => {
// https://stackoverflow.com/questions/18487056/select2-doesnt-work-when-embedded-in-a-bootstrap-modal
// Ensure search feature works in modal
// (known issue with our current select2 version, solved in version 4 with "dropdownParent")
$modal.removeAttr('tabindex', '-1');
})
.on('select2-close', () => {
$modal.attr('tabindex', '-1');
});
this.initialLoading = false;
})
.catch(() => {});
},
beforeDestroy() {
this.$input.select2('destroy');
},
methods: {
fetchBranches(term) {
async fetchBranches(term) {
this.searching = true;
const excludeAnyBranch = term && !term.toLowerCase().includes('any');
return Api.projectProtectedBranches(this.projectId, term).then((results) => ({
results: excludeAnyBranch ? results : [anyBranch, ...results],
}));
},
initialOption(element, callback) {
let currentBranch = anyBranch;
if (this.initRule?.protectedBranches.length) {
const { name, id } = this.initRule.protectedBranches[0];
if (id) {
currentBranch = { name, id };
this.selectedId = id;
}
}
const branches = await Api.projectProtectedBranches(this.projectId, term);
return callback(currentBranch);
this.branches = excludeAnyBranch ? branches : [ANY_BRANCH, ...branches];
this.searching = false;
},
search: debounce(function debouncedSearch() {
this.fetchBranches(this.searchTerm);
}, BRANCH_FETCH_DELAY),
isSelectedBranch(id) {
return this.selected.id === id;
},
onChange() {
const value = this.$input.select2('data');
this.$emit('input', value.id);
onSelect(branch) {
this.selected = branch;
this.$emit('input', branch.id);
},
clear() {
this.$input.select2('data', []);
branchNameClass(id) {
return {
monospace: id !== null,
};
},
},
i18n: {
header: __('Select branch'),
},
};
</script>
<template>
<input ref="input" name="protected_branch_ids" type="hidden" />
<gl-dropdown
:class="dropdownClass"
:text="dropdownText"
:loading="initialLoading"
:header-text="$options.i18n.header"
>
<template #header>
<gl-search-box-by-type v-model="searchTerm" :is-loading="searching" @input="search" />
</template>
<gl-dropdown-item
v-for="branch in branches"
:key="branch.id"
:is-check-item="true"
:is-checked="isSelectedBranch(branch.id)"
@click="onSelect(branch)"
>
<span :class="branchNameClass(branch.id)">{{ branch.name }}</span>
</gl-dropdown-item>
</gl-dropdown>
</template>
......@@ -60,7 +60,9 @@ export default {
</p>
<div class="gl-ml-auto">
<gl-button name="cancel" @click="cancel">{{ __('Cancel') }}</gl-button>
<gl-button name="ok" variant="success" @click="ok">{{ title }}</gl-button>
<gl-button name="ok" category="primary" variant="confirm" @click="ok">{{
title
}}</gl-button>
</div>
</section>
</template>
......
......@@ -32,12 +32,21 @@ export default {
defaultRuleName() {
return this.rule?.defaultRuleName;
},
primaryActionProps() {
return {
text: this.title,
attributes: [{ variant: 'confirm' }],
};
},
},
methods: {
submit() {
this.$refs.form.submit();
},
},
cancelActionProps: {
text: __('Cancel'),
},
};
</script>
......@@ -46,9 +55,8 @@ export default {
modal-module="createModal"
:modal-id="modalId"
:title="title"
:ok-title="title"
ok-variant="success"
:cancel-title="__('Cancel')"
:action-primary="primaryActionProps"
:action-cancel="$options.cancelActionProps"
size="sm"
@ok.prevent="submit"
>
......
......@@ -305,70 +305,59 @@ export default {
<template>
<form novalidate @submit.prevent.stop="submit">
<div class="row">
<div v-if="isNameVisible" class="form-group col-sm-6">
<label class="label-wrapper">
<span class="mb-2 bold inline">{{ s__('ApprovalRule|Rule name') }}</span>
<input
v-model="name"
:class="{ 'is-invalid': validation.name }"
:disabled="isNameDisabled"
class="form-control"
name="name"
type="text"
data-qa-selector="rule_name_field"
/>
<span class="invalid-feedback">{{ validation.name }}</span>
<span class="text-secondary">{{ s__('ApprovalRule|e.g. QA, Security, etc.') }}</span>
</label>
</div>
<div class="form-group col-sm-6">
<label class="label-wrapper">
<span class="mb-2 bold inline">{{ s__('ApprovalRule|Approvals required') }}</span>
<input
v-model.number="approvalsRequired"
:class="{ 'is-invalid': validation.approvalsRequired }"
class="form-control mw-6em"
name="approvals_required"
type="number"
:min="minApprovalsRequired"
data-qa-selector="approvals_required_field"
/>
<span class="invalid-feedback">{{ validation.approvalsRequired }}</span>
</label>
</div>
<div v-if="isNameVisible" class="form-group gl-form-group">
<label class="col-form-label">{{ s__('ApprovalRule|Rule name') }}</label>
<input
v-model="name"
:class="{ 'is-invalid': validation.name }"
:disabled="isNameDisabled"
class="gl-form-input form-control"
name="name"
type="text"
data-qa-selector="rule_name_field"
/>
<span class="invalid-feedback">{{ validation.name }}</span>
<small class="form-text text-gl-muted">
{{ s__('ApprovalRule|Examples: QA, Security.') }}
</small>
</div>
<div v-if="showProtectedBranch" class="form-group">
<label class="label-bold">{{ s__('ApprovalRule|Target branch') }}</label>
<div class="d-flex align-items-start">
<div class="w-100">
<branches-select
v-model="branchesToAdd"
:project-id="settings.projectId"
:is-invalid="!!validation.branches"
:init-rule="rule"
/>
<div class="invalid-feedback">{{ validation.branches }}</div>
</div>
</div>
<p class="text-muted">
<div v-if="showProtectedBranch" class="form-group gl-form-group">
<label class="col-form-label">{{ s__('ApprovalRule|Target branch') }}</label>
<branches-select
v-model="branchesToAdd"
:project-id="settings.projectId"
:is-invalid="Boolean(validation.branches)"
:init-rule="rule"
/>
<span class="invalid-feedback">{{ validation.branches }}</span>
<small class="form-text text-gl-muted">
{{ __('Apply this approval rule to any branch or a specific protected branch.') }}
</p>
</small>
</div>
<div class="form-group">
<label class="label-bold">{{ s__('ApprovalRule|Approvers') }}</label>
<div class="d-flex align-items-start">
<div class="w-100" data-qa-selector="member_select_field">
<approvers-select
v-model="approversToAdd"
:project-id="settings.projectId"
:skip-user-ids="userIds"
:skip-group-ids="groupIds"
:is-invalid="!!validation.approvers"
/>
<div class="invalid-feedback">{{ validation.approvers }}</div>
</div>
</div>
<div class="form-group gl-form-group">
<label class="col-form-label">{{ s__('ApprovalRule|Approvals required') }}</label>
<input
v-model.number="approvalsRequired"
:class="{ 'is-invalid': validation.approvalsRequired }"
class="gl-form-input form-control mw-6em"
name="approvals_required"
type="number"
:min="minApprovalsRequired"
data-qa-selector="approvals_required_field"
/>
<span class="invalid-feedback">{{ validation.approvalsRequired }}</span>
</div>
<div class="form-group gl-form-group">
<label class="col-form-label">{{ s__('ApprovalRule|Add approvers') }}</label>
<approvers-select
v-model="approversToAdd"
:project-id="settings.projectId"
:skip-user-ids="userIds"
:skip-group-ids="groupIds"
:is-invalid="Boolean(validation.approvers)"
data-qa-selector="member_select_field"
/>
<span class="invalid-feedback">{{ validation.approvers }}</span>
</div>
<div class="bordered-box overflow-auto h-12em">
<approvers-list v-model="approvers" />
......
......@@ -48,6 +48,11 @@ export default {
return this.rulesWithTooltips[this.name]?.linkPath;
},
},
methods: {
popoverTarget() {
return this.$refs.helpIcon?.$el;
},
},
};
</script>
......@@ -62,7 +67,7 @@ export default {
:size="14"
class="author-link suggestion-help-hover"
/>
<gl-popover :target="() => $refs.helpIcon.$el" placement="top" triggers="hover focus">
<gl-popover :target="popoverTarget" placement="top" triggers="hover focus">
<template #title>{{ __('Who can approve?') }}</template>
<p>{{ description }}</p>
<gl-link v-if="linkPath" :href="linkPath" class="gl-font-sm" target="_blank">{{
......
......@@ -4,6 +4,12 @@ export const TYPE_USER = 'user';
export const TYPE_GROUP = 'group';
export const TYPE_HIDDEN_GROUPS = 'hidden_groups';
export const BRANCH_FETCH_DELAY = 250;
export const ANY_BRANCH = {
id: null,
name: __('Any branch'),
};
export const RULE_TYPE_FALLBACK = 'fallback';
export const RULE_TYPE_REGULAR = 'regular';
export const RULE_TYPE_REPORT_APPROVER = 'report_approver';
......
---
title: Restructure layout of Add/Update approval rule form
merge_request: 55181
author:
type: changed
......@@ -62,7 +62,7 @@ RSpec.describe 'Merge request > User edits MR with approval rules', :js do
click_button "Add approval rule"
fill_in "Rule name", with: rule_name
fill_in "name", with: rule_name
add_approval_rule_member('user', approver.name)
......
......@@ -234,9 +234,9 @@ RSpec.describe 'Merge request > User sets approvers', :js do
find('.merge-request').click_on 'Edit'
open_modal
expect(page).to have_field 'Approvals required', exact: 2
expect(page).to have_field 'approvals_required', exact: 2
fill_in 'Approvals required', with: '3'
fill_in 'approvals_required', with: '3'
click_button 'Update approval rule'
click_on('Save changes')
......@@ -251,7 +251,7 @@ RSpec.describe 'Merge request > User sets approvers', :js do
open_modal
expect(page).to have_field 'Approvals required', exact: 3
expect(page).to have_field 'approvals_required', exact: 3
end
end
end
......
import { shallowMount, createLocalVue } from '@vue/test-utils';
import $ from 'jquery';
import { GlDropdown, GlDropdownItem, GlSearchBoxByType } from '@gitlab/ui';
import { createLocalVue, shallowMount, mount } from '@vue/test-utils';
import { nextTick } from 'vue';
import Vuex from 'vuex';
import Api from 'ee/api';
import BranchesSelect from 'ee/approvals/components/branches_select.vue';
......@@ -12,9 +13,6 @@ const TEST_PROTECTED_BRANCHES = [
{ id: 2, name: 'development' },
];
const TEST_BRANCHES_SELECTIONS = [TEST_DEFAULT_BRANCH, ...TEST_PROTECTED_BRANCHES];
const waitForEvent = ($input, event) => new Promise((resolve) => $input.one(event, resolve));
const select2Container = () => document.querySelector('.select2-container');
const select2DropdownOptions = () => document.querySelectorAll('.result-name');
const branchNames = () => TEST_BRANCHES_SELECTIONS.map((branch) => branch.name);
const protectedBranchNames = () => TEST_PROTECTED_BRANCHES.map((branch) => branch.name);
const localVue = createLocalVue();
......@@ -24,10 +22,13 @@ localVue.use(Vuex);
describe('Branches Select', () => {
let wrapper;
let store;
let $input;
const createComponent = async (props = {}) => {
wrapper = shallowMount(localVue.extend(BranchesSelect), {
const findDropdown = () => wrapper.findComponent(GlDropdown);
const findDropdownItems = () => wrapper.findAllComponents(GlDropdownItem);
const findSearch = () => wrapper.findComponent(GlSearchBoxByType);
const createComponent = (props = {}, mountFn = shallowMount) => {
wrapper = mountFn(BranchesSelect, {
propsData: {
projectId: '1',
...props,
......@@ -36,14 +37,6 @@ describe('Branches Select', () => {
store: new Vuex.Store(store),
attachTo: document.body,
});
await waitForPromises();
$input = $(wrapper.vm.$refs.input);
};
const search = (term = '') => {
$input.select2('search', term);
};
beforeEach(() => {
......@@ -56,94 +49,87 @@ describe('Branches Select', () => {
wrapper.destroy();
});
it('renders select2 input', async () => {
expect(select2Container()).toBe(null);
it('renders dropdown', async () => {
createComponent();
await waitForPromises();
await createComponent();
expect(findDropdown().exists()).toBe(true);
});
expect(select2Container()).not.toBe(null);
it('sets the initially selected item', async () => {
createComponent(
{
initRule: {
protectedBranches: [
{
id: 1,
name: 'master',
},
],
},
},
mount,
);
await waitForPromises();
expect(findDropdown().props('text')).toBe('master');
expect(
findDropdownItems()
.filter((item) => item.text() === 'master')
.at(0)
.props('isChecked'),
).toBe(true);
});
it('displays all the protected branches and any branch', async (done) => {
await createComponent();
waitForEvent($input, 'select2-loaded')
.then(() => {
const nodeList = select2DropdownOptions();
const names = [...nodeList].map((el) => el.textContent);
expect(names).toEqual(branchNames());
})
.then(done)
.catch(done.fail);
search();
it('displays all the protected branches and any branch', async () => {
createComponent();
await nextTick();
expect(findDropdown().props('loading')).toBe(true);
await waitForPromises();
expect(findDropdownItems()).toHaveLength(branchNames().length);
expect(findDropdown().props('loading')).toBe(false);
});
describe('with search term', () => {
beforeEach(() => {
return createComponent();
createComponent({}, mount);
return waitForPromises();
});
it('fetches protected branches with search term', (done) => {
it('fetches protected branches with search term', async () => {
const term = 'lorem';
waitForEvent($input, 'select2-loaded')
.then(() => {})
.then(done)
.catch(done.fail);
search(term);
findSearch().vm.$emit('input', term);
await nextTick();
expect(findSearch().props('isLoading')).toBe(true);
await waitForPromises();
expect(Api.projectProtectedBranches).toHaveBeenCalledWith(TEST_PROJECT_ID, term);
expect(findSearch().props('isLoading')).toBe(false);
});
it('fetches protected branches with no any branch if there is search', (done) => {
waitForEvent($input, 'select2-loaded')
.then(() => {
const nodeList = select2DropdownOptions();
const names = [...nodeList].map((el) => el.textContent);
expect(names).toEqual(protectedBranchNames());
})
.then(done)
.catch(done.fail);
search('master');
it('fetches protected branches with no any branch if there is a search', async () => {
findSearch().vm.$emit('input', 'master');
await waitForPromises();
expect(findDropdownItems()).toHaveLength(protectedBranchNames().length);
});
it('fetches protected branches with any branch if search contains term "any"', (done) => {
waitForEvent($input, 'select2-loaded')
.then(() => {
const nodeList = select2DropdownOptions();
const names = [...nodeList].map((el) => el.textContent);
expect(names).toEqual(branchNames());
})
.then(done)
.catch(done.fail);
search('any');
it('fetches protected branches with any branch if search contains term "any"', async () => {
findSearch().vm.$emit('input', 'any');
await waitForPromises();
expect(findDropdownItems()).toHaveLength(branchNames().length);
});
});
it('emits input when data changes', async (done) => {
await createComponent();
const selectedIndex = 1;
const selectedId = TEST_BRANCHES_SELECTIONS[selectedIndex].id;
const expected = [[selectedId]];
waitForEvent($input, 'select2-loaded')
.then(() => {
const options = select2DropdownOptions();
$(options[selectedIndex]).trigger('mouseup');
})
.then(done)
.catch(done.fail);
waitForEvent($input, 'change')
.then(() => {
expect(wrapper.emitted().input).toEqual(expected);
})
.then(done)
.catch(done.fail);
search();
it('when the branch is changed it sets the isChecked property and emits the input event', async () => {
createComponent({}, mount);
await waitForPromises();
await findDropdownItems().at(1).vm.$emit('click');
expect(findDropdownItems().at(1).props('isChecked')).toBe(true);
expect(wrapper.emitted().input).toStrictEqual([[1]]);
});
});
......@@ -2,6 +2,7 @@ import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex';
import ModalRuleCreate from 'ee/approvals/components/modal_rule_create.vue';
import RuleForm from 'ee/approvals/components/rule_form.vue';
import { stubComponent } from 'helpers/stub_component';
import GlModalVuex from '~/vue_shared/components/gl_modal_vuex.vue';
const TEST_MODAL_ID = 'test-modal-create-id';
......@@ -17,8 +18,8 @@ describe('Approvals ModalRuleCreate', () => {
let modal;
let form;
const findModal = () => wrapper.find(GlModalVuex);
const findForm = () => wrapper.find(RuleForm);
const findModal = () => wrapper.findComponent(GlModalVuex);
const findForm = () => wrapper.findComponent(RuleForm);
const factory = (options = {}) => {
const store = new Vuex.Store({
......@@ -40,6 +41,11 @@ describe('Approvals ModalRuleCreate', () => {
localVue,
store,
propsData,
stubs: {
GlModalVuex: stubComponent(GlModalVuex, {
props: ['modalModule', 'modalId', 'actionPrimary', 'actionCancel'],
}),
},
});
};
......@@ -63,8 +69,12 @@ describe('Approvals ModalRuleCreate', () => {
expect(modal.exists()).toBe(true);
expect(modal.props('modalModule')).toEqual(MODAL_MODULE);
expect(modal.props('modalId')).toEqual(TEST_MODAL_ID);
expect(modal.props('actionPrimary')).toStrictEqual({
text: 'Add approval rule',
attributes: [{ variant: 'confirm' }],
});
expect(modal.props('actionCancel')).toStrictEqual({ text: 'Cancel' });
expect(modal.attributes('title')).toEqual('Add approval rule');
expect(modal.attributes('ok-title')).toEqual('Add approval rule');
});
it('renders form', () => {
......@@ -91,7 +101,11 @@ describe('Approvals ModalRuleCreate', () => {
it('renders modal', () => {
expect(modal.exists()).toBe(true);
expect(modal.attributes('title')).toEqual('Update approval rule');
expect(modal.attributes('ok-title')).toEqual('Update approval rule');
expect(modal.props('actionPrimary')).toStrictEqual({
text: 'Update approval rule',
attributes: [{ variant: 'confirm' }],
});
expect(modal.props('actionCancel')).toStrictEqual({ text: 'Cancel' });
});
it('renders form', () => {
......@@ -112,7 +126,11 @@ describe('Approvals ModalRuleCreate', () => {
it('renders add rule modal', () => {
expect(modal.exists()).toBe(true);
expect(modal.attributes('title')).toEqual('Add approval rule');
expect(modal.attributes('ok-title')).toEqual('Add approval rule');
expect(modal.props('actionPrimary')).toStrictEqual({
text: 'Add approval rule',
attributes: [{ variant: 'confirm' }],
});
expect(modal.props('actionCancel')).toStrictEqual({ text: 'Cancel' });
});
it('renders form with defaultRuleName', () => {
......
......@@ -3808,6 +3808,9 @@ msgid_plural "ApprovalRuleSummary|%{count} approvals required from %{membersCoun
msgstr[0] ""
msgstr[1] ""
msgid "ApprovalRule|Add approvers"
msgstr ""
msgid "ApprovalRule|Approval rules"
msgstr ""
......@@ -3817,6 +3820,9 @@ msgstr ""
msgid "ApprovalRule|Approvers"
msgstr ""
msgid "ApprovalRule|Examples: QA, Security."
msgstr ""
msgid "ApprovalRule|Name"
msgstr ""
......@@ -3826,9 +3832,6 @@ msgstr ""
msgid "ApprovalRule|Target branch"
msgstr ""
msgid "ApprovalRule|e.g. QA, Security, etc."
msgstr ""
msgid "ApprovalStatusTooltip|Adheres to separation of duties"
msgstr ""
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment