Merge branch 'nfriend-prevent-submission-of-invalid-form-on-new-release-page' into 'master'

Modify submission behavior of the New/Edit Release page

See merge request gitlab-org/gitlab!39145

app/assets/javascripts/lib/utils/keys.js

@@ -2,3 +2,4 @@
 
 export const ESC_KEY = 'Escape';
 export const ESC_KEY_IE11 = 'Esc'; // https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
+export const ENTER_KEY = 'Enter';

app/assets/javascripts/releases/components/app_edit_new.vue

@@ -85,7 +85,7 @@ export default {
     saveButtonLabel() {
       return this.isExistingRelease ? __('Save changes') : __('Create release');
     },
-    isSaveChangesDisabled() {
+    isFormSubmissionDisabled() {
       return this.isUpdatingRelease || !this.isValid;
     },
     milestoneComboboxExtraLinks() {
@@ -116,13 +116,18 @@ export default {
       'updateReleaseNotes',
       'updateReleaseMilestones',
     ]),
+    submitForm() {
+      if (!this.isFormSubmissionDisabled) {
+        this.saveRelease();
+      }
+    },
   },
 };
 </script>
 <template>
   <div class="d-flex flex-column">
     <p class="pt-3 js-subtitle-text" v-html="subtitleText"></p>
-    <form v-if="showForm" @submit.prevent="saveRelease()">
+    <form v-if="showForm" class="js-quick-submit" @submit.prevent="submitForm">
       <tag-field />
       <gl-form-group>
         <label for="release-title">{{ __('Release title') }}</label>
