From 828d50fba06373b6c70ddf788b8c02cf76e78c7d Mon Sep 17 00:00:00 2001 From: jpellizzari Date: Tue, 21 Mar 2017 16:41:05 -0700 Subject: [PATCH 01/10] unit test added --- .../scripts/components/topology-options.js | 15 +- .../scripts/reducers/__tests__/root-test.js | 158 +++++++++++++++--- client/app/scripts/reducers/root.js | 3 +- 3 files changed, 146 insertions(+), 30 deletions(-) diff --git a/client/app/scripts/components/topology-options.js b/client/app/scripts/components/topology-options.js index 2c572060a..4d7210211 100644 --- a/client/app/scripts/components/topology-options.js +++ b/client/app/scripts/components/topology-options.js @@ -11,14 +11,21 @@ class TopologyOptions extends React.Component { const { activeOptions, topologyId } = this.props; const optionId = option.get('id'); const activeValue = activeOptions && activeOptions.has(optionId) - ? activeOptions.get(optionId) : option.get('defaultValue'); + ? activeOptions.get(optionId) + : option.get('defaultValue'); return (
- {option.get('options').map(item => )} + {option.get('options').map(item => ( + + ))}
); diff --git a/client/app/scripts/reducers/__tests__/root-test.js b/client/app/scripts/reducers/__tests__/root-test.js index 58441f5f2..ccefa0827 100644 --- a/client/app/scripts/reducers/__tests__/root-test.js +++ b/client/app/scripts/reducers/__tests__/root-test.js @@ -46,33 +46,118 @@ describe('RootReducer', () => { } }; - const topologies = [{ - hide_if_empty: true, - name: 'Processes', - rank: 1, - sub_topologies: [], - url: '/api/topology/processes', - fullName: 'Processes', - id: 'processes', - options: [ - { - defaultValue: 'hide', - id: 'unconnected', - options: [ - { - label: 'Unconnected nodes hidden', - value: 'hide' - } - ] + const topologies = [ + { + hide_if_empty: true, + name: 'Processes', + rank: 1, + sub_topologies: [], + url: '/api/topology/processes', + fullName: 'Processes', + id: 'processes', + options: [ + { + defaultValue: 'hide', + id: 'unconnected', + selectType: 'one', + options: [ + { + label: 'Unconnected nodes hidden', + value: 'hide' + } + ] + } + ], + stats: { + edge_count: 379, + filtered_nodes: 214, + node_count: 320, + nonpseudo_node_count: 320 } - ], - stats: { - edge_count: 379, - filtered_nodes: 214, - node_count: 320, - nonpseudo_node_count: 320 + }, + { + hide_if_empty: true, + name: 'Pods', + options: [ + { + defaultValue: 'default', + id: 'namespace', + selectType: 'many', + options: [ + { + label: 'monitoring', + value: 'monitoring' + }, + { + label: 'scope', + value: 'scope' + }, + { + label: 'All Namespaces', + value: 'all' + } + ] + }, + { + defaultValue: 'hide', + id: 'pseudo', + options: [ + { + label: 'Show Unmanaged', + value: 'show' + }, + { + label: 'Hide Unmanaged', + value: 'hide' + } + ] + } + ], + rank: 3, + stats: { + edge_count: 15, + filtered_nodes: 16, + node_count: 32, + nonpseudo_node_count: 27 + }, + sub_topologies: [ + { + hide_if_empty: true, + name: 'services', + options: [ + { + defaultValue: 'default', + id: 'namespace', + selectType: 'many', + options: [ + { + label: 'monitoring', + value: 'monitoring' + }, + { + label: 'scope', + value: 'scope' + }, + { + label: 'All Namespaces', + value: 'all' + } + ] + } + ], + rank: 0, + stats: { + edge_count: 14, + filtered_nodes: 16, + node_count: 159, + nonpseudo_node_count: 154 + }, + url: '/api/topology/services' + } + ], + url: '/api/topology/pods' } - }]; + ]; // actions @@ -268,6 +353,29 @@ describe('RootReducer', () => { expect(activeTopologyOptionsSelector(nextState)).toBeUndefined(); expect(getUrlState(nextState).topologyOptions.topo1.option1).toBe('off'); }); + it('changes topologyOptions for selectType "many"', () => { + const action = { + type: ActionTypes.CHANGE_TOPOLOGY_OPTION, + topologyId: 'services', + option: 'namespace', + value: ['scope', 'monitoring'] + }; + let nextState = initialState; + nextState = reducer(nextState, { + type: ActionTypes.RECEIVE_TOPOLOGIES, + topologies + }); + nextState = reducer(nextState, { + type: ActionTypes.CLICK_TOPOLOGY, + topologyId: 'services' + }); + + nextState = reducer(nextState, action); + expect(activeTopologyOptionsSelector(nextState).toJS()).toEqual({ + namespace: ['scope', 'monitoring'], + pseudo: 'hide' + }); + }); it('sets topology options from route', () => { RouteAction.state = { diff --git a/client/app/scripts/reducers/root.js b/client/app/scripts/reducers/root.js index 7221a1d41..ebe271b95 100644 --- a/client/app/scripts/reducers/root.js +++ b/client/app/scripts/reducers/root.js @@ -179,7 +179,8 @@ export function rootReducer(state = initialState, action) { const topology = findTopologyById(state.get('topologies'), action.topologyId); if (topology) { const topologyId = topology.get('parentId') || topology.get('id'); - if (state.getIn(['topologyOptions', topologyId, action.option]) !== action.value) { + const currentOption = state.getIn(['topologyOptions', topologyId, action.option]); + if (currentOption !== action.value) { state = clearNodes(state); } state = state.setIn( From 454acdd9997d9dec7b8681738dfe017fc56e01c5 Mon Sep 17 00:00:00 2001 From: jpellizzari Date: Wed, 22 Mar 2017 17:13:57 -0700 Subject: [PATCH 02/10] Changed option value to list --- .../components/topology-option-action.js | 17 ++-- .../scripts/components/topology-options.js | 17 +++- .../scripts/reducers/__tests__/root-test.js | 83 ++++++++++--------- client/app/scripts/reducers/root.js | 30 +++++-- 4 files changed, 89 insertions(+), 58 deletions(-) diff --git a/client/app/scripts/components/topology-option-action.js b/client/app/scripts/components/topology-option-action.js index f8ee59770..1bb090d38 100644 --- a/client/app/scripts/components/topology-option-action.js +++ b/client/app/scripts/components/topology-option-action.js @@ -1,9 +1,6 @@ import React from 'react'; -import { connect } from 'react-redux'; -import { changeTopologyOption } from '../actions/app-actions'; - -class TopologyOptionAction extends React.Component { +export default class TopologyOptionAction extends React.Component { constructor(props, context) { super(props, context); @@ -13,13 +10,14 @@ class TopologyOptionAction extends React.Component { onClick(ev) { ev.preventDefault(); const { optionId, topologyId, item } = this.props; - this.props.changeTopologyOption(optionId, item.get('value'), topologyId); + this.props.onClick(optionId, item.get('value'), topologyId); } render() { const { activeValue, item } = this.props; - const className = activeValue === item.get('value') - ? 'topology-option-action topology-option-action-selected' : 'topology-option-action'; + const className = activeValue.includes(item.get('value')) + ? 'topology-option-action topology-option-action-selected' + : 'topology-option-action'; return (
{item.get('label')} @@ -27,8 +25,3 @@ class TopologyOptionAction extends React.Component { ); } } - -export default connect( - null, - { changeTopologyOption } -)(TopologyOptionAction); diff --git a/client/app/scripts/components/topology-options.js b/client/app/scripts/components/topology-options.js index 4d7210211..b7ee171f1 100644 --- a/client/app/scripts/components/topology-options.js +++ b/client/app/scripts/components/topology-options.js @@ -4,8 +4,18 @@ import { connect } from 'react-redux'; import { getCurrentTopologyOptions } from '../utils/topology-utils'; import { activeTopologyOptionsSelector } from '../selectors/topology'; import TopologyOptionAction from './topology-option-action'; +import { changeTopologyOption } from '../actions/app-actions'; class TopologyOptions extends React.Component { + constructor(props, context) { + super(props, context); + + this.handleOptionClick = this.handleOptionClick.bind(this); + } + + handleOptionClick(optionId, value, topologyId) { + this.props.changeTopologyOption(optionId, value, topologyId); + } renderOption(option) { const { activeOptions, topologyId } = this.props; @@ -19,6 +29,7 @@ class TopologyOptions extends React.Component {
{option.get('options').map(item => ( - {this.props.options && this.props.options.toIndexedSeq().map( + {options && options.toIndexedSeq().map( option => this.renderOption(option))}
); @@ -50,5 +62,6 @@ function mapStateToProps(state) { } export default connect( - mapStateToProps + mapStateToProps, + { changeTopologyOption } )(TopologyOptions); diff --git a/client/app/scripts/reducers/__tests__/root-test.js b/client/app/scripts/reducers/__tests__/root-test.js index ccefa0827..7d5e9e002 100644 --- a/client/app/scripts/reducers/__tests__/root-test.js +++ b/client/app/scripts/reducers/__tests__/root-test.js @@ -1,4 +1,6 @@ import { is, fromJS } from 'immutable'; +import expect from 'expect'; + import { TABLE_VIEW_MODE } from '../../constants/naming'; // Root reducer test suite using Jasmine matchers import { constructEdgeId } from '../../utils/layouter-utils'; @@ -307,7 +309,16 @@ describe('RootReducer', () => { expect(nextState.get('topologies').size).toBe(2); expect(nextState.get('currentTopology').get('name')).toBe('Topo1'); expect(nextState.get('currentTopology').get('url')).toBe('/topo1'); - expect(nextState.get('currentTopology').get('options').first().get('id')).toBe('option1'); + expect(nextState.get('currentTopology').get('options').first().get('id')).toEqual(['option1']); + expect(nextState.getIn(['currentTopology', 'options']).toJS()).toEqual([{ + id: 'option1', + defaultValue: 'off', + selectType: 'one', + options: [ + { value: 'on'}, + { value: 'off'} + ] + }]); }); it('get sub-topology', () => { @@ -318,7 +329,7 @@ describe('RootReducer', () => { expect(nextState.get('topologies').size).toBe(2); expect(nextState.get('currentTopology').get('name')).toBe('topo 1 grouped'); expect(nextState.get('currentTopology').get('url')).toBe('/topo1-grouped'); - expect(nextState.get('currentTopology').get('options')).toBeUndefined(); + expect(nextState.get('currentTopology').get('options')).toNotExist(); }); // topology options @@ -330,51 +341,49 @@ describe('RootReducer', () => { // default options expect(activeTopologyOptionsSelector(nextState).has('option1')).toBeTruthy(); - expect(activeTopologyOptionsSelector(nextState).get('option1')).toBe('off'); - expect(getUrlState(nextState).topologyOptions.topo1.option1).toBe('off'); + expect(activeTopologyOptionsSelector(nextState).get('option1')).toBeA('array'); + expect(activeTopologyOptionsSelector(nextState).get('option1')).toEqual(['off']); + expect(getUrlState(nextState).topologyOptions.topo1.option1).toEqual(['off']); // turn on nextState = reducer(nextState, ChangeTopologyOptionAction); - expect(activeTopologyOptionsSelector(nextState).get('option1')).toBe('on'); - expect(getUrlState(nextState).topologyOptions.topo1.option1).toBe('on'); + expect(activeTopologyOptionsSelector(nextState).get('option1')).toEqual(['on']); + expect(getUrlState(nextState).topologyOptions.topo1.option1).toEqual(['on']); // turn off nextState = reducer(nextState, ChangeTopologyOptionAction2); - expect(activeTopologyOptionsSelector(nextState).get('option1')).toBe('off'); - expect(getUrlState(nextState).topologyOptions.topo1.option1).toBe('off'); + expect(activeTopologyOptionsSelector(nextState).get('option1')).toEqual(['off']); + expect(getUrlState(nextState).topologyOptions.topo1.option1).toEqual(['off']); // sub-topology should retain main topo options nextState = reducer(nextState, ClickSubTopologyAction); - expect(activeTopologyOptionsSelector(nextState).get('option1')).toBe('off'); - expect(getUrlState(nextState).topologyOptions.topo1.option1).toBe('off'); + expect(activeTopologyOptionsSelector(nextState).get('option1')).toEqual(['off']); + expect(getUrlState(nextState).topologyOptions.topo1.option1).toEqual(['off']); // other topology w/o options dont return options, but keep in app state nextState = reducer(nextState, ClickTopology2Action); - expect(activeTopologyOptionsSelector(nextState)).toBeUndefined(); - expect(getUrlState(nextState).topologyOptions.topo1.option1).toBe('off'); + expect(activeTopologyOptionsSelector(nextState)).toNotExist(); + expect(getUrlState(nextState).topologyOptions.topo1.option1).toEqual(['off']); }); - it('changes topologyOptions for selectType "many"', () => { - const action = { - type: ActionTypes.CHANGE_TOPOLOGY_OPTION, + + it('adds/removes a topology option', () => { + const addAction = { + type: ActionTypes.ADD_TOPOLOGY_OPTION, topologyId: 'services', option: 'namespace', - value: ['scope', 'monitoring'] + value: 'scope' + }; + const removeAction = { + type: ActionTypes.REMOVE_TOPOLOGY_OPTION, + topologyId: 'services', + option: 'namespace', + value: 'scope' }; let nextState = initialState; - nextState = reducer(nextState, { - type: ActionTypes.RECEIVE_TOPOLOGIES, - topologies - }); - nextState = reducer(nextState, { - type: ActionTypes.CLICK_TOPOLOGY, - topologyId: 'services' - }); + nextState = reducer(nextState, { type: ActionTypes.RECEIVE_TOPOLOGIES, topologies}); + nextState = reducer(nextState, { type: ActionTypes.CLICK_TOPOLOGY, topologyId: 'services' }); - nextState = reducer(nextState, action); - expect(activeTopologyOptionsSelector(nextState).toJS()).toEqual({ - namespace: ['scope', 'monitoring'], - pseudo: 'hide' - }); + nextState = reducer(nextState, addAction); }); it('sets topology options from route', () => { @@ -404,8 +413,8 @@ describe('RootReducer', () => { nextState = reducer(nextState, RouteAction); nextState = reducer(nextState, ReceiveTopologiesAction); nextState = reducer(nextState, ClickTopologyAction); - expect(activeTopologyOptionsSelector(nextState).get('option1')).toBe('off'); - expect(getUrlState(nextState).topologyOptions.topo1.option1).toBe('off'); + expect(activeTopologyOptionsSelector(nextState).get('option1')).toEqual(['off']); + expect(getUrlState(nextState).topologyOptions.topo1.option1).toEqual(['off']); }); // nodes delta @@ -423,7 +432,7 @@ describe('RootReducer', () => { it('shows nodes that were received', () => { let nextState = initialState; nextState = reducer(nextState, ReceiveNodesDeltaAction); - expect(nextState.get('nodes').toJS()).toEqual(NODE_SET); + expect(nextState.get('nodes').toJS()).toInclude(NODE_SET); }); it('knows a route was set', () => { @@ -439,11 +448,11 @@ describe('RootReducer', () => { nextState = reducer(nextState, ClickNodeAction); expect(nextState.get('selectedNodeId')).toBe('n1'); - expect(nextState.get('nodes').toJS()).toEqual(NODE_SET); + expect(nextState.get('nodes').toJS()).toInclude(NODE_SET); nextState = reducer(nextState, deSelectNode); expect(nextState.get('selectedNodeId')).toBe(null); - expect(nextState.get('nodes').toJS()).toEqual(NODE_SET); + expect(nextState.get('nodes').toJS()).toInclude(NODE_SET); }); it('keeps showing nodes on navigating back after node click', () => { @@ -460,7 +469,7 @@ describe('RootReducer', () => { RouteAction.state = {topologyId: 'topo1', selectedNodeId: null}; nextState = reducer(nextState, RouteAction); expect(nextState.get('selectedNodeId')).toBe(null); - expect(nextState.get('nodes').toJS()).toEqual(NODE_SET); + expect(nextState.get('nodes').toJS()).toInclude(NODE_SET); }); it('closes details when changing topologies', () => { @@ -486,12 +495,12 @@ describe('RootReducer', () => { it('resets topology on websocket reconnect', () => { let nextState = initialState; nextState = reducer(nextState, ReceiveNodesDeltaAction); - expect(nextState.get('nodes').toJS()).toEqual(NODE_SET); + expect(nextState.get('nodes').toJS()).toInclude(NODE_SET); nextState = reducer(nextState, CloseWebsocketAction); expect(nextState.get('websocketClosed')).toBeTruthy(); // keep showing old nodes - expect(nextState.get('nodes').toJS()).toEqual(NODE_SET); + expect(nextState.get('nodes').toJS()).toInclude(NODE_SET); nextState = reducer(nextState, OpenWebsocketAction); expect(nextState.get('websocketClosed')).toBeFalsy(); diff --git a/client/app/scripts/reducers/root.js b/client/app/scripts/reducers/root.js index ebe271b95..536d5a3f2 100644 --- a/client/app/scripts/reducers/root.js +++ b/client/app/scripts/reducers/root.js @@ -87,15 +87,32 @@ export const initialState = makeMap({ zoomCache: makeMap(), }); +function calcSelectType(topology) { + const result = { + ...topology, + options: topology.options && topology.options.map((option) => { + // Server doesn't return the `selectType` key unless the option is something other than `one`. + // Default to `one` if undefined, so the component doesn't have to handle this. + option.selectType = option.selectType || 'one'; + return option; + }) + }; + + if (topology.sub_topologies) { + result.sub_topologies = topology.sub_topologies.map(calcSelectType); + } + return result; +} + // adds ID field to topology (based on last part of URL path) and save urls in // map for easy lookup function processTopologies(state, nextTopologies) { // filter out hidden topos const visibleTopologies = filterHiddenTopologies(nextTopologies); - + // set `selectType` field for topology and sub_topologies options (recursive). + const topologiesWithSelectType = visibleTopologies.map(calcSelectType); // add IDs to topology objects in-place - const topologiesWithId = updateTopologyIds(visibleTopologies); - + const topologiesWithId = updateTopologyIds(topologiesWithSelectType); // cache URLs by ID state = state.set('topologyUrlsById', setTopologyUrlsById(state.get('topologyUrlsById'), topologiesWithId)); @@ -106,8 +123,7 @@ function processTopologies(state, nextTopologies) { } function setTopology(state, topologyId) { - state = state.set('currentTopology', findTopologyById( - state.get('topologies'), topologyId)); + state = state.set('currentTopology', findTopologyById(state.get('topologies'), topologyId)); return state.set('currentTopologyId', topologyId); } @@ -118,7 +134,7 @@ function setDefaultTopologyOptions(state, topologyList) { topology.get('options').forEach((option) => { const optionId = option.get('id'); const defaultValue = option.get('defaultValue'); - defaultOptions = defaultOptions.set(optionId, defaultValue); + defaultOptions = defaultOptions.set(optionId, [defaultValue]); }); } @@ -185,7 +201,7 @@ export function rootReducer(state = initialState, action) { } state = state.setIn( ['topologyOptions', topologyId, action.option], - action.value + [action.value] ); } return state; From 4612738580e088cc31b99c485e87a41056d5ef59 Mon Sep 17 00:00:00 2001 From: jpellizzari Date: Wed, 22 Mar 2017 17:39:41 -0700 Subject: [PATCH 03/10] Added comma-separated URL params --- client/app/scripts/components/topology-options.js | 15 ++++++++++++++- client/app/scripts/utils/web-api-utils.js | 9 +++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/client/app/scripts/components/topology-options.js b/client/app/scripts/components/topology-options.js index b7ee171f1..f6cd8724e 100644 --- a/client/app/scripts/components/topology-options.js +++ b/client/app/scripts/components/topology-options.js @@ -14,7 +14,20 @@ class TopologyOptions extends React.Component { } handleOptionClick(optionId, value, topologyId) { - this.props.changeTopologyOption(optionId, value, topologyId); + const { activeOptions, options } = this.props; + const selectedOption = options.find(o => o.get('id') === optionId); + + if (selectedOption.get('selectType') === 'union') { + if (activeOptions.get(selectedOption.get('id')).includes(value)) { + // Option already exists; user is de-selecting + console.log('TODO! removeOption action'); + } else { + // Option does not exist; user is enabling. + console.log('TODO! addOption action'); + } + } else { + this.props.changeTopologyOption(optionId, value, topologyId); + } } renderOption(option) { diff --git a/client/app/scripts/utils/web-api-utils.js b/client/app/scripts/utils/web-api-utils.js index c7df5d2cd..41e9fa7e2 100644 --- a/client/app/scripts/utils/web-api-utils.js +++ b/client/app/scripts/utils/web-api-utils.js @@ -1,7 +1,7 @@ import debug from 'debug'; import reqwest from 'reqwest'; import defaults from 'lodash/defaults'; -import { Map as makeMap } from 'immutable'; +import { Map as makeMap, List } from 'immutable'; import { blurSearch, clearControlError, closeWebsocket, openWebsocket, receiveError, receiveApiDetails, receiveNodesDelta, receiveNodeDetails, receiveControlError, @@ -45,7 +45,12 @@ let continuePolling = true; export function buildOptionsQuery(options) { if (options) { - return options.map((value, param) => `${param}=${value}`).join('&'); + return options.map((value, param) => { + if (List.isList(value)) { + value = value.join(','); + } + return `${param}=${value}`; + }).join('&'); } return ''; } From fff47ee6093821e2eb02339012a2d24be45279b8 Mon Sep 17 00:00:00 2001 From: jpellizzari Date: Wed, 22 Mar 2017 18:00:56 -0700 Subject: [PATCH 04/10] Added 'addOrRemove' flag to change topology action --- client/app/scripts/actions/app-actions.js | 5 +++-- .../scripts/components/topology-options.js | 10 +++------- .../scripts/reducers/__tests__/root-test.js | 20 ++++++++++++++----- client/app/scripts/reducers/root.js | 18 +++++++++++------ 4 files changed, 33 insertions(+), 20 deletions(-) diff --git a/client/app/scripts/actions/app-actions.js b/client/app/scripts/actions/app-actions.js index badb484d1..ae25c8770 100644 --- a/client/app/scripts/actions/app-actions.js +++ b/client/app/scripts/actions/app-actions.js @@ -174,13 +174,14 @@ export function blurSearch() { return { type: ActionTypes.BLUR_SEARCH }; } -export function changeTopologyOption(option, value, topologyId) { +export function changeTopologyOption(option, value, topologyId, addOrRemove) { return (dispatch, getState) => { dispatch({ type: ActionTypes.CHANGE_TOPOLOGY_OPTION, topologyId, option, - value + value, + addOrRemove }); updateRoute(getState); // update all request workers with new options diff --git a/client/app/scripts/components/topology-options.js b/client/app/scripts/components/topology-options.js index f6cd8724e..c4276c28e 100644 --- a/client/app/scripts/components/topology-options.js +++ b/client/app/scripts/components/topology-options.js @@ -18,13 +18,9 @@ class TopologyOptions extends React.Component { const selectedOption = options.find(o => o.get('id') === optionId); if (selectedOption.get('selectType') === 'union') { - if (activeOptions.get(selectedOption.get('id')).includes(value)) { - // Option already exists; user is de-selecting - console.log('TODO! removeOption action'); - } else { - // Option does not exist; user is enabling. - console.log('TODO! addOption action'); - } + const isSelectedAlready = activeOptions.get(selectedOption.get('id')).includes(value); + const addOrRemove = isSelectedAlready ? 'remove' : 'add'; + this.props.changeTopologyOption(optionId, value, topologyId, addOrRemove); } else { this.props.changeTopologyOption(optionId, value, topologyId); } diff --git a/client/app/scripts/reducers/__tests__/root-test.js b/client/app/scripts/reducers/__tests__/root-test.js index 7d5e9e002..ec7993be7 100644 --- a/client/app/scripts/reducers/__tests__/root-test.js +++ b/client/app/scripts/reducers/__tests__/root-test.js @@ -368,22 +368,32 @@ describe('RootReducer', () => { it('adds/removes a topology option', () => { const addAction = { - type: ActionTypes.ADD_TOPOLOGY_OPTION, + type: ActionTypes.CHANGE_TOPOLOGY_OPTION, topologyId: 'services', option: 'namespace', - value: 'scope' + value: 'scope', + addOrRemove: 'add' }; const removeAction = { - type: ActionTypes.REMOVE_TOPOLOGY_OPTION, + type: ActionTypes.CHANGE_TOPOLOGY_OPTION, topologyId: 'services', option: 'namespace', - value: 'scope' + value: 'scope', + addOrRemove: 'remove' }; let nextState = initialState; nextState = reducer(nextState, { type: ActionTypes.RECEIVE_TOPOLOGIES, topologies}); nextState = reducer(nextState, { type: ActionTypes.CLICK_TOPOLOGY, topologyId: 'services' }); - nextState = reducer(nextState, addAction); + expect(activeTopologyOptionsSelector(nextState).toJS()).toEqual({ + namespace: ['default', 'scope'], + pseudo: ['hide'] + }); + nextState = reducer(nextState, removeAction); + expect(activeTopologyOptionsSelector(nextState).toJS()).toEqual({ + namespace: ['default'], + pseudo: ['hide'] + }); }); it('sets topology options from route', () => { diff --git a/client/app/scripts/reducers/root.js b/client/app/scripts/reducers/root.js index 536d5a3f2..3e673e837 100644 --- a/client/app/scripts/reducers/root.js +++ b/client/app/scripts/reducers/root.js @@ -1,6 +1,6 @@ /* eslint-disable import/no-webpack-loader-syntax, import/no-unresolved */ import debug from 'debug'; -import { size, each, includes } from 'lodash'; +import { size, each, includes, isEqual } from 'lodash'; import { fromJS, is as isDeepEqual, List as makeList, Map as makeMap, OrderedMap as makeOrderedMap, Set as makeSet } from 'immutable'; @@ -194,15 +194,21 @@ export function rootReducer(state = initialState, action) { // set option on parent topology const topology = findTopologyById(state.get('topologies'), action.topologyId); if (topology) { + let values = [action.value]; const topologyId = topology.get('parentId') || topology.get('id'); + const optionKey = ['topologyOptions', topologyId, action.option]; const currentOption = state.getIn(['topologyOptions', topologyId, action.option]); - if (currentOption !== action.value) { + + if (!isEqual(currentOption, action.value)) { state = clearNodes(state); } - state = state.setIn( - ['topologyOptions', topologyId, action.option], - [action.value] - ); + + if (action.addOrRemove === 'add') { + values = state.getIn(optionKey).concat(values); + } else if (action.addOrRemove === 'remove') { + values = state.getIn(optionKey).filter(o => !values.includes(o)); + } + state = state.setIn(optionKey, values); } return state; } From efb68fb2da915b84c24e4ef422fa0c317d5083f0 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Tue, 21 Mar 2017 18:40:02 -0700 Subject: [PATCH 05/10] api_topologies: Add a selectType field to option groups This field changes the option group behaviour depending on its value. Currently only supports two values: "one" (default): Old behaviour, one option can be selected "union": Any number of options can be selected, and the filters are OR-ed togther It is written in such a way as to easily enable a future "intersection" option, as per union but AND-ing the filters. But this is not done here. YAGNI. --- app/api_topologies.go | 68 ++++++++++++++++++++++++++++++++++++------- render/filters.go | 6 ++++ 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 4d0a38e2f..564216293 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -5,8 +5,10 @@ import ( "net/http" "net/url" "sort" + "strings" "sync" + log "github.com/Sirupsen/logrus" "github.com/gorilla/mux" "golang.org/x/net/context" @@ -295,6 +297,59 @@ type APITopologyOptionGroup struct { ID string `json:"id"` Default string `json:"defaultValue,omitempty"` Options []APITopologyOption `json:"options,omitempty"` + // SelectType describes how options can be picked. Currently defined values: + // "one": Default if empty. Exactly one option may be picked from the list. + // "union": Any number of options may be picked. Nodes matching any option filter selected are displayed. + // Value and Default should be a ","-seperated list. + SelectType string `json:"selectType,omitempty"` +} + +// Get the render filters to use for this option group as a Decorator, if any. +// If second arg is false, no decorator was needed. +func (g APITopologyOptionGroup) getFilterDecorator(value string) (render.Decorator, bool) { + if value == "" { + value = g.Default + } + selectType := g.SelectType + if selectType == "" { + selectType = "one" + } + var values []string + switch selectType { + case "one": + values = []string{value} + case "union": + values = strings.Split(value, ",") + default: + log.Errorf("Invalid select type %s for option group %s, ignoring option", selectType, g.ID) + return nil, false + } + filters := []render.FilterFunc{} + for _, opt := range g.Options { + for _, v := range values { + if v != opt.Value { + continue + } + var filter render.FilterFunc + if opt.filter == nil { + // No filter means match everything (pseudo doesn't matter) + filter = func(n report.Node) bool { return true } + } else if opt.filterPseudo { + // Apply filter to pseudo topologies also + filter = opt.filter + } else { + // Allow all pseudo topology nodes, only apply filter to non-pseudo + filter = render.AnyFilterFunc(render.IsPseudoTopology, opt.filter) + } + filters = append(filters, filter) + } + } + if len(filters) == 0 { + return nil, false + } + // Since we've encoded whether to ignore pseudo topologies into each subfilter, + // we want no special behaviour for pseudo topologies here, which corresponds to MakePseudo + return render.MakeFilterPseudoDecorator(render.AnyFilterFunc(filters...)), true } // APITopologyOption describes a ¶m=value to a given topology. @@ -437,17 +492,8 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt var decorators []render.Decorator for _, group := range topology.Options { value := values.Get(group.ID) - for _, opt := range group.Options { - if opt.filter == nil { - continue - } - if (value == "" && group.Default == opt.Value) || (opt.Value != "" && opt.Value == value) { - if opt.filterPseudo { - decorators = append(decorators, render.MakeFilterPseudoDecorator(opt.filter)) - } else { - decorators = append(decorators, render.MakeFilterDecorator(opt.filter)) - } - } + if decorator, ok := group.getFilterDecorator(value); ok { + decorators = append(decorators, decorator) } } if len(decorators) > 0 { diff --git a/render/filters.go b/render/filters.go index fc5425d46..f7df8d337 100644 --- a/render/filters.go +++ b/render/filters.go @@ -321,6 +321,12 @@ func IsNotPseudo(n report.Node) bool { return n.Topology != Pseudo || strings.HasSuffix(n.ID, TheInternetID) || strings.HasPrefix(n.ID, ServiceNodeIDPrefix) } +// IsPseudoTopology returns true if the node is in a pseudo topology, +// mimicing the check performed by MakeFilter() instead of the more complex check in IsNotPseudo() +func IsPseudoTopology(n report.Node) bool { + return n.Topology == Pseudo +} + // IsNamespace checks if the node is a pod/service in the specified namespace func IsNamespace(namespace string) FilterFunc { return func(n report.Node) bool { From bfb68a54a99289536360a5fed55d5e1efc71bf95 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Wed, 22 Mar 2017 18:14:32 -0700 Subject: [PATCH 06/10] api_topologies: Allow selecting multiple k8s namespace filters This is the first usage of the new 'union' selectType. Note that we're still sending 'all' for now. There's currently no easy way to specify this meaning, and arguably it should be done entirely clientside. But for now it just means some UI weirdness where 'all' is one of the options and having it on means anything else you select doesn't matter. --- app/api_topologies.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 564216293..5d09dfd5e 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -50,7 +50,7 @@ var ( // kubernetesFilters generates the current kubernetes filters based on the // available k8s topologies. func kubernetesFilters(namespaces ...string) APITopologyOptionGroup { - options := APITopologyOptionGroup{ID: "namespace", Default: "all"} + options := APITopologyOptionGroup{ID: "namespace", Default: "all", SelectType: "union"} for _, namespace := range namespaces { if namespace == "default" { options.Default = namespace From 9e1666cb4973305dac7c6b5a5c49082d7889f439 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Thu, 23 Mar 2017 14:44:18 -0700 Subject: [PATCH 07/10] api_topologies: Change semantics of blank or omitted option values Instead of value being "" or omitted meaning "use group.Default", we now allow the empty value to go through. This was done to allow multi-value options to be able to encode 'no options selected'. This is important as the alternative is very confusing, as 'nothing selected' would generally mean 'match everything', not 'select default' (which in the k8s namespace example, means the "default" namespace). Since the UI is the only user of this API, and it already sends the default value always, this does not affect any existing usage. Since the UI still wants to show a 'match all' button to prevent confusion, but it's not a normal option (if it were a normal option, it could be combined with others, which isn't meaningful), we add a new key NoneLabel that indicates the name that should be displayed on this extra button. Finally, we implement these changes for the k8s namespace button, ie. - Get rid of All Namespaces option - Add "All Namespaces" as the NoneLabel - Default to "" when "default" namespace not present, which is equivalent to the old "all" option. --- app/api_topologies.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 5d09dfd5e..d63edd77d 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -50,7 +50,7 @@ var ( // kubernetesFilters generates the current kubernetes filters based on the // available k8s topologies. func kubernetesFilters(namespaces ...string) APITopologyOptionGroup { - options := APITopologyOptionGroup{ID: "namespace", Default: "all", SelectType: "union"} + options := APITopologyOptionGroup{ID: "namespace", Default: "", SelectType: "union", NoneLabel: "All Namespaces"} for _, namespace := range namespaces { if namespace == "default" { options.Default = namespace @@ -59,7 +59,6 @@ func kubernetesFilters(namespaces ...string) APITopologyOptionGroup { Value: namespace, Label: namespace, filter: render.IsNamespace(namespace), filterPseudo: false, }) } - options.Options = append(options.Options, APITopologyOption{Value: "all", Label: "All Namespaces", filter: nil, filterPseudo: false}) return options } @@ -294,22 +293,22 @@ func (a byName) Less(i, j int) bool { return a[i].Name < a[j].Name } // APITopologyOptionGroup describes a group of APITopologyOptions type APITopologyOptionGroup struct { - ID string `json:"id"` + ID string `json:"id"` + // Default value for the UI to adopt. NOT used as the default if the value is omitted, allowing "" as a distinct value. Default string `json:"defaultValue,omitempty"` Options []APITopologyOption `json:"options,omitempty"` // SelectType describes how options can be picked. Currently defined values: // "one": Default if empty. Exactly one option may be picked from the list. // "union": Any number of options may be picked. Nodes matching any option filter selected are displayed. - // Value and Default should be a ","-seperated list. + // Value and Default should be a ","-separated list. SelectType string `json:"selectType,omitempty"` + // For "union" type, this is the label the UI should use to represent the case where nothing is selected + NoneLabel string `json:"noneLabel,omitempty"` } // Get the render filters to use for this option group as a Decorator, if any. // If second arg is false, no decorator was needed. func (g APITopologyOptionGroup) getFilterDecorator(value string) (render.Decorator, bool) { - if value == "" { - value = g.Default - } selectType := g.SelectType if selectType == "" { selectType = "one" From 9bccc9918efe02d487747ebfcde7520ae15c94e7 Mon Sep 17 00:00:00 2001 From: jpellizzari Date: Mon, 27 Mar 2017 11:28:42 -0700 Subject: [PATCH 08/10] Added noneLabel button to topology options --- .../scripts/components/topology-options.js | 44 ++++++++++++++++--- .../scripts/reducers/__tests__/root-test.js | 10 ++--- client/app/scripts/reducers/root.js | 8 +--- 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/client/app/scripts/components/topology-options.js b/client/app/scripts/components/topology-options.js index c4276c28e..bc7a153c0 100644 --- a/client/app/scripts/components/topology-options.js +++ b/client/app/scripts/components/topology-options.js @@ -1,5 +1,7 @@ import React from 'react'; import { connect } from 'react-redux'; +import { Map as makeMap } from 'immutable'; +import includes from 'lodash/includes'; import { getCurrentTopologyOptions } from '../utils/topology-utils'; import { activeTopologyOptionsSelector } from '../selectors/topology'; @@ -11,19 +13,37 @@ class TopologyOptions extends React.Component { super(props, context); this.handleOptionClick = this.handleOptionClick.bind(this); + this.handleNoneClick = this.handleNoneClick.bind(this); } handleOptionClick(optionId, value, topologyId) { + let nextOptions = [value]; const { activeOptions, options } = this.props; const selectedOption = options.find(o => o.get('id') === optionId); if (selectedOption.get('selectType') === 'union') { - const isSelectedAlready = activeOptions.get(selectedOption.get('id')).includes(value); - const addOrRemove = isSelectedAlready ? 'remove' : 'add'; - this.props.changeTopologyOption(optionId, value, topologyId, addOrRemove); - } else { - this.props.changeTopologyOption(optionId, value, topologyId); + // Multi-select topology options (such as k8s namespaces) are handled here. + // Users can select one, many, or none of these options. + // The component builds an array of the next selected values that are sent to the action. + const opts = activeOptions.toJS(); + const selected = selectedOption.get('id'); + const isSelectedAlready = includes(opts[selected], value); + + if (isSelectedAlready) { + // Remove the option if it is already selected + nextOptions = opts[selected].filter(o => o !== value); + } else { + // Add it to the array if it's not selected + nextOptions = opts[selected].concat(value); + } + // Since the user is clicking an option, remove the highlighting from the 'none' option. + nextOptions = nextOptions.filter(o => o !== 'none'); } + this.props.changeTopologyOption(optionId, nextOptions, topologyId); + } + + handleNoneClick(optionId, value, topologyId) { + this.props.changeTopologyOption(optionId, ['none'], topologyId); } renderOption(option) { @@ -32,7 +52,10 @@ class TopologyOptions extends React.Component { const activeValue = activeOptions && activeOptions.has(optionId) ? activeOptions.get(optionId) : option.get('defaultValue'); - + const noneItem = makeMap({ + value: 'none', + label: option.get('noneLabel') + }); return (
@@ -46,6 +69,15 @@ class TopologyOptions extends React.Component { item={item} /> ))} + {option.get('selectType') === 'union' && + + }
); diff --git a/client/app/scripts/reducers/__tests__/root-test.js b/client/app/scripts/reducers/__tests__/root-test.js index ec7993be7..45f46daee 100644 --- a/client/app/scripts/reducers/__tests__/root-test.js +++ b/client/app/scripts/reducers/__tests__/root-test.js @@ -167,14 +167,14 @@ describe('RootReducer', () => { type: ActionTypes.CHANGE_TOPOLOGY_OPTION, topologyId: 'topo1', option: 'option1', - value: 'on' + value: ['on'] }; const ChangeTopologyOptionAction2 = { type: ActionTypes.CHANGE_TOPOLOGY_OPTION, topologyId: 'topo1', option: 'option1', - value: 'off' + value: ['off'] }; const ClickNodeAction = { @@ -371,15 +371,13 @@ describe('RootReducer', () => { type: ActionTypes.CHANGE_TOPOLOGY_OPTION, topologyId: 'services', option: 'namespace', - value: 'scope', - addOrRemove: 'add' + value: ['default', 'scope'], }; const removeAction = { type: ActionTypes.CHANGE_TOPOLOGY_OPTION, topologyId: 'services', option: 'namespace', - value: 'scope', - addOrRemove: 'remove' + value: ['default'] }; let nextState = initialState; nextState = reducer(nextState, { type: ActionTypes.RECEIVE_TOPOLOGIES, topologies}); diff --git a/client/app/scripts/reducers/root.js b/client/app/scripts/reducers/root.js index 3e673e837..199645ea4 100644 --- a/client/app/scripts/reducers/root.js +++ b/client/app/scripts/reducers/root.js @@ -194,7 +194,6 @@ export function rootReducer(state = initialState, action) { // set option on parent topology const topology = findTopologyById(state.get('topologies'), action.topologyId); if (topology) { - let values = [action.value]; const topologyId = topology.get('parentId') || topology.get('id'); const optionKey = ['topologyOptions', topologyId, action.option]; const currentOption = state.getIn(['topologyOptions', topologyId, action.option]); @@ -203,12 +202,7 @@ export function rootReducer(state = initialState, action) { state = clearNodes(state); } - if (action.addOrRemove === 'add') { - values = state.getIn(optionKey).concat(values); - } else if (action.addOrRemove === 'remove') { - values = state.getIn(optionKey).filter(o => !values.includes(o)); - } - state = state.setIn(optionKey, values); + state = state.setIn(optionKey, action.value); } return state; } From 8e20ae5ac26a6f6154e38a4be0ca144aed52fe47 Mon Sep 17 00:00:00 2001 From: jpellizzari Date: Mon, 27 Mar 2017 12:52:03 -0700 Subject: [PATCH 09/10] Added logic to default to noneLabel when no options are selected --- client/app/scripts/components/topology-options.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/client/app/scripts/components/topology-options.js b/client/app/scripts/components/topology-options.js index bc7a153c0..f16c8f923 100644 --- a/client/app/scripts/components/topology-options.js +++ b/client/app/scripts/components/topology-options.js @@ -36,8 +36,13 @@ class TopologyOptions extends React.Component { // Add it to the array if it's not selected nextOptions = opts[selected].concat(value); } - // Since the user is clicking an option, remove the highlighting from the 'none' option. - nextOptions = nextOptions.filter(o => o !== 'none'); + // Since the user is clicking an option, remove the highlighting from the 'none' option, + // unless they are removing the last option. In that case, default to the 'none' label. + if (nextOptions.length === 0) { + nextOptions = ['none']; + } else { + nextOptions = nextOptions.filter(o => o !== 'none'); + } } this.props.changeTopologyOption(optionId, nextOptions, topologyId); } From 96494d92c5a6959bcf5ce28d094bacd14a5cb750 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Thu, 23 Mar 2017 16:18:55 -0700 Subject: [PATCH 10/10] api_topologies: Fix tests Tests relied on url param defaults, which no longer work --- app/api_topologies_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/api_topologies_test.go b/app/api_topologies_test.go index ed28e0c33..6ce9b49f1 100644 --- a/app/api_topologies_test.go +++ b/app/api_topologies_test.go @@ -106,6 +106,8 @@ func TestRendererForTopologyWithFiltering(t *testing.T) { urlvalues := url.Values{} urlvalues.Set(systemGroupID, customAPITopologyOptionFilterID) + urlvalues.Set("stopped", "running") + urlvalues.Set("pseudo", "hide") renderer, decorator, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report) if err != nil { t.Fatalf("Topology Registry Report error: %s", err) @@ -135,6 +137,8 @@ func TestRendererForTopologyNoFiltering(t *testing.T) { urlvalues := url.Values{} urlvalues.Set(systemGroupID, customAPITopologyOptionFilterID) + urlvalues.Set("stopped", "running") + urlvalues.Set("pseudo", "hide") renderer, decorator, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report) if err != nil { t.Fatalf("Topology Registry Report error: %s", err) @@ -171,6 +175,8 @@ func getTestContainerLabelFilterTopologySummary(t *testing.T, exclude bool) (det urlvalues := url.Values{} urlvalues.Set(systemGroupID, customAPITopologyOptionFilterID) + urlvalues.Set("stopped", "running") + urlvalues.Set("pseudo", "hide") renderer, decorator, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report) if err != nil { return nil, err