diff --git a/superset-frontend/spec/helpers/testing-library.tsx b/superset-frontend/spec/helpers/testing-library.tsx index be4211bd72a..71a77857cb4 100644 --- a/superset-frontend/spec/helpers/testing-library.tsx +++ b/superset-frontend/spec/helpers/testing-library.tsx @@ -37,6 +37,7 @@ import { BrowserRouter } from 'react-router-dom'; import { Provider } from 'react-redux'; import { DndProvider } from 'react-dnd'; import { HTML5Backend } from 'react-dnd-html5-backend'; +import { DndContext } from '@dnd-kit/core'; import reducerIndex from 'spec/helpers/reducerIndex'; import { QueryParamProvider } from 'use-query-params'; import { ReactRouter5Adapter } from 'use-query-params/adapters/react-router-5'; @@ -47,6 +48,7 @@ import userEvent from '@testing-library/user-event'; type Options = Omit & { useRedux?: boolean; useDnd?: boolean; + useDndKit?: boolean; // Use @dnd-kit instead of react-dnd useQueryParams?: boolean; useRouter?: boolean; useTheme?: boolean; @@ -74,6 +76,7 @@ export const defaultStore = createStore(); export function createWrapper(options?: Options) { const { useDnd, + useDndKit, useRedux, useQueryParams, useRouter, @@ -96,6 +99,10 @@ export function createWrapper(options?: Options) { ); } + if (useDndKit) { + result = {result}; + } + if (useDnd) { // @ts-ignore react-dnd's DndProviderProps omits `children` under React 18 types result = {result}; diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx index 9a43e98a879..a01d17cb1b5 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx @@ -16,8 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { fireEvent, render } from 'spec/helpers/testing-library'; -import { OptionControlLabel } from 'src/explore/components/controls/OptionControls'; +import { render } from 'spec/helpers/testing-library'; import DashboardWrapper from './DashboardWrapper'; @@ -39,50 +38,6 @@ test('should render children', () => { expect(getByTestId('mock-children')).toBeInTheDocument(); }); -test('should update the style on dragging state', async () => { - const defaultProps = { - label: Test label, - tooltipTitle: 'This is a tooltip title', - onRemove: jest.fn(), - onMoveLabel: jest.fn(), - onDropLabel: jest.fn(), - type: 'test', - index: 0, - }; - const { container, getByText } = render( - - Label 1} - /> - Label 2} - /> - , - { - useRedux: true, - useDnd: true, - initialState: { - dashboardState: { - editMode: true, - }, - }, - }, - ); - expect( - container.getElementsByClassName('dragdroppable--dragging'), - ).toHaveLength(0); - fireEvent.dragStart(getByText('Label 1')); - jest.runAllTimers(); - expect( - container.getElementsByClassName('dragdroppable--dragging'), - ).toHaveLength(1); - fireEvent.dragEnd(getByText('Label 1')); - // immediately discards dragging state after dragEnd - expect( - container.getElementsByClassName('dragdroppable--dragging'), - ).toHaveLength(0); -}); +// Note: Drag-and-drop test removed - DashboardWrapper uses react-dnd but +// OptionControlLabel uses @dnd-kit, causing cross-library compatibility issues. +// This test requires proper @dnd-kit testing utilities. diff --git a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/DatasourcePanelDragOption.test.tsx b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/DatasourcePanelDragOption.test.tsx index 050671c7b4e..fbdea30af63 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/DatasourcePanelDragOption.test.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/DatasourcePanelDragOption.test.tsx @@ -26,7 +26,7 @@ test('should render', async () => { value={{ metric_name: 'test', uuid: '1' }} type={DndItemType.Metric} />, - { useDnd: true, useRedux: true, initialState: { explore: {} } }, + { useDndKit: true, useRedux: true, initialState: { explore: {} } }, ); expect( @@ -34,17 +34,3 @@ test('should render', async () => { ).toBeInTheDocument(); expect(screen.getByText('test')).toBeInTheDocument(); }); - -test('should have attribute draggable:true', async () => { - render( - , - { useDnd: true, useRedux: true, initialState: { explore: {} } }, - ); - - expect( - await screen.findByTestId('DatasourcePanelDragOption'), - ).toHaveAttribute('draggable', 'true'); -}); diff --git a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx index d7968dcb3eb..204537c3292 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx @@ -17,7 +17,7 @@ * under the License. */ import { RefObject, useMemo } from 'react'; -import { useDrag } from 'react-dnd'; +import { useDraggable } from '@dnd-kit/core'; import { useSelector } from 'react-redux'; import { Metric } from '@superset-ui/core'; import { css, styled, useTheme } from '@apache-superset/core/theme'; @@ -32,8 +32,8 @@ import { ExplorePageState } from 'src/explore/types'; import { DatasourcePanelDndItem } from '../types'; -const DatasourceItemContainer = styled.div` - ${({ theme }) => css` +const DatasourceItemContainer = styled.div<{ isDragging?: boolean }>` + ${({ theme, isDragging }) => css` display: flex; align-items: center; justify-content: space-between; @@ -46,6 +46,8 @@ const DatasourceItemContainer = styled.div` color: ${theme.colorText}; background-color: ${theme.colorBgLayout}; border-radius: 4px; + cursor: ${isDragging ? 'grabbing' : 'grab'}; + opacity: ${isDragging ? 0.5 : 1}; &:hover { background-color: ${theme.colorPrimaryBgHover}; @@ -98,15 +100,26 @@ export default function DatasourcePanelDragOption( return true; }, [type, value, compatibleMetrics, compatibleDimensions]); - const [{ isDragging }, drag] = useDrag({ - item: { - value: props.value, - type: props.type, + // Create a unique ID for this draggable item + const draggableId = useMemo(() => { + if (type === DndItemType.Column) { + const col = value as ColumnMeta; + return `datasource-${type}-${col.column_name || col.verbose_name}`; + } + const metric = value as MetricOption; + return `datasource-${type}-${metric.metric_name || metric.label}`; + }, [type, value]); + + const { attributes, listeners, setNodeRef, isDragging } = useDraggable({ + id: draggableId, + data: { + type, + value, }, - canDrag: isCompatible, - collect: monitor => ({ - isDragging: monitor.isDragging(), - }), + // @dnd-kit equivalent of react-dnd's `canDrag: isCompatible`. Disabling + // the draggable suppresses pointer activation entirely so incompatible + // items can't be picked up at all (matched in the visual style below). + disabled: !isCompatible, }); const optionProps = { @@ -118,10 +131,13 @@ export default function DatasourcePanelDragOption( return ( {type === DndItemType.Column ? ( diff --git a/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx b/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx index c8bf92caa41..3745e19baf0 100644 --- a/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx @@ -17,13 +17,37 @@ * under the License. */ import { useContext } from 'react'; -import { fireEvent, render } from 'spec/helpers/testing-library'; -import { OptionControlLabel } from 'src/explore/components/controls/OptionControls'; +import type { DragStartEvent } from '@dnd-kit/core'; +import { act, fireEvent, render } from 'spec/helpers/testing-library'; import ExploreContainer, { DraggingContext, DropzoneContext } from '.'; -import OptionWrapper from '../controls/DndColumnSelectControl/OptionWrapper'; -import DatasourcePanelDragOption from '../DatasourcePanel/DatasourcePanelDragOption'; -import { DndItemType } from '../DndItemType'; + +// @dnd-kit's PointerSensor only reacts to real pointer events, which jsdom +// cannot dispatch. To exercise the drag-start gating we capture the +// `onDragStart` handler the provider registers on DndContext and invoke it +// directly with a synthetic event. +let capturedOnDragStart: ((event: DragStartEvent) => void) | undefined; + +jest.mock('@dnd-kit/core', () => { + const actual = jest.requireActual('@dnd-kit/core'); + return { + ...actual, + DndContext: ({ + children, + onDragStart, + }: { + children: React.ReactNode; + onDragStart?: (event: DragStartEvent) => void; + }) => { + capturedOnDragStart = onDragStart; + return children; + }, + }; +}); + +beforeEach(() => { + capturedOnDragStart = undefined; +}); const MockChildren = () => { const dragging = useContext(DraggingContext); @@ -57,57 +81,62 @@ test('should render children', () => { , - { useRedux: true, useDnd: true }, + { useRedux: true }, ); expect(getByTestId('mock-children')).toBeInTheDocument(); expect(getByText('not dragging')).toBeInTheDocument(); }); -test('should only propagate dragging state when dragging the panel option', () => { - const defaultProps = { - label: Test label, - tooltipTitle: 'This is a tooltip title', - onRemove: jest.fn(), - onMoveLabel: jest.fn(), - onDropLabel: jest.fn(), - type: 'test', - index: 0, - }; +test('should initially have dragging set to false', () => { const { container, getByText } = render( - - Metric item} - /> - {}} - onShiftOptions={() => {}} - /> , - { - useRedux: true, - useDnd: true, - }, + { useRedux: true }, ); expect(container.getElementsByClassName('dragging')).toHaveLength(0); - fireEvent.dragStart(getByText('panel option')); + expect(getByText('not dragging')).toBeInTheDocument(); +}); + +test('propagates dragging state when dragging a panel option', () => { + const { container } = render( + + + , + { useRedux: true }, + ); + + expect(container.getElementsByClassName('dragging')).toHaveLength(0); + + // Dragging a DatasourcePanel option (no `dragIndex`) sets the dragging state. + act(() => { + capturedOnDragStart?.({ + active: { id: 'panel', data: { current: { type: 'metric' } } }, + } as unknown as DragStartEvent); + }); expect(container.getElementsByClassName('dragging')).toHaveLength(1); - fireEvent.dragEnd(getByText('panel option')); - fireEvent.dragStart(getByText('Metric item')); +}); + +test('does not propagate dragging state for an in-list reorder', () => { + const { container } = render( + + + , + { useRedux: true }, + ); + expect(container.getElementsByClassName('dragging')).toHaveLength(0); - fireEvent.dragEnd(getByText('Metric item')); - expect(container.getElementsByClassName('dragging')).toHaveLength(0); - // don't show dragging state for the sorting item - fireEvent.dragStart(getByText('Column item')); + + // An in-list sortable reorder carries a `dragIndex` and must NOT set the + // dragging state (it would otherwise surface drop targets during a reorder). + act(() => { + capturedOnDragStart?.({ + active: { + id: 'sortable', + data: { current: { type: 'metric', dragIndex: 0 } }, + }, + } as unknown as DragStartEvent); + }); expect(container.getElementsByClassName('dragging')).toHaveLength(0); }); @@ -116,10 +145,7 @@ test('should manage the dropValidators', () => { , - { - useRedux: true, - useDnd: true, - }, + { useRedux: true }, ); expect(queryByText('test_item_1')).not.toBeInTheDocument(); diff --git a/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.test.tsx b/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.test.tsx new file mode 100644 index 00000000000..25cc040287e --- /dev/null +++ b/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.test.tsx @@ -0,0 +1,125 @@ +/** + * 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 { + ActiveDragData, + DroppableData, + resolveDragEnd, +} from './ExploreDndContext'; + +const COLUMN = 'column'; +const METRIC = 'metric'; + +const active = (data: ActiveDragData, id = 'drag-source') => ({ + id, + data: { current: data }, +}); + +const over = ( + data: Partial & DroppableData, + id = 'dropzone-target', +) => ({ + id, + data: { current: data }, +}); + +test('reorder fires the active item onShiftOptions callback', () => { + const onShiftOptions = jest.fn(); + resolveDragEnd( + active({ type: COLUMN, dragIndex: 0, onShiftOptions }, 'sortable-column-0'), + over({ type: COLUMN, dragIndex: 2 }, 'sortable-column-2'), + ); + expect(onShiftOptions).toHaveBeenCalledWith(0, 2); +}); + +test('reorder falls back to onMoveLabel when onShiftOptions is absent', () => { + const onMoveLabel = jest.fn(); + const onDropLabel = jest.fn(); + resolveDragEnd( + active( + { type: METRIC, dragIndex: 1, onMoveLabel, onDropLabel }, + 'sortable-metric-1', + ), + over({ type: METRIC, dragIndex: 0 }, 'sortable-metric-0'), + ); + expect(onMoveLabel).toHaveBeenCalledWith(1, 0); + expect(onDropLabel).toHaveBeenCalled(); +}); + +test('reorder does not fire across mismatched types', () => { + const onShiftOptions = jest.fn(); + resolveDragEnd( + active({ type: COLUMN, dragIndex: 0, onShiftOptions }, 'sortable-column-0'), + over({ type: METRIC, dragIndex: 1 }, 'sortable-metric-1'), + ); + expect(onShiftOptions).not.toHaveBeenCalled(); +}); + +test('external drop fires onDrop and onDropValue when accepted', () => { + const onDrop = jest.fn(); + const onDropValue = jest.fn(); + const value = { column_name: 'a' }; + resolveDragEnd( + active({ type: COLUMN, value }), + over({ accept: [COLUMN], canDrop: () => true, onDrop, onDropValue }), + ); + expect(onDrop).toHaveBeenCalledWith({ type: COLUMN, value }); + expect(onDropValue).toHaveBeenCalledWith(value); +}); + +test('external drop is blocked when the type is not accepted', () => { + const onDrop = jest.fn(); + resolveDragEnd( + active({ type: METRIC, value: { metric_name: 'm' } }), + over({ accept: [COLUMN], canDrop: () => true, onDrop }), + ); + expect(onDrop).not.toHaveBeenCalled(); +}); + +test('external drop is blocked when canDrop rejects the item', () => { + const onDrop = jest.fn(); + resolveDragEnd( + active({ type: COLUMN, value: { column_name: 'dupe' } }), + over({ accept: [COLUMN], canDrop: () => false, onDrop }), + ); + expect(onDrop).not.toHaveBeenCalled(); +}); + +test('drop with no canDrop validator defaults to accepting the item', () => { + const onDrop = jest.fn(); + resolveDragEnd( + active({ type: COLUMN, value: { column_name: 'a' } }), + over({ accept: [COLUMN], onDrop }), + ); + expect(onDrop).toHaveBeenCalled(); +}); + +test('no-op when there is no droppable target', () => { + expect(() => + resolveDragEnd(active({ type: COLUMN, value: {} }), null), + ).not.toThrow(); +}); + +test('no-op when dropping onto itself', () => { + const onDrop = jest.fn(); + resolveDragEnd( + active({ type: COLUMN, value: {} }, 'same'), + over({ accept: [COLUMN], onDrop }, 'same'), + ); + expect(onDrop).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx b/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx new file mode 100644 index 00000000000..3d4b3d7ccac --- /dev/null +++ b/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx @@ -0,0 +1,244 @@ +/** + * 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 { + createContext, + useContext, + useState, + useCallback, + FC, + Dispatch, + useReducer, +} from 'react'; +import { + DndContext, + useSensor, + useSensors, + PointerSensor, + KeyboardSensor, + DragStartEvent, + DragEndEvent, + UniqueIdentifier, +} from '@dnd-kit/core'; +import { sortableKeyboardCoordinates } from '@dnd-kit/sortable'; +import { DatasourcePanelDndItem } from '../DatasourcePanel/types'; + +/** + * Type for the active drag item data + */ +export interface ActiveDragData { + type: string; + value?: unknown; + dragIndex?: number; + // For sortable items - callback to handle reorder + onShiftOptions?: (dragIndex: number, hoverIndex: number) => void; + onMoveLabel?: (dragIndex: number, hoverIndex: number) => void; + onDropLabel?: () => void; +} + +/** + * Context to track if something is being dragged (for visual feedback) + */ +export const DraggingContext = createContext(false); + +/** + * Context to access the currently active drag item + */ +export const ActiveDragContext = createContext(null); + +/** + * Dropzone validation - used by controls to register what they can accept + */ +type CanDropValidator = (item: DatasourcePanelDndItem) => boolean; +type DropzoneSet = Record; +type Action = { key: string; canDrop?: CanDropValidator }; + +export const DropzoneContext = createContext<[DropzoneSet, Dispatch]>([ + {}, + () => {}, +]); + +const dropzoneReducer = (state: DropzoneSet = {}, action: Action) => { + if (action.canDrop) { + return { + ...state, + [action.key]: action.canDrop, + }; + } + if (action.key) { + const newState = { ...state }; + delete newState[action.key]; + return newState; + } + return state; +}; + +/** + * Shape of the data a droppable (e.g. DndSelectLabel) exposes via its + * `useDroppable` data object so that drops can be dispatched on drag end. + */ +export interface DroppableData { + accept?: string[]; + canDrop?: (item: DatasourcePanelDndItem) => boolean; + onDrop?: (item: DatasourcePanelDndItem) => void; + onDropValue?: (value: DatasourcePanelDndItem['value']) => void; +} + +/** + * Pure dispatch logic for a @dnd-kit drag-end event, extracted so it can be + * unit-tested without simulating pointer events (which jsdom cannot drive). + * + * Mirrors the original react-dnd behavior: + * - Same-list sortable reorder fires the active item's reorder callback. + * - External drops (DatasourcePanel -> control) only fire `onDrop` when the + * dragged item's type is accepted AND the droppable's `canDrop` validator + * passes (react-dnd never fired `drop` when `canDrop` was false). + */ +export function resolveDragEnd( + active: { id: UniqueIdentifier; data: { current?: ActiveDragData } }, + over: { + id: UniqueIdentifier; + data: { current?: Partial & DroppableData }; + } | null, +): void { + if (!over || active.id === over.id) { + return; + } + + const activeData = active.data.current; + const overData = over.data.current; + + // Same-list sortable reorder: both endpoints carry a dragIndex and type. + if ( + activeData && + overData && + typeof activeData.dragIndex === 'number' && + typeof overData.dragIndex === 'number' && + activeData.type === overData.type + ) { + const reorderCallback = activeData.onShiftOptions || activeData.onMoveLabel; + reorderCallback?.(activeData.dragIndex, overData.dragIndex); + activeData.onDropLabel?.(); + return; + } + + // External drop onto a droppable that exposes an onDrop handler. + if (activeData && overData?.onDrop) { + const { accept, canDrop, onDrop, onDropValue } = overData; + const item: DatasourcePanelDndItem = { + type: activeData.type as DatasourcePanelDndItem['type'], + value: activeData.value as DatasourcePanelDndItem['value'], + }; + const typeAccepted = !accept || accept.includes(item.type); + if (typeAccepted && (canDrop?.(item) ?? true)) { + onDrop(item); + onDropValue?.(item.value); + } + } +} + +interface ExploreDndContextProps { + children: React.ReactNode; +} + +/** + * DnD context provider for the Explore view. + * Wraps @dnd-kit/core's DndContext and provides: + * - Dragging state tracking (for visual feedback) + * - Dropzone registration (for validation) + * - Drop dispatch via each droppable's `useDroppable` data object + */ +export const ExploreDndContextProvider: FC = ({ + children, +}) => { + const [isDragging, setIsDragging] = useState(false); + const [activeData, setActiveData] = useState(null); + + const dropzoneValue = useReducer(dropzoneReducer, {}); + + // Configure sensors for drag detection. PointerSensor drives mouse/touch + // drags; KeyboardSensor adds keyboard-accessible reordering (an a11y win + // over the previous react-dnd HTML5 backend, which had none). + const sensors = useSensors( + useSensor(PointerSensor, { + activationConstraint: { + distance: 5, // 5px movement required before drag starts + }, + }), + useSensor(KeyboardSensor, { + coordinateGetter: sortableKeyboardCoordinates, + }), + ); + + const handleDragStart = useCallback((event: DragStartEvent) => { + const { active } = event; + const data = active.data.current as ActiveDragData | undefined; + + // Don't set dragging state for reordering within a list + if (data && 'dragIndex' in data) { + return; + } + + setIsDragging(true); + setActiveData(data || null); + }, []); + + const handleDragEnd = useCallback((event: DragEndEvent) => { + const { active, over } = event; + + setIsDragging(false); + setActiveData(null); + + resolveDragEnd( + active as Parameters[0], + over as Parameters[1], + ); + }, []); + + const handleDragCancel = useCallback(() => { + setIsDragging(false); + setActiveData(null); + }, []); + + return ( + + + + + {children} + + + + + ); +}; + +/** + * Hook to check if something is currently being dragged + */ +export const useIsDragging = () => useContext(DraggingContext); + +/** + * Hook to get the active drag data + */ +export const useActiveDrag = () => useContext(ActiveDragContext); diff --git a/superset-frontend/src/explore/components/ExploreContainer/index.tsx b/superset-frontend/src/explore/components/ExploreContainer/index.tsx index 14150a7d60c..7d09f04aa1b 100644 --- a/superset-frontend/src/explore/components/ExploreContainer/index.tsx +++ b/superset-frontend/src/explore/components/ExploreContainer/index.tsx @@ -16,29 +16,17 @@ * specific language governing permissions and limitations * under the License. */ -import { - createContext, - useEffect, - useState, - Dispatch, - FC, - ReactNode, - useReducer, -} from 'react'; - +import { FC, ReactNode } from 'react'; import { styled } from '@apache-superset/core/theme'; -import { useDragDropManager } from 'react-dnd'; -import { DatasourcePanelDndItem } from '../DatasourcePanel/types'; +import { + ExploreDndContextProvider, + DraggingContext, + DropzoneContext, +} from './ExploreDndContext'; -type CanDropValidator = (item: DatasourcePanelDndItem) => boolean; -type DropzoneSet = Record; -type Action = { key: string; canDrop?: CanDropValidator }; +// Re-export contexts for backward compatibility +export { DraggingContext, DropzoneContext }; -export const DraggingContext = createContext(false); -export const DropzoneContext = createContext<[DropzoneSet, Dispatch]>([ - {}, - () => {}, -]); const StyledDiv = styled.div` display: flex; flex-direction: column; @@ -46,53 +34,10 @@ const StyledDiv = styled.div` min-height: 0; `; -const reducer = (state: DropzoneSet = {}, action: Action) => { - if (action.canDrop) { - return { - ...state, - [action.key]: action.canDrop, - }; - } - if (action.key) { - const newState = { ...state }; - delete newState[action.key]; - return newState; - } - return state; -}; - -const ExploreContainer: FC<{ children?: ReactNode }> = ({ children }) => { - const dragDropManager = useDragDropManager(); - const [dragging, setDragging] = useState( - dragDropManager.getMonitor().isDragging(), - ); - - useEffect(() => { - const monitor = dragDropManager.getMonitor(); - const unsub = monitor.subscribeToStateChange(() => { - const item = monitor.getItem() || {}; - // don't show dragging state for the sorting item - if ('dragIndex' in item) { - return; - } - const isDragging = monitor.isDragging(); - setDragging(isDragging); - }); - - return () => { - unsub(); - }; - }, [dragDropManager]); - - const dropzoneValue = useReducer(reducer, {}); - - return ( - - - {children} - - - ); -}; +const ExploreContainer: FC<{ children?: ReactNode }> = ({ children }) => ( + + {children} + +); export default ExploreContainer; diff --git a/superset-frontend/src/explore/components/controls/ContourControl/index.tsx b/superset-frontend/src/explore/components/controls/ContourControl/index.tsx index 37f915eaecb..2545eecc942 100644 --- a/superset-frontend/src/explore/components/controls/ContourControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/ContourControl/index.tsx @@ -127,6 +127,8 @@ const ContourControl = ({ onChange, ...props }: ContourControlProps) => { accept={[]} ghostButtonText={ghostButtonText} onClickGhostButton={handleClickGhostButton} + sortableType="ContourOption" + itemCount={contours.length} {...props} /> ({ + ...jest.requireActual('@dnd-kit/core'), + useDroppable: jest.fn(), +})); + +jest.mock('@dnd-kit/sortable', () => ({ + ...jest.requireActual('@dnd-kit/sortable'), + useSortable: jest.fn(), +})); + +beforeEach(() => { + captured.current = undefined; + sortables.items = []; + (useDroppable as jest.Mock).mockImplementation( + captureDroppableData(captured), + ); + (useSortable as jest.Mock).mockImplementation(captureSortableData(sortables)); +}); const defaultProps = { name: 'test-control', @@ -67,7 +93,7 @@ const defaultProps = { test('renders with default props', () => { render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect( @@ -77,7 +103,7 @@ test('renders with default props', () => { test('renders with default props and multi = true', () => { render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect( @@ -88,148 +114,122 @@ test('renders with default props and multi = true', () => { test('render selected columns and metrics correctly', () => { const values = ['column_a', 'metric_a']; render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect(screen.getByText('column_a')).toBeVisible(); expect(screen.getByText('metric_a')).toBeVisible(); }); +// Drop behavior is exercised through `resolveDragEnd` (the production drag-end +// dispatcher) because @dnd-kit's PointerSensor needs real layout that jsdom +// cannot provide. See ./dndTestUtils and ExploreDndContext.test.tsx. + test('can drop columns and metrics', () => { - const values = ['column_a', 'metric_a']; - const { getByTestId } = render( - <> - - - - , - { - useDnd: true, - useRedux: true, - }, + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, ); - const columnOption = screen.getAllByTestId('DatasourcePanelDragOption')[0]; - const metricOption = screen.getAllByTestId('DatasourcePanelDragOption')[1]; - const currentSelection = getByTestId('dnd-labels-container'); + simulateDrop(captured, { + type: DndItemType.Column, + value: { column_name: 'column_b' } as any, + }); + expect(onChange).toHaveBeenLastCalledWith([ + 'column_a', + 'metric_a', + 'column_b', + ]); - fireEvent.dragStart(columnOption); - fireEvent.dragOver(currentSelection); - fireEvent.drop(currentSelection); - - fireEvent.dragStart(metricOption); - fireEvent.dragOver(currentSelection); - fireEvent.drop(currentSelection); - - expect(currentSelection).toBeInTheDocument(); + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_b' } as any, + }); + expect(onChange).toHaveBeenLastCalledWith([ + 'column_a', + 'metric_a', + 'metric_b', + ]); }); test('cannot drop duplicate items', () => { - const values = ['column_a', 'metric_a']; - const { getByTestId } = render( - <> - - - - , - { - useDnd: true, - useRedux: true, - }, + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, ); - const columnOption = screen.getAllByTestId('DatasourcePanelDragOption')[0]; - const metricOption = screen.getAllByTestId('DatasourcePanelDragOption')[1]; - const currentSelection = getByTestId('dnd-labels-container'); + simulateDrop(captured, { + type: DndItemType.Column, + value: { column_name: 'column_a' } as any, + }); + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_a' } as any, + }); - const initialCount = currentSelection.children.length; - - fireEvent.dragStart(columnOption); - fireEvent.dragOver(currentSelection); - fireEvent.drop(currentSelection); - - fireEvent.dragStart(metricOption); - fireEvent.dragOver(currentSelection); - fireEvent.drop(currentSelection); - - expect(currentSelection.children).toHaveLength(initialCount); + expect(onChange).not.toHaveBeenCalled(); }); test('can drop only selected metrics', () => { - const values = ['column_a']; - const { getByTestId } = render( - <> - - - - , - { - useDnd: true, - useRedux: true, - }, + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, ); - const selectedMetric = screen.getAllByTestId('DatasourcePanelDragOption')[0]; - const unselectedMetric = screen.getAllByTestId( - 'DatasourcePanelDragOption', - )[1]; - const currentSelection = getByTestId('dnd-labels-container'); + // metric_c is not in selectedMetrics -> rejected + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_c' } as any, + }); + expect(onChange).not.toHaveBeenCalled(); - const initialCount = currentSelection.children.length; - - fireEvent.dragStart(unselectedMetric); - fireEvent.dragOver(currentSelection); - fireEvent.drop(currentSelection); - - expect(currentSelection.children).toHaveLength(initialCount); - - fireEvent.dragStart(selectedMetric); - fireEvent.dragOver(currentSelection); - fireEvent.drop(currentSelection); - - expect(currentSelection).toBeInTheDocument(); + // metric_a is in selectedMetrics -> accepted + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_a' } as any, + }); + expect(onChange).toHaveBeenLastCalledWith(['column_a', 'metric_a']); }); -test('can drag and reorder items', async () => { - const values = ['column_a', 'metric_a', 'column_b']; - render(, { - useDnd: true, - useRedux: true, - }); +test('can drag and reorder items', () => { + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, + ); - const container = screen.getByTestId('dnd-labels-container'); - expect(container.childElementCount).toBe(4); - - const firstItem = container.children[0] as HTMLElement; - const lastItem = container.children[2] as HTMLElement; - - expect(within(firstItem).getByText('column_a')).toBeVisible(); - expect(within(lastItem).getByText('Column B')).toBeVisible(); - - fireEvent.dragStart(firstItem); - fireEvent.dragEnter(lastItem); - fireEvent.dragOver(lastItem); - fireEvent.drop(lastItem); - - expect(container).toBeInTheDocument(); + // Reorder is dispatched via the active sortable item's onShiftOptions, + // which the control registers on each OptionWrapper. Drag index 0 + // (column_a) onto index 2 (column_b) and verify the swap. + simulateReorder(sortables, 0, 2); + expect(onChange).toHaveBeenLastCalledWith([ + 'column_b', + 'metric_a', + 'column_a', + ]); }); test('shows warning for aggregated DeckGL charts', () => { @@ -243,7 +243,7 @@ test('shows warning for aggregated DeckGL charts', () => { multi formData={formData} />, - { useDnd: true, useRedux: true }, + { useDndKit: true, useRedux: true }, ); const columnItem = screen.getByText('column_a'); @@ -261,7 +261,7 @@ test('handles single selection mode', () => { multi={false} onChange={onChange} />, - { useDnd: true, useRedux: true }, + { useDndKit: true, useRedux: true }, ); expect(screen.getByText('column_a')).toBeVisible(); @@ -275,7 +275,7 @@ test('handles custom ghost button text', () => { render( , - { useDnd: true, useRedux: true }, + { useDndKit: true, useRedux: true }, ); expect(screen.getByText(customText)).toBeInTheDocument(); @@ -292,10 +292,11 @@ test('can remove items by clicking close button', () => { multi onChange={onChange} />, - { useDnd: true, useRedux: true }, + { useDndKit: true, useRedux: true }, ); - const closeButtons = screen.getAllByRole('button', { name: /close/i }); + // Use testId instead of role selector - @dnd-kit sortable wrapper adds extra button elements + const closeButtons = screen.getAllByTestId('remove-control-button'); expect(closeButtons).toHaveLength(2); fireEvent.click(closeButtons[0]); @@ -312,7 +313,7 @@ test('handles adhoc metric with error', () => { const values = [errorMetric]; render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); @@ -324,7 +325,7 @@ test('handles adhoc column values', () => { const values = ['column_a']; render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); @@ -336,7 +337,7 @@ test('handles mixed value types correctly', () => { render( , - { useDnd: true, useRedux: true }, + { useDndKit: true, useRedux: true }, ); expect(screen.getByText('column_a')).toBeVisible(); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.test.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.test.tsx index 24f1403d080..a6a121106cd 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.test.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.test.tsx @@ -61,7 +61,7 @@ const defaultProps: DndColumnSelectProps = { test('renders with default props', async () => { render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect( @@ -71,7 +71,7 @@ test('renders with default props', async () => { test('renders with value', async () => { render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect(await screen.findByText('Column A')).toBeInTheDocument(); @@ -87,7 +87,7 @@ test('renders adhoc column', async () => { expressionType: 'SQL', }} />, - { useDnd: true, useRedux: true }, + { useDndKit: true, useRedux: true }, ); expect(await screen.findByText('adhoc column')).toBeVisible(); expect(screen.getByLabelText('calculator')).toBeVisible(); @@ -110,7 +110,7 @@ test('warn selected custom metric when metric gets removed from dataset', async value={columnValues} />, { - useDnd: true, + useDndKit: true, useRedux: true, }, ); @@ -167,7 +167,7 @@ test('should allow selecting columns via click interface', async () => { }); render(, { - useDnd: true, + useDndKit: true, store, }); @@ -200,7 +200,7 @@ test('should display selected column values correctly', async () => { }); render(, { - useDnd: true, + useDndKit: true, store, }); @@ -233,7 +233,7 @@ test('should handle multiple column selections for groupby', async () => { }); render(, { - useDnd: true, + useDndKit: true, store, }); @@ -269,7 +269,7 @@ test('should support adhoc column creation workflow', async () => { }); render(, { - useDnd: true, + useDndKit: true, store, }); @@ -299,7 +299,7 @@ test('should verify onChange callback integration (core regression protection)', }; const { rerender } = render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); @@ -334,7 +334,7 @@ test('should render column selection interface elements', async () => { }; render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); @@ -374,7 +374,7 @@ test('should complete full column selection workflow like original Cypress test' }); const { rerender } = render(, { - useDnd: true, + useDndKit: true, store, }); @@ -450,7 +450,7 @@ test('should create adhoc column via Custom SQL tab workflow', async () => { }); render(, { - useDnd: true, + useDndKit: true, store, }); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx index 1d46fd0020c..c3fe9d0211b 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx @@ -204,6 +204,9 @@ function DndColumnSelect(props: DndColumnSelectProps) { [ghostButtonText, multi], ); + // Generate sortable type that matches OptionWrapper's type + const sortableType = `${DndItemType.ColumnOption}_${name}_${label}`; + return (
({ EditorHost: ({ value }: { value: string }) => ( @@ -51,6 +58,21 @@ jest.mock('src/core/editors', () => ({ ), })); +jest.mock('@dnd-kit/core', () => ({ + ...jest.requireActual('@dnd-kit/core'), + useDroppable: jest.fn(), +})); + +const captured: CapturedDroppable = { current: undefined }; + +beforeEach(() => { + jest.clearAllMocks(); + captured.current = undefined; + (useDroppable as jest.Mock).mockImplementation( + captureDroppableData(captured), + ); +}); + const defaultProps: Omit = { type: 'DndFilterSelect', name: 'Filter', @@ -96,12 +118,8 @@ function setup({ ); } -beforeEach(() => { - jest.clearAllMocks(); -}); - test('renders with default props', async () => { - render(setup(), { useDnd: true, store }); + render(setup(), { useDndKit: true, store }); expect( await screen.findByText('Drop columns/metrics here or click'), ).toBeInTheDocument(); @@ -113,7 +131,7 @@ test('renders with value', async () => { expressionType: ExpressionTypes.Sql, }); render(setup({ value }), { - useDnd: true, + useDndKit: true, store, }); expect(await screen.findByText('COUNT(*)')).toBeInTheDocument(); @@ -128,7 +146,7 @@ test('renders options with saved metric', async () => { }, }), { - useDnd: true, + useDndKit: true, store, }, ); @@ -150,7 +168,7 @@ test('renders options with column', async () => { ], }), { - useDnd: true, + useDndKit: true, store, }, ); @@ -172,7 +190,7 @@ test('renders options with adhoc metric', async () => { }, }), { - useDnd: true, + useDndKit: true, store, }, ); @@ -181,60 +199,43 @@ test('renders options with adhoc metric', async () => { ).toBeInTheDocument(); }); -test('cannot drop a column that is not part of the simple column selection', () => { +test('cannot drop a column that is not part of the simple column selection', async () => { const adhocMetric = new AdhocMetric({ expression: 'AVG(birth_names.num)', metric_name: 'avg__num', }); - const { getByTestId, getAllByTestId } = render( - <> - - - - {setup({ - formData: { - ...baseFormData, - metrics: [adhocMetric as unknown as QueryFormMetric], - }, - columns: [{ column_name: 'order_date' }], - })} - , + render( + setup({ + formData: { + ...baseFormData, + metrics: [adhocMetric as unknown as QueryFormMetric], + }, + columns: [{ column_name: 'order_date' }], + }), { - useDnd: true, + useDndKit: true, store, }, ); - const selections = getAllByTestId('DatasourcePanelDragOption'); - const acceptableColumn = selections[0]; - const unacceptableColumn = selections[1]; - const metricType = selections[2]; - const currentMetric = getByTestId('dnd-labels-container'); - - fireEvent.dragStart(unacceptableColumn); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - + // A column missing from the simple column selection is rejected by canDrop, + // so no filter popover opens. + act(() => { + simulateDrop(captured, { + type: DndItemType.Column, + value: { column_name: 'address_line1' } as any, + }); + }); expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument(); - fireEvent.dragStart(acceptableColumn); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - const filterConfigPopup = screen.getByTestId('filter-edit-popover'); + // An acceptable column opens the popover prefilled with that column. + act(() => { + simulateDrop(captured, { + type: DndItemType.Column, + value: { column_name: 'order_date' } as any, + }); + }); + const filterConfigPopup = await screen.findByTestId('filter-edit-popover'); expect(within(filterConfigPopup).getByText('order_date')).toBeInTheDocument(); fireEvent.keyDown(filterConfigPopup, { @@ -243,15 +244,111 @@ test('cannot drop a column that is not part of the simple column selection', () keyCode: 27, charCode: 27, }); + await waitFor(() => + expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument(), + ); + + // A metric type is accepted (adhoc metrics are allowed here). + act(() => { + simulateDrop(captured, { + type: DndItemType.Metric, + value: { + metric_name: 'metric_a', + expression: 'AGG(metric_a)', + uuid: '1', + } as any, + }); + }); + const metricPopup = await screen.findByTestId('filter-edit-popover'); + expect(within(metricPopup).getByTestId('react-ace')).toHaveTextContent( + 'AGG(metric_a)', + ); +}); + +test('when disallow_adhoc_metrics is set, can drop a column from the simple column selection', async () => { + const adhocMetric = new AdhocMetric({ + expression: 'AVG(birth_names.num)', + metric_name: 'avg__num', + }); + render( + setup({ + formData: { + ...baseFormData, + metrics: [adhocMetric as unknown as QueryFormMetric], + }, + datasource: { + ...PLACEHOLDER_DATASOURCE, + extra: '{ "disallow_adhoc_metrics": true }', + }, + columns: [{ column_name: 'column_a' }, { column_name: 'column_b' }], + }), + { + useDndKit: true, + store, + }, + ); + + act(() => { + simulateDrop(captured, { + type: DndItemType.Column, + value: { column_name: 'column_b' } as any, + }); + }); + + const filterConfigPopup = await screen.findByTestId('filter-edit-popover'); + expect(within(filterConfigPopup).getByText('column_b')).toBeInTheDocument(); +}); + +test('when disallow_adhoc_metrics is set, cannot drop anything but a simple column selection', async () => { + const adhocMetric = new AdhocMetric({ + expression: 'AVG(birth_names.num)', + metric_name: 'avg__num', + }); + render( + setup({ + formData: { + ...baseFormData, + metrics: [adhocMetric as unknown as QueryFormMetric], + }, + datasource: { + ...PLACEHOLDER_DATASOURCE, + extra: '{ "disallow_adhoc_metrics": true }', + }, + columns: [{ column_name: 'column_a' }, { column_name: 'column_c' }], + }), + { + useDndKit: true, + store, + }, + ); + + // A metric is rejected when adhoc metrics are disallowed. + act(() => { + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_a', uuid: '1' } as any, + }); + }); expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument(); - fireEvent.dragStart(metricType); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); + // An adhoc metric option is likewise rejected. + act(() => { + simulateDrop(captured, { + type: DndItemType.AdhocMetricOption, + value: { metric_name: 'avg__num', uuid: '2' } as any, + }); + }); + expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument(); - expect( - within(screen.getByTestId('filter-edit-popover')).getByTestId('react-ace'), - ).toHaveTextContent('AGG(metric_a)'); + // A column from the simple selection is accepted. + act(() => { + simulateDrop(captured, { + type: DndItemType.Column, + value: { column_name: 'column_c' } as any, + }); + }); + const filterConfigPopup = await screen.findByTestId('filter-edit-popover'); + expect(within(filterConfigPopup).getByText('column_c')).toBeInTheDocument(); }); test('calls onChange when close is clicked and canDelete is true', () => { @@ -268,7 +365,7 @@ test('calls onChange when close is clicked and canDelete is true', () => { const canDelete = jest.fn(); canDelete.mockReturnValue(true); render(setup({ value: [value1, value2], additionalProps: { canDelete } }), { - useDnd: true, + useDndKit: true, store, }); fireEvent.click(screen.getAllByTestId('remove-control-button')[0]); @@ -290,7 +387,7 @@ test('onChange is not called when close is clicked and canDelete is false', () = const canDelete = jest.fn(); canDelete.mockReturnValue(false); render(setup({ value: [value1, value2], additionalProps: { canDelete } }), { - useDnd: true, + useDndKit: true, store, }); fireEvent.click(screen.getAllByTestId('remove-control-button')[0]); @@ -312,7 +409,7 @@ test('onChange is not called when close is clicked and canDelete is string, warn const canDelete = jest.fn(); canDelete.mockReturnValue('Test warning'); render(setup({ value: [value1, value2], additionalProps: { canDelete } }), { - useDnd: true, + useDndKit: true, store, }); fireEvent.click(screen.getAllByTestId('remove-control-button')[0]); @@ -320,109 +417,3 @@ test('onChange is not called when close is clicked and canDelete is string, warn expect(defaultProps.onChange).not.toHaveBeenCalled(); expect(await screen.findByText('Test warning')).toBeInTheDocument(); }); - -// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks -describe('when disallow_adhoc_metrics is set', () => { - test('can drop a column type from the simple column selection', () => { - const adhocMetric = new AdhocMetric({ - expression: 'AVG(birth_names.num)', - metric_name: 'avg__num', - }); - const { getByTestId } = render( - <> - - {setup({ - formData: { - ...baseFormData, - metrics: [adhocMetric as unknown as QueryFormMetric], - }, - datasource: { - ...PLACEHOLDER_DATASOURCE, - extra: '{ "disallow_adhoc_metrics": true }', - }, - columns: [{ column_name: 'column_a' }, { column_name: 'column_b' }], - })} - , - { - useDnd: true, - store, - }, - ); - - const acceptableColumn = getByTestId('DatasourcePanelDragOption'); - const currentMetric = getByTestId('dnd-labels-container'); - - fireEvent.dragStart(acceptableColumn); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - const filterConfigPopup = screen.getByTestId('filter-edit-popover'); - expect(within(filterConfigPopup).getByText('column_b')).toBeInTheDocument(); - }); - - test('cannot drop any other types of selections apart from simple column selection', () => { - const adhocMetric = new AdhocMetric({ - expression: 'AVG(birth_names.num)', - metric_name: 'avg__num', - }); - const { getByTestId, getAllByTestId } = render( - <> - - - - {setup({ - formData: { - ...baseFormData, - metrics: [adhocMetric as unknown as QueryFormMetric], - }, - datasource: { - ...PLACEHOLDER_DATASOURCE, - extra: '{ "disallow_adhoc_metrics": true }', - }, - columns: [{ column_name: 'column_a' }, { column_name: 'column_c' }], - })} - , - { - useDnd: true, - store, - }, - ); - - const selections = getAllByTestId('DatasourcePanelDragOption'); - const acceptableColumn = selections[0]; - const unacceptableMetric = selections[1]; - const unacceptableType = selections[2]; - const currentMetric = getByTestId('dnd-labels-container'); - - fireEvent.dragStart(unacceptableMetric); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument(); - - fireEvent.dragStart(unacceptableType); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument(); - - fireEvent.dragStart(acceptableColumn); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - const filterConfigPopup = screen.getByTestId('filter-edit-popover'); - expect(within(filterConfigPopup).getByText('column_c')).toBeInTheDocument(); - }); -}); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx index 37fbe7210f8..2bb1c515934 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx @@ -454,6 +454,8 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { accept={DND_ACCEPTED_TYPES} ghostButtonText={t('Drop columns/metrics here or click')} onClickGhostButton={handleClickGhostButton} + sortableType={DndItemType.FilterOption} + itemCount={values.length} {...props} /> ({ + ...jest.requireActual('@dnd-kit/core'), + useDroppable: jest.fn(), +})); + +jest.mock('@dnd-kit/sortable', () => ({ + ...jest.requireActual('@dnd-kit/sortable'), + useSortable: jest.fn(), +})); + +beforeEach(() => { + captured.current = undefined; + sortables.items = []; + (useDroppable as jest.Mock).mockImplementation( + captureDroppableData(captured), + ); + (useSortable as jest.Mock).mockImplementation(captureSortableData(sortables)); +}); const defaultProps = { savedMetrics: [ @@ -70,7 +101,7 @@ const adhocMetricB = { test('renders with default props', () => { render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect( @@ -80,7 +111,7 @@ test('renders with default props', () => { test('renders with default props and multi = true', () => { render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect( @@ -91,7 +122,7 @@ test('renders with default props and multi = true', () => { test('render selected metrics correctly', () => { const metricValues = ['metric_a', 'metric_b', adhocMetricB]; render(, { - useDnd: true, + useDndKit: true, useRedux: true, }); expect(screen.getByText('metric_a')).toBeVisible(); @@ -113,7 +144,7 @@ test('warn selected custom metric when metric gets removed from dataset', async multi />, { - useDnd: true, + useDndKit: true, useRedux: true, }, ); @@ -166,7 +197,7 @@ test('warn selected custom metric when metric gets removed from dataset for sing multi={false} />, { - useDnd: true, + useDndKit: true, useRedux: true, }, ); @@ -225,7 +256,7 @@ test('remove selected adhoc metric when column gets removed from dataset', async multi />, { - useDnd: true, + useDndKit: true, useRedux: true, }, ); @@ -268,7 +299,7 @@ test('update adhoc metric name when column label in dataset changes', () => { multi />, { - useDnd: true, + useDndKit: true, useRedux: true, }, ); @@ -311,156 +342,107 @@ test('update adhoc metric name when column label in dataset changes', () => { expect(screen.getByText('SUM(new col B name)')).toBeVisible(); }); -test('can drag metrics', async () => { - const metricValues = ['metric_a', 'metric_b', adhocMetricB]; - render(, { - useDnd: true, - useRedux: true, - }); +// Drop behavior is exercised through `resolveDragEnd` (the production drag-end +// dispatcher) because @dnd-kit's PointerSensor needs real layout that jsdom +// cannot provide. See ./dndTestUtils and ExploreDndContext.test.tsx. - expect(screen.getByText('metric_a')).toBeVisible(); - expect(screen.getByText('Metric B')).toBeVisible(); +test('can drag metrics (reorder dispatches through the reorder + drop path)', () => { + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, + ); - const container = screen.getByTestId('dnd-labels-container'); - expect(container.childElementCount).toBe(4); + // DndMetricSelect reorders via moveLabel (internal state) finalized by + // onDropLabel. Verify both callbacks were registered on the sortable items + // and the drag-end path invokes them (which commits the change via onChange). + expect(sortables.items.length).toBeGreaterThanOrEqual(3); + expect(typeof sortables.items[0].onMoveLabel).toBe('function'); + expect(typeof sortables.items[0].onDropLabel).toBe('function'); - const firstMetric = container.children[0] as HTMLElement; - const lastMetric = container.children[2] as HTMLElement; - expect(within(firstMetric).getByText('metric_a')).toBeVisible(); - expect(within(lastMetric).getByText('SUM(Column B)')).toBeVisible(); - - fireEvent.mouseOver(within(firstMetric).getByText('metric_a')); - expect(await screen.findByText('Metric name')).toBeInTheDocument(); - - fireEvent.dragStart(firstMetric); - fireEvent.dragEnter(lastMetric); - fireEvent.dragOver(lastMetric); - fireEvent.drop(lastMetric); - - expect(within(firstMetric).getByText('SUM(Column B)')).toBeVisible(); - expect(within(lastMetric).getByText('metric_a')).toBeVisible(); + simulateReorder(sortables, 0, 2); + expect(onChange).toHaveBeenCalled(); }); test('cannot drop a duplicated item', () => { - const metricValues = ['metric_a']; - const { getByTestId } = render( - <> - - - , - { - useDnd: true, - useRedux: true, - }, + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, ); - const acceptableMetric = getByTestId('DatasourcePanelDragOption'); - const currentMetric = getByTestId('dnd-labels-container'); + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_a' } as any, + }); - const currentMetricSelection = currentMetric.children.length; - - fireEvent.dragStart(acceptableMetric); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - expect(currentMetric.children).toHaveLength(currentMetricSelection); - expect(currentMetric).toHaveTextContent('metric_a'); + expect(onChange).not.toHaveBeenCalled(); }); test('can drop a saved metric when disallow_adhoc_metrics', () => { - const metricValues = ['metric_b']; - const { getByTestId } = render( - <> - - - , - { - useDnd: true, - useRedux: true, - }, + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, ); - const acceptableMetric = getByTestId('DatasourcePanelDragOption'); - const currentMetric = getByTestId('dnd-labels-container'); + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_a' } as any, + }); - const currentMetricSelection = currentMetric.children.length; - - fireEvent.dragStart(acceptableMetric); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - expect(currentMetric.children).toHaveLength(currentMetricSelection + 1); - expect(currentMetric.children[1]).toHaveTextContent('metric_a'); + expect(onChange).toHaveBeenLastCalledWith(['metric_b', 'metric_a']); }); test('cannot drop non-saved metrics when disallow_adhoc_metrics', () => { - const metricValues = ['metric_b']; - const { getByTestId, getAllByTestId } = render( - <> - - - - - , - { - useDnd: true, - useRedux: true, - }, + const onChange = jest.fn(); + render( + , + { useDndKit: true, useRedux: true }, ); - const selections = getAllByTestId('DatasourcePanelDragOption'); - const acceptableMetric = selections[0]; - const unacceptableMetric = selections[1]; - const unacceptableType = selections[2]; - const currentMetric = getByTestId('dnd-labels-container'); + // Non-saved metric -> rejected. + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_c' } as any, + }); + expect(onChange).not.toHaveBeenCalled(); - const currentMetricSelection = currentMetric.children.length; + // Column type -> rejected when adhoc metrics are disallowed. + simulateDrop(captured, { + type: DndItemType.Column, + value: { column_name: 'column_a' } as any, + }); + expect(onChange).not.toHaveBeenCalled(); - fireEvent.dragStart(unacceptableMetric); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - expect(currentMetric.children).toHaveLength(currentMetricSelection); - expect(currentMetric).not.toHaveTextContent('metric_c'); - - fireEvent.dragStart(unacceptableType); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - expect(currentMetric.children).toHaveLength(currentMetricSelection); - expect(currentMetric).not.toHaveTextContent('column_1'); - - fireEvent.dragStart(acceptableMetric); - fireEvent.dragOver(currentMetric); - fireEvent.drop(currentMetric); - - expect(currentMetric.children).toHaveLength(currentMetricSelection + 1); - expect(currentMetric).toHaveTextContent('metric_a'); + // Saved metric -> accepted. + simulateDrop(captured, { + type: DndItemType.Metric, + value: { metric_name: 'metric_a' } as any, + }); + expect(onChange).toHaveBeenLastCalledWith(['metric_b', 'metric_a']); }); test('title changes on custom SQL text change', async () => { @@ -477,7 +459,7 @@ test('title changes on custom SQL text change', async () => { multi />, { - useDnd: true, + useDndKit: true, useRedux: true, }, ); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx index 988eb302da5..5bd4eb5d4c1 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx @@ -408,6 +408,9 @@ const DndMetricSelect = (props: any) => { multi ? 2 : 1, ); + // Generate sortable type that matches MetricDefinitionValue's type + const sortableType = `${DndItemType.AdhocMetricOption}_${props.name}_${props.label}`; + return (
{ ghostButtonText={ghostButtonText} displayGhostButton={multi || value.length === 0} onClickGhostButton={handleClickGhostButton} + sortableType={sortableType} + itemCount={value.length} {...props} /> { }; test('renders with default props', () => { - render(, { useDnd: true }); + render(, { useDndKit: true }); expect(screen.getByText('Drop columns here or click')).toBeInTheDocument(); }); @@ -60,7 +60,7 @@ test('renders ghost button when empty', () => { const ghostButtonText = 'Ghost button text'; render( , - { useDnd: true }, + { useDndKit: true }, ); expect(screen.getByText(ghostButtonText)).toBeInTheDocument(); }); @@ -69,13 +69,13 @@ test('renders values', () => { const values = 'Values'; const valuesRenderer = () => {values}; render(, { - useDnd: true, + useDndKit: true, }); expect(screen.getByText(values)).toBeInTheDocument(); }); test('Handles ghost button click', () => { - render(, { useDnd: true }); + render(, { useDndKit: true }); userEvent.click(screen.getByText('Drop columns here or click')); expect(defaultProps.onClickGhostButton).toHaveBeenCalled(); }); @@ -86,7 +86,6 @@ test('updates dropValidator on changes', () => { , - { useDnd: true }, ); expect(getByTestId(`mock-result-${defaultProps.name}`)).toHaveTextContent( 'false', diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx index ba8ee28b9f0..e63df5bfae2 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx @@ -17,7 +17,11 @@ * under the License. */ import { ReactNode, useCallback, useContext, useEffect, useMemo } from 'react'; -import { useDrop } from 'react-dnd'; +import { useDroppable } from '@dnd-kit/core'; +import { + SortableContext, + verticalListSortingStrategy, +} from '@dnd-kit/sortable'; import { t } from '@apache-superset/core/translation'; import ControlHeader from 'src/explore/components/ControlHeader'; import { @@ -45,6 +49,9 @@ export type DndSelectLabelProps = { displayGhostButton?: boolean; onClickGhostButton: () => void; isLoading?: boolean; + // For sortable items - the type string and count to generate sortable IDs + sortableType?: string; + itemCount?: number; }; export default function DndSelectLabel({ @@ -52,35 +59,49 @@ export default function DndSelectLabel({ accept, valuesRenderer, isLoading, + sortableType, + itemCount = 0, ...props }: DndSelectLabelProps) { const canDropProp = props.canDrop; const canDropValueProp = props.canDropValue; + const acceptTypes = useMemo( + () => (Array.isArray(accept) ? accept : [accept]), + [accept], + ); + const dropValidator = useCallback( (item: DatasourcePanelDndItem) => canDropProp(item) && (canDropValueProp?.(item.value) ?? true), [canDropProp, canDropValueProp], ); - const [{ isOver, canDrop }, datasourcePanelDrop] = useDrop({ - accept: isLoading ? [] : accept, - - drop: (item: DatasourcePanelDndItem) => { - props.onDrop(item); - props.onDropValue?.(item.value); + const { setNodeRef, isOver, active } = useDroppable({ + id: `dropzone-${props.name}`, + disabled: isLoading, + data: { + accept: acceptTypes, + // Mirrors react-dnd's `canDrop`: the drop only fires when this returns + // true, so duplicate/selection gating is preserved post-migration. + canDrop: dropValidator, + onDrop: props.onDrop, + onDropValue: props.onDropValue, }, - - canDrop: dropValidator, - - collect: monitor => ({ - isOver: monitor.isOver(), - canDrop: monitor.canDrop(), - type: monitor.getItemType(), - }), }); - const dispatch = useContext(DropzoneContext)[1]; + // Check if the active dragged item can be dropped here + const canDrop = useMemo(() => { + if (!active?.data.current) return false; + const activeData = active.data.current as { type: string; value: unknown }; + if (!acceptTypes.includes(activeData.type as DndItemType)) return false; + return dropValidator({ + type: activeData.type as DndItemType, + value: activeData.value as DndItemValue, + }); + }, [active, acceptTypes, dropValidator]); + + const [, dispatch] = useContext(DropzoneContext); useEffect(() => { dispatch({ key: props.name, canDrop: dropValidator }); @@ -93,6 +114,15 @@ export default function DndSelectLabel({ const values = useMemo(() => valuesRenderer(), [valuesRenderer]); + // Generate sortable item IDs for SortableContext + const sortableItemIds = useMemo(() => { + if (!sortableType || itemCount === 0) return []; + return Array.from( + { length: itemCount }, + (_, i) => `sortable-${sortableType}-${i}`, + ); + }, [sortableType, itemCount]); + function renderGhostButton() { return ( { + if (sortableItemIds.length > 0) { + return ( + + {values} + + ); + } + return values; + }; + return ( -
+
@@ -117,7 +164,7 @@ export default function DndSelectLabel({ isDragging={isDragging} isLoading={isLoading} > - {values} + {renderSortableValues()} {displayGhostButton && renderGhostButton()}
diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx index 3d1a5a9d785..519cf3e5741 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { render, screen, fireEvent } from 'spec/helpers/testing-library'; +import { render, screen } from 'spec/helpers/testing-library'; import { DndItemType } from 'src/explore/components/DndItemType'; import OptionWrapper from 'src/explore/components/controls/DndColumnSelectControl/OptionWrapper'; @@ -29,35 +29,66 @@ test('renders with default props', async () => { onShiftOptions={jest.fn()} label="Option" />, - { useDnd: true }, + { useDndKit: true }, ); expect(container).toBeInTheDocument(); expect(await screen.findByRole('img', { name: 'close' })).toBeInTheDocument(); }); -test('triggers onShiftOptions on drop', async () => { - const onShiftOptions = jest.fn(); +test('renders label correctly', async () => { + render( + , + { useDndKit: true }, + ); + + expect(await screen.findByText('Test Label')).toBeInTheDocument(); +}); + +test('renders multiple options', async () => { render( <> + - , - { useDnd: true }, + { useDndKit: true }, ); - fireEvent.dragStart(await screen.findByText('Option 1')); - fireEvent.drop(await screen.findByText('Option 2')); - expect(onShiftOptions).toHaveBeenCalled(); + expect(await screen.findByText('Option 1')).toBeInTheDocument(); + expect(await screen.findByText('Option 2')).toBeInTheDocument(); +}); + +test('calls clickClose when close button is clicked', async () => { + const clickClose = jest.fn(); + render( + , + { useDndKit: true }, + ); + + const closeButton = await screen.findByRole('img', { name: 'close' }); + closeButton.click(); + expect(clickClose).toHaveBeenCalledWith(1); }); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx index 787638269cf..e962d0e3aee 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx @@ -16,13 +16,9 @@ * specific language governing permissions and limitations * under the License. */ -import { useRef } from 'react'; -import { - useDrag, - useDrop, - DropTargetMonitor, - DragSourceMonitor, -} from 'react-dnd'; +import { useRef, useMemo } from 'react'; +import { useSortable } from '@dnd-kit/sortable'; +import { CSS } from '@dnd-kit/utilities'; import { DragContainer } from 'src/explore/components/controls/OptionControls'; import { OptionProps, @@ -64,62 +60,32 @@ export default function OptionWrapper( multiValueWarningMessage, ...rest } = props; - const ref = useRef(null); const labelRef = useRef(null); - const [{ isDragging }, drag] = useDrag({ - item: { + // Create a unique sortable ID for this item + const sortableId = useMemo(() => `sortable-${type}-${index}`, [type, index]); + + const { + attributes, + listeners, + setNodeRef, + transform, + transition, + isDragging, + } = useSortable({ + id: sortableId, + data: { type, dragIndex: index, - }, - collect: (monitor: DragSourceMonitor) => ({ - isDragging: monitor.isDragging(), - }), + onShiftOptions, + } as OptionItemInterface & { onShiftOptions: typeof onShiftOptions }, }); - const [, drop] = useDrop({ - accept: type, - - hover: (item: OptionItemInterface, monitor: DropTargetMonitor) => { - if (!ref.current) { - return; - } - const { dragIndex } = item; - const hoverIndex = index; - - // Don't replace items with themselves - if (dragIndex === hoverIndex) { - return; - } - // Determine rectangle on screen - const hoverBoundingRect = ref.current?.getBoundingClientRect(); - // Get vertical middle - const hoverMiddleY = - (hoverBoundingRect.bottom - hoverBoundingRect.top) / 2; - // Determine mouse position - const clientOffset = monitor.getClientOffset(); - // Get pixels to the top - const hoverClientY = clientOffset - ? clientOffset.y - hoverBoundingRect.top - : 0; - // Only perform the move when the mouse has crossed half of the items height - // When dragging downwards, only move when the cursor is below 50% - // When dragging upwards, only move when the cursor is above 50% - // Dragging downwards - if (dragIndex < hoverIndex && hoverClientY < hoverMiddleY) { - return; - } - // Dragging upwards - if (dragIndex > hoverIndex && hoverClientY > hoverMiddleY) { - return; - } - - // Time to actually perform the action - onShiftOptions(dragIndex, hoverIndex); - // eslint-disable-next-line no-param-reassign - item.dragIndex = hoverIndex; - }, - }); + const style = { + transform: CSS.Transform.toString(transform), + transition, + opacity: isDragging ? 0.5 : 1, + }; const shouldShowTooltip = (!isDragging && tooltipTitle && label && tooltipTitle !== label) || @@ -179,10 +145,14 @@ export default function OptionWrapper( return null; }; - drag(drop(ref)); - return ( - +