@@ -134,7 +139,7 @@ export default {
           class="form-control"
         />
       </gl-form-group>
-      <gl-form-group class="w-50">
+      <gl-form-group class="w-50" @keydown.enter.prevent.capture>
         <label>{{ __('Milestones') }}</label>
         <div class="d-flex flex-column col-md-6 col-sm-10 pl-0">
           <milestone-combobox
@@ -163,8 +168,6 @@ export default {
                 data-supports-quick-actions="false"
                 :aria-label="__('Release notes')"
                 :placeholder="__('Write your release notes or drag your files here…')"
-                @keydown.meta.enter="saveRelease()"
-                @keydown.ctrl.enter="saveRelease()"
               ></textarea>
             </template>
           </markdown-field>
@@ -179,7 +182,7 @@ export default {
           category="primary"
           variant="success"
           type="submit"
-          :disabled="isSaveChangesDisabled"
+          :disabled="isFormSubmissionDisabled"
           data-testid="submit-button"
         >
           {{ saveButtonLabel }}

app/assets/javascripts/releases/components/asset_links_form.vue

@@ -49,6 +49,12 @@ export default {
       this.removeAssetLink(linkId);
       this.ensureAtLeastOneLink();
     },
+    updateUrl(link, newUrl) {
+      this.updateAssetLinkUrl({ linkIdToUpdate: link.id, newUrl });
+    },
+    updateName(link, newName) {
+      this.updateAssetLinkName({ linkIdToUpdate: link.id, newName });
+    },
     hasDuplicateUrl(link) {
       return Boolean(this.getLinkErrors(link).isDuplicate);
     },
@@ -138,7 +144,9 @@ export default {
           type="text"
           class="form-control"
           :state="isUrlValid(link)"
-          @change="updateAssetLinkUrl({ linkIdToUpdate: link.id, newUrl: $event })"
+          @change="updateUrl(link, $event)"
+          @keydown.ctrl.enter="updateUrl(link, $event.target.value)"
+          @keydown.meta.enter="updateUrl(link, $event.target.value)"
         />
         <template #invalid-feedback>
           <span v-if="hasEmptyUrl(link)" class="invalid-feedback d-inline">
@@ -175,7 +183,9 @@ export default {
           type="text"
           class="form-control"
           :state="isNameValid(link)"
-          @change="updateAssetLinkName({ linkIdToUpdate: link.id, newName: $event })"
+          @change="updateName(link, $event)"
+          @keydown.ctrl.enter="updateName(link, $event.target.value)"
+          @keydown.meta.enter="updateName(link, $event.target.value)"
         />
         <template #invalid-feedback>
           <span v-if="hasEmptyName(link)" class="invalid-feedback d-inline">

changelogs/unreleased/nfriend-prevent-submission-of-invalid-form-on-new-release-page.yml

+---
+title: Improve submission behavior of the New/Edit Release page
+merge_request: 39145
+author:
+type: added

spec/frontend/releases/components/app_edit_new_spec.js

@@ -83,11 +83,10 @@ describe('Release edit/new component', () => {
   });
 
   const findSubmitButton = () => wrapper.find('button[type=submit]');
+  const findForm = () => wrapper.find('form');
 
   describe(`basic functionality tests: all tests unrelated to the "${BACK_URL_PARAM}" parameter`, () => {
-    beforeEach(() => {
-      factory();
-    });
+    beforeEach(factory);
 
     it('calls initializeRelease when the component is created', () => {
       expect(actions.initializeRelease).toHaveBeenCalledTimes(1);
@@ -122,15 +121,14 @@ describe('Release edit/new component', () => {
     });
 
     it('calls saveRelease when the form is submitted', () => {
-      wrapper.find('form').trigger('submit');
+      findForm().trigger('submit');
+
       expect(actions.saveRelease).toHaveBeenCalledTimes(1);
     });
   });
 
   describe(`when the URL does not contain a "${BACK_URL_PARAM}" parameter`, () => {
-    beforeEach(() => {
-      factory();
-    });
+    beforeEach(factory);
 
     it(`renders a "Cancel" button with an href pointing to "${BACK_URL_PARAM}"`, () => {
       const cancelButton = wrapper.find('.js-cancel-button');
@@ -246,6 +244,12 @@ describe('Release edit/new component', () => {
       it('renders the submit button as disabled', () => {
         expect(findSubmitButton().attributes('disabled')).toBe('disabled');
       });
+
+      it('does not allow the form to be submitted', () => {
+        findForm().trigger('submit');
+
+        expect(actions.saveRelease).not.toHaveBeenCalled();
+      });
     });
   });
 });

spec/frontend/releases/components/asset_links_form_spec.js

@@ -3,6 +3,7 @@ import { mount, createLocalVue } from '@vue/test-utils';
 import AssetLinksForm from '~/releases/components/asset_links_form.vue';
 import { release as originalRelease } from '../mock_data';
 import * as commonUtils from '~/lib/utils/common_utils';
+import { ENTER_KEY } from '~/lib/utils/keys';
 import { ASSET_LINK_TYPE, DEFAULT_ASSET_LINK_TYPE } from '~/releases/constants';
 
 const localVue = createLocalVue();
@@ -91,42 +92,128 @@ describe('Release edit component', () => {
       expect(actions.removeAssetLink).toHaveBeenCalledTimes(1);
     });
 
-    it('calls the "updateAssetLinkUrl" store method when text is entered into the "URL" input field', () => {
-      const linkIdToUpdate = release.assets.links[0].id;
-      const newUrl = 'updated url';
+    describe('URL input field', () => {
+      let input;
+      let linkIdToUpdate;
+      let newUrl;
 
-      expect(actions.updateAssetLinkUrl).not.toHaveBeenCalled();
+      beforeEach(() => {
+        input = wrapper.find({ ref: 'urlInput' }).element;
+        linkIdToUpdate = release.assets.links[0].id;
+        newUrl = 'updated url';
+      });
 
-      wrapper.find({ ref: 'urlInput' }).vm.$emit('change', newUrl);
+      const expectStoreMethodNotToBeCalled = () => {
+        expect(actions.updateAssetLinkUrl).not.toHaveBeenCalled();
+      };
 
-      expect(actions.updateAssetLinkUrl).toHaveBeenCalledTimes(1);
-      expect(actions.updateAssetLinkUrl).toHaveBeenCalledWith(
-        expect.anything(),
-        {
-          linkIdToUpdate,
-          newUrl,
-        },
-        undefined,
-      );
+      const dispatchKeydowEvent = eventParams => {
+        const event = new KeyboardEvent('keydown', eventParams);
+
+        input.dispatchEvent(event);
+      };
+
+      const expectStoreMethodToBeCalled = () => {
+        expect(actions.updateAssetLinkUrl).toHaveBeenCalledTimes(1);
+        expect(actions.updateAssetLinkUrl).toHaveBeenCalledWith(
+          expect.anything(),
+          {
+            linkIdToUpdate,
+            newUrl,
+          },
+          undefined,
+        );
+      };
+
+      it('calls the "updateAssetLinkUrl" store method when text is entered into the "URL" input field', () => {
+        expectStoreMethodNotToBeCalled();
+
+        wrapper.find({ ref: 'urlInput' }).vm.$emit('change', newUrl);
+
+        expectStoreMethodToBeCalled();
+      });
+
+      it('calls the "updateAssetLinkUrl" store method when Ctrl+Enter is pressed inside the "URL" input field', () => {
+        expectStoreMethodNotToBeCalled();
+
+        input.value = newUrl;
+
+        dispatchKeydowEvent({ key: ENTER_KEY, ctrlKey: true });
+
+        expectStoreMethodToBeCalled();
+      });
+
+      it('calls the "updateAssetLinkUrl" store method when Cmd+Enter is pressed inside the "URL" input field', () => {
+        expectStoreMethodNotToBeCalled();
+
+        input.value = newUrl;
+
+        dispatchKeydowEvent({ key: ENTER_KEY, metaKey: true });
+
+        expectStoreMethodToBeCalled();
+      });
     });
 
-    it('calls the "updateAssetLinkName" store method when text is entered into the "Link title" input field', () => {
-      const linkIdToUpdate = release.assets.links[0].id;
-      const newName = 'updated name';
+    describe('Link title field', () => {
+      let input;
+      let linkIdToUpdate;
+      let newName;
 
-      expect(actions.updateAssetLinkName).not.toHaveBeenCalled();
+      beforeEach(() => {
+        input = wrapper.find({ ref: 'nameInput' }).element;
+        linkIdToUpdate = release.assets.links[0].id;
+        newName = 'updated name';
+      });
 
-      wrapper.find({ ref: 'nameInput' }).vm.$emit('change', newName);
+      const expectStoreMethodNotToBeCalled = () => {
+        expect(actions.updateAssetLinkUrl).not.toHaveBeenCalled();
+      };
 
-      expect(actions.updateAssetLinkName).toHaveBeenCalledTimes(1);
-      expect(actions.updateAssetLinkName).toHaveBeenCalledWith(
-        expect.anything(),
-        {
-          linkIdToUpdate,
-          newName,
-        },
-        undefined,
-      );
+      const dispatchKeydowEvent = eventParams => {
+        const event = new KeyboardEvent('keydown', eventParams);
+
+        input.dispatchEvent(event);
+      };
+
+      const expectStoreMethodToBeCalled = () => {
+        expect(actions.updateAssetLinkName).toHaveBeenCalledTimes(1);
+        expect(actions.updateAssetLinkName).toHaveBeenCalledWith(
+          expect.anything(),
+          {
+            linkIdToUpdate,
+            newName,
+          },
+          undefined,
+        );
+      };
+
+      it('calls the "updateAssetLinkName" store method when text is entered into the "Link title" input field', () => {
+        expectStoreMethodNotToBeCalled();
+
+        wrapper.find({ ref: 'nameInput' }).vm.$emit('change', newName);
+
+        expectStoreMethodToBeCalled();
+      });
+
+      it('calls the "updateAssetLinkName" store method when Ctrl+Enter is pressed inside the "Link title" input field', () => {
+        expectStoreMethodNotToBeCalled();
+
+        input.value = newName;
+
+        dispatchKeydowEvent({ key: ENTER_KEY, ctrlKey: true });
+
+        expectStoreMethodToBeCalled();
+      });
+
+      it('calls the "updateAssetLinkName" store method when Cmd+Enter is pressed inside the "Link title" input field', () => {
+        expectStoreMethodNotToBeCalled();
+
+        input.value = newName;
+
+        dispatchKeydowEvent({ key: ENTER_KEY, metaKey: true });
+
+        expectStoreMethodToBeCalled();
+      });
     });
 
     it('calls the "updateAssetLinkType" store method when an option is selected from the "Type" dropdown', () => {