Merge branch '350343-save-sort-on-vulnerability-report' into 'master'

Save sort on querystring for vulnerability report

See merge request gitlab-org/gitlab!80140

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

@@ -19,12 +19,11 @@ import FalsePositiveBadge from 'ee/vulnerabilities/components/false_positive_bad
 import RemediatedBadge from 'ee/vulnerabilities/components/remediated_badge.vue';
 import { VULNERABILITY_STATES } from 'ee/vulnerabilities/constants';
 import { formatDate } from '~/lib/utils/datetime_utility';
-import { convertToSnakeCase } from '~/lib/utils/text_utility';
-import { FIELDS } from 'ee/security_dashboard/components/shared/vulnerability_report/constants';
 import AutoFixHelpText from '../auto_fix_help_text.vue';
 import IssuesBadge from '../issues_badge.vue';
 import SelectionSummary from '../selection_summary.vue';
 import VulnerabilityCommentIcon from '../vulnerability_comment_icon.vue';
+import { FIELDS } from './constants';
 
 export default {
   components: {
@@ -82,12 +81,18 @@ export default {
       type: String,
       required: true,
     },
+    sort: {
+      type: Object,
+      required: false,
+      default: () => ({
+        sortBy: FIELDS.SEVERITY.key,
+        sortDesc: true,
+      }),
+    },
   },
   data() {
     return {
       selectedVulnerabilities: {},
-      sortBy: 'severity',
-      sortDesc: true,
     };
   },
   computed: {
@@ -231,14 +236,8 @@ export default {
     useConvertReportType(reportType) {
       return convertReportType(reportType);
     },
-    handleSortChange(context) {
-      const fieldName = convertToSnakeCase(context.sortBy);
-      const direction = context.sortDesc ? 'desc' : 'asc';
-
-      if (!fieldName) {
-        return;
-      }
-      this.$emit('sort-changed', `${fieldName}_${direction}`);
+    handleSortChange({ sortBy, sortDesc }) {
+      this.$emit('update:sort', { sortBy, sortDesc });
     },
     getVulnerabilityState(state = '') {
       const stateName = state.toLowerCase();
@@ -267,8 +266,8 @@ export default {
       :fields="displayFields"
       :items="vulnerabilities"
       :thead-class="theadClass"
-      :sort-desc="sortDesc"
-      :sort-by="sortBy"
+      :sort-desc="sort.sortDesc"
+      :sort-by="sort.sortBy"
       sort-icon-left
       no-local-sorting
       stacked="sm"

ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue

@@ -4,7 +4,10 @@ import { produce } from 'immer';
 import createFlash from '~/flash';
 import { s__ } from '~/locale';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
+import { convertToSnakeCase } from '~/lib/utils/text_utility';
+import { parseBoolean } from '~/lib/utils/common_utils';
 import VulnerabilityList from './vulnerability_list.vue';
+import { FIELDS } from './constants';
 
 const PAGE_SIZE = 20;
 
@@ -52,7 +55,6 @@ export default {
   data() {
     return {
       vulnerabilities: [],
-      sort: undefined,
       pageInfo: {},
       // The "before" querystring value on page load.
       initialBefore: this.$route.query.before,
@@ -67,7 +69,7 @@ export default {
       variables() {
         return {
           fullPath: this.fullPath,
-          sort: this.sort,
+          sort: `${convertToSnakeCase(this.sort.sortBy)}_${this.sort.sortDesc ? 'desc' : 'asc'}`,
           vetEnabled: this.canViewFalsePositive,
           includeExternalIssueLinks: this.hasJiraVulnerabilitiesIntegrationEnabled,
           // If we're using "after" we need to use "first", and if we're using "before" we need to
@@ -124,6 +126,17 @@ export default {
         }
       },
     },
+    sort: {
+      get() {
+        return {
+          sortBy: this.$route.query.sortBy || FIELDS.SEVERITY.key,
+          sortDesc: this.$route.query.sortDesc ? parseBoolean(this.$route.query.sortDesc) : true,
+        };
+      },
+      set({ sortBy, sortDesc }) {
+        this.pushQuerystring({ sortBy, sortDesc });
+      },
+    },
     // Used to show the infinite scrolling loading spinner.
     isLoadingVulnerabilities() {
       return this.$apollo.queries.vulnerabilities.loading;
@@ -151,19 +164,12 @@ export default {
         this.vulnerabilities = [];
       }
     },
-  },
-  methods: {
-    updateSort(sort) {
-      if (this.shouldUsePagination) {
-        // Reset the paging whenever the sort is changed.
-        this.resetPaging();
-      } else {
+    sort() {
       // Clear out the vulnerabilities so that the skeleton loader is shown.
       this.vulnerabilities = [];
-      }
-
-      this.sort = sort;
     },
+  },
+  methods: {
     fetchNextPage() {
       this.$apollo.queries.vulnerabilities.fetchMore({
         variables: { after: this.pageInfo.endCursor },
@@ -197,9 +203,9 @@ export default {
       :is-loading="shouldShowLoadingSkeleton"
       :vulnerabilities="vulnerabilities"
       :fields="fields"
+      :sort.sync="sort"
       :should-show-project-namespace="showProjectNamespace"
       :portal-name="portalName"
-      @sort-changed="updateSort"
     />
 
     <div v-if="shouldUsePagination" class="gl-text-center gl-mt-6">

ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql_spec.js

 import Vue, { nextTick } from 'vue';
 import VueApollo from 'vue-apollo';
-
 import { GlIntersectionObserver } from '@gitlab/ui';
 import VueRouter from 'vue-router';
 import VulnerabilityListGraphql from 'ee/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue';
@@ -10,6 +9,7 @@ import vulnerabilitiesQuery from 'ee/security_dashboard/graphql/queries/group_vu
 import createMockApollo from 'helpers/mock_apollo_helper';
 import waitForPromises from 'helpers/wait_for_promises';
 import createFlash from '~/flash';
+import { FIELDS } from 'ee/security_dashboard/components/shared/vulnerability_report/constants';
 
 jest.mock('~/flash');
 
@@ -19,6 +19,9 @@ const router = new VueRouter();
 
 const fullPath = 'path';
 const portalName = 'portal-name';
+// Sort object used by tests that don't need to care about what the values are.
+const SORT_OBJECT = { sortBy: 'state', sortDesc: true };
+const DEFAULT_SORT = `${FIELDS.SEVERITY.key}_desc`;
 
 const createVulnerabilitiesRequestHandler = ({ hasNextPage }) =>
   jest.fn().mockResolvedValue({
@@ -76,6 +79,10 @@ describe('Vulnerability list GraphQL component', () => {
   afterEach(() => {
     wrapper.destroy();
     vulnerabilitiesRequestHandler.mockClear();
+    // Reset querystring.
+    if (Object.keys(router.currentRoute.query).length) {
+      router.push({ query: undefined });
+    }
   });
 
   describe('vulnerabilities query', () => {
@@ -140,18 +147,95 @@ describe('Vulnerability list GraphQL component', () => {
       });
     });
 
-    it('calls the vulnerabilities query with the data from the sort-changed event', async () => {
+    it('calls the GraphQL query with the expected sort data when the vulnerability list changes the sort', async () => {
+      createWrapper();
+      vulnerabilitiesRequestHandler.mockClear();
+      findVulnerabilityList().vm.$emit('update:sort', SORT_OBJECT);
+      await nextTick();
+
+      expect(vulnerabilitiesRequestHandler).toHaveBeenCalledWith(
+        expect.objectContaining({ sort: 'state_desc' }),
+      );
+    });
+  });
+
+  describe('sorting', () => {
+    it.each`
+      sortBy           | sortDesc | expected
+      ${'state'}       | ${true}  | ${'state_desc'}
+      ${'detected'}    | ${false} | ${'detected_asc'}
+      ${'description'} | ${true}  | ${'description_desc'}
+      ${'reportType'}  | ${false} | ${'report_type_asc'}
+    `(
+      'reads the querystring sort info and calls the GraphQL query with "$expected" for sort',
+      ({ sortBy, sortDesc, expected }) => {
+        router.push({ query: { sortBy, sortDesc } });
         createWrapper();
-      // First call should be undefined, which uses the default sort.
+
+        // This is important; we want the sort info to be read from the querystring from the
+        // beginning so that the GraphQL request is only done once, instead of starting with the
+        // default sort, then immediately reading the querystring value, which will trigger the
+        // GraphQL request twice.
+        expect(vulnerabilitiesRequestHandler).toHaveBeenCalledTimes(1);
+        expect(vulnerabilitiesRequestHandler).toHaveBeenCalledWith(
+          expect.objectContaining({ sort: expected }),
+        );
+      },
+    );
+
+    it(`uses the default sort if there's no querystring data`, () => {
+      createWrapper();
+
       expect(vulnerabilitiesRequestHandler).toHaveBeenCalledWith(
-        expect.objectContaining({ sort: undefined }),
+        expect.objectContaining({ sort: DEFAULT_SORT }),
       );
+    });
 
-      const sort = 'sort';
-      findVulnerabilityList().vm.$emit('sort-changed', sort);
+    it('passes the sort data to the vulnerability list', () => {
+      router.push({ query: SORT_OBJECT });
+      createWrapper();
+
+      expect(findVulnerabilityList().props('sort')).toEqual(SORT_OBJECT);
+    });
+
+    it('calls the GraphQL query with the expected sort data when the vulnerability list changes the sort', async () => {
+      createWrapper();
+      findVulnerabilityList().vm.$emit('update:sort', SORT_OBJECT);
       await nextTick();
 
-      expect(vulnerabilitiesRequestHandler).toHaveBeenCalledWith(expect.objectContaining({ sort }));
+      expect(vulnerabilitiesRequestHandler).toHaveBeenCalledWith(
+        expect.objectContaining({ sort: 'state_desc' }),
+      );
+    });
+
+    it.each`
+      sortBy           | sortDesc
+      ${'state'}       | ${true}
+      ${'detected'}    | ${false}
+      ${'description'} | ${true}
+      ${'reportType'}  | ${false}
+    `(
+      'updates the querystring to sortBy = "$sortBy", sortDesc = "$sortDesc" when the vulnerability list changes the sort',
+      ({ sortBy, sortDesc }) => {
+        createWrapper();
+        findVulnerabilityList().vm.$emit('update:sort', { sortBy, sortDesc });
+
+        expect(router.currentRoute.query).toMatchObject({ sortBy, sortDesc: sortDesc.toString() });
+      },
+    );
+
+    // This test is for when the querystring is changed by the user clicking forward/back in the
+    // browser. When a history state is pushed that only changes the querystring, the page does not
+    // refresh, so we need to handle the case where the user is stepping through the browser history
+    // but no page refresh is done.
+    it('calls the GraphQL query with the expected sort data when the querystring is changed', async () => {
+      createWrapper();
+      router.push({ query: { sortBy: 'state', sortDesc: true } });
+      await nextTick();
+
+      expect(vulnerabilitiesRequestHandler).toHaveBeenCalledWith(
+        expect.objectContaining({ sort: 'state_desc' }),
+      );
     });
   });
 

ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_spec.js

@@ -571,20 +571,24 @@ describe('Vulnerability list component', () => {
     });
   });
 
-  describe('sort-changed listener', () => {
-    it('does not emit when sort by data is empty', () => {
-      createWrapper();
+  describe('sorting', () => {
+    it('passes the sort prop to the table', () => {
+      const sort = { sortBy: 'a', sortDesc: true };
+      createWrapper({ stubs: { GlTable: true }, props: { sort } });
 
-      findTable().vm.$emit('sort-changed', { sortBy: '', sortDesc: true });
-      expect(wrapper.emitted('sort-changed')).toBe(undefined);
+      expect(findTable().attributes()).toMatchObject({
+        'sort-by': sort.sortBy,
+        'sort-desc': sort.sortDesc.toString(),
+      });
     });
 
-    it('emits sort by data in expected format', () => {
+    it('emits sort data in expected format', () => {
       createWrapper();
 
-      findTable().vm.$emit('sort-changed', { sortBy: 'state', sortDesc: true });
+      const sort = { sortBy: 'state', sortDesc: true };
+      findTable().vm.$emit('sort-changed', sort);
 
-      expect(wrapper.emitted('sort-changed')[0][0]).toBe('state_desc');
+      expect(wrapper.emitted('update:sort')[0][0]).toEqual(sort);
     });
   });