Merge branch 'nfriend-make-tag-name-required-on-new-release-page' into 'master'

Update "Tag name" field to be required on the "New Release" page

See merge request gitlab-org/gitlab!38951

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

 <script>
-import { mapState, mapActions } from 'vuex';
+import { mapState, mapActions, mapGetters } from 'vuex';
 import { GlFormGroup, GlFormInput } from '@gitlab/ui';
 import { uniqueId } from 'lodash';
 import { __ } from '~/locale';
@@ -9,8 +9,17 @@ import FormFieldContainer from './form_field_container.vue';
 export default {
   name: 'TagFieldNew',
   components: { GlFormGroup, GlFormInput, RefSelector, FormFieldContainer },
+  data() {
+    return {
+      // Keeps track of whether or not the user has interacted with
+      // the input field. This is used to avoid showing validation
+      // errors immediately when the page loads.
+      isInputDirty: false,
+    };
+  },
   computed: {
     ...mapState('detail', ['projectId', 'release', 'createFrom']),
+    ...mapGetters('detail', ['validationErrors']),
     tagName: {
       get() {
         return this.release.tagName;
@@ -27,6 +36,9 @@ export default {
         this.updateCreateFrom(createFrom);
       },
     },
+    showTagNameValidationError() {
+      return this.isInputDirty && this.validationErrors.isTagNameEmpty;
+    },
     tagNameInputId() {
       return uniqueId('tag-name-input-');
     },
@@ -36,6 +48,9 @@ export default {
   },
   methods: {
     ...mapActions('detail', ['updateReleaseTagName', 'updateCreateFrom']),
+    markInputAsDirty() {
+      this.isInputDirty = true;
+    },
   },
   translations: {
     noRefSelected: __('No source selected'),
@@ -46,9 +61,22 @@ export default {
 </script>
 <template>
   <div>
-    <gl-form-group :label="__('Tag name')" :label-for="tagNameInputId" data-testid="tag-name-field">
+    <gl-form-group
+      :label="__('Tag name')"
+      :label-for="tagNameInputId"
+      data-testid="tag-name-field"
+      :state="!showTagNameValidationError"
+      :invalid-feedback="__('Tag name is required')"
+    >
       <form-field-container>
-        <gl-form-input :id="tagNameInputId" v-model="tagName" type="text" class="form-control" />
+        <gl-form-input
+          :id="tagNameInputId"
+          v-model="tagName"
+          :state="!showTagNameValidationError"
+          type="text"
+          class="form-control"
+          @blur.once="markInputAsDirty"
+        />
       </form-field-container>
     </gl-form-group>
     <gl-form-group

app/assets/javascripts/releases/stores/modules/detail/getters.js

@@ -47,6 +47,10 @@ export const validationErrors = state => {
     return errors;
   }
 
+  if (!state.release.tagName?.trim?.().length) {
+    errors.isTagNameEmpty = true;
+  }
+
   // Each key of this object is a URL, and the value is an
   // array of Release link objects that share this URL.
   // This is used for detecting duplicate URLs.
@@ -96,5 +100,6 @@ export const validationErrors = state => {
 
 /** Returns whether or not the release object is valid */
 export const isValid = (_state, getters) => {
-  return Object.values(getters.validationErrors.assets.links).every(isEmpty);
+  const errors = getters.validationErrors;
+  return Object.values(errors.assets.links).every(isEmpty) && !errors.isTagNameEmpty;
 };

locale/gitlab.pot

@@ -23677,6 +23677,9 @@ msgstr ""
 msgid "Tag name"
 msgstr ""
 
+msgid "Tag name is required"
+msgstr ""
+
 msgid "Tag this commit."
 msgstr ""
 

spec/frontend/releases/components/tag_field_new_spec.js

-import { shallowMount } from '@vue/test-utils';
+import { mount, shallowMount } from '@vue/test-utils';
 import { GlFormInput } from '@gitlab/ui';
 import TagFieldNew from '~/releases/components/tag_field_new.vue';
 import createStore from '~/releases/stores';
@@ -16,6 +16,9 @@ describe('releases/components/tag_field_new', () => {
   const createComponent = (mountFn = shallowMount) => {
     wrapper = mountFn(TagFieldNew, {
       store,
+      stubs: {
+        RefSelector: true,
+      },
     });
   };
 
@@ -32,6 +35,9 @@ describe('releases/components/tag_field_new', () => {
 
     store.state.detail.release = {
       tagName: TEST_TAG_NAME,
+      assets: {
+        links: [],
+      },
     };
   });
 
@@ -42,24 +48,76 @@ describe('releases/components/tag_field_new', () => {
 
   const findTagNameFormGroup = () => wrapper.find('[data-testid="tag-name-field"]');
   const findTagNameGlInput = () => findTagNameFormGroup().find(GlFormInput);
+  const findTagNameInput = () => findTagNameFormGroup().find('input');
 
   const findCreateFromFormGroup = () => wrapper.find('[data-testid="create-from-field"]');
   const findCreateFromDropdown = () => findCreateFromFormGroup().find(RefSelector);
 
   describe('"Tag name" field', () => {
-    beforeEach(createComponent);
+    describe('rendering and behavior', () => {
+      beforeEach(createComponent);
 
-    it('renders a label', () => {
-      expect(findTagNameFormGroup().attributes().label).toBe('Tag name');
+      it('renders a label', () => {
+        expect(findTagNameFormGroup().attributes().label).toBe('Tag name');
+      });
+
+      describe('when the user updates the field', () => {
+        it("updates the store's release.tagName property", () => {
+          const updatedTagName = 'updated-tag-name';
+          findTagNameGlInput().vm.$emit('input', updatedTagName);
+
+          return wrapper.vm.$nextTick().then(() => {
+            expect(store.state.detail.release.tagName).toBe(updatedTagName);
+          });
+        });
+      });
     });
 
-    describe('when the user updates the field', () => {
-      it("updates the store's release.tagName property", () => {
-        const updatedTagName = 'updated-tag-name';
-        findTagNameGlInput().vm.$emit('input', updatedTagName);
+    describe('validation', () => {
+      beforeEach(() => {
+        createComponent(mount);
+      });
 
+      /**
+       * Utility function to test the visibility of the validation message
+       * @param {'shown' | 'hidden'} state The expected state of the validation message.
+       * Should be passed either 'shown' or 'hidden'
+       */
+      const expectValidationMessageToBe = state => {
         return wrapper.vm.$nextTick().then(() => {
-          expect(store.state.detail.release.tagName).toBe(updatedTagName);
+          expect(findTagNameFormGroup().element).toHaveClass(
+            state === 'shown' ? 'is-invalid' : 'is-valid',
+          );
+          expect(findTagNameFormGroup().element).not.toHaveClass(
+            state === 'shown' ? 'is-valid' : 'is-invalid',
+          );
+        });
+      };
+
+      describe('when the user has not yet interacted with the component', () => {
+        it('does not display a validation error', () => {
+          findTagNameInput().setValue('');
+
+          return expectValidationMessageToBe('hidden');
+        });
+      });
+
+      describe('when the user has interacted with the component and the value is not empty', () => {
+        it('does not display validation error', () => {
+          findTagNameInput().trigger('blur');
+
+          return expectValidationMessageToBe('hidden');
+        });
+      });
+
+      describe('when the user has interacted with the component and the value is empty', () => {
+        it('displays a validation error', () => {
+          const tagNameInput = findTagNameInput();
+
+          tagNameInput.setValue('');
+          tagNameInput.trigger('blur');
+
+          return expectValidationMessageToBe('shown');
         });
       });
     });

spec/frontend/releases/stores/modules/detail/getters_spec.js

@@ -76,6 +76,7 @@ describe('Release detail getters', () => {
       it('returns no validation errors', () => {
         const state = {
           release: {
+            tagName: 'test-tag-name',
             assets: {
               links: [
                 { id: 1, url: 'https://example.com/valid', name: 'Link 1' },
@@ -110,6 +111,9 @@ describe('Release detail getters', () => {
       beforeEach(() => {
         const state = {
           release: {
+            // empty tag name
+            tagName: '',
+
             assets: {
               links: [
                 // Duplicate URLs
@@ -138,7 +142,15 @@ describe('Release detail getters', () => {
         actualErrors = getters.validationErrors(state);
       });
 
-      it('returns a validation errors if links share a URL', () => {
+      it('returns a validation error if the tag name is empty', () => {
+        const expectedErrors = {
+          isTagNameEmpty: true,
+        };
+
+        expect(actualErrors).toMatchObject(expectedErrors);
+      });
+
+      it('returns a validation error if links share a URL', () => {
         const expectedErrors = {
           assets: {
             links: {
@@ -196,32 +208,53 @@ describe('Release detail getters', () => {
     // the value of state is not actually used by this getter
     const state = {};
 
-    it('returns true when the form is valid', () => {
-      const mockGetters = {
-        validationErrors: {
-          assets: {
-            links: {
-              1: {},
+    describe('when the form is valid', () => {
+      it('returns true', () => {
+        const mockGetters = {
+          validationErrors: {
+            assets: {
+              links: {
+                1: {},
+              },
             },
           },
-        },
-      };
+        };
 
-      expect(getters.isValid(state, mockGetters)).toBe(true);
+        expect(getters.isValid(state, mockGetters)).toBe(true);
+      });
     });
 
-    it('returns false when the form is invalid', () => {
-      const mockGetters = {
-        validationErrors: {
-          assets: {
-            links: {
-              1: { isNameEmpty: true },
+    describe('when an asset link contains a validation error', () => {
+      it('returns false', () => {
+        const mockGetters = {
+          validationErrors: {
+            assets: {
+              links: {
+                1: { isNameEmpty: true },
+              },
             },
           },
-        },
-      };
+        };
 
-      expect(getters.isValid(state, mockGetters)).toBe(false);
+        expect(getters.isValid(state, mockGetters)).toBe(false);
+      });
+    });
+
+    describe('when the tag name is empty', () => {
+      it('returns false', () => {
+        const mockGetters = {
+          validationErrors: {
+            isTagNameEmpty: true,
+            assets: {
+              links: {
+                1: {},
+              },
+            },
+          },
+        };
+
+        expect(getters.isValid(state, mockGetters)).toBe(false);
+      });
     });
   });
 });