Merge branch '210327-improve-vulnerability-filter-logic' into 'master'

Improve vulnerability filter component logic

See merge request gitlab-org/gitlab!45614

ee/app/assets/javascripts/security_dashboard/components/filters/filter_body.vue

@@ -31,7 +31,7 @@ export default {
   },
   computed: {
     firstSelectedOption() {
-      return this.selectedOptions[0] || '-';
+      return this.selectedOptions[0]?.name || '-';
     },
     extraOptionCount() {
       return this.selectedOptions.length - 1;

ee/app/assets/javascripts/security_dashboard/components/filters/standard_filter.vue

 <script>
+import { isEqual, xor } from 'lodash';
 import FilterBody from './filter_body.vue';
 import FilterItem from './filter_item.vue';
 
 export default {
-  components: {
-    FilterBody,
-    FilterItem,
-  },
+  components: { FilterBody, FilterItem },
   props: {
     filter: {
       type: Object,
       required: true,
     },
+    showSearchBox: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
       searchTerm: '',
+      selectedOptions: undefined,
     };
   },
   computed: {
-    selection() {
-      return this.filter.selection;
+    selectedSet() {
+      return new Set(this.selectedOptions);
+    },
+    isNoOptionsSelected() {
+      return this.selectedOptions.length <= 0;
+    },
+    selectedOptionsOrAll() {
+      return this.selectedOptions.length ? this.selectedOptions : [this.filter.allOption];
+    },
+    queryObject() {
+      // This is the object used to update the querystring.
+      return { [this.filter.id]: this.selectedOptionsOrAll.map(x => x.id) };
+    },
+    filterObject() {
+      // This is the object used by the GraphQL query.
+      return { [this.filter.id]: this.selectedOptions.map(x => x.id) };
     },
     filteredOptions() {
       return this.filter.options.filter(option =>
         option.name.toLowerCase().includes(this.searchTerm.toLowerCase()),
       );
     },
-    selectedOptionsNames() {
-      return Array.from(this.selection).map(id => this.filter.options.find(x => x.id === id).name);
+    routeQueryIds() {
+      const ids = this.$route.query[this.filter.id] || [];
+      return Array.isArray(ids) ? ids : [ids];
+    },
+    routeQueryOptions() {
+      const options = this.filter.options.filter(x => this.routeQueryIds.includes(x.id));
+      const hasAllId = this.routeQueryIds.includes(this.filter.allOption.id);
+
+      if (options.length && !hasAllId) {
+        return options;
+      }
+
+      return hasAllId ? [] : this.filter.defaultOptions;
+    },
+  },
+  watch: {
+    selectedOptions() {
+      this.$emit('filter-changed', this.filterObject);
+    },
   },
+  created() {
+    this.selectedOptions = this.routeQueryOptions;
+    // When the user clicks the forward/back browser buttons, update the selected options.
+    window.addEventListener('popstate', () => {
+      this.selectedOptions = this.routeQueryOptions;
+    });
   },
   methods: {
-    clickFilter(option) {
-      this.$emit('setFilter', { filterId: this.filter.id, optionId: option.id });
+    toggleOption(option) {
+      // Toggle the option's existence in the array.
+      this.selectedOptions = xor(this.selectedOptions, [option]);
+      this.updateRouteQuery();
+    },
+    deselectAllOptions() {
+      this.selectedOptions = [];
+      this.updateRouteQuery();
+    },
+    updateRouteQuery() {
+      const query = { query: { ...this.$route.query, ...this.queryObject } };
+      // To avoid a console error, don't update the querystring if it's the same as the current one.
+      if (!isEqual(this.routeQueryIds, this.queryObject[this.filter.id])) {
+        this.$router.push(query);
+      }
     },
     isSelected(option) {
-      return this.selection.has(option.id);
+      return this.selectedSet.has(option);
     },
   },
 };
@@ -46,15 +100,23 @@ export default {
   <filter-body
     v-model.trim="searchTerm"
     :name="filter.name"
-    :selected-options="selectedOptionsNames"
-    :show-search-box="filter.options.length >= 20"
+    :selected-options="selectedOptionsOrAll"
+    :show-search-box="showSearchBox"
   >
+    <filter-item
+      v-if="filter.allOption && !searchTerm.length"
+      :is-checked="isNoOptionsSelected"
+      :text="filter.allOption.name"
+      data-testid="allOption"
+      @click="deselectAllOptions"
+    />
     <filter-item
       v-for="option in filteredOptions"
       :key="option.id"
       :is-checked="isSelected(option)"
       :text="option.name"
-      @click="clickFilter(option)"
+      data-testid="filterOption"
+      @click="toggleOption(option)"
     />
   </filter-body>
 </template>

ee/app/assets/javascripts/security_dashboard/components/first_class_vulnerability_filters.vue

 <script>
-import { isEqual } from 'lodash';
-import { ALL, STATE } from 'ee/security_dashboard/store/modules/filters/constants';
-import { setFilter } from 'ee/security_dashboard/store/modules/filters/utils';
-import StandardFilter from 'ee/security_dashboard/components/filters/standard_filter.vue';
-import { initFirstClassVulnerabilityFilters, mapProjects } from 'ee/security_dashboard/helpers';
+import { debounce } from 'lodash';
+import { stateFilter, severityFilter, scannerFilter, getProjectFilter } from '../helpers';
+import StandardFilter from './filters/standard_filter.vue';
+
+const searchBoxOptionCount = 20; // Number of options before the search box is shown.
 
 export default {
   components: {
@@ -12,73 +12,28 @@ export default {
   props: {
     projects: { type: Array, required: false, default: undefined },
   },
-  data() {
-    return {
-      filters: initFirstClassVulnerabilityFilters(this.projects),
-    };
-  },
+  data: () => ({
+    filterQuery: {},
+  }),
   computed: {
-    selectedFilters() {
-      return this.filters.reduce((acc, { id, selection }) => {
-        if (!selection.has(ALL)) {
-          acc[id] = Array.from(selection);
-        }
-
-        return acc;
-      }, {});
-    },
-  },
-  watch: {
-    /**
-     * Initially the project list empty. We fetch them dynamically from GraphQL while
-     * fetching the list of vulnerabilities. We display the project filter with the base
-     * option and when the projects are fetched we add them to the list.
-     */
-    projects(newProjects, oldProjects) {
-      if (oldProjects.length === 0) {
-        const projectFilter = this.filters[3];
-        projectFilter.options = [projectFilter.options[0], ...mapProjects(this.projects)];
-      }
-    },
-
-    '$route.query': {
-      immediate: true,
-      handler(newQuery) {
-        let changed;
-
-        this.filters.forEach((filter, i) => {
-          let urlFilter = newQuery[filter.id];
-
-          if (typeof urlFilter === 'undefined') {
-            urlFilter = [ALL];
-          } else if (!Array.isArray(urlFilter)) {
-            urlFilter = [urlFilter];
-          }
-
-          if (isEqual(this.selectedFilters[filter.id], newQuery[filter.id]) === false) {
-            changed = true;
-            this.filters[i].selection = new Set(urlFilter);
-          }
-        });
-
-        if (changed) {
-          this.$emit('filterChange', this.selectedFilters);
-        }
-      },
-    },
+    filters() {
+      return this.projects
+        ? [stateFilter, severityFilter, scannerFilter, getProjectFilter(this.projects)]
+        : [stateFilter, severityFilter, scannerFilter];
     },
-  created() {
-    if (Object.keys(this.selectedFilters).length === 0) {
-      this.$router.push({ query: { state: [STATE.DETECTED, STATE.CONFIRMED] } });
-    }
   },
   methods: {
-    setFilter(options) {
-      this.filters = setFilter(this.filters, options);
-      this.$router.push({ query: this.selectedFilters });
-      this.$emit('filterChange', this.selectedFilters);
+    updateFilterQuery(query) {
+      this.filterQuery = { ...this.filterQuery, ...query };
+      this.emitFilterChange();
     },
+    // All the filters will emit @filter-changed when the page is first loaded, which will trigger
+    // this method multiple times. We'll debounce it so that it only runs once.
+    emitFilterChange: debounce(function emit() {
+      this.$emit('filterChange', this.filterQuery);
+    }),
   },
+  searchBoxOptionCount,
 };
 </script>
 
@@ -90,7 +45,9 @@ export default {
         :key="filter.id"
         class="col-sm-6 col-md-4 col-lg-2 p-2"
         :filter="filter"
-        @setFilter="setFilter"
+        :data-testid="filter.id"
+        :show-search-box="filter.options.length >= $options.searchBoxOptionCount"
+        @filter-changed="updateFilterQuery"
       />
     </div>
   </div>

ee/app/assets/javascripts/security_dashboard/helpers.js

 import isPlainObject from 'lodash/isPlainObject';
-import { ALL, BASE_FILTERS } from 'ee/security_dashboard/store/modules/filters/constants';
+import { BASE_FILTERS } from 'ee/security_dashboard/store/modules/filters/constants';
 import { REPORT_TYPES, SEVERITY_LEVELS } from 'ee/security_dashboard/store/constants';
 import { VULNERABILITY_STATES } from 'ee/vulnerabilities/constants';
 import { convertObjectPropsToSnakeCase } from '~/lib/utils/common_utils';
@@ -11,41 +11,41 @@ const parseOptions = obj =>
 export const mapProjects = projects =>
   projects.map(p => ({ id: p.id.split('/').pop(), name: p.name }));
 
-export const initFirstClassVulnerabilityFilters = projects => {
-  const filters = [
-    {
+const stateOptions = parseOptions(VULNERABILITY_STATES);
+const defaultStateOptions = stateOptions.filter(x => ['DETECTED', 'CONFIRMED'].includes(x.id));
+
+export const stateFilter = {
   name: s__('SecurityReports|Status'),
   id: 'state',
-      options: [
-        { id: ALL, name: s__('VulnerabilityStatusTypes|All') },
-        ...parseOptions(VULNERABILITY_STATES),
-      ],
-      selection: new Set([ALL]),
-    },
-    {
+  options: stateOptions,
+  allOption: BASE_FILTERS.state,
+  defaultOptions: defaultStateOptions,
+};
+
+export const severityFilter = {
   name: s__('SecurityReports|Severity'),
   id: 'severity',
-      options: [BASE_FILTERS.severity, ...parseOptions(SEVERITY_LEVELS)],
-      selection: new Set([ALL]),
-    },
-    {
+  options: parseOptions(SEVERITY_LEVELS),
+  allOption: BASE_FILTERS.severity,
+  defaultOptions: [],
+};
+
+export const scannerFilter = {
   name: s__('Reports|Scanner'),
   id: 'reportType',
-      options: [BASE_FILTERS.report_type, ...parseOptions(REPORT_TYPES)],
-      selection: new Set([ALL]),
-    },
-  ];
+  options: parseOptions(REPORT_TYPES),
+  allOption: BASE_FILTERS.report_type,
+  defaultOptions: [],
+};
 
-  if (Array.isArray(projects)) {
-    filters.push({
+export const getProjectFilter = projects => {
+  return {
     name: s__('SecurityReports|Project'),
     id: 'projectId',
-      options: [BASE_FILTERS.project_id, ...mapProjects(projects)],
-      selection: new Set([ALL]),
-    });
-  }
-
-  return filters;
+    options: mapProjects(projects),
+    allOption: BASE_FILTERS.project_id,
+    defaultOptions: [],
+  };
 };
 
 /**

ee/app/assets/javascripts/security_dashboard/store/modules/filters/constants.js

@@ -7,6 +7,10 @@ export const STATE = {
 };
 
 export const BASE_FILTERS = {
+  state: {
+    name: s__('VulnerabilityStatusTypes|All'),
+    id: ALL,
+  },
   severity: {
     name: s__('ciReport|All severities'),
     id: ALL,

ee/spec/frontend/security_dashboard/components/filters/filter_body_spec.js

@@ -33,17 +33,19 @@ describe('Filter Body component', () => {
 
   describe('dropdown button', () => {
     it('shows the selected option name if only one option is selected', () => {
-      const props = { selectedOptions: ['Some Selected Option'] };
+      const option = { name: 'Some Selected Option' };
+      const props = { selectedOptions: [option] };
       createComponent(props);
 
-      expect(dropdownButton().text()).toBe(props.selectedOptions[0]);
+      expect(dropdownButton().text()).toBe(option.name);
     });
 
     it('shows the selected option name and "+x more" if more than one option is selected', () => {
-      const props = { selectedOptions: ['Option 1', 'Option 2', 'Option 3'] };
+      const options = [{ name: 'Option 1' }, { name: 'Option 2' }, { name: 'Option 3' }];
+      const props = { selectedOptions: options };
       createComponent(props);
 
-      expect(dropdownButton().text()).toMatch(/Option 1\s+\+2 more/);
+      expect(dropdownButton().text()).toMatchInterpolatedText('Option 1 +2 more');
     });
   });
 

ee/spec/frontend/security_dashboard/components/filters_spec.js

 import Vuex from 'vuex';
 import Filters from 'ee/security_dashboard/components/filters.vue';
 import createStore from 'ee/security_dashboard/store';
-import { mount, createLocalVue } from '@vue/test-utils';
+import { shallowMount, createLocalVue } from '@vue/test-utils';
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -11,7 +11,7 @@ describe('Filter component', () => {
   let store;
 
   const createWrapper = (props = {}) => {
-    wrapper = mount(Filters, {
+    wrapper = shallowMount(Filters, {
       localVue,
       store,
       propsData: {

ee/spec/frontend/security_dashboard/components/first_class_vulnerability_filters_spec.js

 import VueRouter from 'vue-router';
 import { createLocalVue, shallowMount } from '@vue/test-utils';
-import { initFirstClassVulnerabilityFilters } from 'ee/security_dashboard/helpers';
 import Filters from 'ee/security_dashboard/components/first_class_vulnerability_filters.vue';
 import StandardFilter from 'ee/security_dashboard/components/filters/standard_filter.vue';
 
@@ -10,7 +9,6 @@ localVue.use(VueRouter);
 
 describe('First class vulnerability filters component', () => {
   let wrapper;
-  let filters;
 
   const projects = [
     { id: 'gid://gitlab/Project/11', name: 'GitLab Org' },
@@ -18,11 +16,8 @@ describe('First class vulnerability filters component', () => {
   ];
 
   const findFilters = () => wrapper.findAll(StandardFilter);
-  const findStateFilter = () => findFilters().at(0);
-  const findSeverityFilter = () => findFilters().at(1);
-  const findReportTypeFilter = () => findFilters().at(2);
-  const findProjectFilter = () => findFilters().at(3);
-  const findLastFilter = () => findFilters().at(filters.length - 1);
+  const findStateFilter = () => wrapper.find('[data-testid="state"]');
+  const findProjectFilter = () => wrapper.find('[data-testid="projectId"]');
 
   const createComponent = ({ propsData, listeners } = {}) => {
     return shallowMount(Filters, { localVue, router, propsData, listeners });
@@ -36,190 +31,39 @@ describe('First class vulnerability filters component', () => {
   describe('on render without project filter', () => {
     beforeEach(() => {
       wrapper = createComponent();
-      filters = initFirstClassVulnerabilityFilters();
     });
 
-    it('should render the filters', () => {
-      expect(findFilters()).toHaveLength(filters.length);
+    it('should render the default filters', () => {
+      expect(findFilters()).toHaveLength(3);
     });
 
-    it('should call the setFilter mutation when setting a filter', () => {
-      const stub = jest.fn();
+    it('should emit filterChange when a filter is changed', () => {
       const options = { foo: 'bar' };
+      findStateFilter().vm.$emit('filter-changed', options);
 
-      wrapper.setMethods({ setFilter: stub });
-      findStateFilter().vm.$emit('setFilter', options);
-
-      expect(stub).toHaveBeenCalledWith(options);
+      expect(wrapper.emitted('filterChange')[0][0]).toEqual(options);
     });
   });
 
   describe('when project filter is populated dynamically', () => {
     beforeEach(() => {
-      filters = initFirstClassVulnerabilityFilters([]);
-      wrapper = createComponent({ propsData: { projects: [] } });
+      wrapper = createComponent();
     });
 
-    it('should render the project filter with one option', () => {
-      expect(findLastFilter().props('filter')).toEqual({
-        id: 'projectId',
-        name: 'Project',
-        options: [{ id: 'all', name: 'All projects' }],
-        selection: new Set(['all']),
-      });
+    it('should render the project filter with no options', async () => {
+      wrapper.setProps({ projects: [] });
+      await wrapper.vm.$nextTick();
+      expect(findProjectFilter().props('filter').options).toHaveLength(0);
     });
 
-    it('should set the projects dynamically', () => {
+    it('should render the project filter with the expected options', async () => {
       wrapper.setProps({ projects });
-      return wrapper.vm.$nextTick(() => {
-        expect(findLastFilter().props('filter')).toEqual(
-          expect.objectContaining({
-            options: [
-              { id: 'all', name: 'All projects' },
-              { id: '11', name: 'GitLab Org' },
-              { id: '12', name: 'GitLab Com' },
-            ],
-          }),
-        );
-      });
-    });
-  });
-
-  describe('when project filter is ready on mount', () => {
-    beforeEach(() => {
-      filters = initFirstClassVulnerabilityFilters([]);
-      wrapper = createComponent({ propsData: { projects } });
-    });
-
-    it('should set the projects dynamically', () => {
-      expect(findLastFilter().props('filter')).toEqual(
-        expect.objectContaining({
-          options: [
-            { id: 'all', name: 'All projects' },
-            { id: '11', name: 'GitLab Org' },
-            { id: '12', name: 'GitLab Com' },
-          ],
-        }),
-      );
-    });
-  });
-
-  describe('when no filter is persisted in the URL', () => {
-    beforeEach(() => {
-      wrapper = createComponent({
-        propsData: { projects },
-      });
-    });
-
-    it('should redirect the user to an updated the URL and default the filters to CONFIRMED + DETECTED state', () => {
-      expect(findStateFilter().props('filter')).toEqual(
-        expect.objectContaining({
-          selection: new Set(['DETECTED', 'CONFIRMED']),
-        }),
-      );
-    });
-  });
-
-  describe.each`
-    filter          | value                   | selector
-    ${'state'}      | ${'DETECTED,DISMISSED'} | ${findStateFilter}
-    ${'severity'}   | ${'MEDIUM'}             | ${findSeverityFilter}
-    ${'reportType'} | ${'SAST'}               | ${findReportTypeFilter}
-    ${'projectId'}  | ${'12'}                 | ${findProjectFilter}
-  `('when filters are persisted', ({ filter, value, selector }) => {
-    describe(`with filter set to ${filter}: ${value}`, () => {
-      let filterChangeSpy;
-
-      beforeEach(() => {
-        filterChangeSpy = jest.fn();
-
-        wrapper = createComponent({
-          propsData: { projects },
-          listeners: { filterChange: filterChangeSpy },
-        });
-        // reset the router query in-between test cases
-        router.push({ query: {} });
-        router.push({ query: { [filter]: value.split(',') } }, () => {});
-      });
-
-      it(`should have the ${filter} filter as pre-selected`, () => {
-        expect(selector().props('filter').selection).toEqual(new Set(value.split(',')));
-      });
-
-      it('should emit a filterChange event', () => {
-        expect(wrapper.emitted().filterChange).toBeTruthy();
-      });
+      await wrapper.vm.$nextTick();
 
-      it('should not trigger the filterChange additonally when the filters do not change', () => {
-        router.push({
-          query: {
-            ...wrapper.vm.$route.query,
-            'some-unrelated-query-param': 'true',
-          },
-        });
-
-        return wrapper.vm.$nextTick(() => {
-          expect(filterChangeSpy).toHaveBeenCalledTimes(1);
-        });
-      });
-
-      it('should trigger the filterChange when the filters are reset', () => {
-        router.push({ query: {} });
-
-        return wrapper.vm.$nextTick(() => {
-          expect(filterChangeSpy).toHaveBeenNthCalledWith(2, {});
-        });
-      });
-
-      it('should reset the filters when the URL contains no more filters', () => {
-        router.push({ query: {} });
-
-        return wrapper.vm.$nextTick(() => {
-          expect(selector().props('filter').selection).toEqual(new Set(['all']));
-        });
-      });
-    });
-  });
-
-  describe.each`
-    filter          | selector                | index
-    ${'state'}      | ${findStateFilter}      | ${0}
-    ${'severity'}   | ${findSeverityFilter}   | ${1}
-    ${'reportType'} | ${findReportTypeFilter} | ${2}
-    ${'projectId'}  | ${findProjectFilter}    | ${3}
-  `('when setFilter is called', ({ filter, selector, index }) => {
-    describe(filter, () => {
-      let filterId;
-      let optionId;
-      let routePushSpy;
-
-      beforeEach(() => {
-        filters = initFirstClassVulnerabilityFilters(projects);
-        filterId = filters[index].id;
-        optionId = filters[index].options[1].id;
-        wrapper = createComponent({ propsData: { projects } });
-        routePushSpy = jest.spyOn(router, 'push');
-        selector().vm.$emit('setFilter', { optionId, filterId });
-      });
-
-      afterEach(() => {
-        // This will reset the query state
-        router.push('/');
-      });
-
-      it('should set the filter', () => {
-        expect(selector().props('filter').selection).toEqual(new Set([optionId]));
-      });
-
-      it('should emit a filterChange event', () => {
-        expect(wrapper.emitted().filterChange).toBeTruthy();
-      });
-
-      it('should update the path', () => {
-        expect(routePushSpy).toHaveBeenCalledWith({
-          query: { [filterId]: [optionId] },
-        });
-      });
+      expect(findProjectFilter().props('filter').options).toEqual([
+        { id: '11', name: projects[0].name },
+        { id: '12', name: projects[1].name },
+      ]);
     });
   });
 });