Merge branch 'dpisek-on-demand-scans-site-profile-use-constraints-api' into 'master'

DAST on-demand scans site profile form - refactor form validation

See merge request gitlab-org/gitlab!45488

app/assets/javascripts/vue_shared/directives/validation.js

+import { merge } from 'lodash';
+import { s__ } from '~/locale';
+
+/**
+ * Validation messages will take priority based on the property order.
+ *
+ * For example:
+ * { valueMissing: {...}, urlTypeMismatch: {...} }
+ *
+ * `valueMissing` will be displayed the user has entered a value
+ *  after that, if the input is not a valid URL then `urlTypeMismatch` will show
+ */
+const defaultFeedbackMap = {
+  valueMissing: {
+    isInvalid: el => el.validity?.valueMissing,
+    message: s__('Please fill out this field.'),
+  },
+  urlTypeMismatch: {
+    isInvalid: el => el.type === 'url' && el.validity?.typeMismatch,
+    message: s__('Please enter a valid URL format, ex: http://www.example.com/home'),
+  },
+};
+
+const getFeedbackForElement = (feedbackMap, el) =>
+  Object.values(feedbackMap).find(f => f.isInvalid(el))?.message || el.validationMessage;
+
+const focusFirstInvalidInput = e => {
+  const { target: formEl } = e;
+  const invalidInput = formEl.querySelector('input:invalid');
+
+  if (invalidInput) {
+    invalidInput.focus();
+  }
+};
+
+const isEveryFieldValid = form => Object.values(form.fields).every(({ state }) => state === true);
+
+const createValidator = (context, feedbackMap) => ({ el, reportInvalidInput = false }) => {
+  const { form } = context;
+  const { name } = el;
+
+  if (!name) {
+    if (process.env.NODE_ENV === 'development') {
+      // eslint-disable-next-line no-console
+      console.warn(
+        '[gitlab] the validation directive requires the given input to have "name" attribute',
+      );
+    }
+    return;
+  }
+
+  const formField = form.fields[name];
+  const isValid = el.checkValidity();
+
+  // This makes sure we always report valid fields - this can be useful for cases where the consuming
+  // component's logic depends on certain fields being in a valid state.
+  // Invalid input, on the other hand, should only be reported once we want to display feedback to the user.
+  // (eg.: After a field has been touched and moved away from, a submit-button has been clicked, ...)
+  formField.state = reportInvalidInput ? isValid : isValid || null;
+  formField.feedback = reportInvalidInput ? getFeedbackForElement(feedbackMap, el) : '';
+
+  form.state = isEveryFieldValid(form);
+};
+
+/**
+ * Takes an object that allows to add or change custom feedback messages.
+ *
+ * The passed in object will be merged with the built-in feedback
+ * so it is possible to override a built-in message.
+ *
+ * @example
+ * validate({
+ *   tooLong: {
+ *     check: el => el.validity.tooLong === true,
+ *     message: 'Your custom feedback'
+ *   }
+ * })
+ *
+ * @example
+ *   validate({
+ *     valueMissing: {
+ *       message: 'Your custom feedback'
+ *     }
+ *   })
+ *
+ * @param {Object<string, { message: string, isValid: ?function}>} customFeedbackMap
+ * @returns {{ inserted: function, update: function }} validateDirective
+ */
+export default function(customFeedbackMap = {}) {
+  const feedbackMap = merge(defaultFeedbackMap, customFeedbackMap);
+  const elDataMap = new WeakMap();
+
+  return {
+    inserted(el, binding, { context }) {
+      const { arg: showGlobalValidation } = binding;
+      const { form: formEl } = el;
+
+      const validate = createValidator(context, feedbackMap);
+      const elData = { validate, isTouched: false, isBlurred: false };
+
+      elDataMap.set(el, elData);
+
+      el.addEventListener('input', function markAsTouched() {
+        elData.isTouched = true;
+        // once the element has been marked as touched we can stop listening on the 'input' event
+        el.removeEventListener('input', markAsTouched);
+      });
+
+      el.addEventListener('blur', function markAsBlurred({ target }) {
+        if (elData.isTouched) {
+          elData.isBlurred = true;
+          validate({ el: target, reportInvalidInput: true });
+          // this event handler can be removed, since the live-feedback in `update` takes over
+          el.removeEventListener('blur', markAsBlurred);
+        }
+      });
+
+      if (formEl) {
+        formEl.addEventListener('submit', focusFirstInvalidInput);
+      }
+
+      validate({ el, reportInvalidInput: showGlobalValidation });
+    },
+    update(el, binding) {
+      const { arg: showGlobalValidation } = binding;
+      const { validate, isTouched, isBlurred } = elDataMap.get(el);
+      const showValidationFeedback = showGlobalValidation || (isTouched && isBlurred);
+
+      validate({ el, reportInvalidInput: showValidationFeedback });
+    },
+  };
+}

