From 1ed5db4326d7d8429cf76d0c0fdccef201b745a0 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Mon, 25 Mar 2019 11:22:26 +0800 Subject: [PATCH 01/11] Persist vulnerabilities page number in GSD URL This follows straight-forwardly from [previous work][1] to persist some state of the Group Security Dashboard to the URL, and vice versa. [1]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9108 --- .../security_dashboard/components/app.vue | 4 ++-- .../security_dashboard/store/moderator.js | 7 ++++++- .../store/modules/vulnerabilities/actions.js | 4 ++++ .../modules/vulnerabilities/mutation_types.js | 1 + .../modules/vulnerabilities/mutations.js | 3 +++ .../security_dashboard_table_spec.js | 6 +++++- .../store/moderator_spec.js | 11 ++++++---- .../store/vulnerabilities/actions_spec.js | 21 +++++++++++++++++++ .../store/vulnerabilities/mutations_spec.js | 11 ++++++++++ 9 files changed, 60 insertions(+), 8 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/components/app.vue b/ee/app/assets/javascripts/security_dashboard/components/app.vue index 820ea4b55616cd..6af62147a4e5a4 100644 --- a/ee/app/assets/javascripts/security_dashboard/components/app.vue +++ b/ee/app/assets/javascripts/security_dashboard/components/app.vue @@ -47,7 +47,7 @@ export default { }, }, computed: { - ...mapState('vulnerabilities', ['modal']), + ...mapState('vulnerabilities', ['modal', 'pageInfo']), ...mapState('projects', ['projects']), ...mapGetters('filters', ['activeFilters']), canCreateIssuePermission() { @@ -67,7 +67,7 @@ export default { this.setVulnerabilitiesEndpoint(this.vulnerabilitiesEndpoint); this.setVulnerabilitiesCountEndpoint(this.vulnerabilitiesCountEndpoint); this.setVulnerabilitiesHistoryEndpoint(this.vulnerabilitiesHistoryEndpoint); - this.fetchVulnerabilities(this.activeFilters); + this.fetchVulnerabilities({ ...this.activeFilters, page: this.pageInfo.page }); this.fetchVulnerabilitiesCount(this.activeFilters); this.fetchVulnerabilitiesHistory(this.activeFilters); this.fetchProjects(); diff --git a/ee/app/assets/javascripts/security_dashboard/store/moderator.js b/ee/app/assets/javascripts/security_dashboard/store/moderator.js index b84d20891cdda7..92f791a0e05755 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/moderator.js +++ b/ee/app/assets/javascripts/security_dashboard/store/moderator.js @@ -9,6 +9,10 @@ export default function configureModerator(store) { if (to.name === 'dashboard' && !updatedFromState) { store.dispatch(`filters/setAllFilters`, to.query); + const page = parseInt(to.query.page, 10); + if (Number.isFinite(page)) { + store.dispatch(`vulnerabilities/setVulnerabilitiesPage`, page); + } } next(); @@ -38,9 +42,10 @@ export default function configureModerator(store) { } case `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`: { const activeFilters = store.getters['filters/activeFilters']; + const { page } = store.state.vulnerabilities.pageInfo; store.$router.push({ name: 'dashboard', - query: activeFilters, + query: { ...activeFilters, page }, params: { updatedFromState: true }, }); break; diff --git a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/actions.js b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/actions.js index 18632ca58c704d..39a46de8d2a8f2 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/actions.js +++ b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/actions.js @@ -45,6 +45,10 @@ export const receiveVulnerabilitiesCountError = ({ commit }) => { commit(types.RECEIVE_VULNERABILITIES_COUNT_ERROR); }; +export const setVulnerabilitiesPage = ({ commit }, page) => { + commit(types.SET_VULNERABILITIES_PAGE, page); +}; + export const fetchVulnerabilities = ({ state, dispatch }, params = {}) => { if (!state.vulnerabilitiesEndpoint) { return; diff --git a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutation_types.js b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutation_types.js index ab114581aec0ef..34d8a8bac9feb4 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutation_types.js +++ b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutation_types.js @@ -1,4 +1,5 @@ export const SET_VULNERABILITIES_ENDPOINT = 'SET_VULNERABILITIES_ENDPOINT'; +export const SET_VULNERABILITIES_PAGE = 'SET_VULNERABILITIES_PAGE'; export const REQUEST_VULNERABILITIES = 'REQUEST_VULNERABILITIES'; export const RECEIVE_VULNERABILITIES_SUCCESS = 'RECEIVE_VULNERABILITIES_SUCCESS'; export const RECEIVE_VULNERABILITIES_ERROR = 'RECEIVE_VULNERABILITIES_ERROR'; diff --git a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js index 1ba221a970ff60..7007c493eed240 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js +++ b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js @@ -24,6 +24,9 @@ export default { [types.SET_VULNERABILITIES_COUNT_ENDPOINT](state, payload) { state.vulnerabilitiesCountEndpoint = payload; }, + [types.SET_VULNERABILITIES_PAGE](state, payload) { + state.pageInfo = { ...state.pageInfo, page: payload }; + }, [types.REQUEST_VULNERABILITIES_COUNT](state) { state.isLoadingVulnerabilitiesCount = true; state.errorLoadingVulnerabilitiesCount = false; diff --git a/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_spec.js b/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_spec.js index 118976b1192251..ac81576054c07b 100644 --- a/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_spec.js +++ b/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_spec.js @@ -49,6 +49,7 @@ describe('Security Dashboard Table', () => { beforeEach(() => { store.commit(`vulnerabilities/${RECEIVE_VULNERABILITIES_SUCCESS}`, { vulnerabilities: mockDataVulnerabilities, + pageInfo: {}, }); vm = mountComponentWithStore(Component, { store, props }); }); @@ -62,7 +63,10 @@ describe('Security Dashboard Table', () => { describe('with no vulnerabilties', () => { beforeEach(() => { - store.commit(`vulnerabilities/${RECEIVE_VULNERABILITIES_SUCCESS}`, { vulnerabilities: [] }); + store.commit(`vulnerabilities/${RECEIVE_VULNERABILITIES_SUCCESS}`, { + vulnerabilities: [], + pageInfo: {}, + }); vm = mountComponentWithStore(Component, { store, props }); }); diff --git a/ee/spec/javascripts/security_dashboard/store/moderator_spec.js b/ee/spec/javascripts/security_dashboard/store/moderator_spec.js index d47a551175dd56..875592af7cd4a0 100644 --- a/ee/spec/javascripts/security_dashboard/store/moderator_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/moderator_spec.js @@ -78,14 +78,16 @@ describe('moderator', () => { describe('routing', () => { it('updates store after URL changes', () => { - const query = { example: ['test'] }; + const page = 3; + const query = { example: ['test'], page }; spyOn(store, 'dispatch'); store.$router.push({ name: 'dashboard', query }); - expect(store.dispatch).toHaveBeenCalledTimes(1); + expect(store.dispatch).toHaveBeenCalledTimes(2); expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); + expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); }); it("doesn't update the store if the URL update originated from the moderator", () => { @@ -100,18 +102,19 @@ describe('moderator', () => { it('it updates the route after a successful vulnerability retrieval', () => { const activeFilters = store.getters['filters/activeFilters']; + const page = 2; spyOn(store.$router, 'push'); store.commit( `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`, - {}, + { pageInfo: { page } }, ); expect(store.$router.push).toHaveBeenCalledTimes(1); expect(store.$router.push).toHaveBeenCalledWith({ name: 'dashboard', - query: activeFilters, + query: { ...activeFilters, page }, params: { updatedFromState: true }, }); }); diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/actions_spec.js b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/actions_spec.js index e34fb100fde96c..ccf850de75b8d5 100644 --- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/actions_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/actions_spec.js @@ -325,6 +325,27 @@ describe('vulnerabilities actions', () => { ); }); }); + + describe('setVulnerabilitiesPage', () => { + it('should commit the correct mutuation', done => { + const state = initialState; + const page = 3; + + testAction( + actions.setVulnerabilitiesPage, + page, + state, + [ + { + type: types.SET_VULNERABILITIES_PAGE, + payload: page, + }, + ], + [], + done, + ); + }); + }); }); describe('openModal', () => { diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js index e1561b35e065a8..a2b0a4d72be0b4 100644 --- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js @@ -16,6 +16,17 @@ describe('vulnerabilities module mutations', () => { }); }); + describe('SET_VULNERABILITIES_PAGE', () => { + const page = 3; + it(`should set pageInfo.page to ${page}`, () => { + const state = createState(); + + mutations[types.SET_VULNERABILITIES_PAGE](state, page); + + expect(state.pageInfo.page).toEqual(page); + }); + }); + describe('REQUEST_VULNERABILITIES', () => { let state; -- GitLab From d8ba93b41d4e05b43fda166d7b2a1111aad7fa59 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Mon, 25 Mar 2019 18:42:12 +0800 Subject: [PATCH 02/11] Persist vulnerabilities day range in GSD URL This required an new `syncingRouter` flag to prevent update cycles: a route navigation triggers a mutation in the vulnerabilities history day range state variable, while the store subscription listens for that mutation, and updates the router accordingly. The latter should only happen if the mutation occurred outside of a router navigation. This extra flag is the counterpart to the already-existing `updateFromState` flag, which prevents the other side of the loop from occurring. --- .../security_dashboard/store/moderator.js | 16 +++++++++- .../store/moderator_spec.js | 30 +++++++++++++++++-- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/store/moderator.js b/ee/app/assets/javascripts/security_dashboard/store/moderator.js index 92f791a0e05755..cc6a44f55625f4 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/moderator.js +++ b/ee/app/assets/javascripts/security_dashboard/store/moderator.js @@ -4,15 +4,23 @@ import * as projectsMutationTypes from './modules/projects/mutation_types'; import { BASE_FILTERS } from './modules/filters/constants'; export default function configureModerator(store) { + let syncingRouter = false; + store.$router.beforeEach((to, from, next) => { const updatedFromState = (to.params && to.params.updatedFromState) || false; 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 dayRange = parseInt(to.query.days, 10); + if (Number.isFinite(dayRange)) { + store.dispatch(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, dayRange); + } + syncingRouter = false; } next(); @@ -40,12 +48,18 @@ export default function configureModerator(store) { store.dispatch('vulnerabilities/fetchVulnerabilitiesHistory', activeFilters); break; } + case `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`: case `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`: { + if (syncingRouter) { + break; + } + 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 }, + query: { ...activeFilters, page, days }, params: { updatedFromState: true }, }); break; diff --git a/ee/spec/javascripts/security_dashboard/store/moderator_spec.js b/ee/spec/javascripts/security_dashboard/store/moderator_spec.js index 875592af7cd4a0..778893c44e1872 100644 --- a/ee/spec/javascripts/security_dashboard/store/moderator_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/moderator_spec.js @@ -3,6 +3,7 @@ import * as projectsMutationTypes from 'ee/security_dashboard/store/modules/proj import * as filtersMutationTypes from 'ee/security_dashboard/store/modules/filters/mutation_types'; import * as vulnerabilitiesMutationTypes from 'ee/security_dashboard/store/modules/vulnerabilities/mutation_types'; import { BASE_FILTERS } from 'ee/security_dashboard/store/modules/filters/constants'; +import { DAYS } from 'ee/security_dashboard/store/modules/vulnerabilities/constants'; describe('moderator', () => { let store; @@ -79,15 +80,20 @@ describe('moderator', () => { describe('routing', () => { it('updates store after URL changes', () => { const page = 3; - const query = { example: ['test'], page }; + const days = DAYS.SIXTY; + const query = { example: ['test'], page, days }; spyOn(store, 'dispatch'); + spyOn(store.$router, 'push').and.callThrough(); store.$router.push({ name: 'dashboard', query }); - expect(store.dispatch).toHaveBeenCalledTimes(2); + expect(store.dispatch).toHaveBeenCalledTimes(3); expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); + expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, days); + + expect(store.$router.push).toHaveBeenCalledTimes(1); }); it("doesn't update the store if the URL update originated from the moderator", () => { @@ -114,7 +120,25 @@ describe('moderator', () => { expect(store.$router.push).toHaveBeenCalledTimes(1); expect(store.$router.push).toHaveBeenCalledWith({ name: 'dashboard', - query: { ...activeFilters, page }, + query: jasmine.objectContaining({ ...activeFilters, page }), + params: { updatedFromState: true }, + }); + }); + + it('it updates the route after changing the vulnerability history day range', () => { + const days = DAYS.SIXTY; + + spyOn(store.$router, 'push'); + + store.commit( + `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`, + days, + ); + + expect(store.$router.push).toHaveBeenCalledTimes(1); + expect(store.$router.push).toHaveBeenCalledWith({ + name: 'dashboard', + query: jasmine.objectContaining({ days }), params: { updatedFromState: true }, }); }); -- GitLab From 5728138d9480153b53e5e69e44f1d7d4f5bded18 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Tue, 26 Mar 2019 17:27:10 +0800 Subject: [PATCH 03/11] Extract router synchronisation Vuex plugin This code has exactly one responsibility: keep certain parts of the store state in sync with the URL, and vice versa. Therefore, it's now in a separate file completely. The moderator need not know about it at all. --- .../security_dashboard/store/index.js | 2 + .../security_dashboard/store/moderator.js | 39 ---------- .../store/sync_with_router.js | 52 +++++++++++++ .../store/moderator_spec.js | 69 ---------------- .../store/sync_with_router_spec.js | 78 +++++++++++++++++++ 5 files changed, 132 insertions(+), 108 deletions(-) create mode 100644 ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js create mode 100644 ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js diff --git a/ee/app/assets/javascripts/security_dashboard/store/index.js b/ee/app/assets/javascripts/security_dashboard/store/index.js index 7ef70c5269c69c..71e8e4b475940b 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/index.js +++ b/ee/app/assets/javascripts/security_dashboard/store/index.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import Vuex from 'vuex'; import router from './router'; import configureModerator from './moderator'; +import syncWithRouter from './sync_with_router'; import filters from './modules/filters/index'; import projects from './modules/projects/index'; import vulnerabilities from './modules/vulnerabilities/index'; @@ -15,6 +16,7 @@ export default () => { projects, vulnerabilities, }, + plugins: [syncWithRouter(router)], }); store.$router = router; diff --git a/ee/app/assets/javascripts/security_dashboard/store/moderator.js b/ee/app/assets/javascripts/security_dashboard/store/moderator.js index cc6a44f55625f4..218b27102d1c03 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/moderator.js +++ b/ee/app/assets/javascripts/security_dashboard/store/moderator.js @@ -1,31 +1,8 @@ -import * as vulnerabilitiesMutationTypes from './modules/vulnerabilities/mutation_types'; import * as filtersMutationTypes from './modules/filters/mutation_types'; import * as projectsMutationTypes from './modules/projects/mutation_types'; import { BASE_FILTERS } from './modules/filters/constants'; export default function configureModerator(store) { - let syncingRouter = false; - - store.$router.beforeEach((to, from, next) => { - const updatedFromState = (to.params && to.params.updatedFromState) || false; - - 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 dayRange = parseInt(to.query.days, 10); - if (Number.isFinite(dayRange)) { - store.dispatch(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, dayRange); - } - syncingRouter = false; - } - - next(); - }); - store.subscribe(({ type, payload }) => { switch (type) { case `projects/${projectsMutationTypes.RECEIVE_PROJECTS_SUCCESS}`: @@ -48,22 +25,6 @@ export default function configureModerator(store) { store.dispatch('vulnerabilities/fetchVulnerabilitiesHistory', activeFilters); break; } - case `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`: - case `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`: { - if (syncingRouter) { - break; - } - - 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 }, - }); - break; - } default: } }); diff --git a/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js new file mode 100644 index 00000000000000..c63917865c32a0 --- /dev/null +++ b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js @@ -0,0 +1,52 @@ +import * as vulnerabilitiesMutationTypes 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; + + // Update store from routing events + router.beforeEach((to, from, next) => { + const updatedFromState = (to.params && to.params.updatedFromState) || false; + + 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 dayRange = parseInt(to.query.days, 10); + if (Number.isFinite(dayRange)) { + store.dispatch(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, dayRange); + } + syncingRouter = false; + } + + next(); + }); + + // Update router from store mutations + store.subscribe(({ type, payload }) => { + switch (type) { + case `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`: + case `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`: { + if (syncingRouter) { + break; + } + + 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 }, + }); + break; + } + default: + } + }); +} diff --git a/ee/spec/javascripts/security_dashboard/store/moderator_spec.js b/ee/spec/javascripts/security_dashboard/store/moderator_spec.js index 778893c44e1872..19a21855b774c4 100644 --- a/ee/spec/javascripts/security_dashboard/store/moderator_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/moderator_spec.js @@ -1,9 +1,7 @@ import createStore from 'ee/security_dashboard/store/index'; import * as projectsMutationTypes from 'ee/security_dashboard/store/modules/projects/mutation_types'; import * as filtersMutationTypes from 'ee/security_dashboard/store/modules/filters/mutation_types'; -import * as vulnerabilitiesMutationTypes from 'ee/security_dashboard/store/modules/vulnerabilities/mutation_types'; import { BASE_FILTERS } from 'ee/security_dashboard/store/modules/filters/constants'; -import { DAYS } from 'ee/security_dashboard/store/modules/vulnerabilities/constants'; describe('moderator', () => { let store; @@ -76,71 +74,4 @@ describe('moderator', () => { activeFilters, ); }); - - describe('routing', () => { - it('updates store after URL changes', () => { - const page = 3; - const days = DAYS.SIXTY; - const query = { example: ['test'], page, days }; - - spyOn(store, 'dispatch'); - spyOn(store.$router, 'push').and.callThrough(); - - store.$router.push({ name: 'dashboard', query }); - - expect(store.dispatch).toHaveBeenCalledTimes(3); - expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); - expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); - expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, days); - - expect(store.$router.push).toHaveBeenCalledTimes(1); - }); - - it("doesn't update the store if the URL update originated from the moderator", () => { - const query = { example: ['test'] }; - - spyOn(store, 'commit'); - - store.$router.push({ name: 'dashboard', query, params: { updatedFromState: true } }); - - expect(store.commit).toHaveBeenCalledTimes(0); - }); - - it('it updates the route after a successful vulnerability retrieval', () => { - const activeFilters = store.getters['filters/activeFilters']; - const page = 2; - - spyOn(store.$router, 'push'); - - store.commit( - `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`, - { pageInfo: { page } }, - ); - - expect(store.$router.push).toHaveBeenCalledTimes(1); - expect(store.$router.push).toHaveBeenCalledWith({ - name: 'dashboard', - query: jasmine.objectContaining({ ...activeFilters, page }), - params: { updatedFromState: true }, - }); - }); - - it('it updates the route after changing the vulnerability history day range', () => { - const days = DAYS.SIXTY; - - spyOn(store.$router, 'push'); - - store.commit( - `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`, - days, - ); - - expect(store.$router.push).toHaveBeenCalledTimes(1); - expect(store.$router.push).toHaveBeenCalledWith({ - name: 'dashboard', - query: jasmine.objectContaining({ days }), - params: { updatedFromState: true }, - }); - }); - }); }); diff --git a/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js b/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js new file mode 100644 index 00000000000000..b458e81069cad3 --- /dev/null +++ b/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js @@ -0,0 +1,78 @@ +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', () => { + let store; + + beforeEach(() => { + store = createStore(); + }); + + describe('routing', () => { + it('updates store after URL changes', () => { + const page = 3; + const days = DAYS.SIXTY; + const query = { example: ['test'], page, days }; + + spyOn(store, 'dispatch'); + spyOn(store.$router, 'push').and.callThrough(); + + store.$router.push({ name: 'dashboard', query }); + + expect(store.dispatch).toHaveBeenCalledTimes(3); + expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); + expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); + expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, days); + + expect(store.$router.push).toHaveBeenCalledTimes(1); + }); + + it("doesn't update the store if the URL update originated from the moderator", () => { + const query = { example: ['test'] }; + + spyOn(store, 'commit'); + + store.$router.push({ name: 'dashboard', query, params: { updatedFromState: true } }); + + expect(store.commit).toHaveBeenCalledTimes(0); + }); + + it('it updates the route after a successful vulnerability retrieval', () => { + const activeFilters = store.getters['filters/activeFilters']; + const page = 2; + + spyOn(store.$router, 'push'); + + store.commit( + `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`, + { pageInfo: { page } }, + ); + + expect(store.$router.push).toHaveBeenCalledTimes(1); + expect(store.$router.push).toHaveBeenCalledWith({ + name: 'dashboard', + query: jasmine.objectContaining({ ...activeFilters, page }), + params: { updatedFromState: true }, + }); + }); + + it('it updates the route after changing the vulnerability history day range', () => { + const days = DAYS.SIXTY; + + spyOn(store.$router, 'push'); + + store.commit( + `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`, + days, + ); + + expect(store.$router.push).toHaveBeenCalledTimes(1); + expect(store.$router.push).toHaveBeenCalledWith({ + name: 'dashboard', + query: jasmine.objectContaining({ days }), + params: { updatedFromState: true }, + }); + }); + }); +}); -- GitLab From 019deccc2589cc7901449b4ff8c8240911885463 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Tue, 26 Mar 2019 17:46:45 +0800 Subject: [PATCH 04/11] Execute moderator as a Vuex plugin A Vuex plugin is simply a function which receives the store as its only argument, and is called at construction time, which decribes the moderator exactly. --- ee/app/assets/javascripts/security_dashboard/store/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/store/index.js b/ee/app/assets/javascripts/security_dashboard/store/index.js index 71e8e4b475940b..2d9e64e3a2e944 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/index.js +++ b/ee/app/assets/javascripts/security_dashboard/store/index.js @@ -16,12 +16,10 @@ export default () => { projects, vulnerabilities, }, - plugins: [syncWithRouter(router)], + plugins: [configureModerator, syncWithRouter(router)], }); store.$router = router; - configureModerator(store); - return store; }; -- GitLab From 2416921b56bec9c7b3acc5c7607128e834477b22 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Tue, 26 Mar 2019 18:45:53 +0800 Subject: [PATCH 05/11] Remove unused variable --- .../javascripts/security_dashboard/store/sync_with_router.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js index c63917865c32a0..58ac7c934cde14 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js +++ b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js @@ -28,7 +28,7 @@ export default router => store => { }); // Update router from store mutations - store.subscribe(({ type, payload }) => { + store.subscribe(({ type }) => { switch (type) { case `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`: case `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`: { -- GitLab From 7bbe1877da901e818732448ca999432205db59b2 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Tue, 26 Mar 2019 19:09:08 +0800 Subject: [PATCH 06/11] Prettify all touched files --- .../javascripts/security_dashboard/store/sync_with_router.js | 2 +- .../security_dashboard/store/sync_with_router_spec.js | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js index 58ac7c934cde14..bb44b4dce14ab0 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js +++ b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js @@ -49,4 +49,4 @@ export default router => store => { default: } }); -} +}; diff --git a/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js b/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js index b458e81069cad3..44e37a35a43d76 100644 --- a/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js @@ -23,7 +23,10 @@ describe('syncWithRouter', () => { expect(store.dispatch).toHaveBeenCalledTimes(3); expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); - expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesHistoryDayRange`, days); + expect(store.dispatch).toHaveBeenCalledWith( + `vulnerabilities/setVulnerabilitiesHistoryDayRange`, + days, + ); expect(store.$router.push).toHaveBeenCalledTimes(1); }); -- GitLab From 3f4cad2b7b8237932fea8885ed1caa91bd9b64e3 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Wed, 27 Mar 2019 11:36:39 +0800 Subject: [PATCH 07/11] Tidy up flow control - Use if-statement instead of switch-case. - Initialise relevant mutation types rather than constructing them every time. - Use direct named imports instead of verbose module namespace --- .../store/sync_with_router.js | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js index bb44b4dce14ab0..db943fed712107 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js +++ b/ee/app/assets/javascripts/security_dashboard/store/sync_with_router.js @@ -1,10 +1,17 @@ -import * as vulnerabilitiesMutationTypes from './modules/vulnerabilities/mutation_types'; +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) => { @@ -29,24 +36,15 @@ export default router => store => { // Update router from store mutations store.subscribe(({ type }) => { - switch (type) { - case `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`: - case `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`: { - if (syncingRouter) { - break; - } - - 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 }, - }); - break; - } - default: + 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 }, + }); } }); }; -- GitLab From 4c559ff5dd9e498bcf67385550c0587abc9cc5a2 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Wed, 27 Mar 2019 12:29:01 +0800 Subject: [PATCH 08/11] Remove redundant describe block This extra `describe` block was copied over from the `moderator_spec.js` file, where it provided sensible grouping in that context. Here, everything is about routing, so it's redundant. Ignore whitespace when viewing this commit's diff. --- .../store/sync_with_router_spec.js | 106 +++++++++--------- 1 file changed, 52 insertions(+), 54 deletions(-) diff --git a/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js b/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js index 44e37a35a43d76..1d81514da59712 100644 --- a/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js @@ -9,73 +9,71 @@ describe('syncWithRouter', () => { store = createStore(); }); - describe('routing', () => { - it('updates store after URL changes', () => { - const page = 3; - const days = DAYS.SIXTY; - const query = { example: ['test'], page, days }; - - spyOn(store, 'dispatch'); - spyOn(store.$router, 'push').and.callThrough(); - - store.$router.push({ name: 'dashboard', query }); - - expect(store.dispatch).toHaveBeenCalledTimes(3); - expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); - expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); - expect(store.dispatch).toHaveBeenCalledWith( - `vulnerabilities/setVulnerabilitiesHistoryDayRange`, - days, - ); - - expect(store.$router.push).toHaveBeenCalledTimes(1); - }); + it('updates store after URL changes', () => { + const page = 3; + const days = DAYS.SIXTY; + const query = { example: ['test'], page, days }; - it("doesn't update the store if the URL update originated from the moderator", () => { - const query = { example: ['test'] }; + spyOn(store, 'dispatch'); + spyOn(store.$router, 'push').and.callThrough(); - spyOn(store, 'commit'); + store.$router.push({ name: 'dashboard', query }); - store.$router.push({ name: 'dashboard', query, params: { updatedFromState: true } }); + expect(store.dispatch).toHaveBeenCalledTimes(3); + expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); + expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); + expect(store.dispatch).toHaveBeenCalledWith( + `vulnerabilities/setVulnerabilitiesHistoryDayRange`, + days, + ); - expect(store.commit).toHaveBeenCalledTimes(0); - }); + expect(store.$router.push).toHaveBeenCalledTimes(1); + }); - it('it updates the route after a successful vulnerability retrieval', () => { - const activeFilters = store.getters['filters/activeFilters']; - const page = 2; + it("doesn't update the store if the URL update originated from the moderator", () => { + const query = { example: ['test'] }; - spyOn(store.$router, 'push'); + spyOn(store, 'commit'); - store.commit( - `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`, - { pageInfo: { page } }, - ); + store.$router.push({ name: 'dashboard', query, params: { updatedFromState: true } }); + + expect(store.commit).toHaveBeenCalledTimes(0); + }); - expect(store.$router.push).toHaveBeenCalledTimes(1); - expect(store.$router.push).toHaveBeenCalledWith({ - name: 'dashboard', - query: jasmine.objectContaining({ ...activeFilters, page }), - params: { updatedFromState: true }, - }); + it('it updates the route after a successful vulnerability retrieval', () => { + const activeFilters = store.getters['filters/activeFilters']; + const page = 2; + + spyOn(store.$router, 'push'); + + store.commit( + `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`, + { pageInfo: { page } }, + ); + + expect(store.$router.push).toHaveBeenCalledTimes(1); + expect(store.$router.push).toHaveBeenCalledWith({ + name: 'dashboard', + query: jasmine.objectContaining({ ...activeFilters, page }), + params: { updatedFromState: true }, }); + }); - it('it updates the route after changing the vulnerability history day range', () => { - const days = DAYS.SIXTY; + it('it updates the route after changing the vulnerability history day range', () => { + const days = DAYS.SIXTY; - spyOn(store.$router, 'push'); + spyOn(store.$router, 'push'); - store.commit( - `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`, - days, - ); + store.commit( + `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`, + days, + ); - expect(store.$router.push).toHaveBeenCalledTimes(1); - expect(store.$router.push).toHaveBeenCalledWith({ - name: 'dashboard', - query: jasmine.objectContaining({ days }), - params: { updatedFromState: true }, - }); + expect(store.$router.push).toHaveBeenCalledTimes(1); + expect(store.$router.push).toHaveBeenCalledWith({ + name: 'dashboard', + query: jasmine.objectContaining({ days }), + params: { updatedFromState: true }, }); }); }); -- GitLab From c3ef0d3516e2c895e323011f3faa25c59885e7bb Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Wed, 27 Mar 2019 12:36:27 +0800 Subject: [PATCH 09/11] Add changelog entry --- .../9677-persist-page-and-date-in-security-dashboard-ee.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ee/changelogs/unreleased/9677-persist-page-and-date-in-security-dashboard-ee.yml diff --git a/ee/changelogs/unreleased/9677-persist-page-and-date-in-security-dashboard-ee.yml b/ee/changelogs/unreleased/9677-persist-page-and-date-in-security-dashboard-ee.yml new file mode 100644 index 00000000000000..db56ea813d28f2 --- /dev/null +++ b/ee/changelogs/unreleased/9677-persist-page-and-date-in-security-dashboard-ee.yml @@ -0,0 +1,6 @@ +--- +title: Persist in the URL the page and day range of vulnerabilities viewed in the + Group Security Dashboard. +merge_request: 10402 +author: +type: added -- GitLab From fa2947ac41521f2d11eceeca39a64a0e88acaebe Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Fri, 29 Mar 2019 17:03:37 +0800 Subject: [PATCH 10/11] Migrate test file to jest --- .../store/sync_with_router_spec.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) rename ee/spec/{javascripts => frontend}/security_dashboard/store/sync_with_router_spec.js (84%) diff --git a/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js b/ee/spec/frontend/security_dashboard/store/sync_with_router_spec.js similarity index 84% rename from ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js rename to ee/spec/frontend/security_dashboard/store/sync_with_router_spec.js index 1d81514da59712..61434686784948 100644 --- a/ee/spec/javascripts/security_dashboard/store/sync_with_router_spec.js +++ b/ee/spec/frontend/security_dashboard/store/sync_with_router_spec.js @@ -4,6 +4,7 @@ import { DAYS } from 'ee/security_dashboard/store/modules/vulnerabilities/consta describe('syncWithRouter', () => { let store; + const noop = () => {}; beforeEach(() => { store = createStore(); @@ -14,8 +15,8 @@ describe('syncWithRouter', () => { const days = DAYS.SIXTY; const query = { example: ['test'], page, days }; - spyOn(store, 'dispatch'); - spyOn(store.$router, 'push').and.callThrough(); + jest.spyOn(store, 'dispatch').mockImplementation(noop); + jest.spyOn(store.$router, 'push'); store.$router.push({ name: 'dashboard', query }); @@ -33,7 +34,7 @@ describe('syncWithRouter', () => { it("doesn't update the store if the URL update originated from the moderator", () => { const query = { example: ['test'] }; - spyOn(store, 'commit'); + jest.spyOn(store, 'commit').mockImplementation(noop); store.$router.push({ name: 'dashboard', query, params: { updatedFromState: true } }); @@ -44,7 +45,7 @@ describe('syncWithRouter', () => { const activeFilters = store.getters['filters/activeFilters']; const page = 2; - spyOn(store.$router, 'push'); + jest.spyOn(store.$router, 'push').mockImplementation(noop); store.commit( `vulnerabilities/${vulnerabilitiesMutationTypes.RECEIVE_VULNERABILITIES_SUCCESS}`, @@ -54,7 +55,7 @@ describe('syncWithRouter', () => { expect(store.$router.push).toHaveBeenCalledTimes(1); expect(store.$router.push).toHaveBeenCalledWith({ name: 'dashboard', - query: jasmine.objectContaining({ ...activeFilters, page }), + query: expect.objectContaining({ ...activeFilters, page }), params: { updatedFromState: true }, }); }); @@ -62,7 +63,7 @@ describe('syncWithRouter', () => { it('it updates the route after changing the vulnerability history day range', () => { const days = DAYS.SIXTY; - spyOn(store.$router, 'push'); + jest.spyOn(store.$router, 'push').mockImplementation(noop); store.commit( `vulnerabilities/${vulnerabilitiesMutationTypes.SET_VULNERABILITIES_HISTORY_DAY_RANGE}`, @@ -72,7 +73,7 @@ describe('syncWithRouter', () => { expect(store.$router.push).toHaveBeenCalledTimes(1); expect(store.$router.push).toHaveBeenCalledWith({ name: 'dashboard', - query: jasmine.objectContaining({ days }), + query: expect.objectContaining({ days }), params: { updatedFromState: true }, }); }); -- GitLab From b2475e84398289037da3a57bcac32985ddf826d9 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Fri, 29 Mar 2019 19:51:06 +0800 Subject: [PATCH 11/11] Clarify reason for spy expectation Also let the real `store.dispatch` implementation be called, and adjust expectations accordingly. Without this, the expectation that `$router.push` not be called implicitly again was passing for the wrong reason. When run under karma, for some reason, it *appeared* that it was passing for the right reason. So, hurrah for Jest! --- .../security_dashboard/store/sync_with_router_spec.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ee/spec/frontend/security_dashboard/store/sync_with_router_spec.js b/ee/spec/frontend/security_dashboard/store/sync_with_router_spec.js index 61434686784948..ed87c00ee49551 100644 --- a/ee/spec/frontend/security_dashboard/store/sync_with_router_spec.js +++ b/ee/spec/frontend/security_dashboard/store/sync_with_router_spec.js @@ -15,20 +15,21 @@ describe('syncWithRouter', () => { const days = DAYS.SIXTY; const query = { example: ['test'], page, days }; - jest.spyOn(store, 'dispatch').mockImplementation(noop); + jest.spyOn(store, 'dispatch'); + + const routerPush = store.$router.push.bind(store.$router); jest.spyOn(store.$router, 'push'); + routerPush({ name: 'dashboard', query }); - store.$router.push({ name: 'dashboard', query }); + // Assert no implicit synchronous recursive calls occurred + expect(store.$router.push).not.toHaveBeenCalled(); - expect(store.dispatch).toHaveBeenCalledTimes(3); expect(store.dispatch).toHaveBeenCalledWith(`filters/setAllFilters`, query); expect(store.dispatch).toHaveBeenCalledWith(`vulnerabilities/setVulnerabilitiesPage`, page); expect(store.dispatch).toHaveBeenCalledWith( `vulnerabilities/setVulnerabilitiesHistoryDayRange`, days, ); - - expect(store.$router.push).toHaveBeenCalledTimes(1); }); it("doesn't update the store if the URL update originated from the moderator", () => { -- GitLab