From d8b5d9123c9b0d128228915f4919b9fb5e90ae80 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Thu, 13 Jun 2024 14:49:59 -0300 Subject: [PATCH 1/6] feat: Improves Drill By options --- .../src/ui-overrides/types.ts | 11 + .../ChartContextMenu/ChartContextMenu.tsx | 12 +- .../Chart/DrillBy/DrillByMenuItems.tsx | 193 ++++++++++++------ .../Chart/MenuItemWithTruncation.tsx | 3 +- 4 files changed, 157 insertions(+), 62 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts b/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts index bd2fa9047c7be..775e2c129ad15 100644 --- a/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts @@ -23,6 +23,8 @@ import { ComponentType, } from 'react'; import type { Editor } from 'brace'; +import { BaseFormData } from '../query'; +import { JsonResponse } from '../connection'; /** * A function which returns text (or marked-up text) @@ -30,6 +32,14 @@ import type { Editor } from 'brace'; */ type ReturningDisplayable

= (props: P) => string | ReactElement; +/** + * A function which returns the drill by options for a given dataset and chart's formData. + */ +export type LoadDrillByOptions = ( + datasetId: number, + formData: BaseFormData, +) => Promise; + /** * This type defines all available extensions of Superset's default UI. * Namespace the keys here to follow the form of 'some_domain.functionality.item'. @@ -193,6 +203,7 @@ export interface CustomAutocomplete extends AutocompleteItem { export type Extensions = Partial<{ 'alertsreports.header.icon': ComponentType; + 'load.drillby.options': LoadDrillByOptions; 'embedded.documentation.configuration_details': ComponentType; 'embedded.documentation.description': ReturningDisplayable; 'embedded.documentation.url': string; diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx index b7da3a7382bb1..bf1ea84ff1860 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx @@ -18,6 +18,7 @@ */ import { forwardRef, + Key, ReactNode, RefObject, useCallback, @@ -104,6 +105,7 @@ const ChartContextMenu = ( const crossFiltersEnabled = useSelector( ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled, ); + const [openKeys, setOpenKeys] = useState([]); const isDisplayed = (item: ContextMenuItem) => displayedItems === ContextMenuItem.All || @@ -254,6 +256,8 @@ const ChartContextMenu = ( formData={formData} contextMenuY={clientY} submenuIndex={submenuIndex} + open={openKeys.includes('drill-by-submenu')} + key="drill-by-submenu" {...(additionalConfig?.drillBy || {})} />, ); @@ -288,7 +292,13 @@ const ChartContextMenu = ( return ReactDOM.createPortal( +

{ + setOpenKeys(openKeys); + }} + > {menuItems.length ? ( menuItems ) : ( diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx index c7b4ec8e45b53..635e2f7ea69be 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx @@ -18,11 +18,12 @@ */ import { - ChangeEvent, + CSSProperties, ReactNode, useCallback, useEffect, useMemo, + useRef, useState, } from 'react'; import { Menu } from 'src/components/Menu'; @@ -31,12 +32,21 @@ import { Behavior, Column, ContextMenuFilters, + FAST_DEBOUNCE, + JsonResponse, css, ensureIsArray, getChartMetadataRegistry, + getExtensionsRegistry, + logging, t, useTheme, } from '@superset-ui/core'; +import rison from 'rison'; +import { debounce } from 'lodash'; +import AutoSizer from 'react-virtualized-auto-sizer'; +import { FixedSizeList as List } from 'react-window'; +import { AntdInput } from 'src/components'; import Icons from 'src/components/Icons'; import { Input } from 'src/components/Input'; import { useToasts } from 'src/components/MessageToasts/withToasts'; @@ -65,8 +75,28 @@ export interface DrillByMenuItemsProps { onClick?: (event: MouseEvent) => void; openNewModal?: boolean; excludedColumns?: Column[]; + open: boolean; } +const loadDrillByOptions = getExtensionsRegistry().get('load.drillby.options'); + +const queryString = rison.encode({ + columns: [ + 'table_name', + 'owners.first_name', + 'owners.last_name', + 'created_by.first_name', + 'created_by.last_name', + 'created_on_humanized', + 'changed_by.first_name', + 'changed_by.last_name', + 'changed_on_humanized', + 'columns.column_name', + 'columns.verbose_name', + 'columns.groupby', + ], +}); + export const DrillByMenuItems = ({ drillByConfig, formData, @@ -76,6 +106,7 @@ export const DrillByMenuItems = ({ onClick = () => {}, excludedColumns, openNewModal = true, + open, ...rest }: DrillByMenuItemsProps) => { const theme = useTheme(); @@ -86,6 +117,9 @@ export const DrillByMenuItems = ({ const [columns, setColumns] = useState([]); const [showModal, setShowModal] = useState(false); const [currentColumn, setCurrentColumn] = useState(); + const ref = useRef(null); + const showSearch = + loadDrillByOptions || columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD; const handleSelection = useCallback( (event, column) => { onClick(event); @@ -102,10 +136,14 @@ export const DrillByMenuItems = ({ }, []); useEffect(() => { - // Input is displayed only when columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD - // Reset search input in case Input gets removed - setSearchInput(''); - }, [columns.length]); + if (open) { + ref.current?.input.focus(); + } else { + // Reset search input when menu is closed + ref.current?.setValue(''); + setSearchInput(''); + } + }, [open]); const hasDrillBy = drillByConfig?.groupbyFieldName; @@ -119,51 +157,59 @@ export const DrillByMenuItems = ({ const verboseMap = useVerboseMap(dataset); useEffect(() => { + async function loadOptions() { + const datasetId = +formData.datasource.split('__')[0]; + try { + setIsLoadingColumns(true); + let response: JsonResponse; + if (loadDrillByOptions) { + response = await loadDrillByOptions(datasetId, formData); + } else { + response = await cachedSupersetGet({ + endpoint: `/api/v1/dataset/${datasetId}?q=${queryString}`, + }); + } + const { json } = response; + const { result } = json; + setDataset(result); + setColumns( + ensureIsArray(result.columns) + .filter(column => column.groupby) + .filter( + column => + !ensureIsArray( + formData[drillByConfig?.groupbyFieldName ?? ''], + ).includes(column.column_name) && + column.column_name !== formData.x_axis && + ensureIsArray(excludedColumns)?.every( + excludedCol => excludedCol.column_name !== column.column_name, + ), + ), + ); + } catch (error) { + logging.error(error); + supersetGetCache.delete(`/api/v1/dataset/${datasetId}`); + addDangerToast(t('Failed to load dimensions for drill by')); + } finally { + setIsLoadingColumns(false); + } + } if (handlesDimensionContextMenu && hasDrillBy) { - const datasetId = formData.datasource.split('__')[0]; - cachedSupersetGet({ - endpoint: `/api/v1/dataset/${datasetId}`, - }) - .then(({ json: { result } }) => { - setDataset(result); - setColumns( - ensureIsArray(result.columns) - .filter(column => column.groupby) - .filter( - column => - !ensureIsArray( - formData[drillByConfig.groupbyFieldName ?? ''], - ).includes(column.column_name) && - column.column_name !== formData.x_axis && - ensureIsArray(excludedColumns)?.every( - excludedCol => - excludedCol.column_name !== column.column_name, - ), - ), - ); - }) - .catch(() => { - supersetGetCache.delete(`/api/v1/dataset/${datasetId}`); - addDangerToast(t('Failed to load dimensions for drill by')); - }) - .finally(() => { - setIsLoadingColumns(false); - }); + loadOptions(); } }, [ addDangerToast, + drillByConfig?.groupbyFieldName, excludedColumns, formData, - drillByConfig?.groupbyFieldName, handlesDimensionContextMenu, hasDrillBy, ]); - const handleInput = useCallback((e: ChangeEvent) => { - e.stopPropagation(); - const input = e?.target?.value; - setSearchInput(input); - }, []); + const handleInput = debounce( + (value: string) => setSearchInput(value), + FAST_DEBOUNCE, + ); const filteredColumns = useMemo( () => @@ -182,11 +228,9 @@ export const DrillByMenuItems = ({ filteredColumns.length || 1, submenuIndex, MAX_SUBMENU_HEIGHT, - columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD - ? SEARCH_INPUT_HEIGHT - : 0, + showSearch ? SEARCH_INPUT_HEIGHT : 0, ), - [contextMenuY, filteredColumns.length, submenuIndex, columns.length], + [contextMenuY, filteredColumns.length, submenuIndex, showSearch], ); let tooltip: ReactNode; @@ -208,27 +252,53 @@ export const DrillByMenuItems = ({ ); } + const Row = ({ + index, + data, + style, + }: { + index: number; + data: { columns: Column[] }; + style: CSSProperties; + }) => { + const { columns, ...rest } = data; + const column = columns[index]; + return ( + handleSelection(e, column)} + style={style} + > + {column.verbose_name || column.column_name} + + ); + }; + return ( <>
- {columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD && ( + {showSearch && ( } - onChange={handleInput} + onChange={e => { + e.stopPropagation(); + handleInput(e.target.value); + }} placeholder={t('Search columns')} - value={searchInput} onClick={e => { // prevent closing menu when clicking on input e.nativeEvent.stopImmediatePropagation(); @@ -253,20 +323,23 @@ export const DrillByMenuItems = ({ ) : filteredColumns.length ? (
- {filteredColumns.map(column => ( - handleSelection(e, column)} - > - {column.verbose_name || column.column_name} - - ))} + + {({ height, width }) => ( + + {Row} + + )} +
) : ( diff --git a/superset-frontend/src/components/Chart/MenuItemWithTruncation.tsx b/superset-frontend/src/components/Chart/MenuItemWithTruncation.tsx index fd371fb30a603..1ab3daf485572 100644 --- a/superset-frontend/src/components/Chart/MenuItemWithTruncation.tsx +++ b/superset-frontend/src/components/Chart/MenuItemWithTruncation.tsx @@ -17,7 +17,7 @@ * under the License. */ -import { ReactNode } from 'react'; +import { ReactNode, CSSProperties } from 'react'; import { css, truncationCSS, useCSSTextTruncation } from '@superset-ui/core'; import { Menu } from 'src/components/Menu'; import { Tooltip } from 'src/components/Tooltip'; @@ -27,6 +27,7 @@ export type MenuItemWithTruncationProps = { tooltipText: ReactNode; children: ReactNode; onClick?: MenuProps['onClick']; + style?: CSSProperties; }; export const MenuItemWithTruncation = ({ From e80de8dedeb80741213f7bcdd609d5c98943b7f1 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Thu, 13 Jun 2024 15:17:10 -0300 Subject: [PATCH 2/6] Fixes CI errors --- .../src/components/Chart/DrillBy/DrillByMenuItems.test.tsx | 1 + .../src/components/Chart/DrillBy/DrillByMenuItems.tsx | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx index 0f2dff8bce65f..4d88dd7abb007 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx @@ -68,6 +68,7 @@ const renderMenu = ({
, diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx index 635e2f7ea69be..71235fc4bc19d 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx @@ -327,7 +327,7 @@ export const DrillByMenuItems = ({ `} > - {({ height, width }) => ( + {({ height, width }: { height: number; width: number }) => ( Date: Thu, 13 Jun 2024 16:04:12 -0300 Subject: [PATCH 3/6] Fixes frontend tests --- .../Chart/DrillBy/DrillByMenuItems.test.tsx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx index 4d88dd7abb007..b3ad5978edd01 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +import { ReactChild } from 'react'; import userEvent from '@testing-library/user-event'; import { Behavior, @@ -31,11 +32,24 @@ import { DrillByMenuItems, DrillByMenuItemsProps } from './DrillByMenuItems'; /* eslint jest/expect-expect: ["warn", { "assertFunctionNames": ["expect*"] }] */ -const DATASET_ENDPOINT = 'glob:*/api/v1/dataset/7'; +const DATASET_ENDPOINT = 'glob:*/api/v1/dataset/7*'; const CHART_DATA_ENDPOINT = 'glob:*/api/v1/chart/data*'; const FORM_DATA_KEY_ENDPOINT = 'glob:*/api/v1/explore/form_data'; const { form_data: defaultFormData } = chartQueries[sliceId]; +jest.mock( + 'react-virtualized-auto-sizer', + () => + ({ children }: { children: (params: { height: number }) => ReactChild }) => + children({ height: 500 }), +); + +jest.mock('lodash/debounce', () => (fn: Function & { debounce: Function }) => { + // eslint-disable-next-line no-param-reassign + fn.debounce = jest.fn(); + return fn; +}); + const defaultColumns = [ { column_name: 'col1', groupby: true }, { column_name: 'col2', groupby: true }, From 8a13ac8d6071b9694e4201520679ccdddb9db511 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Fri, 14 Jun 2024 15:58:23 -0300 Subject: [PATCH 4/6] Apply explicit number conversion Co-authored-by: JUST.in DO IT --- .../src/components/Chart/DrillBy/DrillByMenuItems.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx index 71235fc4bc19d..2a02131f95171 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx @@ -158,7 +158,7 @@ export const DrillByMenuItems = ({ useEffect(() => { async function loadOptions() { - const datasetId = +formData.datasource.split('__')[0]; + const datasetId = Number(formData.datasource.split('__')[0]); try { setIsLoadingColumns(true); let response: JsonResponse; From 3d1f4be74646cc942dcfad2e058562b2ed312fc6 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Fri, 14 Jun 2024 16:11:15 -0300 Subject: [PATCH 5/6] Removes auto sizer --- .../Chart/DrillBy/DrillByMenuItems.test.tsx | 8 ----- .../Chart/DrillBy/DrillByMenuItems.tsx | 33 +++++++------------ 2 files changed, 11 insertions(+), 30 deletions(-) diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx index b3ad5978edd01..8b65d214094b6 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { ReactChild } from 'react'; import userEvent from '@testing-library/user-event'; import { Behavior, @@ -37,13 +36,6 @@ const CHART_DATA_ENDPOINT = 'glob:*/api/v1/chart/data*'; const FORM_DATA_KEY_ENDPOINT = 'glob:*/api/v1/explore/form_data'; const { form_data: defaultFormData } = chartQueries[sliceId]; -jest.mock( - 'react-virtualized-auto-sizer', - () => - ({ children }: { children: (params: { height: number }) => ReactChild }) => - children({ height: 500 }), -); - jest.mock('lodash/debounce', () => (fn: Function & { debounce: Function }) => { // eslint-disable-next-line no-param-reassign fn.debounce = jest.fn(); diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx index 2a02131f95171..b0df1b72611bf 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx @@ -44,7 +44,6 @@ import { } from '@superset-ui/core'; import rison from 'rison'; import { debounce } from 'lodash'; -import AutoSizer from 'react-virtualized-auto-sizer'; import { FixedSizeList as List } from 'react-window'; import { AntdInput } from 'src/components'; import Icons from 'src/components/Icons'; @@ -62,7 +61,7 @@ import { getSubmenuYOffset } from '../utils'; import { MenuItemWithTruncation } from '../MenuItemWithTruncation'; import { Dataset } from '../types'; -const MAX_SUBMENU_HEIGHT = 200; +const SUBMENU_HEIGHT = 200; const SHOW_COLUMNS_SEARCH_THRESHOLD = 10; const SEARCH_INPUT_HEIGHT = 48; @@ -227,7 +226,7 @@ export const DrillByMenuItems = ({ contextMenuY, filteredColumns.length || 1, submenuIndex, - MAX_SUBMENU_HEIGHT, + SUBMENU_HEIGHT, showSearch ? SEARCH_INPUT_HEIGHT : 0, ), [contextMenuY, filteredColumns.length, submenuIndex, showSearch], @@ -321,26 +320,16 @@ export const DrillByMenuItems = ({ ) : filteredColumns.length ? ( -
- - {({ height, width }: { height: number; width: number }) => ( - - {Row} - - )} - -
+ {Row} + ) : ( {t('No columns found')} From fe4fa93cc77aca3e23f2a64d291179491853d4fb Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Fri, 14 Jun 2024 16:16:40 -0300 Subject: [PATCH 6/6] Reduces item size --- .../src/components/Chart/DrillBy/DrillByMenuItems.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx index b0df1b72611bf..6df180cb78cac 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx @@ -323,7 +323,7 @@ export const DrillByMenuItems = ({