diff --git a/e2e/support/helpers/e2e-dragndrop-helpers.js b/e2e/support/helpers/e2e-dragndrop-helpers.js index 9870436bf1848b93db489770e74b01d2a552d696..d608115fc9062df8d6e33efe8a24fbaf42315416 100644 --- a/e2e/support/helpers/e2e-dragndrop-helpers.js +++ b/e2e/support/helpers/e2e-dragndrop-helpers.js @@ -37,3 +37,11 @@ export function dragField(startIndex, dropIndex) { }); }); } + +export function dragAndDrop(subjectAlias, targetAlias) { + const dataTransfer = new DataTransfer(); + + cy.get("@" + subjectAlias).trigger("dragstart", { dataTransfer }); + cy.get("@" + targetAlias).trigger("drop", { dataTransfer }); + cy.get("@" + subjectAlias).trigger("dragend"); +} diff --git a/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js b/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js index 4008c525835c39fb96d02571e297f1c57a25b441..94a4b3c39e4ca8f2c1ac61f0b011af9f7326ebc2 100644 --- a/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js +++ b/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js @@ -1,6 +1,7 @@ import { popover, restore, + dragAndDrop, getPinnedSection, openPinnedItemMenu, openUnpinnedItemMenu, @@ -208,6 +209,38 @@ describe("scenarios > collection pinned items overview", () => { cy.findByText("A question").should("be.visible"); }); }); + + it("should be able to pin a visualization by dragging it up", () => { + cy.request("PUT", "/api/card/2", { + collection_position: 1, + collection_preview: false, + }); + openRootCollection(); + + cy.findByTestId("collection-table") + .findByText("Orders, Count, Grouped by Created At (year)") + .as("draggingViz"); + + cy.findByTestId("pinned-items").as("pinnedItems"); + + // this test can give us some degree of confidence, but its effectiveness is limited + // because we are manually firing events on the correct elements. It doesn't seem that there's + // a way to actually simulate the raw user interaction of dragging a certain distance in cypress. + // this will not guarantee that the drag and drop functionality will work in the real world, e.g + // when our various drag + drop libraries start interfering with events on one another. + // for example, this test would not have caught https://github.com/metabase/metabase/issues/30614 + // even libraries like https://github.com/dmtrKovalenko/cypress-real-events rely on firing events + // on specific elements rather than truly simulating mouse movements across the screen + dragAndDrop("draggingViz", "pinnedItems"); + + cy.findByTestId("collection-table") + .findByText("Orders, Count, Grouped by Created At (year)") + .should("not.exist"); + + cy.findByTestId("pinned-items") + .findByText("Orders, Count, Grouped by Created At (year)") + .should("exist"); + }); }); const openRootCollection = () => { diff --git a/e2e/test/scenarios/collections/collections.cy.spec.js b/e2e/test/scenarios/collections/collections.cy.spec.js index 95836e921d979413880b9f0b3cb8d45f8ee5ad5f..32b567367db955dc6cd442562e1d8bd48ed1ed5e 100644 --- a/e2e/test/scenarios/collections/collections.cy.spec.js +++ b/e2e/test/scenarios/collections/collections.cy.spec.js @@ -11,6 +11,7 @@ import { closeNavigationSidebar, openCollectionMenu, visitCollection, + dragAndDrop, openUnpinnedItemMenu, getPinnedSection, } from "e2e/support/helpers"; @@ -662,14 +663,6 @@ function moveOpenedCollectionTo(newParent) { modal().should("not.exist"); } -function dragAndDrop(subjectAlias, targetAlias) { - const dataTransfer = new DataTransfer(); - - cy.get("@" + subjectAlias).trigger("dragstart", { dataTransfer }); - cy.get("@" + targetAlias).trigger("drop", { dataTransfer }); - cy.get("@" + subjectAlias).trigger("dragend"); -} - function moveItemToCollection(itemName, collectionName) { cy.request("GET", "/api/collection/root/items").then(resp => { const ALL_ITEMS = resp.body.data; diff --git a/frontend/src/metabase/collections/containers/CollectionContent.jsx b/frontend/src/metabase/collections/containers/CollectionContent.jsx index cbe2b71f50ee1f50b15ce703bcebb579e23140ca..2f9afaf790b60535babb35e5df8a29ac4c99262f 100644 --- a/frontend/src/metabase/collections/containers/CollectionContent.jsx +++ b/frontend/src/metabase/collections/containers/CollectionContent.jsx @@ -32,6 +32,7 @@ import { isSmallScreen } from "metabase/lib/dom"; import Databases from "metabase/entities/databases"; import UploadOverlay from "../components/UploadOverlay"; +import { getComposedDragProps } from "./utils"; import { CollectionEmptyContent, @@ -201,7 +202,7 @@ function CollectionContent({ const canUpload = uploadsEnabled && collection.can_write; - const dropzoneProps = canUpload ? getRootProps() : {}; + const dropzoneProps = canUpload ? getComposedDragProps(getRootProps()) : {}; const unpinnedQuery = { collection: collectionId, diff --git a/frontend/src/metabase/collections/containers/utils.ts b/frontend/src/metabase/collections/containers/utils.ts new file mode 100644 index 0000000000000000000000000000000000000000..792a2bcb3f1a8f7bb27f7f55c87027873f3b2a6a --- /dev/null +++ b/frontend/src/metabase/collections/containers/utils.ts @@ -0,0 +1,23 @@ +import type { DropzoneRootProps } from "react-dropzone"; +import type { DragEventHandler, DragEvent } from "react"; + +export const composeFileEventHandler = + (fn: DragEventHandler<HTMLElement> | undefined) => + (event: DragEvent<HTMLElement>) => { + if (!event?.dataTransfer?.types.includes("Files")) { + return; + } + fn?.(event); + }; + +export const getComposedDragProps = ( + props: DropzoneRootProps, +): DropzoneRootProps => { + return { + ...props, + onDragEnter: composeFileEventHandler(props.onDragEnter), + onDragLeave: composeFileEventHandler(props.onDragLeave), + onDragOver: composeFileEventHandler(props.onDragOver), + onDrop: composeFileEventHandler(props.onDrop), + }; +}; diff --git a/frontend/src/metabase/collections/containers/utils.unit.spec.ts b/frontend/src/metabase/collections/containers/utils.unit.spec.ts new file mode 100644 index 0000000000000000000000000000000000000000..1233ca46ec944fee9a5715cb3cf6159660974cac --- /dev/null +++ b/frontend/src/metabase/collections/containers/utils.unit.spec.ts @@ -0,0 +1,56 @@ +import type { DropzoneRootProps } from "react-dropzone"; +import type { DragEvent } from "react"; +import { getComposedDragProps, composeFileEventHandler } from "./utils"; +describe("Collections > containers > utils", () => { + const testNonFileEvent = { + dataTransfer: { + types: ["text/plain"], + }, + } as unknown as DragEvent<HTMLElement>; + + const testFileEvent = { + dataTransfer: { + types: ["Files"], + }, + } as unknown as DragEvent<HTMLElement>; + + describe("getComposedDragProps", () => { + it("should compose all drag event handlers to ignore non-file events", () => { + const dragEventSpy = jest.fn(); + const nonDragEventSpy = jest.fn(); + + const mockProps = { + onDragEnter: dragEventSpy, + onDragLeave: dragEventSpy, + onDragOver: dragEventSpy, + onDrop: dragEventSpy, + onClick: nonDragEventSpy, + } as DropzoneRootProps; + + const composedProps = getComposedDragProps(mockProps); + + composedProps.onDragEnter?.(testNonFileEvent); + composedProps.onDragLeave?.(testNonFileEvent); + composedProps.onDragOver?.(testNonFileEvent); + composedProps.onDrop?.(testNonFileEvent); + // this non-drag handler should not get composed + composedProps.onClick?.(testNonFileEvent); + + expect(dragEventSpy).not.toHaveBeenCalled(); + expect(nonDragEventSpy).toHaveBeenCalledTimes(1); + }); + }); + + describe("composeFileEventHandler", () => { + it("should return a function that ignores non-file drag events", () => { + const testFn = jest.fn(); + + const composedFn = composeFileEventHandler(testFn); + + composedFn(testNonFileEvent); + expect(testFn).not.toHaveBeenCalled(); + composedFn(testFileEvent); + expect(testFn).toHaveBeenCalledTimes(1); + }); + }); +});