Merge branch '277087-validate-duplicate-asset-link-names' into 'master'

Add frontend validation to avoid duplicate asset link names

See merge request gitlab-org/gitlab!81591

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

@@ -18,3 +18,13 @@ export const swapArrayItems = (array, leftIndex = 0, rightIndex = 0) => {
   copy[rightIndex] = temp;
   return copy;
 };
+
+/**
+ * Return an array with all duplicate items from the given array
+ *
+ * @param {Array} array - The source array
+ * @returns {Array} new array with all duplicate items
+ */
+export const getDuplicateItemsFromArray = (array) => [
+  ...new Set(array.filter((value, index) => array.indexOf(value) !== index)),
+];

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

@@ -56,6 +56,9 @@ export default {
     hasDuplicateUrl(link) {
       return Boolean(this.getLinkErrors(link).isDuplicate);
     },
+    hasDuplicateName(link) {
+      return Boolean(this.getLinkErrors(link).isTitleDuplicate);
+    },
     hasBadFormat(link) {
       return Boolean(this.getLinkErrors(link).isBadFormat);
     },
@@ -72,7 +75,7 @@ export default {
       return !this.hasDuplicateUrl(link) && !this.hasBadFormat(link) && !this.hasEmptyUrl(link);
     },
     isNameValid(link) {
-      return !this.hasEmptyName(link);
+      return !this.hasEmptyName(link) && !this.hasDuplicateName(link);
     },
 
     /**
@@ -121,7 +124,7 @@ export default {
     <p>
       {{
         __(
-          'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Duplicate URLs are not allowed.',
+          'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Each URL and link title must be unique.',
         )
       }}
     </p>
@@ -165,7 +168,7 @@ export default {
             </gl-sprintf>
           </span>
           <span v-else-if="hasDuplicateUrl(link)" class="invalid-feedback d-inline">
-            {{ __('This URL is already used for another link; duplicate URLs are not allowed') }}
+            {{ __('This URL already exists.') }}
           </span>
         </template>
       </gl-form-group>
@@ -191,6 +194,9 @@ export default {
           <span v-if="hasEmptyName(link)" class="invalid-feedback d-inline">
             {{ __('Link title is required') }}
           </span>
+          <span v-else-if="hasDuplicateName(link)" class="invalid-feedback d-inline">
+            {{ __('This title already exists.') }}
+          </span>
         </template>
       </gl-form-group>
 

app/assets/javascripts/releases/stores/modules/edit_new/actions.js

@@ -162,7 +162,7 @@ const createReleaseLink = async ({ state, link }) => {
       input: {
         projectPath: state.projectPath,
         tagName: state.tagName,
-        name: link.name,
+        name: link.name.trim(),
         url: link.url,
         linkType: link.linkType.toUpperCase(),
         directAssetPath: link.directAssetPath,

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

 import { isEmpty } from 'lodash';
 import { hasContent } from '~/lib/utils/text_utility';
+import { getDuplicateItemsFromArray } from '~/lib/utils/array_utility';
 
 /**
  * @returns {Boolean} `true` if the app is editing an existing release.
@@ -95,6 +96,17 @@ export const validationErrors = (state) => {
     }
   });
 
+  // check for duplicated Link Titles
+  const linkTitles = state.release.assets.links.map((link) => link.name.trim());
+  const duplicatedTitles = getDuplicateItemsFromArray(linkTitles);
+
+  // add a validation error for each link that shares Link Title
+  state.release.assets.links.forEach((link) => {
+    if (hasContent(link.name) && duplicatedTitles.includes(link.name.trim())) {
+      errors.assets.links[link.id].isTitleDuplicate = true;
+    }
+  });
+
   return errors;
 };
 
@@ -131,7 +143,7 @@ export const releaseCreateMutatationVariables = (state, getters) => {
       ref: state.createFrom,
       assets: {
         links: getters.releaseLinksToCreate.map(({ name, url, linkType }) => ({
-          name,
+          name: name.trim(),
           url,
           linkType: linkType.toUpperCase(),
         })),

locale/gitlab.pot

@@ -27717,7 +27717,7 @@ msgstr ""
 msgid "Pods in use"
 msgstr ""
 
-msgid "Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Duplicate URLs are not allowed."
+msgid "Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Each URL and link title must be unique."
 msgstr ""
 
 msgid "Policies"
@@ -37573,7 +37573,7 @@ msgstr ""
 msgid "This Project is currently archived and read-only. Please unarchive the project first if you want to resume Pull mirroring"
 msgstr ""
 
-msgid "This URL is already used for another link; duplicate URLs are not allowed"
+msgid "This URL already exists."
 msgstr ""
 
 msgid "This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention."
@@ -38068,6 +38068,9 @@ msgstr ""
 msgid "This suggestion already matches its content."
 msgstr ""
 
+msgid "This title already exists."
+msgstr ""
+
 msgid "This user cannot be unlocked manually from GitLab"
 msgstr ""
 

spec/frontend/lib/utils/array_utility_spec.js

@@ -29,4 +29,17 @@ describe('array_utility', () => {
       },
     );
   });
+
+  describe('getDuplicateItemsFromArray', () => {
+    it.each`
+      array                                  | result
+      ${[]}                                  | ${[]}
+      ${[1, 2, 2, 3, 3, 4]}                  | ${[2, 3]}
+      ${[1, 2, 3, 2, 3, 4]}                  | ${[2, 3]}
+      ${['foo', 'bar', 'bar', 'foo', 'baz']} | ${['bar', 'foo']}
+      ${['foo', 'foo', 'bar', 'foo', 'bar']} | ${['foo', 'bar']}
+    `('given $array will return $result', ({ array, result }) => {
+      expect(arrayUtils.getDuplicateItemsFromArray(array)).toEqual(result);
+    });
+  });
 });

spec/frontend/releases/components/asset_links_form_spec.js

@@ -256,9 +256,7 @@ describe('Release edit component', () => {
         },
       });
 
-      expect(findUrlValidationMessage().text()).toBe(
-        'This URL is already used for another link; duplicate URLs are not allowed',
-      );
+      expect(findUrlValidationMessage().text()).toBe('This URL already exists.');
     });
 
     it('shows a validation error message when a URL has a bad format', () => {

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

@@ -134,6 +134,14 @@ describe('Release edit/new getters', () => {
                 // Missing title
                 { id: 7, url: 'https://example.com/valid/1', name: '' },
                 { id: 8, url: 'https://example.com/valid/2', name: '  ' },
+
+                // Duplicate title
+                { id: 9, url: 'https://example.com/1', name: 'Link 7' },
+                { id: 10, url: 'https://example.com/2', name: 'Link 7' },
+
+                // title validation ignores leading/trailing whitespace
+                { id: 11, url: 'https://example.com/3', name: '  Link 7\t  ' },
+                { id: 12, url: 'https://example.com/4', name: ' Link 7\n\r\n ' },
               ],
             },
           },
@@ -201,6 +209,21 @@ describe('Release edit/new getters', () => {
 
         expect(actualErrors).toMatchObject(expectedErrors);
       });
+
+      it('returns a validation error if links share a title', () => {
+        const expectedErrors = {
+          assets: {
+            links: {
+              9: { isTitleDuplicate: true },
+              10: { isTitleDuplicate: true },
+              11: { isTitleDuplicate: true },
+              12: { isTitleDuplicate: true },
+            },
+          },
+        };
+
+        expect(actualErrors).toMatchObject(expectedErrors);
+      });
     });
   });