From fe77b57581f00144b0c7301e6b6c4ca82aae9993 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Sat, 22 Dec 2018 18:06:36 -0800 Subject: [PATCH] [refactor] moving some datasource-related code to the frontend (#5769) * [refactor] moving some datasource-related code to the frontend * fix js tests * fix tests * fix test --- .../assets/spec/fixtures/mockDatasource.js | 24 --------- .../components/FixedOrMetricControl_spec.jsx | 6 +-- .../controls/FixedOrMetricControl.jsx | 12 ++--- superset/assets/src/explore/controls.jsx | 53 ++++++++++++------- superset/connectors/base/models.py | 22 ++------ tests/core_tests.py | 4 +- 6 files changed, 48 insertions(+), 73 deletions(-) diff --git a/superset/assets/spec/fixtures/mockDatasource.js b/superset/assets/spec/fixtures/mockDatasource.js index 1de79158e..1aa311c3a 100644 --- a/superset/assets/spec/fixtures/mockDatasource.js +++ b/superset/assets/spec/fixtures/mockDatasource.js @@ -20,7 +20,6 @@ export default { avg__num: 'avg__num', avg__sum_boys: 'avg__sum_boys', }, - gb_cols: [['gender', 'gender'], ['name', 'name'], ['state', 'state']], metrics: [ { expression: 'SUM(birth_names.num)', @@ -160,20 +159,6 @@ export default { ['P1W', 'week'], ['P1M', 'month'], ], - filterable_cols: [ - ['gender', 'gender'], - ['name', 'name'], - ['state', 'state'], - ], - all_cols: [ - ['ds', 'ds'], - ['gender', 'gender'], - ['name', 'name'], - ['num', 'num'], - ['state', 'state'], - ['sum_boys', 'sum_boys'], - ['sum_girls', 'sum_girls'], - ], filter_select: true, order_by_choices: [ ['["ds", true]', 'ds [asc]'], @@ -191,15 +176,6 @@ export default { ['["sum_girls", true]', 'sum_girls [asc]'], ['["sum_girls", false]', 'sum_girls [desc]'], ], - metrics_combo: [ - ['count', 'COUNT(*)'], - ['avg__num', 'avg__num'], - ['avg__sum_boys', 'avg__sum_boys'], - ['avg__sum_girls', 'avg__sum_girls'], - ['sum__num', 'sum__num'], - ['sum__sum_boys', 'sum__sum_boys'], - ['sum__sum_girls', 'sum__sum_girls'], - ], type: 'table', edit_url: '/tablemodelview/edit/7', }, diff --git a/superset/assets/spec/javascripts/explore/components/FixedOrMetricControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/FixedOrMetricControl_spec.jsx index e191c41fd..baff28447 100644 --- a/superset/assets/spec/javascripts/explore/components/FixedOrMetricControl_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/FixedOrMetricControl_spec.jsx @@ -5,10 +5,10 @@ import { OverlayTrigger } from 'react-bootstrap'; import FixedOrMetricControl from '../../../../src/explore/components/controls/FixedOrMetricControl'; -import SelectControl from - '../../../../src/explore/components/controls/SelectControl'; import TextControl from '../../../../src/explore/components/controls/TextControl'; +import MetricsControl from + '../../../../src/explore/components/controls/MetricsControl'; import ControlHeader from '../../../../src/explore/components/ControlHeader'; const defaultProps = { @@ -32,6 +32,6 @@ describe('FixedOrMetricControl', () => { it('renders a TextControl and a SelectControl', () => { const popOver = shallow(inst.renderPopover()); expect(popOver.find(TextControl)).toHaveLength(1); - expect(popOver.find(SelectControl)).toHaveLength(1); + expect(popOver.find(MetricsControl)).toHaveLength(1); }); }); diff --git a/superset/assets/src/explore/components/controls/FixedOrMetricControl.jsx b/superset/assets/src/explore/components/controls/FixedOrMetricControl.jsx index d330b2fb2..ef51788f0 100644 --- a/superset/assets/src/explore/components/controls/FixedOrMetricControl.jsx +++ b/superset/assets/src/explore/components/controls/FixedOrMetricControl.jsx @@ -2,9 +2,8 @@ import React from 'react'; import PropTypes from 'prop-types'; import { Label, Popover, OverlayTrigger } from 'react-bootstrap'; -import controls from '../../controls'; import TextControl from './TextControl'; -import SelectControl from './SelectControl'; +import MetricsControl from './MetricsControl'; import ControlHeader from '../ControlHeader'; import PopoverSection from '../../../components/PopoverSection'; @@ -17,7 +16,7 @@ const propTypes = { onChange: PropTypes.func, value: PropTypes.object, isFloat: PropTypes.bool, - datasource: PropTypes.object, + datasource: PropTypes.object.isRequired, default: PropTypes.shape({ type: PropTypes.oneOf(['fix', 'metric']), value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), @@ -63,7 +62,6 @@ export default class FixedOrMetricControl extends React.Component { renderPopover() { const value = this.props.value || this.props.default; const type = value.type || controlTypes.fixed; - const metricsCombo = this.props.datasource ? this.props.datasource.metrics_combo : null; return (
@@ -84,10 +82,10 @@ export default class FixedOrMetricControl extends React.Component { isSelected={type === controlTypes.metric} onSelect={() => { this.onChange(controlTypes.metric); }} > - { this.setType(controlTypes.metric); }} onChange={this.setMetric} value={this.state.metricValue} diff --git a/superset/assets/src/explore/controls.jsx b/superset/assets/src/explore/controls.jsx index d34fc3653..6dc2431a3 100644 --- a/superset/assets/src/explore/controls.jsx +++ b/superset/assets/src/explore/controls.jsx @@ -163,6 +163,15 @@ const jsFunctionInfo = (
); +function columnChoices(datasource) { + if (datasource && datasource.columns) { + return datasource.columns + .map(col => [col.column_name, col.verbose_name || col.column_name]) + .sort((opt1, opt2) => opt1[1].toLowerCase() > opt2[1].toLowerCase() ? 1 : -1); + } + return []; +} + function jsFunctionControl(label, description, extraDescr = null, height = 100, defaultText = '') { return { type: 'TextAreaControl', @@ -609,7 +618,7 @@ export const controls = { validators: [v.nonEmpty], description: t('Point to your spatial columns'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -619,7 +628,7 @@ export const controls = { validators: [v.nonEmpty], description: t('Point to your spatial columns'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -629,7 +638,7 @@ export const controls = { validators: [v.nonEmpty], description: t('Point to your spatial columns'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -640,7 +649,7 @@ export const controls = { validators: [v.nonEmpty], description: t('Select the longitude column'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -651,7 +660,7 @@ export const controls = { validators: [v.nonEmpty], description: t('Select the latitude column'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -668,7 +677,17 @@ export const controls = { validators: [v.nonEmpty], description: t('Select the geojson column'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), + }), + }, + + polygon: { + type: 'SelectControl', + label: t('Polygon Column'), + validators: [v.nonEmpty], + description: t('Select the polygon column. Each row should contain JSON.array(N) of [longitude, latitude] points'), + mapStateToProps: state => ({ + choices: columnChoices(state.datasource), }), }, @@ -697,7 +716,7 @@ export const controls = { default: null, description: t('Columns to display'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -707,7 +726,7 @@ export const controls = { default: null, description: t('Columns to display'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -1077,26 +1096,22 @@ export const controls = { }, series: { - type: 'SelectControl', + ...groupByControl, label: t('Series'), + multi: false, default: null, description: t('Defines the grouping of entities. ' + 'Each series is shown as a specific color on the chart and ' + 'has a legend toggle'), - mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.gb_cols : [], - }), }, entity: { - type: 'SelectControl', + ...groupByControl, label: t('Entity'), default: null, + multi: false, validators: [v.nonEmpty], description: t('This defines the element to be plotted on the chart'), - mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.gb_cols : [], - }), }, x: { @@ -1684,7 +1699,7 @@ export const controls = { 'Non-numerical columns will be used to label points. ' + 'Leave empty to get a count of points in each cluster.'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), }, @@ -1744,7 +1759,7 @@ export const controls = { 'Either a numerical column or `Auto`, which scales the point based ' + 'on the largest cluster'), mapStateToProps: state => ({ - choices: [].concat([['Auto', 'Auto']], state.datasource.all_cols), + choices: columnChoices(state.datasource), }), }, @@ -2136,7 +2151,7 @@ export const controls = { default: null, description: t('The database columns that contains lines information'), mapStateToProps: state => ({ - choices: (state.datasource) ? state.datasource.all_cols : [], + choices: columnChoices(state.datasource), }), validators: [v.nonEmpty], }, diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index 6876fa095..6de800443 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -95,10 +95,6 @@ class BaseDatasource(AuditMixinNullable, ImportMixin): """String representing the schema of the Datasource (if it applies)""" return None - @property - def groupby_column_names(self): - return sorted([c.column_name for c in self.columns if c.groupby]) - @property def filterable_column_names(self): return sorted([c.column_name for c in self.columns if c.filterable]) @@ -133,14 +129,6 @@ class BaseDatasource(AuditMixinNullable, ImportMixin): metric.table_id = self.id self.metrics += [metric] - @property - def metrics_combo(self): - return sorted( - [ - (m.metric_name, m.verbose_name or m.metric_name or '') - for m in self.metrics], - key=lambda x: x[1]) - @property def short_data(self): """Data representation of the datasource sent to the frontend""" @@ -193,18 +181,16 @@ class BaseDatasource(AuditMixinNullable, ImportMixin): 'cache_timeout': self.cache_timeout, 'params': self.params, 'perm': self.perm, + 'edit_url': self.url, # sqla-specific 'sql': self.sql, - # computed fields - 'all_cols': utils.choicify(self.column_names), + # one to many 'columns': [o.data for o in self.columns], - 'edit_url': self.url, - 'filterable_cols': utils.choicify(self.filterable_column_names), - 'gb_cols': utils.choicify(self.groupby_column_names), 'metrics': [o.data for o in self.metrics], - 'metrics_combo': self.metrics_combo, + + # TODO deprecate, move logic to JS 'order_by_choices': order_by_choices, 'owners': [owner.id for owner in self.owners], 'verbose_map': verbose_map, diff --git a/tests/core_tests.py b/tests/core_tests.py index 5b45cb8c5..886e92fe9 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -536,8 +536,8 @@ class CoreTests(SupersetTestCase): ) resp = self.get_json_resp(url) keys = [ - 'name', 'filterable_cols', 'gb_cols', 'type', 'all_cols', - 'order_by_choices', 'metrics_combo', 'granularity_sqla', + 'name', 'type', + 'order_by_choices', 'granularity_sqla', 'time_grain_sqla', 'id', ] for k in keys: