Add tests

superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx

@@ -90,6 +90,10 @@ const DragDroppableStyles = styled.div`
  opacity: 0.2;
  }
 
+ &.dragdroppable--dragging .dragdroppable-tab {
+ display: none;
+ }
+
  &.dragdroppable-row {
  width: 100%;
  }

superset-frontend/src/dashboard/components/dnd/DragDroppable.test.jsx

@@ -23,7 +23,11 @@ import {
 import sinon from 'sinon';
 
 import newComponentFactory from 'src/dashboard/util/newComponentFactory';
-import { CHART_TYPE, ROW_TYPE } from 'src/dashboard/util/componentTypes';
+import {
+ CHART_TYPE,
+ ROW_TYPE,
+ TAB_TYPE,
+} from 'src/dashboard/util/componentTypes';
 import { UnwrappedDragDroppable as DragDroppable } from 'src/dashboard/components/dnd/DragDroppable';
 
 describe('DragDroppable', () => {
@@ -63,6 +67,16 @@ describe('DragDroppable', () => {
  expect(childrenSpy.callCount).toBe(1);
  });
 
+ it('should call onDropIndicatorChange when isDraggingOver changes', () => {
+ const onDropIndicatorChange = sinon.spy();
+ const wrapper = setup({
+ onDropIndicatorChange,
+ component: newComponentFactory(TAB_TYPE),
+ });
+ wrapper.setProps({ isDraggingOver: true });
+ expect(onDropIndicatorChange.callCount).toBe(1);
+ });
+
  it('should call its child function with "dragSourceRef" if editMode=true', () => {
  const children = sinon.spy();
  const dragSourceRef = () => {};

superset-frontend/src/dashboard/components/gridComponents/Tab.jsx

@@ -78,7 +78,6 @@ const defaultProps = {
 };
 
 const TabTitleContainer = styled.div`
- // TODO remove tab title container from visible if dragging it in edit mode
  ${({ isHighlighted, theme: { gridUnit, colors } }) => `
  padding: ${gridUnit}px ${gridUnit * 2}px;
  margin: ${-gridUnit}px ${gridUnit * -2}px;
@@ -108,7 +107,6 @@ class Tab extends PureComponent {
  this.handleOnHover = this.handleOnHover.bind(this);
  this.handleTopDropTargetDrop = this.handleTopDropTargetDrop.bind(this);
  this.handleChangeTab = this.handleChangeTab.bind(this);
- // this.handleGetDropPosition = this.handleGetDropPosition.bind(this);
  }
 
  handleChangeTab({ pathToTabIndex }) {
@@ -319,7 +317,10 @@ class Tab extends PureComponent {
  />
  )}
  {dropIndicatorProps && (
- <TitleDropIndicator className={dropIndicatorProps.className} />
+ <TitleDropIndicator
+ className={dropIndicatorProps.className}
+ data-test="title-drop-indicator"
+ />
  )}
  </TabTitleContainer>
  )}

superset-frontend/src/dashboard/components/gridComponents/Tab.test.tsx

@@ -196,6 +196,10 @@ test('Drop on a tab', async () => {
  );
 
  fireEvent.dragStart(screen.getByText('Dashboard Component'));
+ fireEvent.dragOver(screen.getByText('Next Tab'));
+ await waitFor(() =>
+ expect(screen.getByTestId('title-drop-indicator')).toBeVisible(),
+ );
  fireEvent.drop(screen.getByText('Next Tab'));
  await waitFor(() => expect(mockOnDropOnTab).toHaveBeenCalledTimes(2));
  expect(mockOnDropOnTab).toHaveBeenLastCalledWith(

superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx

@@ -105,12 +105,6 @@ const StyledTabsContainer = styled.div`
  }
  }
 
- .ant-tabs-tab {
- &:has(.dragdroppable--dragging) {
- display: none;
- }
- }
-
  div .ant-tabs-tab-btn {
  text-transform: none;
  }
