Merge branch...

Merge branch '220413-quickly-resolve-issues-with-your-cleanup-policy-with-improved-validation-and-notifications' into 'master'

Display API error messages under form field

See merge request gitlab-org/gitlab!36190

app/assets/javascripts/registry/settings/components/settings_form.vue

 <script>
+import { get } from 'lodash';
 import { mapActions, mapState, mapGetters } from 'vuex';
 import { GlCard, GlDeprecatedButton, GlLoadingIcon } from '@gitlab/ui';
 import Tracking from '~/tracking';
@@ -31,7 +32,8 @@ export default {
       tracking: {
         label: 'docker_container_retention_and_expiration_policies',
       },
-      formIsValid: true,
+      fieldsAreValid: true,
+      apiErrors: null,
     };
   },
   computed: {
@@ -39,7 +41,7 @@ export default {
     ...mapGetters({ isEdited: 'getIsEdited' }),
     ...mapComputed([{ key: 'settings', getter: 'getSettings' }], 'updateSettings'),
     isSubmitButtonDisabled() {
-      return !this.formIsValid || this.isLoading;
+      return !this.fieldsAreValid || this.isLoading;
     },
     isCancelButtonDisabled() {
       return !this.isEdited || this.isLoading;
@@ -49,13 +51,35 @@ export default {
     ...mapActions(['resetSettings', 'saveSettings']),
     reset() {
       this.track('reset_form');
+      this.apiErrors = null;
       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() {
       this.track('submit_form');
+      this.apiErrors = null;
       this.saveSettings()
         .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 {
       </template>
       <template #default>
         <expiration-policy-fields
-          v-model="settings"
+          :value="settings"
           :form-options="formOptions"
           :is-loading="isLoading"
-          @validated="formIsValid = true"
-          @invalidated="formIsValid = false"
+          :api-errors="apiErrors"
+          @validated="fieldsAreValid = true"
+          @invalidated="fieldsAreValid = false"
+          @input="onModelChange"
         />
       </template>
       <template #footer>

app/assets/javascripts/registry/shared/components/expiration_policy_fields.vue

@@ -34,6 +34,11 @@ export default {
       required: false,
       default: () => ({}),
     },
+    apiErrors: {
+      type: Object,
+      required: false,
+      default: null,
+    },
     isLoading: {
       type: Boolean,
       required: false,
@@ -56,9 +61,8 @@ export default {
     },
   },
   i18n: {
-    textAreaInvalidFeedback: TEXT_AREA_INVALID_FEEDBACK,
-    enableToggleLabel: ENABLE_TOGGLE_LABEL,
-    enableToggleDescription: ENABLE_TOGGLE_DESCRIPTION,
+    ENABLE_TOGGLE_LABEL,
+    ENABLE_TOGGLE_DESCRIPTION,
   },
   selectList: [
     {
@@ -86,7 +90,6 @@ export default {
       label: NAME_REGEX_LABEL,
       model: 'name_regex',
       placeholder: NAME_REGEX_PLACEHOLDER,
-      stateVariable: 'nameRegexState',
       description: NAME_REGEX_DESCRIPTION,
     },
     {
@@ -94,7 +97,6 @@ export default {
       label: NAME_REGEX_KEEP_LABEL,
       model: 'name_regex_keep',
       placeholder: NAME_REGEX_KEEP_PLACEHOLDER,
-      stateVariable: 'nameKeepRegexState',
       description: NAME_REGEX_KEEP_DESCRIPTION,
     },
   ],
@@ -111,16 +113,34 @@ export default {
     policyEnabledText() {
       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 {
-        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() {
       return (
-        this.textAreaState.nameRegexState !== false &&
-        this.textAreaState.nameKeepRegexState !== false
+        this.textAreaValidation.name_regex.state !== false &&
+        this.textAreaValidation.name_regex_keep.state !== false
       );
     },
     isFormElementDisabled() {
@@ -140,8 +160,11 @@ export default {
     },
   },
   methods: {
-    validateNameRegex(value) {
-      return value ? value.length <= NAME_REGEX_LENGTH : null;
+    validateRegexLength(value) {
+      if (!value) {
+        return null;
+      }
+      return value.length <= NAME_REGEX_LENGTH ? '' : TEXT_AREA_INVALID_FEEDBACK;
     },
     idGenerator(id) {
       return `${id}_${this.uniqueId}`;
@@ -160,7 +183,7 @@ export default {
       :label-cols="labelCols"
       :label-align="labelAlign"
       :label-for="idGenerator('expiration-policy-toggle')"
-      :label="$options.i18n.enableToggleLabel"
+      :label="$options.i18n.ENABLE_TOGGLE_LABEL"
     >
       <div class="d-flex align-items-start">
         <gl-toggle
@@ -169,7 +192,7 @@ export default {
           :disabled="isLoading"
         />
         <span class="mb-2 ml-2 lh-2">
-          <gl-sprintf :message="$options.i18n.enableToggleDescription">
+          <gl-sprintf :message="$options.i18n.ENABLE_TOGGLE_DESCRIPTION">
             <template #toggleStatus>
               <strong>{{ policyEnabledText }}</strong>
             </template>
@@ -210,8 +233,8 @@ export default {
       :label-cols="labelCols"
       :label-align="labelAlign"
       :label-for="idGenerator(textarea.name)"
-      :state="textAreaState[textarea.stateVariable]"
-      :invalid-feedback="$options.i18n.textAreaInvalidFeedback"
+      :state="textAreaValidation[textarea.model].state"
+      :invalid-feedback="textAreaValidation[textarea.model].message"
     >
       <template #label>
         <gl-sprintf :message="textarea.label">
@@ -224,7 +247,7 @@ export default {
         :id="idGenerator(textarea.name)"
         :value="value[textarea.model]"
         :placeholder="textarea.placeholder"
-        :state="textAreaState[textarea.stateVariable]"
+        :state="textAreaValidation[textarea.model].state"
         :disabled="isFormElementDisabled"
         trim
         @input="updateModel($event, textarea.model)"

app/assets/javascripts/registry/shared/constants.js

@@ -23,7 +23,7 @@ export const ENABLE_TOGGLE_DESCRIPTION = 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:');

app/assets/javascripts/registry/shared/utils.js

@@ -11,7 +11,7 @@ export const mapComputedToEvent = (list, root) => {
         return this[root][e];
       },
       set(value) {
-        this.$emit('input', { ...this[root], [e]: value });
+        this.$emit('input', { newValue: { ...this[root], [e]: value }, modified: e });
       },
     };
   });

changelogs/unreleased/220413-quickly-resolve-issues-with-your-cleanup-policy-with-improved-vali.yml

+---
+title: 'Cleanup policies: display API error messages under form field'
+merge_request: 36190
+author:
+type: changed

locale/gitlab.pot

@@ -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."
 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 ""
 
 msgid "ContainerRegistry|There are no container images available in this group"

spec/frontend/registry/settings/components/settings_form_spec.js

@@ -7,6 +7,7 @@ import {
   UPDATE_SETTINGS_ERROR_MESSAGE,
   UPDATE_SETTINGS_SUCCESS_MESSAGE,
 } from '~/registry/shared/constants';
+import waitForPromises from 'helpers/wait_for_promises';
 import { stringifiedFormOptions } from '../../shared/mock_data';
 
 describe('Settings Form', () => {
@@ -36,12 +37,17 @@ describe('Settings Form', () => {
   const findSaveButton = () => wrapper.find({ ref: 'save-button' });
   const findLoadingIcon = (parent = wrapper) => parent.find(GlLoadingIcon);
 
-  const mountComponent = () => {
+  const mountComponent = (data = {}) => {
     wrapper = shallowMount(component, {
       stubs: {
         GlCard,
         GlLoadingIcon,
       },
+      data() {
+        return {
+          ...data,
+        };
+      },
       mocks: {
         $toast: {
           show: jest.fn(),
@@ -55,7 +61,6 @@ describe('Settings Form', () => {
     store = createStore();
     store.dispatch('setInitialState', stringifiedFormOptions);
     dispatchSpy = jest.spyOn(store, 'dispatch');
-    mountComponent();
     jest.spyOn(Tracking, 'event');
   });
 
@@ -63,20 +68,30 @@ describe('Settings Form', () => {
     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', () => {
     let form;
     beforeEach(() => {
+      mountComponent();
       form = findForm();
       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', () => {
       beforeEach(() => {
         form.trigger('reset');
@@ -108,24 +123,40 @@ describe('Settings Form', () => {
         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();
         form.trigger('submit');
-        return wrapper.vm.$nextTick().then(() => {
-          expect(wrapper.vm.$toast.show).toHaveBeenCalledWith(UPDATE_SETTINGS_SUCCESS_MESSAGE, {
-            type: 'success',
-          });
+        await waitForPromises();
+        expect(wrapper.vm.$toast.show).toHaveBeenCalledWith(UPDATE_SETTINGS_SUCCESS_MESSAGE, {
+          type: 'success',
         });
       });
 
-      it('show an error toast when submit fails', () => {
-        dispatchSpy.mockRejectedValue();
-        form.trigger('submit');
-        return wrapper.vm.$nextTick().then(() => {
+      describe('when submit fails', () => {
+        it('shows an error', async () => {
+          dispatchSpy.mockRejectedValue({ response: {} });
+          form.trigger('submit');
+          await waitForPromises();
           expect(wrapper.vm.$toast.show).toHaveBeenCalledWith(UPDATE_SETTINGS_ERROR_MESSAGE, {
             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', () => {
     describe('cancel button', () => {
       beforeEach(() => {
         store.commit('SET_SETTINGS', { foo: 'bar' });
+        mountComponent();
       });
 
       it('has type reset', () => {
@@ -165,6 +197,7 @@ describe('Settings Form', () => {
     describe('when isLoading is true', () => {
       beforeEach(() => {
         store.commit('TOGGLE_LOADING');
+        mountComponent();
       });
       afterEach(() => {
         store.commit('TOGGLE_LOADING');

spec/frontend/registry/shared/components/__snapshots__/expiration_policy_fields_spec.js.snap

@@ -114,7 +114,6 @@ exports[`Expiration Policy Form renders 1`] = `
    
   <gl-form-group-stub
     id="expiration-policy-name-matching-group"
-    invalid-feedback="The value of this input should be less than 255 characters"
     label-align="right"
     label-cols="3"
     label-for="expiration-policy-name-matching"
@@ -131,7 +130,6 @@ exports[`Expiration Policy Form renders 1`] = `
   </gl-form-group-stub>
   <gl-form-group-stub
     id="expiration-policy-keep-name-group"
-    invalid-feedback="The value of this input should be less than 255 characters"
     label-align="right"
     label-cols="3"
     label-for="expiration-policy-keep-name"

spec/frontend/registry/shared/components/expiration_policy_fields_spec.js

@@ -94,7 +94,9 @@ describe('Expiration Policy Form', () => {
           : 'input';
         element.vm.$emit(modelUpdateEvent, value);
         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', () => {
   });
 
   describe.each`
-    modelName            | elementName        | stateVariable
-    ${'name_regex'}      | ${'name-matching'} | ${'nameRegexState'}
-    ${'name_regex_keep'} | ${'keep-name'}     | ${'nameKeepRegexState'}
-  `('regex textarea validation', ({ modelName, elementName, stateVariable }) => {
-    describe(`when name regex is longer than ${NAME_REGEX_LENGTH}`, () => {
-      const invalidString = new Array(NAME_REGEX_LENGTH + 2).join(',');
-
-      beforeEach(() => {
-        mountComponent({ value: { [modelName]: invalidString } });
+    modelName            | elementName
+    ${'name_regex'}      | ${'name-matching'}
+    ${'name_regex_keep'} | ${'keep-name'}
+  `('regex textarea validation', ({ modelName, elementName }) => {
+    const invalidString = new Array(NAME_REGEX_LENGTH + 2).join(',');
+
+    describe('when apiError contains an error message', () => {
+      const errorMessage = 'something went wrong';
+
+      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`, () => {
-        expect(wrapper.vm.textAreaState[stateVariable]).toBe(false);
-      });
-
-      it('emit the @invalidated event', () => {
-        expect(wrapper.emitted('invalidated')).toBeTruthy();
+      it('gives precedence to API errors compared to local ones', () => {
+        mountComponent({
+          apiErrors: { [modelName]: errorMessage },
+          value: { [modelName]: invalidString },
+        });
+        expect(findFormGroup(elementName).attributes('invalid-feedback')).toBe(errorMessage);
       });
     });
 
-    it('if the user did not type validation is null', () => {
-      mountComponent({ value: { [modelName]: '' } });
-      return wrapper.vm.$nextTick().then(() => {
-        expect(wrapper.vm.textAreaState[stateVariable]).toBe(null);
+    describe('when apiErrors is empty', () => {
+      it('if the user did not type validation is null', async () => {
+        mountComponent({ value: { [modelName]: '' } });
+        expect(findFormGroup(elementName).attributes('state')).toBeUndefined();
         expect(wrapper.emitted('validated')).toBeTruthy();
       });
-    });
 
-    it(`if the user typed and is less than ${NAME_REGEX_LENGTH} state is true`, () => {
-      mountComponent({ value: { [modelName]: 'foo' } });
-      return wrapper.vm.$nextTick().then(() => {
+      it(`if the user typed and is less than ${NAME_REGEX_LENGTH} state is true`, () => {
+        mountComponent({ value: { [modelName]: 'foo' } });
+
         const formGroup = findFormGroup(elementName);
         const formElement = findFormElements(elementName, formGroup);
         expect(formGroup.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();
+        });
+      });
     });
   });