feat(core): remove defaults for time range filter and Metrics (apache…

…#1114) * feat(core): remove defaults for time range filter and Metrics * Display errors on all 3 controls * Fix for raw mode * Refactor duplicated code
zhaoyongjie · Nov 25, 2021 · f37d199 · f37d199
1 parent e0aa7dc
commit f37d199
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 25 deletions.
diff --git a/...et_ui/superset-ui/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx b/...et_ui/superset-ui/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx
@@ -17,16 +17,10 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { Metric, t, validateNonEmpty } from '@superset-ui/core';
+import { t, validateNonEmpty } from '@superset-ui/core';
 import { ExtraControlProps, SharedControlConfig } from '../types';
-import { mainMetric } from '../utils';
 import { TIME_COLUMN_OPTION } from '../constants';
 
-type Control = {
-  savedMetrics?: Metric[] | null;
-  default?: unknown;
-};
-
 export const dndGroupByControl: SharedControlConfig<'DndColumnSelect'> = {
   type: 'DndColumnSelect',
   label: t('Group by'),
@@ -93,10 +87,6 @@ export const dnd_adhoc_metrics: SharedControlConfig<'DndMetricSelect'> = {
   multi: true,
   label: t('Metrics'),
   validators: [validateNonEmpty],
-  default: (c: Control) => {
-    const metric = mainMetric(c.savedMetrics);
-    return metric ? [metric] : null;
-  },
   mapStateToProps: ({ datasource }) => ({
     columns: datasource ? datasource.columns : [],
     savedMetrics: datasource ? datasource.metrics : [],
@@ -110,7 +100,6 @@ export const dnd_adhoc_metric: SharedControlConfig<'DndMetricSelect'> = {
   multi: false,
   label: t('Metric'),
   description: t('Metric'),
-  default: (c: Control) => mainMetric(c.savedMetrics),
 };
 
 export const dnd_sort_by: SharedControlConfig<'DndMetricSelect'> = {

diff --git a/...superset_ui/superset-ui/packages/superset-ui-chart-controls/src/shared-controls/index.tsx b/...superset_ui/superset-ui/packages/superset-ui-chart-controls/src/shared-controls/index.tsx
@@ -46,7 +46,6 @@ import {
 } from '@superset-ui/core';
 
 import {
-  mainMetric,
   formatSelectOptions,
   D3_FORMAT_OPTIONS,
   D3_FORMAT_DOCS,
@@ -130,10 +129,6 @@ const metrics: SharedControlConfig<'MetricsControl'> = {
   multi: true,
   label: t('Metrics'),
   validators: [validateNonEmpty],
-  default: (c: Control) => {
-    const metric = mainMetric(c.savedMetrics);
-    return metric ? [metric] : null;
-  },
   mapStateToProps: ({ datasource }) => ({
     columns: datasource ? datasource.columns : [],
     savedMetrics: datasource ? datasource.metrics : [],
@@ -147,7 +142,6 @@ const metric: SharedControlConfig<'MetricsControl'> = {
   multi: false,
   label: t('Metric'),
   description: t('Metric'),
-  default: (c: Control) => mainMetric(c.savedMetrics),
 };
 
 const datasourceControl: SharedControlConfig<'DatasourceControl'> = {
@@ -304,7 +298,7 @@ const time_range: SharedControlConfig<'DateFilterControl'> = {
   type: 'DateFilterControl',
   freeForm: true,
   label: TIME_FILTER_LABELS.time_range,
-  default: t('Last week'), // this value is translated, but the backend wouldn't understand a translated value?
+  default: t('No filter'), // this value is translated, but the backend wouldn't understand a translated value?
   description: t(
     'The time range for the visualization. All relative times, e.g. "Last month", ' +
       '"Last 7 days", "now", etc. are evaluated on the server using the server\'s ' +

diff --git a/...rontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-table/src/controlPanel.tsx b/...rontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-table/src/controlPanel.tsx
@@ -36,10 +36,12 @@ import {
   ControlPanelsContainerProps,
   ControlStateMapping,
   D3_TIME_FORMAT_OPTIONS,
-  ExtraControlProps,
   QueryModeLabel,
   sections,
   sharedControls,
+  ControlPanelState,
+  ExtraControlProps,
+  ControlState,
 } from '@superset-ui/chart-controls';
 
 import i18n from './i18n';
@@ -68,6 +70,13 @@ function isQueryMode(mode: QueryMode) {
 const isAggMode = isQueryMode(QueryMode.aggregate);
 const isRawMode = isQueryMode(QueryMode.raw);
 
+const validateAggControlValues = (controls: ControlStateMapping, values: any[]) => {
+  const areControlsEmpty = values.every(val => ensureIsArray(val).length === 0);
+  return areControlsEmpty && isAggMode({ controls })
+    ? [t('Group By, Metrics or Percentage Metrics must have a value')]
+    : [];
+};
+
 const queryMode: ControlConfig<'RadioButtonControl'> = {
   type: 'RadioButtonControl',
   label: t('Query mode'),
@@ -77,6 +86,7 @@ const queryMode: ControlConfig<'RadioButtonControl'> = {
     [QueryMode.raw, QueryModeLabel[QueryMode.raw]],
   ],
   mapStateToProps: ({ controls }) => ({ value: getQueryMode(controls) }),
+  rerender: ['all_columns', 'groupby', 'metrics', 'percent_metrics'],
 };
 
 const all_columns: typeof sharedControls.groupby = {
@@ -91,9 +101,13 @@ const all_columns: typeof sharedControls.groupby = {
   optionRenderer: c => <ColumnOption showType column={c} />,
   valueRenderer: c => <ColumnOption column={c} />,
   valueKey: 'column_name',
-  mapStateToProps: ({ datasource, controls }) => ({
+  mapStateToProps: ({ datasource, controls }, controlState) => ({
     options: datasource?.columns || [],
     queryMode: getQueryMode(controls),
+    externalValidationErrors:
+      isRawMode({ controls }) && ensureIsArray(controlState.value).length === 0
+        ? [t('must have a value')]
+        : [],
   }),
   visibility: isRawMode,
 };
@@ -127,12 +141,18 @@ const percent_metrics: typeof sharedControls.metrics = {
   ),
   multi: true,
   visibility: isAggMode,
-  mapStateToProps: ({ datasource, controls }) => ({
+  mapStateToProps: ({ datasource, controls }, controlState) => ({
     columns: datasource?.columns || [],
     savedMetrics: datasource?.metrics || [],
     datasourceType: datasource?.type,
     queryMode: getQueryMode(controls),
+    externalValidationErrors: validateAggControlValues(controls, [
+      controls.groupby?.value,
+      controls.metrics?.value,
+      controlState.value,
+    ]),
   }),
+  rerender: ['groupby', 'metrics'],
   default: [],
   validators: [],
 };
@@ -160,6 +180,20 @@ const config: ControlPanelConfig = {
             name: 'groupby',
             override: {
               visibility: isAggMode,
+              mapStateToProps: (state: ControlPanelState, controlState: ControlState) => {
+                const { controls } = state;
+                const originalMapStateToProps = sharedControls?.groupby?.mapStateToProps;
+                // @ts-ignore
+                const newState = originalMapStateToProps?.(state, controlState) ?? {};
+                newState.externalValidationErrors = validateAggControlValues(controls, [
+                  controls.metrics?.value,
+                  controls.percent_metrics?.value,
+                  controlState.value,
+                ]);
+
+                return newState;
+              },
+              rerender: ['metrics', 'percent_metrics'],
             },
           },
         ],
@@ -169,6 +203,22 @@ const config: ControlPanelConfig = {
             override: {
               validators: [],
               visibility: isAggMode,
+              mapStateToProps: (
+                { controls, datasource, form_data }: ControlPanelState,
+                controlState: ControlState,
+              ) => ({
+                columns: datasource?.columns.filter(c => c.filterable) || [],
+                savedMetrics: datasource?.metrics || [],
+                // current active adhoc metrics
+                selectedMetrics: form_data.metrics || (form_data.metric ? [form_data.metric] : []),
+                datasource,
+                externalValidationErrors: validateAggControlValues(controls, [
+                  controls.groupby?.value,
+                  controls.percent_metrics?.value,
+                  controlState.value,
+                ]),
+              }),
+              rerender: ['groupby', 'percent_metrics'],
             },
           },
           {
@@ -181,9 +231,11 @@ const config: ControlPanelConfig = {
         [
           {
             name: 'percent_metrics',
-            config: isFeatureEnabled(FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP)
-              ? dnd_percent_metrics
-              : percent_metrics,
+            config: {
+              ...(isFeatureEnabled(FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP)
+                ? dnd_percent_metrics
+                : percent_metrics),
+            },
           },
         ],
         [