diff --git a/CHANGELOG.md b/CHANGELOG.md index eccb8b7d04..219cc83d23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,5 @@ +* Fix an issue adding additional targets when querying a host + * Show loading spinner while newly added Host Details are saved * Show a generic computer icon when when referring to hosts with an unknown platform instead of the text "All" diff --git a/frontend/pages/queries/QueryPage/QueryPage.jsx b/frontend/pages/queries/QueryPage/QueryPage.jsx index 39904e9b4a..5721beacda 100644 --- a/frontend/pages/queries/QueryPage/QueryPage.jsx +++ b/frontend/pages/queries/QueryPage/QueryPage.jsx @@ -13,7 +13,9 @@ import debounce from 'utilities/debounce'; import deepDifference from 'utilities/deep_difference'; import entityGetter from 'redux/utilities/entityGetter'; import { formatSelectedTargetsForApi } from 'kolide/helpers'; +import helpers from 'pages/queries/QueryPage/helpers'; import hostActions from 'redux/nodes/entities/hosts/actions'; +import hostInterface from 'interfaces/host'; import QueryForm from 'components/forms/queries/QueryForm'; import osqueryTableInterface from 'interfaces/osquery_table'; import queryActions from 'redux/nodes/entities/queries/actions'; @@ -52,12 +54,14 @@ export class QueryPage extends Component { pathname: PropTypes.string, }), query: queryInterface, + selectedHosts: PropTypes.arrayOf(hostInterface), selectedOsqueryTable: osqueryTableInterface, selectedTargets: PropTypes.arrayOf(targetInterface), }; static defaultProps = { loadingQueries: false, + selectedHosts: [], }; constructor (props) { @@ -78,23 +82,36 @@ export class QueryPage extends Component { } componentWillMount () { - const { dispatch, hostIDs } = this.props; + const { dispatch, hostIDs, selectedHosts, selectedTargets } = this.props; if (hostIDs) { dispatch(hostActions.loadAll()); } + helpers.selectHosts(dispatch, { + hosts: selectedHosts, + selectedTargets, + }); + return false; } componentWillReceiveProps (nextProps) { - const nextPathname = nextProps.location.pathname; + const { dispatch, location, selectedHosts, selectedTargets } = nextProps; + const nextPathname = location.pathname; const { pathname } = this.props.location; if (nextPathname !== pathname) { this.resetCampaignAndTargets(); } + if (!isEqual(selectedHosts, this.props.selectedHosts)) { + helpers.selectHosts(dispatch, { + hosts: selectedHosts, + selectedTargets, + }); + } + return false; } @@ -539,22 +556,19 @@ const mapStateToProps = (state, ownProps) => { const { errors, loading: loadingQueries } = state.entities.queries; const queryStub = { description: '', name: '', query: queryText }; const query = reduxQuery || queryStub; - let { selectedTargets } = state.components.QueryPages; + const { selectedTargets } = state.components.QueryPages; const { host_ids: hostIDs } = ownProps.location.query; const { isSmallNav } = state.app; + let selectedHosts = []; // hostIDs are URL params so they are strings if (hostIDs && !queryID) { + const hostFilter = isArray(hostIDs) + ? h => includes(hostIDs, String(h.id)) + : { id: Number(hostIDs) }; + const { entities: hosts } = stateEntities.get('hosts'); - let hostFilter; - - if (isArray(hostIDs)) { - hostFilter = h => includes(hostIDs, String(h.id)); - } else { - hostFilter = { id: Number(hostIDs) }; - } - - selectedTargets = filter(hosts, hostFilter); + selectedHosts = filter(hosts, hostFilter); } return { @@ -564,6 +578,7 @@ const mapStateToProps = (state, ownProps) => { loadingQueries, query, selectedOsqueryTable, + selectedHosts, selectedTargets, }; }; diff --git a/frontend/pages/queries/QueryPage/QueryPage.tests.jsx b/frontend/pages/queries/QueryPage/QueryPage.tests.jsx index 9301bfca76..c15499bad3 100644 --- a/frontend/pages/queries/QueryPage/QueryPage.tests.jsx +++ b/frontend/pages/queries/QueryPage/QueryPage.tests.jsx @@ -104,14 +104,35 @@ describe('QueryPage - component', () => { }); }); - it('sets selectedTargets based on host_ids', () => { + it('sets selectedTargets in redux based on host_ids', () => { + const singleHostMockStore = reduxMockStore(store); + const multipleHostMockStore = reduxMockStore(store); const singleHostProps = { params: {}, location: { query: { host_ids: String(hostStub.id) } } }; const multipleHostsProps = { params: {}, location: { query: { host_ids: [String(hostStub.id), '99'] } } }; - const singleHostPage = mount(connectedComponent(ConnectedQueryPage, { mockStore, props: singleHostProps })); - const multipleHostsPage = mount(connectedComponent(ConnectedQueryPage, { mockStore, props: multipleHostsProps })); - expect(singleHostPage.find('QueryPage').prop('selectedTargets')).toEqual([hostStub]); - expect(multipleHostsPage.find('QueryPage').prop('selectedTargets')).toEqual([hostStub, { ...hostStub, id: 99 }]); + mount(connectedComponent(ConnectedQueryPage, { + mockStore: singleHostMockStore, + props: singleHostProps, + })); + + mount(connectedComponent(ConnectedQueryPage, { + mockStore: multipleHostMockStore, + props: multipleHostsProps, + })); + + expect(singleHostMockStore.getActions()).toInclude({ + type: 'SET_SELECTED_TARGETS', + payload: { + selectedTargets: [hostStub], + }, + }); + + expect(multipleHostMockStore.getActions()).toInclude({ + type: 'SET_SELECTED_TARGETS', + payload: { + selectedTargets: [hostStub, { ...hostStub, id: 99 }], + }, + }); }); it('sets targetError in state when the query is run and there are no selected targets', () => { diff --git a/frontend/pages/queries/QueryPage/helpers.js b/frontend/pages/queries/QueryPage/helpers.js new file mode 100644 index 0000000000..222399c90b --- /dev/null +++ b/frontend/pages/queries/QueryPage/helpers.js @@ -0,0 +1,34 @@ +import { differenceWith, isEqual, uniqWith } from 'lodash'; + +import { setSelectedTargets } from 'redux/nodes/components/QueryPages/actions'; + +const targetsChanged = (hosts, targets) => { + const sameLength = hosts.length === targets.length; + + if (sameLength) { + const delta = differenceWith(hosts, targets, isEqual); + + return !!delta.length; + } + + return true; +}; + +const comparator = (arrayVal, otherVal) => { + return arrayVal.target_type === otherVal.target_type && + arrayVal.id === otherVal.id; +}; + +const selectHosts = (dispatch, { hosts = [], selectedTargets = [] }) => { + if (!hosts.length || !targetsChanged(hosts, selectedTargets)) { + return false; + } + + const newTargets = uniqWith([...hosts, ...selectedTargets], comparator); + + dispatch(setSelectedTargets(newTargets)); + + return false; +}; + +export default { selectHosts }; diff --git a/frontend/pages/queries/QueryPage/helpers.tests.js b/frontend/pages/queries/QueryPage/helpers.tests.js new file mode 100644 index 0000000000..bd5394d1c0 --- /dev/null +++ b/frontend/pages/queries/QueryPage/helpers.tests.js @@ -0,0 +1,100 @@ +import expect from 'expect'; + +import helpers from 'pages/queries/QueryPage/helpers'; +import { initialState } from 'redux/nodes/components/QueryPages/reducer'; +import Test from 'test'; + +describe('QueryPage - helpers', () => { + describe('#selectHosts', () => { + const createMockStore = () => { + return Test.Helpers.reduxMockStore({ + components: { + QueryPages: initialState, + }, + }); + }; + + context('when there are selected targets and no selected hosts', () => { + it('does not dispatch an action to set targets', () => { + const mockStore = createMockStore(); + const selectedTargets = [Test.Stubs.labelStub]; + + helpers.selectHosts(mockStore.dispatch, { + hosts: [], + selectedTargets, + }); + + expect(mockStore.getActions()).toEqual([]); + }); + }); + + context('when there are selected hosts and no selected targets', () => { + it('sets the selected targets to the selected hosts', () => { + const mockStore = createMockStore(); + const selectedHosts = [Test.Stubs.hostStub]; + + helpers.selectHosts(mockStore.dispatch, { + hosts: selectedHosts, + selectedTargets: [], + }); + + expect(mockStore.getActions()).toInclude({ + type: 'SET_SELECTED_TARGETS', + payload: { + selectedTargets: selectedHosts, + }, + }); + }); + }); + + context('when there are selected hosts and selected targets', () => { + it('sets the selected targets to the combined selected hosts and selected targets', () => { + const mockStore = createMockStore(); + + helpers.selectHosts(mockStore.dispatch, { + hosts: [Test.Stubs.hostStub], + selectedTargets: [Test.Stubs.labelStub], + }); + + expect(mockStore.getActions()).toInclude({ + type: 'SET_SELECTED_TARGETS', + payload: { + selectedTargets: [ + Test.Stubs.hostStub, + Test.Stubs.labelStub, + ], + }, + }); + }); + }); + + context('when a target is duplicated', () => { + it('does not duplicate the target when setting selected targets', () => { + const mockStore = createMockStore(); + + helpers.selectHosts(mockStore.dispatch, { + hosts: [Test.Stubs.hostStub], + selectedTargets: [Test.Stubs.labelStub, Test.Stubs.hostStub], + }); + + expect(mockStore.getActions()).toInclude({ + type: 'SET_SELECTED_TARGETS', + payload: { + selectedTargets: [Test.Stubs.hostStub, Test.Stubs.labelStub], + }, + }); + }); + + it('does not set targets if the hosts and selectedTargets are equal', () => { + const mockStore = createMockStore(); + + helpers.selectHosts(mockStore.dispatch, { + hosts: [Test.Stubs.hostStub], + selectedTargets: [Test.Stubs.hostStub], + }); + + expect(mockStore.getActions()).toEqual([]); + }); + }); + }); +}); diff --git a/frontend/redux/nodes/components/QueryPages/actions.js b/frontend/redux/nodes/components/QueryPages/actions.js index a64088d859..641d439227 100644 --- a/frontend/redux/nodes/components/QueryPages/actions.js +++ b/frontend/redux/nodes/components/QueryPages/actions.js @@ -1,6 +1,6 @@ import { find } from 'lodash'; -import { osqueryTables } from '../../../../utilities/osquery_tables'; +import { osqueryTables } from 'utilities/osquery_tables'; export const SELECT_OSQUERY_TABLE = 'SELECT_OSQUERY_TABLE'; export const SET_QUERY_TEXT = 'SET_QUERY_TEXT';