Merge branch 'kp-filtered-search-remove-duplicates' into 'master'

Prevents duplicate tokens from being applied in Filtered Search Bar

See merge request gitlab-org/gitlab!40955

app/assets/javascripts/vue_shared/components/filtered_search_bar/filtered_search_bar_root.vue

@@ -15,7 +15,7 @@ import { deprecatedCreateFlash as createFlash } from '~/flash';
 import RecentSearchesStore from '~/filtered_search/stores/recent_searches_store';
 import RecentSearchesService from '~/filtered_search/services/recent_searches_service';
 
-import { stripQuotes } from './filtered_search_utils';
+import { stripQuotes, uniqueTokens } from './filtered_search_utils';
 import { SortDirection } from './constants';
 
 export default {
@@ -120,10 +120,31 @@ export default {
         ? __('Sort direction: Ascending')
         : __('Sort direction: Descending');
     },
+    /**
+     * This prop fixes a behaviour affecting GlFilteredSearch
+     * where selecting duplicate token values leads to history
+     * dropdown also showing that selection.
+     */
     filteredRecentSearches() {
-      return this.recentSearchesStorageKey
-        ? this.recentSearches.filter(item => typeof item !== 'string')
-        : undefined;
+      if (this.recentSearchesStorageKey) {
+        const knownItems = [];
+        return this.recentSearches.reduce((historyItems, item) => {
+          // Only include non-string history items (discard items from legacy search)
+          if (typeof item !== 'string') {
+            const sanitizedItem = uniqueTokens(item);
+            const itemString = JSON.stringify(sanitizedItem);
+            // Only include items which aren't already part of history
+            if (!knownItems.includes(itemString)) {
+              historyItems.push(sanitizedItem);
+              // We're storing string for comparision as doing direct object compare
+              // won't work due to object reference not being the same.
+              knownItems.push(itemString);
+            }
+          }
+          return historyItems;
+        }, []);
+      }
+      return undefined;
     },
   },
   watch: {
@@ -245,12 +266,14 @@ export default {
       this.recentSearchesService.save(resultantSearches);
       this.recentSearches = [];
     },
-    handleFilterSubmit(filters) {
+    handleFilterSubmit() {
+      const filterTokens = uniqueTokens(this.filterValue);
+      this.filterValue = filterTokens;
       if (this.recentSearchesStorageKey) {
         this.recentSearchesPromise
           .then(() => {
-            if (filters.length) {
-              const resultantSearches = this.recentSearchesStore.addRecentSearch(filters);
+            if (filterTokens.length) {
+              const resultantSearches = this.recentSearchesStore.addRecentSearch(filterTokens);
               this.recentSearchesService.save(resultantSearches);
               this.recentSearches = resultantSearches;
             }
@@ -260,7 +283,7 @@ export default {
           });
       }
       this.blurSearchInput();
-      this.$emit('onFilter', this.removeQuotesEnclosure(filters));
+      this.$emit('onFilter', this.removeQuotesEnclosure(filterTokens));
     },
   },
 };

app/assets/javascripts/vue_shared/components/filtered_search_bar/filtered_search_utils.js

-export const stripQuotes = value => {
-  return value.includes(' ') ? value.slice(1, -1) : value;
+/**
+ * Strips enclosing quotations from a string if it has one.
+ *
+ * @param {String} value String to strip quotes from
+ *
+ * @returns {String} String without any enclosure
+ */
+export const stripQuotes = value => value.replace(/^('|")(.*)('|")$/, '$2');
+
+/**
+ * This method removes duplicate tokens from tokens array.
+ *
+ * @param {Array} tokens Array of tokens as defined by `GlFilteredSearch`
+ *
+ * @returns {Array} Unique array of tokens
+ */
+export const uniqueTokens = tokens => {
+  const knownTokens = [];
+  return tokens.reduce((uniques, token) => {
+    if (typeof token === 'object' && token.type !== 'filtered-search-term') {
+      const tokenString = `${token.type}${token.value.operator}${token.value.data}`;
+      if (!knownTokens.includes(tokenString)) {
+        uniques.push(token);
+        knownTokens.push(tokenString);
+      }
+    } else {
+      uniques.push(token);
+    }
+    return uniques;
+  }, []);
 };

spec/frontend/vue_shared/components/filtered_search_bar/filtered_search_bar_root_spec.js

@@ -8,12 +8,27 @@ import {
 } from '@gitlab/ui';
 
 import FilteredSearchBarRoot from '~/vue_shared/components/filtered_search_bar/filtered_search_bar_root.vue';
+import { uniqueTokens } from '~/vue_shared/components/filtered_search_bar/filtered_search_utils';
 import { SortDirection } from '~/vue_shared/components/filtered_search_bar/constants';
 
 import RecentSearchesStore from '~/filtered_search/stores/recent_searches_store';
 import RecentSearchesService from '~/filtered_search/services/recent_searches_service';
 
-import { mockAvailableTokens, mockSortOptions, mockHistoryItems } from './mock_data';
+import {
+  mockAvailableTokens,
+  mockSortOptions,
+  mockHistoryItems,
+  tokenValueAuthor,
+  tokenValueLabel,
+  tokenValueMilestone,
+} from './mock_data';
+
+jest.mock('~/vue_shared/components/filtered_search_bar/filtered_search_utils', () => ({
+  uniqueTokens: jest.fn().mockImplementation(tokens => tokens),
+  stripQuotes: jest.requireActual(
+    '~/vue_shared/components/filtered_search_bar/filtered_search_utils',
+  ).stripQuotes,
+}));
 
 const createComponent = ({
   shallow = true,
@@ -73,13 +88,21 @@ describe('FilteredSearchBarRoot', () => {
   describe('computed', () => {
     describe('tokenSymbols', () => {
       it('returns a map containing type and symbols from `tokens` prop', () => {
-        expect(wrapper.vm.tokenSymbols).toEqual({ author_username: '@', label_name: '~' });
+        expect(wrapper.vm.tokenSymbols).toEqual({
+          author_username: '@',
+          label_name: '~',
+          milestone_title: '%',
+        });
       });
     });
 
     describe('tokenTitles', () => {
       it('returns a map containing type and title from `tokens` prop', () => {
-        expect(wrapper.vm.tokenTitles).toEqual({ author_username: 'Author', label_name: 'Label' });
+        expect(wrapper.vm.tokenTitles).toEqual({
+          author_username: 'Author',
+          label_name: 'Label',
+          milestone_title: 'Milestone',
+        });
       });
     });
 
@@ -131,6 +154,20 @@ describe('FilteredSearchBarRoot', () => {
         expect(wrapper.vm.filteredRecentSearches[0]).toEqual({ foo: 'bar' });
       });
 
+      it('returns array of recent searches sanitizing any duplicate token values', async () => {
+        wrapper.setData({
+          recentSearches: [
+            [tokenValueAuthor, tokenValueLabel, tokenValueMilestone, tokenValueLabel],
+            [tokenValueAuthor, tokenValueMilestone],
+          ],
+        });
+
+        await wrapper.vm.$nextTick();
+
+        expect(wrapper.vm.filteredRecentSearches).toHaveLength(2);
+        expect(uniqueTokens).toHaveBeenCalled();
+      });
+
       it('returns undefined when recentSearchesStorageKey prop is not set on component', async () => {
         wrapper.setProps({
           recentSearchesStorageKey: '',
@@ -182,40 +219,12 @@ describe('FilteredSearchBarRoot', () => {
     });
 
     describe('removeQuotesEnclosure', () => {
-      const mockFilters = [
-        {
-          type: 'author_username',
-          value: {
-            data: 'root',
-            operator: '=',
-          },
-        },
-        {
-          type: 'label_name',
-          value: {
-            data: '"Documentation Update"',
-            operator: '=',
-          },
-        },
-        'foo',
-      ];
+      const mockFilters = [tokenValueAuthor, tokenValueLabel, 'foo'];
 
       it('returns filter array with unescaped strings for values which have spaces', () => {
         expect(wrapper.vm.removeQuotesEnclosure(mockFilters)).toEqual([
-          {
-            type: 'author_username',
-            value: {
-              data: 'root',
-              operator: '=',
-            },
-          },
-          {
-            type: 'label_name',
-            value: {
-              data: 'Documentation Update',
-              operator: '=',
-            },
-          },
+          tokenValueAuthor,
+          tokenValueLabel,
           'foo',
         ]);
       });
@@ -277,21 +286,26 @@ describe('FilteredSearchBarRoot', () => {
     });
 
     describe('handleFilterSubmit', () => {
-      const mockFilters = [
-        {
-          type: 'author_username',
-          value: {
-            data: 'root',
-            operator: '=',
-          },
-        },
-        'foo',
-      ];
+      const mockFilters = [tokenValueAuthor, 'foo'];
+
+      beforeEach(async () => {
+        wrapper.setData({
+          filterValue: mockFilters,
+        });
+
+        await wrapper.vm.$nextTick();
+      });
+
+      it('calls `uniqueTokens` on `filterValue` prop to remove duplicates', () => {
+        wrapper.vm.handleFilterSubmit();
+
+        expect(uniqueTokens).toHaveBeenCalledWith(wrapper.vm.filterValue);
+      });
 
       it('calls `recentSearchesStore.addRecentSearch` with serialized value of provided `filters` param', () => {
         jest.spyOn(wrapper.vm.recentSearchesStore, 'addRecentSearch');
 
-        wrapper.vm.handleFilterSubmit(mockFilters);
+        wrapper.vm.handleFilterSubmit();
 
         return wrapper.vm.recentSearchesPromise.then(() => {
           expect(wrapper.vm.recentSearchesStore.addRecentSearch).toHaveBeenCalledWith(mockFilters);
@@ -301,7 +315,7 @@ describe('FilteredSearchBarRoot', () => {
       it('calls `recentSearchesService.save` with array of searches', () => {
         jest.spyOn(wrapper.vm.recentSearchesService, 'save');
 
-        wrapper.vm.handleFilterSubmit(mockFilters);
+        wrapper.vm.handleFilterSubmit();
 
         return wrapper.vm.recentSearchesPromise.then(() => {
           expect(wrapper.vm.recentSearchesService.save).toHaveBeenCalledWith([mockFilters]);
@@ -311,7 +325,7 @@ describe('FilteredSearchBarRoot', () => {
       it('sets `recentSearches` data prop with array of searches', () => {
         jest.spyOn(wrapper.vm.recentSearchesService, 'save');
 
-        wrapper.vm.handleFilterSubmit(mockFilters);
+        wrapper.vm.handleFilterSubmit();
 
         return wrapper.vm.recentSearchesPromise.then(() => {
           expect(wrapper.vm.recentSearches).toEqual([mockFilters]);
@@ -329,7 +343,7 @@ describe('FilteredSearchBarRoot', () => {
       it('emits component event `onFilter` with provided filters param', () => {
         jest.spyOn(wrapper.vm, 'removeQuotesEnclosure');
 
-        wrapper.vm.handleFilterSubmit(mockFilters);
+        wrapper.vm.handleFilterSubmit();
 
         expect(wrapper.emitted('onFilter')[0]).toEqual([mockFilters]);
         expect(wrapper.vm.removeQuotesEnclosure).toHaveBeenCalledWith(mockFilters);
@@ -366,7 +380,9 @@ describe('FilteredSearchBarRoot', () => {
         '.gl-search-box-by-click-menu .gl-search-box-by-click-history-item',
       );
 
-      expect(searchHistoryItemsEl.at(0).text()).toBe('Author := @tobyLabel := ~Bug"duo"');
+      expect(searchHistoryItemsEl.at(0).text()).toBe(
+        'Author := @rootLabel := ~bugMilestone := %v1.0"duo"',
+      );
 
       wrapperFullMount.destroy();
     });

spec/frontend/vue_shared/components/filtered_search_bar/filtered_search_utils_spec.js

 import * as filteredSearchUtils from '~/vue_shared/components/filtered_search_bar/filtered_search_utils';
 
+import {
+  tokenValueAuthor,
+  tokenValueLabel,
+  tokenValueMilestone,
+  tokenValuePlain,
+} from './mock_data';
+
 describe('Filtered Search Utils', () => {
   describe('stripQuotes', () => {
     it.each`
@@ -9,6 +16,7 @@ describe('Filtered Search Utils', () => {
       ${'FooBar'}    | ${'FooBar'}
       ${"Foo'Bar"}   | ${"Foo'Bar"}
       ${'Foo"Bar'}   | ${'Foo"Bar'}
+      ${'Foo Bar'}   | ${'Foo Bar'}
     `(
       'returns string $outputValue when called with string $inputValue',
       ({ inputValue, outputValue }) => {
@@ -16,4 +24,29 @@ describe('Filtered Search Utils', () => {
       },
     );
   });
+
+  describe('uniqueTokens', () => {
+    it('returns tokens array with duplicates removed', () => {
+      expect(
+        filteredSearchUtils.uniqueTokens([
+          tokenValueAuthor,
+          tokenValueLabel,
+          tokenValueMilestone,
+          tokenValueLabel,
+          tokenValuePlain,
+        ]),
+      ).toHaveLength(4); // Removes 2nd instance of tokenValueLabel
+    });
+
+    it('returns tokens array as it is if it does not have duplicates', () => {
+      expect(
+        filteredSearchUtils.uniqueTokens([
+          tokenValueAuthor,
+          tokenValueLabel,
+          tokenValueMilestone,
+          tokenValuePlain,
+        ]),
+      ).toHaveLength(4);
+    });
+  });
 });

spec/frontend/vue_shared/components/filtered_search_bar/mock_data.js

@@ -89,36 +89,40 @@ export const mockMilestoneToken = {
   fetchMilestones: () => Promise.resolve({ data: mockMilestones }),
 };
 
-export const mockAvailableTokens = [mockAuthorToken, mockLabelToken];
+export const mockAvailableTokens = [mockAuthorToken, mockLabelToken, mockMilestoneToken];
+
+export const tokenValueAuthor = {
+  type: 'author_username',
+  value: {
+    data: 'root',
+    operator: '=',
+  },
+};
+
+export const tokenValueLabel = {
+  type: 'label_name',
+  value: {
+    operator: '=',
+    data: 'bug',
+  },
+};
+
+export const tokenValueMilestone = {
+  type: 'milestone_title',
+  value: {
+    operator: '=',
+    data: 'v1.0',
+  },
+};
+
+export const tokenValuePlain = {
+  type: 'filtered-search-term',
+  value: { data: 'foo' },
+};
 
 export const mockHistoryItems = [
-  [
-    {
-      type: 'author_username',
-      value: {
-        data: 'toby',
-        operator: '=',
-      },
-    },
-    {
-      type: 'label_name',
-      value: {
-        data: 'Bug',
-        operator: '=',
-      },
-    },
-    'duo',
-  ],
-  [
-    {
-      type: 'author_username',
-      value: {
-        data: 'root',
-        operator: '=',
-      },
-    },
-    'si',
-  ],
+  [tokenValueAuthor, tokenValueLabel, tokenValueMilestone, 'duo'],
+  [tokenValueAuthor, 'si'],
 ];
 
 export const mockSortOptions = [

spec/frontend/vue_shared/components/filtered_search_bar/tokens/milestone_token_spec.js

@@ -157,7 +157,7 @@ describe('MilestoneToken', () => {
       const tokenSegments = wrapper.findAll(GlFilteredSearchTokenSegment);
 
       expect(tokenSegments).toHaveLength(3); // Milestone, =, '%"4.0"'
-      expect(tokenSegments.at(2).text()).toBe(`%"${mockRegularMilestone.title}"`); // "4.0 RC1"
+      expect(tokenSegments.at(2).text()).toBe(`%${mockRegularMilestone.title}`); // "4.0 RC1"
     });
 
     it('renders provided defaultMilestones as suggestions', async () => {