Merge branch '320885-show-loading-for-vulnerability-list' into 'master'

Show loading state for vulnerability list when filter/sort is changed

See merge request gitlab-org/gitlab!53934

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

@@ -64,6 +64,16 @@ export default {
       return `${this.sortBy}_${this.sortDirection}`;
     },
   },
+  watch: {
+    filters() {
+      // Clear out the existing vulnerabilities so that the skeleton loader is shown.
+      this.vulnerabilities = [];
+    },
+    sort() {
+      // Clear out the existing vulnerabilities so that the skeleton loader is shown.
+      this.vulnerabilities = [];
+    },
+  },
   methods: {
     onErrorDismiss() {
       this.errorLoadingVulnerabilities = false;
@@ -73,14 +83,13 @@ export default {
         this.$apollo.queries.vulnerabilities.fetchMore({
           variables: { after: this.pageInfo.endCursor },
           updateQuery: (previousResult, { fetchMoreResult }) => {
-            const results = produce(fetchMoreResult, (draftData) => {
+            return produce(fetchMoreResult, (draftData) => {
               // eslint-disable-next-line no-param-reassign
               draftData.group.vulnerabilities.nodes = [
                 ...previousResult.group.vulnerabilities.nodes,
                 ...draftData.group.vulnerabilities.nodes,
               ];
             });
-            return results;
           },
         });
       }

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

@@ -24,7 +24,6 @@ export default {
   data() {
     return {
       pageInfo: {},
-      isFirstResultLoading: true,
       vulnerabilities: [],
       errorLoadingVulnerabilities: false,
       sortBy: 'severity',
@@ -35,6 +34,9 @@ export default {
     isLoadingQuery() {
       return this.$apollo.queries.vulnerabilities.loading;
     },
+    isLoadingFirstResult() {
+      return this.isLoadingQuery && this.vulnerabilities.length === 0;
+    },
     sort() {
       return `${this.sortBy}_${this.sortDirection}`;
     },
@@ -51,8 +53,7 @@ export default {
         };
       },
       update: ({ vulnerabilities }) => vulnerabilities.nodes,
-      result({ data, loading }) {
-        this.isFirstResultLoading = loading;
+      result({ data }) {
         this.pageInfo = preparePageInfo(data?.vulnerabilities?.pageInfo);
       },
       error() {
@@ -60,6 +61,16 @@ export default {
       },
     },
   },
+  watch: {
+    filters() {
+      // Clear out the existing vulnerabilities so that the skeleton loader is shown.
+      this.vulnerabilities = [];
+    },
+    sort() {
+      // Clear out the existing vulnerabilities so that the skeleton loader is shown.
+      this.vulnerabilities = [];
+    },
+  },
   methods: {
     onErrorDismiss() {
       this.errorLoadingVulnerabilities = false;
@@ -69,14 +80,13 @@ export default {
         this.$apollo.queries.vulnerabilities.fetchMore({
           variables: { after: this.pageInfo.endCursor },
           updateQuery: (previousResult, { fetchMoreResult }) => {
-            const results = produce(fetchMoreResult, (draftData) => {
+            return produce(fetchMoreResult, (draftData) => {
               // eslint-disable-next-line no-param-reassign
               draftData.vulnerabilities.nodes = [
                 ...previousResult.vulnerabilities.nodes,
                 ...draftData.vulnerabilities.nodes,
               ];
             });
-            return results;
           },
         });
       }
@@ -106,7 +116,7 @@ export default {
     <vulnerability-list
       v-else
       :filters="filters"
-      :is-loading="isFirstResultLoading"
+      :is-loading="isLoadingFirstResult"
       :vulnerabilities="vulnerabilities"
       should-show-project-namespace
       @sort-changed="handleSortChange"

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

@@ -95,20 +95,29 @@ export default {
       return `${this.sortBy}_${this.sortDirection}`;
     },
   },
+  watch: {
+    filters() {
+      // Clear out the existing vulnerabilities so that the skeleton loader is shown.
+      this.vulnerabilities = [];
+    },
+    sort() {
+      // Clear out the existing vulnerabilities so that the skeleton loader is shown.
+      this.vulnerabilities = [];
+    },
+  },
   methods: {
     fetchNextPage() {
       if (this.pageInfo.hasNextPage) {
         this.$apollo.queries.vulnerabilities.fetchMore({
           variables: { after: this.pageInfo.endCursor },
           updateQuery: (previousResult, { fetchMoreResult }) => {
-            const results = produce(fetchMoreResult, (draftData) => {
+            return produce(fetchMoreResult, (draftData) => {
               // eslint-disable-next-line no-param-reassign
               draftData.project.vulnerabilities.nodes = [
                 ...previousResult.project.vulnerabilities.nodes,
                 ...draftData.project.vulnerabilities.nodes,
               ];
             });
-            return results;
           },
         });
       }

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

@@ -458,7 +458,7 @@ export default {
 
       <template #cell(activity)="{ item }">
         <div class="gl-display-flex gl-justify-content-end">
-          <auto-fix-help-text v-if="item.hasSolutions" :merge-request="item.mergeRequest" />
+          <auto-fix-help-text v-if="item.mergeRequest" :merge-request="item.mergeRequest" />
           <issues-badge
             v-if="badgeIssues(item).length > 0"
             :issues="badgeIssues(item)"

ee/changelogs/unreleased/320885-show-loading-for-vulnerability-list.yml

+---
+title: Show loading state for vulnerability list when filter/sort is changed
+merge_request: 53934
+author:
+type: changed

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

@@ -6,6 +6,9 @@ import { generateVulnerabilities } from './mock_data';
 
 describe('First Class Group Dashboard Vulnerabilities Component', () => {
   let wrapper;
+  const apolloMock = {
+    queries: { vulnerabilities: { loading: true } },
+  };
 
   const groupFullPath = 'group-full-path';
 
@@ -14,7 +17,12 @@ describe('First Class Group Dashboard Vulnerabilities Component', () => {
   const findAlert = () => wrapper.find(GlAlert);
   const findLoadingIcon = () => wrapper.find(GlLoadingIcon);
 
-  const createWrapper = ({ $apollo, stubs }) => {
+  const expectLoadingState = ({ initial = false, nextPage = false }) => {
+    expect(findVulnerabilities().props('isLoading')).toBe(initial);
+    expect(findLoadingIcon().exists()).toBe(nextPage);
+  };
+
+  const createWrapper = ({ $apollo = apolloMock, stubs } = {}) => {
     return shallowMount(FirstClassGroupVulnerabilities, {
       propsData: {
         groupFullPath,
@@ -44,12 +52,8 @@ describe('First Class Group Dashboard Vulnerabilities Component', () => {
       });
     });
 
-    it('passes down isLoading correctly', () => {
-      expect(findVulnerabilities().props()).toMatchObject({ isLoading: true });
-    });
-
-    it('does not show the loading spinner', () => {
-      expect(findLoadingIcon().exists()).toBe(false);
+    it('shows the initial loading state', () => {
+      expectLoadingState({ initial: true });
     });
   });
 
@@ -135,6 +139,10 @@ describe('First Class Group Dashboard Vulnerabilities Component', () => {
       expect(wrapper.vm.sortBy).toBe('description');
       expect(wrapper.vm.sortDirection).toBe('asc');
     });
+
+    it('does not show loading any state', () => {
+      expectLoadingState({ initial: false, nextPage: false });
+    });
   });
 
   describe('when there is more than a page of vulnerabilities', () => {
@@ -160,7 +168,7 @@ describe('First Class Group Dashboard Vulnerabilities Component', () => {
     });
   });
 
-  describe('when the query is loading and there is another page', () => {
+  describe('when the query is loading the next page', () => {
     beforeEach(() => {
       wrapper = createWrapper({
         $apollo: {
@@ -169,6 +177,7 @@ describe('First Class Group Dashboard Vulnerabilities Component', () => {
       });
 
       wrapper.setData({
+        vulnerabilities: generateVulnerabilities(),
         pageInfo: {
           hasNextPage: true,
         },
@@ -176,7 +185,28 @@ describe('First Class Group Dashboard Vulnerabilities Component', () => {
     });
 
     it('should render the loading spinner', () => {
-      expect(findLoadingIcon().exists()).toBe(true);
+      expectLoadingState({ nextPage: true });
+    });
+  });
+
+  describe('when filter or sort is changed', () => {
+    beforeEach(() => {
+      wrapper = createWrapper();
+    });
+
+    it('should show the initial loading state when the filter is changed', () => {
+      wrapper.setProps({ filter: {} });
+
+      expectLoadingState({ initial: true });
+    });
+
+    it('should show the initial loading state when the sort is changed', () => {
+      findVulnerabilities().vm.$emit('sort-changed', {
+        sortBy: 'description',
+        sortDesc: false,
+      });
+
+      expectLoadingState({ initial: true });
     });
   });
 });

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

 import { GlAlert, GlTable, GlEmptyState, GlIntersectionObserver, GlLoadingIcon } from '@gitlab/ui';
-import { shallowMount, createLocalVue } from '@vue/test-utils';
-import Vuex from 'vuex';
+import { shallowMount } from '@vue/test-utils';
 import FirstClassInstanceVulnerabilities from 'ee/security_dashboard/components/first_class_instance_security_dashboard_vulnerabilities.vue';
 import VulnerabilityList from 'ee/security_dashboard/components/vulnerability_list.vue';
 import { generateVulnerabilities } from './mock_data';
 
-const localVue = createLocalVue();
-localVue.use(Vuex);
-
 describe('First Class Instance Dashboard Vulnerabilities Component', () => {
   let wrapper;
-  let store;
 
   const findIntersectionObserver = () => wrapper.find(GlIntersectionObserver);
   const findVulnerabilities = () => wrapper.find(VulnerabilityList);
   const findAlert = () => wrapper.find(GlAlert);
   const findLoadingIcon = () => wrapper.find(GlLoadingIcon);
 
-  const createWrapper = ({ stubs, loading = false, isUpdatingProjects, data } = {}) => {
-    store = new Vuex.Store({
-      modules: {
-        projectSelector: {
-          namespaced: true,
-          actions: {
-            fetchProjects() {},
-            setProjectEndpoints() {},
-          },
-          getters: {
-            isUpdatingProjects: jest.fn().mockReturnValue(isUpdatingProjects),
-          },
-          state: {
-            projects: [],
-          },
-        },
-      },
-    });
+  const expectLoadingState = ({ initial = false, nextPage = false }) => {
+    expect(findVulnerabilities().props('isLoading')).toBe(initial);
+    expect(findLoadingIcon().exists()).toBe(nextPage);
+  };
 
+  const createWrapper = ({ stubs, loading = false, data } = {}) => {
     return shallowMount(FirstClassInstanceVulnerabilities, {
-      localVue,
-      store,
       stubs,
       mocks: {
         $apollo: {
@@ -61,17 +41,11 @@ describe('First Class Instance Dashboard Vulnerabilities Component', () => {
 
   describe('when the query is loading', () => {
     beforeEach(() => {
-      wrapper = createWrapper({
-        loading: true,
-      });
-    });
-
-    it('passes down isLoading correctly', () => {
-      expect(findVulnerabilities().props()).toMatchObject({ isLoading: true });
+      wrapper = createWrapper({ loading: true });
     });
 
-    it('does not render the loading spinner', () => {
-      expect(findLoadingIcon().exists()).toBe(false);
+    it('shows the initial loading state', () => {
+      expectLoadingState({ initial: true });
     });
   });
 
@@ -81,10 +55,7 @@ describe('First Class Instance Dashboard Vulnerabilities Component', () => {
         stubs: {
           GlAlert,
         },
-        data: () => ({
-          isFirstResultLoading: false,
-          errorLoadingVulnerabilities: true,
-        }),
+        data: () => ({ errorLoadingVulnerabilities: true }),
       });
     });
 
@@ -117,10 +88,7 @@ describe('First Class Instance Dashboard Vulnerabilities Component', () => {
           GlTable,
           GlEmptyState,
         },
-        data: () => ({
-          vulnerabilities,
-          isFirstResultLoading: false,
-        }),
+        data: () => ({ vulnerabilities }),
       });
     });
 
@@ -151,6 +119,10 @@ describe('First Class Instance Dashboard Vulnerabilities Component', () => {
       expect(wrapper.vm.sortBy).toBe('description');
       expect(wrapper.vm.sortDirection).toBe('asc');
     });
+
+    it('does not show loading any state', () => {
+      expectLoadingState({ initial: false, nextPage: false });
+    });
   });
 
   describe('when there is more than a page of vulnerabilities', () => {
@@ -177,6 +149,7 @@ describe('First Class Instance Dashboard Vulnerabilities Component', () => {
       wrapper = createWrapper({
         loading: true,
         data: () => ({
+          vulnerabilities: generateVulnerabilities(),
           pageInfo: {
             hasNextPage: true,
           },
@@ -188,8 +161,29 @@ describe('First Class Instance Dashboard Vulnerabilities Component', () => {
       expect(findIntersectionObserver().exists()).toBe(true);
     });
 
-    it('should render the loading spinner', () => {
-      expect(findLoadingIcon().exists()).toBe(true);
+    it('should render the next page loading spinner', () => {
+      expectLoadingState({ nextPage: true });
+    });
+  });
+
+  describe('when filter or sort is changed', () => {
+    beforeEach(() => {
+      wrapper = createWrapper({ loading: true });
+    });
+
+    it('should show the initial loading state when the filter is changed', () => {
+      wrapper.setProps({ filter: {} });
+
+      expectLoadingState({ initial: true });
+    });
+
+    it('should show the initial loading state when the sort is changed', () => {
+      findVulnerabilities().vm.$emit('sort-changed', {
+        sortBy: 'description',
+        sortDesc: false,
+      });
+
+      expectLoadingState({ initial: true });
     });
   });
 });

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

@@ -34,6 +34,11 @@ describe('Vulnerabilities app component', () => {
   const findVulnerabilityList = () => wrapper.find(VulnerabilityList);
   const findLoadingIcon = () => wrapper.find(GlLoadingIcon);
 
+  const expectLoadingState = ({ initial = false, nextPage = false }) => {
+    expect(findVulnerabilityList().props('isLoading')).toBe(initial);
+    expect(findLoadingIcon().exists()).toBe(nextPage);
+  };
+
   beforeEach(() => {
     createWrapper();
   });
@@ -48,12 +53,8 @@ describe('Vulnerabilities app component', () => {
       createWrapper();
     });
 
-    it('should be in the loading state', () => {
-      expect(findVulnerabilityList().props().isLoading).toBe(true);
-    });
-
-    it('should not render the loading spinner', () => {
-      expect(findLoadingIcon().exists()).toBe(false);
+    it('should show the initial loading state', () => {
+      expectLoadingState({ initial: true });
     });
   });
 
@@ -64,13 +65,11 @@ describe('Vulnerabilities app component', () => {
       createWrapper();
 
       vulnerabilities = generateVulnerabilities();
-      wrapper.setData({
-        vulnerabilities,
-      });
+      wrapper.setData({ vulnerabilities });
     });
 
-    it('should not be in the loading state', () => {
-      expect(findVulnerabilityList().props().isLoading).toBe(false);
+    it('should not show any loading state', () => {
+      expectLoadingState({ initial: false, nextPage: false });
     });
 
     it('should pass the vulnerabilities to the vulnerabilities list', () => {
@@ -94,17 +93,14 @@ describe('Vulnerabilities app component', () => {
     });
 
     it('handles sorting', () => {
-      findVulnerabilityList().vm.$listeners['sort-changed']({
+      findVulnerabilityList().vm.$emit('sort-changed', {
         sortBy: 'description',
         sortDesc: false,
       });
+
       expect(wrapper.vm.sortBy).toBe('description');
       expect(wrapper.vm.sortDirection).toBe('asc');
     });
-
-    it('should render the loading spinner', () => {
-      expect(findLoadingIcon().exists()).toBe(false);
-    });
   });
 
   describe('with more than a page of vulnerabilities', () => {
@@ -126,12 +122,12 @@ describe('Vulnerabilities app component', () => {
       expect(findIntersectionObserver().exists()).toBe(true);
     });
 
-    it('should render the loading spinner', () => {
-      expect(findLoadingIcon().exists()).toBe(true);
+    it('should render the next page loading spinner', () => {
+      expectLoadingState({ nextPage: true });
     });
   });
 
-  describe("when there's an error loading vulnerabilities", () => {
+  describe(`when there's an error loading vulnerabilities`, () => {
     beforeEach(() => {
       createWrapper();
       wrapper.setData({ errorLoadingVulnerabilities: true });
@@ -142,6 +138,27 @@ describe('Vulnerabilities app component', () => {
     });
   });
 
+  describe('when filter or sort is changed', () => {
+    beforeEach(() => {
+      createWrapper();
+    });
+
+    it('should show the initial loading state when the filter is changed', () => {
+      wrapper.setProps({ filter: {} });
+
+      expectLoadingState({ initial: true });
+    });
+
+    it('should show the initial loading state when the sort is changed', () => {
+      findVulnerabilityList().vm.$emit('sort-changed', {
+        sortBy: 'description',
+        sortDesc: false,
+      });
+
+      expectLoadingState({ initial: true });
+    });
+  });
+
   describe('security scanners', () => {
     const notEnabledScannersHelpPath = '#not-enabled';
     const noPipelineRunScannersHelpPath = '#no-pipeline';