villebro commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873478705
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
+ columns: (AdhocColumn | PhysicalColumn)[];
+}
+
+export interface StandardizedFormDataInterface {
Review Comment:
Do we need the `Interface` suffix here?
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
+ columns: (AdhocColumn | PhysicalColumn)[];
Review Comment:
You can also use this:
```suggestion
columns: QueryFormColumn[];
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/controlPanel.ts:
##########
@@ -96,4 +96,8 @@ export default {
label: t('Number format'),
},
},
+ denormalizeFormData: formData => ({
+ ...formData,
+ metric: formData.standardized_form_data.sharedFormData.metrics[0],
Review Comment:
I know this mixing of camel and snake case is rampant from before, but I
wonder if we could just call it `standardizedFormData` to make our eyes hurt
slightly less? 😆
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
I'm wondering if we should consider adding `percent_metrics` to the metrics
array in `sharedFormData`? Let's say we have a table chart with just a single
metric in `percentage_metrics` and change to Pie - currently the metric will be
lost.
As a solution - Maybe we could add a property to the normalized control
values that states some additional context about the origin of the value. In
this case it could specify that it originated from the `percent_metrics`
control rather than a "regular" metric control. So the additional value coult,
for instance, be `sourceControl`.
This would also address the problem identified in the Pivot table - if we
had the additional context about what type of a control the value originated
from, we would more easily be able to map them back to their original controls
(`groupbyRows` or `groupbyColums`). Ping @kgabryje
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
Review Comment:
Would it make sense to call this `NormalizedFormData` or similar? "Shared"
feels slightly too generic, and "normalized" or "standardized" would more
clearly indicate that a transformation has taken place. To disambiguate with
the already existing `StandardizedFormDataInterface`, maybe we should consider
also renaming that to `StandardizedState`, as it's not directly `FormData`, but
a kind-of struct containing "generic stuff" that's needed to perform the
normalization operations.
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
Review Comment:
Shouldn't this be `QueryFormMetric` to also handle saved metrics?
```suggestion
metrics: QueryFormMetric[];
```
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.test.tsx:
##########
@@ -0,0 +1,267 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { getChartControlPanelRegistry, SqlaFormData } from '@superset-ui/core';
+import TableChartPlugin from '@superset-ui/plugin-chart-table';
+import { BigNumberTotalChartPlugin } from '@superset-ui/plugin-chart-echarts';
+import { sections } from '@superset-ui/chart-controls';
+import {
+ StandardizedFormData,
+ sharedControls,
+ publicControls,
+} from './standardizedFormData';
+import { xAxisControl } from
'../../../plugins/plugin-chart-echarts/src/controls';
+
+describe('should collect control values and create SFD', () => {
+ const sharedControlsFormData = {};
+ Object.entries(sharedControls).forEach(([, names]) => {
+ names.forEach(name => {
+ sharedControlsFormData[name] = name;
+ });
+ });
+ const publicControlsFormData = Object.fromEntries(
+ publicControls.map((name, idx) => [[name], idx]),
+ );
+ const sourceMockFormData: SqlaFormData = {
+ ...sharedControlsFormData,
+ ...publicControlsFormData,
+ datasource: '100__table',
+ viz_type: 'source_viz',
+ };
+ const sourceMockStore = {
+ form_data: sourceMockFormData,
+ controls: Object.fromEntries(
+ Object.entries(sourceMockFormData).map(([key, value]) => [
+ key,
+ { value },
+ ]),
+ ),
+ datasource: {
+ type: 'table',
+ columns: [],
+ },
+ };
+ beforeAll(() => {
+ getChartControlPanelRegistry().registerValue('source_viz', {
+ controlPanelSections: [
+ sections.advancedAnalyticsControls,
+ {
+ label: 'transform controls',
+ controlSetRows: publicControls.map(control => [control]),
+ },
+ {
+ label: 'axis column',
+ controlSetRows: [xAxisControl],
+ },
+ ],
+ });
+ getChartControlPanelRegistry().registerValue('target_viz', {
+ controlPanelSections: [
+ sections.advancedAnalyticsControls,
+ {
+ label: 'transform controls',
+ controlSetRows: publicControls.map(control => [control]),
+ },
+ {
+ label: 'axis column',
+ controlSetRows: [[xAxisControl]],
+ },
+ ],
+ denormalizeFormData: (formData: SqlaFormData) => ({
+ ...formData,
+ columns: formData.standardized_form_data.sharedFormData.columns,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
+ }),
+ });
+ });
+
+ test('collect sharedControls', () => {
+ const sfd = new StandardizedFormData(sourceMockFormData);
+
+ expect(sfd.dumpSFD().sharedFormData.metrics).toEqual(
+ sharedControls.metrics.map(controlName => controlName),
+ );
+ expect(sfd.dumpSFD().sharedFormData.columns).toEqual(
+ sharedControls.columns.map(controlName => controlName),
+ );
+ });
+
+ test('should transform all publicControls', () => {
+ const sfd = new StandardizedFormData(sourceMockFormData);
+ const { formData } = sfd.transform('target_viz', sourceMockStore);
+ Object.entries(publicControlsFormData).forEach(([key]) => {
+ expect(formData).toHaveProperty(key);
+ });
+ Object.entries(sharedControls).forEach(([key, value]) => {
+ expect(formData[key]).toEqual(value);
+ });
+ });
+});
+
+describe('should transform form_data between table and bigNumberTotal', () => {
+ const tableVizFormData = {
+ datasource: '30__table',
+ viz_type: 'table',
+ time_grain_sqla: 'P1D',
+ time_range: 'No filter',
+ query_mode: 'aggregate',
+ groupby: ['name'],
+ metrics: ['count'],
+ all_columns: [],
+ percent_metrics: [],
+ adhoc_filters: [],
+ order_by_cols: [],
+ row_limit: 10000,
+ server_page_length: 10,
+ order_desc: true,
+ table_timestamp_format: 'smart_date',
+ show_cell_bars: true,
+ color_pn: true,
+ applied_time_extras: {},
+ url_params: {
+ form_data_key:
+ 'p3No_sqDW7k-kMTzlBPAPd9vwp1IXTf6stbyzjlrPPa0ninvdYUUiMC6F1iKit3Y',
+ dataset_id: '30',
+ },
+ };
+ const tableVizStore = {
+ form_data: tableVizFormData,
+ controls: {
+ datasource: {
+ value: '30__table',
+ },
+ viz_type: {
+ value: 'table',
+ },
+ slice_id: {},
+ cache_timeout: {},
+ url_params: {
+ value: {
+ form_data_key:
+ 'p3No_sqDW7k-kMTzlBPAPd9vwp1IXTf6stbyzjlrPPa0ninvdYUUiMC6F1iKit3Y',
+ dataset_id: '30',
+ },
+ },
+ granularity_sqla: {},
+ time_grain_sqla: {
+ value: 'P1D',
+ },
+ time_range: {
+ value: 'No filter',
+ },
+ query_mode: {
+ value: 'aggregate',
+ },
+ groupby: {
+ value: ['name'],
+ },
+ metrics: {
+ value: ['count'],
+ },
+ all_columns: {
+ value: [],
+ },
+ percent_metrics: {
+ value: [],
+ },
+ adhoc_filters: {
+ value: [],
+ },
+ timeseries_limit_metric: {},
+ order_by_cols: {
+ value: [],
+ },
+ server_pagination: {},
+ row_limit: {
+ value: 10000,
+ },
+ server_page_length: {
+ value: 10,
+ },
+ include_time: {},
+ order_desc: {
+ value: true,
+ },
+ show_totals: {},
+ emit_filter: {},
+ table_timestamp_format: {
+ value: 'smart_date',
+ },
+ page_length: {},
+ include_search: {},
+ show_cell_bars: {
+ value: true,
+ },
+ align_pn: {},
+ color_pn: {
+ value: true,
+ },
+ column_config: {},
+ conditional_formatting: {},
+ },
+ datasource: {
+ type: 'table',
+ columns: [],
+ },
+ };
+
+ beforeAll(() => {
+ getChartControlPanelRegistry().registerValue(
+ 'big_number_total',
+ new BigNumberTotalChartPlugin().controlPanel,
+ );
+ getChartControlPanelRegistry().registerValue(
+ 'table',
+ new TableChartPlugin().controlPanel,
+ );
+ });
+
+ test('transform', () => {
+ // table -> bigNumberTotal
+ const sfd = new StandardizedFormData(tableVizFormData);
+ const { formData: bntFormData, controlsState: btnControlsState } =
Review Comment:
I assume this is a typo:
```suggestion
const { formData: bntFormData, controlsState: bntControlsState } =
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