@@ -127,7 +121,7 @@ const RightDropIndicator = styled.div`
  position: absolute;
  top: 0;
  right: -4px;
- border-radius: 4px;
+ border-radius: 2px;
 `;
 const LeftDropIndicator = styled.div`
  border: 2px solid ${({ theme }) => theme.colors.primary.base};
@@ -136,7 +130,7 @@ const LeftDropIndicator = styled.div`
  position: absolute;
  top: 0;
  left: -4px;
- border-radius: 4px;
+ border-radius: 2px;
 `;
 
 const CloseIconWithDropIndicator = props => (
@@ -329,17 +323,8 @@ export class Tabs extends PureComponent {
 
  handleGetDropPosition(dragObject) {
  const { dropIndicator, isDraggingOver, index } = dragObject;
- const { dragOverTabIndex } = this.state;
 
  if (isDraggingOver) {
- console.log(
- 'dropIndicator',
- dropIndicator,
- 'dragOverTabIndex',
- dragOverTabIndex,
- 'index',
- index,
- );
  this.setState(() => ({
  dropPosition: dropIndicator,
  dragOverTabIndex: index,
@@ -445,7 +430,6 @@ export class Tabs extends PureComponent {
  type={editMode ? 'editable-card' : 'card'}
  >
  {tabIds.map((tabId, tabIndex) => (
- // Add droppable component to each tab
  <LineEditableTabs.TabPane
  key={tabId}
  tab={

superset-frontend/src/dashboard/components/gridComponents/Tabs.test.tsx

@@ -122,21 +122,24 @@ test('Should render editMode:true', () => {
  const props = createProps();
  render(<Tabs {...props} />, { useRedux: true, useDnd: true });
  expect(screen.getAllByRole('tab')).toHaveLength(3);
- expect(Draggable).toBeCalledTimes(1);
- expect(DashboardComponent).toBeCalledTimes(4);
- expect(DeleteComponentButton).toBeCalledTimes(1);
  expect(screen.getAllByRole('button', { name: 'remove' })).toHaveLength(3);
  expect(screen.getAllByRole('button', { name: 'Add tab' })).toHaveLength(2);
+ expect(Draggable).toHaveBeenCalledTimes(1);
+ expect(DashboardComponent).toHaveBeenCalledTimes(4);
+ expect(DeleteComponentButton).toHaveBeenCalledTimes(1);
 });
 
 test('Should render editMode:false', () => {
  const props = createProps();
  props.editMode = false;
- render(<Tabs {...props} />, { useRedux: true, useDnd: true });
+ render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
  expect(screen.getAllByRole('tab')).toHaveLength(3);
- expect(Draggable).toBeCalledTimes(1);
- expect(DashboardComponent).toBeCalledTimes(4);
- expect(DeleteComponentButton).not.toBeCalled();
+ expect(Draggable).toHaveBeenCalledTimes(1);
+ expect(DashboardComponent).toHaveBeenCalledTimes(4);
+ expect(DeleteComponentButton).not.toHaveBeenCalled();
  expect(
  screen.queryByRole('button', { name: 'remove' }),
  ).not.toBeInTheDocument();
@@ -153,11 +156,11 @@ test('Update component props', () => {
  useRedux: true,
  useDnd: true,
  });
- expect(DeleteComponentButton).not.toBeCalled();
+ expect(DeleteComponentButton).not.toHaveBeenCalled();
 
  props.editMode = true;
  rerender(<Tabs {...props} />);
- expect(DeleteComponentButton).toBeCalledTimes(1);
+ expect(DeleteComponentButton).toHaveBeenCalledTimes(1);
 });
 
 test('Clicking on "DeleteComponentButton"', () => {
@@ -167,9 +170,12 @@ test('Clicking on "DeleteComponentButton"', () => {
  useDnd: true,
  });
 
- expect(props.deleteComponent).not.toBeCalled();
+ expect(props.deleteComponent).not.toHaveBeenCalled();
  userEvent.click(screen.getByTestId('DeleteComponentButton'));
- expect(props.deleteComponent).toBeCalledWith('TABS-L-d9eyOE-b', 'GRID_ID');
+ expect(props.deleteComponent).toHaveBeenCalledWith(
+ 'TABS-L-d9eyOE-b',
+ 'GRID_ID',
+ );
 });
 
 test('Add new tab', () => {
@@ -179,9 +185,9 @@ test('Add new tab', () => {
  useDnd: true,
  });
 
- expect(props.createComponent).not.toBeCalled();
+ expect(props.createComponent).not.toHaveBeenCalled();
  userEvent.click(screen.getAllByRole('button', { name: 'Add tab' })[0]);
- expect(props.createComponent).toBeCalled();
+ expect(props.createComponent).toHaveBeenCalled();
 });
 
 test('Removing a tab', async () => {
@@ -191,16 +197,16 @@ test('Removing a tab', async () => {
  useDnd: true,
  });
 
- expect(props.deleteComponent).not.toBeCalled();
+ expect(props.deleteComponent).not.toHaveBeenCalled();
  expect(screen.queryByText('Delete dashboard tab?')).not.toBeInTheDocument();
  userEvent.click(screen.getAllByRole('button', { name: 'remove' })[0]);
- expect(props.deleteComponent).not.toBeCalled();
+ expect(props.deleteComponent).not.toHaveBeenCalled();
 
  expect(await screen.findByText('Delete dashboard tab?')).toBeInTheDocument();
 
- expect(props.deleteComponent).not.toBeCalled();
+ expect(props.deleteComponent).not.toHaveBeenCalled();
  userEvent.click(screen.getByRole('button', { name: 'DELETE' }));
- expect(props.deleteComponent).toBeCalled();
+ expect(props.deleteComponent).toHaveBeenCalled();
 });
 
 test('Switching tabs', () => {
@@ -210,11 +216,11 @@ test('Switching tabs', () => {
  useDnd: true,
  });
 
- expect(props.logEvent).not.toBeCalled();
- expect(props.onChangeTab).not.toBeCalled();
+ expect(props.logEvent).not.toHaveBeenCalled();
+ expect(props.onChangeTab).not.toHaveBeenCalled();
  userEvent.click(screen.getAllByRole('tab')[2]);
- expect(props.logEvent).toBeCalled();
- expect(props.onChangeTab).toBeCalled();
+ expect(props.logEvent).toHaveBeenCalled();
+ expect(props.onChangeTab).toHaveBeenCalled();
 });
 
 test('Call "DashboardComponent.onDropOnTab"', async () => {
@@ -224,12 +230,12 @@ test('Call "DashboardComponent.onDropOnTab"', async () => {
  useDnd: true,
  });
 
- expect(props.logEvent).not.toBeCalled();
- expect(props.onChangeTab).not.toBeCalled();
+ expect(props.logEvent).not.toHaveBeenCalled();
+ expect(props.onChangeTab).not.toHaveBeenCalled();
  userEvent.click(screen.getAllByText('DashboardComponent')[0]);
 
  await waitFor(() => {
- expect(props.logEvent).toBeCalled();
- expect(props.onChangeTab).toBeCalled();
+ expect(props.logEvent).toHaveBeenCalled();
+ expect(props.onChangeTab).toHaveBeenCalled();
  });
 });

superset-frontend/src/dashboard/util/dnd-reorder.test.js

@@ -17,6 +17,8 @@
  * under the License.
  */
 import reorderItem from 'src/dashboard/util/dnd-reorder';
+import { TABS_TYPE } from './componentTypes';
+import { DROP_LEFT, DROP_RIGHT } from './getDropPosition';
 
 describe('dnd-reorderItem', () => {
  it('should remove the item from its source entity and add it to its destination entity', () => {
@@ -72,6 +74,121 @@ describe('dnd-reorderItem', () => {
  destination: { id: 'b', index: 1 },
  });
 
- expect(result.iAmExtra === extraEntity).toBe(true);
+ expect(result.iAmExtra).toBe(extraEntity);
+ });
+
+ it('should handle out of bounds destination index gracefully', () => {
+ const result = reorderItem({
+ entitiesMap: {
+ a: {
+ id: 'a',
+ children: ['x', 'y', 'z'],
+ },
+ b: {
+ id: 'b',
+ children: ['banana'],
+ },
+ },
+ source: { id: 'a', index: 1 },
+ destination: { id: 'b', index: 5 },
+ });
+
+ expect(result.a.children).toEqual(['x', 'z']);
+ expect(result.b.children).toEqual(['banana', 'y']);
+ });
+
+ it('should do nothing if source and destination are the same and indices are the same', () => {
+ const result = reorderItem({
+ entitiesMap: {
+ a: {
+ id: 'a',
+ children: ['x', 'y', 'z'],
+ },
+ },
+ source: { id: 'a', index: 1 },
+ destination: { id: 'a', index: 1 },
+ });
+
+ expect(result.a.children).toEqual(['x', 'y', 'z']);
+ });
+
+ it('should handle DROP_LEFT in the same TABS_TYPE list correctly', () => {
+ const result = reorderItem({
+ entitiesMap: {
+ a: {
+ id: 'a',
+ type: TABS_TYPE,
+ children: ['x', 'y', 'z'],
+ },
+ },
+ source: { id: 'a', type: TABS_TYPE, index: 2 },
+ destination: { id: 'a', type: TABS_TYPE, index: 1 },
+ position: DROP_LEFT,
+ });
+
+ expect(result.a.children).toEqual(['x', 'z', 'y']);
+ });
+
+ it('should handle DROP_RIGHT in the same TABS_TYPE list correctly', () => {
+ const result = reorderItem({
+ entitiesMap: {
+ a: {
+ id: 'a',
+ type: TABS_TYPE,
+ children: ['x', 'y', 'z'],
+ },
+ },
+ source: { id: 'a', type: TABS_TYPE, index: 0 },
+ destination: { id: 'a', type: TABS_TYPE, index: 1 },
+ position: DROP_RIGHT,
+ });
+
+ expect(result.a.children).toEqual(['y', 'x', 'z']);
+ });
+
+ it('should handle DROP_LEFT when moving between different TABS_TYPE lists', () => {
+ const result = reorderItem({
+ entitiesMap: {
+ a: {
+ id: 'a',
+ type: TABS_TYPE,
+ children: ['x', 'y'],
+ },
+ b: {
+ id: 'b',
+ type: TABS_TYPE,
+ children: ['banana'],
+ },
+ },
+ source: { id: 'a', type: TABS_TYPE, index: 1 },
+ destination: { id: 'b', type: TABS_TYPE, index: 0 },
+ position: DROP_LEFT,
+ });
+
+ expect(result.a.children).toEqual(['x']);
+ expect(result.b.children).toEqual(['y', 'banana']);
+ });
+
+ it('should handle DROP_RIGHT when moving between different TABS_TYPE lists', () => {
+ const result = reorderItem({
+ entitiesMap: {
+ a: {
+ id: 'a',
+ type: TABS_TYPE,
+ children: ['x', 'y'],
+ },
+ b: {
+ id: 'b',
+ type: TABS_TYPE,
+ children: ['banana'],
+ },
+ },
+ source: { id: 'a', type: TABS_TYPE, index: 0 },
+ destination: { id: 'b', type: TABS_TYPE, index: 0 },
+ position: DROP_RIGHT,
+ });
+
+ expect(result.a.children).toEqual(['y']);
+ expect(result.b.children).toEqual(['banana', 'x']);
  });
 });