Commit 03e8e135 authored by Nicolò Maria Mezzopera's avatar Nicolò Maria Mezzopera Committed by Natalia Tepluhina

Capture API errors in settings form

- update component
- adjust tests
parent 1c95ebb6
<script> <script>
import { get } from 'lodash';
import { mapActions, mapState, mapGetters } from 'vuex'; import { mapActions, mapState, mapGetters } from 'vuex';
import { GlCard, GlDeprecatedButton, GlLoadingIcon } from '@gitlab/ui'; import { GlCard, GlDeprecatedButton, GlLoadingIcon } from '@gitlab/ui';
import Tracking from '~/tracking'; import Tracking from '~/tracking';
...@@ -31,7 +32,8 @@ export default { ...@@ -31,7 +32,8 @@ export default {
tracking: { tracking: {
label: 'docker_container_retention_and_expiration_policies', label: 'docker_container_retention_and_expiration_policies',
}, },
formIsValid: true, fieldsAreValid: true,
apiErrors: null,
}; };
}, },
computed: { computed: {
...@@ -39,7 +41,7 @@ export default { ...@@ -39,7 +41,7 @@ export default {
...mapGetters({ isEdited: 'getIsEdited' }), ...mapGetters({ isEdited: 'getIsEdited' }),
...mapComputed([{ key: 'settings', getter: 'getSettings' }], 'updateSettings'), ...mapComputed([{ key: 'settings', getter: 'getSettings' }], 'updateSettings'),
isSubmitButtonDisabled() { isSubmitButtonDisabled() {
return !this.formIsValid || this.isLoading; return !this.fieldsAreValid || this.isLoading;
}, },
isCancelButtonDisabled() { isCancelButtonDisabled() {
return !this.isEdited || this.isLoading; return !this.isEdited || this.isLoading;
...@@ -49,13 +51,35 @@ export default { ...@@ -49,13 +51,35 @@ export default {
...mapActions(['resetSettings', 'saveSettings']), ...mapActions(['resetSettings', 'saveSettings']),
reset() { reset() {
this.track('reset_form'); this.track('reset_form');
this.apiErrors = null;
this.resetSettings(); this.resetSettings();
}, },
setApiErrors(response) {
const messages = get(response, 'data.message', []);
this.apiErrors = Object.keys(messages).reduce((acc, curr) => {
if (curr.startsWith('container_expiration_policy.')) {
const key = curr.replace('container_expiration_policy.', '');
acc[key] = get(messages, [curr, 0], '');
}
return acc;
}, {});
},
submit() { submit() {
this.track('submit_form'); this.track('submit_form');
this.apiErrors = null;
this.saveSettings() this.saveSettings()
.then(() => this.$toast.show(UPDATE_SETTINGS_SUCCESS_MESSAGE, { type: 'success' })) .then(() => this.$toast.show(UPDATE_SETTINGS_SUCCESS_MESSAGE, { type: 'success' }))
.catch(() => this.$toast.show(UPDATE_SETTINGS_ERROR_MESSAGE, { type: 'error' })); .catch(({ response }) => {
this.setApiErrors(response);
this.$toast.show(UPDATE_SETTINGS_ERROR_MESSAGE, { type: 'error' });
});
},
onModelChange(changePayload) {
this.settings = changePayload.newValue;
if (this.apiErrors) {
this.apiErrors[changePayload.modified] = undefined;
}
}, },
}, },
}; };
...@@ -69,11 +93,13 @@ export default { ...@@ -69,11 +93,13 @@ export default {
</template> </template>
<template #default> <template #default>
<expiration-policy-fields <expiration-policy-fields
v-model="settings" :value="settings"
:form-options="formOptions" :form-options="formOptions"
:is-loading="isLoading" :is-loading="isLoading"
@validated="formIsValid = true" :api-errors="apiErrors"
@invalidated="formIsValid = false" @validated="fieldsAreValid = true"
@invalidated="fieldsAreValid = false"
@input="onModelChange"
/> />
</template> </template>
<template #footer> <template #footer>
......
...@@ -34,6 +34,11 @@ export default { ...@@ -34,6 +34,11 @@ export default {
required: false, required: false,
default: () => ({}), default: () => ({}),
}, },
apiErrors: {
type: Object,
required: false,
default: null,
},
isLoading: { isLoading: {
type: Boolean, type: Boolean,
required: false, required: false,
...@@ -56,9 +61,8 @@ export default { ...@@ -56,9 +61,8 @@ export default {
}, },
}, },
i18n: { i18n: {
textAreaInvalidFeedback: TEXT_AREA_INVALID_FEEDBACK, ENABLE_TOGGLE_LABEL,
enableToggleLabel: ENABLE_TOGGLE_LABEL, ENABLE_TOGGLE_DESCRIPTION,
enableToggleDescription: ENABLE_TOGGLE_DESCRIPTION,
}, },
selectList: [ selectList: [
{ {
...@@ -86,7 +90,6 @@ export default { ...@@ -86,7 +90,6 @@ export default {
label: NAME_REGEX_LABEL, label: NAME_REGEX_LABEL,
model: 'name_regex', model: 'name_regex',
placeholder: NAME_REGEX_PLACEHOLDER, placeholder: NAME_REGEX_PLACEHOLDER,
stateVariable: 'nameRegexState',
description: NAME_REGEX_DESCRIPTION, description: NAME_REGEX_DESCRIPTION,
}, },
{ {
...@@ -94,7 +97,6 @@ export default { ...@@ -94,7 +97,6 @@ export default {
label: NAME_REGEX_KEEP_LABEL, label: NAME_REGEX_KEEP_LABEL,
model: 'name_regex_keep', model: 'name_regex_keep',
placeholder: NAME_REGEX_KEEP_PLACEHOLDER, placeholder: NAME_REGEX_KEEP_PLACEHOLDER,
stateVariable: 'nameKeepRegexState',
description: NAME_REGEX_KEEP_DESCRIPTION, description: NAME_REGEX_KEEP_DESCRIPTION,
}, },
], ],
...@@ -111,16 +113,34 @@ export default { ...@@ -111,16 +113,34 @@ export default {
policyEnabledText() { policyEnabledText() {
return this.enabled ? ENABLED_TEXT : DISABLED_TEXT; return this.enabled ? ENABLED_TEXT : DISABLED_TEXT;
}, },
textAreaState() { textAreaValidation() {
const nameRegexErrors =
this.apiErrors?.name_regex || this.validateRegexLength(this.name_regex);
const nameKeepRegexErrors =
this.apiErrors?.name_regex_keep || this.validateRegexLength(this.name_regex_keep);
return { return {
nameRegexState: this.validateNameRegex(this.name_regex), /*
nameKeepRegexState: this.validateNameRegex(this.name_regex_keep), * The state has this form:
* null: gray border, no message
* true: green border, no message ( because none is configured)
* false: red border, error message
* So in this function we keep null if the are no message otherwise we 'invert' the error message
*/
name_regex: {
state: nameRegexErrors === null ? null : !nameRegexErrors,
message: nameRegexErrors,
},
name_regex_keep: {
state: nameKeepRegexErrors === null ? null : !nameKeepRegexErrors,
message: nameKeepRegexErrors,
},
}; };
}, },
fieldsValidity() { fieldsValidity() {
return ( return (
this.textAreaState.nameRegexState !== false && this.textAreaValidation.name_regex.state !== false &&
this.textAreaState.nameKeepRegexState !== false this.textAreaValidation.name_regex_keep.state !== false
); );
}, },
isFormElementDisabled() { isFormElementDisabled() {
...@@ -140,8 +160,11 @@ export default { ...@@ -140,8 +160,11 @@ export default {
}, },
}, },
methods: { methods: {
validateNameRegex(value) { validateRegexLength(value) {
return value ? value.length <= NAME_REGEX_LENGTH : null; if (!value) {
return null;
}
return value.length <= NAME_REGEX_LENGTH ? '' : TEXT_AREA_INVALID_FEEDBACK;
}, },
idGenerator(id) { idGenerator(id) {
return `${id}_${this.uniqueId}`; return `${id}_${this.uniqueId}`;
...@@ -160,7 +183,7 @@ export default { ...@@ -160,7 +183,7 @@ export default {
:label-cols="labelCols" :label-cols="labelCols"
:label-align="labelAlign" :label-align="labelAlign"
:label-for="idGenerator('expiration-policy-toggle')" :label-for="idGenerator('expiration-policy-toggle')"
:label="$options.i18n.enableToggleLabel" :label="$options.i18n.ENABLE_TOGGLE_LABEL"
> >
<div class="d-flex align-items-start"> <div class="d-flex align-items-start">
<gl-toggle <gl-toggle
...@@ -169,7 +192,7 @@ export default { ...@@ -169,7 +192,7 @@ export default {
:disabled="isLoading" :disabled="isLoading"
/> />
<span class="mb-2 ml-2 lh-2"> <span class="mb-2 ml-2 lh-2">
<gl-sprintf :message="$options.i18n.enableToggleDescription"> <gl-sprintf :message="$options.i18n.ENABLE_TOGGLE_DESCRIPTION">
<template #toggleStatus> <template #toggleStatus>
<strong>{{ policyEnabledText }}</strong> <strong>{{ policyEnabledText }}</strong>
</template> </template>
...@@ -210,8 +233,8 @@ export default { ...@@ -210,8 +233,8 @@ export default {
:label-cols="labelCols" :label-cols="labelCols"
:label-align="labelAlign" :label-align="labelAlign"
:label-for="idGenerator(textarea.name)" :label-for="idGenerator(textarea.name)"
:state="textAreaState[textarea.stateVariable]" :state="textAreaValidation[textarea.model].state"
:invalid-feedback="$options.i18n.textAreaInvalidFeedback" :invalid-feedback="textAreaValidation[textarea.model].message"
> >
<template #label> <template #label>
<gl-sprintf :message="textarea.label"> <gl-sprintf :message="textarea.label">
...@@ -224,7 +247,7 @@ export default { ...@@ -224,7 +247,7 @@ export default {
:id="idGenerator(textarea.name)" :id="idGenerator(textarea.name)"
:value="value[textarea.model]" :value="value[textarea.model]"
:placeholder="textarea.placeholder" :placeholder="textarea.placeholder"
:state="textAreaState[textarea.stateVariable]" :state="textAreaValidation[textarea.model].state"
:disabled="isFormElementDisabled" :disabled="isFormElementDisabled"
trim trim
@input="updateModel($event, textarea.model)" @input="updateModel($event, textarea.model)"
......
...@@ -23,7 +23,7 @@ export const ENABLE_TOGGLE_DESCRIPTION = s__( ...@@ -23,7 +23,7 @@ export const ENABLE_TOGGLE_DESCRIPTION = s__(
); );
export const TEXT_AREA_INVALID_FEEDBACK = s__( export const TEXT_AREA_INVALID_FEEDBACK = s__(
'ContainerRegistry|The value of this input should be less than 255 characters', 'ContainerRegistry|The value of this input should be less than 256 characters',
); );
export const EXPIRATION_INTERVAL_LABEL = s__('ContainerRegistry|Expiration interval:'); export const EXPIRATION_INTERVAL_LABEL = s__('ContainerRegistry|Expiration interval:');
......
...@@ -11,7 +11,7 @@ export const mapComputedToEvent = (list, root) => { ...@@ -11,7 +11,7 @@ export const mapComputedToEvent = (list, root) => {
return this[root][e]; return this[root][e];
}, },
set(value) { set(value) {
this.$emit('input', { ...this[root], [e]: value }); this.$emit('input', { newValue: { ...this[root], [e]: value }, modified: e });
}, },
}; };
}); });
......
---
title: 'Cleanup policies: display API error messages under form field'
merge_request: 36190
author:
type: changed
...@@ -6433,7 +6433,7 @@ msgstr "" ...@@ -6433,7 +6433,7 @@ msgstr ""
msgid "ContainerRegistry|The last tag related to this image was recently removed. This empty image and any associated data will be automatically removed as part of the regular Garbage Collection process. If you have any questions, contact your administrator." msgid "ContainerRegistry|The last tag related to this image was recently removed. This empty image and any associated data will be automatically removed as part of the regular Garbage Collection process. If you have any questions, contact your administrator."
msgstr "" msgstr ""
msgid "ContainerRegistry|The value of this input should be less than 255 characters" msgid "ContainerRegistry|The value of this input should be less than 256 characters"
msgstr "" msgstr ""
msgid "ContainerRegistry|There are no container images available in this group" msgid "ContainerRegistry|There are no container images available in this group"
......
...@@ -7,6 +7,7 @@ import { ...@@ -7,6 +7,7 @@ import {
UPDATE_SETTINGS_ERROR_MESSAGE, UPDATE_SETTINGS_ERROR_MESSAGE,
UPDATE_SETTINGS_SUCCESS_MESSAGE, UPDATE_SETTINGS_SUCCESS_MESSAGE,
} from '~/registry/shared/constants'; } from '~/registry/shared/constants';
import waitForPromises from 'helpers/wait_for_promises';
import { stringifiedFormOptions } from '../../shared/mock_data'; import { stringifiedFormOptions } from '../../shared/mock_data';
describe('Settings Form', () => { describe('Settings Form', () => {
...@@ -36,12 +37,17 @@ describe('Settings Form', () => { ...@@ -36,12 +37,17 @@ describe('Settings Form', () => {
const findSaveButton = () => wrapper.find({ ref: 'save-button' }); const findSaveButton = () => wrapper.find({ ref: 'save-button' });
const findLoadingIcon = (parent = wrapper) => parent.find(GlLoadingIcon); const findLoadingIcon = (parent = wrapper) => parent.find(GlLoadingIcon);
const mountComponent = () => { const mountComponent = (data = {}) => {
wrapper = shallowMount(component, { wrapper = shallowMount(component, {
stubs: { stubs: {
GlCard, GlCard,
GlLoadingIcon, GlLoadingIcon,
}, },
data() {
return {
...data,
};
},
mocks: { mocks: {
$toast: { $toast: {
show: jest.fn(), show: jest.fn(),
...@@ -55,7 +61,6 @@ describe('Settings Form', () => { ...@@ -55,7 +61,6 @@ describe('Settings Form', () => {
store = createStore(); store = createStore();
store.dispatch('setInitialState', stringifiedFormOptions); store.dispatch('setInitialState', stringifiedFormOptions);
dispatchSpy = jest.spyOn(store, 'dispatch'); dispatchSpy = jest.spyOn(store, 'dispatch');
mountComponent();
jest.spyOn(Tracking, 'event'); jest.spyOn(Tracking, 'event');
}); });
...@@ -63,20 +68,30 @@ describe('Settings Form', () => { ...@@ -63,20 +68,30 @@ describe('Settings Form', () => {
wrapper.destroy(); wrapper.destroy();
}); });
describe('data binding', () => {
it('v-model change update the settings property', () => {
mountComponent();
findFields().vm.$emit('input', { newValue: 'foo' });
expect(dispatchSpy).toHaveBeenCalledWith('updateSettings', { settings: 'foo' });
});
it('v-model change update the api error property', () => {
const apiErrors = { baz: 'bar' };
mountComponent({ apiErrors });
expect(findFields().props('apiErrors')).toEqual(apiErrors);
findFields().vm.$emit('input', { newValue: 'foo', modified: 'baz' });
expect(findFields().props('apiErrors')).toEqual({});
});
});
describe('form', () => { describe('form', () => {
let form; let form;
beforeEach(() => { beforeEach(() => {
mountComponent();
form = findForm(); form = findForm();
dispatchSpy.mockReturnValue(); dispatchSpy.mockReturnValue();
}); });
describe('data binding', () => {
it('v-model change update the settings property', () => {
findFields().vm.$emit('input', 'foo');
expect(dispatchSpy).toHaveBeenCalledWith('updateSettings', { settings: 'foo' });
});
});
describe('form reset event', () => { describe('form reset event', () => {
beforeEach(() => { beforeEach(() => {
form.trigger('reset'); form.trigger('reset');
...@@ -108,24 +123,40 @@ describe('Settings Form', () => { ...@@ -108,24 +123,40 @@ describe('Settings Form', () => {
expect(Tracking.event).toHaveBeenCalledWith(undefined, 'submit_form', trackingPayload); expect(Tracking.event).toHaveBeenCalledWith(undefined, 'submit_form', trackingPayload);
}); });
it('show a success toast when submit succeed', () => { it('show a success toast when submit succeed', async () => {
dispatchSpy.mockResolvedValue(); dispatchSpy.mockResolvedValue();
form.trigger('submit'); form.trigger('submit');
return wrapper.vm.$nextTick().then(() => { await waitForPromises();
expect(wrapper.vm.$toast.show).toHaveBeenCalledWith(UPDATE_SETTINGS_SUCCESS_MESSAGE, { expect(wrapper.vm.$toast.show).toHaveBeenCalledWith(UPDATE_SETTINGS_SUCCESS_MESSAGE, {
type: 'success', type: 'success',
});
}); });
}); });
it('show an error toast when submit fails', () => { describe('when submit fails', () => {
dispatchSpy.mockRejectedValue(); it('shows an error', async () => {
form.trigger('submit'); dispatchSpy.mockRejectedValue({ response: {} });
return wrapper.vm.$nextTick().then(() => { form.trigger('submit');
await waitForPromises();
expect(wrapper.vm.$toast.show).toHaveBeenCalledWith(UPDATE_SETTINGS_ERROR_MESSAGE, { expect(wrapper.vm.$toast.show).toHaveBeenCalledWith(UPDATE_SETTINGS_ERROR_MESSAGE, {
type: 'error', type: 'error',
}); });
}); });
it('parses the error messages', async () => {
dispatchSpy.mockRejectedValue({
response: {
data: {
message: {
foo: 'bar',
'container_expiration_policy.name': ['baz'],
},
},
},
});
form.trigger('submit');
await waitForPromises();
expect(findFields().props('apiErrors')).toEqual({ name: 'baz' });
});
}); });
}); });
}); });
...@@ -134,6 +165,7 @@ describe('Settings Form', () => { ...@@ -134,6 +165,7 @@ describe('Settings Form', () => {
describe('cancel button', () => { describe('cancel button', () => {
beforeEach(() => { beforeEach(() => {
store.commit('SET_SETTINGS', { foo: 'bar' }); store.commit('SET_SETTINGS', { foo: 'bar' });
mountComponent();
}); });
it('has type reset', () => { it('has type reset', () => {
...@@ -165,6 +197,7 @@ describe('Settings Form', () => { ...@@ -165,6 +197,7 @@ describe('Settings Form', () => {
describe('when isLoading is true', () => { describe('when isLoading is true', () => {
beforeEach(() => { beforeEach(() => {
store.commit('TOGGLE_LOADING'); store.commit('TOGGLE_LOADING');
mountComponent();
}); });
afterEach(() => { afterEach(() => {
store.commit('TOGGLE_LOADING'); store.commit('TOGGLE_LOADING');
......
...@@ -114,7 +114,6 @@ exports[`Expiration Policy Form renders 1`] = ` ...@@ -114,7 +114,6 @@ exports[`Expiration Policy Form renders 1`] = `
<gl-form-group-stub <gl-form-group-stub
id="expiration-policy-name-matching-group" id="expiration-policy-name-matching-group"
invalid-feedback="The value of this input should be less than 255 characters"
label-align="right" label-align="right"
label-cols="3" label-cols="3"
label-for="expiration-policy-name-matching" label-for="expiration-policy-name-matching"
...@@ -131,7 +130,6 @@ exports[`Expiration Policy Form renders 1`] = ` ...@@ -131,7 +130,6 @@ exports[`Expiration Policy Form renders 1`] = `
</gl-form-group-stub> </gl-form-group-stub>
<gl-form-group-stub <gl-form-group-stub
id="expiration-policy-keep-name-group" id="expiration-policy-keep-name-group"
invalid-feedback="The value of this input should be less than 255 characters"
label-align="right" label-align="right"
label-cols="3" label-cols="3"
label-for="expiration-policy-keep-name" label-for="expiration-policy-keep-name"
......
...@@ -94,7 +94,9 @@ describe('Expiration Policy Form', () => { ...@@ -94,7 +94,9 @@ describe('Expiration Policy Form', () => {
: 'input'; : 'input';
element.vm.$emit(modelUpdateEvent, value); element.vm.$emit(modelUpdateEvent, value);
return wrapper.vm.$nextTick().then(() => { return wrapper.vm.$nextTick().then(() => {
expect(wrapper.emitted('input')).toEqual([[{ [modelName]: value }]]); expect(wrapper.emitted('input')).toEqual([
[{ newValue: { [modelName]: value }, modified: modelName }],
]);
}); });
}); });
...@@ -126,42 +128,61 @@ describe('Expiration Policy Form', () => { ...@@ -126,42 +128,61 @@ describe('Expiration Policy Form', () => {
}); });
describe.each` describe.each`
modelName | elementName | stateVariable modelName | elementName
${'name_regex'} | ${'name-matching'} | ${'nameRegexState'} ${'name_regex'} | ${'name-matching'}
${'name_regex_keep'} | ${'keep-name'} | ${'nameKeepRegexState'} ${'name_regex_keep'} | ${'keep-name'}
`('regex textarea validation', ({ modelName, elementName, stateVariable }) => { `('regex textarea validation', ({ modelName, elementName }) => {
describe(`when name regex is longer than ${NAME_REGEX_LENGTH}`, () => { const invalidString = new Array(NAME_REGEX_LENGTH + 2).join(',');
const invalidString = new Array(NAME_REGEX_LENGTH + 2).join(',');
describe('when apiError contains an error message', () => {
beforeEach(() => { const errorMessage = 'something went wrong';
mountComponent({ value: { [modelName]: invalidString } });
it('shows the error message on the relevant field', () => {
mountComponent({ apiErrors: { [modelName]: errorMessage } });
expect(findFormGroup(elementName).attributes('invalid-feedback')).toBe(errorMessage);
}); });
it(`${stateVariable} is false`, () => { it('gives precedence to API errors compared to local ones', () => {
expect(wrapper.vm.textAreaState[stateVariable]).toBe(false); mountComponent({
}); apiErrors: { [modelName]: errorMessage },
value: { [modelName]: invalidString },
it('emit the @invalidated event', () => { });
expect(wrapper.emitted('invalidated')).toBeTruthy(); expect(findFormGroup(elementName).attributes('invalid-feedback')).toBe(errorMessage);
}); });
}); });
it('if the user did not type validation is null', () => { describe('when apiErrors is empty', () => {
mountComponent({ value: { [modelName]: '' } }); it('if the user did not type validation is null', async () => {
return wrapper.vm.$nextTick().then(() => { mountComponent({ value: { [modelName]: '' } });
expect(wrapper.vm.textAreaState[stateVariable]).toBe(null); expect(findFormGroup(elementName).attributes('state')).toBeUndefined();
expect(wrapper.emitted('validated')).toBeTruthy(); expect(wrapper.emitted('validated')).toBeTruthy();
}); });
});
it(`if the user typed and is less than ${NAME_REGEX_LENGTH} state is true`, () => { it(`if the user typed and is less than ${NAME_REGEX_LENGTH} state is true`, () => {
mountComponent({ value: { [modelName]: 'foo' } }); mountComponent({ value: { [modelName]: 'foo' } });
return wrapper.vm.$nextTick().then(() => {
const formGroup = findFormGroup(elementName); const formGroup = findFormGroup(elementName);
const formElement = findFormElements(elementName, formGroup); const formElement = findFormElements(elementName, formGroup);
expect(formGroup.attributes('state')).toBeTruthy(); expect(formGroup.attributes('state')).toBeTruthy();
expect(formElement.attributes('state')).toBeTruthy(); expect(formElement.attributes('state')).toBeTruthy();
}); });
describe(`when name regex is longer than ${NAME_REGEX_LENGTH}`, () => {
beforeEach(() => {
mountComponent({ value: { [modelName]: invalidString } });
});
it('textAreaValidation state is false', () => {
expect(findFormGroup(elementName).attributes('state')).toBeUndefined();
// we are forced to check the model attribute because falsy attrs are all casted to undefined in attrs
// while in this case false shows an error and null instead shows nothing.
expect(wrapper.vm.textAreaValidation[modelName].state).toBe(false);
});
it('emit the @invalidated event', () => {
expect(wrapper.emitted('invalidated')).toBeTruthy();
});
});
}); });
}); });
......
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