From fa610eb2044fd48e22f158b1643afe1545bb0366 Mon Sep 17 00:00:00 2001 From: GabeLoins Date: Wed, 7 Mar 2018 16:44:33 -0800 Subject: [PATCH 1/3] adding streamlined metric editing --- .../components/ColumnTypeLabel.jsx | 4 +- .../javascripts/components/OnPasteSelect.jsx | 6 +- .../assets/javascripts/explore/AdhocMetric.js | 37 ++++ .../components/AdhocMetricEditPopover.jsx | 123 +++++++++++ .../AdhocMetricEditPopoverTitle.jsx | 53 +++++ .../explore/components/AdhocMetricOption.jsx | 57 +++++ .../explore/components/AggregateOption.jsx | 20 ++ .../components/MetricDefinitionOption.jsx | 29 +++ .../components/MetricDefinitionValue.jsx | 28 +++ .../components/controls/MetricsControl.jsx | 202 ++++++++++++++++++ .../explore/components/controls/index.js | 2 + .../assets/javascripts/explore/constants.js | 9 + superset/assets/javascripts/explore/main.css | 8 + .../explore/propTypes/adhocMetricType.js | 10 + .../explore/propTypes/columnType.js | 6 + .../explore/propTypes/legacyMetricType.js | 6 + .../explore/propTypes/savedMetricType.js | 6 + .../javascripts/explore/stores/controls.jsx | 9 +- .../javascripts/explore/stores/visTypes.js | 3 +- superset/assets/package.json | 2 +- .../javascripts/explore/AdhocMetric_spec.js | 82 +++++++ .../AdhocMetricEditPopoverTitle_spec.jsx | 48 +++++ .../AdhocMetricEditPopover_spec.jsx | 90 ++++++++ .../components/AdhocMetricOption_spec.jsx | 42 ++++ .../components/AggregateOption_spec.jsx | 15 ++ .../MetricDefinitionOption_spec.jsx | 28 +++ .../components/MetricDefinitionValue_spec.jsx | 29 +++ .../components/MetricsControl_spec.jsx | 184 ++++++++++++++++ superset/assets/stylesheets/superset.less | 6 + superset/connectors/sqla/models.py | 30 ++- superset/viz.py | 9 +- 31 files changed, 1169 insertions(+), 14 deletions(-) create mode 100644 superset/assets/javascripts/explore/AdhocMetric.js create mode 100644 superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx create mode 100644 superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx create mode 100644 superset/assets/javascripts/explore/components/AdhocMetricOption.jsx create mode 100644 superset/assets/javascripts/explore/components/AggregateOption.jsx create mode 100644 superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx create mode 100644 superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx create mode 100644 superset/assets/javascripts/explore/components/controls/MetricsControl.jsx create mode 100644 superset/assets/javascripts/explore/constants.js create mode 100644 superset/assets/javascripts/explore/propTypes/adhocMetricType.js create mode 100644 superset/assets/javascripts/explore/propTypes/columnType.js create mode 100644 superset/assets/javascripts/explore/propTypes/legacyMetricType.js create mode 100644 superset/assets/javascripts/explore/propTypes/savedMetricType.js create mode 100644 superset/assets/spec/javascripts/explore/AdhocMetric_spec.js create mode 100644 superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx create mode 100644 superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx create mode 100644 superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx create mode 100644 superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx create mode 100644 superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx create mode 100644 superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx create mode 100644 superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx diff --git a/superset/assets/javascripts/components/ColumnTypeLabel.jsx b/superset/assets/javascripts/components/ColumnTypeLabel.jsx index 719891e4a278a..9037d2f05cc4a 100644 --- a/superset/assets/javascripts/components/ColumnTypeLabel.jsx +++ b/superset/assets/javascripts/components/ColumnTypeLabel.jsx @@ -9,9 +9,11 @@ export default function ColumnTypeLabel({ type }) { let stringIcon = ''; if (type === '' || type === 'expression') { stringIcon = 'ƒ'; + } else if (type === 'aggregate') { + stringIcon = 'AGG'; } else if (type.match(/.*char.*/i) || type.match(/string.*/i) || type.match(/.*text.*/i)) { stringIcon = 'ABC'; - } else if (type.match(/.*int.*/i) || type === 'LONG' || type === 'DOUBLE') { + } else if (type.match(/.*int.*/i) || type === 'LONG' || type === 'DOUBLE' || type === 'FLOAT') { stringIcon = '#'; } else if (type.match(/.*bool.*/i)) { stringIcon = 'T/F'; diff --git a/superset/assets/javascripts/components/OnPasteSelect.jsx b/superset/assets/javascripts/components/OnPasteSelect.jsx index b043bf317d175..40bbbd09328d7 100644 --- a/superset/assets/javascripts/components/OnPasteSelect.jsx +++ b/superset/assets/javascripts/components/OnPasteSelect.jsx @@ -49,8 +49,8 @@ export default class OnPasteSelect extends React.Component { render() { const SelectComponent = this.props.selectWrap; const refFunc = (ref) => { - if (this.props.ref) { - this.props.ref(ref); + if (this.props.refFunc) { + this.props.refFunc(ref); } this.pasteInput = ref; }; @@ -68,7 +68,7 @@ export default class OnPasteSelect extends React.Component { OnPasteSelect.propTypes = { separator: PropTypes.string.isRequired, selectWrap: PropTypes.func.isRequired, - ref: PropTypes.func, + refFunc: PropTypes.func, onChange: PropTypes.func.isRequired, valueKey: PropTypes.string.isRequired, labelKey: PropTypes.string.isRequired, diff --git a/superset/assets/javascripts/explore/AdhocMetric.js b/superset/assets/javascripts/explore/AdhocMetric.js new file mode 100644 index 0000000000000..10aff628f9a5f --- /dev/null +++ b/superset/assets/javascripts/explore/AdhocMetric.js @@ -0,0 +1,37 @@ +export default class AdhocMetric { + constructor(adhocMetric) { + this.column = adhocMetric.column; + this.aggregate = adhocMetric.aggregate; + this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label); + + if (this.hasCustomLabel) { + this.label = adhocMetric.label; + } else { + this.label = this.getDefaultLabel(); + } + + this.optionName = adhocMetric.optionName || + `metric_${Math.random().toString(36).substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`; + } + + getDefaultLabel() { + return `${this.aggregate || ''}(${(this.column && this.column.column_name) || ''})` + } + + duplicateWith(nextFields) { + return new AdhocMetric({ + ...this, + ...nextFields, + }); + } + + equals(adhocMetric) { + return adhocMetric.label === this.label && + adhocMetric.aggregate === this.aggregate && + ( + (adhocMetric.column && adhocMetric.column.column_name) === + (this.column && this.column.column_name) + ); + } +} + diff --git a/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx b/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx new file mode 100644 index 0000000000000..f6320876cdcfa --- /dev/null +++ b/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx @@ -0,0 +1,123 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { Button, ControlLabel, FormGroup, Popover } from 'react-bootstrap'; +import VirtualizedSelect from 'react-virtualized-select'; + +import { AGGREGATES } from '../constants'; +import { t } from '../../locales'; +import VirtualizedRendererWrap from '../../components/VirtualizedRendererWrap'; +import OnPasteSelect from '../../components/OnPasteSelect'; +import AdhocMetricEditPopoverTitle from './AdhocMetricEditPopoverTitle'; +import columnType from '../propTypes/columnType'; +import AdhocMetric from '../AdhocMetric'; +import ColumnOption from '../../components/ColumnOption'; + +const propTypes = { + adhocMetric: PropTypes.instanceOf(AdhocMetric).isRequired, + onChange: PropTypes.func.isRequired, + onClose: PropTypes.func.isRequired, + columns: PropTypes.arrayOf(columnType), +}; + +const defaultProps = { + columns: [], +}; + +export default class AdhocMetricEditPopover extends React.Component { + constructor(props) { + super(props); + this.onSave = this.onSave.bind(this); + this.onColumnChange = this.onColumnChange.bind(this); + this.onAggregateChange = this.onAggregateChange.bind(this); + this.onLabelChange = this.onLabelChange.bind(this); + this.state = { adhocMetric: this.props.adhocMetric }; + this.selectProps = { + multi: false, + name: "select-column", + labelKey: 'label', + autosize: false, + clearable: true, + }; + } + + onSave() { + this.props.onChange(this.state.adhocMetric); + this.props.onClose(); + } + + onColumnChange(column) { + this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({ column }) }); + } + + onAggregateChange(aggregate) { + // we construct this object explicitly because we still want to overwrite in the case aggregate is null + this.setState({ + adhocMetric: this.state.adhocMetric.duplicateWith({ aggregate: aggregate && aggregate.aggregate }), + }); + } + + onLabelChange(e) { + this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({ label: e.target.value, hasCustomLabel: true })}); + } + + render() { + const { adhocMetric, columns, onChange, onClose, ...rest } = this.props; + + const columnSelectProps = { + placeholder: t('%s column(s)', columns.length), + options: columns, + value: this.state.adhocMetric.column && this.state.adhocMetric.column.column_name, + onChange: this.onColumnChange, + optionRenderer: VirtualizedRendererWrap((option) => ( + + )), + valueRenderer: (column) => column.column_name, + valueKey: 'column_name', + }; + + const aggregateSelectProps = { + placeholder: t('%s aggregates(s)', Object.keys(AGGREGATES).length), + options: Object.keys(AGGREGATES).map((aggregate) => ({ aggregate })), + value: this.state.adhocMetric.aggregate, + onChange: this.onAggregateChange, + optionRenderer: VirtualizedRendererWrap((aggregate) => aggregate.aggregate), + valueRenderer: (aggregate) => aggregate.aggregate, + valueKey: 'aggregate', + }; + + const popoverTitle = ( + + ); + + const stateIsValid = this.state.adhocMetric.column && this.state.adhocMetric.aggregate; + const hasUnsavedChanges = this.state.adhocMetric.equals(this.props.adhocMetric); + + return ( + + + column + + + + aggregate + + + + + + ) + } +} +AdhocMetricEditPopover.propTypes = propTypes; +AdhocMetricEditPopover.defaultProps = defaultProps; diff --git a/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx b/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx new file mode 100644 index 0000000000000..79ac7359cfbb1 --- /dev/null +++ b/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx @@ -0,0 +1,53 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { FormControl, OverlayTrigger, Tooltip } from 'react-bootstrap'; +import AdhocMetric from '../AdhocMetric'; + +const propTypes = { + adhocMetric: PropTypes.instanceOf(AdhocMetric), + onChange: PropTypes.func.isRequired, +}; + +export default class AdhocMetricEditPopoverTitle extends React.Component { + constructor(props) { + super(props); + this.state = { + isHovered: false, + isEditable: false, + }; + } + + render() { + const { adhocMetric, onChange } = this.props; + + const editPrompt = Click to edit label; + + return ( + this.setState({ isHovered: true }) } + onMouseOut={() => this.setState({ isHovered: false }) } + onClick={() => this.setState({ isEditable: true }) } + onBlur={() => this.setState({ isEditable: false }) } + > + {this.state.isEditable ? + ref && ref.focus()} + /> : + + {adhocMetric.hasCustomLabel ? adhocMetric.label : 'My Metric'} +   + + + } + + ) + } +} +AdhocMetricEditPopoverTitle.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx b/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx new file mode 100644 index 0000000000000..c48613212c8b8 --- /dev/null +++ b/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx @@ -0,0 +1,57 @@ +import React from 'react'; +import ReactDOM from 'react-dom'; +import PropTypes from 'prop-types'; +import { FormControl, Label, OverlayTrigger, Popover } from 'react-bootstrap'; + +import AdhocMetricEditPopover from './AdhocMetricEditPopover'; +import AdhocMetric from '../AdhocMetric'; +import columnType from '../propTypes/columnType'; + +const propTypes = { + adhocMetric: PropTypes.instanceOf(AdhocMetric), + onMetricEdit: PropTypes.func.isRequired, + columns: PropTypes.arrayOf(columnType), +}; + +export default class AdhocMetricOption extends React.PureComponent { + constructor(props) { + super(props); + this.closeMetricEditOverlay = this.closeMetricEditOverlay.bind(this); + } + + closeMetricEditOverlay() { + this.refs.overlay.hide(); + } + + render() { + const { adhocMetric } = this.props; + const overlay = ( + + ); + + return ( +
{e.stopPropagation()}} > + + + +
+ ); + } +} +AdhocMetricOption.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/AggregateOption.jsx b/superset/assets/javascripts/explore/components/AggregateOption.jsx new file mode 100644 index 0000000000000..993c2b4b8a0f9 --- /dev/null +++ b/superset/assets/javascripts/explore/components/AggregateOption.jsx @@ -0,0 +1,20 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import ColumnTypeLabel from '../../components/ColumnTypeLabel'; + +const propTypes = { + aggregate: PropTypes.object.isRequired, + showType: PropTypes.bool, +}; + +export default function AggregateOption({ aggregate, showType }) { + return ( +
+ {showType && } + + {aggregate.aggregate_name} + +
+ ); +} +AggregateOption.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx b/superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx new file mode 100644 index 0000000000000..a129329c95632 --- /dev/null +++ b/superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx @@ -0,0 +1,29 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +import MetricOption from '../../components/MetricOption'; +import ColumnOption from '../../components/ColumnOption'; +import AggregateOption from './AggregateOption'; + +const propTypes = { + option: PropTypes.object.isRequired, +}; + +export default function MetricDefinitionOption({ option }) { + if (option.metric_name) { + return ( + + ); + } else if (option.column_name) { + return ( + + ); + } else if (option.aggregate_name) { + return ( + + ); + } + console.error("You must supply either a saved metric, column or aggregate to MetricDefinitionOption"); + return null; +} +MetricDefinitionOption.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx b/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx new file mode 100644 index 0000000000000..2b0bdcab66af8 --- /dev/null +++ b/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx @@ -0,0 +1,28 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +import AdhocMetricOption from './AdhocMetricOption'; +import AdhocMetric from '../AdhocMetric'; +import columnType from '../propTypes/columnType'; +import MetricOption from '../../components/MetricOption'; + +const propTypes = { + option: PropTypes.object.isRequired, + onMetricEdit: PropTypes.func, + columns: PropTypes.arrayOf(columnType), +}; + +export default function MetricDefinitionValue({ option, onMetricEdit, columns }) { + if (option.metric_name) { + return ( + + ); + } else if (option instanceof AdhocMetric) { + return ( + + ); + } + console.error("You must supply either a saved metric or adhoc metric to MetricDefinitionValue"); + return null; +} +MetricDefinitionValue.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx b/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx new file mode 100644 index 0000000000000..067373efd09c3 --- /dev/null +++ b/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx @@ -0,0 +1,202 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import VirtualizedSelect from 'react-virtualized-select'; +import Select, { Creatable } from 'react-select'; +import ControlHeader from '../ControlHeader'; +import { t } from '../../../locales'; +import VirtualizedRendererWrap from '../../../components/VirtualizedRendererWrap'; +import OnPasteSelect from '../../../components/OnPasteSelect'; +import MetricDefinitionOption from '../MetricDefinitionOption'; +import MetricDefinitionValue from '../MetricDefinitionValue'; +import AdhocMetric from '../../AdhocMetric'; +import columnType from '../../propTypes/columnType'; +import savedMetricType from '../../propTypes/savedMetricType'; +import adhocMetricType from '../../propTypes/adhocMetricType'; +import { AGGREGATES } from '../../constants'; + +const propTypes = { + multi: PropTypes.bool, + name: PropTypes.string.isRequired, + onChange: PropTypes.func, + value: PropTypes.arrayOf(PropTypes.oneOfType([ PropTypes.string, adhocMetricType, ])), + columns: PropTypes.arrayOf(columnType), + savedMetrics: PropTypes.arrayOf(savedMetricType), +}; + +const defaultProps = { + onChange: () => {}, +}; + +function isDictionaryForAdhocMetric(value) { + return value && !(value instanceof AdhocMetric) && value.column && value.aggregate && value.label; +} + +// adhoc metrics are stored as dictionaries in URL params. When we reload the page we want to convert them +// back into the AdhocMetric class for typechecking, consistency and instance method access. +function coerceAdhocMetrics(values) { + if (!values) { + return []; + } + return values.map(value => { + if (isDictionaryForAdhocMetric(value)) { + return new AdhocMetric(value); + } else { + return value + } + }); +} + +function getDefaultAggregateForColumn(column) { + const type = column.type; + if (type === '' || type === 'expression') { + return AGGREGATES.SUM; + } else if (type.match(/.*char.*/i) || type.match(/string.*/i) || type.match(/.*text.*/i)) { + return AGGREGATES.COUNT_DISTINCT; + } else if (type.match(/.*int.*/i) || type === 'LONG' || type === 'DOUBLE' || type === 'FLOAT') { + return AGGREGATES.SUM; + } else if (type.match(/.*bool.*/i)) { + return AGGREGATES.MAX; + } else if (type.match(/.*time.*/i)) { + return AGGREGATES.COUNT; + } else if (type.match(/unknown/i)) { + return AGGREGATES.COUNT; + } +} + +function selectFilterOption(option, filterValue) { + let endIndex = filterValue.length; + if (filterValue.endsWith(')')) { + endIndex = filterValue.length - 1; + } + const valueAfterAggregate = filterValue.substring(filterValue.indexOf('(') + 1, endIndex); + return option.column_name && (option.column_name.indexOf(valueAfterAggregate) >= 0); +} + +export default class MetricsControl extends React.PureComponent { + constructor(props) { + super(props); + this.onChange = this.onChange.bind(this); + this.onMetricEdit = this.onMetricEdit.bind(this); + this.checkIfAggregateInInput = this.checkIfAggregateInInput.bind(this); + this.optionsForSelect = this.optionsForSelect.bind(this); + this.state = { + aggregateInInput: null, + options: this.optionsForSelect(this.props), + value: coerceAdhocMetrics(this.props.value), + }; + } + + componentDidUpdate(previousProps) { + if ( this.props.columns !== previousProps.columns || this.props.savedMetrics !== previousProps.savedMetrics) { + console.log('updating options for select'); + this.setState({ options: this.optionsForSelect() }); + } + if (this.props.value !== previousProps.value) { + console.log('coercing adhocMetrics'); + this.setState({ value: coerceAdhocMetrics(this.props.value) }); + } + } + + optionsForSelect() { + const options = [ + ...this.props.columns, + ...Object.keys(AGGREGATES).map(aggregate => ({ aggregate_name: aggregate })), + ...this.props.savedMetrics, + ]; + + return options.map((option) => { + if (option.metric_name) { + return { ...option, optionName: option.metric_name }; + } else if (option.column_name) { + return { ...option, optionName: '_col_' + option.column_name }; + } else if (option.aggregate_name) { + return { ...option, optionName: '_aggregate_' + option.aggregate_name }; + } + }); + } + + onMetricEdit(changedMetric) { + this.props.onChange(this.state.value.map((value) => { + if (value.optionName === changedMetric.optionName) { + return changedMetric; + } + return value; + })); + } + + checkIfAggregateInInput(input) { + let nextState = { aggregateInInput: null }; + Object.keys(AGGREGATES).forEach(aggregate => { + if (input.toLowerCase().startsWith(aggregate.toLowerCase() + '(')) { + nextState = { aggregateInInput: aggregate }; + } + }); + this.clearedAggregateInInput = this.state.aggregateInInput; + this.setState(nextState); + } + + onChange(opts) { + const optionValues = opts.map((option) => { + if (option.metric_name) { + return option.metric_name; + } else if (option.column_name) { + const clearedAggregate = this.clearedAggregateInInput; + this.clearedAggregateInInput = null; + return new AdhocMetric({ + column: option, + aggregate: clearedAggregate || getDefaultAggregateForColumn(option), + }); + } else if (option instanceof AdhocMetric) { + return option; + } else if (option.aggregate_name) { + const newValue = `${option.aggregate_name}()`; + this.select.setInputValue(newValue); + this.select.handleInputChange({ target: { value: newValue }}); + // we need to set a timeout here or the selectionWill be overwritten by some browsers (e.g. Chrome) + setTimeout(() => { + this.select.input.input.selectionStart = newValue.length - 1; + this.select.input.input.selectionEnd = newValue.length - 1; + }, 0); + } + }).filter(option => option); + this.props.onChange(optionValues); + } + + render() { + // TODO figure out why the dropdown isnt appearing as soon as a metric is selected + return ( +
+ + ( + + ))} + valueRenderer={(option) => ( + + )} + onInputChange={this.checkIfAggregateInInput} + filterOption={this.state.aggregateInInput ? selectFilterOption : null} + refFunc={(ref) => ref && (this.select = ref._selectRef)} + selectWrap={VirtualizedSelect} + /> +
+ ); + } +} + +MetricsControl.propTypes = propTypes; +MetricsControl.defaultProps = defaultProps; diff --git a/superset/assets/javascripts/explore/components/controls/index.js b/superset/assets/javascripts/explore/components/controls/index.js index 35aaeeff2b774..a7ca4636051ec 100644 --- a/superset/assets/javascripts/explore/components/controls/index.js +++ b/superset/assets/javascripts/explore/components/controls/index.js @@ -17,6 +17,7 @@ import TextControl from './TextControl'; import TimeSeriesColumnControl from './TimeSeriesColumnControl'; import ViewportControl from './ViewportControl'; import VizTypeControl from './VizTypeControl'; +import MetricsControl from './MetricsControl'; const controlMap = { AnnotationLayerControl, @@ -38,5 +39,6 @@ const controlMap = { TimeSeriesColumnControl, ViewportControl, VizTypeControl, + MetricsControl, }; export default controlMap; diff --git a/superset/assets/javascripts/explore/constants.js b/superset/assets/javascripts/explore/constants.js new file mode 100644 index 0000000000000..330841f4acbec --- /dev/null +++ b/superset/assets/javascripts/explore/constants.js @@ -0,0 +1,9 @@ +export const AGGREGATES = { + AVG: 'AVG', + COUNT: 'COUNT ', + COUNT_DISTINCT: 'COUNT_DISTINCT', + MAX: 'MAX', + MIN: 'MIN', + SUM: 'SUM', +}; + diff --git a/superset/assets/javascripts/explore/main.css b/superset/assets/javascripts/explore/main.css index 2add0abf68d4b..8cebc6c3080c1 100644 --- a/superset/assets/javascripts/explore/main.css +++ b/superset/assets/javascripts/explore/main.css @@ -39,6 +39,10 @@ width: 100px; } +.control-panel-section .metrics-select .Select-multi-value-wrapper .Select-input > input { + width: 300px; +} + .background-transparent { background-color: transparent !important; } @@ -134,3 +138,7 @@ .datasource-container { overflow: auto; } + +.edit-adhoc-metric-save-btn { + margin-right: 5px; +} diff --git a/superset/assets/javascripts/explore/propTypes/adhocMetricType.js b/superset/assets/javascripts/explore/propTypes/adhocMetricType.js new file mode 100644 index 0000000000000..b11126e8fe954 --- /dev/null +++ b/superset/assets/javascripts/explore/propTypes/adhocMetricType.js @@ -0,0 +1,10 @@ +import PropTypes from 'prop-types'; + +import { AGGREGATES } from '../constants'; +import columnType from './columnType'; + +export default PropTypes.shape({ + column: columnType.isRequired, + aggregate: PropTypes.oneOf(Object.keys(AGGREGATES)).isRequired, + label: PropTypes.string.isRequired, +}); diff --git a/superset/assets/javascripts/explore/propTypes/columnType.js b/superset/assets/javascripts/explore/propTypes/columnType.js new file mode 100644 index 0000000000000..10d6346eb9632 --- /dev/null +++ b/superset/assets/javascripts/explore/propTypes/columnType.js @@ -0,0 +1,6 @@ +import PropTypes from 'prop-types'; + +export default PropTypes.shape({ + column_name: PropTypes.string.isRequired, + type: PropTypes.string.isRequired, +}); diff --git a/superset/assets/javascripts/explore/propTypes/legacyMetricType.js b/superset/assets/javascripts/explore/propTypes/legacyMetricType.js new file mode 100644 index 0000000000000..dccafaa867484 --- /dev/null +++ b/superset/assets/javascripts/explore/propTypes/legacyMetricType.js @@ -0,0 +1,6 @@ +import PropTypes from 'prop-types'; + +export default PropTypes.shape({ + column: PropTypes.string.isRequired, + type: PropTypes.string.isRequired, +}); diff --git a/superset/assets/javascripts/explore/propTypes/savedMetricType.js b/superset/assets/javascripts/explore/propTypes/savedMetricType.js new file mode 100644 index 0000000000000..3e713a85d73ca --- /dev/null +++ b/superset/assets/javascripts/explore/propTypes/savedMetricType.js @@ -0,0 +1,6 @@ +import PropTypes from 'prop-types'; + +export default PropTypes.shape({ + metric_name: PropTypes.string.isRequired, + expression: PropTypes.string.isRequired, +}); diff --git a/superset/assets/javascripts/explore/stores/controls.jsx b/superset/assets/javascripts/explore/stores/controls.jsx index 617c717d5fa06..bb97bd5050ad0 100644 --- a/superset/assets/javascripts/explore/stores/controls.jsx +++ b/superset/assets/javascripts/explore/stores/controls.jsx @@ -129,19 +129,18 @@ export const controls = { }, metrics: { - type: 'SelectControl', + type: 'MetricsControl', multi: true, label: t('Metrics'), validators: [v.nonEmpty], - valueKey: 'metric_name', - optionRenderer: m => , - valueRenderer: m => , + valueKey: 'optionName', default: (c) => { const metric = mainMetric(c.options); return metric ? [metric] : null; }, mapStateToProps: state => ({ - options: (state.datasource) ? state.datasource.metrics : [], + columns: state.datasource ? state.datasource.columns : [], + savedMetrics: state.datasource ? state.datasource.metrics : [], }), description: t('One or many metrics to display'), }, diff --git a/superset/assets/javascripts/explore/stores/visTypes.js b/superset/assets/javascripts/explore/stores/visTypes.js index 75443469f3889..d848f40e10ffe 100644 --- a/superset/assets/javascripts/explore/stores/visTypes.js +++ b/superset/assets/javascripts/explore/stores/visTypes.js @@ -139,7 +139,8 @@ export const visTypes = { label: t('Query'), expanded: true, controlSetRows: [ - ['metrics', 'groupby'], + ['metrics'], + ['groupby'], ['limit'], ], }, diff --git a/superset/assets/package.json b/superset/assets/package.json index 7d41be37283b3..039c21bd62d93 100644 --- a/superset/assets/package.json +++ b/superset/assets/package.json @@ -96,7 +96,7 @@ "react-map-gl": "^3.0.4", "react-redux": "^5.0.2", "react-resizable": "^1.3.3", - "react-select": "1.0.0-rc.10", + "react-select": "1.2.1", "react-select-fast-filter-options": "^0.2.1", "react-sortable-hoc": "^0.6.7", "react-split-pane": "^0.1.66", diff --git a/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js b/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js new file mode 100644 index 0000000000000..3a3e9de273c2c --- /dev/null +++ b/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js @@ -0,0 +1,82 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import AdhocMetric from '../../../javascripts/explore/AdhocMetric'; +import { AGGREGATES } from '../../../javascripts/explore/constants'; + +const valueColumn = { type: 'DOUBLE', column_name: 'value' }; + +describe('AdhocMetric', () => { + it('sets label, hasCustomLabel and optionName in constructor', () => { + const adhocMetric = new AdhocMetric({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + }); + expect(adhocMetric.optionName.length).to.be.above(10); + expect(adhocMetric).to.deep.equal({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + label: 'SUM(value)', + hasCustomLabel: false, + optionName: adhocMetric.optionName, + }); + }) + + it('can create altered duplicates', () => { + const adhocMetric1 = new AdhocMetric({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + }); + const adhocMetric2 = adhocMetric1.duplicateWith({ aggregate: AGGREGATES.AVG }); + + expect(adhocMetric1.column).to.equal(adhocMetric2.column); + expect(adhocMetric1.column).to.equal(valueColumn); + + expect(adhocMetric1.aggregate).to.equal(AGGREGATES.SUM); + expect(adhocMetric2.aggregate).to.equal(AGGREGATES.AVG); + }) + + it('can verify equality', () => { + const adhocMetric1 = new AdhocMetric({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + }); + const adhocMetric2 = adhocMetric1.duplicateWith({}); + + expect(adhocMetric1.equals(adhocMetric2)).to.be.true; + }) + + it('can verify inequality', () => { + const adhocMetric1 = new AdhocMetric({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + label: 'old label', + hasCustomLabel: true, + }); + const adhocMetric2 = adhocMetric1.duplicateWith({ label: 'new label' }); + + expect(adhocMetric1.equals(adhocMetric2)).to.be.false; + }) + + it('updates label if hasCustomLabel is false', () => { + const adhocMetric1 = new AdhocMetric({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + }); + const adhocMetric2 = adhocMetric1.duplicateWith({ aggregate: AGGREGATES.AVG }); + + expect(adhocMetric2.label).to.equal('AVG(value)'); + }) + + it('keeps label if hasCustomLabel is true', () => { + const adhocMetric1 = new AdhocMetric({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + hasCustomLabel: true, + label: 'label1', + }); + const adhocMetric2 = adhocMetric1.duplicateWith({ aggregate: AGGREGATES.AVG }); + + expect(adhocMetric2.label).to.equal('label1'); + }) +}); diff --git a/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx new file mode 100644 index 0000000000000..517186829f530 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx @@ -0,0 +1,48 @@ +/* eslint-disable no-unused-expressions */ +import React from 'react'; +import sinon from 'sinon'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; +import { FormControl, OverlayTrigger } from 'react-bootstrap'; + +import AdhocMetric from '../../../../javascripts/explore/AdhocMetric'; +import AdhocMetricEditPopoverTitle from '../../../../javascripts/explore/components/AdhocMetricEditPopoverTitle'; +import { AGGREGATES } from '../../../../javascripts/explore/constants'; + +const columns = [ + { type: 'VARCHAR(255)', column_name: 'source' }, + { type: 'VARCHAR(255)', column_name: 'target' }, + { type: 'DOUBLE', column_name: 'value' }, +]; + +const sumValueAdhocMetric = new AdhocMetric({ + column: columns[2], + aggregate: AGGREGATES.SUM, +}); + +function setup(overrides) { + const onChange = sinon.spy(); + const props = { + adhocMetric: sumValueAdhocMetric, + onChange, + ...overrides, + } + const wrapper = shallow(); + return { wrapper, onChange }; +} + +describe('AdhocMetricEditPopoverTitle', () => { + it('renders an OverlayTrigger wrapper with the title', () => { + const { wrapper } = setup(); + expect(wrapper.find(OverlayTrigger)).to.have.lengthOf(1); + expect(wrapper.find(OverlayTrigger).dive().text()).to.equal('My Metric\xa0') + }); + + it('transfers to edit mode when clicked', () => { + const { wrapper } = setup(); + expect(wrapper.state('isEditable')).to.be.false; + wrapper.simulate('click') + expect(wrapper.state('isEditable')).to.be.true; + }); +}); diff --git a/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx new file mode 100644 index 0000000000000..ecabb46ee3ac7 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx @@ -0,0 +1,90 @@ +/* eslint-disable no-unused-expressions */ +import React from 'react'; +import sinon from 'sinon'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; +import { Button, FormGroup, Popover } from 'react-bootstrap'; + +import AdhocMetric from '../../../../javascripts/explore/AdhocMetric'; +import AdhocMetricEditPopover from '../../../../javascripts/explore/components/AdhocMetricEditPopover'; +import { AGGREGATES } from '../../../../javascripts/explore/constants'; + +const columns = [ + { type: 'VARCHAR(255)', column_name: 'source' }, + { type: 'VARCHAR(255)', column_name: 'target' }, + { type: 'DOUBLE', column_name: 'value' }, +]; + +const sumValueAdhocMetric = new AdhocMetric({ + column: columns[2], + aggregate: AGGREGATES.SUM, +}); + +function setup(overrides) { + const onChange = sinon.spy(); + const onClose = sinon.spy(); + const props = { + adhocMetric: sumValueAdhocMetric, + onChange, + onClose, + columns, + ...overrides, + } + const wrapper = shallow(); + return { wrapper, onChange, onClose }; +} + +describe('AdhocMetricEditPopover', () => { + it('renders a popover with edit metric form contents', () => { + const { wrapper } = setup(); + expect(wrapper.find(Popover)).to.have.lengthOf(1); + expect(wrapper.find(FormGroup)).to.have.lengthOf(2); + expect(wrapper.find(Button)).to.have.lengthOf(2); + }); + + it('overwrites the adhocMetric in state with onColumnChange', () => { + const { wrapper } = setup(); + wrapper.instance().onColumnChange(columns[0]); + expect(wrapper.state('adhocMetric')).to.deep.equal(sumValueAdhocMetric.duplicateWith({ column: columns[0] })); + }); + + it('overwrites the adhocMetric in state with onAggregateChange', () => { + const { wrapper } = setup(); + wrapper.instance().onAggregateChange({ aggregate: AGGREGATES.AVG }); + expect(wrapper.state('adhocMetric')).to.deep.equal(sumValueAdhocMetric.duplicateWith({ aggregate: AGGREGATES.AVG })); + }); + + it('overwrites the adhocMetric in state with onLabelChange', () => { + const { wrapper } = setup(); + wrapper.instance().onLabelChange({ target: { value: 'new label' }}); + expect(wrapper.state('adhocMetric').label).to.equal('new label'); + expect(wrapper.state('adhocMetric').hasCustomLabel).to.be.true; + }); + + it('returns to default labels when the custom label is cleared', () => { + const { wrapper } = setup(); + wrapper.instance().onLabelChange({ target: { value: 'new label' }}); + wrapper.instance().onLabelChange({ target: { value: '' }}); + expect(wrapper.state('adhocMetric').label).to.equal('SUM(value)'); + expect(wrapper.state('adhocMetric').hasCustomLabel).to.be.false; + }); + + it('prevents saving if no column or aggregate is chosen', () => { + const { wrapper, onChange, onClose } = setup(); + expect(wrapper.find(Button).find({ disabled: true })).to.have.lengthOf(0); + wrapper.instance().onColumnChange(null); + expect(wrapper.find(Button).find({ disabled: true })).to.have.lengthOf(1); + wrapper.instance().onColumnChange({ column: columns[0] }); + expect(wrapper.find(Button).find({ disabled: true })).to.have.lengthOf(0); + wrapper.instance().onAggregateChange(null); + expect(wrapper.find(Button).find({ disabled: true })).to.have.lengthOf(1); + }); + + it('highlights save if changes are present', () => { + const { wrapper, onChange, onClose } = setup(); + expect(wrapper.find(Button).find({ bsStyle: 'primary' })).to.have.lengthOf(0); + wrapper.instance().onColumnChange({ column: columns[1] }); + expect(wrapper.find(Button).find({ bsStyle: 'primary' })).to.have.lengthOf(1); + }); +}); diff --git a/superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx b/superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx new file mode 100644 index 0000000000000..c39d515841b48 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx @@ -0,0 +1,42 @@ +/* eslint-disable no-unused-expressions */ +import React from 'react'; +import sinon from 'sinon'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; +import { Label, OverlayTrigger } from 'react-bootstrap'; + +import AdhocMetric from '../../../../javascripts/explore/AdhocMetric'; +import AdhocMetricOption from '../../../../javascripts/explore/components/AdhocMetricOption'; +import { AGGREGATES } from '../../../../javascripts/explore/constants'; + +const columns = [ + { type: 'VARCHAR(255)', column_name: 'source' }, + { type: 'VARCHAR(255)', column_name: 'target' }, + { type: 'DOUBLE', column_name: 'value' }, +]; + +const sumValueAdhocMetric = new AdhocMetric({ + column: columns[2], + aggregate: AGGREGATES.SUM, +}); + +function setup(overrides) { + const onMetricEdit = sinon.spy(); + const props = { + adhocMetric: sumValueAdhocMetric, + onMetricEdit, + columns, + ...overrides, + } + const wrapper = shallow(); + return { wrapper, onMetricEdit }; +} + +describe('AdhocMetricOption', () => { + it('renders an overlay trigger wrapper for the label', () => { + const { wrapper } = setup(); + expect(wrapper.find(OverlayTrigger)).to.have.lengthOf(1); + expect(wrapper.find(Label)).to.have.lengthOf(1); + }); +}); diff --git a/superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx b/superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx new file mode 100644 index 0000000000000..eec97e99b53a0 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx @@ -0,0 +1,15 @@ +/* eslint-disable no-unused-expressions */ +import React from 'react'; +import sinon from 'sinon'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; + +import AggregateOption from '../../../../javascripts/explore/components/AggregateOption'; + +describe('AggregateOption', () => { + it('renders the aggregate', () => { + const wrapper = shallow(); + expect(wrapper.text()).to.equal('SUM'); + }); +}); diff --git a/superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx b/superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx new file mode 100644 index 0000000000000..0e8207a685341 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx @@ -0,0 +1,28 @@ +/* eslint-disable no-unused-expressions */ +import React from 'react'; +import sinon from 'sinon'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; + +import MetricDefinitionOption from '../../../../javascripts/explore/components/MetricDefinitionOption'; +import MetricOption from '../../../../javascripts/components/MetricOption'; +import ColumnOption from '../../../../javascripts/components/ColumnOption'; +import AggregateOption from '../../../../javascripts/explore/components/AggregateOption'; + +describe('MetricDefinitionOption', () => { + it('renders a MetricOption given a saved metric', () => { + const wrapper = shallow(); + expect(wrapper.find(MetricOption)).to.have.lengthOf(1); + }); + + it('renders a ColumnOption given a column', () => { + const wrapper = shallow(); + expect(wrapper.find(ColumnOption)).to.have.lengthOf(1); + }); + + it('renders an AggregateOption given an aggregate metric', () => { + const wrapper = shallow(); + expect(wrapper.find(AggregateOption)).to.have.lengthOf(1); + }); +}); diff --git a/superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx b/superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx new file mode 100644 index 0000000000000..556bd9b5b47e1 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx @@ -0,0 +1,29 @@ +/* eslint-disable no-unused-expressions */ +import React from 'react'; +import sinon from 'sinon'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; + +import MetricDefinitionValue from '../../../../javascripts/explore/components/MetricDefinitionValue'; +import MetricOption from '../../../../javascripts/components/MetricOption'; +import AdhocMetricOption from '../../../../javascripts/explore/components/AdhocMetricOption'; +import AdhocMetric from '../../../../javascripts/explore/AdhocMetric'; +import { AGGREGATES } from '../../../../javascripts/explore/constants'; + +const sumValueAdhocMetric = new AdhocMetric({ + column: { type: 'DOUBLE', column_name: 'value' }, + aggregate: AGGREGATES.SUM, +}); + +describe('MetricDefinitionValue', () => { + it('renders a MetricOption given a saved metric', () => { + const wrapper = shallow(); + expect(wrapper.find(MetricOption)).to.have.lengthOf(1); + }); + + it('renders an AdhocMetricOption given an adhoc metric', () => { + const wrapper = shallow( {}} option={sumValueAdhocMetric} />); + expect(wrapper.find(AdhocMetricOption)).to.have.lengthOf(1); + }); +}); diff --git a/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx new file mode 100644 index 0000000000000..f710de8c385d7 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx @@ -0,0 +1,184 @@ +/* eslint-disable no-unused-expressions */ +import React from 'react'; +import sinon from 'sinon'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; + +import MetricsControl from '../../../../javascripts/explore/components/controls/MetricsControl'; +import { AGGREGATES } from '../../../../javascripts/explore/constants'; +import OnPasteSelect from '../../../../javascripts/components/OnPasteSelect'; +import AdhocMetric from '../../../../javascripts/explore/AdhocMetric'; + +const defaultProps = { + name: 'metrics', + label: 'Metrics', + value: undefined, + columns: [ + { type: 'VARCHAR(255)', column_name: 'source' }, + { type: 'VARCHAR(255)', column_name: 'target' }, + { type: 'DOUBLE', column_name: 'value' }, + ], + savedMetrics: [ + { metric_name: 'sum__value', expression: 'SUM(energy_usage.value)' }, + { metric_name: 'avg__value', expression: 'AVG(energy_usage.value)' }, + ], +}; + +function setup(overrides) { + const onChange = sinon.spy(); + const props = { + onChange, + ...defaultProps, + ...overrides, + } + const wrapper = shallow(); + return { wrapper, onChange }; +} + +const valueColumn = { type: 'DOUBLE', column_name: 'value' }; + +const sumValueAdhocMetric = new AdhocMetric({ + column: valueColumn, + aggregate: AGGREGATES.SUM, + label: 'SUM(value)', +}); + +describe('MetricsControl', () => { + + it('renders an OnPasteSelect', () => { + const { wrapper } = setup(); + expect(wrapper.find(OnPasteSelect)).to.have.lengthOf(1); + }); + + describe('constructor', () => { + + it('unifies options for the dropdown select with aggregates', () => { + const { wrapper } = setup(); + expect(wrapper.state('options')).to.deep.equal([ + { optionName: '_col_source', type: 'VARCHAR(255)', column_name: 'source' }, + { optionName: '_col_target', type: 'VARCHAR(255)', column_name: 'target' }, + { optionName: '_col_value', type: 'DOUBLE', column_name: 'value' }, + ...Object.keys(AGGREGATES).map( + aggregate => ({ aggregate_name: aggregate, optionName: '_aggregate_' + aggregate }) + ), + { optionName: 'sum__value', metric_name: 'sum__value', expression: 'SUM(energy_usage.value)' }, + { optionName: 'avg__value', metric_name: 'avg__value', expression: 'AVG(energy_usage.value)' }, + ]); + }); + + it('coerces Adhoc Metrics from form data into instances of the AdhocMetric class and leaves saved metrics', () => { + const { wrapper } = setup({ + value: [ + { + column: { type: 'double', column_name: 'value' }, + aggregate: AGGREGATES.SUM, + label: 'SUM(value)', + }, + 'avg__value', + ], + }); + + const adhocMetric = wrapper.state('value')[0]; + expect(adhocMetric instanceof AdhocMetric).to.be.true; + expect(adhocMetric.optionName.length).to.be.above(10); + expect(wrapper.state('value')).to.deep.equal([ + { + column: { type: 'double', column_name: 'value' }, + aggregate: AGGREGATES.SUM, + label: 'SUM(value)', + hasCustomLabel: false, + optionName: adhocMetric.optionName, + }, + 'avg__value', + ]); + }); + + }); + + describe('onChange', () => { + + it('handles saved metrics being selected', () => { + const { wrapper, onChange } = setup(); + const select = wrapper.find(OnPasteSelect); + select.simulate('change', [{ metric_name: 'sum__value' }]); + expect(onChange.lastCall.args).to.deep.equal([['sum__value']]); + }); + + it('handles columns being selected', () => { + const { wrapper, onChange } = setup(); + const select = wrapper.find(OnPasteSelect); + select.simulate('change', [valueColumn]); + + const adhocMetric = onChange.lastCall.args[0][0]; + expect(adhocMetric instanceof AdhocMetric).to.be.true; + expect(onChange.lastCall.args).to.deep.equal([[{ + column: valueColumn, + aggregate: AGGREGATES.SUM, + label: 'SUM(value)', + hasCustomLabel: false, + optionName: adhocMetric.optionName, + }]]); + }); + + it('handles aggregates being selected', () => { + const { wrapper, onChange } = setup(); + const select = wrapper.find(OnPasteSelect); + + // mock out the Select ref + const setInputSpy = sinon.spy(); + const handleInputSpy = sinon.spy(); + wrapper.instance().select = { + setInputValue: setInputSpy, + handleInputChange: handleInputSpy, + input: { input: {} }, + } + + select.simulate('change', [{ aggregate_name: 'SUM', optionName: 'SUM' }]); + + expect(setInputSpy.calledWith('SUM()')).to.be.true; + expect(handleInputSpy.calledWith({ target: { value: 'SUM()' }})).to.be.true; + expect(onChange.lastCall.args).to.deep.equal([[]]); + }); + + it('preserves existing selected AdhocMetrics', () => { + const { wrapper, onChange } = setup(); + const select = wrapper.find(OnPasteSelect); + select.simulate('change', [{ metric_name: 'sum__value' }, sumValueAdhocMetric ]); + expect(onChange.lastCall.args).to.deep.equal([['sum__value', sumValueAdhocMetric]]); + }); + }); + + describe('onMetricEdit', () => { + it('accepts an edited metric from an AdhocMetricEditPopover', () => { + const { wrapper, onChange } = setup({ + value: [sumValueAdhocMetric], + }); + + const editedMetric = sumValueAdhocMetric.duplicateWith({ aggregate: AGGREGATES.AVG }); + wrapper.instance().onMetricEdit(editedMetric); + + expect(onChange.lastCall.args).to.deep.equal([[ + editedMetric, + ]]); + }); + }); + + describe('checkIfAggregateInInput', () => { + it('handles an aggregate in the input', () => { + const { wrapper } = setup(); + + expect(wrapper.state('aggregateInInput')).to.be.null; + wrapper.instance().checkIfAggregateInInput('AVG('); + expect(wrapper.state('aggregateInInput')).to.equal(AGGREGATES.AVG); + }); + + it('handles no aggregate in the input', () => { + const { wrapper } = setup(); + + expect(wrapper.state('aggregateInInput')).to.be.null; + wrapper.instance().checkIfAggregateInInput('colu'); + expect(wrapper.state('aggregateInInput')).to.be.null; + }); + }); +}); diff --git a/superset/assets/stylesheets/superset.less b/superset/assets/stylesheets/superset.less index 4ac5ba8ae2c3b..035acceb1f041 100644 --- a/superset/assets/stylesheets/superset.less +++ b/superset/assets/stylesheets/superset.less @@ -449,3 +449,9 @@ g.annotation-container { color: @brand-primary; border-color: @brand-primary; } + +.metric-edit-popover-label-input { + border-radius: 4px; + height: 30px; + padding-left: 10px; +} diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index e2dca32137e45..b5d1ae9734fdc 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -436,6 +436,25 @@ def get_from_clause(self, template_processor=None, db_engine_spec=None): return TextAsFrom(sa.text(from_sql), []).alias('expr_qry') return self.get_sqla_table() + def is_adhoc_metric(self, metric): + return isinstance(metric, dict) and metric['column'] and metric['aggregate'] + + def adhoc_metric_to_sa(self, metric): + if metric['aggregate'] == 'COUNT_DISTINCT': + sa_metric = sa.func.COUNT(sa.distinct(column(metric['column']['column_name']))) + if metric['aggregate'] == 'COUNT': + sa_metric = sa.func.COUNT(column(metric['column']['column_name'])) + if metric['aggregate'] == 'SUM': + sa_metric = sa.func.SUM(column(metric['column']['column_name'])) + if metric['aggregate'] == 'AVG': + sa_metric = sa.func.AVG(column(metric['column']['column_name'])) + if metric['aggregate'] == 'MIN': + sa_metric = sa.func.MIN(column(metric['column']['column_name'])) + if metric['aggregate'] == 'MAX': + sa_metric = sa.func.MAX(column(metric['column']['column_name'])) + sa_metric = sa_metric.label(metric['label']) + return sa_metric + def get_sqla_query( # sqla self, groupby, metrics, @@ -484,10 +503,17 @@ def get_sqla_query( # sqla 'and is required by this type of chart')) if not groupby and not metrics and not columns: raise Exception(_('Empty query?')) + metrics_exprs = [] for m in metrics: - if m not in metrics_dict: + if self.is_adhoc_metric(m): + metrics_exprs.append(self.adhoc_metric_to_sa(m)) + elif m in metrics_dict: + metrics_exprs.append(metrics_dict.get(m).sqla_col) + else: raise Exception(_("Metric '{}' is not valid".format(m))) - metrics_exprs = [metrics_dict.get(m).sqla_col for m in metrics] + print(metrics) + print(metrics_dict) + print(metrics_exprs) if metrics_exprs: main_metric_expr = metrics_exprs[0] else: diff --git a/superset/viz.py b/superset/viz.py index fc874301619c3..6bef4f76577dd 100644 --- a/superset/viz.py +++ b/superset/viz.py @@ -64,7 +64,14 @@ def __init__(self, datasource, form_data, force=False): self.query = '' self.token = self.form_data.get( 'token', 'token_' + uuid.uuid4().hex[:8]) - self.metrics = self.form_data.get('metrics') or [] + metrics = self.form_data.get('metrics') or [] + self.metrics = [] + for metric in metrics: + if isinstance(metric, dict): + self.metrics.append(metric['label']) + else: + self.metrics.append(metric) + self.groupby = self.form_data.get('groupby') or [] self.time_shift = timedelta() From 7419a947a1e74edc013dfe1822cbbc70aeaf0598 Mon Sep 17 00:00:00 2001 From: GabeLoins Date: Mon, 19 Mar 2018 17:51:53 -0700 Subject: [PATCH 2/3] addressing lint issues on new metrics control --- superset/assets/.eslintrc | 3 + .../assets/javascripts/dashboard/reducers.js | 1 + .../assets/javascripts/explore/AdhocMetric.js | 2 +- .../components/AdhocMetricEditPopover.jsx | 42 +++--- .../AdhocMetricEditPopoverTitle.jsx | 16 +-- .../explore/components/AdhocMetricOption.jsx | 11 +- .../components/MetricDefinitionOption.jsx | 2 +- .../components/MetricDefinitionValue.jsx | 2 +- .../components/controls/MetricsControl.jsx | 123 ++++++++++-------- superset/assets/package.json | 2 +- .../javascripts/explore/AdhocMetric_spec.js | 14 +- .../AdhocMetricEditPopoverTitle_spec.jsx | 8 +- .../AdhocMetricEditPopover_spec.jsx | 12 +- .../components/AdhocMetricOption_spec.jsx | 2 +- .../components/AggregateOption_spec.jsx | 1 - .../MetricDefinitionOption_spec.jsx | 7 +- .../components/MetricDefinitionValue_spec.jsx | 7 +- .../components/MetricsControl_spec.jsx | 10 +- 18 files changed, 146 insertions(+), 119 deletions(-) diff --git a/superset/assets/.eslintrc b/superset/assets/.eslintrc index c8b076604cfe6..921e927b82596 100644 --- a/superset/assets/.eslintrc +++ b/superset/assets/.eslintrc @@ -38,5 +38,8 @@ "react/no-unescaped-entities": 0, "react/no-unused-prop-types": 0, "react/no-string-refs": 0, + "indent": 0, + "no-multi-spaces": 0, + "padded-blocks": 0, } } diff --git a/superset/assets/javascripts/dashboard/reducers.js b/superset/assets/javascripts/dashboard/reducers.js index bf42532edb51a..1cc3e76cd8586 100644 --- a/superset/assets/javascripts/dashboard/reducers.js +++ b/superset/assets/javascripts/dashboard/reducers.js @@ -1,3 +1,4 @@ +/* eslint-disable camelcase */ import { combineReducers } from 'redux'; import d3 from 'd3'; import shortid from 'shortid'; diff --git a/superset/assets/javascripts/explore/AdhocMetric.js b/superset/assets/javascripts/explore/AdhocMetric.js index 10aff628f9a5f..15202d8cde439 100644 --- a/superset/assets/javascripts/explore/AdhocMetric.js +++ b/superset/assets/javascripts/explore/AdhocMetric.js @@ -15,7 +15,7 @@ export default class AdhocMetric { } getDefaultLabel() { - return `${this.aggregate || ''}(${(this.column && this.column.column_name) || ''})` + return `${this.aggregate || ''}(${(this.column && this.column.column_name) || ''})`; } duplicateWith(nextFields) { diff --git a/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx b/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx index f6320876cdcfa..86f97d012a98d 100644 --- a/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx +++ b/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx @@ -33,10 +33,11 @@ export default class AdhocMetricEditPopover extends React.Component { this.state = { adhocMetric: this.props.adhocMetric }; this.selectProps = { multi: false, - name: "select-column", + name: 'select-column', labelKey: 'label', autosize: false, clearable: true, + selectWrap: VirtualizedSelect, }; } @@ -50,14 +51,20 @@ export default class AdhocMetricEditPopover extends React.Component { } onAggregateChange(aggregate) { - // we construct this object explicitly because we still want to overwrite in the case aggregate is null - this.setState({ - adhocMetric: this.state.adhocMetric.duplicateWith({ aggregate: aggregate && aggregate.aggregate }), + // we construct this object explicitly to overwrite the value in the case aggregate is null + this.setState({ + adhocMetric: this.state.adhocMetric.duplicateWith({ + aggregate: aggregate && aggregate.aggregate, + }), }); } onLabelChange(e) { - this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({ label: e.target.value, hasCustomLabel: true })}); + this.setState({ + adhocMetric: this.state.adhocMetric.duplicateWith({ + label: e.target.value, hasCustomLabel: true, + }), + }); } render() { @@ -68,25 +75,28 @@ export default class AdhocMetricEditPopover extends React.Component { options: columns, value: this.state.adhocMetric.column && this.state.adhocMetric.column.column_name, onChange: this.onColumnChange, - optionRenderer: VirtualizedRendererWrap((option) => ( + optionRenderer: VirtualizedRendererWrap(option => ( )), - valueRenderer: (column) => column.column_name, + valueRenderer: column => column.column_name, valueKey: 'column_name', }; const aggregateSelectProps = { placeholder: t('%s aggregates(s)', Object.keys(AGGREGATES).length), - options: Object.keys(AGGREGATES).map((aggregate) => ({ aggregate })), + options: Object.keys(AGGREGATES).map(aggregate => ({ aggregate })), value: this.state.adhocMetric.aggregate, onChange: this.onAggregateChange, - optionRenderer: VirtualizedRendererWrap((aggregate) => aggregate.aggregate), - valueRenderer: (aggregate) => aggregate.aggregate, + optionRenderer: VirtualizedRendererWrap(aggregate => aggregate.aggregate), + valueRenderer: aggregate => aggregate.aggregate, valueKey: 'aggregate', }; const popoverTitle = ( - + ); const stateIsValid = this.state.adhocMetric.column && this.state.adhocMetric.aggregate; @@ -100,23 +110,23 @@ export default class AdhocMetricEditPopover extends React.Component { > column - + aggregate - + - - ) + ); } } AdhocMetricEditPopover.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx b/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx index 79ac7359cfbb1..2b01e9dd59934 100644 --- a/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx +++ b/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx @@ -26,10 +26,10 @@ export default class AdhocMetricEditPopoverTitle extends React.Component { this.setState({ isHovered: true }) } - onMouseOut={() => this.setState({ isHovered: false }) } - onClick={() => this.setState({ isEditable: true }) } - onBlur={() => this.setState({ isEditable: false }) } + onMouseOver={() => this.setState({ isHovered: true })} + onMouseOut={() => this.setState({ isHovered: false })} + onClick={() => this.setState({ isEditable: true })} + onBlur={() => this.setState({ isEditable: false })} > {this.state.isEditable ? ref && ref.focus()} + onChange={onChange} + inputRef={ref => ref && ref.focus()} /> : {adhocMetric.hasCustomLabel ? adhocMetric.label : 'My Metric'}   - + } - ) + ); } } AdhocMetricEditPopoverTitle.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx b/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx index c48613212c8b8..52350c9fb1485 100644 --- a/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx +++ b/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx @@ -1,7 +1,6 @@ import React from 'react'; -import ReactDOM from 'react-dom'; import PropTypes from 'prop-types'; -import { FormControl, Label, OverlayTrigger, Popover } from 'react-bootstrap'; +import { Label, OverlayTrigger } from 'react-bootstrap'; import AdhocMetricEditPopover from './AdhocMetricEditPopover'; import AdhocMetric from '../AdhocMetric'; @@ -26,16 +25,16 @@ export default class AdhocMetricOption extends React.PureComponent { render() { const { adhocMetric } = this.props; const overlay = ( - ); return ( -
{e.stopPropagation()}} > +
{ e.stopPropagation(); }} > ); } - console.error("You must supply either a saved metric, column or aggregate to MetricDefinitionOption"); + notify.error('You must supply either a saved metric, column or aggregate to MetricDefinitionOption'); return null; } MetricDefinitionOption.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx b/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx index 2b0bdcab66af8..6b0b34bc10201 100644 --- a/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx +++ b/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx @@ -22,7 +22,7 @@ export default function MetricDefinitionValue({ option, onMetricEdit, columns }) ); } - console.error("You must supply either a saved metric or adhoc metric to MetricDefinitionValue"); + notify.error('You must supply either a saved metric or adhoc metric to MetricDefinitionValue'); return null; } MetricDefinitionValue.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx b/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx index 067373efd09c3..4084d2321aa28 100644 --- a/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx +++ b/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx @@ -1,7 +1,6 @@ import React from 'react'; import PropTypes from 'prop-types'; import VirtualizedSelect from 'react-virtualized-select'; -import Select, { Creatable } from 'react-select'; import ControlHeader from '../ControlHeader'; import { t } from '../../../locales'; import VirtualizedRendererWrap from '../../../components/VirtualizedRendererWrap'; @@ -16,9 +15,9 @@ import { AGGREGATES } from '../../constants'; const propTypes = { multi: PropTypes.bool, - name: PropTypes.string.isRequired, + name: PropTypes.string.isRequired, onChange: PropTypes.func, - value: PropTypes.arrayOf(PropTypes.oneOfType([ PropTypes.string, adhocMetricType, ])), + value: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, adhocMetricType])), columns: PropTypes.arrayOf(columnType), savedMetrics: PropTypes.arrayOf(savedMetricType), }; @@ -31,18 +30,18 @@ function isDictionaryForAdhocMetric(value) { return value && !(value instanceof AdhocMetric) && value.column && value.aggregate && value.label; } -// adhoc metrics are stored as dictionaries in URL params. When we reload the page we want to convert them -// back into the AdhocMetric class for typechecking, consistency and instance method access. +// adhoc metrics are stored as dictionaries in URL params. We convert them back into the +// AdhocMetric class for typechecking, consistency and instance method access. function coerceAdhocMetrics(values) { if (!values) { return []; } - return values.map(value => { + return values.map((value) => { if (isDictionaryForAdhocMetric(value)) { return new AdhocMetric(value); - } else { - return value } + return value; + }); } @@ -61,6 +60,7 @@ function getDefaultAggregateForColumn(column) { } else if (type.match(/unknown/i)) { return AGGREGATES.COUNT; } + return null; } function selectFilterOption(option, filterValue) { @@ -86,35 +86,19 @@ export default class MetricsControl extends React.PureComponent { }; } - componentDidUpdate(previousProps) { - if ( this.props.columns !== previousProps.columns || this.props.savedMetrics !== previousProps.savedMetrics) { - console.log('updating options for select'); - this.setState({ options: this.optionsForSelect() }); + componentWillReceiveProps(nextProps) { + if ( + this.props.columns !== nextProps.columns || + this.props.savedMetrics !== nextProps.savedMetrics + ) { + this.setState({ options: this.optionsForSelect(nextProps) }); + this.props.onChange([]); } - if (this.props.value !== previousProps.value) { - console.log('coercing adhocMetrics'); - this.setState({ value: coerceAdhocMetrics(this.props.value) }); + if (this.props.value !== nextProps.value) { + this.setState({ value: coerceAdhocMetrics(nextProps.value) }); } } - optionsForSelect() { - const options = [ - ...this.props.columns, - ...Object.keys(AGGREGATES).map(aggregate => ({ aggregate_name: aggregate })), - ...this.props.savedMetrics, - ]; - - return options.map((option) => { - if (option.metric_name) { - return { ...option, optionName: option.metric_name }; - } else if (option.column_name) { - return { ...option, optionName: '_col_' + option.column_name }; - } else if (option.aggregate_name) { - return { ...option, optionName: '_aggregate_' + option.aggregate_name }; - } - }); - } - onMetricEdit(changedMetric) { this.props.onChange(this.state.value.map((value) => { if (value.optionName === changedMetric.optionName) { @@ -124,17 +108,6 @@ export default class MetricsControl extends React.PureComponent { })); } - checkIfAggregateInInput(input) { - let nextState = { aggregateInInput: null }; - Object.keys(AGGREGATES).forEach(aggregate => { - if (input.toLowerCase().startsWith(aggregate.toLowerCase() + '(')) { - nextState = { aggregateInInput: aggregate }; - } - }); - this.clearedAggregateInInput = this.state.aggregateInInput; - this.setState(nextState); - } - onChange(opts) { const optionValues = opts.map((option) => { if (option.metric_name) { @@ -142,8 +115,8 @@ export default class MetricsControl extends React.PureComponent { } else if (option.column_name) { const clearedAggregate = this.clearedAggregateInInput; this.clearedAggregateInInput = null; - return new AdhocMetric({ - column: option, + return new AdhocMetric({ + column: option, aggregate: clearedAggregate || getDefaultAggregateForColumn(option), }); } else if (option instanceof AdhocMetric) { @@ -151,37 +124,72 @@ export default class MetricsControl extends React.PureComponent { } else if (option.aggregate_name) { const newValue = `${option.aggregate_name}()`; this.select.setInputValue(newValue); - this.select.handleInputChange({ target: { value: newValue }}); - // we need to set a timeout here or the selectionWill be overwritten by some browsers (e.g. Chrome) + this.select.handleInputChange({ target: { value: newValue } }); + // we need to set a timeout here or the selectionWill be overwritten + // by some browsers (e.g. Chrome) setTimeout(() => { this.select.input.input.selectionStart = newValue.length - 1; this.select.input.input.selectionEnd = newValue.length - 1; }, 0); + return null; } + return null; }).filter(option => option); this.props.onChange(optionValues); } + checkIfAggregateInInput(input) { + let nextState = { aggregateInInput: null }; + Object.keys(AGGREGATES).forEach((aggregate) => { + if (input.toLowerCase().startsWith(aggregate.toLowerCase() + '(')) { + nextState = { aggregateInInput: aggregate }; + } + }); + this.clearedAggregateInInput = this.state.aggregateInInput; + this.setState(nextState); + } + + + optionsForSelect(props) { + const options = [ + ...props.columns, + ...Object.keys(AGGREGATES).map(aggregate => ({ aggregate_name: aggregate })), + ...props.savedMetrics, + ]; + + return options.map((option) => { + if (option.metric_name) { + return { ...option, optionName: option.metric_name }; + } else if (option.column_name) { + return { ...option, optionName: '_col_' + option.column_name }; + } else if (option.aggregate_name) { + return { ...option, optionName: '_aggregate_' + option.aggregate_name }; + } + notify.error(`provided invalid option to MetricsControl, ${option}`); + return null; + }); + } + render() { // TODO figure out why the dropdown isnt appearing as soon as a metric is selected return (
- ( + optionRenderer={VirtualizedRendererWrap(option => ( ))} - valueRenderer={(option) => ( + valueRenderer={option => ( ref && (this.select = ref._selectRef)} - selectWrap={VirtualizedSelect} + refFunc={(ref) => { + if (ref) { + // eslint-disable-next-line no-underscore-dangle + this.select = ref._selectRef; + } + }} + selectWrap={VirtualizedSelect} />
); diff --git a/superset/assets/package.json b/superset/assets/package.json index 039c21bd62d93..5bf03c037c5ad 100644 --- a/superset/assets/package.json +++ b/superset/assets/package.json @@ -127,7 +127,7 @@ "clean-webpack-plugin": "^0.1.16", "css-loader": "^0.28.0", "enzyme": "^2.0.0", - "eslint": "^3.19.0", + "eslint": "^4.19.0", "eslint-config-airbnb": "^15.0.1", "eslint-plugin-import": "^2.2.0", "eslint-plugin-jsx-a11y": "^5.1.1", diff --git a/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js b/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js index 3a3e9de273c2c..37a84ce1ae439 100644 --- a/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js +++ b/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js @@ -20,7 +20,7 @@ describe('AdhocMetric', () => { hasCustomLabel: false, optionName: adhocMetric.optionName, }); - }) + }); it('can create altered duplicates', () => { const adhocMetric1 = new AdhocMetric({ @@ -34,7 +34,7 @@ describe('AdhocMetric', () => { expect(adhocMetric1.aggregate).to.equal(AGGREGATES.SUM); expect(adhocMetric2.aggregate).to.equal(AGGREGATES.AVG); - }) + }); it('can verify equality', () => { const adhocMetric1 = new AdhocMetric({ @@ -43,8 +43,9 @@ describe('AdhocMetric', () => { }); const adhocMetric2 = adhocMetric1.duplicateWith({}); + // eslint-disable-next-line no-unused-expressions expect(adhocMetric1.equals(adhocMetric2)).to.be.true; - }) + }); it('can verify inequality', () => { const adhocMetric1 = new AdhocMetric({ @@ -55,8 +56,9 @@ describe('AdhocMetric', () => { }); const adhocMetric2 = adhocMetric1.duplicateWith({ label: 'new label' }); + // eslint-disable-next-line no-unused-expressions expect(adhocMetric1.equals(adhocMetric2)).to.be.false; - }) + }); it('updates label if hasCustomLabel is false', () => { const adhocMetric1 = new AdhocMetric({ @@ -66,7 +68,7 @@ describe('AdhocMetric', () => { const adhocMetric2 = adhocMetric1.duplicateWith({ aggregate: AGGREGATES.AVG }); expect(adhocMetric2.label).to.equal('AVG(value)'); - }) + }); it('keeps label if hasCustomLabel is true', () => { const adhocMetric1 = new AdhocMetric({ @@ -78,5 +80,5 @@ describe('AdhocMetric', () => { const adhocMetric2 = adhocMetric1.duplicateWith({ aggregate: AGGREGATES.AVG }); expect(adhocMetric2.label).to.equal('label1'); - }) + }); }); diff --git a/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx index 517186829f530..b732a5a78fc65 100644 --- a/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx @@ -4,7 +4,7 @@ import sinon from 'sinon'; import { expect } from 'chai'; import { describe, it } from 'mocha'; import { shallow } from 'enzyme'; -import { FormControl, OverlayTrigger } from 'react-bootstrap'; +import { OverlayTrigger } from 'react-bootstrap'; import AdhocMetric from '../../../../javascripts/explore/AdhocMetric'; import AdhocMetricEditPopoverTitle from '../../../../javascripts/explore/components/AdhocMetricEditPopoverTitle'; @@ -27,7 +27,7 @@ function setup(overrides) { adhocMetric: sumValueAdhocMetric, onChange, ...overrides, - } + }; const wrapper = shallow(); return { wrapper, onChange }; } @@ -36,13 +36,13 @@ describe('AdhocMetricEditPopoverTitle', () => { it('renders an OverlayTrigger wrapper with the title', () => { const { wrapper } = setup(); expect(wrapper.find(OverlayTrigger)).to.have.lengthOf(1); - expect(wrapper.find(OverlayTrigger).dive().text()).to.equal('My Metric\xa0') + expect(wrapper.find(OverlayTrigger).dive().text()).to.equal('My Metric\xa0'); }); it('transfers to edit mode when clicked', () => { const { wrapper } = setup(); expect(wrapper.state('isEditable')).to.be.false; - wrapper.simulate('click') + wrapper.simulate('click'); expect(wrapper.state('isEditable')).to.be.true; }); }); diff --git a/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx index ecabb46ee3ac7..0ba69fba0e68b 100644 --- a/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx @@ -30,7 +30,7 @@ function setup(overrides) { onClose, columns, ...overrides, - } + }; const wrapper = shallow(); return { wrapper, onChange, onClose }; } @@ -57,21 +57,21 @@ describe('AdhocMetricEditPopover', () => { it('overwrites the adhocMetric in state with onLabelChange', () => { const { wrapper } = setup(); - wrapper.instance().onLabelChange({ target: { value: 'new label' }}); + wrapper.instance().onLabelChange({ target: { value: 'new label' } }); expect(wrapper.state('adhocMetric').label).to.equal('new label'); expect(wrapper.state('adhocMetric').hasCustomLabel).to.be.true; }); it('returns to default labels when the custom label is cleared', () => { const { wrapper } = setup(); - wrapper.instance().onLabelChange({ target: { value: 'new label' }}); - wrapper.instance().onLabelChange({ target: { value: '' }}); + wrapper.instance().onLabelChange({ target: { value: 'new label' } }); + wrapper.instance().onLabelChange({ target: { value: '' } }); expect(wrapper.state('adhocMetric').label).to.equal('SUM(value)'); expect(wrapper.state('adhocMetric').hasCustomLabel).to.be.false; }); it('prevents saving if no column or aggregate is chosen', () => { - const { wrapper, onChange, onClose } = setup(); + const { wrapper } = setup(); expect(wrapper.find(Button).find({ disabled: true })).to.have.lengthOf(0); wrapper.instance().onColumnChange(null); expect(wrapper.find(Button).find({ disabled: true })).to.have.lengthOf(1); @@ -82,7 +82,7 @@ describe('AdhocMetricEditPopover', () => { }); it('highlights save if changes are present', () => { - const { wrapper, onChange, onClose } = setup(); + const { wrapper } = setup(); expect(wrapper.find(Button).find({ bsStyle: 'primary' })).to.have.lengthOf(0); wrapper.instance().onColumnChange({ column: columns[1] }); expect(wrapper.find(Button).find({ bsStyle: 'primary' })).to.have.lengthOf(1); diff --git a/superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx b/superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx index c39d515841b48..ac3682598414d 100644 --- a/superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx @@ -28,7 +28,7 @@ function setup(overrides) { onMetricEdit, columns, ...overrides, - } + }; const wrapper = shallow(); return { wrapper, onMetricEdit }; } diff --git a/superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx b/superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx index eec97e99b53a0..a1fb317969c37 100644 --- a/superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/AggregateOption_spec.jsx @@ -1,6 +1,5 @@ /* eslint-disable no-unused-expressions */ import React from 'react'; -import sinon from 'sinon'; import { expect } from 'chai'; import { describe, it } from 'mocha'; import { shallow } from 'enzyme'; diff --git a/superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx b/superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx index 0e8207a685341..e39c225b6a0c1 100644 --- a/superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/MetricDefinitionOption_spec.jsx @@ -1,6 +1,5 @@ /* eslint-disable no-unused-expressions */ import React from 'react'; -import sinon from 'sinon'; import { expect } from 'chai'; import { describe, it } from 'mocha'; import { shallow } from 'enzyme'; @@ -12,17 +11,17 @@ import AggregateOption from '../../../../javascripts/explore/components/Aggregat describe('MetricDefinitionOption', () => { it('renders a MetricOption given a saved metric', () => { - const wrapper = shallow(); + const wrapper = shallow(); expect(wrapper.find(MetricOption)).to.have.lengthOf(1); }); it('renders a ColumnOption given a column', () => { - const wrapper = shallow(); + const wrapper = shallow(); expect(wrapper.find(ColumnOption)).to.have.lengthOf(1); }); it('renders an AggregateOption given an aggregate metric', () => { - const wrapper = shallow(); + const wrapper = shallow(); expect(wrapper.find(AggregateOption)).to.have.lengthOf(1); }); }); diff --git a/superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx b/superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx index 556bd9b5b47e1..5690c21010664 100644 --- a/superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx @@ -1,6 +1,5 @@ /* eslint-disable no-unused-expressions */ import React from 'react'; -import sinon from 'sinon'; import { expect } from 'chai'; import { describe, it } from 'mocha'; import { shallow } from 'enzyme'; @@ -18,12 +17,14 @@ const sumValueAdhocMetric = new AdhocMetric({ describe('MetricDefinitionValue', () => { it('renders a MetricOption given a saved metric', () => { - const wrapper = shallow(); + const wrapper = shallow(); expect(wrapper.find(MetricOption)).to.have.lengthOf(1); }); it('renders an AdhocMetricOption given an adhoc metric', () => { - const wrapper = shallow( {}} option={sumValueAdhocMetric} />); + const wrapper = shallow(( + {}} option={sumValueAdhocMetric} /> + )); expect(wrapper.find(AdhocMetricOption)).to.have.lengthOf(1); }); }); diff --git a/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx index f710de8c385d7..1c4ce0142fb53 100644 --- a/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx @@ -31,7 +31,7 @@ function setup(overrides) { onChange, ...defaultProps, ...overrides, - } + }; const wrapper = shallow(); return { wrapper, onChange }; } @@ -60,7 +60,7 @@ describe('MetricsControl', () => { { optionName: '_col_target', type: 'VARCHAR(255)', column_name: 'target' }, { optionName: '_col_value', type: 'DOUBLE', column_name: 'value' }, ...Object.keys(AGGREGATES).map( - aggregate => ({ aggregate_name: aggregate, optionName: '_aggregate_' + aggregate }) + aggregate => ({ aggregate_name: aggregate, optionName: '_aggregate_' + aggregate }), ), { optionName: 'sum__value', metric_name: 'sum__value', expression: 'SUM(energy_usage.value)' }, { optionName: 'avg__value', metric_name: 'avg__value', expression: 'AVG(energy_usage.value)' }, @@ -132,19 +132,19 @@ describe('MetricsControl', () => { setInputValue: setInputSpy, handleInputChange: handleInputSpy, input: { input: {} }, - } + }; select.simulate('change', [{ aggregate_name: 'SUM', optionName: 'SUM' }]); expect(setInputSpy.calledWith('SUM()')).to.be.true; - expect(handleInputSpy.calledWith({ target: { value: 'SUM()' }})).to.be.true; + expect(handleInputSpy.calledWith({ target: { value: 'SUM()' } })).to.be.true; expect(onChange.lastCall.args).to.deep.equal([[]]); }); it('preserves existing selected AdhocMetrics', () => { const { wrapper, onChange } = setup(); const select = wrapper.find(OnPasteSelect); - select.simulate('change', [{ metric_name: 'sum__value' }, sumValueAdhocMetric ]); + select.simulate('change', [{ metric_name: 'sum__value' }, sumValueAdhocMetric]); expect(onChange.lastCall.args).to.deep.equal([['sum__value', sumValueAdhocMetric]]); }); }); From 7d593858d7ce1539facf61eeac101c27eadef350 Mon Sep 17 00:00:00 2001 From: GabeLoins Date: Wed, 21 Mar 2018 16:42:40 -0700 Subject: [PATCH 3/3] enabling druid --- .../components/ColumnTypeLabel.jsx | 6 +- .../assets/javascripts/explore/AdhocMetric.js | 9 +- .../components/AdhocMetricEditPopover.jsx | 16 ++- .../AdhocMetricEditPopoverTitle.jsx | 36 +++++- .../explore/components/AdhocMetricOption.jsx | 30 +++-- .../explore/components/AggregateOption.jsx | 4 +- .../components/MetricDefinitionOption.jsx | 9 +- .../components/MetricDefinitionValue.jsx | 25 +++- .../components/controls/MetricsControl.jsx | 119 ++++++++++++------ superset/assets/javascripts/explore/main.css | 4 - .../explore/propTypes/aggregateOptionType.js | 5 + .../explore/propTypes/columnType.js | 2 +- .../explore/propTypes/legacyMetricType.js | 6 - .../javascripts/explore/stores/controls.jsx | 13 +- .../javascripts/explore/AdhocMetric_spec.js | 1 + .../components/MetricsControl_spec.jsx | 68 +++++++++- superset/connectors/druid/models.py | 44 +++++-- superset/connectors/sqla/models.py | 33 ++--- superset/utils.py | 14 ++- superset/viz.py | 4 +- tests/druid_func_tests.py | 114 +++++++++++++++-- 21 files changed, 426 insertions(+), 136 deletions(-) create mode 100644 superset/assets/javascripts/explore/propTypes/aggregateOptionType.js delete mode 100644 superset/assets/javascripts/explore/propTypes/legacyMetricType.js diff --git a/superset/assets/javascripts/components/ColumnTypeLabel.jsx b/superset/assets/javascripts/components/ColumnTypeLabel.jsx index 9037d2f05cc4a..33f831912038a 100644 --- a/superset/assets/javascripts/components/ColumnTypeLabel.jsx +++ b/superset/assets/javascripts/components/ColumnTypeLabel.jsx @@ -2,12 +2,14 @@ import React from 'react'; import PropTypes from 'prop-types'; const propTypes = { - type: PropTypes.string.isRequired, + type: PropTypes.string, }; export default function ColumnTypeLabel({ type }) { let stringIcon = ''; - if (type === '' || type === 'expression') { + if (typeof type !== 'string') { + stringIcon = '?'; + } else if (type === '' || type === 'expression') { stringIcon = 'ƒ'; } else if (type === 'aggregate') { stringIcon = 'AGG'; diff --git a/superset/assets/javascripts/explore/AdhocMetric.js b/superset/assets/javascripts/explore/AdhocMetric.js index 15202d8cde439..e123521b4c722 100644 --- a/superset/assets/javascripts/explore/AdhocMetric.js +++ b/superset/assets/javascripts/explore/AdhocMetric.js @@ -3,12 +3,8 @@ export default class AdhocMetric { this.column = adhocMetric.column; this.aggregate = adhocMetric.aggregate; this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label); - - if (this.hasCustomLabel) { - this.label = adhocMetric.label; - } else { - this.label = this.getDefaultLabel(); - } + this.fromFormData = !!adhocMetric.optionName; + this.label = this.hasCustomLabel ? adhocMetric.label : this.getDefaultLabel(); this.optionName = adhocMetric.optionName || `metric_${Math.random().toString(36).substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`; @@ -34,4 +30,3 @@ export default class AdhocMetric { ); } } - diff --git a/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx b/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx index 86f97d012a98d..0964a51c5dd43 100644 --- a/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx +++ b/superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx @@ -17,6 +17,7 @@ const propTypes = { onChange: PropTypes.func.isRequired, onClose: PropTypes.func.isRequired, columns: PropTypes.arrayOf(columnType), + datasourceType: PropTypes.string, }; const defaultProps = { @@ -68,7 +69,7 @@ export default class AdhocMetricEditPopover extends React.Component { } render() { - const { adhocMetric, columns, onChange, onClose, ...rest } = this.props; + const { adhocMetric, columns, onChange, onClose, datasourceType, ...popoverProps } = this.props; const columnSelectProps = { placeholder: t('%s column(s)', columns.length), @@ -92,6 +93,12 @@ export default class AdhocMetricEditPopover extends React.Component { valueKey: 'aggregate', }; + if (this.props.datasourceType === 'druid') { + aggregateSelectProps.options = aggregateSelectProps.options.filter(( + option => option.aggregate !== 'AVG' + )); + } + const popoverTitle = ( column @@ -119,12 +126,13 @@ export default class AdhocMetricEditPopover extends React.Component { - + ); } diff --git a/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx b/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx index 2b01e9dd59934..d14b1111897b0 100644 --- a/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx +++ b/superset/assets/javascripts/explore/components/AdhocMetricEditPopoverTitle.jsx @@ -11,12 +11,38 @@ const propTypes = { export default class AdhocMetricEditPopoverTitle extends React.Component { constructor(props) { super(props); + this.onMouseOver = this.onMouseOver.bind(this); + this.onMouseOut = this.onMouseOut.bind(this); + this.onClick = this.onClick.bind(this); + this.onBlur = this.onBlur.bind(this); this.state = { isHovered: false, isEditable: false, }; } + onMouseOver() { + this.setState({ isHovered: true }); + } + + onMouseOut() { + this.setState({ isHovered: false }); + } + + onClick() { + this.setState({ isEditable: true }); + } + + onBlur() { + this.setState({ isEditable: false }); + } + + refFunc(ref) { + if (ref) { + ref.focus(); + } + } + render() { const { adhocMetric, onChange } = this.props; @@ -26,10 +52,10 @@ export default class AdhocMetricEditPopoverTitle extends React.Component { this.setState({ isHovered: true })} - onMouseOut={() => this.setState({ isHovered: false })} - onClick={() => this.setState({ isEditable: true })} - onBlur={() => this.setState({ isEditable: false })} + onMouseOver={this.onMouseOver} + onMouseOut={this.onMouseOut} + onClick={this.onClick} + onBlur={this.onBlur} > {this.state.isEditable ? ref && ref.focus()} + inputRef={this.refFunc} /> : {adhocMetric.hasCustomLabel ? adhocMetric.label : 'My Metric'} diff --git a/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx b/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx index 52350c9fb1485..88dd0d7ee1877 100644 --- a/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx +++ b/superset/assets/javascripts/explore/components/AdhocMetricOption.jsx @@ -10,6 +10,8 @@ const propTypes = { adhocMetric: PropTypes.instanceOf(AdhocMetric), onMetricEdit: PropTypes.func.isRequired, columns: PropTypes.arrayOf(columnType), + multi: PropTypes.bool, + datasourceType: PropTypes.string, }; export default class AdhocMetricOption extends React.PureComponent { @@ -30,26 +32,28 @@ export default class AdhocMetricOption extends React.PureComponent { onChange={this.props.onMetricEdit} onClose={this.closeMetricEditOverlay} columns={this.props.columns} + datasourceType={this.props.datasourceType} /> ); return ( -
{ e.stopPropagation(); }} > - -
+ + ); } } diff --git a/superset/assets/javascripts/explore/components/AggregateOption.jsx b/superset/assets/javascripts/explore/components/AggregateOption.jsx index 993c2b4b8a0f9..e56ccf9dbf46f 100644 --- a/superset/assets/javascripts/explore/components/AggregateOption.jsx +++ b/superset/assets/javascripts/explore/components/AggregateOption.jsx @@ -1,9 +1,11 @@ import React from 'react'; import PropTypes from 'prop-types'; + import ColumnTypeLabel from '../../components/ColumnTypeLabel'; +import aggregateOptionType from '../propTypes/aggregateOptionType'; const propTypes = { - aggregate: PropTypes.object.isRequired, + aggregate: aggregateOptionType, showType: PropTypes.bool, }; diff --git a/superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx b/superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx index 566096282611f..c275b9c4d571e 100644 --- a/superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx +++ b/superset/assets/javascripts/explore/components/MetricDefinitionOption.jsx @@ -4,9 +4,16 @@ import PropTypes from 'prop-types'; import MetricOption from '../../components/MetricOption'; import ColumnOption from '../../components/ColumnOption'; import AggregateOption from './AggregateOption'; +import columnType from '../propTypes/columnType'; +import savedMetricType from '../propTypes/savedMetricType'; +import aggregateOptionType from '../propTypes/aggregateOptionType'; const propTypes = { - option: PropTypes.object.isRequired, + option: PropTypes.oneOfType([ + columnType, + savedMetricType, + aggregateOptionType, + ]).isRequired, }; export default function MetricDefinitionOption({ option }) { diff --git a/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx b/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx index 6b0b34bc10201..4997dcee3bcb8 100644 --- a/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx +++ b/superset/assets/javascripts/explore/components/MetricDefinitionValue.jsx @@ -5,21 +5,40 @@ import AdhocMetricOption from './AdhocMetricOption'; import AdhocMetric from '../AdhocMetric'; import columnType from '../propTypes/columnType'; import MetricOption from '../../components/MetricOption'; +import savedMetricType from '../propTypes/savedMetricType'; +import adhocMetricType from '../propTypes/adhocMetricType'; const propTypes = { - option: PropTypes.object.isRequired, + option: PropTypes.oneOfType([ + savedMetricType, + adhocMetricType, + ]).isRequired, onMetricEdit: PropTypes.func, columns: PropTypes.arrayOf(columnType), + multi: PropTypes.bool, + datasourceType: PropTypes.string, }; -export default function MetricDefinitionValue({ option, onMetricEdit, columns }) { +export default function MetricDefinitionValue({ + option, + onMetricEdit, + columns, + multi, + datasourceType, +}) { if (option.metric_name) { return ( ); } else if (option instanceof AdhocMetric) { return ( - + ); } notify.error('You must supply either a saved metric or adhoc metric to MetricDefinitionValue'); diff --git a/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx b/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx index 4084d2321aa28..9a16f525a0ec5 100644 --- a/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx +++ b/superset/assets/javascripts/explore/components/controls/MetricsControl.jsx @@ -14,12 +14,16 @@ import adhocMetricType from '../../propTypes/adhocMetricType'; import { AGGREGATES } from '../../constants'; const propTypes = { - multi: PropTypes.bool, name: PropTypes.string.isRequired, onChange: PropTypes.func, - value: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, adhocMetricType])), + value: PropTypes.oneOfType([ + PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, adhocMetricType])), + PropTypes.oneOfType([PropTypes.string, adhocMetricType]), + ]), columns: PropTypes.arrayOf(columnType), savedMetrics: PropTypes.arrayOf(savedMetricType), + multi: PropTypes.bool, + datasourceType: PropTypes.string, }; const defaultProps = { @@ -32,22 +36,29 @@ function isDictionaryForAdhocMetric(value) { // adhoc metrics are stored as dictionaries in URL params. We convert them back into the // AdhocMetric class for typechecking, consistency and instance method access. -function coerceAdhocMetrics(values) { - if (!values) { +function coerceAdhocMetrics(value) { + if (!value) { return []; } - return values.map((value) => { + if (!Array.isArray(value)) { if (isDictionaryForAdhocMetric(value)) { - return new AdhocMetric(value); + return [new AdhocMetric(value)]; } - return value; - + return [value]; + } + return value.map((val) => { + if (isDictionaryForAdhocMetric(val)) { + return new AdhocMetric(val); + } + return val; }); } function getDefaultAggregateForColumn(column) { const type = column.type; - if (type === '' || type === 'expression') { + if (typeof type !== 'string') { + return AGGREGATES.COUNT; + } else if (type === '' || type === 'expression') { return AGGREGATES.SUM; } else if (type.match(/.*char.*/i) || type.match(/string.*/i) || type.match(/.*text.*/i)) { return AGGREGATES.COUNT_DISTINCT; @@ -63,13 +74,12 @@ function getDefaultAggregateForColumn(column) { return null; } -function selectFilterOption(option, filterValue) { - let endIndex = filterValue.length; - if (filterValue.endsWith(')')) { - endIndex = filterValue.length - 1; - } - const valueAfterAggregate = filterValue.substring(filterValue.indexOf('(') + 1, endIndex); - return option.column_name && (option.column_name.indexOf(valueAfterAggregate) >= 0); +const autoGeneratedMetricRegex = /^(LONG|DOUBLE|FLOAT)?(SUM|AVG|MAX|MIN|COUNT)\([A-Z_][A-Z0-9_]*\)$/i; +function isAutoGeneratedMetric(savedMetric) { + return ( + autoGeneratedMetricRegex.test(savedMetric.expression) || + autoGeneratedMetricRegex.test(savedMetric.verbose_name) + ); } export default class MetricsControl extends React.PureComponent { @@ -79,6 +89,25 @@ export default class MetricsControl extends React.PureComponent { this.onMetricEdit = this.onMetricEdit.bind(this); this.checkIfAggregateInInput = this.checkIfAggregateInInput.bind(this); this.optionsForSelect = this.optionsForSelect.bind(this); + this.selectFilterOption = this.selectFilterOption.bind(this); + this.optionRenderer = VirtualizedRendererWrap(option => ( + + ), { ignoreAutogeneratedMetrics: true }); + this.valueRenderer = option => ( + + ); + this.refFunc = (ref) => { + if (ref) { + // eslint-disable-next-line no-underscore-dangle + this.select = ref._selectRef; + } + }; this.state = { aggregateInInput: null, options: this.optionsForSelect(this.props), @@ -100,16 +129,24 @@ export default class MetricsControl extends React.PureComponent { } onMetricEdit(changedMetric) { - this.props.onChange(this.state.value.map((value) => { + let newValue = this.state.value.map((value) => { if (value.optionName === changedMetric.optionName) { return changedMetric; } return value; - })); + }); + if (!this.props.multi) { + newValue = newValue[0]; + } + this.props.onChange(newValue); } onChange(opts) { - const optionValues = opts.map((option) => { + let transformedOpts = opts; + if (!this.props.multi) { + transformedOpts = [opts].filter(option => option); + } + let optionValues = transformedOpts.map((option) => { if (option.metric_name) { return option.metric_name; } else if (option.column_name) { @@ -135,6 +172,9 @@ export default class MetricsControl extends React.PureComponent { } return null; }).filter(option => option); + if (!this.props.multi) { + optionValues = optionValues[0]; + } this.props.onChange(optionValues); } @@ -149,7 +189,6 @@ export default class MetricsControl extends React.PureComponent { this.setState(nextState); } - optionsForSelect(props) { const options = [ ...props.columns, @@ -170,6 +209,21 @@ export default class MetricsControl extends React.PureComponent { }); } + selectFilterOption(option, filterValue) { + if (this.state.aggregateInInput) { + let endIndex = filterValue.length; + if (filterValue.endsWith(')')) { + endIndex = filterValue.length - 1; + } + const valueAfterAggregate = filterValue.substring(filterValue.indexOf('(') + 1, endIndex); + return option.column_name && + (option.column_name.toLowerCase().indexOf(valueAfterAggregate.toLowerCase()) >= 0); + } + return option.optionName && + (!option.metric_name || !isAutoGeneratedMetric(option)) && + (option.optionName.toLowerCase().indexOf(filterValue.toLowerCase()) >= 0); + } + render() { // TODO figure out why the dropdown isnt appearing as soon as a metric is selected return ( @@ -178,32 +232,19 @@ export default class MetricsControl extends React.PureComponent { ( - - ))} - valueRenderer={option => ( - - )} + optionRenderer={this.optionRenderer} + valueRenderer={this.valueRenderer} onInputChange={this.checkIfAggregateInInput} - filterOption={this.state.aggregateInInput ? selectFilterOption : null} - refFunc={(ref) => { - if (ref) { - // eslint-disable-next-line no-underscore-dangle - this.select = ref._selectRef; - } - }} + filterOption={this.selectFilterOption} + refFunc={this.refFunc} selectWrap={VirtualizedSelect} />
diff --git a/superset/assets/javascripts/explore/main.css b/superset/assets/javascripts/explore/main.css index 8cebc6c3080c1..c6fede9074ebf 100644 --- a/superset/assets/javascripts/explore/main.css +++ b/superset/assets/javascripts/explore/main.css @@ -138,7 +138,3 @@ .datasource-container { overflow: auto; } - -.edit-adhoc-metric-save-btn { - margin-right: 5px; -} diff --git a/superset/assets/javascripts/explore/propTypes/aggregateOptionType.js b/superset/assets/javascripts/explore/propTypes/aggregateOptionType.js new file mode 100644 index 0000000000000..0a5eebfa32ed5 --- /dev/null +++ b/superset/assets/javascripts/explore/propTypes/aggregateOptionType.js @@ -0,0 +1,5 @@ +import PropTypes from 'prop-types'; + +export default PropTypes.shape({ + aggregate_name: PropTypes.string.isRequired, +}); diff --git a/superset/assets/javascripts/explore/propTypes/columnType.js b/superset/assets/javascripts/explore/propTypes/columnType.js index 10d6346eb9632..5ff33e590aab0 100644 --- a/superset/assets/javascripts/explore/propTypes/columnType.js +++ b/superset/assets/javascripts/explore/propTypes/columnType.js @@ -2,5 +2,5 @@ import PropTypes from 'prop-types'; export default PropTypes.shape({ column_name: PropTypes.string.isRequired, - type: PropTypes.string.isRequired, + type: PropTypes.string, }); diff --git a/superset/assets/javascripts/explore/propTypes/legacyMetricType.js b/superset/assets/javascripts/explore/propTypes/legacyMetricType.js deleted file mode 100644 index dccafaa867484..0000000000000 --- a/superset/assets/javascripts/explore/propTypes/legacyMetricType.js +++ /dev/null @@ -1,6 +0,0 @@ -import PropTypes from 'prop-types'; - -export default PropTypes.shape({ - column: PropTypes.string.isRequired, - type: PropTypes.string.isRequired, -}); diff --git a/superset/assets/javascripts/explore/stores/controls.jsx b/superset/assets/javascripts/explore/stores/controls.jsx index bb97bd5050ad0..f0d00bf030bea 100644 --- a/superset/assets/javascripts/explore/stores/controls.jsx +++ b/superset/assets/javascripts/explore/stores/controls.jsx @@ -133,7 +133,6 @@ export const controls = { multi: true, label: t('Metrics'), validators: [v.nonEmpty], - valueKey: 'optionName', default: (c) => { const metric = mainMetric(c.options); return metric ? [metric] : null; @@ -141,6 +140,7 @@ export const controls = { mapStateToProps: state => ({ columns: state.datasource ? state.datasource.columns : [], savedMetrics: state.datasource ? state.datasource.metrics : [], + datasourceType: state.datasource && state.datasource.type, }), description: t('One or many metrics to display'), }, @@ -218,17 +218,16 @@ export const controls = { }, metric: { - type: 'SelectControl', + type: 'MetricsControl', + multi: false, label: t('Metric'), clearable: false, - description: t('Choose the metric'), validators: [v.nonEmpty], - optionRenderer: m => , - valueRenderer: m => , default: c => mainMetric(c.options), - valueKey: 'metric_name', mapStateToProps: state => ({ - options: (state.datasource) ? state.datasource.metrics : [], + columns: state.datasource ? state.datasource.columns : [], + savedMetrics: state.datasource ? state.datasource.metrics : [], + datasourceType: state.datasource && state.datasource.type, }), }, diff --git a/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js b/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js index 37a84ce1ae439..ad9ab47254ce6 100644 --- a/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js +++ b/superset/assets/spec/javascripts/explore/AdhocMetric_spec.js @@ -16,6 +16,7 @@ describe('AdhocMetric', () => { expect(adhocMetric).to.deep.equal({ column: valueColumn, aggregate: AGGREGATES.SUM, + fromFormData: false, label: 'SUM(value)', hasCustomLabel: false, optionName: adhocMetric.optionName, diff --git a/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx index 1c4ce0142fb53..346a2872cda34 100644 --- a/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/MetricsControl_spec.jsx @@ -14,6 +14,7 @@ const defaultProps = { name: 'metrics', label: 'Metrics', value: undefined, + multi: true, columns: [ { type: 'VARCHAR(255)', column_name: 'source' }, { type: 'VARCHAR(255)', column_name: 'target' }, @@ -74,6 +75,7 @@ describe('MetricsControl', () => { column: { type: 'double', column_name: 'value' }, aggregate: AGGREGATES.SUM, label: 'SUM(value)', + optionName: 'blahblahblah', }, 'avg__value', ], @@ -86,9 +88,10 @@ describe('MetricsControl', () => { { column: { type: 'double', column_name: 'value' }, aggregate: AGGREGATES.SUM, + fromFormData: true, label: 'SUM(value)', hasCustomLabel: false, - optionName: adhocMetric.optionName, + optionName: 'blahblahblah', }, 'avg__value', ]); @@ -116,6 +119,7 @@ describe('MetricsControl', () => { column: valueColumn, aggregate: AGGREGATES.SUM, label: 'SUM(value)', + fromFormData: false, hasCustomLabel: false, optionName: adhocMetric.optionName, }]]); @@ -181,4 +185,66 @@ describe('MetricsControl', () => { expect(wrapper.state('aggregateInInput')).to.be.null; }); }); + + describe('option filter', () => { + it('includes user defined metrics', () => { + const { wrapper } = setup(); + + expect(!!wrapper.instance().selectFilterOption( + { + metric_name: 'a_metric', + optionName: 'a_metric', + expression: 'SUM(FANCY(metric))', + }, + 'a', + )).to.be.true; + }); + + it('includes columns and aggregates', () => { + const { wrapper } = setup(); + + expect(!!wrapper.instance().selectFilterOption( + { type: 'VARCHAR(255)', column_name: 'source', optionName: '_col_source' }, + 'Sou', + )).to.be.true; + + expect(!!wrapper.instance().selectFilterOption( + { aggregate_name: 'AVG', optionName: '_aggregate_AVG' }, + 'av', + )).to.be.true; + }); + + it('excludes auto generated metrics', () => { + const { wrapper } = setup(); + + expect(!!wrapper.instance().selectFilterOption( + { + metric_name: 'sum__value', + optionName: 'sum__value', + expression: 'SUM(value)', + }, + 'sum', + )).to.be.false; + }); + + it('filters out metrics if the input begins with an aggregate', () => { + const { wrapper } = setup(); + wrapper.setState({ aggregateInInput: true }); + + expect(!!wrapper.instance().selectFilterOption( + { metric_name: 'metric', expression: 'SUM(FANCY(metric))' }, + 'SUM(', + )).to.be.false; + }); + + it('includes columns if the input begins with an aggregate', () => { + const { wrapper } = setup(); + wrapper.setState({ aggregateInInput: true }); + + expect(!!wrapper.instance().selectFilterOption( + { type: 'DOUBLE', column_name: 'value' }, + 'SUM(', + )).to.be.true; + }); + }); }); diff --git a/superset/connectors/druid/models.py b/superset/connectors/druid/models.py index 107e3c84ff9df..d514a2f4eecf9 100644 --- a/superset/connectors/druid/models.py +++ b/superset/connectors/druid/models.py @@ -897,13 +897,16 @@ def resolve_postagg(postagg, post_aggs, agg_names, visited_postaggs, metrics_dic def metrics_and_post_aggs(metrics, metrics_dict): # Separate metrics into those that are aggregations # and those that are post aggregations - agg_names = set() + saved_agg_names = set() + adhoc_agg_configs = [] postagg_names = [] - for metric_name in metrics: - if metrics_dict[metric_name].metric_type != 'postagg': - agg_names.add(metric_name) + for metric in metrics: + if utils.is_adhoc_metric(metric): + adhoc_agg_configs.append(metric) + elif metrics_dict[metric].metric_type != 'postagg': + saved_agg_names.add(metric) else: - postagg_names.append(metric_name) + postagg_names.append(metric) # Create the post aggregations, maintain order since postaggs # may depend on previous ones post_aggs = OrderedDict() @@ -912,8 +915,8 @@ def metrics_and_post_aggs(metrics, metrics_dict): postagg = metrics_dict[postagg_name] visited_postaggs.add(postagg_name) DruidDatasource.resolve_postagg( - postagg, post_aggs, agg_names, visited_postaggs, metrics_dict) - return list(agg_names), post_aggs + postagg, post_aggs, saved_agg_names, visited_postaggs, metrics_dict) + return list(saved_agg_names), adhoc_agg_configs, post_aggs def values_for_column(self, column_name, @@ -968,11 +971,29 @@ def _add_filter_from_pre_query_data(self, df, dimensions, dim_filter): ret = Filter(type='and', fields=[ff, dim_filter]) return ret - def get_aggregations(self, all_metrics): + @staticmethod + def druid_type_from_adhoc_metric(adhoc_metric): + column_type = adhoc_metric.get('column').get('type').lower() + aggregate = adhoc_metric.get('aggregate').lower() + if (aggregate == 'count'): + return 'count' + if (aggregate == 'count_distinct'): + return 'cardinality' + else: + return column_type + aggregate.capitalize() + + def get_aggregations(self, saved_metrics, adhoc_metrics=[]): aggregations = OrderedDict() for m in self.metrics: - if m.metric_name in all_metrics: + if m.metric_name in saved_metrics: aggregations[m.metric_name] = m.json_obj + for adhoc_metric in adhoc_metrics: + aggregations[adhoc_metric['label']] = { + 'fieldName': adhoc_metric['column']['column_name'], + 'fieldNames': [adhoc_metric['column']['column_name']], + 'type': self.druid_type_from_adhoc_metric(adhoc_metric), + 'name': adhoc_metric['label'], + } return aggregations def check_restricted_metrics(self, aggregations): @@ -1066,11 +1087,11 @@ def run_query( # noqa / druid metrics_dict = {m.metric_name: m for m in self.metrics} columns_dict = {c.column_name: c for c in self.columns} - all_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( + saved_metrics, adhoc_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( metrics, metrics_dict) - aggregations = self.get_aggregations(all_metrics) + aggregations = self.get_aggregations(saved_metrics, adhoc_metrics) self.check_restricted_metrics(aggregations) # the dimensions list with dimensionSpecs expanded @@ -1246,6 +1267,7 @@ def query(self, query_obj): cols += query_obj.get('columns') or [] cols += query_obj.get('metrics') or [] + cols = utils.get_metric_names(cols) cols = [col for col in cols if col in df.columns] df = df[cols] diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index b5d1ae9734fdc..c8ee40269c599 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -278,6 +278,15 @@ class SqlaTable(Model, BaseDatasource): export_parent = 'database' export_children = ['metrics', 'columns'] + sqla_aggregations = { + 'COUNT_DISTINCT': lambda column_name: sa.func.COUNT(sa.distinct(column_name)), + 'COUNT': sa.func.COUNT, + 'SUM': sa.func.SUM, + 'AVG': sa.func.AVG, + 'MIN': sa.func.MIN, + 'MAX': sa.func.MAX, + } + def __repr__(self): return self.name @@ -436,23 +445,10 @@ def get_from_clause(self, template_processor=None, db_engine_spec=None): return TextAsFrom(sa.text(from_sql), []).alias('expr_qry') return self.get_sqla_table() - def is_adhoc_metric(self, metric): - return isinstance(metric, dict) and metric['column'] and metric['aggregate'] - def adhoc_metric_to_sa(self, metric): - if metric['aggregate'] == 'COUNT_DISTINCT': - sa_metric = sa.func.COUNT(sa.distinct(column(metric['column']['column_name']))) - if metric['aggregate'] == 'COUNT': - sa_metric = sa.func.COUNT(column(metric['column']['column_name'])) - if metric['aggregate'] == 'SUM': - sa_metric = sa.func.SUM(column(metric['column']['column_name'])) - if metric['aggregate'] == 'AVG': - sa_metric = sa.func.AVG(column(metric['column']['column_name'])) - if metric['aggregate'] == 'MIN': - sa_metric = sa.func.MIN(column(metric['column']['column_name'])) - if metric['aggregate'] == 'MAX': - sa_metric = sa.func.MAX(column(metric['column']['column_name'])) - sa_metric = sa_metric.label(metric['label']) + column_name = metric.get('column').get('column_name') + sa_metric = self.sqla_aggregations[metric.get('aggregate')](column(column_name)) + sa_metric = sa_metric.label(metric.get('label')) return sa_metric def get_sqla_query( # sqla @@ -505,15 +501,12 @@ def get_sqla_query( # sqla raise Exception(_('Empty query?')) metrics_exprs = [] for m in metrics: - if self.is_adhoc_metric(m): + if utils.is_adhoc_metric(m): metrics_exprs.append(self.adhoc_metric_to_sa(m)) elif m in metrics_dict: metrics_exprs.append(metrics_dict.get(m).sqla_col) else: raise Exception(_("Metric '{}' is not valid".format(m))) - print(metrics) - print(metrics_dict) - print(metrics_exprs) if metrics_exprs: main_metric_expr = metrics_exprs[0] else: diff --git a/superset/utils.py b/superset/utils.py index 8e91e8dc80406..4163739bd4ae3 100644 --- a/superset/utils.py +++ b/superset/utils.py @@ -832,8 +832,7 @@ def get_or_create_main_db(): dbobj = ( db.session.query(models.Database) .filter_by(database_name='main') - .first() - ) + .first()) if not dbobj: dbobj = models.Database(database_name='main') dbobj.set_sqlalchemy_uri(conf.get('SQLALCHEMY_DATABASE_URI')) @@ -842,3 +841,14 @@ def get_or_create_main_db(): db.session.add(dbobj) db.session.commit() return dbobj + + +def is_adhoc_metric(metric): + return (isinstance(metric, dict) and + metric['column'] and + metric['aggregate'] and + metric['label']) + + +def get_metric_names(metrics): + return [metric['label'] if is_adhoc_metric(metric) else metric for metric in metrics] diff --git a/superset/viz.py b/superset/viz.py index 6bef4f76577dd..5d98d5e60e55b 100644 --- a/superset/viz.py +++ b/superset/viz.py @@ -1065,12 +1065,12 @@ def process_data(self, df, aggregate=False): df = df.pivot_table( index=DTTM_ALIAS, columns=fd.get('groupby'), - values=fd.get('metrics')) + values=utils.get_metric_names(fd.get('metrics'))) else: df = df.pivot_table( index=DTTM_ALIAS, columns=fd.get('groupby'), - values=fd.get('metrics'), + values=utils.get_metric_names(fd.get('metrics')), fill_value=0, aggfunc=sum) diff --git a/tests/druid_func_tests.py b/tests/druid_func_tests.py index 5b535e9b7150b..22c1f38dc9159 100644 --- a/tests/druid_func_tests.py +++ b/tests/druid_func_tests.py @@ -180,6 +180,51 @@ def test_run_query_no_groupby(self): self.assertIn('post_aggregations', called_args) # restore functions + def test_run_query_with_adhoc_metric(self): + client = Mock() + from_dttm = Mock() + to_dttm = Mock() + from_dttm.replace = Mock(return_value=from_dttm) + to_dttm.replace = Mock(return_value=to_dttm) + from_dttm.isoformat = Mock(return_value='from') + to_dttm.isoformat = Mock(return_value='to') + timezone = 'timezone' + from_dttm.tzname = Mock(return_value=timezone) + ds = DruidDatasource(datasource_name='datasource') + metric1 = DruidMetric(metric_name='metric1') + metric2 = DruidMetric(metric_name='metric2') + ds.metrics = [metric1, metric2] + col1 = DruidColumn(column_name='col1') + col2 = DruidColumn(column_name='col2') + ds.columns = [col1, col2] + all_metrics = [] + post_aggs = ['some_agg'] + ds._metrics_and_post_aggs = Mock(return_value=(all_metrics, post_aggs)) + groupby = [] + metrics = [{ + 'column': {'type': 'DOUBLE', 'column_name': 'col1'}, + 'aggregate': 'SUM', + 'label': 'My Adhoc Metric', + }] + + ds.get_having_filters = Mock(return_value=[]) + client.query_builder = Mock() + client.query_builder.last_query = Mock() + client.query_builder.last_query.query_dict = {'mock': 0} + # no groupby calls client.timeseries + ds.run_query( + groupby, metrics, None, from_dttm, + to_dttm, client=client, filter=[], row_limit=100, + ) + self.assertEqual(0, len(client.topn.call_args_list)) + self.assertEqual(0, len(client.groupby.call_args_list)) + self.assertEqual(1, len(client.timeseries.call_args_list)) + # check that there is no dimensions entry + called_args = client.timeseries.call_args_list[0][1] + self.assertNotIn('dimensions', called_args) + self.assertIn('post_aggregations', called_args) + # restore functions + def test_run_query_single_groupby(self): client = Mock() from_dttm = Mock() @@ -467,7 +512,7 @@ def depends_on(index, fields): depends_on('I', ['H', 'K']) depends_on('J', 'K') depends_on('K', ['m8', 'm9']) - all_metrics, postaggs = DruidDatasource.metrics_and_post_aggs( + all_metrics, saved_metrics, postaggs = DruidDatasource.metrics_and_post_aggs( metrics, metrics_dict) expected_metrics = set(all_metrics) self.assertEqual(9, len(all_metrics)) @@ -541,25 +586,80 @@ def test_metrics_and_post_aggs(self): ), } + adhoc_metric = { + 'column': {'type': 'DOUBLE', 'column_name': 'value'}, + 'aggregate': 'SUM', + 'label': 'My Adhoc Metric', + } + metrics = ['some_sum'] - all_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( + saved_metrics, adhoc_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( + metrics, metrics_dict) + + assert saved_metrics == ['some_sum'] + assert adhoc_metrics == [] + assert post_aggs == {} + + metrics = [adhoc_metric] + saved_metrics, adhoc_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( + metrics, metrics_dict) + + assert saved_metrics == [] + assert adhoc_metrics == [adhoc_metric] + assert post_aggs == {} + + metrics = ['some_sum', adhoc_metric] + saved_metrics, adhoc_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( metrics, metrics_dict) - assert all_metrics == ['some_sum'] + assert saved_metrics == ['some_sum'] + assert adhoc_metrics == [adhoc_metric] assert post_aggs == {} metrics = ['quantile_p95'] - all_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( + saved_metrics, adhoc_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( metrics, metrics_dict) result_postaggs = set(['quantile_p95']) - assert all_metrics == ['a_histogram'] + assert saved_metrics == ['a_histogram'] + assert adhoc_metrics == [] assert set(post_aggs.keys()) == result_postaggs metrics = ['aCustomPostAgg'] - all_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( + saved_metrics, adhoc_metrics, post_aggs = DruidDatasource.metrics_and_post_aggs( metrics, metrics_dict) result_postaggs = set(['aCustomPostAgg']) - assert all_metrics == ['aCustomMetric'] + assert saved_metrics == ['aCustomMetric'] + assert adhoc_metrics == [] assert set(post_aggs.keys()) == result_postaggs + + def test_druid_type_from_adhoc_metric(self): + + druid_type = DruidDatasource.druid_type_from_adhoc_metric({ + 'column': {'type': 'DOUBLE', 'column_name': 'value'}, + 'aggregate': 'SUM', + 'label': 'My Adhoc Metric', + }) + assert(druid_type == 'doubleSum') + + druid_type = DruidDatasource.druid_type_from_adhoc_metric({ + 'column': {'type': 'LONG', 'column_name': 'value'}, + 'aggregate': 'MAX', + 'label': 'My Adhoc Metric', + }) + assert(druid_type == 'longMax') + + druid_type = DruidDatasource.druid_type_from_adhoc_metric({ + 'column': {'type': 'VARCHAR(255)', 'column_name': 'value'}, + 'aggregate': 'COUNT', + 'label': 'My Adhoc Metric', + }) + assert(druid_type == 'count') + + druid_type = DruidDatasource.druid_type_from_adhoc_metric({ + 'column': {'type': 'VARCHAR(255)', 'column_name': 'value'}, + 'aggregate': 'COUNT_DISTINCT', + 'label': 'My Adhoc Metric', + }) + assert(druid_type == 'cardinality')