Merge branch '299369-add-sorting-to-table-columns-in-the-compliance-report' into 'master'

Add sorting to compliance report violations table

See merge request gitlab-org/gitlab!79210

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

+import { convertToSnakeCase, convertToCamelCase } from '~/lib/utils/text_utility';
 import { DEFAULT_TH_CLASSES } from './constants';
 
 /**
@@ -7,3 +8,37 @@ import { DEFAULT_TH_CLASSES } from './constants';
  * @returns {String} The classes to be used in GlTable fields object.
  */
 export const thWidthClass = (width) => `gl-w-${width}p ${DEFAULT_TH_CLASSES}`;
+
+/**
+ * Converts a GlTable sort-changed event object into string format.
+ * This string can be used as a sort argument on GraphQL queries.
+ *
+ * @param {Object} - The table state context object.
+ * @returns {String} A string with the sort key and direction, for example 'NAME_DESC'.
+ */
+export const sortObjectToString = ({ sortBy, sortDesc }) => {
+  const sortingDirection = sortDesc ? 'DESC' : 'ASC';
+  const sortingColumn = convertToSnakeCase(sortBy).toUpperCase();
+
+  return `${sortingColumn}_${sortingDirection}`;
+};
+
+/**
+ * Converts a sort string into a sort state object that can be used to
+ * set the sort order on GlTable.
+ *
+ * @param {String} - The string with the sort key and direction, for example 'NAME_DESC'.
+ * @returns {Object} An object with the sortBy and sortDesc properties.
+ */
+export const sortStringToObject = (sortString) => {
+  let sortBy = null;
+  let sortDesc = null;
+
+  if (sortString && sortString.includes('_')) {
+    const [key, direction] = sortString.split(/_(ASC|DESC)$/);
+    sortBy = convertToCamelCase(key.toLowerCase());
+    sortDesc = direction === 'DESC';
+  }
+
+  return { sortBy, sortDesc };
+};

ee/app/assets/javascripts/compliance_dashboard/components/report.vue

@@ -2,7 +2,7 @@
 import { GlAlert, GlLoadingIcon, GlTable, GlLink } from '@gitlab/ui';
 import * as Sentry from '@sentry/browser';
 import { s__, __ } from '~/locale';
-import { thWidthClass } from '~/lib/utils/table_utility';
+import { thWidthClass, sortObjectToString, sortStringToObject } from '~/lib/utils/table_utility';
 import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue';
 import { DRAWER_Z_INDEX } from '~/lib/utils/constants';
 import UrlSync from '~/vue_shared/components/url_sync.vue';
@@ -10,6 +10,7 @@ import { helpPagePath } from '~/helpers/help_page_helper';
 import SeverityBadge from 'ee/vue_shared/security_reports/components/severity_badge.vue';
 import complianceViolationsQuery from '../graphql/compliance_violations.query.graphql';
 import { mapViolations } from '../graphql/mappers';
+import { DEFAULT_SORT } from '../constants';
 import { parseViolationsQueryFilter } from '../utils';
 import MergeCommitsExportButton from './merge_requests/merge_commits_export_button.vue';
 import MergeRequestDrawer from './drawer.vue';
