Merge branch '217530-improve-bulk-dismissal' into 'master'

Remove item when dismissed on security dashboard if no longer in filter

See merge request gitlab-org/gitlab!43468

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

@@ -23,6 +23,9 @@ export default {
     popoverTitle() {
       return n__('1 Issue', '%d Issues', this.numberOfIssues);
     },
+    issueBadgeEl() {
+      return () => this.$refs.issueBadge?.$el;
+    },
   },
 };
 </script>
@@ -33,7 +36,7 @@ export default {
       <gl-icon name="issues" class="gl-mr-2" />
       {{ numberOfIssues }}
     </gl-badge>
-    <gl-popover ref="popover" :target="() => $refs.issueBadge.$el" triggers="hover" placement="top">
+    <gl-popover ref="popover" :target="issueBadgeEl" triggers="hover" placement="top">
       <template #title>
         {{ popoverTitle }}
       </template>

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

@@ -2,7 +2,7 @@
 import { GlButton, GlFormSelect } from '@gitlab/ui';
 import { s__, n__ } from '~/locale';
 import toast from '~/vue_shared/plugins/global_toast';
-import { deprecatedCreateFlash as createFlash } from '~/flash';
+import createFlash from '~/flash';
 import vulnerabilityDismiss from '../graphql/vulnerability_dismiss.mutation.graphql';
 
 const REASON_NONE = s__('SecurityReports|[No reason]');
@@ -48,30 +48,46 @@ export default {
       this.dismissSelectedVulnerabilities();
     },
     dismissSelectedVulnerabilities() {
+      let fulfilledCount = 0;
+      let rejectedCount = 0;
+
       const promises = this.selectedVulnerabilities.map(vulnerability =>
-        this.$apollo.mutate({
-          mutation: vulnerabilityDismiss,
-          variables: { id: vulnerability.id, comment: this.dismissalReason },
-        }),
+        this.$apollo
+          .mutate({
+            mutation: vulnerabilityDismiss,
+            variables: { id: vulnerability.id, comment: this.dismissalReason },
+          })
+          .then(() => {
+            fulfilledCount += 1;
+            this.$emit('vulnerability-updated', vulnerability.id);
+          })
+          .catch(() => {
+            rejectedCount += 1;
+          }),
       );
 
       Promise.all(promises)
         .then(() => {
-          toast(
-            n__(
-              '%d vulnerability dismissed',
-              '%d vulnerabilities dismissed',
-              this.selectedVulnerabilities.length,
-            ),
-          );
+          if (fulfilledCount > 0) {
+            toast(
+              n__('%d vulnerability dismissed', '%d vulnerabilities dismissed', fulfilledCount),
+            );
+          }
 
-          this.$emit('deselect-all-vulnerabilities');
+          if (rejectedCount > 0) {
+            createFlash({
+              message: n__(
+                'SecurityReports|There was an error dismissing %d vulnerability. Please try again later.',
+                'SecurityReports|There was an error dismissing %d vulnerabilities. Please try again later.',
+                rejectedCount,
+              ),
+            });
+          }
         })
         .catch(() => {
-          createFlash(
-            s__('SecurityReports|There was an error dismissing the vulnerabilities.'),
-            'alert',
-          );
+          createFlash({
+            message: s__('SecurityReports|There was an error dismissing the vulnerabilities.'),
+          });
         });
     },
   },

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

@@ -15,6 +15,7 @@ import SeverityBadge from 'ee/vue_shared/security_reports/components/severity_ba
 import VulnerabilityCommentIcon from 'ee/security_dashboard/components/vulnerability_comment_icon.vue';
 import convertReportType from 'ee/vue_shared/security_reports/store/utils/convert_report_type';
 import getPrimaryIdentifier from 'ee/vue_shared/security_reports/store/utils/get_primary_identifier';
+import { VULNERABILITY_STATES } from 'ee/vulnerabilities/constants';
 import SecurityScannerAlert from './security_scanner_alert.vue';
 import LocalStorageSync from '~/vue_shared/components/local_storage_sync.vue';
 import { formatDate } from '~/lib/utils/datetime_utility';
