sadpandajoe commented on code in PR #45680: URL: https://github.com/apache/superset/pull/45680#discussion_r4392328709
########## superset-frontend/plugins/plugin-chart-echarts/test/Waterfall/EchartsWaterfall.test.tsx: ########## @@ -0,0 +1,186 @@ +/** + * 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 { supersetTheme } from '@apache-superset/core/theme'; +import { ChartProps, SqlaFormData } from '@superset-ui/core'; +import { render } from '@testing-library/react'; +import EchartsWaterfall from '../../src/Waterfall/EchartsWaterfall'; +import transformProps from '../../src/Waterfall/transformProps'; +import { EchartsWaterfallChartProps } from '../../src/Waterfall/types'; +import Echart from '../../src/components/Echart'; +import { EventHandlers } from '../../src/types'; + +jest.mock('../../src/components/Echart', () => ({ + __esModule: true, + default: jest.fn(() => null), +})); + +const mockedEchart = jest.mocked(Echart); + +const rows = [ + { year: '2019', name: 'Sylvester', sum: 10 }, + { year: '2019', name: 'Arnold', sum: 3 }, + { year: '2020', name: 'Sylvester', sum: -10 }, + { year: '2020', name: 'Arnold', sum: 5 }, +]; + +const formData = { + colorScheme: 'bnbColors', + datasource: '3__table', + granularity_sqla: 'ds', + metric: 'sum', + x_axis: 'year', + groupby: 'name', + showTotal: true, +}; + +function renderChart( + overrides: Partial<ChartProps> & { emitCrossFilters?: boolean } = {}, +) { + const setDataMask = jest.fn(); + const onContextMenu = jest.fn(); + const chartProps = new ChartProps({ + formData: formData as unknown as SqlaFormData, + width: 800, + height: 600, + queriesData: [{ data: rows }], + hooks: { setDataMask, onContextMenu }, + theme: supersetTheme, + ...overrides, + }); + const props = transformProps( + chartProps as unknown as EchartsWaterfallChartProps, + ); + render(<EchartsWaterfall {...props} />); + const lastCall = mockedEchart.mock.calls[mockedEchart.mock.calls.length - 1]; + const { eventHandlers } = lastCall[0] as { eventHandlers: EventHandlers }; + return { eventHandlers, setDataMask, onContextMenu }; +} + +beforeEach(() => { + mockedEchart.mockClear(); +}); + +function pointerEvent(dataIndex: number) { + return { + dataIndex, + event: { + stop: jest.fn(), + event: { clientX: 12, clientY: 24 } as PointerEvent, + }, + }; +} + +test('clicking a breakdown bar filters the period and the breakdown', () => { + const { eventHandlers, setDataMask } = renderChart({ + emitCrossFilters: true, + }); + eventHandlers.click(pointerEvent(0)); + expect(setDataMask).toHaveBeenCalledWith( + expect.objectContaining({ + extraFormData: { + filters: [ + { col: 'year', op: 'IN', val: ['2019'] }, + { col: 'name', op: 'IN', val: ['Sylvester'] }, + ], + }, + }), + ); +}); + +test('clicking a period total filters the period only', () => { + const { eventHandlers, setDataMask } = renderChart({ + emitCrossFilters: true, + }); + eventHandlers.click(pointerEvent(2)); + expect(setDataMask).toHaveBeenCalledWith( + expect.objectContaining({ + extraFormData: { + filters: [{ col: 'year', op: 'IN', val: ['2019'] }], + }, + }), + ); +}); + +test('clicking the same bar again clears the filter', () => { + const { eventHandlers, setDataMask } = renderChart({ + emitCrossFilters: true, + filterState: { selectedValues: [JSON.stringify(['2019', 'Sylvester'])] }, + }); + eventHandlers.click(pointerEvent(0)); + expect(setDataMask).toHaveBeenCalledWith( + expect.objectContaining({ + extraFormData: { filters: [] }, + }), + ); +}); + +test('does not emit a filter when cross-filtering is disabled', () => { + const { eventHandlers, setDataMask } = renderChart({ + emitCrossFilters: false, + }); + eventHandlers.click(pointerEvent(0)); + expect(setDataMask).not.toHaveBeenCalled(); +}); + +test('clicking the grand total does not change the current filter', () => { + const { eventHandlers, setDataMask } = renderChart({ + emitCrossFilters: true, + formData: { ...formData, groupby: undefined } as unknown as SqlaFormData, + }); + eventHandlers.click(pointerEvent(2)); + expect(setDataMask).not.toHaveBeenCalled(); +}); + +test('a context menu carries the drill filters for the clicked bar', () => { + const { eventHandlers, onContextMenu } = renderChart({ + emitCrossFilters: true, + }); + const event = pointerEvent(0); + eventHandlers.contextmenu(event); + expect(event.event.stop).toHaveBeenCalled(); + expect(onContextMenu).toHaveBeenCalledWith( + 12, + 24, + expect.objectContaining({ Review Comment: `expect.objectContaining` here only pins `drillToDetail` and `drillBy`, so the `crossFilter` entry of the context-menu payload could be dropped or wrongly built (for example `isCurrentValueSelected` always false) and this test would still pass, which would break "filter" and "clear filter" from the right-click menu. Would asserting `crossFilter.dataMask` and `isCurrentValueSelected` for both an unselected and an already-selected bar cover it? ########## superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/crossFilter.ts: ########## @@ -0,0 +1,245 @@ +/** + * 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 { + DataRecordValue, + getColumnLabel, + getTimeFormatter, + QueryFormColumn, + QueryObjectFilterClause, +} from '@superset-ui/core'; +import { GenericDataType } from '@apache-superset/core/common'; +import { NULL_STRING } from '../constants'; +import { formatSeriesName } from '../utils/series'; + +/** Matches `optionLabel` in `src/utils/common.ts`. */ +const EMPTY_STRING = '<empty string>'; + +export type WaterfallCategory = { + /** The trailing total of a chart without a breakdown. Not a data category. */ + isGrandTotal: boolean; + /** Filter the X axis only. Period totals and charts without a breakdown. */ + xOnly: boolean; + xColumn: QueryFormColumn; + xValue: DataRecordValue; + breakdownColumn?: QueryFormColumn; + breakdownValue?: DataRecordValue; +}; + +/** + * Selection-key part. `bigint` is wrapped so it does not share a key with the + * decimal string of the same digits (`42n` vs `'42'`). `JSON.stringify` cannot + * encode a bare `bigint`. + */ +function toKeyPart( + value: DataRecordValue, +): string | number | boolean | null | { bigint: string } { + if (value == null) { + return null; + } + if (typeof value === 'bigint') { + return { bigint: value.toString() }; + } + return value instanceof Date ? value.toISOString() : value; +} + +/** + * `bigint` throws in `JSON.stringify`, and the data mask is serialized into + * dashboard state and the chart-data request. + */ +function toJsonSafeFilterValue( + value: Exclude<DataRecordValue, null>, +): Exclude<DataRecordValue, null | bigint> { + return typeof value === 'bigint' ? value.toString() : value; +} + +export function getWaterfallCrossFilterKey( + category: WaterfallCategory, +): string | null { + if (category.isGrandTotal) { + return null; + } + const xPart = toKeyPart(category.xValue); + if (category.xOnly || !category.breakdownColumn) { + return JSON.stringify([xPart]); + } + return JSON.stringify([xPart, toKeyPart(category.breakdownValue ?? null)]); +} + +export function getSelectedWaterfallKeys(selectedValues: unknown): string[] { + if (!selectedValues) { + return []; + } + if (Array.isArray(selectedValues)) { + return selectedValues.map(value => String(value)); + } + if (typeof selectedValues === 'object') { + return Object.values(selectedValues as Record<string, unknown>).map(value => + String(value), + ); + } + return []; +} + +export function isWaterfallBarDimmed( + category: WaterfallCategory, + selectedKeys: string[], +): boolean { + if (selectedKeys.length === 0) { + return false; + } + if (category.isGrandTotal) { + return true; + } + const exactKey = getWaterfallCrossFilterKey(category); + if (exactKey && selectedKeys.includes(exactKey)) { + return false; + } + // An X-only selection (a period total) keeps every bar of that X value. + const xOnlyKey = JSON.stringify([toKeyPart(category.xValue)]); + return !selectedKeys.includes(xOnlyKey); +} + +function formatDimensionValue( + col: QueryFormColumn, + value: DataRecordValue, + coltypeMapping?: Record<string, number>, + timeFormat?: string, +): string { + if (value == null) { + return NULL_STRING; + } + if (value === '') { + return EMPTY_STRING; + } + return formatSeriesName(value, { + timeFormatter: getTimeFormatter(timeFormat), + coltype: coltypeMapping?.[getColumnLabel(col)] as + | GenericDataType + | undefined, + }); +} + +function dimensionFilter( + col: QueryFormColumn, + value: DataRecordValue, +): QueryObjectFilterClause { + if (value == null) { + return { col, op: 'IS NULL' }; + } + return { col, op: 'IN', val: [toJsonSafeFilterValue(value)] }; Review Comment: For a temporal X axis with a time grain, the bar's value is the bucket start (e.g. Jan 1 for a monthly grain), but this emits `col IN [<that timestamp>]`, and the drill path below emits `==`. Both compare against the raw, unbucketed column, so clicking the January bar of a chart over orders dated Jan 10 and Jan 20 returns no rows (or only rows exactly at midnight Jan 1) instead of the month. Timeseries handles this with a grain-aware `TEMPORAL_RANGE` filter; should temporal X columns do the same here for both cross-filter and drill? ########## superset-frontend/plugins/plugin-chart-echarts/test/Waterfall/crossFilter.test.ts: ########## @@ -0,0 +1,238 @@ +/** + * 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 { GenericDataType } from '@apache-superset/core/common'; +import { getTimeFormatter } from '@superset-ui/core'; +import { + getSelectedWaterfallKeys, + getWaterfallCrossFilterDataMask, + getWaterfallCrossFilterKey, + getWaterfallDrillFilters, + isWaterfallBarDimmed, + WaterfallCategory, +} from '../../src/Waterfall/crossFilter'; + +const yearBar: WaterfallCategory = { + isGrandTotal: false, + xOnly: true, + xColumn: 'year', + xValue: 2019, +}; + +const breakdownBar: WaterfallCategory = { + isGrandTotal: false, + xOnly: false, + xColumn: 'year', + xValue: 2019, + breakdownColumn: 'name', + breakdownValue: 'Sylvester', +}; + +const periodTotal: WaterfallCategory = { + isGrandTotal: false, + xOnly: true, + xColumn: 'year', + xValue: 2019, + breakdownColumn: 'name', +}; + +const grandTotal: WaterfallCategory = { + isGrandTotal: true, + xOnly: true, + xColumn: 'year', + xValue: null, +}; + +test('builds an X-only key and an X plus breakdown key', () => { + expect(getWaterfallCrossFilterKey(yearBar)).toBe(JSON.stringify([2019])); + expect(getWaterfallCrossFilterKey(breakdownBar)).toBe( + JSON.stringify([2019, 'Sylvester']), + ); + expect(getWaterfallCrossFilterKey(grandTotal)).toBeNull(); +}); + +test('filters the X column for a chart without a breakdown', () => { + const { dataMask } = getWaterfallCrossFilterDataMask({ + category: yearBar, + selectedKeys: [], + }); + expect(dataMask.extraFormData.filters).toEqual([ + { col: 'year', op: 'IN', val: [2019] }, + ]); + expect(dataMask.filterState.label).toBe('2019'); + expect(dataMask.filterState.selectedValues).toEqual([JSON.stringify([2019])]); +}); + +test('filters both the X column and the breakdown', () => { + const { dataMask } = getWaterfallCrossFilterDataMask({ + category: breakdownBar, + selectedKeys: [], + }); + expect(dataMask.extraFormData.filters).toEqual([ + { col: 'year', op: 'IN', val: [2019] }, + { col: 'name', op: 'IN', val: ['Sylvester'] }, + ]); + expect(dataMask.filterState.label).toBe('2019, Sylvester'); +}); + +test('a period total filters the X column only', () => { + const { dataMask } = getWaterfallCrossFilterDataMask({ + category: periodTotal, + selectedKeys: [], + }); + expect(dataMask.extraFormData.filters).toEqual([ + { col: 'year', op: 'IN', val: [2019] }, + ]); +}); + +test('clicking the same bar again clears the filter', () => { + const key = getWaterfallCrossFilterKey(yearBar)!; + const { dataMask } = getWaterfallCrossFilterDataMask({ + category: yearBar, + selectedKeys: [key], + }); + expect(dataMask.extraFormData.filters).toEqual([]); + expect(dataMask.filterState.selectedValues).toBeNull(); +}); + +test('a grand total produces no filter', () => { + const { dataMask } = getWaterfallCrossFilterDataMask({ + category: grandTotal, + selectedKeys: [], + }); + expect(dataMask.extraFormData.filters).toEqual([]); + expect(dataMask.filterState.selectedValues).toBeNull(); +}); + +test('null and empty breakdown values stay distinct', () => { + const nullBar: WaterfallCategory = { + ...breakdownBar, + breakdownValue: null, + }; + const emptyBar: WaterfallCategory = { + ...breakdownBar, + breakdownValue: '', + }; + expect( + getWaterfallCrossFilterDataMask({ + category: nullBar, + selectedKeys: [], + }).dataMask.extraFormData.filters, + ).toEqual([ + { col: 'year', op: 'IN', val: [2019] }, + { col: 'name', op: 'IS NULL' }, + ]); + const emptyMask = getWaterfallCrossFilterDataMask({ + category: emptyBar, + selectedKeys: [], + }).dataMask; + expect(emptyMask.extraFormData.filters).toEqual([ + { col: 'year', op: 'IN', val: [2019] }, + { col: 'name', op: 'IN', val: [''] }, + ]); + expect(emptyMask.filterState.label).toBe('2019, <empty string>'); +}); + +test('keeps a numeric category unabbreviated', () => { + const mask = getWaterfallCrossFilterDataMask({ + category: { ...yearBar, xValue: 2024 }, + selectedKeys: [], + }).dataMask; + expect(mask.filterState.label).toBe('2024'); +}); + +test('a bigint category does not share a selection key with the same decimal string', () => { + const bigintBar: WaterfallCategory = { ...yearBar, xValue: BigInt(42) }; + const stringBar: WaterfallCategory = { ...yearBar, xValue: '42' }; + const bigintKey = getWaterfallCrossFilterKey(bigintBar); + const stringKey = getWaterfallCrossFilterKey(stringBar); + + expect(bigintKey).not.toBe(stringKey); + + const mask = getWaterfallCrossFilterDataMask({ + category: bigintBar, + selectedKeys: [], + }).dataMask; + expect(mask.extraFormData.filters).toEqual([ + { col: 'year', op: 'IN', val: ['42'] }, + ]); + expect(mask.filterState.selectedValues).toEqual([bigintKey]); + expect(() => JSON.stringify(mask)).not.toThrow(); + + const stringMask = getWaterfallCrossFilterDataMask({ + category: stringBar, + selectedKeys: bigintKey ? [bigintKey] : [], + }).dataMask; + expect(stringMask.extraFormData.filters).not.toEqual([]); + expect(isWaterfallBarDimmed(stringBar, bigintKey ? [bigintKey] : [])).toBe( + true, + ); + expect(isWaterfallBarDimmed(bigintBar, bigintKey ? [bigintKey] : [])).toBe( + false, + ); +}); + +test('formats a temporal X value with the axis time format', () => { + const epoch = Date.parse('2020-01-15T00:00:00.000Z'); + const mask = getWaterfallCrossFilterDataMask({ + category: { + ...yearBar, + xValue: epoch, + }, + selectedKeys: [], + coltypeMapping: { year: GenericDataType.Temporal }, + timeFormat: '%Y-%m-%d', + }).dataMask; + expect(mask.filterState.label).toBe( + getTimeFormatter('%Y-%m-%d')(new Date(epoch)), + ); +}); + +test('reads selected keys from an array or an object', () => { + expect(getSelectedWaterfallKeys(['a'])).toEqual(['a']); + expect(getSelectedWaterfallKeys({ 0: 'a' })).toEqual(['a']); + expect(getSelectedWaterfallKeys(undefined)).toEqual([]); +}); + +test('an X-only selection keeps every bar of that period bright', () => { Review Comment: The dimming tests only use an X-only selection (`[2019]`), so nothing pins the breakdown case: with `[2019, 'Sylvester']` selected, Sylvester should stay bright while the other 2019 breakdown bar and the period total dim. An implementation that keeps every bar sharing the selected X value bright would pass all of these assertions. Could a case selecting the breakdown key assert that? -- 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]
