From 46579b1bd67b798f19e0242f997cd8df60843e40 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Tue, 30 Apr 2019 11:45:06 -0700 Subject: [PATCH] Refactor out controlUtils.js module + unit tests (#7350) * [WiP]refactor out a controlUtils.js file * unit tests * add missing license * Addressing comments --- .../ControlPanelsContainer_spec.jsx | 3 +- .../javascripts/explore/controlUtils_spec.jsx | 164 ++++++++++++++++++ .../spec/javascripts/explore/store_spec.jsx | 66 +++++++ .../components/ExploreViewContainer.jsx | 2 +- superset/assets/src/explore/controlUtils.js | 124 +++++++++++++ .../src/explore/reducers/exploreReducer.js | 13 +- .../src/explore/reducers/getInitialState.js | 3 +- superset/assets/src/explore/store.js | 127 +++----------- .../assets/src/visualizations/deckgl/utils.js | 3 + 9 files changed, 392 insertions(+), 113 deletions(-) create mode 100644 superset/assets/spec/javascripts/explore/controlUtils_spec.jsx create mode 100644 superset/assets/spec/javascripts/explore/store_spec.jsx create mode 100644 superset/assets/src/explore/controlUtils.js diff --git a/superset/assets/spec/javascripts/explore/components/ControlPanelsContainer_spec.jsx b/superset/assets/spec/javascripts/explore/components/ControlPanelsContainer_spec.jsx index 842c5e2640199..cda5e0ac67661 100644 --- a/superset/assets/spec/javascripts/explore/components/ControlPanelsContainer_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/ControlPanelsContainer_spec.jsx @@ -18,7 +18,8 @@ */ import React from 'react'; import { shallow } from 'enzyme'; -import { getFormDataFromControls, defaultControls } from 'src/explore/store'; +import { defaultControls } from 'src/explore/store'; +import { getFormDataFromControls } from 'src/explore/controlUtils'; import { ControlPanelsContainer } from 'src/explore/components/ControlPanelsContainer'; import ControlPanelSection from 'src/explore/components/ControlPanelSection'; import * as featureFlags from 'src/featureFlags'; diff --git a/superset/assets/spec/javascripts/explore/controlUtils_spec.jsx b/superset/assets/spec/javascripts/explore/controlUtils_spec.jsx new file mode 100644 index 0000000000000..50f766a1b7b48 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/controlUtils_spec.jsx @@ -0,0 +1,164 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + getControlConfig, + getControlState, + getControlKeys, + applyMapStateToPropsToControl, +} from '../../../src/explore/controlUtils'; + +describe('controlUtils', () => { + const state = { + datasource: { + columns: [ + 'a', 'b', 'c', + ], + metrics: [ + { metric_name: 'first' }, + { metric_name: 'second' }, + ], + }, + }; + + describe('getControlConfig', () => { + it('returns a valid spatial controlConfig', () => { + const spatialControl = getControlConfig('spatial', 'deck_grid'); + expect(spatialControl.type).toEqual('SpatialControl'); + expect(spatialControl.validators).toHaveLength(1); + }); + it('overrides according to vizType', () => { + let control = getControlConfig('metric', 'line'); + expect(control.type).toEqual('MetricsControl'); + expect(control.validators).toHaveLength(1); + + // deck_polygon overrides and removes validators + control = getControlConfig('metric', 'deck_polygon'); + expect(control.type).toEqual('MetricsControl'); + expect(control.validators).toHaveLength(0); + }); + }); + + describe('getControlKeys', () => { + + window.featureFlags = { + SCOPED_FILTER: false, + }; + + it('gets only strings, even when React components are in conf', () => { + const keys = getControlKeys('filter_box'); + expect(keys.every(k => typeof k === 'string')).toEqual(true); + expect(keys).toHaveLength(16); + }); + it('gets the right set of controlKeys for filter_box', () => { + const keys = getControlKeys('filter_box'); + expect(keys.sort()).toEqual([ + 'adhoc_filters', + 'cache_timeout', + 'datasource', + 'date_filter', + 'druid_time_origin', + 'filter_configs', + 'granularity', + 'instant_filtering', + 'show_druid_time_granularity', + 'show_druid_time_origin', + 'show_sqla_time_column', + 'show_sqla_time_granularity', + 'slice_id', + 'time_range', + 'url_params', + 'viz_type', + ]); + }); + }); + + describe('applyMapStateToPropsToControl,', () => { + it('applies state to props as expected', () => { + let control = getControlConfig('all_columns', 'table'); + control = applyMapStateToPropsToControl(control, state); + expect(control.options).toEqual(['a', 'b', 'c']); + }); + + it('removes the mapStateToProps key from the object', () => { + let control = getControlConfig('all_columns', 'table'); + control = applyMapStateToPropsToControl(control, state); + expect(control.mapStateToProps).toBe(undefined); + }); + + }); + + describe('getControlState', () => { + + it('to be function free', () => { + const control = getControlState('all_columns', 'table', state, ['a']); + expect(control.mapStateToProps).toBe(undefined); + expect(control.validators).toBe(undefined); + }); + + it('to fix multi with non-array values', () => { + const control = getControlState('all_columns', 'table', state, 'a'); + expect(control.value).toEqual(['a']); + }); + + it('removes missing/invalid choice', () => { + let control = getControlState('stacked_style', 'area', state, 'stack'); + expect(control.value).toBe('stack'); + + control = getControlState('stacked_style', 'area', state, 'FOO'); + expect(control.value).toBe(null); + }); + + it('applies the default function for metrics', () => { + const control = getControlState('metrics', 'table', state); + expect(control.default).toEqual(['first']); + }); + + it('applies the default function for metric', () => { + const control = getControlState('metric', 'table', state); + expect(control.default).toEqual('first'); + }); + + it('applies the default function, prefers count if it exists', () => { + const stateWithCount = { + ...state, + datasource: { + ...state.datasource, + metrics: [ + { metric_name: 'first' }, + { metric_name: 'second' }, + { metric_name: 'count' }, + ], + }, + }; + const control = getControlState('metrics', 'table', stateWithCount); + expect(control.default).toEqual(['count']); + }); + + }); + + describe('validateControl', () => { + + it('validates the control, returns an error if empty', () => { + const control = getControlState('metric', 'table', state, null); + expect(control.validationErrors).toEqual(['cannot be empty']); + }); + + }); + +}); diff --git a/superset/assets/spec/javascripts/explore/store_spec.jsx b/superset/assets/spec/javascripts/explore/store_spec.jsx new file mode 100644 index 0000000000000..4884ed180f1ed --- /dev/null +++ b/superset/assets/spec/javascripts/explore/store_spec.jsx @@ -0,0 +1,66 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { applyDefaultFormData } from '../../../src/explore/store'; + +describe('store', () => { + + describe('applyDefaultFormData', () => { + + window.featureFlags = { + SCOPED_FILTER: false, + }; + + it('applies default to formData if the key is missing', () => { + const inputFormData = { + datasource: '11_table', + viz_type: 'table', + }; + let outputFormData = applyDefaultFormData(inputFormData); + expect(outputFormData.row_limit).toEqual(10000); + + const inputWithRowLimit = { + ...inputFormData, + row_limit: 888, + }; + outputFormData = applyDefaultFormData(inputWithRowLimit); + expect(outputFormData.row_limit).toEqual(888); + }); + + it('keeps null if key is defined with null', () => { + const inputFormData = { + datasource: '11_table', + viz_type: 'table', + row_limit: null, + }; + const outputFormData = applyDefaultFormData(inputFormData); + expect(outputFormData.row_limit).toBe(null); + }); + + it('removes out of scope, or deprecated keys', () => { + const inputFormData = { + datasource: '11_table', + viz_type: 'table', + this_should_no_be_here: true, + }; + const outputFormData = applyDefaultFormData(inputFormData); + expect(outputFormData.this_should_no_be_here).toBe(undefined); + }); + + }); +}); diff --git a/superset/assets/src/explore/components/ExploreViewContainer.jsx b/superset/assets/src/explore/components/ExploreViewContainer.jsx index 431f2527a090d..9e6647fc55a01 100644 --- a/superset/assets/src/explore/components/ExploreViewContainer.jsx +++ b/superset/assets/src/explore/components/ExploreViewContainer.jsx @@ -29,7 +29,7 @@ import SaveModal from './SaveModal'; import QueryAndSaveBtns from './QueryAndSaveBtns'; import { getExploreUrlAndPayload, getExploreLongUrl } from '../exploreUtils'; import { areObjectsEqual } from '../../reduxUtils'; -import { getFormDataFromControls } from '../store'; +import { getFormDataFromControls } from '../controlUtils'; import { chartPropShape } from '../../dashboard/util/propShapes'; import * as exploreActions from '../actions/exploreActions'; import * as saveModalActions from '../actions/saveModalActions'; diff --git a/superset/assets/src/explore/controlUtils.js b/superset/assets/src/explore/controlUtils.js new file mode 100644 index 0000000000000..65e8c47ea7921 --- /dev/null +++ b/superset/assets/src/explore/controlUtils.js @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import controlPanelConfigs, { sectionsToRender } from './controlPanels'; +import controls from './controls'; + +export function getFormDataFromControls(controlsState) { + const formData = {}; + Object.keys(controlsState).forEach((controlName) => { + formData[controlName] = controlsState[controlName].value; + }); + return formData; +} + +export function validateControl(control) { + const validators = control.validators; + if (validators && validators.length > 0) { + const validatedControl = { ...control }; + const validationErrors = []; + validators.forEach((f) => { + const v = f(control.value); + if (v) { + validationErrors.push(v); + } + }); + delete validatedControl.validators; + return { ...validatedControl, validationErrors }; + } + return control; +} + +export function getControlKeys(vizType, datasourceType) { + const controlKeys = []; + sectionsToRender(vizType, datasourceType).forEach( + section => section.controlSetRows.forEach( + fieldsetRow => fieldsetRow.forEach( + (field) => { + if (typeof field === 'string') { + controlKeys.push(field); + } + }))); + return controlKeys; +} + +export function getControlConfig(controlKey, vizType) { + // Gets the control definition, applies overrides, and executes + // the mapStatetoProps + const vizConf = controlPanelConfigs[vizType] || {}; + const controlOverrides = vizConf.controlOverrides || {}; + const control = { + ...controls[controlKey], + ...controlOverrides[controlKey], + }; + return control; +} + +export function applyMapStateToPropsToControl(control, state) { + if (control.mapStateToProps) { + const appliedControl = { ...control }; + if (state) { + Object.assign(appliedControl, control.mapStateToProps(state, control)); + } + delete appliedControl.mapStateToProps; + return appliedControl; + } + return control; +} + +function handleMissingChoice(controlKey, control) { + // If the value is not valid anymore based on choices, clear it + const value = control.value; + if ( + control.type === 'SelectControl' && + !control.freeForm && + control.choices && + value + ) { + const alteredControl = { ...control }; + const choiceValues = control.choices.map(c => c[0]); + if (control.multi && value.length > 0) { + alteredControl.value = value.filter(el => choiceValues.indexOf(el) > -1); + return alteredControl; + } else if (!control.multi && choiceValues.indexOf(value) < 0) { + alteredControl.value = null; + return alteredControl; + } + } + return control; +} + +export function getControlState(controlKey, vizType, state, value) { + let controlValue = value; + const controlConfig = getControlConfig(controlKey, vizType); + let controlState = { ...controlConfig }; + controlState = applyMapStateToPropsToControl(controlState, state); + + // If default is a function, evaluate it + if (typeof controlState.default === 'function') { + controlState.default = controlState.default(controlState); + } + + // If a choice control went from multi=false to true, wrap value in array + if (controlConfig.multi && value && !Array.isArray(value)) { + controlValue = [value]; + } + controlState.value = controlValue === undefined ? controlState.default : controlValue; + controlState = handleMissingChoice(controlKey, controlState); + return validateControl(controlState); +} diff --git a/superset/assets/src/explore/reducers/exploreReducer.js b/superset/assets/src/explore/reducers/exploreReducer.js index 6f6a1a9e6b4bd..44d5a72c1ebb4 100644 --- a/superset/assets/src/explore/reducers/exploreReducer.js +++ b/superset/assets/src/explore/reducers/exploreReducer.js @@ -17,8 +17,10 @@ * under the License. */ /* eslint camelcase: 0 */ -import { validateControl, getControlsState, getFormDataFromControls } from '../store'; -import controls from '../controls'; +import { + getControlsState, +} from '../store'; +import { getControlState, getFormDataFromControls } from '../controlUtils'; import * as actions from '../actions/exploreActions'; export default function exploreReducer(state = {}, action) { @@ -78,11 +80,10 @@ export default function exploreReducer(state = {}, action) { [actions.SET_FIELD_VALUE]() { // These errors are reported from the Control components let errors = action.validationErrors || []; - let control = { - ...controls[action.controlName], - value: action.value, + const vizType = state.form_data.viz_type; + const control = { + ...getControlState(action.controlName, vizType, state, action.value), }; - control = validateControl(control); // These errors are based on control config `validators` errors = errors.concat(control.validationErrors || []); diff --git a/superset/assets/src/explore/reducers/getInitialState.js b/superset/assets/src/explore/reducers/getInitialState.js index 98b979914a085..892224de57f72 100644 --- a/superset/assets/src/explore/reducers/getInitialState.js +++ b/superset/assets/src/explore/reducers/getInitialState.js @@ -20,7 +20,8 @@ import shortid from 'shortid'; import getToastsFromPyFlashMessages from '../../messageToasts/utils/getToastsFromPyFlashMessages'; import { getChartKey } from '../exploreUtils'; -import { getControlsState, getFormDataFromControls } from '../store'; +import { getControlsState } from '../store'; +import { getFormDataFromControls } from '../controlUtils'; export default function getInitialState(bootstrapData) { const controls = getControlsState(bootstrapData, bootstrapData.form_data); diff --git a/superset/assets/src/explore/store.js b/superset/assets/src/explore/store.js index df456c29ebc94..6d58b8d04055d 100644 --- a/superset/assets/src/explore/store.js +++ b/superset/assets/src/explore/store.js @@ -17,44 +17,13 @@ * under the License. */ /* eslint camelcase: 0 */ -import React from 'react'; +import { + getControlState, + getControlKeys, + getFormDataFromControls, +} from './controlUtils'; import controls from './controls'; -import controlPanelConfigs, { sectionsToRender } from './controlPanels'; - -export function getFormDataFromControls(controlsState) { - const formData = {}; - Object.keys(controlsState).forEach((controlName) => { - formData[controlName] = controlsState[controlName].value; - }); - return formData; -} - -export function validateControl(control) { - const validators = control.validators; - const validationErrors = []; - if (validators && validators.length > 0) { - validators.forEach((f) => { - const v = f(control.value); - if (v) { - validationErrors.push(v); - } - }); - } - if (validationErrors.length > 0) { - return { ...control, validationErrors }; - } - return control; -} - - -export function getControlNames(vizType, datasourceType) { - const controlNames = []; - sectionsToRender(vizType, datasourceType).forEach( - section => section.controlSetRows.forEach( - fsr => fsr.forEach( - f => controlNames.push(f)))); - return controlNames; -} +import controlPanelConfigs from './controlPanels'; function handleDeprecatedControls(formData) { // Reacffectation / handling of deprecated controls @@ -66,104 +35,54 @@ function handleDeprecatedControls(formData) { } } -export function getControlsState(state, form_data) { +export function getControlsState(state, inputFormData) { /* * Gets a new controls object to put in the state. The controls object * is similar to the configuration control with only the controls * related to the current viz_type, materializes mapStateToProps functions, - * adds value keys coming from form_data passed here. This can't be an action creator + * adds value keys coming from inputFormData passed here. This can't be an action creator * just yet because it's used in both the explore and dashboard views. * */ // Getting a list of active control names for the current viz - const formData = Object.assign({}, form_data); + const formData = Object.assign({}, inputFormData); const vizType = formData.viz_type || 'table'; handleDeprecatedControls(formData); - const controlNames = getControlNames(vizType, state.datasource.type); + const controlNames = getControlKeys(vizType, state.datasource.type); const viz = controlPanelConfigs[vizType] || {}; - const controlOverrides = viz.controlOverrides || {}; const controlsState = {}; - controlNames.forEach((k) => { - if (React.isValidElement(k)) { - // no state - return; - } - const control = Object.assign({}, controls[k], controlOverrides[k]); - if (control.mapStateToProps) { - Object.assign(control, control.mapStateToProps(state, control)); - delete control.mapStateToProps; - } - - formData[k] = (control.multi && formData[k] && !Array.isArray(formData[k])) ? [formData[k]] - : formData[k]; - - // If the value is not valid anymore based on choices, clear it - if ( - control.type === 'SelectControl' && - !control.freeForm && - control.choices && - k !== 'datasource' && - formData[k] - ) { - const choiceValues = control.choices.map(c => c[0]); - if (control.multi && formData[k].length > 0) { - formData[k] = formData[k].filter(el => choiceValues.indexOf(el) > -1); - } else if (!control.multi && choiceValues.indexOf(formData[k]) < 0) { - delete formData[k]; - } - } - if (typeof control.default === 'function') { - control.default = control.default(control); - } - control.validationErrors = []; - control.value = control.default; - // formData[k]'s type should match control value type - if (formData[k] !== undefined && - (Array.isArray(formData[k]) && control.multi || !control.multi) - ) { - control.value = formData[k]; - } - controlsState[k] = validateControl(control); + controlNames.forEach((k) => { + const control = getControlState(k, vizType, state, formData[k]); + controlsState[k] = control; + formData[k] = control.value; }); + if (viz.onInit) { return viz.onInit(controlsState); } return controlsState; } -export function applyDefaultFormData(form_data) { - const datasourceType = form_data.datasource.split('__')[1]; - const vizType = form_data.viz_type || 'table'; - const viz = controlPanelConfigs[vizType] || {}; - const controlNames = getControlNames(vizType, datasourceType); - const controlOverrides = viz.controlOverrides || {}; +export function applyDefaultFormData(inputFormData) { + const datasourceType = inputFormData.datasource.split('__')[1]; + const vizType = inputFormData.viz_type; + const controlNames = getControlKeys(vizType, datasourceType); const formData = {}; controlNames.forEach((k) => { - const control = Object.assign({}, controls[k]); - if (controlOverrides[k]) { - Object.assign(control, controlOverrides[k]); - } - if (form_data[k] === undefined) { - if (typeof control.default === 'function') { - formData[k] = control.default(controls[k]); - } else { - formData[k] = control.default; - } + const controlState = getControlState(k, vizType, null, inputFormData[k]); + if (inputFormData[k] === undefined) { + formData[k] = controlState.value; } else { - formData[k] = form_data[k]; + formData[k] = inputFormData[k]; } }); return formData; } -export const autoQueryControls = [ - 'datasource', - 'viz_type', -]; const defaultControls = Object.assign({}, controls); Object.keys(controls).forEach((f) => { diff --git a/superset/assets/src/visualizations/deckgl/utils.js b/superset/assets/src/visualizations/deckgl/utils.js index b2b130a4835ee..62024efa0d1fc 100644 --- a/superset/assets/src/visualizations/deckgl/utils.js +++ b/superset/assets/src/visualizations/deckgl/utils.js @@ -34,6 +34,9 @@ export function getBreakPoints({ // compute evenly distributed break points based on number of buckets const numBuckets = formDataNumBuckets ? parseInt(formDataNumBuckets, 10) : DEFAULT_NUM_BUCKETS; const [minValue, maxValue] = extent(features, accessor); + if (minValue === undefined) { + return []; + } const delta = (maxValue - minValue) / numBuckets; const precision = delta === 0 ? 0