[Bug Fix] Items can be dragged from the Trash into the Trash (#46389)

* prevents users from moving items to the trash if they are in the trash already and archives instead of moves in cases where item is moved to or from the trash

* reverts detecting if dragging in or out of the trash not setting the collection id, this will be prevented on the BE instead

* adds test coverage for dragging and dropping items to/from/within the trash (one of which is failing until we can make a BE change)

* Throw away `collection_id` when marking archived

This is a hack around a frontend issue. Apparently, the undo
functionality depends on calculating a diff between the current state
and the previous state. Sometimes this results in the frontend telling
us to *both* mark an item as archived *and* "move" it to the Trash.

Let's just say that if you're marking something as archived, we throw
away any `collection_id` you passed in along with it.

---------

Co-authored-by: John Swanson <john.swanson@metabase.com>

e2e/test/scenarios/collections/trash.cy.spec.js

@@ -5,6 +5,8 @@ import {
   ORDERS_QUESTION_ID,
 } from "e2e/support/cypress_sample_instance_data";
 import {
+  undo,
+  main,
   popover,
   createNativeQuestion as _createNativeQuestion,
   selectSidebarItem,
@@ -691,6 +693,76 @@ describe("scenarios > collections > trash", () => {
     });
   });
 
+  describe("sidebar drag and drop", () => {
+    it("should not allow items in the trash to be moved into the trash", () => {
+      createDashboard({ name: "Dashboard A" }, true);
+      cy.intercept("PUT", "/api/dashboard/**").as("updateDashboard");
+      cy.visit("/trash");
+
+      dragAndDrop(
+        main().findByText("Dashboard A"),
+        navigationSidebar().findByText("Trash"),
+      );
+
+      cy.wait(100); // small wait to make sure a network request could have gone out
+      // assert no update request went out
+      cy.get("@updateDashboard.all").should("have.length", 0);
+      cy.findByTestId("toast-undo").should("not.exist");
+      main(() => {
+        cy.findByText(/Deleted items will appear here/).should("not.exist");
+        cy.findByText("Dashboard A").should("exist");
+      });
+    });
+
+    it("should allow items in the trash to be moved out of the trash and allow it to be undone", () => {
+      createDashboard({ name: "Dashboard A" }, true);
+      cy.intercept("PUT", "/api/dashboard/**").as("updateDashboard");
+      cy.visit("/trash");
+
+      dragAndDrop(
+        main().findByText("Dashboard A"),
+        navigationSidebar().findByText("First collection"),
+      );
+
+      cy.get("@updateDashboard.all").should("have.length", 1);
+      main()
+        .findByText(/Deleted items will appear here/)
+        .should("exist");
+      cy.findByTestId("toast-undo").should("exist");
+      undo();
+
+      cy.get("@updateDashboard.all").should("have.length", 2);
+      main().within(() => {
+        cy.findByText(/Deleted items will appear here/).should("not.exist");
+        cy.findByText("Dashboard A").should("exist");
+      });
+    });
+
+    it("should allow items outside the trash to be moved in the trash and allow it to be undone", () => {
+      createDashboard({
+        name: "Dashboard A",
+        collection_id: FIRST_COLLECTION_ID,
+      });
+      cy.intercept("PUT", "/api/dashboard/**").as("updateDashboard");
+      visitCollection(FIRST_COLLECTION_ID);
+
+      dragAndDrop(
+        main().findByText("Dashboard A"),
+        navigationSidebar().findByText("Trash"),
+      );
+
+      cy.get("@updateDashboard.all").should("have.length", 1);
+      main().findByText("Dashboard A").should("not.exist");
+      cy.findByTestId("toast-undo").should("exist");
+      undo();
+
+      cy.get("@updateDashboard.all").should("have.length", 2);
+      main().within(() => {
+        cy.findByText("Dashboard A").should("exist");
+      });
+    });
+  });
+
   it("should open only one context menu at a time (metabase#44910)", () => {
     cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { archived: true });
     cy.request("PUT", `/api/card/${ORDERS_COUNT_QUESTION_ID}`, {
@@ -806,3 +878,10 @@ function ensureBookmarkVisible(bookmark) {
     .findByText(bookmark)
     .should("be.visible");
 }
+
+function dragAndDrop(subjectEl, targetEl) {
+  const dataTransfer = new DataTransfer();
+  subjectEl.trigger("dragstart", { dataTransfer });
+  targetEl.trigger("drop", { dataTransfer });
+  subjectEl.trigger("dragend");
+}

frontend/src/metabase/containers/dnd/CollectionDropTarget.jsx

 import { DropTarget } from "react-dnd";
 
-import { canonicalCollectionId } from "metabase/collections/utils";
+import {
+  canonicalCollectionId,
+  isRootTrashCollection,
+} from "metabase/collections/utils";
 
 import DropArea from "./DropArea";
 
@@ -18,10 +21,16 @@ const CollectionDropTarget = DropTarget(
       if (collection.can_write === false) {
         return false;
       }
+      const droppingToTrashFromTrash =
+        isRootTrashCollection(collection) && item.archived;
       const droppingToSameCollection =
         canonicalCollectionId(item.collection_id) ===
         canonicalCollectionId(collection.id);
-      return item.model !== "collection" && !droppingToSameCollection;
+      return (
+        item.model !== "collection" &&
+        !droppingToSameCollection &&
+        !droppingToTrashFromTrash
+      );
     },
   },
   (connect, monitor) => ({

src/metabase/api/common.clj

@@ -705,7 +705,17 @@
   [current-obj obj-updates]
   (cond-> obj-updates
     (column-will-change? :archived current-obj obj-updates)
-    (assoc :archived_directly (boolean (:archived obj-updates)))))
+    (assoc :archived_directly (boolean (:archived obj-updates)))
+
+    ;; This is a hack around a frontend issue. Apparently, the undo functionality depends on calculating a diff
+    ;; between the current state and the previous state. Sometimes this results in the frontend telling us to
+    ;; *both* mark an item as archived *and* "move" it to the Trash.
+    ;;
+    ;; Let's just say that if you're marking something as archived, we throw away any `collection_id` you passed in
+    ;; along with it.
+    (and (column-will-change? :archived current-obj obj-updates)
+         (:archived obj-updates))
+    (dissoc :collection_id)))
 
 (defn present-in-trash-if-archived-directly
   "If `:archived_directly` is `true`, set `:collection_id` to the trash collection ID."