@@ -87,11 +88,19 @@ export default {
     };
   },
   computed: {
+    // This is a workaround to remove vulnerabilities from the list when their state has changed
+    // through the bulk update feature, but no longer matches the filters. For more details:
+    // https://gitlab.com/gitlab-org/gitlab/-/merge_requests/43468#note_420050017
+    filteredVulnerabilities() {
+      return this.vulnerabilities.filter(x =>
+        this.filters.state?.length ? this.filters.state.includes(x.state) : true,
+      );
+    },
     isSortable() {
       return Boolean(this.$listeners['sort-changed']);
     },
     hasAnyScannersOtherThanGitLab() {
-      return this.vulnerabilities.some(v => v.scanner?.vendor !== 'GitLab');
+      return this.filteredVulnerabilities.some(v => v.scanner?.vendor !== 'GitLab');
     },
     notEnabledSecurityScanners() {
       const { available = [], enabled = [] } = this.securityScanners;
@@ -112,16 +121,16 @@ export default {
       return Object.keys(this.filters).length > 0;
     },
     hasSelectedAllVulnerabilities() {
-      if (!this.vulnerabilities.length) {
+      if (!this.filteredVulnerabilities.length) {
         return false;
       }
-      return this.numOfSelectedVulnerabilities === this.vulnerabilities.length;
+      return this.numOfSelectedVulnerabilities === this.filteredVulnerabilities.length;
     },
     numOfSelectedVulnerabilities() {
       return Object.keys(this.selectedVulnerabilities).length;
     },
     shouldShowSelectionSummary() {
-      return this.shouldShowSelection && Boolean(this.numOfSelectedVulnerabilities);
+      return this.shouldShowSelection && this.numOfSelectedVulnerabilities > 0;
     },
     theadClass() {
       return this.shouldShowSelectionSummary ? 'below-selection-summary' : '';
@@ -195,8 +204,8 @@ export default {
     filters() {
       this.selectedVulnerabilities = {};
     },
-    vulnerabilities(vulnerabilities) {
-      const ids = new Set(vulnerabilities.map(v => v.id));
+    filteredVulnerabilities() {
+      const ids = new Set(this.filteredVulnerabilities.map(v => v.id));
 
       Object.keys(this.selectedVulnerabilities).forEach(vulnerabilityId => {
         if (!ids.has(vulnerabilityId)) {
@@ -219,6 +228,9 @@ export default {
 
       return file;
     },
+    deselectVulnerability(vulnerabilityId) {
+      this.$delete(this.selectedVulnerabilities, vulnerabilityId);
+    },
     deselectAllVulnerabilities() {
       this.selectedVulnerabilities = {};
     },
@@ -282,6 +294,11 @@ export default {
         this.$emit('sort-changed', { ...args, sortBy: convertToSnakeCase(args.sortBy) });
       }
     },
+    getVulnerabilityState(state = '') {
+      const stateName = state.toLowerCase();
+      // Use the raw state name if we don't have a localization for it.
+      return VULNERABILITY_STATES[stateName] || stateName;
+    },
   },
   VULNERABILITIES_PER_PAGE,
   SCANNER_ALERT_DISMISSED_LOCAL_STORAGE_KEY,
@@ -298,12 +315,12 @@ export default {
     <selection-summary
       v-if="shouldShowSelectionSummary"
       :selected-vulnerabilities="Object.values(selectedVulnerabilities)"
-      @deselect-all-vulnerabilities="deselectAllVulnerabilities"
+      @vulnerability-updated="deselectVulnerability"
     />
     <gl-table
       :busy="isLoading"
       :fields="fields"
-      :items="vulnerabilities"
+      :items="filteredVulnerabilities"
       :thead-class="theadClass"
       :sort-desc="sortDesc"
       :sort-by="sortBy"
@@ -313,6 +330,7 @@ export default {
       class="vulnerability-list"
       show-empty
       responsive
+      primary-key="id"
       @sort-changed="handleSortChange"
     >
       <template #head(checkbox)>
@@ -350,7 +368,7 @@ export default {
       </template>
 
       <template #cell(state)="{ item }">
-        <span class="text-capitalize js-status">{{ item.state.toLowerCase() }}</span>
+        <span class="text-capitalize js-status">{{ getVulnerabilityState(item.state) }}</span>
       </template>
 
       <template #cell(severity)="{ item }">

ee/changelogs/unreleased/217530-improve-bulk-dismissal.yml

+---
+title: Remove item when dismissed on security dashboard if it no longer matches filter
+merge_request: 43468
+author:
+type: changed

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

@@ -14,7 +14,7 @@ export const generateVulnerabilities = () => [
     ],
     title: 'Vulnerability 0',
     severity: 'critical',
-    state: 'dismissed',
+    state: 'DISMISSED',
     reportType: 'SAST',
     resolvedOnDefaultBranch: true,
     location: {
@@ -39,7 +39,7 @@ export const generateVulnerabilities = () => [
     ],
     title: 'Vulnerability 1',
     severity: 'high',
-    state: 'opened',
+    state: 'DETECTED',
     reportType: 'DEPENDENCY_SCANNING',
     location: {
       file: 'src/main/java/com/gitlab/security_products/tests/App.java',
@@ -58,7 +58,7 @@ export const generateVulnerabilities = () => [
     identifiers: [],
     title: 'Vulnerability 2',
     severity: 'high',
-    state: 'opened',
+    state: 'DETECTED',
     reportType: 'CUSTOM_SCANNER_WITHOUT_TRANSLATION',
     location: {
       file: 'src/main/java/com/gitlab/security_products/tests/App.java',
@@ -74,7 +74,7 @@ export const generateVulnerabilities = () => [
     id: 'id_3',
     title: 'Vulnerability 3',
     severity: 'high',
-    state: 'opened',
+    state: 'DETECTED',
     location: {
       file: 'yarn.lock',
     },
@@ -87,7 +87,7 @@ export const generateVulnerabilities = () => [
     id: 'id_4',
     title: 'Vulnerability 4',
     severity: 'critical',
-    state: 'dismissed',
+    state: 'DISMISSED',
     location: {},
     project: {
       nameWithNamespace: 'Administrator / Security reports',

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

@@ -2,7 +2,7 @@ import { mount } from '@vue/test-utils';
 import SelectionSummary from 'ee/security_dashboard/components/selection_summary.vue';
 import { GlFormSelect, GlButton } from '@gitlab/ui';
 import waitForPromises from 'helpers/wait_for_promises';
-import { deprecatedCreateFlash as createFlash } from '~/flash';
+import createFlash from '~/flash';
 import toast from '~/vue_shared/plugins/global_toast';
 
 jest.mock('~/flash');
@@ -25,11 +25,7 @@ describe('Selection Summary component', () => {
   const dismissButton = () => wrapper.find(GlButton);
   const dismissMessage = () => wrapper.find({ ref: 'dismiss-message' });
   const formSelect = () => wrapper.find(GlFormSelect);
-  const createComponent = ({ props = {}, data = defaultData, mocks = defaultMocks }) => {
-    if (wrapper) {
-      throw new Error('Please avoid recreating components in the same spec');
-    }
-
+  const createComponent = ({ props = {}, data = defaultData, mocks = defaultMocks } = {}) => {
     spyMutate = mocks.$apollo.mutate;
     wrapper = mount(SelectionSummary, {
       mocks: {
@@ -63,7 +59,7 @@ describe('Selection Summary component', () => {
         expect(dismissButton().attributes('disabled')).toBe('disabled');
       });
 
-      it('should have the button enabled if a vulnerability is selected and an option is selected', () => {
+      it('should have the button enabled if a vulnerability is selected and an option is selected', async () => {
         expect(wrapper.vm.dismissalReason).toBe(null);
         expect(wrapper.findAll('option')).toHaveLength(4);
         formSelect()
@@ -71,15 +67,15 @@ describe('Selection Summary component', () => {
           .at(1)
           .setSelected();
         formSelect().trigger('change');
-        return wrapper.vm.$nextTick().then(() => {
-          expect(wrapper.vm.dismissalReason).toEqual(expect.any(String));
-          expect(dismissButton().attributes('disabled')).toBe(undefined);
-        });
+        await wrapper.vm.$nextTick();
+
+        expect(wrapper.vm.dismissalReason).toEqual(expect.any(String));
+        expect(dismissButton().attributes('disabled')).toBe(undefined);
       });
     });
   });
 
-  describe('with 1 vulnerabilities selected', () => {
+  describe('with multiple vulnerabilities selected', () => {
     beforeEach(() => {
       createComponent({ props: { selectedVulnerabilities: [{ id: 'id_0' }, { id: 'id_1' }] } });
     });
@@ -93,10 +89,12 @@ describe('Selection Summary component', () => {
     let mutateMock;
 
     beforeEach(() => {
-      mutateMock = jest.fn().mockResolvedValue();
+      mutateMock = jest.fn(data =>
+        data.variables.id % 2 === 0 ? Promise.resolve() : Promise.reject(),
+      );
 
       createComponent({
-        props: { selectedVulnerabilities: [{ id: 'id_0' }, { id: 'id_1' }] },
+        props: { selectedVulnerabilities: [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }, { id: 5 }] },
         data: { dismissalReason: 'Will Not Fix' },
         mocks: { $apollo: { mutate: mutateMock } },
       });
@@ -104,31 +102,29 @@ describe('Selection Summary component', () => {
 
     it('should make an API request for each vulnerability', () => {
       dismissButton().trigger('submit');
-      expect(spyMutate).toHaveBeenCalledTimes(2);
+      expect(spyMutate).toHaveBeenCalledTimes(5);
     });
 
-    it('should show toast with the right message if all calls were successful', () => {
+    it('should show toast with the right message for the successful calls', async () => {
       dismissButton().trigger('submit');
-      return waitForPromises().then(() => {
-        expect(toast).toHaveBeenCalledWith('2 vulnerabilities dismissed');
-      });
+      await waitForPromises();
+
+      expect(toast).toHaveBeenCalledWith('2 vulnerabilities dismissed');
     });
 
-    it('should show flash with the right message if some calls failed', () => {
-      mutateMock.mockRejectedValue();
+    it('should show flash with the right message for the failed calls', async () => {
       dismissButton().trigger('submit');
-      return waitForPromises().then(() => {
-        expect(createFlash).toHaveBeenCalledWith(
-          'There was an error dismissing the vulnerabilities.',
-          'alert',
-        );
+      await waitForPromises();
+
+      expect(createFlash).toHaveBeenCalledWith({
+        message: 'There was an error dismissing 3 vulnerabilities. Please try again later.',
       });
     });
   });
 
   describe('when vulnerabilities are not selected', () => {
     beforeEach(() => {
-      createComponent({});
+      createComponent();
     });
 
     it('should have the button disabled', () => {

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

@@ -12,6 +12,7 @@ import VulnerabilityList, {
 import FiltersProducedNoResults from 'ee/security_dashboard/components/empty_states/filters_produced_no_results.vue';
 import DashboardHasNoVulnerabilities from 'ee/security_dashboard/components/empty_states/dashboard_has_no_vulnerabilities.vue';
 import { trimText } from 'helpers/text_helper';
+import { capitalize } from 'lodash';
 import { generateVulnerabilities, vulnerabilities } from './mock_data';
 
 describe('Vulnerability list component', () => {
@@ -19,7 +20,7 @@ describe('Vulnerability list component', () => {
 
   let wrapper;
 
-  const createWrapper = ({ props = {}, listeners }) => {
+  const createWrapper = ({ props = {}, listeners } = {}) => {
     return mount(VulnerabilityList, {
       propsData: {
         vulnerabilities: [],
@@ -42,7 +43,9 @@ describe('Vulnerability list component', () => {
   const findTable = () => wrapper.find(GlTable);
   const findSortableColumn = () => wrapper.find('[aria-sort="descending"]');
   const findCell = label => wrapper.find(`.js-${label}`);
-  const findRow = (index = 0) => wrapper.findAll('tbody tr').at(index);
+  const findRows = () => wrapper.findAll('tbody tr');
+  const findRow = (index = 0) => findRows().at(index);
+  const findRowById = id => wrapper.find(`tbody tr[data-pk="${id}"`);
   const findIssuesBadge = () => wrapper.find(IssuesBadge);
   const findRemediatedBadge = () => wrapper.find(RemediatedBadge);
   const findSecurityScannerAlert = () => wrapper.find(SecurityScannerAlert);
@@ -76,7 +79,7 @@ describe('Vulnerability list component', () => {
     it('should correctly render the status', () => {
       const cell = findCell('status');
 
-      expect(cell.text()).toBe(newVulnerabilities[0].state);
+      expect(cell.text()).toBe(capitalize(newVulnerabilities[0].state));
     });
 
     it('should correctly render the severity', () => {
@@ -133,12 +136,11 @@ describe('Vulnerability list component', () => {
       expect(findSelectionSummary().exists()).toBe(false);
     });
 
-    it('should show the selection summary when a checkbox is selected', () => {
+    it('should show the selection summary when a checkbox is selected', async () => {
       findDataCell('vulnerability-checkbox').setChecked(true);
+      await wrapper.vm.$nextTick();
 
-      return wrapper.vm.$nextTick().then(() => {
-        expect(findSelectionSummary().exists()).toBe(true);
-      });
+      expect(findSelectionSummary().exists()).toBe(true);
     });
 
     it('should sync selected vulnerabilities when the vulnerability list is updated', async () => {
@@ -154,6 +156,18 @@ describe('Vulnerability list component', () => {
 
       expect(findSelectionSummary().exists()).toBe(false);
     });
+
+    it('should uncheck a selected vulnerability after the vulnerability is updated', async () => {
+      const checkbox = () => findDataCell('vulnerability-checkbox');
+      checkbox().setChecked(true);
+      expect(checkbox().element.checked).toBe(true);
+
+      await wrapper.vm.$nextTick();
+      findSelectionSummary().vm.$emit('vulnerability-updated', newVulnerabilities[0].id);
+      await wrapper.vm.$nextTick();
+
+      expect(checkbox().element.checked).toBe(false);
+    });
   });
 
   describe('when vulnerability selection is disabled', () => {
@@ -371,7 +385,7 @@ describe('Vulnerability list component', () => {
 
   describe('with no vulnerabilities when there are no filters', () => {
     beforeEach(() => {
-      wrapper = createWrapper({});
+      wrapper = createWrapper();
     });
 
     it('should show the empty state', () => {
@@ -398,6 +412,26 @@ describe('Vulnerability list component', () => {
     });
   });
 
+  describe('with vulnerabilities when there are filters', () => {
+    it.each`
+      state
+      ${['DETECTED']}
+      ${['DISMISSED']}
+      ${[]}
+      ${['DETECTED', 'DISMISSED']}
+    `('should only show vulnerabilities that match filter $state', state => {
+      wrapper = createWrapper({ props: { vulnerabilities, filters: { state } } });
+      const filteredVulnerabilities = vulnerabilities.filter(x =>
+        state.length ? state.includes(x.state) : true,
+      );
+      expect(findRows().length).toBe(filteredVulnerabilities.length);
+
+      filteredVulnerabilities.forEach(vulnerability => {
+        expect(findRowById(vulnerability.id).exists()).toBe(true);
+      });
+    });
+  });
+
   describe('security scanner alerts', () => {
     describe.each`
       available   | enabled     | pipelineRun | expectAlertShown
@@ -492,7 +526,7 @@ describe('Vulnerability list component', () => {
 
   describe('when does not have a sort-changed listener defined', () => {
     beforeEach(() => {
-      wrapper = createWrapper({});
+      wrapper = createWrapper();
     });
 
     it('is not sortable', () => {

locale/gitlab.pot

@@ -23027,6 +23027,11 @@ msgstr ""
 msgid "SecurityReports|There was an error deleting the comment."
 msgstr ""
 
+msgid "SecurityReports|There was an error dismissing %d vulnerability. Please try again later."
+msgid_plural "SecurityReports|There was an error dismissing %d vulnerabilities. Please try again later."
+msgstr[0] ""
+msgstr[1] ""
+
 msgid "SecurityReports|There was an error dismissing the vulnerabilities."
 msgstr ""