Commit b41f073f authored by Enrique Alcántara's avatar Enrique Alcántara

Merge branch '332548-update-approvals-to-use-objects' into 'master'

Update approval settings from boolean to object format

See merge request gitlab-org/gitlab!69505
parents 863f783d a469111a
......@@ -3,7 +3,6 @@ import { GlAlert, GlButton, GlForm, GlFormGroup, GlLoadingIcon, GlLink } from '@
import { isEmpty } from 'lodash';
import { mapActions, mapGetters, mapState } from 'vuex';
import { helpPagePath } from '~/helpers/help_page_helper';
import { mapComputed } from '~/vuex_shared/bindings';
import { APPROVAL_SETTINGS_I18N } from '../constants';
import ApprovalSettingsCheckbox from './approval_settings_checkbox.vue';
......@@ -48,18 +47,14 @@ export default {
isUpdated: (state) => state.approvalSettings.isUpdated,
settings: (state) => state.approvalSettings.settings,
errorMessage: (state) => state.approvalSettings.errorMessage,
preventAuthorApproval: (state) => state.approvalSettings.settings.preventAuthorApproval,
preventCommittersApproval: (state) =>
state.approvalSettings.settings.preventCommittersApproval,
preventMrApprovalRuleEdit: (state) =>
state.approvalSettings.settings.preventMrApprovalRuleEdit,
removeApprovalsOnPush: (state) => state.approvalSettings.settings.removeApprovalsOnPush,
requireUserPassword: (state) => state.approvalSettings.settings.requireUserPassword,
}),
...mapComputed(
[
{ key: 'preventAuthorApproval', updateFn: 'setPreventAuthorApproval' },
{ key: 'preventCommittersApproval', updateFn: 'setPreventCommittersApproval' },
{ key: 'preventMrApprovalRuleEdit', updateFn: 'setPreventMrApprovalRuleEdit' },
{ key: 'removeApprovalsOnPush', updateFn: 'setRemoveApprovalsOnPush' },
{ key: 'requireUserPassword', updateFn: 'setRequireUserPassword' },
],
undefined,
(state) => state.approvalSettings.settings,
),
...mapGetters(['settingChanged']),
hasSettings() {
return !isEmpty(this.settings);
......@@ -77,6 +72,11 @@ export default {
'updateSettings',
'dismissErrorMessage',
'dismissSuccessMessage',
'setPreventAuthorApproval',
'setPreventCommittersApproval',
'setPreventMrApprovalRuleEdit',
'setRemoveApprovalsOnPush',
'setRequireUserPassword',
]),
async onSubmit() {
await this.updateSettings(this.approvalSettingsPath);
......@@ -124,35 +124,44 @@ export default {
</p>
<gl-form-group>
<approval-settings-checkbox
v-model="preventAuthorApproval"
:checked="preventAuthorApproval.value"
:label="settingsLabels.authorApprovalLabel"
:locked="!canPreventAuthorApproval"
:locked="!canPreventAuthorApproval || preventAuthorApproval.locked"
:locked-text="$options.i18n.lockedByAdmin"
data-testid="prevent-author-approval"
@input="setPreventAuthorApproval"
/>
<approval-settings-checkbox
v-model="preventCommittersApproval"
:checked="preventCommittersApproval.value"
:label="settingsLabels.preventCommittersApprovalLabel"
:locked="!canPreventCommittersApproval"
:locked="!canPreventCommittersApproval || preventCommittersApproval.locked"
:locked-text="$options.i18n.lockedByAdmin"
data-testid="prevent-committers-approval"
@input="setPreventCommittersApproval"
/>
<approval-settings-checkbox
v-model="preventMrApprovalRuleEdit"
:checked="preventMrApprovalRuleEdit.value"
:label="settingsLabels.preventMrApprovalRuleEditLabel"
:locked="!canPreventMrApprovalRuleEdit"
:locked="!canPreventMrApprovalRuleEdit || preventMrApprovalRuleEdit.locked"
:locked-text="$options.i18n.lockedByAdmin"
data-testid="prevent-mr-approval-rule-edit"
@input="setPreventMrApprovalRuleEdit"
/>
<approval-settings-checkbox
v-model="requireUserPassword"
:checked="requireUserPassword.value"
:label="settingsLabels.requireUserPasswordLabel"
:locked="requireUserPassword.locked"
:locked-text="$options.i18n.lockedByAdmin"
data-testid="require-user-password"
@input="setRequireUserPassword"
/>
<approval-settings-checkbox
v-model="removeApprovalsOnPush"
:checked="removeApprovalsOnPush.value"
:label="settingsLabels.removeApprovalsOnPushLabel"
:locked="removeApprovalsOnPush.locked"
:locked-text="$options.i18n.lockedByAdmin"
data-testid="remove-approvals-on-push"
@input="setRemoveApprovalsOnPush"
/>
</gl-form-group>
<gl-button
......
......@@ -14,7 +14,7 @@ export default {
type: String,
required: true,
},
value: {
checked: {
type: Boolean,
required: false,
default: false,
......@@ -47,7 +47,7 @@ export default {
</script>
<template>
<gl-form-checkbox :disabled="locked" :checked="value" @input="input">
<gl-form-checkbox :disabled="locked" :checked="checked" @input="input">
{{ label }}
<template v-if="locked">
<gl-icon :id="lockIconId" data-testid="lock-icon" name="lock" />
......
......@@ -100,36 +100,40 @@ export const mapMRApprovalSettingsResponse = (res) => {
};
};
const invertApprovalSetting = ({ value, ...rest }) => ({ value: !value, ...rest });
export const groupApprovalsMappers = {
mapDataToState: (data) => ({
preventAuthorApproval: !data.allow_author_approval.value,
preventMrApprovalRuleEdit: !data.allow_overrides_to_approver_list_per_merge_request.value,
requireUserPassword: data.require_password_to_approve.value,
removeApprovalsOnPush: !data.retain_approvals_on_push.value,
preventCommittersApproval: !data.allow_committer_approval.value,
preventAuthorApproval: invertApprovalSetting(data.allow_author_approval),
preventMrApprovalRuleEdit: invertApprovalSetting(
data.allow_overrides_to_approver_list_per_merge_request,
),
requireUserPassword: data.require_password_to_approve,
removeApprovalsOnPush: invertApprovalSetting(data.retain_approvals_on_push),
preventCommittersApproval: invertApprovalSetting(data.allow_committer_approval),
}),
mapStateToPayload: (state) => ({
allow_author_approval: !state.settings.preventAuthorApproval,
allow_overrides_to_approver_list_per_merge_request: !state.settings.preventMrApprovalRuleEdit,
require_password_to_approve: state.settings.requireUserPassword,
retain_approvals_on_push: !state.settings.removeApprovalsOnPush,
allow_committer_approval: !state.settings.preventCommittersApproval,
mapStateToPayload: ({ settings }) => ({
allow_author_approval: !settings.preventAuthorApproval.value,
allow_overrides_to_approver_list_per_merge_request: !settings.preventMrApprovalRuleEdit.value,
require_password_to_approve: settings.requireUserPassword.value,
retain_approvals_on_push: !settings.removeApprovalsOnPush.value,
allow_committer_approval: !settings.preventCommittersApproval.value,
}),
};
export const projectApprovalsMappers = {
mapDataToState: (data) => ({
preventAuthorApproval: !data.merge_requests_author_approval,
preventMrApprovalRuleEdit: data.disable_overriding_approvers_per_merge_request,
requireUserPassword: data.require_password_to_approve,
removeApprovalsOnPush: data.reset_approvals_on_push,
preventCommittersApproval: data.merge_requests_disable_committers_approval,
preventAuthorApproval: { value: !data.merge_requests_author_approval },
preventMrApprovalRuleEdit: { value: data.disable_overriding_approvers_per_merge_request },
requireUserPassword: { value: data.require_password_to_approve },
removeApprovalsOnPush: { value: data.reset_approvals_on_push },
preventCommittersApproval: { value: data.merge_requests_disable_committers_approval },
}),
mapStateToPayload: (state) => ({
merge_requests_author_approval: !state.settings.preventAuthorApproval,
disable_overriding_approvers_per_merge_request: state.settings.preventMrApprovalRuleEdit,
require_password_to_approve: state.settings.requireUserPassword,
reset_approvals_on_push: state.settings.removeApprovalsOnPush,
merge_requests_disable_committers_approval: state.settings.preventCommittersApproval,
mapStateToPayload: ({ settings }) => ({
merge_requests_author_approval: !settings.preventAuthorApproval.value,
disable_overriding_approvers_per_merge_request: settings.preventMrApprovalRuleEdit.value,
require_password_to_approve: settings.requireUserPassword.value,
reset_approvals_on_push: settings.removeApprovalsOnPush.value,
merge_requests_disable_committers_approval: settings.preventCommittersApproval.value,
}),
};
......@@ -46,23 +46,23 @@ export default (mapStateToPayload, updateMethod = 'put') => ({
commit(types.DISMISS_ERROR_MESSAGE);
},
setPreventAuthorApproval({ commit }, { preventAuthorApproval }) {
commit(types.SET_PREVENT_AUTHOR_APPROVAL, preventAuthorApproval);
setPreventAuthorApproval({ commit }, value) {
commit(types.SET_PREVENT_AUTHOR_APPROVAL, value);
},
setPreventCommittersApproval({ commit }, { preventCommittersApproval }) {
commit(types.SET_PREVENT_COMMITTERS_APPROVAL, preventCommittersApproval);
setPreventCommittersApproval({ commit }, value) {
commit(types.SET_PREVENT_COMMITTERS_APPROVAL, value);
},
setPreventMrApprovalRuleEdit({ commit }, { preventMrApprovalRuleEdit }) {
commit(types.SET_PREVENT_MR_APPROVAL_RULE_EDIT, preventMrApprovalRuleEdit);
setPreventMrApprovalRuleEdit({ commit }, value) {
commit(types.SET_PREVENT_MR_APPROVAL_RULE_EDIT, value);
},
setRemoveApprovalsOnPush({ commit }, { removeApprovalsOnPush }) {
commit(types.SET_REMOVE_APPROVALS_ON_PUSH, removeApprovalsOnPush);
setRemoveApprovalsOnPush({ commit }, value) {
commit(types.SET_REMOVE_APPROVALS_ON_PUSH, value);
},
setRequireUserPassword({ commit }, { requireUserPassword }) {
commit(types.SET_REQUIRE_USER_PASSWORD, requireUserPassword);
setRequireUserPassword({ commit }, value) {
commit(types.SET_REQUIRE_USER_PASSWORD, value);
},
});
export const settingChanged = ({ settings, initialSettings }) => {
return Object.entries(settings).findIndex(([key, value]) => initialSettings[key] !== value) > -1;
return (
Object.entries(settings).findIndex(([key, { value }]) => initialSettings[key].value !== value) >
-1
);
};
import { cloneDeep } from 'lodash';
import { APPROVAL_SETTINGS_I18N } from '../../../constants';
import * as types from './mutation_types';
......@@ -8,7 +9,7 @@ export default (mapDataToState) => ({
},
[types.RECEIVE_SETTINGS_SUCCESS](state, data) {
state.settings = { ...mapDataToState(data) };
state.initialSettings = { ...state.settings };
state.initialSettings = cloneDeep(state.settings);
state.isLoading = false;
},
[types.RECEIVE_SETTINGS_ERROR](state) {
......@@ -22,7 +23,7 @@ export default (mapDataToState) => ({
},
[types.UPDATE_SETTINGS_SUCCESS](state, data) {
state.settings = { ...mapDataToState(data) };
state.initialSettings = { ...state.settings };
state.initialSettings = cloneDeep(state.settings);
state.isLoading = false;
state.isUpdated = true;
},
......@@ -36,19 +37,19 @@ export default (mapDataToState) => ({
[types.DISMISS_ERROR_MESSAGE](state) {
state.errorMessage = '';
},
[types.SET_PREVENT_AUTHOR_APPROVAL](state, preventAuthorApproval) {
state.settings.preventAuthorApproval = preventAuthorApproval;
[types.SET_PREVENT_AUTHOR_APPROVAL](state, value) {
state.settings.preventAuthorApproval.value = value;
},
[types.SET_PREVENT_COMMITTERS_APPROVAL](state, preventCommittersApproval) {
state.settings.preventCommittersApproval = preventCommittersApproval;
[types.SET_PREVENT_COMMITTERS_APPROVAL](state, value) {
state.settings.preventCommittersApproval.value = value;
},
[types.SET_PREVENT_MR_APPROVAL_RULE_EDIT](state, preventMrApprovalRuleEdit) {
state.settings.preventMrApprovalRuleEdit = preventMrApprovalRuleEdit;
[types.SET_PREVENT_MR_APPROVAL_RULE_EDIT](state, value) {
state.settings.preventMrApprovalRuleEdit.value = value;
},
[types.SET_REMOVE_APPROVALS_ON_PUSH](state, removeApprovalsOnPush) {
state.settings.removeApprovalsOnPush = removeApprovalsOnPush;
[types.SET_REMOVE_APPROVALS_ON_PUSH](state, value) {
state.settings.removeApprovalsOnPush.value = value;
},
[types.SET_REQUIRE_USER_PASSWORD](state, requireUserPassword) {
state.settings.requireUserPassword = requireUserPassword;
[types.SET_REQUIRE_USER_PASSWORD](state, value) {
state.settings.requireUserPassword.value = value;
},
});
......@@ -45,15 +45,15 @@ describe('ApprovalSettingsCheckbox', () => {
});
});
describe('value', () => {
it('defaults to false when no value is given', () => {
describe('checked', () => {
it('defaults to false when no checked value is given', () => {
createWrapper();
expect(findCheckbox().props('checked')).toBe(false);
});
it('sets the checkbox to `true` when a `true` value is given', () => {
createWrapper({ value: true });
it('sets the checkbox to `true` when checked is `true`', () => {
createWrapper({ checked: true });
expect(findCheckbox().props('checked')).toBe(true);
});
......
......@@ -12,7 +12,7 @@ import createStore from 'ee/approvals/stores';
import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings/';
import { extendedWrapper } from 'helpers/vue_test_utils_helper';
import waitForPromises from 'helpers/wait_for_promises';
import { createGroupApprovalsPayload } from '../mocks';
import { createGroupApprovalsPayload, createGroupApprovalsState } from '../mocks';
const localVue = createLocalVue();
localVue.use(Vuex);
......@@ -101,13 +101,7 @@ describe('ApprovalSettings', () => {
});
describe('with settings', () => {
const settings = {
allow_author_approval: false,
allow_committer_approval: false,
allow_overrides_to_approver_list_per_merge_request: false,
require_password_to_approve: false,
retain_approvals_on_push: false,
};
const { settings } = createGroupApprovalsState();
beforeEach(() => {
setupStore(settings);
......@@ -146,7 +140,15 @@ describe('ApprovalSettings', () => {
});
it('renders the button as enabled when a setting was changed', async () => {
setupStore({ ...settings, allow_author_approval: true }, settings);
const changedSettings = {
...settings,
preventAuthorApproval: {
...settings.preventAuthorApproval,
value: true,
},
};
setupStore(changedSettings, settings);
createWrapper();
await waitForPromises();
......@@ -203,11 +205,18 @@ describe('ApprovalSettings', () => {
expect(checkbox.props('label')).toBe(PROJECT_APPROVAL_SETTINGS_LABELS_I18N[labelKey]);
});
it('sets the locked and lockedText based on the setting values', () => {
expect(checkbox.props()).toMatchObject({
locked: settings[setting].locked,
lockedText: APPROVAL_SETTINGS_I18N.lockedByAdmin,
});
});
it(`triggers the action ${action} when the value is changed`, async () => {
await checkbox.vm.$emit('input', true);
await waitForPromises();
expect(store.dispatch).toHaveBeenLastCalledWith(action, { [setting]: true });
expect(store.dispatch).toHaveBeenLastCalledWith(action, true);
});
});
......@@ -263,6 +272,16 @@ describe('ApprovalSettings', () => {
});
describe('locked settings', () => {
beforeEach(() => {
setupStore({
...settings,
preventAuthorApproval: {
...settings.preventAuthorApproval,
locked: false,
},
});
});
it.each`
property | value | locked | testid
${'canPreventAuthorApproval'} | ${true} | ${false} | ${'prevent-author-approval'}
......@@ -276,10 +295,7 @@ describe('ApprovalSettings', () => {
({ property, value, locked, testid }) => {
createWrapper({ [property]: value });
expect(wrapper.findByTestId(testid).props()).toMatchObject({
locked,
lockedText: APPROVAL_SETTINGS_I18N.lockedByAdmin,
});
expect(wrapper.findByTestId(testid).props('locked')).toBe(locked);
},
);
});
......
import { groupApprovalsMappers } from 'ee/approvals/mappers';
import { createGroupApprovalsPayload } from './mocks';
import { createGroupApprovalsPayload, createGroupApprovalsState } from './mocks';
describe('approvals mappers', () => {
describe('groupApprovalsMappers', () => {
const groupFetchPayload = createGroupApprovalsPayload();
const groupUpdatePayload = {
const approvalsState = createGroupApprovalsState();
const approvalsFetchPayload = createGroupApprovalsPayload();
const approvalsUpdatePayload = {
allow_author_approval: true,
allow_overrides_to_approver_list_per_merge_request: true,
require_password_to_approve: true,
retain_approvals_on_push: true,
allow_committer_approval: true,
};
const groupState = {
settings: {
preventAuthorApproval: false,
preventMrApprovalRuleEdit: false,
requireUserPassword: true,
removeApprovalsOnPush: false,
preventCommittersApproval: false,
},
};
it('maps data to state', () => {
expect(groupApprovalsMappers.mapDataToState(groupFetchPayload)).toStrictEqual(
groupState.settings,
expect(groupApprovalsMappers.mapDataToState(approvalsFetchPayload)).toStrictEqual(
approvalsState.settings,
);
});
it('maps state to payload', () => {
expect(groupApprovalsMappers.mapStateToPayload(groupState)).toStrictEqual(groupUpdatePayload);
expect(groupApprovalsMappers.mapStateToPayload(approvalsState)).toStrictEqual(
approvalsUpdatePayload,
);
});
});
});
......@@ -63,3 +63,33 @@ export const createGroupApprovalsPayload = () => ({
inherited_from: null,
},
});
export const createGroupApprovalsState = () => ({
settings: {
preventAuthorApproval: {
inherited_from: 'instance',
locked: true,
value: false,
},
preventCommittersApproval: {
inherited_from: null,
locked: false,
value: false,
},
preventMrApprovalRuleEdit: {
inherited_from: null,
locked: false,
value: false,
},
removeApprovalsOnPush: {
inherited_from: null,
locked: null,
value: false,
},
requireUserPassword: {
inherited_from: null,
locked: null,
value: true,
},
},
});
......@@ -78,11 +78,11 @@ describe('EE approvals group settings module actions', () => {
describe('on success', () => {
it('dispatches the request and updates payload', () => {
const data = {
allow_author_approval: true,
allow_committer_approval: true,
allow_overrides_to_approver_list_per_merge_request: true,
require_password_to_approve: true,
retain_approvals_on_push: true,
allow_author_approval: { value: true },
allow_committer_approval: { value: true },
allow_overrides_to_approver_list_per_merge_request: { value: true },
require_password_to_approve: { value: true },
retain_approvals_on_push: { value: true },
};
mock[onMethod](approvalSettingsPath).replyOnce(httpStatus.OK, data);
......@@ -142,17 +142,15 @@ describe('EE approvals group settings module actions', () => {
});
describe.each`
action | type | prop
${'setPreventAuthorApproval'} | ${types.SET_PREVENT_AUTHOR_APPROVAL} | ${'preventAuthorApproval'}
${'setPreventCommittersApproval'} | ${types.SET_PREVENT_COMMITTERS_APPROVAL} | ${'preventCommittersApproval'}
${'setPreventMrApprovalRuleEdit'} | ${types.SET_PREVENT_MR_APPROVAL_RULE_EDIT} | ${'preventMrApprovalRuleEdit'}
${'setRemoveApprovalsOnPush'} | ${types.SET_REMOVE_APPROVALS_ON_PUSH} | ${'removeApprovalsOnPush'}
${'setRequireUserPassword'} | ${types.SET_REQUIRE_USER_PASSWORD} | ${'requireUserPassword'}
`('$action', ({ action, type, prop }) => {
action | type
${'setPreventAuthorApproval'} | ${types.SET_PREVENT_AUTHOR_APPROVAL}
${'setPreventCommittersApproval'} | ${types.SET_PREVENT_COMMITTERS_APPROVAL}
${'setPreventMrApprovalRuleEdit'} | ${types.SET_PREVENT_MR_APPROVAL_RULE_EDIT}
${'setRemoveApprovalsOnPush'} | ${types.SET_REMOVE_APPROVALS_ON_PUSH}
${'setRequireUserPassword'} | ${types.SET_REQUIRE_USER_PASSWORD}
`('$action', ({ action, type }) => {
it(`commits ${type}`, () => {
const payload = { [prop]: true };
return testAction(actions[action], payload, state, [{ type, payload: true }], []);
return testAction(actions[action], true, state, [{ type, payload: true }], []);
});
});
});
import { cloneDeep } from 'lodash';
import * as getters from 'ee/approvals/stores/modules/approval_settings/getters';
describe('Group settings store getters', () => {
let settings;
const initialSettings = {
preventAuthorApproval: true,
preventMrApprovalRuleEdit: true,
requireUserPassword: true,
removeApprovalsOnPush: true,
preventAuthorApproval: { value: true },
preventCommittersApproval: { value: true },
preventMrApprovalRuleEdit: { value: true },
requireUserPassword: { value: true },
removeApprovalsOnPush: { value: false },
};
beforeEach(() => {
settings = { ...initialSettings };
settings = cloneDeep(initialSettings);
});
describe('settingChanged', () => {
it('returns true when a setting is changed', () => {
settings.preventAuthorApproval = false;
settings.preventAuthorApproval.value = false;
expect(getters.settingChanged({ settings, initialSettings })).toBe(true);
});
......
......@@ -8,10 +8,11 @@ describe('Group settings store mutations', () => {
const mapperFn = jest.fn((data) => data);
const mutations = mutationsFactory(mapperFn);
const settings = {
preventAuthorApproval: true,
preventMrApprovalRuleEdit: true,
requireUserPassword: true,
removeApprovalsOnPush: true,
preventAuthorApproval: { value: false },
preventCommittersApproval: { value: false },
preventMrApprovalRuleEdit: { value: false },
requireUserPassword: { value: false },
removeApprovalsOnPush: { value: false },
};
beforeEach(() => {
......@@ -100,10 +101,14 @@ describe('Group settings store mutations', () => {
${'SET_REMOVE_APPROVALS_ON_PUSH'} | ${'removeApprovalsOnPush'}
${'SET_REQUIRE_USER_PASSWORD'} | ${'requireUserPassword'}
`('$mutation', ({ mutation, prop }) => {
beforeEach(() => {
mutations.RECEIVE_SETTINGS_SUCCESS(state, settings);
});
it(`sets the ${prop}`, () => {
mutations[mutation](state, true);
expect(state.settings[prop]).toBe(true);
expect(state.settings[prop].value).toBe(true);
});
});
});
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