Merge branch '11412-fix-security-dashboard-history-bug' into 'master'

Fix routing bugs in security dashboards

See merge request gitlab-org/gitlab!16738

changelogs/unreleased/11412-fix-security-dashboard-history-bug.yml

+---
+title: Fix routing bugs in security dashboards
+merge_request: 16738
+author:
+type: fixed

ee/app/assets/javascripts/pages/projects/security/dashboard/show/index.js

 import Vue from 'vue';
 import createStore from 'ee/security_dashboard/store';
+import router from 'ee/security_dashboard/store/router';
 import SecurityReportApp from 'ee/vue_shared/security_reports/card_security_reports_app.vue';
 import { parseBoolean } from '~/lib/utils/common_utils';
 
@@ -75,6 +76,7 @@ document.addEventListener('DOMContentLoaded', () => {
   return new Vue({
     el: securityTab,
     store,
+    router,
     components: {
       SecurityReportApp,
     },

ee/app/assets/javascripts/security_dashboard/store/plugins/mediator.js

 import * as filtersMutationTypes from '../modules/filters/mutation_types';
 
 export default store => {
+  const refreshVulnerabilities = payload => {
+    store.dispatch('vulnerabilities/fetchVulnerabilities', payload);
+    store.dispatch('vulnerabilities/fetchVulnerabilitiesCount', payload);
+    store.dispatch('vulnerabilities/fetchVulnerabilitiesHistory', payload);
+  };
+
   store.subscribe(({ type }) => {
     switch (type) {
+      // SET_ALL_FILTERS mutations are triggered by navigation events, in such case we
+      // want to preserve the page number that was set in the sync_with_router plugin
       case `filters/${filtersMutationTypes.SET_ALL_FILTERS}`:
+        refreshVulnerabilities({
+          ...store.getters['filters/activeFilters'],
+          page: store.state.vulnerabilities.pageInfo.page,
+        });
+        break;
+      // SET_FILTER and SET_TOGGLE_VALUE mutations happen when users interact with the UI,
+      // in that case we want to reset the page number
       case `filters/${filtersMutationTypes.SET_FILTER}`:
       case `filters/${filtersMutationTypes.SET_TOGGLE_VALUE}`: {
-        const activeFilters = store.getters['filters/activeFilters'];
-        store.dispatch('vulnerabilities/fetchVulnerabilities', activeFilters);
-        store.dispatch('vulnerabilities/fetchVulnerabilitiesCount', activeFilters);
-        store.dispatch('vulnerabilities/fetchVulnerabilitiesHistory', activeFilters);
+        refreshVulnerabilities(store.getters['filters/activeFilters']);
         break;
       }
       default:

ee/app/assets/javascripts/security_dashboard/store/plugins/sync_with_router.js

-import {
-  SET_VULNERABILITIES_HISTORY_DAY_RANGE,
-  RECEIVE_VULNERABILITIES_SUCCESS,
-} from '../modules/vulnerabilities/mutation_types';
-
 /**
  * Vuex store plugin to sync some Group Security Dashboard view settings with the URL.
  */
 export default router => store => {
   let syncingRouter = false;
-  const MUTATION_TYPES = [
-    `vulnerabilities/${SET_VULNERABILITIES_HISTORY_DAY_RANGE}`,
-    `vulnerabilities/${RECEIVE_VULNERABILITIES_SUCCESS}`,
-  ];
 
   // Update store from routing events
   router.beforeEach((to, from, next) => {
@@ -19,15 +10,13 @@ export default router => store => {
 
     if (to.name === 'dashboard' && !updatedFromState) {
       syncingRouter = true;
-      store.dispatch(`filters/setAllFilters`, to.query);
-      const page = parseInt(to.query.page, 10);
-      if (Number.isFinite(page)) {
-        store.dispatch(`vulnerabilities/setVulnerabilitiesPage`, page);
-      }
+      const page = parseInt(to.query.page, 10) || 1;
+      store.dispatch(`vulnerabilities/setVulnerabilitiesPage`, page);
       const dayRange = parseInt(to.query.days, 10);
       if (Number.isFinite(dayRange)) {
         store.dispatch(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, dayRange);
       }
+      store.dispatch(`filters/setAllFilters`, to.query);
       syncingRouter = false;
     }
 
@@ -35,16 +24,36 @@ export default router => store => {
   });
 
   // Update router from store mutations
-  store.subscribe(({ type }) => {
-    if (!syncingRouter && MUTATION_TYPES.includes(type)) {
-      const activeFilters = store.getters['filters/activeFilters'];
-      const { page } = store.state.vulnerabilities.pageInfo;
-      const days = store.state.vulnerabilities.vulnerabilitiesHistoryDayRange;
-      store.$router.push({
-        name: 'dashboard',
-        query: { ...activeFilters, page, days },
-        params: { updatedFromState: true },
-      });
+  const updateRouter = (queryParams = {}) => {
+    const activeFilters = store.getters['filters/activeFilters'];
+    const routePayload = {
+      name: 'dashboard',
+      query: {
+        ...activeFilters,
+        page: store.state.vulnerabilities.pageInfo.page,
+        days: store.state.vulnerabilities.vulnerabilitiesHistoryDayRange,
+        ...queryParams,
+      },
+      params: { updatedFromState: true },
+    };
+    const resolvedRoute = router.resolve(routePayload);
+    if (resolvedRoute.route.fullPath !== router.currentRoute.fullPath) {
+      router.push(routePayload);
+    }
+  };
+
+  store.subscribeAction(({ type, payload }) => {
+    if (syncingRouter) {
+      return;
+    }
+    switch (type) {
+      case `vulnerabilities/fetchVulnerabilities`:
+        updateRouter({ page: payload.page });
+        break;
+      case `vulnerabilities/setVulnerabilitiesHistoryDayRange`:
+        updateRouter({ days: payload });
+        break;
+      default:
     }
   });
 };

ee/spec/frontend/security_dashboard/store/plugins/sync_with_router_spec.js

 import createStore from 'ee/security_dashboard/store/index';
-import * as vulnerabilitiesMutationTypes from 'ee/security_dashboard/store/modules/vulnerabilities/mutation_types';
 import { DAYS } from 'ee/security_dashboard/store/modules/vulnerabilities/constants';
 
 describe('syncWithRouter', () => {
@@ -32,6 +31,19 @@ describe('syncWithRouter', () => {
     );
   });
 
+  it('sets page to 1 if query query string does not contain a page param after URL changes', () => {
+    const page = undefined;
+    const days = DAYS.SIXTY;
+    const query = { example: ['test'], page, days };
+
+    jest.spyOn(store, 'dispatch');
+
+    const routerPush = store.$router.push.bind(store.$router);
+    jest.spyOn(store.$router, 'push');
+    routerPush({ name: 'dashboard', query });
+    expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, 1);
+  });
+
   it("doesn't update the store if the URL update originated from the mediator", () => {
     const query = { example: ['test'] };
 
@@ -48,34 +60,41 @@ describe('syncWithRouter', () => {
 
     jest.spyOn(store.$router, 'push').mockImplementation(noop);
 
-    store.commit(
-      `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`,
-      { pageInfo: { page } },
-    );
+    store.dispatch(`vulnerabilities/fetchVulnerabilities`, { page });
 
     expect(store.$router.push).toHaveBeenCalledTimes(1);
-    expect(store.$router.push).toHaveBeenCalledWith({
-      name: 'dashboard',
-      query: expect.objectContaining({ ...activeFilters, page }),
-      params: { updatedFromState: true },
-    });
+    expect(store.$router.push).toHaveBeenCalledWith(
+      expect.objectContaining({
+        name: 'dashboard',
+        query: expect.objectContaining({
+          ...activeFilters,
+          page,
+          days: store.state.vulnerabilities.vulnerabilitiesHistoryDayRange,
+        }),
+        params: { updatedFromState: true },
+      }),
+    );
   });
 
   it('it updates the route after changing the vulnerability history day range', () => {
+    const activeFilters = store.getters['filters/activeFilters'];
     const days = DAYS.SIXTY;
 
     jest.spyOn(store.$router, 'push').mockImplementation(noop);
 
-    store.commit(
-      `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`,
-      days,
-    );
+    store.dispatch(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, days);
 
     expect(store.$router.push).toHaveBeenCalledTimes(1);
-    expect(store.$router.push).toHaveBeenCalledWith({
-      name: 'dashboard',
-      query: expect.objectContaining({ days }),
-      params: { updatedFromState: true },
-    });
+    expect(store.$router.push).toHaveBeenCalledWith(
+      expect.objectContaining({
+        name: 'dashboard',
+        query: expect.objectContaining({
+          ...activeFilters,
+          page: store.state.vulnerabilities.pageInfo.page,
+          days,
+        }),
+        params: { updatedFromState: true },
+      }),
+    );
   });
 });

ee/spec/frontend/vue_shared/security_reports/card_security_reports_app_spec.js

@@ -6,6 +6,7 @@ import { TEST_HOST } from 'helpers/test_constants';
 
 import CardSecurityDashboardApp from 'ee/vue_shared/security_reports/card_security_reports_app.vue';
 import createStore from 'ee/security_dashboard/store';
+import router from 'ee/security_dashboard/store/router';
 import { trimText } from 'helpers/text_helper';
 
 const localVue = createLocalVue();
@@ -24,6 +25,7 @@ describe('Card security reports app', () => {
     wrapper = mount(CardSecurityDashboardApp, {
       localVue,
       store: createStore(),
+      router,
       sync: false,
       stubs: ['security-dashboard-table'],
       propsData: {

ee/spec/javascripts/security_dashboard/store/plugins/mediator_spec.js

@@ -35,24 +35,24 @@ describe('mediator', () => {
   it('triggers fetching vulnerabilities after filters change', () => {
     spyOn(store, 'dispatch');
 
-    const activeFilters = store.getters['filters/activeFilters'];
+    const payload = {
+      ...store.getters['filters/activeFilters'],
+      page: store.state.vulnerabilities.pageInfo.page,
+    };
 
     store.commit(`filters/${filtersMutationTypes.SET_ALL_FILTERS}`, {});
 
     expect(store.dispatch).toHaveBeenCalledTimes(3);
-    expect(store.dispatch).toHaveBeenCalledWith(
-      'vulnerabilities/fetchVulnerabilities',
-      activeFilters,
-    );
+    expect(store.dispatch).toHaveBeenCalledWith('vulnerabilities/fetchVulnerabilities', payload);
 
     expect(store.dispatch).toHaveBeenCalledWith(
       'vulnerabilities/fetchVulnerabilitiesCount',
-      activeFilters,
+      payload,
     );
 
     expect(store.dispatch).toHaveBeenCalledWith(
       'vulnerabilities/fetchVulnerabilitiesHistory',
-      activeFilters,
+      payload,
     );
   });