@@ -47,6 +48,9 @@ export default {
     },
   },
   data() {
+    const sortParam = this.defaultQuery.sort || DEFAULT_SORT;
+    const { sortBy, sortDesc } = sortStringToObject(sortParam);
+
     return {
       urlQuery: { ...this.defaultQuery },
       queryError: false,
@@ -54,6 +58,9 @@ export default {
       showDrawer: false,
       drawerMergeRequest: {},
       drawerProject: {},
+      sortBy,
+      sortDesc,
+      sortParam,
     };
   },
   apollo: {
@@ -63,6 +70,7 @@ export default {
         return {
           fullPath: this.groupPath,
           filter: parseViolationsQueryFilter(this.urlQuery),
+          sort: this.sortParam,
         };
       },
       update(data) {
@@ -83,6 +91,10 @@ export default {
     },
   },
   methods: {
+    handleSortChanged(sortState) {
+      this.sortParam = sortObjectToString(sortState);
+      this.updateUrlQuery({ ...this.urlQuery, sort: this.sortParam });
+    },
     toggleDrawer(rows) {
       const { id, mergeRequest, project } = rows[0] || {};
 
@@ -108,7 +120,7 @@ export default {
     updateUrlQuery({ projectIds = [], ...rest }) {
       this.urlQuery = {
         // Clear the URL param when the id array is empty
-        projectIds: projectIds.length > 0 ? projectIds : null,
+        projectIds: projectIds?.length > 0 ? projectIds : null,
         ...rest,
       };
     },
@@ -118,21 +130,25 @@ export default {
       key: 'severity',
       label: __('Severity'),
       thClass: thWidthClass(10),
+      sortable: true,
     },
     {
-      key: 'reason',
+      key: 'violationReason',
       label: __('Violation'),
       thClass: thWidthClass(15),
+      sortable: true,
     },
     {
-      key: 'mergeRequest',
+      key: 'mergeRequestTitle',
       label: __('Merge request'),
       thClass: thWidthClass(40),
+      sortable: true,
     },
     {
       key: 'mergedAt',
       label: __('Date merged'),
       thClass: thWidthClass(20),
+      sortable: true,
     },
   ],
   i18n: {
@@ -185,6 +201,10 @@ export default {
       :busy="isLoading"
       :empty-text="$options.i18n.noViolationsFound"
       :selectable="true"
+      :sort-by="sortBy"
+      :sort-desc="sortDesc"
+      no-local-sorting
+      sort-icon-left
       show-empty
       head-variant="white"
       stacked="lg"
@@ -193,14 +213,15 @@ export default {
       selected-variant="primary"
       thead-class="gl-border-b-solid gl-border-b-1 gl-border-b-gray-100"
       @row-selected="toggleDrawer"
+      @sort-changed="handleSortChanged"
     >
       <template #cell(severity)="{ item: { severity } }">
         <severity-badge class="gl-reset-text-align!" :severity="severity" />
       </template>
-      <template #cell(reason)="{ item: { reason, violatingUser } }">
+      <template #cell(violationReason)="{ item: { reason, violatingUser } }">
         <violation-reason :reason="reason" :user="violatingUser" />
       </template>
-      <template #cell(mergeRequest)="{ item: { mergeRequest } }">
+      <template #cell(mergeRequestTitle)="{ item: { mergeRequest } }">
         {{ mergeRequest.title }}
       </template>
       <template #cell(mergedAt)="{ item: { mergeRequest } }">

ee/app/assets/javascripts/compliance_dashboard/constants.js

@@ -36,3 +36,5 @@ export const MERGE_REQUEST_VIOLATION_SEVERITY_LEVELS = {
   3: 'low',
   4: 'info',
 };
+
+export const DEFAULT_SORT = 'SEVERITY_DESC';

ee/app/assets/javascripts/compliance_dashboard/graphql/compliance_violations.query.graphql

 # TODO: Add the correct filter type once it has been added in https://gitlab.com/gitlab-org/gitlab/-/issues/347325
-query getComplianceViolations($fullPath: ID!, $filter: Object) {
-  group(fullPath: $fullPath, filter: $filter) @client {
+query getComplianceViolations($fullPath: ID!, $filter: Object, $sort: String) {
+  group(fullPath: $fullPath, filter: $filter, sort: $sort) @client {
     id
     mergeRequestViolations {
       nodes {

ee/spec/frontend/compliance_dashboard/components/report_spec.js

@@ -18,7 +18,9 @@ import createMockApollo from 'helpers/mock_apollo_helper';
 import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue';
 import UrlSync from '~/vue_shared/components/url_sync.vue';
 import { stubComponent } from 'helpers/stub_component';
+import { sortObjectToString } from '~/lib/utils/table_utility';
 import { parseViolationsQueryFilter } from 'ee/compliance_dashboard/utils';
+import { DEFAULT_SORT } from 'ee/compliance_dashboard/constants';
 
 Vue.use(VueApollo);
 
@@ -34,6 +36,7 @@ describe('ComplianceReport component', () => {
     projectIds: ['20'],
     createdAfter,
     createdBefore,
+    sort: DEFAULT_SORT,
   };
   const mockGraphQlError = new Error('GraphQL networkError');
 
@@ -49,7 +52,7 @@ describe('ComplianceReport component', () => {
   const findViolationFilter = () => wrapper.findComponent(ViolationFilter);
   const findUrlSync = () => wrapper.findComponent(UrlSync);
 
-  const findTableHeaders = () => findViolationsTable().findAll('th');
+  const findTableHeaders = () => findViolationsTable().findAll('th div');
   const findTablesFirstRowData = () =>
     findViolationsTable().findAll('tbody > tr').at(0).findAll('td');
   const findSelectedRows = () => findViolationsTable().findAll('tr.b-table-row-selected');
@@ -131,12 +134,33 @@ describe('ComplianceReport component', () => {
       expect(findTableLoadingIcon().exists()).toBe(true);
     });
 
-    it('fetches the list of merge request violations with the filter query', async () => {
+    it('fetches the list of merge request violations with the default filter and sort params', async () => {
       expect(mockResolver).toHaveBeenCalledTimes(1);
       expect(mockResolver).toHaveBeenCalledWith(
         ...expectApolloVariables({
           fullPath: groupPath,
           filter: parseViolationsQueryFilter(defaultQuery),
+          sort: DEFAULT_SORT,
+        }),
+      );
+    });
+  });
+
+  describe('when the defaultQuery has a sort param', () => {
+    const sort = 'SEVERITY_ASC';
+
+    beforeEach(() => {
+      mockResolver = jest.fn();
+      wrapper = createComponent(mount, { defaultQuery: { ...defaultQuery, sort } });
+    });
+
+    it('fetches the list of merge request violations with sort params', async () => {
+      expect(mockResolver).toHaveBeenCalledTimes(1);
+      expect(mockResolver).toHaveBeenCalledWith(
+        ...expectApolloVariables({
+          fullPath: groupPath,
+          filter: parseViolationsQueryFilter(defaultQuery),
+          sort,
         }),
       );
     });
@@ -313,11 +337,46 @@ describe('ComplianceReport component', () => {
             ...expectApolloVariables({
               fullPath: groupPath,
               filter: parseViolationsQueryFilter(query),
+              sort: DEFAULT_SORT,
             }),
           );
         });
       });
     });
+
+    describe('when the table sort changes', () => {
+      const sortState = { sortBy: 'mergedAt', sortDesc: true };
+
+      beforeEach(async () => {
+        mockResolver = jest.fn().mockReturnValue(resolvers.Query.group());
+        wrapper = createComponent(mount);
+
+        await waitForPromises();
+        await findViolationsTable().vm.$emit('sort-changed', sortState);
+      });
+
+      it('updates the URL query', () => {
+        expect(findUrlSync().props('query')).toMatchObject({
+          sort: sortObjectToString(sortState),
+        });
+      });
+
+      it('shows the table loading icon', () => {
+        expect(findTableLoadingIcon().exists()).toBe(true);
+      });
+
+      it('fetches the sorted violations', async () => {
+        expect(mockResolver).toHaveBeenCalledTimes(2);
+        expect(mockResolver).toHaveBeenNthCalledWith(
+          2,
+          ...expectApolloVariables({
+            fullPath: groupPath,
+            filter: parseViolationsQueryFilter(defaultQuery),
+            sort: sortObjectToString(sortState),
+          }),
+        );
+      });
+    });
   });
 
   describe('when there are no violations', () => {

spec/frontend/lib/utils/table_utility_spec.js

@@ -8,4 +8,33 @@ describe('table_utility', () => {
       expect(tableUtils.thWidthClass(width)).toBe(`gl-w-${width}p ${DEFAULT_TH_CLASSES}`);
     });
   });
+
+  describe('sortObjectToString', () => {
+    it('returns the expected sorting string ending in "DESC" when sortDesc is true', () => {
+      expect(tableUtils.sortObjectToString({ sortBy: 'mergedAt', sortDesc: true })).toBe(
+        'MERGED_AT_DESC',
+      );
+    });
+
+    it('returns the expected sorting string ending in "ASC" when sortDesc is false', () => {
+      expect(tableUtils.sortObjectToString({ sortBy: 'mergedAt', sortDesc: false })).toBe(
+        'MERGED_AT_ASC',
+      );
+    });
+  });
+
+  describe('sortStringToObject', () => {
+    it.each`
+      sortBy        | sortDesc | sortString
+      ${'mergedAt'} | ${true}  | ${'MERGED_AT_DESC'}
+      ${'mergedAt'} | ${false} | ${'MERGED_AT_ASC'}
+      ${'severity'} | ${true}  | ${'SEVERITY_DESC'}
+      ${'severity'} | ${false} | ${'SEVERITY_ASC'}
+    `(
+      'returns $sortString when sortBy = "$sortBy" and sortDesc = "sortDesc"',
+      ({ sortBy, sortDesc, sortString }) => {
+        expect(tableUtils.sortStringToObject(sortString)).toStrictEqual({ sortBy, sortDesc });
+      },
+    );
+  });
 });