Merge branch '284471-misc-vulnerability-filters-refactor' into 'master'

Use querystring as source of truth for filters on vulnerability report

See merge request gitlab-org/gitlab!59981

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

@@ -39,8 +39,6 @@ export default {
         // Toggle the option's existence in the array.
         this.selectedOptions = xor(this.selectedOptions, [option]);
       }
-
-      this.updateQuerystring();
     },
   },
   NO_ACTIVITY,

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

@@ -81,8 +81,6 @@ export default {
       this.selectedOptions = options.every((option) => this.selectedSet.has(option))
         ? without(this.selectedOptions, ...options)
         : union(this.selectedOptions, options);
-
-      this.updateQuerystring();
     },
   },
 };

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

@@ -25,13 +25,45 @@ export default {
   data() {
     return {
       searchTerm: '',
-      selectedOptions: undefined,
     };
   },
   computed: {
     options() {
       return this.filter.options;
     },
+    querystringIds() {
+      const ids = this.$route.query[this.filter.id] || [];
+      return Array.isArray(ids) ? ids : [ids];
+    },
+    selectedOptions: {
+      get() {
+        const hasAllId = this.querystringIds.includes(this.filter.allOption.id);
+        // If the querystring IDs includes the All option, return an empty array. We'll do this even
+        // if there are other IDs because the special All option takes precedence.
+        if (hasAllId) {
+          return [];
+        }
+
+        const options = this.options.filter((x) => this.querystringIds.includes(x.id));
+        // If the querystring IDs didn't match any options, return the default options.
+        if (!options.length) {
+          return this.filter.defaultOptions;
+        }
+
+        return options;
+      },
+      set(options) {
+        const selectedOptions = options.length ? options : [this.filter.allOption];
+        const ids = selectedOptions.map((x) => x.id);
+        // To avoid a console error, don't update the querystring if it's the same as the current one.
+        if (isEqual(this.querystringIds, ids)) {
+          return;
+        }
+
+        const query = { ...this.$route.query, [this.filter.id]: ids };
+        this.$router.push({ query });
+      },
+    },
     selectedSet() {
       return new Set(this.selectedOptions);
     },
@@ -50,55 +82,29 @@ export default {
         option.name.toLowerCase().includes(this.searchTerm.toLowerCase()),
       );
     },
-    querystringIds() {
-      const ids = this.$route?.query[this.filter.id] || [];
-      return Array.isArray(ids) ? ids : [ids];
-    },
-    querystringOptions() {
-      const options = this.options.filter((x) => this.querystringIds.includes(x.id));
-      const hasAllId = this.querystringIds.includes(this.filter.allOption.id);
-
-      if (options.length && !hasAllId) {
-        return options;
-      }
-
-      return hasAllId ? [] : this.filter.defaultOptions;
-    },
     showSearchBox() {
       return this.options.length >= this.searchBoxShowThreshold;
     },
   },
   watch: {
-    selectedOptions() {
-      this.$emit('filter-changed', this.filterObject);
+    selectedOptions: {
+      immediate: true,
+      handler(newOptions, oldOptions) {
+        // This check is needed because updating the querystring for one filter will trigger an
+        // update for all filters, even if the querystring for this filter didn't change.
+        if (!isEqual(newOptions, oldOptions)) {
+          this.$emit('filter-changed', this.filterObject);
+        }
+      },
     },
   },
-  created() {
-    this.selectedOptions = this.querystringOptions;
-    // When the user clicks the forward/back browser buttons, update the selected options.
-    window.addEventListener('popstate', () => {
-      this.selectedOptions = this.querystringOptions;
-    });
-  },
   methods: {
     toggleOption(option) {
       // Toggle the option's existence in the array.
       this.selectedOptions = xor(this.selectedOptions, [option]);
-      this.updateQuerystring();
     },
     deselectAllOptions() {
       this.selectedOptions = [];
-      this.updateQuerystring();
-    },
-    updateQuerystring() {
-      const options = this.selectedOptionsOrAll.map((x) => x.id);
-      // To avoid a console error, don't update the querystring if it's the same as the current one.
-      if (!this.$router || isEqual(this.querystringIds, options)) {
-        return;
-      }
-
-      const query = { ...this.$route.query, [this.filter.id]: options };
-      this.$router.push({ query });
     },
     isSelected(option) {
       return this.selectedSet.has(option);

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

@@ -311,6 +311,7 @@ export default {
       @vulnerability-updated="deselectVulnerability"
     />
     <gl-table
+      v-if="filters"
       :busy="isLoading"
       :fields="fields"
       :items="filteredVulnerabilities"

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

-import { shallowMount } from '@vue/test-utils';
+import { createLocalVue, shallowMount } from '@vue/test-utils';
+import VueRouter from 'vue-router';
 import ActivityFilter from 'ee/security_dashboard/components/filters/activity_filter.vue';
 import { activityFilter, activityOptions } from 'ee/security_dashboard/helpers';
 
+const localVue = createLocalVue();
+localVue.use(VueRouter);
+const router = new VueRouter();
+
 const { NO_ACTIVITY, WITH_ISSUES, NO_LONGER_DETECTED } = activityOptions;
 
 describe('Activity Filter component', () => {
@@ -22,6 +27,8 @@ describe('Activity Filter component', () => {
 
   const createWrapper = () => {
     wrapper = shallowMount(ActivityFilter, {
+      localVue,
+      router,
       propsData: { filter: activityFilter },
     });
   };
@@ -36,6 +43,10 @@ describe('Activity Filter component', () => {
 
   afterEach(() => {
     wrapper.destroy();
+    // Clear out the querystring if one exists, it persists between tests.
+    if (router.currentRoute.query[activityFilter.id]) {
+      router.replace('/');
+    }
   });
 
   it('renders the options', () => {
@@ -51,7 +62,7 @@ describe('Activity Filter component', () => {
   `(
     'deselects mutually exclusive options when $expectedOption.id is selected',
     async ({ selectedOptions, expectedOption }) => {
-      await wrapper.setData({ selectedOptions });
+      await selectedOptions.map(clickItem);
 
       expectSelectedItems(selectedOptions);
 
@@ -63,10 +74,12 @@ describe('Activity Filter component', () => {
 
   describe('filter-changed event', () => {
     it('contains the correct filterObject for the all option', async () => {
+      // Click on another option first.
+      await clickItem(NO_ACTIVITY);
       await clickItem(activityFilter.allOption);
 
-      expect(wrapper.emitted('filter-changed')).toHaveLength(2);
-      expect(wrapper.emitted('filter-changed')[1][0]).toStrictEqual({
+      expect(wrapper.emitted('filter-changed')).toHaveLength(3);
+      expect(wrapper.emitted('filter-changed')[2][0]).toStrictEqual({
         hasIssues: undefined,
         hasResolution: undefined,
       });

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

@@ -56,10 +56,12 @@ describe('Scanner Filter component', () => {
   });
 
   afterEach(() => {
-    // Clear out the querystring if one exists. It persists between tests.
-    if (wrapper.vm.$route.query[filter.id]) {
-      wrapper.vm.$router.push('/');
+    wrapper.destroy();
+    // Clear out the querystring if one exists, it persists between tests.
+    if (router.currentRoute.query[filter.id]) {
+      router.replace('/');
     }
+
     wrapper.destroy();
   });
 

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

@@ -66,11 +66,10 @@ describe('Standard Filter component', () => {
   };
 
   afterEach(() => {
-    // Clear out the querystring if one exists. It persists between tests.
+    // Clear out the querystring if one exists, it persists between tests.
     if (filterQuery()) {
-      wrapper.vm.$router.push('/');
+      router.replace('/');
     }
-    wrapper.destroy();
   });
 
   describe('filter options', () => {
@@ -118,11 +117,17 @@ describe('Standard Filter component', () => {
   });
 
   describe('loading prop', () => {
-    it.each([true, false])(`sets the filter body loading prop to %s`, (loading) => {
-      createWrapper({}, { loading });
+    it.each([true, false])(
+      `sets the filter body loading prop to %s and emits the expected event data`,
+      (loading) => {
+        const query = { [filter.id]: optionIdsAt([1, 3, 5]) };
+        router.replace({ query });
+        createWrapper({}, { loading });
 
-      expect(filterBody().props('loading')).toBe(loading);
-    });
+        expect(filterBody().props('loading')).toBe(loading);
+        expect(wrapper.emitted('filter-changed')[0][0]).toEqual(query);
+      },
+    );
   });
 
   describe('selecting options', () => {
@@ -186,12 +191,8 @@ describe('Standard Filter component', () => {
   });
 
   describe('filter querystring', () => {
-    const updateQuerystring = async (ids) => {
-      // window.history.back() won't change the location nor fire the popstate event, so we need
-      // to fake it by doing it manually.
+    const updateQuerystring = (ids) => {
       router.replace({ query: { [filter.id]: ids } });
-      window.dispatchEvent(new Event('popstate'));
-      await wrapper.vm.$nextTick();
     };
 
     describe('clicking on items', () => {
@@ -328,12 +329,25 @@ describe('Standard Filter component', () => {
         const other = ['6', '7', '8'];
         const query = { [filter.id]: ids, other };
         router.replace({ query });
-        window.dispatchEvent(new Event('popstate'));
         await wrapper.vm.$nextTick();
 
         expectSelectedItems([1, 2, 3]);
         expect(wrapper.vm.$route.query.other).toEqual(other);
       });
+
+      it('does not emit a filter-changed event if the querystring change was not for the current filter', async () => {
+        const query = { [filter.id]: optionIdsAt([2, 5]) };
+        router.replace({ query });
+        createWrapper();
+
+        expect(wrapper.emitted('filter-changed')).toHaveLength(1);
+
+        query.other = [1, 2, 3];
+        router.push({ query });
+        await wrapper.vm.$nextTick();
+
+        expect(wrapper.emitted('filter-changed')).toHaveLength(1);
+      });
     });
   });
 });