ee/app/assets/javascripts/security_configuration/dast_site_profiles_form/components/dast_site_profile_form.vue

@@ -12,10 +12,11 @@ import {
 } from '@gitlab/ui';
 import * as Sentry from '~/sentry/wrapper';
 import { __, s__ } from '~/locale';
-import { isAbsolute, redirectTo } from '~/lib/utils/url_utility';
-import { serializeFormObject, isEmptyValue } from '~/lib/utils/forms';
+import { redirectTo } from '~/lib/utils/url_utility';
+import { serializeFormObject } from '~/lib/utils/forms';
 import { fetchPolicies } from '~/lib/graphql';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
+import validation from '~/vue_shared/directives/validation';
 import DastSiteValidation from './dast_site_validation.vue';
 import dastSiteProfileCreateMutation from '../graphql/dast_site_profile_create.mutation.graphql';
 import dastSiteProfileUpdateMutation from '../graphql/dast_site_profile_update.mutation.graphql';
@@ -44,6 +45,9 @@ export default {
     GlToggle,
     DastSiteValidation,
   },
+  directives: {
+    validation: validation(),
+  },
   mixins: [glFeatureFlagsMixin()],
   props: {
     fullPath: {
@@ -64,14 +68,18 @@ export default {
     const { name = '', targetUrl = '' } = this.siteProfile || {};
 
     const form = {
-      profileName: initField(name),
-      targetUrl: initField(targetUrl),
+      state: false,
+      showValidation: false,
+      fields: {
+        profileName: initField(name),
+        targetUrl: initField(targetUrl),
+      },
     };
 
     return {
       fetchValidationTimeout: null,
       form,
-      initialFormValues: serializeFormObject(form),
+      initialFormValues: serializeFormObject(form.fields),
       isFetchingValidationStatus: false,
       isValidatingSite: false,
       isLoading: false,
@@ -90,7 +98,7 @@ export default {
       return Boolean(this.siteProfile?.id);
     },
     isSiteValidationDisabled() {
-      return !this.form.targetUrl.state || this.validationStatusMatches(INPROGRESS);
+      return !this.form.fields.targetUrl.state || this.validationStatusMatches(INPROGRESS);
     },
     i18n() {
       const { isEdit } = this;
@@ -119,20 +127,12 @@ export default {
       };
     },
     formTouched() {
-      return !isEqual(serializeFormObject(this.form), this.initialFormValues);
-    },
-    formHasErrors() {
-      return Object.values(this.form).some(({ state }) => state === false);
-    },
-    someFieldEmpty() {
-      return Object.values(this.form).some(({ value }) => isEmptyValue(value));
+      return !isEqual(serializeFormObject(this.form.fields), this.initialFormValues);
     },
     isSubmitDisabled() {
       return (
-        (this.isSiteValidationActive && !this.validationStatusMatches(PASSED)) ||
-        this.formHasErrors ||
-        this.someFieldEmpty ||
-        this.validationStatusMatches(INPROGRESS)
+        this.validationStatusMatches(INPROGRESS) ||
+        (this.isSiteValidationActive && !this.validationStatusMatches(PASSED))
       );
     },
     showValidationSection() {
@@ -169,9 +169,9 @@ export default {
         : defaultDescription;
     },
   },
-  async created() {
+  async mounted() {
     if (this.isEdit) {
-      this.validateTargetUrl();
+      this.form.showValidation = true;
 
       if (this.glFeatures.securityOnDemandScansSiteValidation) {
         await this.fetchValidationStatus();
@@ -213,17 +213,6 @@ export default {
     validationStatusMatches(status) {
       return this.validationStatus === status;
     },
-    validateTargetUrl() {
-      if (!isAbsolute(this.form.targetUrl.value)) {
-        this.form.targetUrl.state = false;
-        this.form.targetUrl.feedback = s__(
-          'DastProfiles|Please enter a valid URL format, ex: http://www.example.com/home',
-        );
-        return;
-      }
-      this.form.targetUrl.state = true;
-      this.form.targetUrl.feedback = null;
-    },
     async fetchValidationStatus() {
       this.isFetchingValidationStatus = true;
 
@@ -238,7 +227,7 @@ export default {
           query: dastSiteValidationQuery,
           variables: {
             fullPath: this.fullPath,
-            targetUrl: this.form.targetUrl.value,
+            targetUrl: this.form.fields.targetUrl.value,
           },
           fetchPolicy: fetchPolicies.NETWORK_ONLY,
         });
@@ -269,7 +258,10 @@ export default {
           },
         } = await this.$apollo.mutate({
           mutation: dastSiteTokenCreateMutation,
-          variables: { projectFullPath: this.fullPath, targetUrl: this.form.targetUrl.value },
+          variables: {
+            projectFullPath: this.fullPath,
+            targetUrl: this.form.fields.targetUrl.value,
+          },
         });
         if (errors.length) {
           this.showErrors({ message: errorMessage, errors });
@@ -284,6 +276,12 @@ export default {
       }
     },
     onSubmit() {
+      this.form.showValidation = true;
+
+      if (!this.form.state) {
+        return;
+      }
+
       this.isLoading = true;
       this.hideErrors();
       const { errorMessage } = this.i18n;
@@ -291,7 +289,7 @@ export default {
       const variables = {
         fullPath: this.fullPath,
         ...(this.isEdit ? { id: this.siteProfile.id } : {}),
-        ...serializeFormObject(this.form),
+        ...serializeFormObject(this.form.fields),
       };
 
       this.$apollo
@@ -351,7 +349,7 @@ export default {
 </script>
 
 <template>
-  <gl-form @submit.prevent="onSubmit">
+  <gl-form novalidate @submit.prevent="onSubmit">
     <h2 class="gl-mb-6">
       {{ i18n.title }}
     </h2>
@@ -369,12 +367,19 @@ export default {
       </ul>
     </gl-alert>
 
-    <gl-form-group :label="s__('DastProfiles|Profile name')">
+    <gl-form-group
+      :label="s__('DastProfiles|Profile name')"
+      :invalid-feedback="form.fields.profileName.feedback"
+    >
       <gl-form-input
-        v-model="form.profileName.value"
+        v-model="form.fields.profileName.value"
+        v-validation:[form.showValidation]
+        name="profileName"
         class="mw-460"
         data-testid="profile-name-input"
         type="text"
+        required
+        :state="form.fields.profileName.state"
       />
     </gl-form-group>
 
@@ -382,7 +387,7 @@ export default {
 
     <gl-form-group
       data-testid="target-url-input-group"
-      :invalid-feedback="form.targetUrl.feedback"
+      :invalid-feedback="form.fields.targetUrl.feedback"
       :description="
         isSiteValidationActive && !isValidatingSite
           ? s__('DastProfiles|Validation must be turned off to change the target URL')
@@ -391,13 +396,15 @@ export default {
       :label="s__('DastProfiles|Target URL')"
     >
       <gl-form-input
-        v-model="form.targetUrl.value"
+        v-model="form.fields.targetUrl.value"
+        v-validation:[form.showValidation]
+        name="targetUrl"
         class="mw-460"
         data-testid="target-url-input"
+        required
         type="url"
-        :state="form.targetUrl.state"
+        :state="form.fields.targetUrl.state"
         :disabled="isSiteValidationActive"
-        @input="validateTargetUrl"
       />
     </gl-form-group>
 
@@ -429,7 +436,7 @@ export default {
           :full-path="fullPath"
           :token-id="tokenId"
           :token="token"
-          :target-url="form.targetUrl.value"
+          :target-url="form.fields.targetUrl.value"
           @success="onValidationSuccess"
         />
       </gl-collapse>

ee/changelogs/unreleased/dpisek-on-demand-scans-site-profile-use-constraints-api.yml

+---
+title: DAST on-demand scans site profile form - refactor form validation
+merge_request: 45488
+author:
+type: changed

ee/spec/frontend/security_configuration/dast_site_profiles_form/components/dast_site_profile_form_spec.js

@@ -62,6 +62,11 @@ describe('DastSiteProfileForm', () => {
   const findSiteValidationToggle = () => findByTestId('dast-site-validation-toggle');
   const findDastSiteValidation = () => wrapper.find(DastSiteValidation);
 
+  const setFieldValue = async (field, value) => {
+    await field.setValue(value);
+    field.trigger('blur');
+  };
+
   const mockClientFactory = handlers => {
     const mockClient = createMockClient();
 
@@ -124,34 +129,6 @@ describe('DastSiteProfileForm', () => {
     expect(wrapper.html()).not.toBe('');
   });
 
-  describe('submit button', () => {
-    beforeEach(() => {
-      createComponent();
-    });
-
-    describe('is disabled if', () => {
-      it('form contains errors', async () => {
-        findProfileNameInput().vm.$emit('input', profileName);
-        await findTargetUrlInput().vm.$emit('input', 'invalid URL');
-        expect(findSubmitButton().props('disabled')).toBe(true);
-      });
-
-      it('at least one field is empty', async () => {
-        findProfileNameInput().vm.$emit('input', '');
-        await findTargetUrlInput().vm.$emit('input', targetUrl);
-        expect(findSubmitButton().props('disabled')).toBe(true);
-      });
-    });
-
-    describe('is enabled if', () => {
-      it('all fields are filled in and valid', async () => {
-        findProfileNameInput().vm.$emit('input', profileName);
-        await findTargetUrlInput().vm.$emit('input', targetUrl);
-        expect(findSubmitButton().props('disabled')).toBe(false);
-      });
-    });
-  });
-
   describe('target URL input', () => {
     const errorMessage = 'Please enter a valid URL format, ex: http://www.example.com/home';
 
@@ -160,15 +137,13 @@ describe('DastSiteProfileForm', () => {
     });
 
     it.each(['asd', 'example.com'])('is marked as invalid provided an invalid URL', async value => {
-      findTargetUrlInput().setValue(value);
-      await wrapper.vm.$nextTick();
+      await setFieldValue(findTargetUrlInput(), value);
 
       expect(wrapper.text()).toContain(errorMessage);
     });
 
     it('is marked as valid provided a valid URL', async () => {
-      findTargetUrlInput().setValue(targetUrl);
-      await wrapper.vm.$nextTick();
+      await setFieldValue(findTargetUrlInput(), targetUrl);
 
       expect(wrapper.text()).not.toContain(errorMessage);
     });
@@ -176,7 +151,7 @@ describe('DastSiteProfileForm', () => {
 
   describe('validation', () => {
     const enableValidationToggle = async () => {
-      await findTargetUrlInput().vm.$emit('input', targetUrl);
+      await setFieldValue(findTargetUrlInput(), targetUrl);
       await findSiteValidationToggle().vm.$emit('change', true);
     };
 
@@ -186,7 +161,7 @@ describe('DastSiteProfileForm', () => {
       ${'Edit site profile'} | ${siteProfileOne}
     `('$title with feature flag disabled', ({ siteProfile }) => {
       beforeEach(() => {
-        createComponent({
+        createFullComponent({
           provide: {
             glFeatures: { securityOnDemandScansSiteValidation: false },
           },
@@ -208,7 +183,7 @@ describe('DastSiteProfileForm', () => {
 
     describe('with feature flag enabled', () => {
       beforeEach(() => {
-        createComponent({
+        createFullComponent({
           provide: {
             glFeatures: { securityOnDemandScansSiteValidation: true },
           },
@@ -223,7 +198,7 @@ describe('DastSiteProfileForm', () => {
       it('toggle is disabled until target URL is valid', async () => {
         expect(findSiteValidationToggle().props('disabled')).toBe(true);
 
-        await findTargetUrlInput().vm.$emit('input', targetUrl);
+        await setFieldValue(findTargetUrlInput(), targetUrl);
 
         expect(findSiteValidationToggle().props('disabled')).toBe(false);
       });
@@ -238,10 +213,10 @@ describe('DastSiteProfileForm', () => {
         await enableValidationToggle();
         await waitForPromises();
 
-        expect(targetUrlInputGroup.attributes('description')).toBe(
+        expect(targetUrlInputGroup.text()).toContain(
           'Validation must be turned off to change the target URL',
         );
-        expect(targetUrlInput.attributes('disabled')).toBe('true');
+        expect(targetUrlInput.attributes('disabled')).toBe('disabled');
       });
 
       it('checks the target URLs validation status when validation is enabled', async () => {
@@ -331,11 +306,15 @@ describe('DastSiteProfileForm', () => {
     });
 
     describe('submission', () => {
+      const fillAndSubmitForm = async () => {
+        await setFieldValue(findProfileNameInput(), profileName);
+        await setFieldValue(findTargetUrlInput(), targetUrl);
+        submitForm();
+      };
+
       describe('on success', () => {
-        beforeEach(() => {
-          findProfileNameInput().vm.$emit('input', profileName);
-          findTargetUrlInput().vm.$emit('input', targetUrl);
-          submitForm();
+        beforeEach(async () => {
+          await fillAndSubmitForm();
         });
 
         it('sets loading state', () => {
@@ -361,23 +340,22 @@ describe('DastSiteProfileForm', () => {
       });
 
       describe('on top-level error', () => {
-        beforeEach(() => {
+        beforeEach(async () => {
           respondWith({
             [mutationKind]: jest.fn().mockRejectedValue(new Error('GraphQL Network Error')),
           });
 
-          const input = findTargetUrlInput();
-          input.vm.$emit('input', targetUrl);
-          submitForm();
-
-          return waitForPromises();
+          await fillAndSubmitForm();
+          await waitForPromises();
         });
 
         it('resets loading state', () => {
           expect(findSubmitButton().props('loading')).toBe(false);
         });
 
-        it('shows an error alert', () => {
+        it('shows an error alert', async () => {
+          await wrapper.vm.$nextTick();
+
           expect(findAlert().exists()).toBe(true);
         });
       });
@@ -385,16 +363,13 @@ describe('DastSiteProfileForm', () => {
       describe('on errors as data', () => {
         const errors = ['error#1', 'error#2', 'error#3'];
 
-        beforeEach(() => {
+        beforeEach(async () => {
           respondWith({
             [mutationKind]: jest.fn().mockResolvedValue(responses[mutationKind](errors)),
           });
 
-          const input = findTargetUrlInput();
-          input.vm.$emit('input', targetUrl);
-          submitForm();
-
-          return waitForPromises();
+          await fillAndSubmitForm();
+          await waitForPromises();
         });
 
         it('resets loading state', () => {

locale/gitlab.pot

@@ -8352,9 +8352,6 @@ msgstr ""
 msgid "DastProfiles|Passive"
 msgstr ""
 
-msgid "DastProfiles|Please enter a valid URL format, ex: http://www.example.com/home"
-msgstr ""
-
 msgid "DastProfiles|Please enter a valid timeout value"
 msgstr ""
 
@@ -19893,6 +19890,9 @@ msgstr ""
 msgid "Please enter a number greater than %{number} (from the project settings)"
 msgstr ""
 
+msgid "Please enter a valid URL format, ex: http://www.example.com/home"
+msgstr ""
+
 msgid "Please enter a valid number"
 msgstr ""
 
@@ -19902,6 +19902,9 @@ msgstr ""
 msgid "Please fill in a descriptive name for your group."
 msgstr ""
 
+msgid "Please fill out this field."
+msgstr ""
+
 msgid "Please follow the %{link_start}Let's Encrypt troubleshooting instructions%{link_end} to re-obtain your Let's Encrypt certificate."
 msgstr ""
 

spec/frontend/vue_shared/directives/validation_spec.js

+import { shallowMount } from '@vue/test-utils';
+import validation from '~/vue_shared/directives/validation';
+
+describe('validation directive', () => {
+  let wrapper;
+
+  const createComponent = ({ inputAttributes, showValidation } = {}) => {
+    const defaultInputAttributes = {
+      type: 'text',
+      required: true,
+    };
+
+    const component = {
+      directives: {
+        validation: validation(),
+      },
+      data() {
+        return {
+          attributes: inputAttributes || defaultInputAttributes,
+          showValidation,
+          form: {
+            state: null,
+            fields: {
+              exampleField: {
+                state: null,
+                feedback: '',
+              },
+            },
+          },
+        };
+      },
+      template: `
+        <form>
+          <input v-validation:[showValidation] name="exampleField" v-bind="attributes" />
+        </form>
+      `,
+    };
+
+    wrapper = shallowMount(component, { attachToDocument: true });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+    wrapper = null;
+  });
+
+  const getFormData = () => wrapper.vm.form;
+  const findForm = () => wrapper.find('form');
+  const findInput = () => wrapper.find('input');
+
+  describe.each([true, false])(
+    'with fields untouched and "showValidation" set to "%s"',
+    showValidation => {
+      beforeEach(() => {
+        createComponent({ showValidation });
+      });
+
+      it('sets the fields validity correctly', () => {
+        expect(getFormData().fields.exampleField).toEqual({
+          state: showValidation ? false : null,
+          feedback: showValidation ? expect.any(String) : '',
+        });
+      });
+
+      it('sets the form validity correctly', () => {
+        expect(getFormData().state).toBe(false);
+      });
+    },
+  );
+
+  describe.each`
+    inputAttributes                       | validValue          | invalidValue
+    ${{ required: true }}                 | ${'foo'}            | ${''}
+    ${{ type: 'url' }}                    | ${'http://foo.com'} | ${'foo'}
+    ${{ type: 'number', min: 1, max: 5 }} | ${3}                | ${0}
+    ${{ type: 'number', min: 1, max: 5 }} | ${3}                | ${6}
+    ${{ pattern: 'foo|bar' }}             | ${'bar'}            | ${'quz'}
+  `(
+    'with input-attributes set to $inputAttributes',
+    ({ inputAttributes, validValue, invalidValue }) => {
+      const setValueAndTriggerValidation = value => {
+        const input = findInput();
+        input.setValue(value);
+        input.trigger('blur');
+      };
+
+      beforeEach(() => {
+        createComponent({ inputAttributes });
+      });
+
+      describe('with valid value', () => {
+        beforeEach(() => {
+          setValueAndTriggerValidation(validValue);
+        });
+
+        it('sets the field to be valid', () => {
+          expect(getFormData().fields.exampleField).toEqual({
+            state: true,
+            feedback: '',
+          });
+        });
+
+        it('sets the form to be valid', () => {
+          expect(getFormData().state).toBe(true);
+        });
+      });
+
+      describe('with invalid value', () => {
+        beforeEach(() => {
+          setValueAndTriggerValidation(invalidValue);
+        });
+
+        it('sets the field to be invalid', () => {
+          expect(getFormData().fields.exampleField).toEqual({
+            state: false,
+            feedback: expect.any(String),
+          });
+          expect(getFormData().fields.exampleField.feedback.length).toBeGreaterThan(0);
+        });
+
+        it('sets the form to be invalid', () => {
+          expect(getFormData().state).toBe(false);
+        });
+
+        it('sets focus on the first invalid input when the form is submitted', () => {
+          findForm().trigger('submit');
+          expect(findInput().element).toBe(document.activeElement);
+        });
+      });
+    },
+  );
+});