Fix bug selecting targets when querying a host (#1366)

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"

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,
   };
 };

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', () => {

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 };

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([]);
+      });
+    });
+  });
+});

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';