From 6e59f11f4ce76305c1b0adee883f3b958199805b Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Thu, 13 Jul 2023 11:33:16 -0300 Subject: [PATCH] fix: Dashboard time grain in Pivot Table (#24665) --- .../src/plugin/buildQuery.ts | 9 +- .../test/plugin/buildQuery.test.ts | 146 +++++++++++------- 2 files changed, 96 insertions(+), 59 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts index ac2ddcd33290b..55a6cefd083a4 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts @@ -30,7 +30,10 @@ import { import { PivotTableQueryFormData } from '../types'; export default function buildQuery(formData: PivotTableQueryFormData) { - const { groupbyColumns = [], groupbyRows = [] } = formData; + const { groupbyColumns = [], groupbyRows = [], extra_form_data } = formData; + const time_grain_sqla = + extra_form_data?.time_grain_sqla || formData.time_grain_sqla; + // TODO: add deduping of AdhocColumns const columns = Array.from( new Set([ @@ -40,7 +43,7 @@ export default function buildQuery(formData: PivotTableQueryFormData) { ).map(col => { if ( isPhysicalColumn(col) && - formData.time_grain_sqla && + time_grain_sqla && hasGenericChartAxes && /* Charts created before `GENERIC_CHART_AXES` is enabled have a different * form data, with `granularity_sqla` set instead. @@ -49,7 +52,7 @@ export default function buildQuery(formData: PivotTableQueryFormData) { formData.granularity_sqla === col) ) { return { - timeGrain: formData.time_grain_sqla, + timeGrain: time_grain_sqla, columnType: 'BASE_AXIS', sqlExpression: col, label: col, diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts index f3ac8b82d4c49..7bb47d785cd8a 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts @@ -22,65 +22,99 @@ import * as supersetCoreModule from '@superset-ui/core'; import buildQuery from '../../src/plugin/buildQuery'; import { PivotTableQueryFormData } from '../../src/types'; -describe('PivotTableChart buildQuery', () => { - const formData: PivotTableQueryFormData = { - groupbyRows: ['row1', 'row2'], - groupbyColumns: ['col1', 'col2'], - metrics: ['metric1', 'metric2'], - tableRenderer: 'Table With Subtotal', - colOrder: 'key_a_to_z', - rowOrder: 'key_a_to_z', - aggregateFunction: 'Sum', - transposePivot: true, - rowSubtotalPosition: true, - colSubtotalPosition: true, - colTotals: true, - rowTotals: true, - valueFormat: 'SMART_NUMBER', - datasource: '5__table', - viz_type: 'my_chart', - width: 800, - height: 600, - combineMetric: false, - verboseMap: {}, - columnFormats: {}, - currencyFormats: {}, - metricColorFormatters: [], - dateFormatters: {}, - setDataMask: () => {}, - legacy_order_by: 'count', - order_desc: true, - margin: 0, +const formData: PivotTableQueryFormData = { + groupbyRows: ['row1', 'row2'], + groupbyColumns: ['col1', 'col2'], + metrics: ['metric1', 'metric2'], + tableRenderer: 'Table With Subtotal', + colOrder: 'key_a_to_z', + rowOrder: 'key_a_to_z', + aggregateFunction: 'Sum', + transposePivot: true, + rowSubtotalPosition: true, + colSubtotalPosition: true, + colTotals: true, + rowTotals: true, + valueFormat: 'SMART_NUMBER', + datasource: '5__table', + viz_type: 'my_chart', + width: 800, + height: 600, + combineMetric: false, + verboseMap: {}, + columnFormats: {}, + currencyFormats: {}, + metricColorFormatters: [], + dateFormatters: {}, + setDataMask: () => {}, + legacy_order_by: 'count', + order_desc: true, + margin: 0, + time_grain_sqla: TimeGranularity.MONTH, + temporal_columns_lookup: { col1: true }, +}; + +test('should build groupby with series in form data', () => { + const queryContext = buildQuery(formData); + const [query] = queryContext.queries; + expect(query.columns).toEqual(['col1', 'col2', 'row1', 'row2']); +}); + +test('should work with old charts after GENERIC_CHART_AXES is enabled', () => { + Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', { + value: true, + }); + const modifiedFormData = { + ...formData, + time_grain_sqla: TimeGranularity.MONTH, + granularity_sqla: 'col1', }; + const queryContext = buildQuery(modifiedFormData); + const [query] = queryContext.queries; + expect(query.columns).toEqual([ + { + timeGrain: 'P1M', + columnType: 'BASE_AXIS', + sqlExpression: 'col1', + label: 'col1', + expressionType: 'SQL', + }, + 'col2', + 'row1', + 'row2', + ]); +}); - it('should build groupby with series in form data', () => { - const queryContext = buildQuery(formData); - const [query] = queryContext.queries; - expect(query.columns).toEqual(['col1', 'col2', 'row1', 'row2']); +test('should prefer extra_form_data.time_grain_sqla over formData.time_grain_sqla', () => { + Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', { + value: true, }); + const modifiedFormData = { + ...formData, + extra_form_data: { time_grain_sqla: TimeGranularity.QUARTER }, + }; + const queryContext = buildQuery(modifiedFormData); + const [query] = queryContext.queries; + expect(query.columns?.[0]).toEqual({ + timeGrain: TimeGranularity.QUARTER, + columnType: 'BASE_AXIS', + sqlExpression: 'col1', + label: 'col1', + expressionType: 'SQL', + }); +}); - it('should work with old charts after GENERIC_CHART_AXES is enabled', () => { - Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', { - value: true, - }); - const modifiedFormData = { - ...formData, - time_grain_sqla: TimeGranularity.MONTH, - granularity_sqla: 'col1', - }; - const queryContext = buildQuery(modifiedFormData); - const [query] = queryContext.queries; - expect(query.columns).toEqual([ - { - timeGrain: 'P1M', - columnType: 'BASE_AXIS', - sqlExpression: 'col1', - label: 'col1', - expressionType: 'SQL', - }, - 'col2', - 'row1', - 'row2', - ]); +test('should fallback to formData.time_grain_sqla if extra_form_data.time_grain_sqla is not set', () => { + Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', { + value: true, + }); + const queryContext = buildQuery(formData); + const [query] = queryContext.queries; + expect(query.columns?.[0]).toEqual({ + timeGrain: formData.time_grain_sqla, + columnType: 'BASE_AXIS', + sqlExpression: 'col1', + label: 'col1', + expressionType: 'SQL', }); });